From b94f7c1979fae0fccdcc9dc6e831fcb1325900f3 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Mon, 3 Aug 2026 20:48:48 -0400 Subject: [PATCH] test: Cover `visitDifferences` node skipping `visitDifferences` had no test coverage: `LedgerMaster::populateFetchPack` is its only caller, and nothing exercised that path, so the private `hasLeafNode` and `hasInnerNode` it consults were untested too. Codecov flagged the resulting gap against an unrelated refactor, since that commit happened to touch a line inside `hasLeafNode`. Four cases: a map missing one item reports only the nodes on that item's path, an identical map reports nothing, a null comparison map reports every node the plain `visitNodes` walk sees, and a callback returning false stops the walk where `populateFetchPack` would stop once a pack is full. The first case uses 64 shared items rather than a couple, so the shared leaves build inner nodes of their own and the walk has whole matching subtrees to skip. Verified by mutation: with only two shared items, breaking `hasInnerNode` to always claim a match left every test passing, since the tree was too shallow for that branch to be taken. It also asserts that some inner nodes are reported but not all, which is what distinguishes skipping a subtree from walking it. Breaking `hasLeafNode` in either direction, or `hasInnerNode`, now fails the case. The maps are hashed via `getHash` before being compared, since node hashes are computed on demand and `visitDifferences` returns early while the root hash is still zero. Without that the walk visits nothing and every assertion here would hold vacuously. --- src/tests/libxrpl/shamap/SHAMapSync.cpp | 141 ++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index e4bcbd89705..0bb27f9cbb0 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -2,11 +2,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include @@ -180,4 +182,143 @@ TEST_F(SHAMapSyncTest, sync) destination.invariants(); } +// `visitDifferences` walks this map and reports only the nodes the other map does not already +// have, which is how a fetch pack is assembled (see LedgerMaster's populateFetchPack). It decides +// what to skip by asking `hasInnerNode` and `hasLeafNode`, both private, so these tests are the +// only way to reach them. +// +// Both answers matter to a peer waiting on the pack. Reporting a node it already has wastes space +// in a size-limited message; skipping one it does not have leaves it unable to complete the +// ledger. The tests below therefore check exactly which nodes come back, not just how many. + +// Node hashes are computed on demand, and `visitDifferences` returns early while the root hash is +// still zero, so a map has to be hashed before it can be compared against another. +static void +finalize(SHAMap& map) +{ + map.setImmutable(); + ASSERT_FALSE(map.getHash().isZero()); +} + +TEST_F(SHAMapSyncTest, visit_differences_reports_only_what_is_missing) +{ + TestNodeFamily f{j_}; + + // Enough shared items that they build inner nodes of their own: the walk then has whole + // matching subtrees to skip, which is what hasInnerNode decides. With only a couple of shared + // leaves the tree is too shallow for that path to be taken at all. + std::vector> shared; + shared.reserve(64); + for (int i = 0; i < 64; ++i) + shared.push_back(makeRandomAS()); + + auto const extra = makeRandomAS(); + + SHAMap have{SHAMapType::FREE, f}; + for (auto const& item : shared) + ASSERT_TRUE(have.addItem(SHAMapNodeType::TnAccountState, item)); + finalize(have); + + SHAMap want{SHAMapType::FREE, f}; + for (auto const& item : shared) + ASSERT_TRUE(want.addItem(SHAMapNodeType::TnAccountState, item)); + ASSERT_TRUE(want.addItem(SHAMapNodeType::TnAccountState, extra)); + finalize(want); + + std::vector leaves; + std::size_t inners = 0; + want.visitDifferences(&have, [&leaves, &inners](SHAMapTreeNode const& node) { + if (node.isLeaf()) + leaves.push_back(safeDowncast(node).peekItem()->key()); + else + ++inners; + return true; + }); + + // Every shared leaf is already on the far side, so only `extra` is worth sending. + EXPECT_EQ(leaves, std::vector{extra->key()}); + + // The inner nodes on `extra`'s path are reported, but the matching subtrees are skipped, so + // the walk must not have visited every inner node in the tree. + std::size_t allInners = 0; + want.visitNodes([&allInners](SHAMapTreeNode& node) { + if (!node.isLeaf()) + ++allInners; + return true; + }); + EXPECT_GT(inners, 0u); + EXPECT_LT(inners, allInners); +} + +TEST_F(SHAMapSyncTest, visit_differences_against_identical_map_reports_nothing) +{ + TestNodeFamily f{j_}; + + auto const item = makeRandomAS(); + + SHAMap have{SHAMapType::FREE, f}; + ASSERT_TRUE(have.addItem(SHAMapNodeType::TnAccountState, item)); + finalize(have); + + SHAMap want{SHAMapType::FREE, f}; + ASSERT_TRUE(want.addItem(SHAMapNodeType::TnAccountState, item)); + finalize(want); + ASSERT_EQ(want.getHash(), have.getHash()); + + std::size_t visited = 0; + want.visitDifferences(&have, [&visited]([[maybe_unused]] SHAMapTreeNode const& node) { + ++visited; + return true; + }); + + EXPECT_EQ(visited, 0u); +} + +TEST_F(SHAMapSyncTest, visit_differences_against_no_map_reports_every_node) +{ + TestNodeFamily f{j_}; + + SHAMap want{SHAMapType::FREE, f}; + for (int i = 0; i < 32; ++i) + ASSERT_TRUE(want.addItem(SHAMapNodeType::TnAccountState, makeRandomAS())); + finalize(want); + + // A null `have` means the far side holds nothing, so every node counts as missing. Compared + // against visitNodes, which walks the same tree with no such filtering. + std::size_t differences = 0; + want.visitDifferences(nullptr, [&differences]([[maybe_unused]] SHAMapTreeNode const& node) { + ++differences; + return true; + }); + + std::size_t all = 0; + want.visitNodes([&all]([[maybe_unused]] SHAMapTreeNode& node) { + ++all; + return true; + }); + + EXPECT_GT(differences, 0u); + EXPECT_EQ(differences, all); +} + +TEST_F(SHAMapSyncTest, visit_differences_stops_when_callback_returns_false) +{ + TestNodeFamily f{j_}; + + SHAMap want{SHAMapType::FREE, f}; + for (int i = 0; i < 32; ++i) + ASSERT_TRUE(want.addItem(SHAMapNodeType::TnAccountState, makeRandomAS())); + finalize(want); + + // Returning false is how populateFetchPack stops once the pack is full, so the walk must + // honour it rather than visiting the rest of the tree. + std::size_t visited = 0; + want.visitDifferences(nullptr, [&visited]([[maybe_unused]] SHAMapTreeNode const& node) { + ++visited; + return visited < 3; + }); + + EXPECT_EQ(visited, 3u); +} + } // namespace xrpl::tests