ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-27 15:28:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Honour an early stop at the SHAMap root, and sharpen the walk contract

Browse Source 
`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.
This commit is contained in:
Bart
2026-09-23 14:42:00 +02:00
parent 7517c1b7cd
commit bb0cd0d1b5
4 changed files with 80 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
include/xrpl/shamap/SHAMap.h
View File

@@ -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<bool(SHAMapTreeNode&)> 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

10
src/libxrpl/shamap/SHAMapSync.cpp
View File

@@ -49,7 +49,9 @@ SHAMap::visitNodes(std::function<bool(SHAMapTreeNode&)> 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<bool(SHAMapTreeNode&)> 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;

61
src/tests/libxrpl/shamap/SHAMapMissingNode.cpp
View File

@@ -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)
{

7
src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp
View File

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