From 82eeb14577a0fe58d0f5ab00fbc81d7d6ec4fa2d 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 | 75 ++++++++++++----------------- src/tests/libxrpl/shamap/SHAMap.cpp | 46 ++++++++++++++++++ 3 files changed, 81 insertions(+), 44 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 0baea78931..a29f8fb679 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -592,6 +592,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 0e28c0222a..137f9d32a2 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -575,8 +575,13 @@ 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()) @@ -584,63 +589,45 @@ SHAMap::upperBound(uint256 const& id) const 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 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)); - } - 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 9aaa021301..abceade814 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -455,6 +455,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_};