From f0c5ce5266d2e401813adcb4510f2e59af1756a5 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:20:50 -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, the smallest map that reaches that branch, 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. The same comment also claimed the leaf branch settles every probe before `root_`'s inner-node scan could run, which only holds for a probe the leaf qualifies against: for the rest the leaf is popped and that scan is exactly what reaches `end()`. The test name said `leaf root` for the same reason, and now names the leaf below the root. --- include/xrpl/shamap/SHAMap.h | 4 ++ src/libxrpl/shamap/SHAMap.cpp | 77 ++++++++++++----------------- src/tests/libxrpl/shamap/SHAMap.cpp | 46 +++++++++++++++++ 3 files changed, 82 insertions(+), 45 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 1f392c28ce..c408a530f4 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -593,6 +593,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 514d3dbf88..e549e969fe 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -420,6 +420,52 @@ 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 scans every branch on + // the requested side of the one id selects, finds them all empty, and falls 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_below_the_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 the stack holds that + // inner root plus the leaf, and boundHelper examines the leaf first. Only a probe the leaf + // qualifies against is answered there; for the rest the leaf is popped and root_'s own + // inner-node scan runs, finds nothing on the requested side, and falls through to end(). + 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_};