test: Cover visitDifferences node skipping

src/tests/libxrpl/shamap/SHAMapSync.cpp

@@ -2,11 +2,13 @@
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/random.h>
+#include <xrpl/basics/safe_cast.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/xor_shift_engine.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapItem.h>
+#include <xrpl/shamap/SHAMapLeafNode.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
@@ -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<boost::intrusive_ptr<SHAMapItem>> 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<uint256> leaves;
+    std::size_t inners = 0;
+    want.visitDifferences(&have, [&leaves, &inners](SHAMapTreeNode const& node) {
+        if (node.isLeaf())
+            leaves.push_back(safeDowncast<SHAMapLeafNode const&>(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<uint256>{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