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