From 5891da909d25d2e49ab0353a1cba9beff47fa847 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:35:11 -0400 Subject: [PATCH] docs: Correct the reachability claims on the leaf depth guards The `visitDifferences` guard said an inner node at leaf depth could be seeded by a peer through an earlier fetch-pack exchange, while the code marks the branch `UNREACHABLE`, which is documented as a line that must not be reached in normal use or under fuzzing. Both cannot hold: a peer-reachable assert is a peer-triggered abort. The code is the correct half. `addKnownNode` marks the map invalid instead of hooking such a node in, fetch-pack blobs are checked against their content hash in `LedgerMaster::getFetchPack`, and the parent child-hash slots they attach to chain up to a validated root, so provoking this needs a preimage rather than a crafted message. The comment now attributes the branch to a defect or a corrupt store, and records why the node is still reported into the pack before its children are skipped: the wire form of an inner node carries only child hashes, so the bad depth is never transmitted, and the recipient hooks blobs in by hash at positions its own traversal picks. Withholding it would instead surface as a peer that cannot complete a ledger, with nothing to diagnose locally. The `hasLeafNode` comment credited its caller with a bound that does not apply. That guard limits the depth of the caller's own traversal, whereas `hasLeafNode` runs on the map passed in and descends from that map's root, so the check here is the only thing between a malformed map and the throw in `getChildNodeID`, not a second line of defense. `hasInnerNode` is the one bounded by its caller. Co-Authored-By: Claude Opus 5 (1M context) --- src/libxrpl/shamap/SHAMapSync.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 8849bbda14..4319d0bcd4 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -143,9 +143,12 @@ SHAMap::visitDifferences( if (!function(*node)) return; - // Nibbles run out at kLeafDepth, so only a leaf belongs there. An inner node at that depth - // means the store holds one, possibly seeded by a peer via an earlier fetch-pack exchange; - // skip its children rather than descending into it. + // Nibbles run out at kLeafDepth, so only a leaf belongs there. A well-formed map never + // holds an inner node at that depth: addKnownNode marks the map invalid rather than hooking + // one in, and fetch-pack data is hash-verified against a validated root, so reaching this + // means a defect or a corrupt store, not something a peer can provoke. Report the node + // anyway - the wire form carries no depth, and the recipient hooks blobs in by hash - but + // skip the children rather than letting getChildNodeID throw on them. if (nodeID.getDepth() >= kLeafDepth) { // LCOV_EXCL_START @@ -760,9 +763,9 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const do { - // Same kLeafDepth hazard as in visitDifferences above. That's the only caller and already - // bails out before reaching here; this is a second line of defense since the loop below - // descends on its own and doesn't depend on the caller's bound. + // Same kLeafDepth hazard as in visitDifferences above. That guard bounds the caller's own + // traversal, not the map queried here, and the loop below descends from this map's root + // independently, so this check is what keeps a malformed map from reaching getChildNodeID. if (nodeID.getDepth() >= kLeafDepth) { // LCOV_EXCL_START