ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-26 15:10:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: Unify upperBound and lowerBound into boundHelper

Browse Source 
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.
This commit is contained in:
Bart
2026-08-24 11:20:50 -04:00
parent 00606bec1a
commit 82eeb14577
3 changed files with 81 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
include/xrpl/shamap/SHAMap.h
View File

@@ -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*

75
src/libxrpl/shamap/SHAMap.cpp
View File

@@ -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<SHAMapLeafNode*>(node.get());
if (leaf->peekItem()->key() > id)
return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
auto const& item = safeDowncast<SHAMapLeafNode const&>(*node).peekItem();
if (searchingForward ? (item->key() > id) : (item->key() < id))
return ConstIterator(this, item.get(), std::move(stack));
}
else
{
auto& inner = safeDowncast<SHAMapInnerNode&>(*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<SHAMapMissingNode>(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<SHAMapMissingNode>(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<SHAMapLeafNode*>(node.get());
if (leaf->peekItem()->key() < id)
return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
}
else
{
auto& inner = safeDowncast<SHAMapInnerNode&>(*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<SHAMapMissingNode>(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

46
src/tests/libxrpl/shamap/SHAMap.cpp
View File

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