From a09a255ed4a4ceaef8887ea03122e43ebf372b0f Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:55:16 +0200 Subject: [PATCH] fix: Report unreadable SHAMap nodes instead of throwing `SHAMap::walkMap` and `walkMapParallel` treat a null `descendNoStore` result as "this node is missing" and record it in their `missingNodes` output, but `descendNoStore` used the throwing `fetchNode`, so that branch could never run for a backed map. `Ledger::walkLedger` propagated the throw instead of returning false, its "N missing account node(s)" log never printed, and `LedgerCleaner` never reached the path that clears the ledger and re-acquires it. `descendNoStore` now uses `fetchNodeNT`, which makes all three behave as written, and the null check after `fetchNode` in `descend` goes away as the dead code it always was. `visitNodes` and `visitLeaves` now return whether the walk read every node it reached, and the walk ends at the first node it cannot read, since no caller can use a partial result. `SHAMapStoreImp::run` abandons the rotation cycle on false, which is what its former `catch (SHAMapMissingNode)` did and what `clearPrior` plus the archive deletion in `rotate` require. `processReplayDeltaRequest` answers `reNO_NODE`, the code `xrpl.proto` documents for nodes we do not have, rather than sending a transaction list it knows is short, and clears the header it had already set so an error reply carries no partial payload. The two `RCLConsensus` walks run on unbacked maps, which have no node store to fail to read, so they cannot report an incomplete result. `walkMapParallel` decided its result from the exceptions its workers caught, yet those workers record an unreadable child in `missingNodes` instead, so it never consulted the list it was filling. The result now counts what this call recorded, measured against the caller's initial vector size. The pass that reads the root's children runs before any worker and dropped a null child silently, because the loop that spawns workers skips one; it records the miss itself now. A one-node map reports complete, which is what `walkMap` already reported for the same input, and the worker handler catches `std::exception` so that nothing leaves a worker's thread. Nine gtests in `src/tests/libxrpl/shamap/SHAMapMissingNode.cpp` cover the three walks over a partially copied map, the missing-node budget, an early stop by the visitor, a stop at the root, a map holding only its root, and a map whose root is a leaf. --- include/xrpl/shamap/SHAMap.h | 20 +- src/libxrpl/shamap/SHAMap.cpp | 5 +- src/libxrpl/shamap/SHAMapDelta.cpp | 64 ++- src/libxrpl/shamap/SHAMapSync.cpp | 33 +- .../libxrpl/shamap/SHAMapMissingNode.cpp | 441 ++++++++++++++++++ src/tests/libxrpl/shamap/SHAMapSync.cpp | 10 +- src/tests/libxrpl/shamap/common.h | 24 + src/xrpld/app/consensus/RCLConsensus.cpp | 4 + .../ledger/detail/LedgerReplayMsgHandler.cpp | 15 +- src/xrpld/app/misc/SHAMapStoreImp.cpp | 17 +- 10 files changed, 580 insertions(+), 53 deletions(-) create mode 100644 src/tests/libxrpl/shamap/SHAMapMissingNode.cpp diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 0baea78931..228f517797 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -261,12 +261,17 @@ public: lowerBound(uint256 const& id) const; /** - * Visit every node in this SHAMap + * Visit every node in this SHAMap. * - * @param function called with every node visited. - * If function returns false, visitNodes exits. + * The walk ends at the first node it cannot read, so a caller that needs a + * complete walk to be correct must check the return value. + * + * @param function Called with every node visited. Returning false from it + * ends the walk, which on its own keeps the result true. + * @return True when the walk read every node it reached. False when a node + * could not be read. */ - void + bool visitNodes(std::function const& function) const; /** @@ -280,11 +285,12 @@ public: visitDifferences(SHAMap const* have, std::function const&) const; /** - * Visit every leaf node in this SHAMap + * Visit every leaf node in this SHAMap. * - * @param function called with every non inner node visited. + * @param function Called with every non-inner node visited. + * @return What visitNodes reports about the walk. */ - void + bool visitLeaves(std::function const&)> const&) const; // comparison/sync functions diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 0e28c0222a..797d6e8712 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -326,9 +326,6 @@ SHAMap::descend(SHAMapInnerNode& parent, unsigned int branch) const return node; node = fetchNode(parent.getChildHash(branch)); - if (!node) - return {}; - node = parent.canonicalizeChild(branch, std::move(node)); return node; } @@ -340,7 +337,7 @@ SHAMap::descendNoStore(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr ret = parent.getChild(branch); if (!ret && backed_) - ret = fetchNode(parent.getChildHash(branch)); + ret = fetchNodeNT(parent.getChildHash(branch)); return ret; } diff --git a/src/libxrpl/shamap/SHAMapDelta.cpp b/src/libxrpl/shamap/SHAMapDelta.cpp index 1306fe6990..3f7a8ff6b7 100644 --- a/src/libxrpl/shamap/SHAMapDelta.cpp +++ b/src/libxrpl/shamap/SHAMapDelta.cpp @@ -13,9 +13,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -283,21 +285,36 @@ bool SHAMap::walkMapParallel(std::vector& missingNodes, int maxMissing) const { if (!root_->isInner()) // root_ is only node, and we have it - return false; + return true; + + // Only the nodes this call records count towards the result, so remember what the + // caller already had. + auto const initialMissing = missingNodes.size(); using StackEntry = intr_ptr::SharedPtr; std::array topChildren; { + // This loop runs before the workers start, so it needs no lock. auto const& innerRoot = intr_ptr::staticPointerCast(root_); for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if (!innerRoot->isEmptyBranch(i)) - topChildren[i] = descendNoStore(*innerRoot, i); + if (innerRoot->isEmptyBranch(i)) + continue; + + topChildren[i] = descendNoStore(*innerRoot, i); + if (!topChildren[i]) + { + // A root child that cannot be read hides its whole subtree. Record it here, + // because the loop below skips a null child without visiting it. + missingNodes.emplace_back(type_, innerRoot->getChildHash(i)); + if (--maxMissing <= 0) + return false; + } } } std::vector workers; workers.reserve(SHAMapInnerNode::kBranchFactor); - std::vector exceptions; + std::vector exceptions; exceptions.reserve(SHAMapInnerNode::kBranchFactor); std::array>, SHAMapInnerNode::kBranchFactor> @@ -353,10 +370,14 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis } } } - catch (SHAMapMissingNode const& e) + catch (std::exception const& e) { + // LCOV_EXCL_START + // A worker must not let an exception leave its thread, so record it + // and let the join below report it. std::scoped_lock const l(m); - exceptions.push_back(e); + exceptions.emplace_back(e.what()); + // LCOV_EXCL_STOP } }, std::move(nodeStacks[rootChildIndex])); @@ -366,14 +387,29 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis worker.join(); std::scoped_lock const l(m); - if (exceptions.empty()) - return true; - std::stringstream ss; - ss << "Exception(s) in ledger load: "; - for (auto const& e : exceptions) - ss << e.what() << ", "; - JLOG(journal_.error()) << ss.str(); - return false; + if (!exceptions.empty()) + { + // LCOV_EXCL_START + std::stringstream ss; + ss << "Exception(s) in ledger load: "; + for (auto const& e : exceptions) + ss << e << ", "; + JLOG(journal_.error()) << ss.str(); + return false; + // LCOV_EXCL_STOP + } + + // A node the workers could not read is recorded in `missingNodes` rather than thrown, + // so the result has to consult it too. + auto const found = missingNodes.size() - initialMissing; + if (found != 0) + { + JLOG(journal_.error()) << "Missing node(s) in ledger load: " << found + << ", first: " << missingNodes[initialMissing].what(); + return false; + } + + return true; } } // namespace xrpl diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 602d8e629c..db2102f586 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -31,28 +31,36 @@ namespace xrpl { -void +bool SHAMap::visitLeaves( std::function const& item)> const& leafFunction) const { - visitNodes([&leafFunction](SHAMapTreeNode& node) { + return visitNodes([&leafFunction](SHAMapTreeNode& node) { if (!node.isInner()) leafFunction(safeDowncast(node).peekItem()); return true; }); } -void +bool SHAMap::visitNodes(std::function const& function) const { if (!root_) - return; + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::visitNodes : non-null root"); + return true; + // LCOV_EXCL_STOP + } - function(*root_); + // The root is readable by definition, so a stop here still reports a complete walk. + if (!function(*root_)) + return true; + // A map whose root is a leaf holds one node, and this walk just visited it. if (!root_->isInner()) - return; + return true; using StackEntry = std::pair>; std::stack> stack; @@ -67,8 +75,17 @@ SHAMap::visitNodes(std::function const& function) const if (!node->isEmptyBranch(pos)) { SHAMapTreeNodePtr const child = descendNoStore(*node, pos); + if (!child) + { + // No caller can use a partial walk, so end it here rather than pay + // for the rest of the map. The callers that act on the result log + // it at a level an operator reads, so keep this one at debug. + JLOG(journal_.debug()) + << "visitNodes: unreadable child node " << node->getChildHash(pos); + return false; + } if (!function(*child)) - return; + return true; if (child->isLeaf()) { @@ -103,6 +120,8 @@ SHAMap::visitNodes(std::function const& function) const std::tie(pos, node) = stack.top(); stack.pop(); } + + return true; } void diff --git a/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp b/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp new file mode 100644 index 0000000000..bad900d23b --- /dev/null +++ b/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp @@ -0,0 +1,441 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include + +namespace xrpl::tests { + +namespace { + +// Fixed seed, so a failure reproduces when the test runs on its own. +constexpr std::uint32_t kSeed = 0xdeadbeefU; + +// Largest number of nodes getMissingNodes may report per round. +constexpr int kMaxNodesPerRequest = 2048; + +/** + * Copy part of a SHAMap into another SHAMap. + * + * The function answers at most nodeLimit of the requests that + * dest.getMissingNodes() makes. The remaining subtrees stay absent from the + * destination family's database, which is the state this test needs. + * + * @param source The complete map to copy from. + * @param dest The map to copy into. It must be in the synching state. + * @param nodeLimit The largest number of missing-node requests to answer. + * @return Nothing. The function reports a fatal gtest failure on error, so + * wrap the call in ASSERT_NO_FATAL_FAILURE. + */ +void +copyPartialMap(SHAMap const& source, SHAMap& dest, std::size_t nodeLimit) +{ + std::vector rootData; + if (!source.getNodeFat(SHAMapNodeID{}, rootData, false, 0)) + FAIL() << "Could not get root node"; + + auto rootNode = SHAMapTreeNode::makeFromWire(makeSlice(rootData[0].data)); + if (!rootNode) + FAIL() << "Could not deserialize root node"; + if (!dest.addRootNode(source.getHash(), std::move(rootNode), nullptr).isGood()) + FAIL() << "Could not add root node"; + + std::size_t answered = 0; + while (answered < nodeLimit) + { + auto const missing = dest.getMissingNodes(kMaxNodesPerRequest, nullptr); + if (missing.empty()) + break; + + for (auto const& request : missing) + { + if (answered >= nodeLimit) + return; + ++answered; + + std::vector nodeData; + if (!source.getNodeFat(request.first, nodeData, false, 0)) + continue; + + for (auto const& entry : nodeData) + { + auto node = SHAMapTreeNode::makeFromWire(makeSlice(entry.data)); + if (!node) + FAIL() << "Could not deserialize node " << entry.nodeID; + if (!dest.addKnownNode(entry.nodeID, std::move(node), nullptr).isGood()) + FAIL() << "Could not add known node " << entry.nodeID; + } + } + } +} + +/** + * Lift a leaf out of a one-item map in the wire format a peer would send it in. + * + * A locally built map always has an inner node for its root, so this is how a + * test reaches the one-node shape that addRootNode accepts. + * + * @param map A map holding exactly one item. + * @param wire Set to the leaf's wire form. + * @param hash Set to the leaf's hash. + * @return Nothing. The function reports a fatal gtest failure on error, so + * wrap the call in ASSERT_NO_FATAL_FAILURE. + */ +void +extractSoleLeaf(SHAMap const& map, Blob& wire, SHAMapHash& hash) +{ + auto leaves = 0; + map.visitNodes([&](SHAMapTreeNode& node) { + if (!node.isInner()) + { + ++leaves; + Serializer s; + node.serializeForWire(s); + wire = s.modData(); + hash = node.getHash(); + } + return true; + }); + if (leaves != 1) + FAIL() << "Expected one leaf, found " << leaves; +} + +struct NodeCounts +{ + bool complete; + int inner; + int leaves; +}; + +/** + * Count the inner and leaf nodes that visitNodes reaches. + * + * @param map The map to walk. + * @return The counts, plus what visitNodes reported about the walk. + */ +NodeCounts +countNodes(SHAMap const& map) +{ + int inner = 0; + int leaves = 0; + bool const complete = map.visitNodes([&inner, &leaves](SHAMapTreeNode& node) { + if (node.isInner()) + { + ++inner; + } + else + { + ++leaves; + } + return true; + }); + return {.complete = complete, .inner = inner, .leaves = leaves}; +} + +} // namespace + +// visitLeaves on a map with unreadable child nodes must not throw. It must end the walk +// at the first one and report that the walk was incomplete. +TEST(SHAMapMissingNode, visit_leaves_stops_at_an_unreadable_node) +{ + static constexpr auto kItems = 200; + static constexpr auto kNodesToCopy = 3uz; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + // The source map is complete, so it holds one leaf per item added. + auto beforeCount = 0; + EXPECT_TRUE(source.visitLeaves([&beforeCount](auto const&) { ++beforeCount; })); + EXPECT_EQ(beforeCount, kItems); + + // Copy only a few nodes, so most subtrees stay absent from destFamily's + // database. This is the state a node reaches when a child is evicted after + // a database rotation, or when a sync is still incomplete. + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + // The walk must return, must report that it was incomplete, and must reach fewer + // leaves than the source holds. + auto afterCount = 0; + auto complete = true; + EXPECT_NO_THROW(complete = dest.visitLeaves([&afterCount](auto const&) { ++afterCount; })); + EXPECT_FALSE(complete); + EXPECT_LT(afterCount, beforeCount); +} + +// walkMap must report the nodes it cannot read rather than throwing, so that +// Ledger::walkLedger can return false and let its caller re-acquire the ledger. +TEST(SHAMapMissingNode, walk_map_reports_missing_nodes) +{ + static constexpr auto kItems = 200; + static constexpr auto kNodesToCopy = 3uz; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + std::vector missing; + EXPECT_NO_THROW(dest.walkMap(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); + + // A complete map reports nothing missing. + std::vector none; + EXPECT_NO_THROW(source.walkMap(none, kMaxMissing)); + EXPECT_TRUE(none.empty()); +} + +// walkMapParallel must report a partial map as incomplete. Its workers record an +// unreadable child instead of throwing, so the result has to consult that list and not +// only the exceptions the workers caught. +TEST(SHAMapMissingNode, walk_map_parallel_reports_missing_nodes) +{ + static constexpr auto kItems = 200; + static constexpr auto kNodesToCopy = 3uz; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + std::vector missing; + EXPECT_FALSE(dest.walkMapParallel(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); + + // A complete map reports nothing missing. + std::vector none; + EXPECT_TRUE(source.walkMapParallel(none, kMaxMissing)); + EXPECT_TRUE(none.empty()); +} + +// walkMapParallel reads the root's children before it starts any worker, and skips a null +// one. That pass has to record the miss itself, or a map holding nothing but its root +// reports as complete. +TEST(SHAMapMissingNode, walk_map_parallel_reports_missing_root_children) +{ + static constexpr auto kItems = 200; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + // Copy the root and nothing else, so every one of its children is unreadable and no + // worker ever runs. + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, 0)); + + std::vector missing; + EXPECT_FALSE(dest.walkMapParallel(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); +} + +// Stopping the walk through the visitor is not the same thing as an unreadable node, so it +// must not make the result false. A stop at the root counts too; the root's answer used to +// be discarded. +TEST(SHAMapMissingNode, early_stop_on_a_complete_map_reports_complete) +{ + static constexpr auto kItems = 200; + static constexpr auto kVisitsBeforeStop = 3; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily family{j}; + + SHAMap map{SHAMapType::FREE, family}; + for (auto i = 0; i < kItems; ++i) + map.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + map.setImmutable(); + + auto visits = 0; + EXPECT_TRUE(map.visitNodes([&visits](SHAMapTreeNode&) { + ++visits; + return visits < kVisitsBeforeStop; + })); + EXPECT_EQ(visits, kVisitsBeforeStop); + + // Stopping at the root leaves the rest of the map unvisited. + auto rootVisits = 0; + EXPECT_TRUE(map.visitNodes([&rootVisits](SHAMapTreeNode&) { + ++rootVisits; + return false; + })); + EXPECT_EQ(rootVisits, 1); +} + +// A map holding nothing but its root cannot read any branch, so the walk visits the root +// alone and reports itself incomplete. +TEST(SHAMapMissingNode, a_root_only_map_reports_incomplete) +{ + static constexpr auto kItems = 200; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, 0)); + + auto visits = 0; + EXPECT_FALSE(dest.visitNodes([&visits](SHAMapTreeNode&) { + ++visits; + return true; + })); + EXPECT_EQ(visits, 1); +} + +// visitNodes on a complete map must reach every node. +TEST(SHAMapMissingNode, visit_nodes_complete_map) +{ + static constexpr auto kItems = 50; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily family{j}; + + SHAMap map{SHAMapType::FREE, family}; + for (auto i = 0; i < kItems; ++i) + map.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + map.setImmutable(); + + // The walk must report itself complete, and the map must hold one leaf per item + // added, plus the inner nodes that hold those leaves. + auto const [complete, inner, leaves] = countNodes(map); + EXPECT_TRUE(complete); + EXPECT_EQ(leaves, kItems); + EXPECT_GT(inner, 0); +} + +// The budget caps what a walk records. Once it is spent the walk stops, and the parallel +// walk spends it on the root's children before it starts a worker. +TEST(SHAMapMissingNode, a_spent_missing_node_budget_stops_the_walk) +{ + static constexpr auto kItems = 200; + static constexpr auto kMaxMissing = 1; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + // Copy the root and nothing else, so more than one child is unreadable and the + // budget of one runs out. + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, 0)); + + std::vector serial; + EXPECT_NO_THROW(dest.walkMap(serial, kMaxMissing)); + EXPECT_EQ(serial.size(), 1u); + + std::vector parallel; + EXPECT_FALSE(dest.walkMapParallel(parallel, kMaxMissing)); + EXPECT_EQ(parallel.size(), 1u); +} + +// A map whose root is a leaf holds one node and has it, so every walk reports it +// complete. addRootNode is the only way into that shape. +TEST(SHAMapMissingNode, a_leaf_rooted_map_reports_complete) +{ + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + Blob wire; + SHAMapHash hash; + ASSERT_NO_FATAL_FAILURE(extractSoleLeaf(source, wire, hash)); + + SHAMap dest{SHAMapType::FREE, hash.asUInt256(), destFamily}; + dest.setSynching(); + auto leaf = SHAMapTreeNode::makeFromWire(makeSlice(wire)); + ASSERT_TRUE(leaf); + ASSERT_TRUE(dest.addRootNode(hash, std::move(leaf), nullptr).isGood()); + + auto visits = 0; + EXPECT_TRUE(dest.visitNodes([&visits](SHAMapTreeNode&) { + ++visits; + return true; + })); + EXPECT_EQ(visits, 1); + + std::vector serial; + EXPECT_NO_THROW(dest.walkMap(serial, kMaxMissing)); + EXPECT_TRUE(serial.empty()); + + std::vector parallel; + EXPECT_TRUE(dest.walkMapParallel(parallel, kMaxMissing)); + EXPECT_TRUE(parallel.empty()); +} + +} // namespace xrpl::tests diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index e4bcbd8970..5403eb4a2d 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -18,7 +17,6 @@ #include #include -#include #include #include #include @@ -34,13 +32,7 @@ protected: boost::intrusive_ptr makeRandomAS() { - static constexpr auto kWordsPerState = 3uz; - - Serializer s; - - for (auto word = 0uz; word < kWordsPerState; ++word) - s.add32(randInt(eng_)); - return makeShamapitem(s.getSHA512Half(), s.slice()); + return makeRandomAccountStateItem(eng_); } bool diff --git a/src/tests/libxrpl/shamap/common.h b/src/tests/libxrpl/shamap/common.h index 91401d2973..09f92bb6dc 100644 --- a/src/tests/libxrpl/shamap/common.h +++ b/src/tests/libxrpl/shamap/common.h @@ -4,16 +4,21 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include +#include #include +#include + #include #include #include @@ -21,6 +26,25 @@ namespace xrpl::tests { +/** + * Build a SHAMapItem holding random account-state data. + * + * @param engine The random engine to draw the data from. Pass an engine with a + * fixed seed to make a test reproducible on its own. + * @return The new item. + */ +template +boost::intrusive_ptr +makeRandomAccountStateItem(Engine& engine) +{ + static constexpr auto kWordsPerState = 3uz; + + Serializer s; + for (auto word = 0uz; word < kWordsPerState; ++word) + s.add32(randInt(engine)); + return makeShamapitem(s.getSHA512Half(), s.slice()); +} + class TestNodeFamily : public Family { private: diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 42270a91f1..b842fc4248 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -391,6 +391,8 @@ RCLConsensus::Adaptor::onClose( LedgerIndex const seq = prevLedger->header().seq + 1; CensorshipDetector::TxIDSeqVec proposed; + // initialSet is unbacked, so the walk has no node store to fail to read and + // cannot report an incomplete result. initialSet->visitLeaves( [&proposed, seq](boost::intrusive_ptr const& item) { proposed.emplace_back(item->key(), seq); @@ -539,6 +541,8 @@ RCLConsensus::Adaptor::doAccept( { std::vector accepted; + // The consensus transaction set is unbacked, so the walk has no node store to + // fail to read and cannot report an incomplete result. result.txns.map->visitLeaves( [&accepted](boost::intrusive_ptr const& item) { accepted.push_back(item->key()); diff --git a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp index 6ed4a296ac..b72627cc17 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp @@ -222,9 +222,18 @@ LedgerReplayMsgHandler::processReplayDeltaRequest( reply.set_ledgerheader(nData.getDataPtr(), nData.getLength()); // pack transactions auto const& txMap = ledger->txMap(); - txMap.visitLeaves([&](boost::intrusive_ptr const& txNode) { - reply.add_transaction(txNode->data(), txNode->size()); - }); + if (!txMap.visitLeaves([&](boost::intrusive_ptr const& txNode) { + reply.add_transaction(txNode->data(), txNode->size()); + })) + { + // The transaction list above is incomplete, so name the missing nodes instead. + // The ledger itself is present, and an error reply carries no partial payload. + JLOG(journal_.debug()) << "getReplayDelta: Incomplete tx map for ledger " << ledgerHash; + reply.clear_transaction(); + reply.clear_ledgerheader(); + reply.set_error(protocol::TMReplyError::reNO_NODE); + return reply; + } JLOG(journal_.debug()) << "getReplayDelta for ledger " << ledgerHash << " txMap hash " << txMap.getHash().asUInt256(); diff --git a/src/xrpld/app/misc/SHAMapStoreImp.cpp b/src/xrpld/app/misc/SHAMapStoreImp.cpp index e19df597a2..33d82b4779 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -25,7 +25,6 @@ #include #include #include -#include #include #include @@ -408,17 +407,17 @@ SHAMapStoreImp::run() JLOG(journal_.debug()) << "copying ledger " << validatedSeq; std::uint64_t nodeCount = 0; - try - { - validatedLedger->stateMap().snapShot(false)->visitNodes( + // A partial copy must not be followed by a rotation: clearPrior() above has + // already advanced minimumOnline_, and rotate() below deletes the archive, so + // a node the walk did not reach would have no remaining copy. Abandon this + // cycle and retry on a later ledger instead. + if (!validatedLedger->stateMap().snapShot(false)->visitNodes( [this, &nodeCount](SHAMapTreeNode const& node) { return copyNode(nodeCount, node); - }); - } - catch (SHAMapMissingNode const& e) + })) { - JLOG(journal_.error()) - << "Missing node while copying ledger before rotate: " << e.what(); + JLOG(journal_.error()) << "Missing node while copying ledger " << validatedSeq + << " before rotate; abandoning this rotation"; continue; }