From bb0cd0d1b5da9b497930c1dd9a9a6bf756706255 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:42:00 +0200 Subject: [PATCH] fix: Honour an early stop at the SHAMap root, and sharpen the walk contract `visitNodes` discarded what the visitor returned for the root, so a visitor that stopped there had its answer ignored and the whole tree was walked anyway. The online-delete copy loop returns false from `copyNode` on a health abort, so an abort at the root was silently overridden. The root's answer is now honoured, and the walk reports itself complete, since the root was readable. The `@return` wording on `visitNodes` and `visitLeaves` claimed a true result meant every node was visited "including when the visitor stopped the walk early", which contradicts itself. Both now say what the result measures: whether every node the walk reached was readable, with early termination called out as a separate thing. The per-node missing-child log drops from info to debug. It fires once per unreadable child and an incomplete sync reaches it often, while the callers that act on the result already log the aggregate at a level an operator reads. `processReplayDeltaRequest` now answers `reNO_NODE` rather than `reNO_LEDGER` and clears the header it had already set. The ledger is present, so the missing nodes are what the error should name, and an error reply should not carry a partial payload. Both codes charge the requester the same fee, and the requester only checks whether an error is set, so this changes no peer's behaviour. Three tests cover the contract the reviewers asked about: an early stop on a complete map still reports complete, a stop at the root visits nothing further, and a map holding only its root reports incomplete after visiting one node. --- include/xrpl/shamap/SHAMap.h | 14 ++--- src/libxrpl/shamap/SHAMapSync.cpp | 10 ++- .../libxrpl/shamap/SHAMapMissingNode.cpp | 61 +++++++++++++++++++ .../ledger/detail/LedgerReplayMsgHandler.cpp | 7 ++- 4 files changed, 80 insertions(+), 12 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index fb6644ae4e..72cecc5de5 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -267,11 +267,11 @@ public: * subtree, so the walk always finishes. Callers that need a complete walk to be * correct must check the return value. * - * @param function Called with every node visited. If it returns false, visitNodes - * stops early. - * @return True when every node in the map was visited, including when @p function - * stopped the walk early. False when at least one subtree was skipped - * because its child node could not be read. + * @param function Called with every node visited. Returning false from it stops the + * walk, which is the only thing that stops the walk early. + * @return True when every node the walk reached was readable. False when at least + * one subtree was skipped because its child node could not be read. Stopping + * the walk through @p function does not on its own make the result false. */ bool visitNodes(std::function const& function) const; @@ -290,8 +290,8 @@ public: * Visit every leaf node in this SHAMap. * * @param function Called with every non-inner node visited. - * @return True when every leaf in the map was visited. False when at least one - * subtree was skipped because its child node could not be read. See + * @return True when every node the walk reached was readable. False when at least + * one subtree was skipped because its child node could not be read. See * visitNodes. */ bool diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index dc9b096743..35dee701f4 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -49,7 +49,9 @@ SHAMap::visitNodes(std::function const& function) const if (!root_) return true; - function(*root_); + // The root is readable by definition, so a stop here still reports a complete walk. + if (!function(*root_)) + return true; if (!root_->isInner()) return true; @@ -72,8 +74,10 @@ SHAMap::visitNodes(std::function const& function) const { // The child could not be read, which has several possible causes (an // incomplete sync, a gap in the node store). Skip the subtree and record - // that this walk did not cover the whole map. - JLOG(journal_.info()) + // that this walk did not cover the whole map. This logs per node, and an + // incomplete sync reaches it often, so keep it at debug; the callers that + // act on the result log the aggregate at a level an operator reads. + JLOG(journal_.debug()) << "visitNodes: missing child node " << node->getChildHash(pos); complete = false; ++pos; diff --git a/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp b/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp index 3e15362f97..1a3e1802a3 100644 --- a/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp +++ b/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp @@ -247,6 +247,67 @@ TEST(SHAMapMissingNode, walk_map_parallel_reports_missing_root_children) EXPECT_FALSE(missing.empty()); } +// Stopping the walk through the visitor is not the same thing as an unreadable subtree, +// so it must not make the result false. Stopping at the root must also be honoured; 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 reports every branch as unreadable, 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) { diff --git a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp index b9374d546a..1558e02be2 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp @@ -227,10 +227,13 @@ LedgerReplayMsgHandler::processReplayDeltaRequest( })) { // Part of the transaction map could not be read, so the list above is incomplete. - // Report that instead of sending a reply the requester would have to reject. + // Report that instead of sending a reply the requester would have to reject. The + // ledger itself is present, so the error names the missing nodes, and the header + // set above goes with it rather than accompanying an error. JLOG(journal_.debug()) << "getReplayDelta: Incomplete tx map for ledger " << ledgerHash; reply.clear_transaction(); - reply.set_error(protocol::TMReplyError::reNO_LEDGER); + reply.clear_ledgerheader(); + reply.set_error(protocol::TMReplyError::reNO_NODE); return reply; }