diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index a29f8fb679..8aea9743d9 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -444,20 +444,46 @@ private: return stack_.size(); } + /** + * The node at the end of the path, paired with its ID. + * + * Reading an empty stack would be undefined, and the assert alone is stripped in release, + * so an empty path yields a null node the caller can test instead. + */ [[nodiscard]] std::pair const& top() const { - XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::top : non-empty stack"); + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::top : empty stack"); + static std::pair const kEmpty; + return kEmpty; + // LCOV_EXCL_STOP + } return stack_.top(); } void pop() { - XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::pop : non-empty stack"); + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pop : empty stack"); + return; + // LCOV_EXCL_STOP + } stack_.pop(); } + /** + * Discard the whole path. + * + * For a walk that pushed a node it then found unusable: the node never became a + * meaningful path entry, so it must not be mistaken for one by whatever the caller + * does next with an empty-vs-nonempty check. + */ void clear() { @@ -466,12 +492,22 @@ private: /** * Start a path at the root of the map, whose ID is the zero-depth ID by definition. + * + * @return false, leaving the path unchanged, if a path was already started. A malformed + * call must not abort a release build, so callers stop rather than overwrite it. */ - void + [[nodiscard]] bool pushRoot(SHAMapTreeNodePtr node) { - XRPL_ASSERT(stack_.empty(), "xrpl::SHAMap::NodePathStack::pushRoot : empty stack"); + if (!stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pushRoot : non-empty stack"); + return false; + // LCOV_EXCL_STOP + } stack_.emplace(std::move(node), SHAMapNodeID{}); + return true; } /** @@ -479,23 +515,54 @@ private: * * A node keeps the depth it was reached at, never a normalized kLeafDepth. Only a leaf may * sit at kLeafDepth, since an inner node there would have no branch left to select. + * + * @return false, leaving the path unchanged, if the current node can have no child, or if + * a leaf child does not lie under `branch`. A malformed map must not abort a + * release build, so callers stop walking instead. */ - void + [[nodiscard]] bool pushChild(SHAMapTreeNodePtr node, unsigned int branch) { - XRPL_ASSERT(node, "xrpl::SHAMap::NodePathStack::pushChild : non-null node input"); - XRPL_ASSERT( - !stack_.empty(), "xrpl::SHAMap::NodePathStack::pushChild : non-empty stack"); - auto childID = stack_.top().second.getChildNodeID(branch); - XRPL_ASSERT_IF( - node->isInner(), - childID.getDepth() < kLeafDepth, - "xrpl::SHAMap::NodePathStack::pushChild : inner node above leaf depth"); - XRPL_ASSERT_IF( - node->isLeaf(), - childID.isPrefixOf(leafKey(*node)), - "xrpl::SHAMap::NodePathStack::pushChild : leaf key below branch"); + if (stack_.empty() || !node || branch >= kBranchFactor) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pushChild : no child to push"); + return false; + // LCOV_EXCL_STOP + } + + // Only a leaf may sit at kLeafDepth, so an inner child must land one level short of + // it, tighter than the plain depth bound a leaf child needs. + auto const& parentID = stack_.top().second; + auto const parentDepth = parentID.getDepth(); + if (node->isInner() ? parentDepth + 1u >= kLeafDepth : parentDepth >= kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pushChild : child past leaf depth"); + return false; + // LCOV_EXCL_STOP + } + + // A leaf's own key names its position, so a leaf reached by this branch must agree with + // the ID that branch derives. Where the two disagree the pair is not a path entry at + // all, and keeping it would make every later walk read the ID rather than the key. + // + // Not UNREACHABLE, unlike the malformed-call cases above: a map assembled from peer + // data can hold such a leaf, because a node arriving through a sync filter is judged by + // hash and a hash says nothing about position. The paths that hook a node reject one + // first (see SHAMap::descend and SHAMap::gmnProcessNodes), so reaching here means one + // got past them, which is not a reason to abort an instrumented build. + auto childID = parentID.getChildNodeID(branch); + bool const misplaced = !belongsAt(childID, *node); + SOMETIMES( + misplaced, "xrpl::SHAMap::NodePathStack::pushChild : leaf key outside branch"); + if (misplaced) + { + return false; + } + stack_.emplace(std::move(node), std::move(childID)); + return true; } /** @@ -504,17 +571,14 @@ private: * For nodes not reached by descending a known branch: the walk tracks only the key it is * heading for, or the node is newly created. Either way `target` selects the branch. */ - void + [[nodiscard]] bool pushNode(SHAMapTreeNodePtr node, uint256 const& target) { if (stack_.empty()) { - pushRoot(std::move(node)); - } - else - { - pushChild(std::move(node), selectBranch(stack_.top().second, target)); + return pushRoot(std::move(node)); } + return pushChild(std::move(node), selectBranch(stack_.top().second, target)); } private: @@ -720,7 +784,7 @@ private: // getMissingNodes helper functions void gmnProcessNodes(MissingNodes&, MissingNodes::StackEntry& node); - static void + void gmnProcessDeferredReads(MissingNodes&); // fetch from DB helper function diff --git a/include/xrpl/shamap/SHAMapLeafNode.h b/include/xrpl/shamap/SHAMapLeafNode.h index ab5bd574ed..e9c7e6557d 100644 --- a/include/xrpl/shamap/SHAMapLeafNode.h +++ b/include/xrpl/shamap/SHAMapLeafNode.h @@ -75,4 +75,22 @@ leafKey(SHAMapTreeNode const& node) return safeDowncast(node).peekItem()->key(); } +/** + * Whether a node may occupy a position in a SHAMap. + * + * A leaf's own key names its position, so an ID that is not a prefix of that + * key names a different subtree than the one the leaf belongs to. An inner + * node carries no key, so every position is consistent with it and the + * caller's own depth rules are what bound it. + * + * @param nodeID the position the node is claimed to occupy. + * @param node the node to judge. + * @return whether the node's own key agrees with that position. + */ +[[nodiscard]] inline bool +belongsAt(SHAMapNodeID const& nodeID, SHAMapTreeNode const& node) +{ + return !node.isLeaf() || nodeID.isPrefixOf(leafKey(node)); +} + } // namespace xrpl diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 137f9d32a2..f47ee22f35 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -128,32 +128,70 @@ SHAMap::dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr c SHAMapLeafNode* SHAMap::walkTowardsKey(uint256 const& id, NodePathStack* stack) const { - XRPL_ASSERT( - stack == nullptr || stack->empty(), "xrpl::SHAMap::walkTowardsKey : empty stack input"); + if (stack != nullptr && !stack->empty()) + { + // A plain XRPL_ASSERT here is a no-op under NDEBUG; without this guard a non-empty stack + // would be appended to below, leaving the caller with a path that starts mid-walk instead + // of at the root. + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::walkTowardsKey : empty stack input"); + stack->clear(); + return nullptr; + // LCOV_EXCL_STOP + } + auto inNode = root_; SHAMapNodeID nodeID; - // Every node on this walk lies on the path to `id`, so the stack can derive each ID from the - // branch `id` selects at the node above it. - auto pushCurrent = [&] { - if (stack != nullptr) - stack->pushNode(inNode, id); + // Without a caller-supplied stack, `nodeID` is the only record of position, so it is derived + // directly here instead of read back from a push. A failure below means the map is malformed + // (an inner node one level too deep), not that `id` is merely absent; the stack is cleared + // rather than left holding a node that never became a real path entry. + auto pushCurrent = [&]() -> bool { + if (stack == nullptr || stack->pushNode(inNode, id)) + { + return true; + } + stack->clear(); + return false; }; while (inNode->isInner()) { - pushCurrent(); + if (!pushCurrent()) + { + return nullptr; + } auto& inner = safeDowncast(*inNode); - auto const branch = selectBranch(nodeID, id); + auto const branch = selectBranch(stack != nullptr ? stack->top().second : nodeID, id); if (inner.isEmptyBranch(branch)) return nullptr; inNode = descendThrow(inner, branch); - nodeID = nodeID.getChildNodeID(branch); + if (stack == nullptr) + { + // Only a leaf may sit at kLeafDepth, so an inner child needs the tighter bound: this + // must mirror pushChild's guard exactly, or a malformed map fails one mode earlier + // than the other and stack/no-stack callers disagree on the outcome. + auto const depth = nodeID.getDepth(); + if (inNode->isInner() ? depth + 1u >= kLeafDepth : depth >= kLeafDepth) + { + // Reported here as well, since pushChild reports it for the stack mode and a + // malformed map must not be silent in one mode only. + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::walkTowardsKey : child too deep"); + return nullptr; + // LCOV_EXCL_STOP + } + nodeID = nodeID.getChildNodeID(branch); + } } - pushCurrent(); + if (!pushCurrent()) + { + return nullptr; + } return safeDowncast(inNode.get()); } @@ -357,12 +395,34 @@ SHAMap::descend( !parent->isEmptyBranch(branch), "xrpl::SHAMap::descend : parent branch is non-empty"); SHAMapTreeNode* child = parent->getChildPointer(branch); // NOLINT(misc-const-correctness) + auto childID = parentID.getChildNodeID(branch); if (child == nullptr) { auto const& childHash = parent->getChildHash(branch); SHAMapTreeNodePtr childNode = fetchNodeNT(childHash, filter); + if (childNode && !belongsAt(childID, *childNode)) + { + // A node arriving through the filter is judged by hash, and a hash covers a node's + // contents rather than its position, so this is where a leaf that belongs elsewhere + // enters the map. Judged before canonicalizeChild, after which every later walk would + // see it as part of the tree. + // + // The map is the verdict rather than the node, because refusing one node would only + // make the walk fetch the same thing again: the filter answers from a local cache, so + // the next attempt resolves the same blob to the same place. + // + // Note what this does NOT rest on. checkFilter parses the blob but does not recompute + // its hash: makeFromPrefix passes hashValid = true and the makers adopt the hash they + // are handed. So the position is judged here, and the content is taken on trust from + // the filter. + JLOG(journal_.warn()) << "Leaf " << childHash << " does not belong at " << childID + << ", map is invalid"; + state_ = SHAMapState::Invalid; + return std::make_pair(nullptr, std::move(childID)); + } + if (childNode) { childNode = parent->canonicalizeChild(branch, std::move(childNode)); @@ -370,7 +430,7 @@ SHAMap::descend( } } - return std::make_pair(child, parentID.getChildNodeID(branch)); + return std::make_pair(child, std::move(childID)); } SHAMapTreeNode* @@ -436,6 +496,10 @@ SHAMapLeafNode* SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack input"); + if (stack.empty()) + { + return nullptr; + } if (auto const& top = stack.top().first; top->isLeaf()) return safeDowncast(top.get()); @@ -455,7 +519,30 @@ SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const continue; } - stack.pushChild(descendThrow(*inner, childBranch), childBranch); + auto descended = descendThrow(*inner, childBranch); + if (!stack.pushChild(std::move(descended), childBranch)) + { + // A refused push means the map holds a node that cannot be walked, which is a different + // thing from a subtree with no leaf below it. Throwing keeps nullptr meaning only the + // latter, so peekFirstItem cannot report such a map as empty while peekNextItem throws + // on the same condition. descendThrow above already throws this, so every caller of + // belowHelper already handles it. + // + // The node named below is resident rather than missing, so this exception is a poor + // description of what happened. It is still the right one to throw: every caller of + // belowHelper already handles it, and the alternative is the silent empty map above. + // + // The map is deliberately NOT condemned here. belowHelper is reached from begin(), + // upperBound() and lowerBound(), which are const, run on immutable ledger snapshots + // and are called from several RPC threads at once, while no reader anywhere checks + // isValid() -- every caller that does is on the acquisition path. So the write would + // buy nothing, would race those other readers, and would make a later compare() abort + // on its own isValid() assertion. A map assembled from peer data is judged where it is + // assembled instead (see SHAMap::descend and gmnProcessNodes). + JLOG(journal_.warn()) << "Cannot walk below " << stack.top().second << " at branch " + << childBranch; + Throw(type_, inner->getChildHash(childBranch)); + } auto const& child = stack.top().first; if (child->isLeaf()) @@ -512,10 +599,15 @@ SHAMapLeafNode const* SHAMap::peekFirstItem(NodePathStack& stack) const { XRPL_ASSERT(stack.empty(), "xrpl::SHAMap::peekFirstItem : empty stack input"); - stack.pushRoot(root_); + if (!stack.pushRoot(root_)) + { + return nullptr; + } SHAMapLeafNode const* node = belowHelper(stack, BelowDirection::First); if (node == nullptr) { + // Whether the map was empty or belowHelper's walk otherwise failed to find a leaf, the + // stack is cleared rather than left holding a partial path the caller cannot use. stack.clear(); return nullptr; } @@ -526,6 +618,10 @@ SHAMapLeafNode const* SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::peekNextItem : non-empty stack input"); + if (stack.empty()) + { + return nullptr; + } XRPL_ASSERT(stack.top().first->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf"); stack.pop(); while (!stack.empty()) @@ -537,7 +633,11 @@ SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const { if (!inner.isEmptyBranch(i)) { - stack.pushChild(descendThrow(inner, i), i); + auto child = descendThrow(inner, i); + if (!stack.pushChild(std::move(child), i)) + { + Throw(type_, id); + } auto leaf = belowHelper(stack, BelowDirection::First); if (leaf == nullptr) Throw(type_, id); @@ -606,7 +706,11 @@ SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const if (inner.isEmptyBranch(branch)) continue; - stack.pushChild(descendThrow(inner, branch), branch); + auto child = descendThrow(inner, branch); + if (!stack.pushChild(std::move(child), branch)) + { + Throw(type_, id); + } auto const leaf = belowHelper(stack, direction); if (leaf == nullptr) Throw(type_, id); @@ -768,7 +872,10 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key()))) { - stack.pushNode(node, tag); + if (!stack.pushNode(node, tag)) + { + Throw(type_, tag); + } // we need a new inner node, since both go on same branch at this // level diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 602d8e629c..bdfc251155 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -238,6 +238,30 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se) if (--mn.max <= 0) return; } + // The depth is tested first so getChildNodeID is only asked for a child that can exist. + // A node already at kLeafDepth has no branch left, and the missing-node case below + // reaches that the same way, so this adds no throw of its own. + else if ( + nodeID.getDepth() < kLeafDepth && !belongsAt(nodeID.getChildNodeID(branch), *d)) + { + // The same judgment SHAMap::descend makes, for the path that consults the filter + // through descendAsync instead. descendAsync hooks what it resolves, so the node is + // already part of the tree and refusing it here would not remove it. + // + // The verdict belongs to the map for the reason given in SHAMap::descend, which + // also records what this does not rest on. + // + // `fullBelow` is cleared first, as on the missing-node path above. It is a + // reference into the caller's stack entry, and this node is left on that stack, so + // a later pass over its remaining branches would otherwise reach the full-below + // test with it still set and record this subtree's hash as complete in the + // family-wide cache, where another map would trust it. + JLOG(journal_.warn()) << "Leaf " << childHash << " does not belong below " << nodeID + << " at branch " << branch << ", map is invalid"; + fullBelow = false; + state_ = SHAMapState::Invalid; + return; + } else if (d->isInner() && !safeDowncast(d)->isFullBelow(mn.generation)) { mn.stack.push(se); @@ -291,6 +315,23 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn) auto nodePtr = std::get<3>(deferredNode); auto const& nodeHash = parent->getChildHash(branch); + if (nodePtr && !belongsAt(parentID.getChildNodeID(branch), *nodePtr)) + { + // The same judgment the two synchronous paths make (see SHAMap::descend and the + // descendAsync case in gmnProcessNodes), for a node an async read resolved. Every site + // that knows the position a node is about to take judges it here, which is what lets + // the traversal treat a misplaced leaf as a rarity rather than a routine case. + // + // Skips this node rather than returning: the reads still outstanding hold a pointer to + // `mn`, which lives in getMissingNodes' frame, and this loop is the only thing that + // waits for them. Returning early would let that frame go while a read was still due + // to write through it. + JLOG(journal_.warn()) << "Leaf " << nodeHash << " does not belong below " << parentID + << " at branch " << branch << ", map is invalid"; + state_ = SHAMapState::Invalid; + continue; + } + if (nodePtr) { // Got the node nodePtr = parent->canonicalizeChild(branch, std::move(nodePtr)); @@ -416,7 +457,11 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) } while (node != nullptr); - if (mn.missingNodes.empty()) + // An empty result does not mean the map is complete when the walk judged it impossible on the + // way down: clearSynching() moves the state to Modifying, which would erase that verdict and + // report the map as satisfied. Asking nothing is the only part this has to get right, since + // clearSynching() is what a later walk would read. + if (mn.missingNodes.empty() && isValid()) clearSynching(); return std::move(mn.missingNodes); @@ -606,6 +651,17 @@ SHAMap::addKnownNode( auto prevNode = inner; std::tie(currNode, currNodeID) = descend(inner, currNodeID, branch, filter); + if (!isValid()) + { + // descend judged a node on the way down and condemned the map. Stops here rather than + // falling through, for two reasons: `childHash` was read before that descent, so the + // hash comparison below would report a corrupt node against a sender that sent nothing + // wrong, and if the node descend refused is the one offered here, that comparison would + // instead succeed and hook it after all. + JLOG(journal_.warn()) << "Node " << nodeID << " cannot be hooked into an invalid map"; + return SHAMapAddNode::invalid(); + } + if (currNode != nullptr) continue; diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index abceade814..1dd82cfcbe 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -8,11 +8,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include @@ -23,7 +25,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -273,8 +277,8 @@ INSTANTIATE_TEST_SUITE_P( shamapBackingModeName); // Exercises the traversal stacks built by belowHelper. Each stack entry pairs a node with the ID -// naming its position, and SHAMap asserts that pairing on every push, so these traversals fail -// loudly in a Debug build if a node ID is ever derived from the wrong branch. +// naming its position, and SHAMap enforces that pairing on every push, failing if a node ID is +// ever derived from the wrong branch (both Debug and Release builds). class SHAMapTraversal : public ::testing::Test { protected: @@ -947,4 +951,278 @@ TEST_F(SHAMapPathProof, substituted_leaf_for_other_key_is_rejected) EXPECT_FALSE(SHAMap::verifyProofPath(badRoot, kKey, badPath)); } +/** + * A filter that resolves exactly one node, by hash. + * + * Stands in for the real sync filters, which serve a node from a local cache keyed on its hash and + * so say nothing about where in a tree it belongs. + */ +class OneNodeFilter : public SHAMapSyncFilter +{ + std::map nodes_; + +public: + OneNodeFilter(SHAMapHash const& hash, Blob blob) + { + nodes_.emplace(hash, std::move(blob)); + } + + explicit OneNodeFilter(std::vector> nodes) + { + for (auto& [hash, blob] : nodes) + nodes_.emplace(hash, std::move(blob)); + } + + void + gotNode( + bool, + SHAMapHash const&, + std::uint32_t, + Blob&&, // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved) + SHAMapNodeType) const override + { + } + + [[nodiscard]] std::optional + getNode(SHAMapHash const& hash) const override + { + if (auto const it = nodes_.find(hash); it != nodes_.end()) + return it->second; + return std::nullopt; + } +}; + +// A tree whose hashes all agree can still put a leaf where its key does not belong, because a hash +// covers a node's contents rather than its position. Such a tree is what a proposer builds, and it +// is accepted node by node, so the paths that hook a node are where the position has to be judged. +class SHAMapMisplacedLeaf : public ::testing::Test +{ +protected: + beast::Journal const j_{TestSink::instance()}; + + // An arbitrary key whose first nibble is 1, so its leaf belongs under branch 1 of the root. + static constexpr uint256 kKey{ + "1c8cec8e5e9b0e5e0e0f5b3e2c9f7a1d6b4e8c2a0d7f3b9e5c1a8d4f2b6e0c93"}; + + // Any branch other than the one kKey selects at depth 0. + static constexpr unsigned int kWrongBranch = 5; + + /** + * A genuine leaf holding kKey, in the form a sync filter serves, with its hash. + * + * Taken from a map that placed the leaf correctly, so only its position is ever wrong below. + * Serialized with its prefix rather than in wire form, since that is what checkFilter parses. + * + * @param f the family the throwaway source map belongs to. + * @return the leaf's prefixed form and its hash, or an empty blob if the map rejected the item. + */ + static std::pair + genuineLeaf(Family& f) + { + SHAMap source{SHAMapType::FREE, f}; + source.setUnbacked(); + if (!source.addItem( + SHAMapNodeType::TnAccountState, + makeShamapitem(kKey, Slice{kKey.data(), kKey.size()}))) + { + return {}; + } + + auto const path = source.getProofPath(kKey); + if (!path.has_value() || path->empty()) + return {}; + + auto leaf = SHAMapTreeNode::makeFromWire(makeSlice(path->front())); + if (!leaf || !leaf->isLeaf()) + return {}; + leaf->updateHash(); + + Serializer s; + leaf->serializeWithPrefix(s); + return {s.getData(), leaf->getHash()}; + } + + /** + * Assemble `map` as a root inner node holding a leaf's hash under the wrong branch. + * + * The root is installed directly, as a peer's would be, so the leaf itself stays unresolved + * until a walk consults the filter for it. + * + * @param map the map to assemble, which must be synching and empty. + * @param leafHash the hash the forged root records under kWrongBranch. + * @return whether the root was accepted. + */ + static bool + forgeRoot(SHAMap& map, SHAMapHash const& leafHash) + { + Serializer s; + for (auto i = 0u; i < SHAMap::kBranchFactor; ++i) + s.addBitString(i == kWrongBranch ? leafHash.asUInt256() : uint256{}); + s.add8(kWireTypeInner); + + auto root = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData())); + if (!root) + return false; + root->updateHash(); + + auto const rootHash = root->getHash(); + return map.addRootNode(rootHash, std::move(root), nullptr).isGood(); + } +}; + +// getMissingNodes reaches a filter through descendAsync, which hooks whatever it resolves. The +// verdict lands on the map, since every node from the root down hash-verified to get here. +TEST_F(SHAMapMisplacedLeaf, walking_for_missing_nodes_invalidates_the_map) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + // Its own family, so the leaf is reachable only through the filter rather than from a cache the + // source map warmed. + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + ASSERT_TRUE(map.isValid()); + + OneNodeFilter const filter{leafHash, leafBlob}; + map.getMissingNodes(1, &filter); + + EXPECT_FALSE(map.isValid()); +} + +// addKnownNode reaches a filter through the synchronous descend on its way to the position it was +// given, which is the other route a node takes into a tree during acquisition. +TEST_F(SHAMapMisplacedLeaf, hooking_a_known_node_invalidates_the_map) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + ASSERT_TRUE(map.isValid()); + + // A key whose first nibble is kWrongBranch, so the walk descends the branch holding the leaf. + // An inner node is offered rather than a leaf, since a leaf would have to agree with this + // position and the point here is to reach the descent, not to hook what is offered. + auto const target = SHAMapNodeID::createID( + 2, uint256{"5000000000000000000000000000000000000000000000000000000000000000"}); + + Serializer s; + for (auto i = 0u; i < SHAMap::kBranchFactor; ++i) + s.addBitString(i == 0u ? uint256{1} : uint256{}); + s.add8(kWireTypeInner); + auto offered = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData())); + ASSERT_TRUE(offered); + offered->updateHash(); + + OneNodeFilter const filter{leafHash, leafBlob}; + auto const result = map.addKnownNode(target, std::move(offered), &filter); + + EXPECT_FALSE(map.isValid()); + + // The verdict matters as much as the state: it is what the acquisition paths charge a peer on, + // so a later change to it should fail here rather than pass quietly. + EXPECT_TRUE(result.isInvalid()); + EXPECT_FALSE(result.isGood()); +} + +// A whole subtree can sit under the wrong branch through a single wrong child pointer, and that is +// cheaper to produce than one misplaced leaf. Every leaf below such a subtree agrees with its own +// final branch, because the subtree is internally well formed, and disagrees only at the level the +// pointer is wrong. So judging a leaf against the last branch alone accepts all of them, and only +// judging it against every branch above it refuses them. +TEST_F(SHAMapMisplacedLeaf, iterating_a_misplaced_subtree_throws) +{ + // Two keys sharing their first nibble, so they hang off one inner node at depth 1. + constexpr uint256 kFirst{"a100000000000000000000000000000000000000000000000000000000000000"}; + constexpr uint256 kSecond{"a200000000000000000000000000000000000000000000000000000000000000"}; + + tests::TestNodeFamily sourceFamily{j_}; + SHAMap source{SHAMapType::FREE, sourceFamily}; + source.setUnbacked(); + for (auto const& k : {kFirst, kSecond}) + { + ASSERT_TRUE(source.addItem( + SHAMapNodeType::TnAccountState, makeShamapitem(k, Slice{k.data(), k.size()}))); + } + source.invariants(); + + // The inner node holding both leaves, as the filter will serve it. It belongs under branch 10, + // the nibble the two keys share, and the forged root below files it under kWrongBranch instead. + auto const subtree = source.getProofPath(kFirst); + ASSERT_TRUE(subtree.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) has_value() checked above + ASSERT_GE(subtree->size(), 2u); + + // getProofPath returns the path deepest element first, so the element above the leaf is the + // inner node the two keys share. + auto inner = SHAMapTreeNode::makeFromWire(makeSlice((*subtree)[1])); + // NOLINTEND(bugprone-unchecked-optional-access) + ASSERT_TRUE(inner); + ASSERT_TRUE(inner->isInner()); + inner->updateHash(); + + Serializer innerPrefixed; + inner->serializeWithPrefix(innerPrefixed); + + // Both leaves are served as well. Without them the walk would stop on a node it genuinely does + // not have, and the throw below would say nothing about position. + std::vector> served; + served.emplace_back(inner->getHash(), innerPrefixed.getData()); + for (auto const& k : {kFirst, kSecond}) + { + auto const leafPath = source.getProofPath(k); + ASSERT_TRUE(leafPath.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) has_value() checked above + ASSERT_FALSE(leafPath->empty()); + auto leaf = SHAMapTreeNode::makeFromWire(makeSlice(leafPath->front())); + // NOLINTEND(bugprone-unchecked-optional-access) + ASSERT_TRUE(leaf); + ASSERT_TRUE(leaf->isLeaf()); + leaf->updateHash(); + + Serializer leafPrefixed; + leaf->serializeWithPrefix(leafPrefixed); + served.emplace_back(leaf->getHash(), leafPrefixed.getData()); + } + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, inner->getHash())); + + // The inner node itself carries no key, so nothing about it is out of place. Only a leaf below + // it can show that the branch it was reached through disagrees with the keys underneath. + OneNodeFilter const filter{std::move(served)}; + map.getMissingNodes(4, &filter); + + EXPECT_THROW(map.begin(), SHAMapMissingNode); +} + +// The descendAsync walk leaves the leaf hooked, since it resolved the node before the position +// could be judged. Iterating it must not abort an instrumented build, and must not report the map +// as empty either, which is what a plain nullptr from belowHelper would have meant. +TEST_F(SHAMapMisplacedLeaf, iterating_a_hooked_misplaced_leaf_throws) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + + OneNodeFilter const filter{leafHash, leafBlob}; + map.getMissingNodes(1, &filter); + ASSERT_FALSE(map.isValid()); + + EXPECT_THROW(map.begin(), SHAMapMissingNode); +} + } // namespace xrpl::tests