From f45cce80e62b2284322d9f0c8a03a86af87f7b2a Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 22 Aug 2026 22:15:47 -0400 Subject: [PATCH] fix: Report a map-invalidating node as invalid data SHAMap::addKnownNode() reports invalid() for the two kinds of node it does not hook into the map: an inner node arriving at kLeafDepth, a depth only a leaf may occupy, and a node whose ID does not match the position the descent stopped at. That is the verdict callers already handle as bad data, so neither counts as forward progress. Each emits one warning naming the node and where the descent stopped, matching the sibling branches beside them, and carries a SOMETIMES() hint for the fuzzer. The depth rule is spelled once, as a file-local isLeafDepth() that hasLeafNode() reads too. The verdict on a map-invalidating node belongs to the root hash that was asked for rather than to this copy of the tree, since every node from the root down hash-verified to get there: no peer can satisfy such a hash, retrying is futile, and it cannot arise by accident. A charge for it is a deterrent rather than a control even so, which the comment says, because the same node can reach a map through a fetch pack or an unsolicited object reply and neither passes through here. The depth test precedes the full-below cache lookup on the way down. That cache is keyed by node hash and shared by every map of a family, and a hash covers a node's children but not its depth, so the same subtree hash can be cached as complete at one depth and reached at kLeafDepth here. Testing the depth first is what keeps the verdict independent of whatever an unrelated map cached, which is the determinism the acquisition paths need from it. Skipping the shortcut at the deepest level only forgoes an optimization, and the depth it guards cannot occur in a real tree. Three tests drive the map through DeepChain's fill() and addOffendingNode(), so a case names the position no valid tree can occupy without spelling out the descent. They cover the map-invalidating node; the three ways a node cannot be hooked anywhere while leaving the map sound; and the same offending node with a full-below entry already in place, so the depth test is what has to reach the verdict. A file-local tallyIs() reads each verdict as counts, which leaves get()'s wording pinned in one place rather than at every site with a verdict to check. --- include/xrpl/shamap/SHAMap.h | 5 ++ src/libxrpl/shamap/SHAMapSync.cpp | 72 +++++++++++---- src/tests/libxrpl/shamap/SHAMapSync.cpp | 111 ++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 16 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 05c06812e3..96ccb34643 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -393,6 +393,11 @@ public: * @param filter Optional sync filter to track received nodes. * @return Status indicating whether the node was useful, duplicate, or invalid. * + * A node no valid tree could hold makes the map Invalid, which is + * terminal: the root hash committed to an impossible shape, so no peer + * can satisfy it. An acquisition reaching this verdict must give up + * rather than retry; nothing may promote the map back to a valid state. + * * @note This function expects the treeNode to be a valid, deserialized SHAMapTreeNode. The * caller is responsible for deserialization and basic validation before calling this * function. This also means that the nodeID must be consistent with the node's content. diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 9b6e1477c3..79e342fe46 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -31,6 +31,26 @@ namespace xrpl { +namespace { + +/** + * Whether a depth is one only a leaf may occupy. + * + * Nibbles run out at SHAMap::kLeafDepth, so an inner node there would need two + * keys agreeing in all 64 nibbles. Spelled once, since the sync path tests it + * for a node, for a node's child, and for a position a descent reached. + * + * @param depth The depth to judge. + * @return Whether an inner node at that depth would make the map impossible. + */ +[[nodiscard]] bool +isLeafDepth(unsigned int depth) +{ + return depth >= SHAMap::kLeafDepth; +} + +} // namespace + void SHAMap::visitLeaves( std::function const& item)> const& leafFunction) @@ -584,7 +604,15 @@ SHAMap::addKnownNode( } auto childHash = inner->getChildHash(branch); - if (f_.getFullBelowCache()->touchIfExists(childHash.asUInt256())) + + // The depth test precedes the cache lookup deliberately: the cache is keyed by node hash + // and shared across every map of this family, and a hash covers a node's children but not + // its depth, so the same subtree hash can be cached as complete at one depth and reached at + // another. Taking the shortcut first would make the verdict below depend on what an + // unrelated map cached, and the acquisition paths rely on it being deterministic. Skipping + // the shortcut only forgoes an optimization. + if (!isLeafDepth(currNodeID.getDepth() + 1) && + f_.getFullBelowCache()->touchIfExists(childHash.asUInt256())) { return SHAMapAddNode::duplicate(); } @@ -602,25 +630,37 @@ SHAMap::addKnownNode( return SHAMapAddNode::invalid(); } - // Inner nodes must be at a level strictly less than 64 - // but leaf nodes (while notionally at level 64) can be - // at any depth up to and including 64: - if ((currNodeID.getDepth() > kLeafDepth) || - (treeNode->isInner() && currNodeID.getDepth() == kLeafDepth)) + // Only leaves may sit at kLeafDepth (see isLeafDepth), so an inner node there makes the map + // impossible. Nothing is hooked in, so this is bad data rather than progress. + // + // Every node from the root down hash-verified to get here, so it is the requested root hash + // itself that commits to a shape no valid tree can have. The verdict therefore belongs to + // that hash rather than to our copy of the tree: no peer can satisfy it, retrying is + // futile, and it cannot arise by accident. The acquisition paths rely on all three. + // + // A charge is a deterrent rather than a control: the same node can reach a map through a + // fetch pack or an unsolicited object reply, neither of which comes through here, so + // nothing may assume the sender of such a node was made to pay for it. + bool const badDepth = treeNode->isInner() && isLeafDepth(currNodeID.getDepth()); + SOMETIMES(badDepth, "xrpl::SHAMap::addKnownNode : map is invalid"); + if (badDepth) { - // Map is provably invalid + JLOG(journal_.warn()) << "Node " << nodeID << " makes the map invalid at " + << currNodeID; setInvalid(); - return SHAMapAddNode::useful(); + return SHAMapAddNode::invalid(); } - if (currNodeID != nodeID) + // The data hashes to the child we need at currNodeID but is labeled as belonging at nodeID, + // so it cannot be hooked anywhere. Only the label is wrong, so the map stays sound and the + // node is still obtainable from another sender. + bool const badPosition = (currNodeID != nodeID); + SOMETIMES(badPosition, "xrpl::SHAMap::addKnownNode : node ID does not match its position"); + if (badPosition) { - // Either this node is broken or we didn't request it (yet) - JLOG(journal_.warn()) << "unable to hook node " << nodeID; - JLOG(journal_.info()) << " stuck at " << currNodeID; - JLOG(journal_.info()) << "got depth=" << nodeID.getDepth() - << ", walked to= " << currNodeID.getDepth(); - return SHAMapAddNode::useful(); + JLOG(journal_.warn()) << "Unable to hook node " << nodeID << ", stuck at " + << currNodeID; + return SHAMapAddNode::invalid(); } if (backed_) @@ -754,7 +794,7 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const // node claiming kLeafDepth, and getChildNodeID below throws in that case: reject rather // than let the throw escape uncaught. Not reachable through any public entry point, // since addKnownNode already marks such a map invalid, so no test can cover this. - if (nodeID.getDepth() >= kLeafDepth) + if (isLeafDepth(nodeID.getDepth())) { // LCOV_EXCL_START UNREACHABLE("xrpl::SHAMap::hasLeafNode : inner node at leaf depth"); diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index c9c257df97..fa383f357a 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -21,6 +22,7 @@ #include #include +#include #include #include @@ -49,6 +51,29 @@ noAmendments() return Rules{std::unordered_set>{}}; } +/** + * Whether a verdict carries exactly the given counts. + * + * The counts rather than get(): that string is a log format, not an API. It is + * pinned once, in the SHAMapAddNode tests, and read here only to describe a + * failure. + * + * @param san The verdict to check. + * @param good How many nodes the batch should have hooked in. + * @param bad How many it should have rejected. + * @param duplicate How many it should have already held. + * @return Whether the verdict matches, naming the actual tally if it does not. + */ +[[nodiscard]] static ::testing::AssertionResult +tallyIs(SHAMapAddNode const& san, int good, int bad, int duplicate) +{ + if (san.getGood() == good && san.getBad() == bad && san.getDuplicate() == duplicate) + return ::testing::AssertionSuccess(); + + return ::testing::AssertionFailure() << "tally is " << san.get() << ", expected good:" << good + << " bad:" << bad << " dupe:" << duplicate; +} + class SHAMapSyncTest : public ::testing::Test { protected: @@ -226,6 +251,92 @@ protected: }; }; +// An inner node at kLeafDepth, where only leaves can live, leaves the map provably invalid. It +// must be reported as bad data rather than counted as useful. +TEST_F(SHAMapSyncTest, innerNodeAtLeafDepth) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(chain.fill(map)); + ASSERT_TRUE(map.isValid()); + + auto const result = chain.addOffendingNode(map); + + EXPECT_TRUE(tallyIs(result, 0, 1, 0)); + EXPECT_FALSE(result.isGood()); + EXPECT_FALSE(map.isValid()); +} + +// A node that cannot be hooked anywhere is bad data, so the batch counts no progress, but the map +// itself is unharmed and another sender can still complete it. All three ways of getting there are +// covered, since they share that verdict. +TEST_F(SHAMapSyncTest, nodeThatCannotBeHookedIsBadData) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + + // nodeAt(1) is the node the root is missing and its hash matches, but we claim depth 2. + auto const wrongDepth = map.addKnownNode(SHAMapNodeID{2, uint256{}}, chain.nodeAt(1), nullptr); + + EXPECT_TRUE(tallyIs(wrongDepth, 0, 1, 0)); + EXPECT_FALSE(wrongDepth.isUseful()); + + // The chain sits on branch 0 at every depth, so a node claiming a position on branch 1 asks the + // descent to follow a branch the root does not have. + uint256 otherBranch; + otherBranch.begin()[0] = 0x10; + auto const emptyBranch = + map.addKnownNode(SHAMapNodeID{1, otherBranch}, chain.nodeAt(1), nullptr); + + EXPECT_TRUE(tallyIs(emptyBranch, 0, 1, 0)); + + // The right position this time, but the data hashes to something other than the child the root + // says belongs there. + auto const corrupt = map.addKnownNode(SHAMapNodeID{1, uint256{}}, chain.nodeAt(2), nullptr); + + EXPECT_TRUE(tallyIs(corrupt, 0, 1, 0)); + + // Nothing was hooked in and nothing was proven about the tree, so the map stays usable. + EXPECT_TRUE(map.isValid()); +} + +// The verdict must not be bypassable through the full-below cache. That cache is keyed by node hash +// and shared by every map of a family, and a hash covers a node's children but not its depth, so an +// earlier walk can mark the same subtree hash complete at one depth while this map reaches it at +// kLeafDepth, with no collision involved. A hit there would report a duplicate and return before +// the verdict, leaving what every later caller relies on dependent on what an unrelated map cached. +TEST_F(SHAMapSyncTest, mapInvalidatingNodeIsJudgedOnCacheHit) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(chain.fill(map)); + ASSERT_TRUE(map.isValid()); + + // The cache is keyed on the hash of the child being considered, so at the kLeafDepth boundary + // that is the offending node itself, and a hit is what would skip the whole branch. This test + // seeds that entry, unlike the cases above, which never touch the cache and so always miss. + f.getFullBelowCache()->insert(chain.nodeAt(SHAMap::kLeafDepth)->getHash().asUInt256()); + + auto const result = chain.addOffendingNode(map); + + EXPECT_TRUE(tallyIs(result, 0, 1, 0)); + EXPECT_FALSE(result.isGood()); + EXPECT_FALSE(map.isValid()); +} + // A map marked complete in the database withdraws that claim the first time a read misses, and // reports the miss once so the ledger can be re-acquired. Sixteen unresolvable branches are posted // in one pass, so with four reader threads the misses overlap and finishFetch() runs concurrently