fix: Reject a misplaced leaf at entry, fail closed if one slips past

`NodePathStack`'s position and depth checks were `XRPL_ASSERT_IF`s, which are
stripped under `NDEBUG`, so a release build walked on with a node sitting where
it did not belong. Each is now a live test that refuses the push and lets the
caller stop, and each reports `SOMETIMES` rather than `UNREACHABLE`: a node
resolved from the local store reaches a walk through `descend(parent, branch)`,
which fetches by the parent's recorded child hash and judges neither position nor
type, so external data can reach either case and neither may abort a build.

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. `getMissingNodes` no longer calls
`clearSynching()` on a map it has condemned, at either of its two returns, since
that would move the state to `Modifying` and erase the verdict.
`gmnProcessDeferredReads` became non-static so it can record one.

`boundHelper` now throws where it used to answer `end()`. An empty map still
leaves its root on the path, so an empty path means only that a node was refused,
while `end()` is the positive claim that no key lies on the requested side of the
key asked for. New `SHAMapMisplacedLeaf` tests build a tree whose hashes agree
but whose leaf sits under the wrong branch, drive it in through both acquisition
routes, and check that iteration and both bounds refuse it.

include/xrpl/shamap/SHAMap.h

@@ -420,6 +420,28 @@ public:
     invariants() const;
 
 private:
+    /**
+     * Whether placing `node` one level below `parentDepth` leaves it no room.
+     *
+     * Only a leaf may sit at kLeafDepth, since an inner node there would have
+     * no branch left to select. Both of the places that bound a descent call
+     * this, so a walk with a caller-supplied path and one without cannot drift
+     * apart and refuse at different nodes.
+     *
+     * The depth is tested before the node's type so the virtual call runs only
+     * where the bound can bite, which is the last level of a 65-level walk.
+     *
+     * @param parentDepth the depth of the node being descended from.
+     * @param node the node about to be placed one level below it.
+     * @return whether that placement is past the deepest level this kind of
+     *         node may occupy.
+     */
+    [[nodiscard]] static bool
+    pastLeafDepth(unsigned int parentDepth, SHAMapTreeNode const& node)
+    {
+        return parentDepth + 1u >= kLeafDepth && (node.isInner() || parentDepth >= kLeafDepth);
+    }
+
     /**
      * A path from the root of the map down to some node, pairing each node with the ID naming its
      * position.
@@ -444,20 +466,53 @@ 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();
         }
 
+        /**
+         * Shorten the path by one node.
+         *
+         * Popping an empty path would be undefined, and the assert alone is
+         * stripped in release, so an empty path is left alone instead.
+         */
         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 +521,23 @@ 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 +545,65 @@ 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.
+         *
+         * @param node the child to append.
+         * @param branch the branch of the current node that `node` was
+         *               reached through.
+         * @return false, leaving the path unchanged, if there is no node to
+         *         descend from, no node to push, no branch of that number, no
+         *         room left below for the kind of node offered, or a leaf
+         *         whose own key does not lie under `branch`. A malformed call
+         *         or 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.
+            //
+            // Reachable, for the same reason the misplaced-leaf case below is: a node resolved from
+            // the local store has had neither its position nor its type judged. The two-argument
+            // SHAMap::descend fetches by the parent's recorded child hash and hooks what comes
+            // back, and a parsed node adopts that hash rather than recomputing it, so an inner node
+            // can arrive one level too deep. So this refuses rather than aborting an instrumented
+            // build.
+            //
+            auto const& parentID = stack_.top().second;
+            auto const parentDepth = parentID.getDepth();
+            bool const tooDeep = pastLeafDepth(parentDepth, *node);
+            SOMETIMES(tooDeep, "xrpl::SHAMap::NodePathStack::pushChild : child past leaf depth");
+            if (tooDeep)
+            {
+                return false;
+            }
+
+            // 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, for the reason given above: the paths that hook a node from a peer
+            // reject a misplaced one first (see SHAMap::descend and SHAMap::gmnProcessNodes), but a
+            // map read lazily from the local store never passes through them.
+            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 +612,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:
@@ -588,10 +693,28 @@ private:
     /**
      * Returns the first or last item at or below the node already on top of `stack`, extending
      * `stack` with the path walked to reach it.
+     *
+     * @param stack the path to extend, whose last node the search starts from.
+     * @param direction whether to take the lowest or the highest branch at
+     *                  each level.
+     * @return the leaf found, or nullptr if no leaf lies below that node.
      */
     SHAMapLeafNode*
     belowHelper(NodePathStack& stack, BelowDirection direction) const;
 
+    /**
+     * The nearest item on one side of `id`, which upperBound and lowerBound
+     * both answer.
+     *
+     * @param id the key to search around, which need not be in the map.
+     * @param direction First for the nearest key greater than `id`, Last for
+     *                  the nearest lesser.
+     * @return an iterator at that item, or end() if the map holds no key on
+     *         that side.
+     */
+    [[nodiscard]] ConstIterator
+    boundHelper(uint256 const& id, BelowDirection direction) const;
+
     // Simple descent
     // Get a child of the specified node
     SHAMapTreeNode*
@@ -714,10 +837,30 @@ private:
     };
 
     // getMissingNodes helper functions
+
+    /**
+     * Examine the remaining branches of one inner node, recording or
+     * requesting what is missing.
+     *
+     * @param mn the walk's shared state, which collects the missing nodes.
+     * @param node the walk's current position, updated to the node to process
+     *             next.
+     */
     void
-    gmnProcessNodes(MissingNodes&, MissingNodes::StackEntry& node);
-    static void
-    gmnProcessDeferredReads(MissingNodes&);
+    gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& node);
+
+    /**
+     * Wait for every read this pass posted, then hook up or record what each
+     * one resolved.
+     *
+     * Drains all of them even after judging the map, since an outstanding
+     * read holds a pointer to `mn` and this is the only thing that waits for
+     * it.
+     *
+     * @param mn the walk's shared state, holding the posted reads.
+     */
+    void
+    gmnProcessDeferredReads(MissingNodes& mn);
 
     // fetch from DB helper function
     SHAMapTreeNodePtr

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,69 @@ 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 : non-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 push fails when the map is malformed, by
+    // holding a leaf outside the branch it was reached through or a node with no room left below
+    // it, not because `id` is merely absent; the stack is cleared rather than left holding a node
+    // that never became a real path entry. Callers tell the two apart by the path, which is empty
+    // only in the first case.
+    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)
+        {
+            // Shares pastLeafDepth with pushChild, so this mode and the one with a
+            // caller-supplied path refuse at the same node. Reachable for the reason that helper
+            // gives, so it refuses rather than aborts.
+            auto const depth = nodeID.getDepth();
+            bool const tooDeep = pastLeafDepth(depth, *inNode);
+            SOMETIMES(tooDeep, "xrpl::SHAMap::walkTowardsKey : child too deep");
+            if (tooDeep)
+            {
+                return nullptr;
+            }
+            nodeID = nodeID.getChildNodeID(branch);
+        }
     }
 
-    pushCurrent();
+    if (!pushCurrent())
+    {
+        return nullptr;
+    }
     return safeDowncast<SHAMapLeafNode*>(inNode.get());
 }
 
@@ -357,12 +394,29 @@ 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.
+            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 +424,7 @@ SHAMap::descend(
         }
     }
 
-    return std::make_pair(child, parentID.getChildNodeID(branch));
+    return std::make_pair(child, std::move(childID));
 }
 
 SHAMapTreeNode*
@@ -436,6 +490,12 @@ SHAMapLeafNode*
 SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const
 {
     XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack input");
+    if (stack.empty())
+    {
+        // LCOV_EXCL_START
+        return nullptr;
+        // LCOV_EXCL_STOP
+    }
     if (auto const& top = stack.top().first; top->isLeaf())
         return safeDowncast<SHAMapLeafNode*>(top.get());
 
@@ -455,7 +515,25 @@ 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 not the
+            // same as a subtree with no leaf below it. Throwing keeps nullptr meaning only the
+            // latter, so begin() cannot report such a map as empty while an iterator increment
+            // throws on the same condition. SHAMapMissingNode describes a resident node poorly,
+            // but descendThrow above throws it too, so every caller already handles it.
+            //
+            // The map is deliberately NOT condemned here. Every caller of belowHelper is a const
+            // read on an immutable snapshot, called from several RPC threads at once, and no
+            // reader checks isValid(); the callers that do are on the acquisition path. So the
+            // write would buy nothing, would race those readers, and would make a later compare()
+            // trip its own isValid() assertion. A map from peer data is judged where it is
+            // assembled (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 +590,17 @@ SHAMapLeafNode const*
 SHAMap::peekFirstItem(NodePathStack& stack) const
 {
     XRPL_ASSERT(stack.empty(), "xrpl::SHAMap::peekFirstItem : empty stack input");
-    stack.pushRoot(root_);
+    if (!stack.pushRoot(root_))
+    {
+        // LCOV_EXCL_START
+        return nullptr;
+        // LCOV_EXCL_STOP
+    }
     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 +611,12 @@ SHAMapLeafNode const*
 SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const
 {
     XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::peekNextItem : non-empty stack input");
+    if (stack.empty())
+    {
+        // LCOV_EXCL_START
+        return nullptr;
+        // LCOV_EXCL_STOP
+    }
     XRPL_ASSERT(stack.top().first->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf");
     stack.pop();
     while (!stack.empty())
@@ -537,7 +628,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);
@@ -575,72 +670,72 @@ SHAMap::peekItem(uint256 const& id, SHAMapHash& hash) const
 }
 
 SHAMap::ConstIterator
-SHAMap::upperBound(uint256 const& id) const
+SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const
 {
+    // Walk back up the path to `id` looking for the nearest leaf on the requested side. At each
+    // inner node the candidates are the branches on that side of the one `id` takes: the higher
+    // ones searching forward, the lower ones searching back. The nearest non-empty candidate holds
+    // the answer, which is its lowest leaf searching forward and its highest searching back.
+    auto const searchingForward = direction == BelowDirection::First;
+
     NodePathStack stack;
     walkTowardsKey(id, &stack);
+
+    // An empty path means the walk refused a node, not that the map is empty: an empty map still
+    // leaves its root on the path. end() is the positive claim that no key lies on the requested
+    // side of `id`, so it must not stand in for "cannot answer", which is what every other entry
+    // point reports by throwing.
+    if (stack.empty())
+        Throw<SHAMapMissingNode>(type_, id);
+
     while (!stack.empty())
     {
         auto const [node, nodeID] = stack.top();
         if (node->isLeaf())
         {
-            auto leaf = safeDowncast<SHAMapLeafNode*>(node.get());
-            if (leaf->peekItem()->key() > id)
-                return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
+            auto const& item = safeDowncast<SHAMapLeafNode const&>(*node).peekItem();
+            if (searchingForward ? (item->key() > id) : (item->key() < id))
+                return ConstIterator(this, item.get(), std::move(stack));
         }
         else
         {
             auto& inner = safeDowncast<SHAMapInnerNode&>(*node);
-            for (auto branch = selectBranch(nodeID, id) + 1; branch < kBranchFactor; ++branch)
+            auto const taken = selectBranch(nodeID, id);
+            auto const remaining = searchingForward ? (kBranchFactor - 1u - taken) : taken;
+
+            for (auto scanned = 0u; scanned < remaining; ++scanned)
             {
-                if (!inner.isEmptyBranch(branch))
+                auto const branch =
+                    searchingForward ? (taken + 1u + scanned) : (taken - 1u - scanned);
+                if (inner.isEmptyBranch(branch))
+                    continue;
+
+                auto child = descendThrow(inner, branch);
+                if (!stack.pushChild(std::move(child), branch))
                 {
-                    stack.pushChild(descendThrow(inner, branch), branch);
-                    auto leaf = belowHelper(stack, BelowDirection::First);
-                    if (leaf == nullptr)
-                        Throw<SHAMapMissingNode>(type_, id);
-                    return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
+                    Throw<SHAMapMissingNode>(type_, id);
                 }
+                auto const leaf = belowHelper(stack, direction);
+                if (leaf == nullptr)
+                    Throw<SHAMapMissingNode>(type_, id);
+                return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
             }
         }
         stack.pop();
     }
     return end();
 }
+
+SHAMap::ConstIterator
+SHAMap::upperBound(uint256 const& id) const
+{
+    return boundHelper(id, BelowDirection::First);
+}
+
 SHAMap::ConstIterator
 SHAMap::lowerBound(uint256 const& id) const
 {
-    NodePathStack stack;
-    walkTowardsKey(id, &stack);
-    while (!stack.empty())
-    {
-        auto const [node, nodeID] = stack.top();
-        if (node->isLeaf())
-        {
-            auto leaf = safeDowncast<SHAMapLeafNode*>(node.get());
-            if (leaf->peekItem()->key() < id)
-                return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
-        }
-        else
-        {
-            auto& inner = safeDowncast<SHAMapInnerNode&>(*node);
-            for (auto branch = selectBranch(nodeID, id); branch > 0u;)
-            {
-                --branch;
-                if (!inner.isEmptyBranch(branch))
-                {
-                    stack.pushChild(descendThrow(inner, branch), branch);
-                    auto leaf = belowHelper(stack, BelowDirection::Last);
-                    if (leaf == nullptr)
-                        Throw<SHAMapMissingNode>(type_, id);
-                    return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
-                }
-            }
-        }
-        stack.pop();
-    }
-    // TODO: what to return here?
-    return end();
+    return boundHelper(id, BelowDirection::Last);
 }
 
 bool
@@ -781,7 +876,16 @@ 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))
+            {
+                // The node pushed here is freshly made and inner, so only the depth bound could
+                // refuse it, and the loop cannot reach that bound: it advances only while the two
+                // keys agree at the current nibble, and keys agreeing at all 64 nibbles are equal,
+                // which the caller already returned false for.
+                // LCOV_EXCL_START
+                Throw<SHAMapMissingNode>(type_, tag);
+                // LCOV_EXCL_STOP
+            }
 
             // we need a new inner node, since both go on same branch at this
             // level

src/libxrpl/shamap/SHAMapSync.cpp

@@ -238,6 +238,28 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
                 if (--mn.max <= 0)
                     return;
             }
+            // Only a leaf has a position of its own to judge, so the type is tested first: that
+            // also keeps getChildNodeID, which builds a SHAMapNodeID, off every inner child on the
+            // walk. The depth is tested next so the ID is only asked for a child that can exist.
+            else if (
+                d->isLeaf() && 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.
+                //
+                // `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 +313,29 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn)
         auto nodePtr = std::get<3>(deferredNode);
         auto const& nodeHash = parent->getChildHash(branch);
 
+        // Guarded on depth for the same reason as the sibling test in gmnProcessNodes: a deferred
+        // entry carries the position the walk held when it posted the read, and the `pending`
+        // branch there records that position without building a child ID from it. So a child ID is
+        // asked for here only where the tree has room for one, which is the bound getChildNodeID
+        // keeps for itself.
+        if (nodePtr && nodePtr->isLeaf() && parentID.getDepth() < kLeafDepth &&
+            !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));
@@ -328,10 +373,15 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
         512,  // number of async reads per pass
         f_.getFullBelowCache()->getGeneration());
 
+    // Guarded with isValid() for the same reason the late return below is: clearSynching() moves
+    // the state to Modifying, which would erase a verdict an earlier walk already reached. No path
+    // to that was found, since every site that condemns the map also clears the fullBelow flag this
+    // return reads, but the rule holds either way and one conjunct is what it costs.
     if (!root_->isInner() ||
         intr_ptr::staticPointerCast<SHAMapInnerNode>(root_)->isFullBelow(mn.generation))
     {
-        clearSynching();
+        if (isValid())
+            clearSynching();
         return std::move(mn.missingNodes);
     }
 
@@ -416,7 +466,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);
@@ -569,10 +623,6 @@ SHAMap::addKnownNode(
 {
     XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node");
     XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node");
-    XRPL_ASSERT_IF(
-        treeNode->isLeaf(),
-        nodeID.isPrefixOf(leafKey(*treeNode)),
-        "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID");
 
     if (!isSynching())
     {
@@ -606,6 +656,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;
 
@@ -637,6 +698,19 @@ SHAMap::addKnownNode(
             return SHAMapAddNode::useful();
         }
 
+        // A leaf's own key names its position, so a leaf offered for this slot has to agree with
+        // the ID it was offered under. The hash test above already proves the parent records this
+        // exact leaf here, so a disagreement is a property of the map rather than of the sender.
+        // This was an entry assertion, which is stripped under NDEBUG, and the node is hooked
+        // immediately below.
+        if (!belongsAt(nodeID, *treeNode))
+        {
+            JLOG(journal_.warn()) << "Leaf " << treeNode->getHash() << " does not belong at "
+                                  << nodeID << ", map is invalid";
+            state_ = SHAMapState::Invalid;
+            return SHAMapAddNode::invalid();
+        }
+
         if (backed_)
             canonicalize(childHash, treeNode);
 

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 every push refuses a leaf whose own key does not lie under the branch it
+// was reached through, in Release builds as well as Debug ones.
 class SHAMapTraversal : public ::testing::Test
 {
 protected:
@@ -455,6 +459,49 @@ TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys)
     }
 }
 
+TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end)
+{
+    tests::TestNodeFamily f{j_};
+    SHAMap map{SHAMapType::FREE, f};
+    map.setUnbacked();
+
+    // An empty map still leaves its root on the path, so end() here is an answer rather than a
+    // refusal. This is what stops boundHelper from reading an empty path as an empty map.
+    EXPECT_EQ(map.upperBound(uint256{}), map.end());
+    EXPECT_EQ(map.lowerBound(uint256{}), map.end());
+
+    uint256 probe;
+    std::fill_n(probe.begin(), probe.size(), std::uint8_t{0xff});
+    EXPECT_EQ(map.upperBound(probe), map.end());
+    EXPECT_EQ(map.lowerBound(probe), map.end());
+}
+
+TEST_F(SHAMapTraversal, bounds_on_single_item_map_use_the_leaf_below_the_root)
+{
+    tests::TestNodeFamily f{j_};
+    SHAMap map{SHAMapType::FREE, f};
+
+    auto const key = deepFanOutKeys().front();
+    fillMap(map, {key});
+
+    // fillMap uses addItem, which leaves root_ the inner node the map was constructed with and the
+    // single leaf one level below it. So the path holds both, and boundHelper judges the leaf
+    // first; for a probe the leaf does not qualify against it pops back to the root, whose scan
+    // finds nothing on the requested side.
+    uint256 below = key;
+    --below;
+    uint256 above = key;
+    ++above;
+
+    EXPECT_EQ(map.upperBound(below)->key(), key);
+    EXPECT_EQ(map.upperBound(key), map.end());
+    EXPECT_EQ(map.upperBound(above), map.end());
+
+    EXPECT_EQ(map.lowerBound(above)->key(), key);
+    EXPECT_EQ(map.lowerBound(key), map.end());
+    EXPECT_EQ(map.lowerBound(below), map.end());
+}
+
 TEST_F(SHAMapTraversal, iteration_survives_deletions)
 {
     tests::TestNodeFamily f{j_};
@@ -901,4 +948,327 @@ 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());
+}
+
+// addKnownNode also hooks the very node it was handed, on the path where the local store has
+// nothing to resolve for that slot. Such a node's position is known only from the ID the caller
+// supplied, so it is judged against the leaf's own key before it is hooked.
+TEST_F(SHAMapMisplacedLeaf, hooking_an_offered_misplaced_leaf_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());
+
+    // The branch the forged root files the leaf under, which is not the one kKey selects.
+    uint256 wrongPrefix;
+    wrongPrefix.begin()[0] = static_cast<std::uint8_t>(kWrongBranch << 4);
+    auto const target = SHAMapNodeID::createID(1, wrongPrefix);
+
+    auto offered = SHAMapTreeNode::makeFromPrefix(makeSlice(leafBlob), leafHash);
+    ASSERT_TRUE(offered);
+    ASSERT_TRUE(offered->isLeaf());
+
+    // No filter, so the walk resolves nothing locally and the node offered here is the one that
+    // would be hooked.
+    auto const result = map.addKnownNode(target, std::move(offered), nullptr);
+
+    EXPECT_FALSE(map.isValid());
+    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 bounds have to refuse the same map, and refusing is not the same as answering end().
+    // This probe selects kWrongBranch at depth 0 and then the branch holding kFirst, so the walk
+    // reaches the misplaced leaf and clears the path. Both keys in the map are greater than the
+    // probe, so end() here would be the positive and wrong claim that no greater key exists.
+    uint256 probe;
+    probe.begin()[0] = static_cast<std::uint8_t>((kWrongBranch << 4) | 0x1u);
+    ASSERT_GT(kFirst, probe);
+    ASSERT_GT(kSecond, probe);
+
+    EXPECT_THROW(map.upperBound(probe), SHAMapMissingNode);
+    EXPECT_THROW(map.lowerBound(probe), 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