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