From 7ead12d572e7ced5025222bf3df3e0365d352bfc Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 1 Aug 2026 21:43:34 -0400 Subject: [PATCH] fix: Derive traversal node IDs from the branch actually descended `belowHelper` built each stack entry's `SHAMapNodeID` from `branch`, the branch used to reach the subtree root, rather than `childBranch`, the branch it had just descended. The resulting IDs carried a correct depth but named a different subtree, and nothing rejected them: such an ID has a legal depth and a legal mask, so only comparing it against an actual leaf key exposes the mismatch. The affected stacks feed read-only traversals whose consumers use only the depth, so no ledger state, hash, or peer message was affected, but any future consumer of `getNodeID()` would have silently received the wrong position. Rather than fix the one call, make the mistake unrepresentable. `NodePathStack` replaces the bare `std::stack` and refuses to accept an ID at all: every push takes the branch being descended and derives the ID itself, so a node and its ID cannot disagree. `isPrefixOf` assertions on each push catch a wrong branch at the point it happens rather than wherever the ID is later read. Leaf entries now keep the depth they were reached at instead of a normalized `kLeafDepth`, which is what lets those assertions hold: `addGiveItem` splits a leaf from the depth it actually sits at. The new traversal tests fail on the previous code: reverting the branch derivation trips the leaf-key assertion on the first iteration. Also adds a `deepFanOutKeysAtLeafDepth` helper and mirrors them against it, since the existing `deepFanOutKeys`'s fan-out at the 6th nibble keeps its tree only about 6 levels deep and never exercised the depth-63/64 code these tests are meant to protect, plus a case that collapses the entire depth-63 chain of single-child inner nodes into a leaf on the final delete, which the every-other-key deletion pattern the other new tests use never triggers. --- include/xrpl/shamap/SHAMap.h | 139 ++++++++--- src/libxrpl/shamap/SHAMap.cpp | 142 +++++------ src/libxrpl/shamap/SHAMapSync.cpp | 2 +- src/tests/libxrpl/shamap/SHAMap.cpp | 361 ++++++++++++++++++++++++++++ 4 files changed, 533 insertions(+), 111 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 05de33ddf3..c223c772a0 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -420,7 +420,103 @@ public: invariants() const; private: - using SharedPtrNodeStack = std::stack>; + /** + * A path from the root of the map down to some node, pairing each node with the ID naming its + * position. + * + * The two halves of an entry must agree, and the only way to get that wrong is to compute an ID + * from the wrong branch. So this type does not accept an ID at all: every push takes the branch + * being descended and derives the ID itself, so a node and its ID cannot disagree. Reads are + * exposed through the same accessors a std::stack would offer. + */ + class NodePathStack + { + public: + [[nodiscard]] bool + empty() const + { + return stack_.empty(); + } + + [[nodiscard]] std::size_t + size() const + { + return stack_.size(); + } + + [[nodiscard]] std::pair const& + top() const + { + XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::top : non-empty stack"); + return stack_.top(); + } + + void + pop() + { + XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::pop : non-empty stack"); + stack_.pop(); + } + + void + clear() + { + stack_ = {}; + } + + /** + * Start a path at the root of the map, whose ID is the zero-depth ID by definition. + */ + void + pushRoot(SHAMapTreeNodePtr node) + { + XRPL_ASSERT(stack_.empty(), "xrpl::SHAMap::NodePathStack::pushRoot : empty stack"); + stack_.emplace(std::move(node), SHAMapNodeID{}); + } + + /** + * Extend the path to the child of the current node reached by `branch`. + * + * A node keeps the depth it was reached at, never a normalized kLeafDepth. Only a leaf may + * sit at kLeafDepth, since an inner node there would have no branch left to select. + */ + void + pushChild(SHAMapTreeNodePtr node, unsigned int branch) + { + XRPL_ASSERT(node, "xrpl::SHAMap::NodePathStack::pushChild : non-null node input"); + XRPL_ASSERT( + !stack_.empty(), "xrpl::SHAMap::NodePathStack::pushChild : non-empty stack"); + auto childID = stack_.top().second.getChildNodeID(branch); + XRPL_ASSERT_IF( + node->isInner(), + childID.getDepth() < kLeafDepth, + "xrpl::SHAMap::NodePathStack::pushChild : inner node above leaf depth"); + XRPL_ASSERT_IF( + node->isLeaf(), + childID.isPrefixOf(leafKey(*node)), + "xrpl::SHAMap::NodePathStack::pushChild : leaf key below branch"); + stack_.emplace(std::move(node), std::move(childID)); + } + + /** + * Extend the path to a node lying on the path to `target`. + * + * For nodes not reached by descending a known branch: the walk tracks only the key it is + * heading for, or the node is newly created. Either way `target` selects the branch. + */ + void + pushNode(SHAMapTreeNodePtr node, uint256 const& target) + { + if (stack_.empty()) + pushRoot(std::move(node)); + else + pushChild(std::move(node), selectBranch(stack_.top().second, target)); + } + + private: + std::stack> stack_; + }; + using DeltaRef = std::pair, boost::intrusive_ptr>; @@ -447,7 +543,7 @@ private: * Update hashes up to the root */ void - dirtyUp(SharedPtrNodeStack& stack, uint256 const& target, SHAMapTreeNodePtr terminal); + dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr terminal); /** * Walk towards the specified id, returning the node. Caller must check @@ -455,7 +551,7 @@ private: * id */ SHAMapLeafNode* - walkTowardsKey(uint256 const& id, SharedPtrNodeStack* stack = nullptr) const; + walkTowardsKey(uint256 const& id, NodePathStack* stack = nullptr) const; /** * Return nullptr if key not found */ @@ -482,27 +578,15 @@ private: SHAMapTreeNodePtr writeNode(NodeObjectType t, SHAMapTreeNodePtr node) const; - // returns the first item at or below this node - SHAMapLeafNode* - firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch = 0u) const; - - // returns the last item at or below this node - SHAMapLeafNode* - lastBelow( - SHAMapTreeNodePtr node, - SharedPtrNodeStack& stack, - unsigned int branch = kBranchFactor) const; - - // direction in which belowHelper scans an inner node's branches + // direction in which a scan walks an inner node's branches enum class BelowDirection { First, Last }; - // helper function for firstBelow and lastBelow + /** + * Returns the first or last item at or below the node already on top of `stack`, extending + * `stack` with the path walked to reach it. + */ SHAMapLeafNode* - belowHelper( - SHAMapTreeNodePtr node, - SharedPtrNodeStack& stack, - unsigned int branch, - BelowDirection direction) const; + belowHelper(NodePathStack& stack, BelowDirection direction) const; // Simple descent // Get a child of the specified node @@ -550,9 +634,9 @@ private: hasLeafNode(uint256 const& tag, SHAMapHash const& hash) const; SHAMapLeafNode const* - peekFirstItem(SharedPtrNodeStack& stack) const; + peekFirstItem(NodePathStack& stack) const; SHAMapLeafNode const* - peekNextItem(uint256 const& id, SharedPtrNodeStack& stack) const; + peekNextItem(uint256 const& id, NodePathStack& stack) const; bool walkBranch( SHAMapTreeNode* node, @@ -697,7 +781,7 @@ public: using pointer = value_type const*; private: - SharedPtrNodeStack stack_; + NodePathStack stack_; SHAMap const* map_ = nullptr; pointer item_ = nullptr; @@ -723,7 +807,7 @@ public: private: explicit ConstIterator(SHAMap const* map); ConstIterator(SHAMap const* map, std::nullptr_t); - ConstIterator(SHAMap const* map, pointer item, SharedPtrNodeStack&& stack); + ConstIterator(SHAMap const* map, pointer item, NodePathStack&& stack); friend bool operator==(ConstIterator const& x, ConstIterator const& y); @@ -742,10 +826,7 @@ inline SHAMap::ConstIterator::ConstIterator(SHAMap const* map, std::nullptr_t) : { } -inline SHAMap::ConstIterator::ConstIterator( - SHAMap const* map, - pointer item, - SharedPtrNodeStack&& stack) +inline SHAMap::ConstIterator::ConstIterator(SHAMap const* map, pointer item, NodePathStack&& stack) : stack_(std::move(stack)), map_(map), item_(item) { } diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 3fa8d66be0..7acff483be 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -97,7 +97,7 @@ SHAMap::snapShot(bool isMutable) const } void -SHAMap::dirtyUp(SharedPtrNodeStack& stack, uint256 const& target, SHAMapTreeNodePtr child) +SHAMap::dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr child) { // walk the tree up from through the inner nodes to the root_ // update hashes and links @@ -126,29 +126,34 @@ SHAMap::dirtyUp(SharedPtrNodeStack& stack, uint256 const& target, SHAMapTreeNode } SHAMapLeafNode* -SHAMap::walkTowardsKey(uint256 const& id, SharedPtrNodeStack* stack) const +SHAMap::walkTowardsKey(uint256 const& id, NodePathStack* stack) const { XRPL_ASSERT( stack == nullptr || stack->empty(), "xrpl::SHAMap::walkTowardsKey : empty stack input"); auto inNode = root_; SHAMapNodeID nodeID; + // Every node on this walk lies on the path to `id`, so the stack can derive each ID from the + // branch `id` selects at the node above it. + auto pushCurrent = [&] { + if (stack != nullptr) + stack->pushNode(inNode, id); + }; + while (inNode->isInner()) { - if (stack != nullptr) - stack->emplace(inNode, nodeID); + pushCurrent(); - auto const inner = intr_ptr::staticPointerCast(inNode); + auto& inner = safeDowncast(*inNode); auto const branch = selectBranch(nodeID, id); - if (inner->isEmptyBranch(branch)) + if (inner.isEmptyBranch(branch)) return nullptr; - inNode = descendThrow(*inner, branch); + inNode = descendThrow(inner, branch); nodeID = nodeID.getChildNodeID(branch); } - if (stack != nullptr) - stack->emplace(inNode, nodeID); + pushCurrent(); return safeDowncast(inNode.get()); } @@ -428,65 +433,40 @@ SHAMap::unshareNode(intr_ptr::SharedPtr node, SHAMapNodeID const& nodeID) } SHAMapLeafNode* -SHAMap::belowHelper( - SHAMapTreeNodePtr node, - SharedPtrNodeStack& stack, - unsigned int branch, - BelowDirection direction) const +SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const { - if (node->isLeaf()) - { - auto n = intr_ptr::staticPointerCast(node); - stack.push({node, {kLeafDepth, n->peekItem()->key()}}); - return n.get(); - } - auto inner = intr_ptr::staticPointerCast(node); - if (stack.empty()) - { - stack.emplace(inner, SHAMapNodeID{}); - } - else - { - stack.emplace(inner, stack.top().second.getChildNodeID(branch)); - } - // `scanned` counts how many branches of `inner` we have examined; the branch we look at is - // derived from it, so no index ever goes out of range. + XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack input"); + if (auto const& top = stack.top().first; top->isLeaf()) + return safeDowncast(top.get()); + + // The stack owns the node/ID pairing, so descending is only ever "push the branch we took". + // `scanned` counts how many branches of the current node we have examined; the branch we look + // at is derived from it, so no index ever goes out of range. `inner` tracks the node on top of + // the stack, which keeps it alive, so it only needs recomputing after a push. + auto* inner = safeDowncast(stack.top().first.get()); for (auto scanned = 0u; scanned < kBranchFactor;) { auto const childBranch = (direction == BelowDirection::Last) ? (kBranchFactor - 1u - scanned) : scanned; - if (!inner->isEmptyBranch(childBranch)) - { - node.adopt(descendThrow(inner.get(), childBranch)); - XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack"); - if (node->isLeaf()) - { - auto n = intr_ptr::staticPointerCast(node); - stack.push({n, {kLeafDepth, n->peekItem()->key()}}); - return n.get(); - } - inner = intr_ptr::staticPointerCast(node); - stack.emplace(inner, stack.top().second.getChildNodeID(branch)); - scanned = 0u; // descend and restart the scan on the new node - } - else + if (inner->isEmptyBranch(childBranch)) { ++scanned; // scan next branch + continue; } + + stack.pushChild(descendThrow(*inner, childBranch), childBranch); + + auto const& child = stack.top().first; + if (child->isLeaf()) + return safeDowncast(child.get()); + + inner = safeDowncast(child.get()); + scanned = 0u; // descend and restart the scan on the new node } return nullptr; } -SHAMapLeafNode* -SHAMap::lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch) const -{ - return belowHelper(node, stack, branch, BelowDirection::Last); -} -SHAMapLeafNode* -SHAMap::firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch) const -{ - return belowHelper(node, stack, branch, BelowDirection::First); -} + static boost::intrusive_ptr const kNoItem; boost::intrusive_ptr const& @@ -529,36 +509,36 @@ SHAMap::onlyBelow(SHAMapTreeNode* node) const } SHAMapLeafNode const* -SHAMap::peekFirstItem(SharedPtrNodeStack& stack) const +SHAMap::peekFirstItem(NodePathStack& stack) const { XRPL_ASSERT(stack.empty(), "xrpl::SHAMap::peekFirstItem : empty stack input"); - SHAMapLeafNode const* node = firstBelow(root_, stack); + stack.pushRoot(root_); + SHAMapLeafNode const* node = belowHelper(stack, BelowDirection::First); if (node == nullptr) { - while (!stack.empty()) - stack.pop(); + stack.clear(); return nullptr; } return node; } SHAMapLeafNode const* -SHAMap::peekNextItem(uint256 const& id, SharedPtrNodeStack& stack) const +SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::peekNextItem : non-empty stack input"); XRPL_ASSERT(stack.top().first->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf"); stack.pop(); while (!stack.empty()) { - auto [node, nodeID] = stack.top(); + auto const [node, nodeID] = stack.top(); XRPL_ASSERT(!node->isLeaf(), "xrpl::SHAMap::peekNextItem : another node is not leaf"); - auto inner = intr_ptr::staticPointerCast(node); + auto& inner = safeDowncast(*node); for (auto i = selectBranch(nodeID, id) + 1; i < kBranchFactor; ++i) { - if (!inner->isEmptyBranch(i)) + if (!inner.isEmptyBranch(i)) { - node = descendThrow(*inner, i); - auto leaf = firstBelow(node, stack, i); + stack.pushChild(descendThrow(inner, i), i); + auto leaf = belowHelper(stack, BelowDirection::First); if (leaf == nullptr) Throw(type_, id); XRPL_ASSERT(leaf->isLeaf(), "xrpl::SHAMap::peekNextItem : leaf is valid"); @@ -597,7 +577,7 @@ SHAMap::peekItem(uint256 const& id, SHAMapHash& hash) const SHAMap::ConstIterator SHAMap::upperBound(uint256 const& id) const { - SharedPtrNodeStack stack; + NodePathStack stack; walkTowardsKey(id, &stack); while (!stack.empty()) { @@ -610,13 +590,13 @@ SHAMap::upperBound(uint256 const& id) const } else { - auto inner = intr_ptr::staticPointerCast(node); + auto& inner = safeDowncast(*node); for (auto branch = selectBranch(nodeID, id) + 1; branch < kBranchFactor; ++branch) { - if (!inner->isEmptyBranch(branch)) + if (!inner.isEmptyBranch(branch)) { - node = descendThrow(*inner, branch); - auto leaf = firstBelow(node, stack, branch); + stack.pushChild(descendThrow(inner, branch), branch); + auto leaf = belowHelper(stack, BelowDirection::First); if (leaf == nullptr) Throw(type_, id); return ConstIterator(this, leaf->peekItem().get(), std::move(stack)); @@ -630,7 +610,7 @@ SHAMap::upperBound(uint256 const& id) const SHAMap::ConstIterator SHAMap::lowerBound(uint256 const& id) const { - SharedPtrNodeStack stack; + NodePathStack stack; walkTowardsKey(id, &stack); while (!stack.empty()) { @@ -643,14 +623,14 @@ SHAMap::lowerBound(uint256 const& id) const } else { - auto inner = intr_ptr::staticPointerCast(node); + auto& inner = safeDowncast(*node); for (auto branch = selectBranch(nodeID, id); branch > 0u;) { --branch; - if (!inner->isEmptyBranch(branch)) + if (!inner.isEmptyBranch(branch)) { - node = descendThrow(*inner, branch); - auto leaf = lastBelow(node, stack, branch); + stack.pushChild(descendThrow(inner, branch), branch); + auto leaf = belowHelper(stack, BelowDirection::Last); if (leaf == nullptr) Throw(type_, id); return ConstIterator(this, leaf->peekItem().get(), std::move(stack)); @@ -675,7 +655,7 @@ SHAMap::delItem(uint256 const& id) // delete the item with this ID XRPL_ASSERT(state_ != SHAMapState::Immutable, "xrpl::SHAMap::delItem : not immutable"); - SharedPtrNodeStack stack; + NodePathStack stack; walkTowardsKey(id, &stack); if (stack.empty()) @@ -761,7 +741,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr // add the specified item, does not update uint256 const tag = item->key(); - SharedPtrNodeStack stack; + NodePathStack stack; walkTowardsKey(tag, &stack); if (stack.empty()) @@ -801,7 +781,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key()))) { - stack.emplace(node, nodeID); + stack.pushNode(node, tag); // we need a new inner node, since both go on same branch at this // level @@ -848,7 +828,7 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptrisLeaf(), "xrpl::SHAMap::invariants : root node is not leaf"); - SharedPtrNodeStack stack; + NodePathStack stack; for (auto leaf = peekFirstItem(stack); leaf != nullptr; leaf = peekNextItem(leaf->peekItem()->key(), stack)) ; diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 8849bbda14..6b171e794d 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -790,7 +790,7 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const std::optional> SHAMap::getProofPath(uint256 const& key) const { - SharedPtrNodeStack stack; + NodePathStack stack; walkTowardsKey(key, &stack); if (stack.empty()) diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index 92492c514e..7742515dd8 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -272,6 +272,367 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Values(kBackedMode, kUnbackedMode), shamapBackingModeName); +// Exercises the traversal stacks built by firstBelow/lastBelow. Each stack entry pairs a node with +// the ID naming its position, and SHAMap asserts that pairing on every push, so these traversals +// fail loudly in a Debug build if a node ID is ever derived from the wrong branch. +class SHAMapTraversal : public ::testing::Test +{ +protected: + beast::Journal const j_{TestSink::instance()}; + + // Keys that share a long prefix and then fan out across distinct branches, so the deeper inner + // nodes have several children and traversal must descend many levels. + static std::vector + deepFanOutKeys() + { + std::vector keys; + for (unsigned int branch = 0; branch < SHAMap::kBranchFactor; ++branch) + { + // Vary the 6th nibble, keeping the first five identical. + auto text = std::string("abcde") + "0123456789abcdef"[branch]; + text.append(64 - text.size(), '7'); + keys.emplace_back(std::string_view{text}); + } + return keys; + } + + // Keys that share all 63 leading nibbles and fan out only at the last one, so the tree is a + // chain of single-child inner nodes down to depth 63 with the leaves as siblings at depth 64. + // This exercises kLeafDepth directly, unlike deepFanOutKeys() above, whose fan-out at the 6th + // nibble keeps the tree only about 6 levels deep. + static std::vector + deepFanOutKeysAtLeafDepth() + { + std::vector keys; + for (unsigned int branch = 0; branch < SHAMap::kBranchFactor; ++branch) + { + auto text = std::string(63, 'a') + "0123456789abcdef"[branch]; + keys.emplace_back(std::string_view{text}); + } + return keys; + } + + static void + fillMap(SHAMap& map, std::vector const& keys) + { + map.setUnbacked(); + for (auto const& k : keys) + { + Buffer vuc{32}; + std::fill_n(vuc.data(), vuc.size(), std::uint8_t{1}); + map.addItem(SHAMapNodeType::TnAccountState, makeShamapitem(k, std::move(vuc))); + map.invariants(); + } + } +}; + +TEST_F(SHAMapTraversal, forward_iteration_visits_every_key_in_order) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + + std::sort(keys.begin(), keys.end()); + std::vector visited; + for (auto const& item : map) + visited.push_back(item.key()); + + EXPECT_EQ(visited, keys); +} + +TEST_F(SHAMapTraversal, upper_bound_walks_the_whole_map) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // upperBound from each key must land on its successor, driving firstBelow across every subtree. + for (std::size_t k = 0; k + 1 < keys.size(); ++k) + { + auto it = map.upperBound(keys[k]); + ASSERT_NE(it, map.end()) << "no successor for key " << k; + EXPECT_EQ(it->key(), keys[k + 1]) << "wrong successor for key " << k; + } + EXPECT_EQ(map.upperBound(keys.back()), map.end()); +} + +TEST_F(SHAMapTraversal, lower_bound_walks_the_whole_map) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // lowerBound is the lastBelow counterpart: it descends to the greatest key below a subtree. + for (std::size_t k = 1; k < keys.size(); ++k) + { + auto it = map.lowerBound(keys[k]); + ASSERT_NE(it, map.end()) << "no predecessor for key " << k; + EXPECT_EQ(it->key(), keys[k - 1]) << "wrong predecessor for key " << k; + } + EXPECT_EQ(map.lowerBound(keys.front()), map.end()); +} + +TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // Probe keys that are not in the map, so the traversal starts mid-tree rather than at a leaf. + for (unsigned char c : {0x00, 0x40, 0x80, 0xc0, 0xff}) + { + uint256 probe; + std::fill_n(probe.begin(), probe.size(), c); + + auto const expectedUpper = std::upper_bound(keys.begin(), keys.end(), probe); + auto const upper = map.upperBound(probe); + if (expectedUpper == keys.end()) + { + EXPECT_EQ(upper, map.end()) << "probe " << static_cast(c); + } + else + { + ASSERT_NE(upper, map.end()) << "probe " << static_cast(c); + EXPECT_EQ(upper->key(), *expectedUpper) << "probe " << static_cast(c); + } + + auto const lowerCount = std::lower_bound(keys.begin(), keys.end(), probe) - keys.begin(); + auto const lower = map.lowerBound(probe); + if (lowerCount == 0) + { + EXPECT_EQ(lower, map.end()) << "probe " << static_cast(c); + } + else + { + ASSERT_NE(lower, map.end()) << "probe " << static_cast(c); + EXPECT_EQ(lower->key(), keys[lowerCount - 1]) << "probe " << static_cast(c); + } + } +} + +TEST_F(SHAMapTraversal, iteration_survives_deletions) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // Deleting every other key drops the fan-out node's branch count from 16 to 8, never the 1 + // that would make delItem collapse it into a leaf. So this pins that iteration survives + // deletions that reshape the map without collapsing any inner node; the case that does + // collapse one is iteration_survives_a_collapsed_inner_node below. + for (std::size_t k = 0; k < keys.size(); k += 2) + { + ASSERT_TRUE(map.delItem(keys[k])); + map.invariants(); + } + + std::vector expected; + for (std::size_t k = 1; k < keys.size(); k += 2) + expected.push_back(keys[k]); + + std::vector visited; + for (auto const& item : map) + visited.push_back(item.key()); + EXPECT_EQ(visited, expected); + + for (std::size_t k = 0; k + 1 < expected.size(); ++k) + { + auto it = map.upperBound(expected[k]); + ASSERT_NE(it, map.end()); + EXPECT_EQ(it->key(), expected[k + 1]); + } +} + +TEST_F(SHAMapTraversal, iteration_survives_a_collapsed_inner_node) +{ + tests::TestNodeFamily f{j_}; + SHAMap map{SHAMapType::FREE, f}; + + // One key in a separate subtree, diverging from the fan-out group at the very first nibble, so + // it survives untouched while the fan-out group below is collapsed. + auto const sentinel = uint256{std::string_view{std::string(64, '0')}}; + + auto fanOutKeys = deepFanOutKeysAtLeafDepth(); + fillMap(map, fanOutKeys); + Buffer vuc{32}; + std::fill_n(vuc.data(), vuc.size(), std::uint8_t{1}); + ASSERT_TRUE( + map.addItem(SHAMapNodeType::TnAccountState, makeShamapitem(sentinel, std::move(vuc)))); + map.invariants(); + + std::sort(fanOutKeys.begin(), fanOutKeys.end()); + + // Delete all but the last fan-out key. The fan-out node's branch count drops to 1 on the final + // delete, which delItem collapses by pulling the sole remaining leaf up in its place; every + // ancestor above it has exactly one child by construction, so each of those also drops to + // branch count 1 and collapses in turn, all the way up to (but not including) the root. That + // final delete replaces the entire 63-level chain with the root pointing straight at the one + // remaining leaf, so the surviving traversal stack is rebuilt over a drastically different tree + // shape, not just missing one inner node. + for (std::size_t k = 0; k + 1 < fanOutKeys.size(); ++k) + { + ASSERT_TRUE(map.delItem(fanOutKeys[k])); + map.invariants(); + } + + std::vector const expected{sentinel, fanOutKeys.back()}; + std::vector visited; + for (auto const& item : map) + visited.push_back(item.key()); + EXPECT_EQ(visited, expected); + + auto it = map.upperBound(sentinel); + ASSERT_NE(it, map.end()); + EXPECT_EQ(it->key(), fanOutKeys.back()); + EXPECT_EQ(map.upperBound(fanOutKeys.back()), map.end()); +} + +// The tests below mirror the ones above but use deepFanOutKeysAtLeafDepth(), whose keys share all +// 63 leading nibbles and fan out only at the last one. That puts the leaves at depth +// SHAMap::kLeafDepth, so these traversals walk a chain of single-child inner nodes all the way down +// and exercise the kLeafDepth guards that deepFanOutKeys() alone (fanning out at the 6th nibble) +// never reaches. + +TEST_F(SHAMapTraversal, forward_iteration_visits_every_key_in_order_at_leaf_depth) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeysAtLeafDepth(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + + std::sort(keys.begin(), keys.end()); + std::vector visited; + for (auto const& item : map) + visited.push_back(item.key()); + + EXPECT_EQ(visited, keys); +} + +TEST_F(SHAMapTraversal, upper_bound_walks_the_whole_map_at_leaf_depth) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeysAtLeafDepth(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // upperBound from each key must land on its successor, driving firstBelow down to depth + // kLeafDepth for every subtree. + for (std::size_t k = 0; k + 1 < keys.size(); ++k) + { + auto it = map.upperBound(keys[k]); + ASSERT_NE(it, map.end()) << "no successor for key " << k; + EXPECT_EQ(it->key(), keys[k + 1]) << "wrong successor for key " << k; + } + EXPECT_EQ(map.upperBound(keys.back()), map.end()); +} + +TEST_F(SHAMapTraversal, lower_bound_walks_the_whole_map_at_leaf_depth) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeysAtLeafDepth(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // lowerBound is the lastBelow counterpart: it descends to depth kLeafDepth to find the greatest + // key below a subtree. + for (std::size_t k = 1; k < keys.size(); ++k) + { + auto it = map.lowerBound(keys[k]); + ASSERT_NE(it, map.end()) << "no predecessor for key " << k; + EXPECT_EQ(it->key(), keys[k - 1]) << "wrong predecessor for key " << k; + } + EXPECT_EQ(map.lowerBound(keys.front()), map.end()); +} + +TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys_at_leaf_depth) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeysAtLeafDepth(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // The keys fill all 16 branches of the last nibble, so an absent key must diverge from the + // shared 'a' prefix earlier than that. Diverging at increasingly deep nibbles forces + // walkTowardsKey to descend through more single-child inner nodes before it finds the empty + // branch, right up to the one just above kLeafDepth. + for (unsigned int divergeAt : {0u, 31u, 61u, 62u}) + { + auto text = std::string(divergeAt, 'a') + "b"; + text.append(64 - text.size(), '0'); + uint256 const probe{std::string_view{text}}; + + auto const expectedUpper = std::upper_bound(keys.begin(), keys.end(), probe); + auto const upper = map.upperBound(probe); + if (expectedUpper == keys.end()) + { + EXPECT_EQ(upper, map.end()) << "divergeAt " << divergeAt; + } + else + { + ASSERT_NE(upper, map.end()) << "divergeAt " << divergeAt; + EXPECT_EQ(upper->key(), *expectedUpper) << "divergeAt " << divergeAt; + } + + auto const lowerCount = std::lower_bound(keys.begin(), keys.end(), probe) - keys.begin(); + auto const lower = map.lowerBound(probe); + if (lowerCount == 0) + { + EXPECT_EQ(lower, map.end()) << "divergeAt " << divergeAt; + } + else + { + ASSERT_NE(lower, map.end()) << "divergeAt " << divergeAt; + EXPECT_EQ(lower->key(), keys[lowerCount - 1]) << "divergeAt " << divergeAt; + } + } +} + +TEST_F(SHAMapTraversal, iteration_survives_deletions_at_leaf_depth) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeysAtLeafDepth(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + std::sort(keys.begin(), keys.end()); + + // Deleting every other key drops the fan-out node's branch count from 16 to 8, the same + // non-collapsing case as iteration_survives_deletions above, but reached by descending through + // a chain of single-child inner nodes down to kLeafDepth instead of a shallow one. + for (std::size_t k = 0; k < keys.size(); k += 2) + { + ASSERT_TRUE(map.delItem(keys[k])); + map.invariants(); + } + + std::vector expected; + for (std::size_t k = 1; k < keys.size(); k += 2) + expected.push_back(keys[k]); + + std::vector visited; + for (auto const& item : map) + visited.push_back(item.key()); + EXPECT_EQ(visited, expected); + + for (std::size_t k = 0; k + 1 < expected.size(); ++k) + { + auto it = map.upperBound(expected[k]); + ASSERT_NE(it, map.end()); + EXPECT_EQ(it->key(), expected[k + 1]); + } +} + class SHAMapPathProof : public ::testing::Test { protected: