fix: Reject a misplaced leaf at entry, and fail closed on one that slips past

`NodePathStack`'s checks were `XRPL_ASSERT`s, which are stripped under `NDEBUG`,
so a release build walked on with a node and an ID that disagreed. Each one is
now a live test that refuses the push and lets the caller stop, since a
malformed map must not be walked and must not abort either.

The leaf-position check was `UNREACHABLE`, and that was wrong rather than merely
strict. A leaf enters a tree through paths that carry no `SHAMapNodeID` at all
(`descend(parent, branch)`, `descendThrow`, `descendNoStore`), so no upstream
check can exist for them and `pushChild` is the first place the position is
known again. It is therefore a live, non-aborting test, and every path that does
know the position now judges a node before hooking it: the two filter descents
and the deferred-read hook, each marking the map invalid the way `addKnownNode`
already did for an inner node too deep. `getMissingNodes` no longer calls
`clearSynching()` on such a map, which would have moved the state to `Modifying`
and erased that verdict. `gmnProcessDeferredReads` became non-static so it can
record it.

New `SHAMapMisplacedLeaf` tests build a tree whose hashes agree but whose leaf
sits under the wrong branch, and drive it in through each of the two acquisition
routes. Reverting the entry checks fails both; reverting only the `belowHelper`
throw leaves `map.begin()` reporting such a map as empty, which the third test
catches.

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<SHAMapTreeNodePtr, SHAMapNodeID> 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<SHAMapTreeNodePtr, SHAMapNodeID> 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

include/xrpl/shamap/SHAMapLeafNode.h

@@ -75,4 +75,22 @@ leafKey(SHAMapTreeNode const& node)
     return safeDowncast<SHAMapLeafNode const&>(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

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<SHAMapInnerNode&>(*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<SHAMapLeafNode*>(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<SHAMapLeafNode*>(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<SHAMapMissingNode>(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<SHAMapMissingNode>(type_, id);
+                }
                 auto leaf = belowHelper(stack, BelowDirection::First);
                 if (leaf == nullptr)
                     Throw<SHAMapMissingNode>(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<SHAMapMissingNode>(type_, id);
+                }
                 auto const leaf = belowHelper(stack, direction);
                 if (leaf == nullptr)
                     Throw<SHAMapMissingNode>(type_, id);
@@ -768,7 +872,10 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
 
         while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key())))
         {
-            stack.pushNode(node, tag);
+            if (!stack.pushNode(node, tag))
+            {
+                Throw<SHAMapMissingNode>(type_, tag);
+            }
 
             // we need a new inner node, since both go on same branch at this
             // level

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<SHAMapInnerNode*>(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;
 

src/tests/libxrpl/shamap/SHAMap.cpp

@@ -8,11 +8,13 @@
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/protocol/Serializer.h>
+#include <xrpl/shamap/Family.h>
 #include <xrpl/shamap/SHAMapInnerNode.h>
 #include <xrpl/shamap/SHAMapItem.h>
 #include <xrpl/shamap/SHAMapLeafNode.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapSyncFilter.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <gtest/gtest.h>
@@ -23,7 +25,9 @@
 #include <array>
 #include <cstddef>
 #include <cstdint>
+#include <map>
 #include <memory>
+#include <optional>
 #include <string>
 #include <string_view>
 #include <type_traits>
@@ -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<SHAMapHash, Blob> nodes_;
+
+public:
+    OneNodeFilter(SHAMapHash const& hash, Blob blob)
+    {
+        nodes_.emplace(hash, std::move(blob));
+    }
+
+    explicit OneNodeFilter(std::vector<std::pair<SHAMapHash, Blob>> 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<Blob>
+    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<Blob, SHAMapHash>
+    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<std::pair<SHAMapHash, Blob>> 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