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_};