From 0f0b8fc6506102d69e1e711d0611de954d44d617 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 1 Aug 2026 21:48:41 -0400 Subject: [PATCH] refactor: Unify `upperBound` and `lowerBound` into `boundHelper` The two functions were near duplicates: walk to the key, then look for the nearest leaf on one side. Only the scan direction, the comparison deciding a leaf qualifies, and whether to take the first or last leaf below the subtree differed, exactly the distinction `BelowDirection` already draws for `belowHelper`, so the pair collapse into one parameterised walk. Also drops the stale `// TODO: what to return here?` above `lowerBound`'s `return end()`: no predecessor is the correct answer for the smallest key, and the tests pin it. Existing coverage only exercised `boundHelper`'s inner-node branch, every test map had at least three items, so the root was always an inner node and the leaf branch at the top of the function was never reached with a real answer to give. Adds coverage for a single-item map, where the leaf branch alone decides the outcome, and an empty map, where the scan must find nothing on every branch before falling through to `end()`. Fixes the single-item test's own comment, which claimed `root_` becomes a leaf, when in fact `root_` stays the inner node it was constructed with for any map built via `addItem`; only a single-item map synced from a peer (`addRootNode`) ever replaces `root_` with a leaf directly. --- include/xrpl/shamap/SHAMap.h | 4 ++ src/libxrpl/shamap/SHAMap.cpp | 77 ++++++++++++----------------- src/tests/libxrpl/shamap/SHAMap.cpp | 44 +++++++++++++++++ 3 files changed, 80 insertions(+), 45 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index c223c772a0..42d37cce93 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -588,6 +588,10 @@ private: SHAMapLeafNode* belowHelper(NodePathStack& stack, BelowDirection direction) const; + // helper function for upperBound and lowerBound + ConstIterator + boundHelper(uint256 const& id, BelowDirection direction) const; + // Simple descent // Get a child of the specified node SHAMapTreeNode* diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 7acff483be..137f9d32a2 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -575,72 +575,59 @@ SHAMap::peekItem(uint256 const& id, SHAMapHash& hash) const } SHAMap::ConstIterator -SHAMap::upperBound(uint256 const& id) const +SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const { + // Walk back up the path to `id` looking for the nearest leaf on the requested side. At each + // inner node the branches beyond the one `id` takes hold the candidates; the first non-empty + // one is the closest, and the extreme leaf below it is the answer. + auto const searchingForward = direction == BelowDirection::First; + NodePathStack stack; walkTowardsKey(id, &stack); while (!stack.empty()) { - auto [node, nodeID] = stack.top(); + auto const [node, nodeID] = stack.top(); if (node->isLeaf()) { - auto leaf = safeDowncast(node.get()); - if (leaf->peekItem()->key() > id) - return ConstIterator(this, leaf->peekItem().get(), std::move(stack)); + auto const& item = safeDowncast(*node).peekItem(); + if (searchingForward ? (item->key() > id) : (item->key() < id)) + return ConstIterator(this, item.get(), std::move(stack)); } else { auto& inner = safeDowncast(*node); - for (auto branch = selectBranch(nodeID, id) + 1; branch < kBranchFactor; ++branch) + auto const taken = selectBranch(nodeID, id); + auto const remaining = searchingForward ? (kBranchFactor - 1u - taken) : taken; + + for (auto scanned = 0u; scanned < remaining; ++scanned) { - if (!inner.isEmptyBranch(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)); - } + auto const branch = + searchingForward ? (taken + 1u + scanned) : (taken - 1u - scanned); + if (inner.isEmptyBranch(branch)) + continue; + + stack.pushChild(descendThrow(inner, branch), branch); + auto const leaf = belowHelper(stack, direction); + if (leaf == nullptr) + Throw(type_, id); + return ConstIterator(this, leaf->peekItem().get(), std::move(stack)); } } stack.pop(); } return end(); } + +SHAMap::ConstIterator +SHAMap::upperBound(uint256 const& id) const +{ + return boundHelper(id, BelowDirection::First); +} + SHAMap::ConstIterator SHAMap::lowerBound(uint256 const& id) const { - NodePathStack stack; - walkTowardsKey(id, &stack); - while (!stack.empty()) - { - auto [node, nodeID] = stack.top(); - if (node->isLeaf()) - { - auto leaf = safeDowncast(node.get()); - if (leaf->peekItem()->key() < id) - return ConstIterator(this, leaf->peekItem().get(), std::move(stack)); - } - else - { - auto& inner = safeDowncast(*node); - for (auto branch = selectBranch(nodeID, id); branch > 0u;) - { - --branch; - if (!inner.isEmptyBranch(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)); - } - } - } - stack.pop(); - } - // TODO: what to return here? - return end(); + return boundHelper(id, BelowDirection::Last); } bool diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index 7742515dd8..349f7c6fd9 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -417,6 +417,50 @@ TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys) } } +TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end) +{ + tests::TestNodeFamily f{j_}; + SHAMap map{SHAMapType::FREE, f}; + map.setUnbacked(); + + // The root is a childless inner node, so boundHelper's inner-node branch must scan all 16 + // branches, find every one empty, and fall through to end() rather than dereference a child. + EXPECT_EQ(map.upperBound(uint256{}), map.end()); + EXPECT_EQ(map.lowerBound(uint256{}), map.end()); + + uint256 probe; + std::fill_n(probe.begin(), probe.size(), std::uint8_t{0xff}); + EXPECT_EQ(map.upperBound(probe), map.end()); + EXPECT_EQ(map.lowerBound(probe), map.end()); +} + +TEST_F(SHAMapTraversal, bounds_on_single_item_map_use_the_leaf_root) +{ + tests::TestNodeFamily f{j_}; + SHAMap map{SHAMapType::FREE, f}; + + auto const key = deepFanOutKeys().front(); + fillMap(map, {key}); + + // root_ can be a leaf, but only after syncing a single-item map from a peer (addRootNode); + // fillMap builds this map in-process via addItem, which always leaves root_ as the inner node + // it was constructed with, with the single leaf one level below it. So here the stack holds + // that inner root plus the leaf, and boundHelper's leaf branch, examined first, decides the + // outcome before root_'s own inner-node scan would ever run. + uint256 below = key; + --below; + uint256 above = key; + ++above; + + EXPECT_EQ(map.upperBound(below)->key(), key); + EXPECT_EQ(map.upperBound(key), map.end()); + EXPECT_EQ(map.upperBound(above), map.end()); + + EXPECT_EQ(map.lowerBound(above)->key(), key); + EXPECT_EQ(map.lowerBound(key), map.end()); + EXPECT_EQ(map.lowerBound(below), map.end()); +} + TEST_F(SHAMapTraversal, iteration_survives_deletions) { tests::TestNodeFamily f{j_};