test: Cover a childless inner node and share the sync test helpers

ServingFilter, in the new src/tests/libxrpl/shamap/SyncFilter.h, holds
the node-serving logic three sync filters each had a copy of.
FixedNodeFilter goes away, while RecordingFilter and ChainFilter derive
from it and keep only what is theirs. tallyIs(), in the new Tally.h,
replaces one copy per test tree. kFastRetry moves to file scope in
AcquireTestHelpers.h beside waitFor(). DeepChain::addOffendingNode()
becomes addRejectedNode(), the fixture helper forgeRoot() becomes
buildRoot(), and map_invalidating_node_is_judged_before_the_cache_is_read
becomes inner_node_at_leaf_depth_is_judged_before_the_cache_is_read, since
an inner node at leaf depth is the shape it judges.

The same commit covers the two throws a childless inner node reaches.
Serializing one asserts it has a branch, so the fixture assembles the
wire form and reads it back through the store. The map is well formed by
every position rule yet cannot be walked, and both entry points throw
rather than answer end(), the positive claim that no greater key exists.

include/xrpl/basics/IntrusivePointer.h

@@ -86,12 +86,15 @@ public:
         requires std::convertible_to<TT*, T*>
     SharedIntrusive(SharedIntrusive<TT> const& rhs);
 
-    SharedIntrusive(SharedIntrusive&& rhs);
+    // noexcept so move_if_noexcept moves rather than copies, which NodePathStack's path_ vector
+    // relies on when it relocates. The body is a std::exchange on a raw pointer, which is what
+    // makes the noexcept honest.
+    SharedIntrusive(SharedIntrusive&& rhs) noexcept;
 
     template <class TT>
         requires std::convertible_to<TT*, T*>
-    SharedIntrusive(
-        SharedIntrusive<TT>&& rhs);  // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved)
+    // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved)
+    SharedIntrusive(SharedIntrusive<TT>&& rhs) noexcept;
 
     SharedIntrusive&
     operator=(SharedIntrusive const& rhs);
@@ -529,6 +532,22 @@ staticPointerCast(TT const& v)
     return SharedPtr<T>(StaticCastTagSharedIntrusive{}, v);
 }
 
+/**
+ * Statically cast an intrusive pointer, moving out of it.
+ *
+ * Spelling the parameter as `SharedIntrusive<TT>&&` binds rvalues only and
+ * leaves lvalues to the `const&` overload above.
+ *
+ * @param v the pointer to cast, left empty afterwards.
+ * @return a pointer of the requested type to the same object.
+ */
+template <class T, class TT>
+[[nodiscard]] SharedPtr<T>
+staticPointerCast(SharedIntrusive<TT>&& v)
+{
+    return SharedPtr<T>(StaticCastTagSharedIntrusive{}, std::move(v));
+}
+
 template <class T, class TT>
 SharedPtr<T>
 dynamicPointerCast(TT const& v)

include/xrpl/basics/IntrusivePointer.ipp

@@ -47,7 +47,7 @@ SharedIntrusive<T>::SharedIntrusive(SharedIntrusive<TT> const& rhs)
 }
 
 template <class T>
-SharedIntrusive<T>::SharedIntrusive(SharedIntrusive&& rhs)
+SharedIntrusive<T>::SharedIntrusive(SharedIntrusive&& rhs) noexcept
     : ptr_{std::move(rhs).unsafeExchange(nullptr)}
 {
 }
@@ -55,7 +55,7 @@ SharedIntrusive<T>::SharedIntrusive(SharedIntrusive&& rhs)
 template <class T>
 template <class TT>
     requires std::convertible_to<TT*, T*>
-SharedIntrusive<T>::SharedIntrusive(SharedIntrusive<TT>&& rhs)
+SharedIntrusive<T>::SharedIntrusive(SharedIntrusive<TT>&& rhs) noexcept
     : ptr_{std::move(rhs).unsafeExchange(nullptr)}
 {
 }

include/xrpl/basics/KeyCache.h

@@ -1,10 +1,15 @@
 #pragma once
 
 #include <xrpl/basics/TaggedCache.h>
-#include <xrpl/basics/base_uint.h>
 
 namespace xrpl {
 
-using KeyCache = TaggedCache<UInt256, int, true>;
+/**
+ * TaggedCache in key-only mode, holding no value per key.
+ *
+ * @tparam Key the key type to remember.
+ */
+template <class Key>
+using KeyCache = TaggedCache<Key, int, true>;
 
 }  // namespace xrpl

include/xrpl/ledger/Ledger.h

@@ -105,6 +105,16 @@ public:
         std::vector<UInt256> const& amendments,
         Family& family);
 
+    /**
+     * Create a ledger from a header whose maps are filled in afterwards.
+     *
+     * Both maps start Synching against the hashes the header carries, which are
+     * input rather than derived, so setImmutable() leaves them alone.
+     *
+     * @param info The header to build from.
+     * @param rules The rules in force.
+     * @param family The SHAMap family the maps belong to.
+     */
     Ledger(LedgerHeader const& info, Rules rules, Family& family);
 
     /**
@@ -278,23 +288,25 @@ public:
     /**
      * Mark this ledger as immutable, so it can no longer be modified.
      *
-     * A ledger built or loaded locally cannot have an invalid map, since only
-     * a map syncing against hashes from outside can be proven impossible (see
-     * SHAMap::addKnownNode), so a caller on such a path may treat a false
-     * return as a broken internal invariant. A caller assembling a ledger from
-     * peer data may not: for it, false is an outcome a peer can produce.
+     * Only a map syncing against hashes from outside can be found unsound (see
+     * SHAMap::addKnownNode), so a caller whose ledger was built or loaded
+     * locally treats a false return as a broken internal invariant.
      *
      * @param rehash Whether to recompute the ledger hash from the header
      *        fields. The transaction and account hashes are recomputed from the
-     *        maps too, but only the first time.
+     *        maps too, but only the first time and only if the header did not
+     *        supply them.
      * @return false if either map is Invalid, leaving the immutable flag as it
-     *         was and the header untouched. A map invalidated partway through can
-     *         still leave the other one immutable, so a false return means the
-     *         ledger must be discarded rather than retried.
+     *         was and the header untouched. The ledger must then be discarded
+     *         rather than retried, since one map may already be immutable.
      */
     [[nodiscard]] bool
     setImmutable(bool rehash = true);
 
+    /**
+     * @return Whether both maps have been settled, so the ledger can no longer
+     *         change.
+     */
     bool
     isImmutable() const
     {
@@ -470,6 +482,15 @@ private:
 
     bool immutable_;
 
+    /**
+     * Whether the header supplied the transaction and account hashes, so they
+     * must not be derived from the maps.
+     *
+     * True only for a ledger built from a header, which stays verified against
+     * the hash it was asked for. Fixed at construction, unlike immutable_.
+     */
+    bool const mapHashesFromHeader_ = false;
+
     // A SHAMap containing the transactions associated with this ledger.
     SHAMap mutable txMap_;
 

include/xrpl/shamap/FullBelowCache.h

@@ -3,9 +3,12 @@
 #include <xrpl/basics/KeyCache.h>
 #include <xrpl/basics/TaggedCache.h>
 #include <xrpl/basics/base_uint.h>
+#include <xrpl/basics/partitioned_unordered_map.h>
+#include <xrpl/beast/hash/hash_append.h>
 #include <xrpl/beast/insight/Collector.h>
 #include <xrpl/beast/insight/NullCollector.h>
 #include <xrpl/beast/utility/Journal.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
 
 #include <atomic>
 #include <chrono>
@@ -15,6 +18,112 @@
 
 namespace xrpl {
 
+/**
+ * Names a subtree whose descendants are all resident, by hash and by position.
+ *
+ * A node's hash covers its children but neither its depth nor its path, and one
+ * cache answers for every map built from a NodeFamily, so the position is what
+ * makes a hit answer for the position asked about and no other. The depth is
+ * kept beside the id because SHAMapNodeID masks its id to its own depth, so an
+ * all-zero id belongs to the root and to every all-zero prefix alike.
+ *
+ * Held as two fields rather than as a SHAMapNodeID, which is a CountedObject:
+ * one per entry would report the cache's population as live node IDs in the
+ * get_counts RPC, and would put an atomic increment on every temporary key a
+ * lookup builds.
+ */
+struct FullBelowKey
+{
+    /**
+     * The hash of the subtree.
+     */
+    UInt256 hash;
+
+    /**
+     * The masked id of the node the subtree was completed at.
+     */
+    UInt256 position;
+
+    /**
+     * The depth of that node.
+     */
+    std::uint32_t depth{0};
+
+    /**
+     * @param subtreeHash The hash of the subtree whose descendants are all
+     *        resident.
+     * @param nodeID The node the subtree was completed at.
+     */
+    FullBelowKey(UInt256 const& subtreeHash, SHAMapNodeID const& nodeID)
+        : hash(subtreeHash)
+        , position(nodeID.getNodeID())
+        , depth(static_cast<std::uint32_t>(nodeID.getDepth()))
+    {
+    }
+
+    /**
+     * The same key, named by the node above the position and the branch taken.
+     *
+     * Produces the fields the constructor above would for that child's own node
+     * ID, so the two forms name one entry.
+     *
+     * @param subtreeHash The hash of the subtree that is fully resident.
+     * @param parentID The node above the one the subtree was completed at.
+     * @param branch The branch of that node leading to it.
+     */
+    FullBelowKey(UInt256 const& subtreeHash, SHAMapNodeID const& parentID, unsigned int branch)
+        : hash(subtreeHash)
+        , position(childNodeID(parentID.getNodeID(), parentID.getDepth(), branch))
+        , depth(static_cast<std::uint32_t>(parentID.getDepth() + 1))
+    {
+    }
+
+    [[nodiscard]] bool
+    operator==(FullBelowKey const& other) const = default;
+};
+
+// The per-entry cost of this cache is computed from these sizes, so pin the layout that arithmetic
+// assumes: the three fields sit back to back with no padding.
+static_assert(
+    sizeof(FullBelowKey) == (2 * sizeof(UInt256)) + sizeof(std::uint32_t),
+    "FullBelowKey must stay free of padding");
+static_assert(
+    alignof(FullBelowKey) == alignof(std::uint32_t),
+    "FullBelowKey's lack of padding rests on its alignment");
+
+/**
+ * Feed a key to a hasher, field by field.
+ *
+ * Written out rather than hashed in one call over the object's bytes, since
+ * beast::IsUniquelyRepresented is opt-in per type and has no case for a
+ * user-defined struct, whatever its layout.
+ *
+ * @param h The hasher to feed.
+ * @param key The key to hash.
+ */
+template <class Hasher>
+void
+hash_append(Hasher& h, FullBelowKey const& key) noexcept
+{
+    using beast::hash_append;
+    hash_append(h, key.hash, key.position, key.depth);
+}
+
+/**
+ * Choose which partition of the cache's map a key lives in.
+ *
+ * Taken from the subtree hash, which is already uniformly distributed.
+ *
+ * @param key The key to place.
+ * @return The value PartitionedUnorderedMap reduces by its partition count.
+ */
+template <>
+inline std::size_t
+extract(FullBelowKey const& key)
+{
+    return extract(key.hash);
+}
+
 namespace detail {
 
 /**
@@ -24,12 +133,12 @@ namespace detail {
 class BasicFullBelowCache
 {
 private:
-    using CacheType = KeyCache;
+    using CacheType = KeyCache<FullBelowKey>;
 
 public:
     static constexpr auto kDefaultCacheTargetSize = 0;
 
-    using key_type = UInt256;
+    using key_type = FullBelowKey;
     using ClockType = CacheType::ClockType;
 
     /**
@@ -86,13 +195,15 @@ public:
      * Refresh the last access time of an item, if it exists.
      * Thread safety:
      *     Safe to call from any thread.
-     * @param key The key to refresh.
-     * @return `true` If the key exists.
+     * @param hash The hash of the subtree to ask about.
+     * @param parentID The node above the position to ask about it at.
+     * @param branch The branch of that node leading to that position.
+     * @return `true` If that subtree is recorded as resident at that position.
      */
     bool
-    touchIfExists(key_type const& key)
+    touchIfExists(UInt256 const& hash, SHAMapNodeID const& parentID, unsigned int branch)
     {
-        return cache_.touchIfExists(key);
+        return cache_.touchIfExists(key_type{hash, parentID, branch});
     }
 
     /**
@@ -101,12 +212,13 @@ public:
      * be refreshed.
      * Thread safety:
      *     Safe to call from any thread.
-     * @param key The key to insert.
+     * @param hash The hash of the subtree whose descendants are all resident.
+     * @param nodeID The node that subtree was completed at.
      */
     void
-    insert(key_type const& key)
+    insert(UInt256 const& hash, SHAMapNodeID const& nodeID)
     {
-        cache_.insert(key);
+        cache_.insert(key_type{hash, nodeID});
     }
 
     /**

include/xrpl/shamap/README.md

@@ -278,11 +278,14 @@ This cache remembers which trie keys have all of their children resident in a
 when creating the missing nodes list. Missing nodes are those nodes that a
 `SHAMap` refers to but that are not stored in the local database.
 
-As a depth-first walk of a `SHAMap` is performed, if an inner node answers true to
-`isFullBelow()` then it is known that none of this node's children are missing
-nodes, and thus that subtree does not need to be walked. These nodes are stored
-in the FullBelowCache. Subsequent walks check the FullBelowCache first when
-encountering a node, and ignore that subtree if found.
+As a depth-first walk of a `SHAMap` completes an inner node, meaning none of the
+nodes below it are missing, the node's hash and position are stored in the
+FullBelowCache. Subsequent walks check the FullBelowCache when encountering a
+child, and skip that subtree if found. The entry is keyed by position as well as
+hash, since a hash commits to a node's contents rather than its place, and the
+same node object is shared by hash between maps and positions. For the same
+reason the inner node's own `isFullBelow()` flag is written and read for a
+walk's root alone, where the position is fixed.
 
 ## `SHAMapTreeNode`
 

include/xrpl/shamap/SHAMap.h

@@ -32,6 +32,7 @@
 #include <set>
 #include <stack>
 #include <tuple>
+#include <type_traits>
 #include <utility>
 #include <vector>
 
@@ -73,20 +74,21 @@ enum class SHAMapState : std::uint8_t {
 };
 
 /**
- * A SHAMap is both a radix tree with a fan-out of 16 and a Merkle tree.
+ * A SHAMap is both a trie with a fan-out of 16 and a Merkle tree.
  *
- * A radix tree is a tree with two properties:
+ * A trie holds a key in the position of its nodes: the path from the root down
+ * to a node spells out the prefix every key below it shares (the "prefix
+ * property"). A SHAMap spends one nibble of the 256-bit key per level, so an
+ * inner node has at most 16 children and a leaf sits at depth 64 at the
+ * deepest. A leaf also carries its own full key, so a reader can check that it
+ * was reached through the branches that key names.
  *
- *   1. The key for a node is represented by the node's position in the tree
- *      (the "prefix property").
- *   2. A node with only one child is merged with that child
- *      (the "merge property")
+ * An insert creates an inner node at every nibble two keys share, and a delete
+ * collapses a chain that reduces to one leaf. Every edge spans exactly one
+ * nibble, so a path has one entry per level and 65 entries at the most, and a
+ * path's length names each node's depth. Traversal relies on that.
  *
- * These properties result in a significantly smaller memory footprint for
- * a radix tree.
- *
- * A fan-out of 16 means that each node in the tree has at most 16
- * children. See https://en.wikipedia.org/wiki/Radix_tree
+ * See https://en.wikipedia.org/wiki/Trie
  *
  * A Merkle tree is a tree where each non-leaf node is labelled with the hash
  * of the combined labels of its children nodes.
@@ -131,21 +133,18 @@ private:
     /**
      * The sequence of the ledger that this map references, if any.
      *
-     * Written when a map's ledger sequence is established (Ledger::setFull(),
-     * InboundLedger) while a nodestore reader thread reads it. Relaxed either
-     * way: it serves as a lookup hint for a nodestore keyed by hash.
+     * Written by Ledger::setFull() and by InboundLedger, and read on a
+     * nodestore reader thread. Relaxed both ways, since it is only a lookup
+     * hint for a store keyed by hash.
      */
     std::atomic<std::uint32_t> ledgerSeq_ = 0;
 
     SHAMapTreeNodePtr root_;
 
     /**
-     * The map's state.
-     *
-     * A getMissingNodes() walk writes it, through setInvalid() and
-     * clearSynching(), while whatever drives the acquisition reads it.
-     * The walk runs with the acquisition's lock released, so this is atomic
-     * rather than guarded.
+     * The map's state. Atomic because a getMissingNodes() walk writes it with
+     * the acquisition's lock released. Mutable because a const descend() can
+     * record the Invalid verdict.
      */
     mutable std::atomic<SHAMapState> state_;
     SHAMapType const type_;
@@ -161,8 +160,7 @@ private:
 
 public:
     /**
-     * Number of children each non-leaf node has (the 'radix tree' part of the
-     * map)
+     * Number of children each non-leaf node has, which is the trie's fan-out
      */
     static constexpr unsigned int kBranchFactor = SHAMapInnerNode::kBranchFactor;
 
@@ -171,6 +169,18 @@ public:
      */
     static constexpr unsigned int kLeafDepth = 64;
 
+    /**
+     * Whether only a leaf may occupy a position at `depth`.
+     *
+     * @param depth the depth to judge.
+     * @return whether that depth is at or past kLeafDepth.
+     */
+    [[nodiscard]] static constexpr bool
+    isLeafDepth(unsigned int depth)
+    {
+        return depth >= kLeafDepth;
+    }
+
     using DeltaItem =
         std::pair<boost::intrusive_ptr<SHAMapItem const>, boost::intrusive_ptr<SHAMapItem const>>;
     using Delta = std::map<UInt256, DeltaItem>;
@@ -294,9 +304,11 @@ public:
     /**
      * Find the first item after the given item.
      *
-     * @param id the identifier of the item.
-     *
-     * @note The item does not need to exist.
+     * @param id the identifier of the item, which need not exist in the map.
+     * @return an iterator at the first item with a greater key, or end() if the
+     *         map holds no greater key.
+     * @throws SHAMapMissingNode if the map cannot be walked. end() is the
+     *         separate answer that the map holds no greater key.
      */
     ConstIterator
     upperBound(UInt256 const& id) const;
@@ -304,9 +316,11 @@ public:
     /**
      * Find the object with the greatest object id smaller than the input id.
      *
-     * @param id the identifier of the item.
-     *
-     * @note The item does not need to exist.
+     * @param id the identifier of the item, which need not exist in the map.
+     * @return an iterator at the last item with a smaller key, or end() if the
+     *         map holds no smaller key.
+     * @throws SHAMapMissingNode if the map cannot be walked. end() is the
+     *         separate answer that the map holds no smaller key.
      */
     ConstIterator
     lowerBound(UInt256 const& id) const;
@@ -419,17 +433,25 @@ public:
      * deserialized.
      *
      * A node that no valid tree can hold makes the map Invalid, which is
-     * terminal: the root hash committed to an impossible shape, so no peer
-     * can satisfy it.
+     * terminal. An acquisition reaching that verdict gives up.
      *
      * @param nodeID The position in the tree where this node belongs.
      * @param treeNode A deserialized tree node to add.
      * @param filter Optional sync filter to track received nodes.
      * @return Status indicating whether the node was useful, duplicate, or invalid.
      *
-     * @note This function expects the treeNode to be a valid, deserialized SHAMapTreeNode. The
-     *       caller is responsible for deserialization and basic validation before calling this
-     *       function. This also means that the nodeID must be consistent with the node's content.
+     * @note The caller is responsible for deserialization. The position is
+     *       checked here, in three steps that share two verdicts. A node
+     *       the descent reaches at a position other than the one nodeID
+     *       claims is refused with invalid(), and the map stays valid, since
+     *       only the label was wrong. A leaf that hash-verified at the
+     *       position nodeID claims, but whose own key does not lie under
+     *       nodeID, condemns the map and returns mapInvalidated(). A node the
+     *       descent itself condemns on the way, resolved from the local store
+     *       or the filter rather than supplied by the caller, leaves the map
+     *       Invalid and returns invalid(), since that verdict is not the
+     *       caller's doing. Any map outside Synching, including one already
+     *       condemned, returns duplicate().
      */
     SHAMapAddNode
     addKnownNode(
@@ -524,104 +546,299 @@ public:
 
 private:
     /**
-     * A path from the root of the map down to some node, pairing each node with the ID naming its
-     * position.
+     * Whether placing `node` one level below `parentDepth` leaves it no room.
      *
-     * The two halves of an entry must agree, and the only way to get that wrong is to compute an ID
-     * from the wrong branch. So this type does not accept an ID at all: every push takes the branch
-     * being descended and derives the ID itself, so a node and its ID cannot disagree. Reads are
-     * exposed through the same accessors a std::stack would offer.
+     * Only a leaf may sit at kLeafDepth, and only a parent above kLeafDepth has
+     * a nibble left to record the branch taken. Both bounds are needed, since a
+     * leaf one level above kLeafDepth is allowed. Position is judged separately,
+     * by the walk that keeps a path.
+     *
+     * The conjunct order is load bearing: the depth is tested first, so the
+     * virtual isInner() call runs only at the one level where the bound
+     * applies.
+     *
+     * @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 isLeafDepth(parentDepth + 1u) && (node.isInner() || isLeafDepth(parentDepth));
+    }
+
+    /**
+     * A root-down path of nodes through the map.
+     *
+     * Entry `i` is the node at depth `i`, since every edge spans one nibble
+     * (see the class docstring). Only the node is stored. A caller that needs a
+     * position reads a depth and takes the nibbles from its own key.
      */
     class NodePathStack
     {
     public:
+        /**
+         * @return whether the path holds no node at all.
+         */
         [[nodiscard]] bool
         empty() const
         {
-            return stack_.empty();
+            return path_.empty();
         }
 
+        /**
+         * @return how many nodes the path holds, one more than the last node's
+         *         depth.
+         */
         [[nodiscard]] std::size_t
         size() const
         {
-            return stack_.size();
+            return path_.size();
         }
 
-        [[nodiscard]] std::pair<SHAMapTreeNodePtr, SHAMapNodeID> const&
+        /**
+         * The node at the end of the path.
+         *
+         * @return a reference into the path, which a later push may invalidate
+         *         by reallocating, so a caller that pushes must ask again. An
+         *         empty path yields a null node the caller can test.
+         */
+        [[nodiscard]] SHAMapTreeNodePtr const&
         top() const
         {
-            XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::top : non-empty stack");
-            return stack_.top();
+            if (path_.empty())
+            {
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::top : empty stack");
+                static SHAMapTreeNodePtr const kEmpty;
+                return kEmpty;
+                // LCOV_EXCL_STOP
+            }
+            return path_.back();
         }
 
+        /**
+         * The depth of the node at the end of the path.
+         *
+         * @return the depth, which is the entry's own index. Zero on an empty
+         *         path.
+         */
+        [[nodiscard]] unsigned int
+        topDepth() const
+        {
+            if (path_.empty())
+            {
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::topDepth : empty stack");
+                return 0;
+                // LCOV_EXCL_STOP
+            }
+            return static_cast<unsigned int>(path_.size() - 1);
+        }
+
+        /**
+         * Shorten the path by one node. An empty path is left as it is.
+         */
         void
         pop()
         {
-            XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::pop : non-empty stack");
-            stack_.pop();
+            if (path_.empty())
+            {
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::pop : empty stack");
+                return;
+                // LCOV_EXCL_STOP
+            }
+            path_.pop_back();
         }
 
+        /**
+         * Discard the whole path.
+         */
         void
         clear()
         {
-            stack_ = {};
+            path_.clear();
+            pathKey_ = UInt256{};
         }
 
         /**
-         * Start a path at the root of the map, whose ID is the zero-depth ID by definition.
-         */
-        void
-        pushRoot(SHAMapTreeNodePtr node)
-        {
-            XRPL_ASSERT(stack_.empty(), "xrpl::SHAMap::NodePathStack::pushRoot : empty stack");
-            stack_.emplace(std::move(node), SHAMapNodeID{});
-        }
-
-        /**
-         * Extend the path to the child of the current node reached by `branch`.
+         * Shorten the path by one node and hand that node to the caller.
          *
-         * 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.
-         */
-        void
-        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 const 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");
-            stack_.emplace(std::move(node), childID);
-        }
-
-        /**
-         * Extend the path to a node lying on the path to `target`.
+         * Moves the node out, transferring the existing reference.
          *
-         * 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.
+         * @return the node that was at the end of the path, or an empty
+         *         pointer if there was none.
          */
-        void
-        pushNode(SHAMapTreeNodePtr node, UInt256 const& target)
+        [[nodiscard]] SHAMapTreeNodePtr
+        releaseNode()
         {
-            if (stack_.empty())
+            if (path_.empty())
             {
-                pushRoot(std::move(node));
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::releaseNode : empty stack");
+                return {};
+                // LCOV_EXCL_STOP
+            }
+            auto node = std::move(path_.back());
+            path_.pop_back();
+            return node;
+        }
+
+        /**
+         * Shorten the path by one node and hand it over as a `Node`.
+         *
+         * @tparam Node SHAMapInnerNode or SHAMapLeafNode.
+         * @return the node that was at the end of the path, or an empty pointer
+         *         if the path was empty or that node is not a `Node`. The path
+         *         is shortened either way.
+         */
+        template <class Node>
+        [[nodiscard]] intr_ptr::SharedPtr<Node>
+        releaseNodeAs()
+        {
+            static_assert(
+                std::is_same_v<Node, SHAMapInnerNode> || std::is_same_v<Node, SHAMapLeafNode>,
+                "releaseNodeAs serves the two concrete node kinds");
+
+            auto node = releaseNode();
+            if (!node)
+            {
+                // The other half of releaseNode()'s empty-path contract, asserted there by the
+                // UNREACHABLE in its own LCOV_EXCL block.
+                return {};  // LCOV_EXCL_LINE
+            }
+
+            if constexpr (std::is_same_v<Node, SHAMapInnerNode>)
+            {
+                // A leaf sits only at the end of a path, which both callers asking for an inner
+                // node have released first. The other half of the contract their own UNREACHABLEs
+                // assert, in SHAMap::dirtyUp and SHAMap::delItem.
+                if (!node->isInner())
+                    return {};  // LCOV_EXCL_LINE
             }
             else
             {
-                pushChild(std::move(node), selectBranch(stack_.top().second, target));
+                if (!node->isLeaf())
+                    return {};
             }
+
+            return intr_ptr::staticPointerCast<Node>(std::move(node));
+        }
+
+        /**
+         * Start a path at the root of the map, which sits at depth zero by
+         * definition.
+         *
+         * @return false, leaving the path unchanged, if a path was already
+         *         started.
+         */
+        [[nodiscard]] bool
+        pushRoot(SHAMapTreeNodePtr node)
+        {
+            if (!path_.empty())
+            {
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::pushRoot : non-empty stack");
+                return false;
+                // LCOV_EXCL_STOP
+            }
+            // Reserved here rather than in the constructor, to keep SHAMap::end()
+            // allocation-free. A path holds at most kLeafDepth + 1 entries, so no push on a path
+            // started here reallocates. A copy is not given that room, so top()'s invalidation
+            // rule still holds.
+            path_.reserve(kLeafDepth + 1u);
+            path_.push_back(std::move(node));
+            return true;
+        }
+
+        /**
+         * Extend the path to the child of the node at its end reached by
+         * `branch`.
+         *
+         * 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, which serves to judge the node offered.
+         * @return false if there is no node to descend from or 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 select
+         *         `branch`. The path keeps its nodes.
+         */
+        [[nodiscard]] bool
+        pushChild(SHAMapTreeNodePtr node, unsigned int branch)
+        {
+            if (path_.empty() || !node || branch >= kBranchFactor)
+            {
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::pushChild : no child to push");
+                return false;
+                // LCOV_EXCL_STOP
+            }
+
+            // A node must have room below it at the depth it is offered for. Reachable, since a
+            // node resolved by hash carries no position of its own.
+            auto const parentDepth = topDepth();
+            bool const tooDeep = pastLeafDepth(parentDepth, *node);
+            SOMETIMES(tooDeep, "xrpl::SHAMap::NodePathStack::pushChild : child past leaf depth");
+            if (tooDeep)
+            {
+                return false;
+            }
+
+            // A leaf's key must agree with every branch recorded above it, which the chain in
+            // `pathKey_` supplies. A path's length gives only the depths.
+            pathKey_ = childNodeID(pathKey_, parentDepth, branch);
+
+            // The test takes the depth and the two keys, so no SHAMapNodeID is built: it
+            // is a CountedObject, and one per push would report as a live node ID.
+            bool const misplaced =
+                node->isLeaf() && !samePositionAtDepth(parentDepth + 1u, pathKey_, leafKey(*node));
+            SOMETIMES(
+                misplaced, "xrpl::SHAMap::NodePathStack::pushChild : leaf key outside branch");
+            if (misplaced)
+            {
+                return false;
+            }
+
+            path_.push_back(std::move(node));
+            return true;
+        }
+
+        /**
+         * Extend the path by one node lying on the way to `target`, starting
+         * it if it is empty.
+         *
+         * 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` names the branch.
+         *
+         * @param node the node to append.
+         * @param target the key the walk is heading for.
+         * @return whatever pushRoot or pushChild returned.
+         */
+        [[nodiscard]] bool
+        pushNode(SHAMapTreeNodePtr node, UInt256 const& target)
+        {
+            if (path_.empty())
+            {
+                return pushRoot(std::move(node));
+            }
+            return pushChild(std::move(node), selectBranch(topDepth(), target));
         }
 
     private:
-        std::stack<std::pair<SHAMapTreeNodePtr, SHAMapNodeID>> stack_;
+        // path_[i] holds the node at depth i, by construction: pushRoot starts at depth 0 and
+        // pushChild only ever appends one level.
+        std::vector<SHAMapTreeNodePtr> path_;
+
+        // The branches descended, one nibble per level: nibble i is the branch taken from depth i.
+        // Nibbles below the path's end are stale after a pop, so every read masks at the path's
+        // own depth. childNodeID clears a nibble before writing it, so re-descending overwrites.
+        UInt256 pathKey_;
     };
 
     using DeltaRef =
@@ -649,28 +866,41 @@ private:
     /**
      * Record that the map is provably not the one it claims to be.
      *
-     * Private because only the map itself can prove that, from a node that
-     * contradicts the hashes it is syncing against. Cannot fail, since
-     * Invalid outranks every other state; see trySetState().
+     * Cannot fail, since Invalid outranks every other state. Const because a
+     * const descend() can reach this verdict while resolving a node.
      */
     void
-    setInvalid();
+    setInvalid() const;
+
+    /**
+     * Condemn the map, naming in the log the node whose position proves it.
+     *
+     * Const, because a read path can reach this verdict. See setInvalid().
+     *
+     * @param node the node that cannot sit where it was offered.
+     * @param hash the hash it was resolved under.
+     * @param position the place in the tree it was offered for.
+     */
+    void
+    condemn(SHAMapTreeNode const& node, SHAMapHash const& hash, SHAMapNodeID const& position) const;
 
     /**
      * Move the map to a new state, atomically.
      *
-     * With clearSynching(), the only writer of state_ past construction, so
-     * the order between the states lives in one place: Invalid outranks all
-     * of them and is always stored, while every other transition is refused
-     * once the map is Invalid, which is what makes that verdict terminal.
+     * With clearSynching(), the only writer of state_ past construction.
+     * Invalid is always stored, and every other transition is refused once
+     * the map is Invalid, which is what makes that verdict terminal.
      * clearSynching() is narrower and moves only Synching.
      *
+     * Const to let setInvalid() be const. state_ is mutable, which makes that
+     * legal.
+     *
      * @param desired The state to move to.
      * @return false if the map is Invalid and the requested state is not,
-     *         leaving it unchanged; true otherwise.
+     *         leaving it unchanged. True otherwise.
      */
     bool
-    trySetState(SHAMapState desired);
+    trySetState(SHAMapState desired) const;
 
     // tree node cache operations
     SHAMapTreeNodePtr
@@ -698,9 +928,13 @@ private:
     dirtyUp(NodePathStack& stack, UInt256 const& target, SHAMapTreeNodePtr terminal);
 
     /**
-     * Walk towards the specified id, returning the node.  Caller must check
-     *  if the return is nullptr, and if not, if the node->peekItem()->key() ==
-     * id
+     * Walk towards the specified id, returning the node.
+     *
+     * @param id the key to walk towards, which need not be in the map.
+     * @param stack records the path walked, or nullptr to skip recording it.
+     * @return the leaf the walk ended on, or nullptr if it ended on an inner
+     *         node or was refused. A returned leaf need not hold `id`, so
+     *         callers compare its key themselves.
      */
     SHAMapLeafNode*
     walkTowardsKey(UInt256 const& id, NodePathStack* stack = nullptr) const;
@@ -712,10 +946,16 @@ private:
 
     /**
      * Unshare the node, allowing it to be modified
+     *
+     * @param node the node to unshare.
+     * @param depth the depth the node sits at, which says whether it is the
+     *        root. A clone of the root has to be adopted as the new root. A
+     *        clone of any other node is hooked up by the caller.
+     * @return the node, cloned if it was shared.
      */
     template <class Node>
-    intr_ptr::SharedPtr<Node>
-    unshareNode(intr_ptr::SharedPtr<Node>, SHAMapNodeID const& nodeID);
+    [[nodiscard]] intr_ptr::SharedPtr<Node>
+    unshareNode(intr_ptr::SharedPtr<Node> node, unsigned int depth);
 
     /**
      * prepare a node to be modified before flushing
@@ -736,8 +976,13 @@ 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*
+    [[nodiscard]] SHAMapLeafNode*
     belowHelper(NodePathStack& stack, BelowDirection direction) const;
 
     /**
@@ -751,7 +996,7 @@ private:
      * @param direction First to search upwards from `id`, Last to search downwards.
      * @return An iterator to the item found, or end() if no item lies on that side of `id`.
      */
-    ConstIterator
+    [[nodiscard]] ConstIterator
     boundHelper(UInt256 const& id, BelowDirection direction) const;
 
     // Simple descent
@@ -776,6 +1021,22 @@ private:
         bool& pending,
         DescendCallback&&) const;
 
+    /**
+     * Resolve the child of `parent` on `branch`, judging where it lands.
+     *
+     * Const, yet it records the Invalid verdict through setInvalid(). A new
+     * caller must therefore be a path that may reach that verdict. Today
+     * addKnownNode() is the only one.
+     *
+     * @param parent the inner node to descend from.
+     * @param parentID the position of `parent`.
+     * @param branch the branch of `parent` to resolve, which the caller has
+     *               already bounded.
+     * @param filter an alternate source of nodes, or null for the store alone.
+     * @return the child and the child's position. A null child where the branch
+     *         could not be resolved, or where what came back does not belong at
+     *         that position, in which case the map is left Invalid.
+     */
     std::pair<SHAMapTreeNode*, SHAMapNodeID>
     descend(
         SHAMapInnerNode* parent,
@@ -876,10 +1137,29 @@ 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`.
+     *
+     * @param mn the walk's shared state, holding the posted reads.
+     */
+    void
+    gmnProcessDeferredReads(MissingNodes& mn);
 
     // fetch from DB helper function
     SHAMapTreeNodePtr
@@ -912,10 +1192,9 @@ SHAMap::state() const
 }
 
 inline bool
-SHAMap::trySetState(SHAMapState desired)
+SHAMap::trySetState(SHAMapState desired) const
 {
-    // Invalid is stored outright: a walk reaching that verdict has to win against a thread
-    // settling the map.
+    // Invalid is stored outright: that verdict outranks a concurrent transition.
     if (desired == SHAMapState::Invalid)
     {
         state_.store(SHAMapState::Invalid, std::memory_order_release);
@@ -997,7 +1276,7 @@ SHAMap::isValid() const
 }
 
 inline void
-SHAMap::setInvalid()
+SHAMap::setInvalid() const
 {
     // Through trySetState() like every other transition, so state_ has one writer funnel.
     trySetState(SHAMapState::Invalid);

include/xrpl/shamap/SHAMapAddNode.h

@@ -12,6 +12,10 @@ private:
     int bad_;
     int duplicate_;
 
+    // Whether a node in the batch proved the map impossible. A fact about the batch rather than
+    // about the map, so a caller can tell this batch's doing from a concurrent walk's verdict.
+    bool invalidatedMap_;
+
 public:
     SHAMapAddNode();
 
@@ -96,6 +100,26 @@ public:
     static SHAMapAddNode
     invalid();
 
+    /**
+     * @return A verdict recording one invalid node that proved the map
+     *         impossible.
+     */
+    static SHAMapAddNode
+    mapInvalidated();
+
+    /**
+     * Whether a node in the batch proved the map impossible.
+     *
+     * Reports what the batch did rather than what the map now holds, so a
+     * verdict another thread's walk reached is not read as this batch's. The
+     * two differ: SHAMap::setInvalid() is written with no lock held, by a
+     * getMissingNodes() walk its caller runs lock-free.
+     *
+     * @return Whether a node in the batch invalidated the map.
+     */
+    [[nodiscard]] bool
+    invalidatedMap() const;
+
     /**
      * Clear every count back to zero.
      */
@@ -121,15 +145,15 @@ public:
     operator+=(SHAMapAddNode const& n);
 
 private:
-    SHAMapAddNode(int good, int bad, int duplicate);
+    SHAMapAddNode(int good, int bad, int duplicate, bool invalidatedMap = false);
 };
 
-inline SHAMapAddNode::SHAMapAddNode() : good_(0), bad_(0), duplicate_(0)
+inline SHAMapAddNode::SHAMapAddNode() : good_(0), bad_(0), duplicate_(0), invalidatedMap_(false)
 {
 }
 
-inline SHAMapAddNode::SHAMapAddNode(int good, int bad, int duplicate)
-    : good_(good), bad_(bad), duplicate_(duplicate)
+inline SHAMapAddNode::SHAMapAddNode(int good, int bad, int duplicate, bool invalidatedMap)
+    : good_(good), bad_(bad), duplicate_(duplicate), invalidatedMap_(invalidatedMap)
 {
 }
 
@@ -187,6 +211,12 @@ SHAMapAddNode::isUseful() const
     return good_ > 0;
 }
 
+inline bool
+SHAMapAddNode::invalidatedMap() const
+{
+    return invalidatedMap_;
+}
+
 inline SHAMapAddNode
 SHAMapAddNode::duplicate()
 {
@@ -205,10 +235,17 @@ SHAMapAddNode::invalid()
     return SHAMapAddNode(0, 1, 0);
 }
 
+inline SHAMapAddNode
+SHAMapAddNode::mapInvalidated()
+{
+    return SHAMapAddNode(0, 1, 0, true);
+}
+
 inline void
 SHAMapAddNode::reset()
 {
     good_ = bad_ = duplicate_ = 0;
+    invalidatedMap_ = false;
 }
 
 inline std::string
@@ -246,6 +283,9 @@ SHAMapAddNode::operator+=(SHAMapAddNode const& n)
     bad_ += n.bad_;
     duplicate_ += n.duplicate_;
 
+    // Or-ed, not summed: one node in the batch having proved the map impossible is the whole fact.
+    invalidatedMap_ = invalidatedMap_ || n.invalidatedMap_;
+
     return *this;
 }
 

include/xrpl/shamap/SHAMapInnerNode.h

@@ -19,7 +19,8 @@ class SHAMapInnerNode final : public SHAMapTreeNode, public CountedObject<SHAMap
 {
 public:
     /**
-     * Each inner node has 16 children (the 'radix tree' part of the map)
+     * Children per inner node: one branch per value of the key nibble that a
+     * node's depth selects.
      */
     static constexpr unsigned int kBranchFactor = 16;
 
@@ -38,10 +39,16 @@ private:
     static_assert(alignof(std::atomic<std::uint32_t>) == alignof(std::uint32_t));
 
     /**
-     * Written from more than one thread, since canonicalization shares a node
-     * between maps and a walk can run with the acquisition lock released (see
-     * SHAMap::state_). Relaxed both ways, since a generation is only compared
-     * for equality and the children it vouches for are published through lock_.
+     * The generation in which a walk found every node below this one resident,
+     * with this node as the walk's root. The node object is shared by hash
+     * between maps and positions, and a hash commits to a node's contents
+     * rather than its place, so the flag is written and read for the root
+     * position only; below the root, the position-keyed FullBelowCache is the
+     * memo. Written from more than one thread, since canonicalization shares a
+     * node between maps and a walk can run with the acquisition lock released
+     * (see SHAMap::state_). Relaxed both ways, since a generation is only
+     * compared for equality and the children it vouches for are published
+     * through lock_.
      */
     std::atomic<std::uint32_t> fullBelowGen_ = 0;
     std::uint16_t isBranch_ = 0;
@@ -160,10 +167,23 @@ public:
     SHAMapTreeNodePtr
     canonicalizeChild(unsigned int branch, SHAMapTreeNodePtr node);
 
-    // sync functions
+    /**
+     * Whether a walk rooted at this node found every node below it resident.
+     *
+     * Meaningful only for the node a walk starts at; see fullBelowGen_.
+     *
+     * @param generation The FullBelowCache generation the asking walk runs in.
+     * @return True if a walk rooted here completed in that generation.
+     */
     bool
     isFullBelow(std::uint32_t generation) const;
 
+    /**
+     * Record that a walk rooted at this node found every node below it
+     * resident.
+     *
+     * @param gen The FullBelowCache generation the completing walk ran in.
+     */
     void
     setFullBelowGen(std::uint32_t gen);
 

include/xrpl/shamap/SHAMapLeafNode.h

@@ -75,4 +75,41 @@ 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. An inner node carries no key, so every
+ * position agrees with it and a 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));
+}
+
+/**
+ * Whether a node may occupy the child position a branch leads to.
+ *
+ * The overload above, asked about a child without building the child's id.
+ *
+ * @param parentID the position of the node above, whose depth must be below
+ *                 SHAMap::kLeafDepth.
+ * @param branch the branch of that node leading to the position judged.
+ * @param node the node to judge.
+ * @return whether the node's own key agrees with that position.
+ */
+[[nodiscard]] inline bool
+belongsAt(SHAMapNodeID const& parentID, unsigned int branch, SHAMapTreeNode const& node)
+{
+    if (!node.isLeaf())
+        return true;
+
+    auto const& key = leafKey(node);
+    return parentID.isPrefixOf(key) && selectBranch(parentID, key) == branch;
+}
+
 }  // namespace xrpl

include/xrpl/shamap/SHAMapNodeID.h

@@ -123,6 +123,38 @@ operator<<(std::ostream& out, SHAMapNodeID const& node)
     return out << to_string(node);
 }
 
+/**
+ * Returns the id of the child a parent's branch leads to
+ *
+ * A child's id is its parent's with the nibble at the parent's own depth set to
+ * the branch taken. Every other nibble is copied through, so a masked parent
+ * gives a masked child. The write counterpart of selectBranch.
+ *
+ * @param parentID the id of the parent. The nibble at parentDepth is
+ *                 overwritten rather than merged, so an id already carrying one
+ *                 there is accepted.
+ * @param parentDepth the depth of the parent, below SHAMap::kLeafDepth.
+ * @param branch the branch of the parent leading to the child.
+ * @return that child's id, whose depth is parentDepth + 1.
+ */
+[[nodiscard]] UInt256
+childNodeID(UInt256 const& parentID, unsigned int parentDepth, unsigned int branch);
+
+/**
+ * Whether two keys name the same position at a given depth
+ *
+ * Both sides are masked, so either may carry bits below `depth`.
+ * SHAMapNodeID::isPrefixOf is the stricter form.
+ *
+ * @param depth how many leading nibbles name the position, at most
+ *              SHAMap::kLeafDepth. A greater depth is clamped to it.
+ * @param lhs one of the keys.
+ * @param rhs the other key.
+ * @return whether the two agree on those nibbles.
+ */
+[[nodiscard]] bool
+samePositionAtDepth(unsigned int depth, UInt256 const& lhs, UInt256 const& rhs);
+
 /**
  * Return an object representing a serialized SHAMap Node ID
  *
@@ -144,9 +176,26 @@ deserializeSHAMapNodeID(std::string_view s)
 /** @} */
 
 /**
- * Returns the branch that would contain the given hash
+ * Returns the branch at the given depth that would contain the given hash
+ *
+ * @param depth the depth of the node whose branch to select.
+ * @param hash the key whose nibble at that depth names the branch.
+ * @return the branch containing the hash.
  */
 [[nodiscard]] unsigned int
-selectBranch(SHAMapNodeID const& id, UInt256 const& hash);
+selectBranch(unsigned int depth, UInt256 const& hash);
+
+/**
+ * Returns the branch that would contain the given hash
+ *
+ * @param id the node whose depth to read.
+ * @param hash the key whose nibble at that depth names the branch.
+ * @return the branch containing the hash.
+ */
+[[nodiscard]] inline unsigned int
+selectBranch(SHAMapNodeID const& id, UInt256 const& hash)
+{
+    return selectBranch(id.getDepth(), hash);
+}
 
 }  // namespace xrpl

include/xrpl/shamap/SHAMapSyncFilter.h

@@ -30,6 +30,17 @@ public:
         Blob&& nodeData,
         SHAMapNodeType type) const = 0;
 
+    /**
+     * Fetch the node data for a hash, if this filter holds it.
+     *
+     * Postcondition: the data returned hashes to `nodeHash`. Callers adopt that
+     * hash without recomputing it and publish the node to the map's caches. An
+     * implementation checks the digest before answering, unless its own storage
+     * ties the data to the key.
+     *
+     * @param nodeHash the hash of the node wanted.
+     * @return the node's hash-prefixed wire data, or nothing.
+     */
     [[nodiscard]] virtual std::optional<Blob>
     getNode(SHAMapHash const& nodeHash) const = 0;
 };

include/xrpl/shamap/SHAMapTreeNode.h

@@ -177,7 +177,18 @@ public:
     virtual void
     invariants(bool isRoot = false) const = 0;
 
-    static SHAMapTreeNodePtr
+    /**
+     * Parse a prefixed node, adopting `hash` without verifying it.
+     *
+     * Every caller must establish `hash` first. A node from the local store
+     * carries its own key, and a node from a sync filter is covered by
+     * SHAMapSyncFilter::getNode()'s postcondition.
+     *
+     * @param rawNode the serialized node, including its 4-byte hash prefix.
+     * @param hash the hash to adopt.
+     * @return the node.
+     */
+    [[nodiscard]] static SHAMapTreeNodePtr
     makeFromPrefix(Slice rawNode, SHAMapHash const& hash);
 
     static SHAMapTreeNodePtr

src/libxrpl/ledger/Ledger.cpp

@@ -305,7 +305,8 @@ Ledger::Ledger(Ledger const& prevLedger, NetClock::time_point closeTime)
 }
 
 Ledger::Ledger(LedgerHeader const& info, Rules rules, Family& family)
-    : immutable_(true)
+    : immutable_(false)
+    , mapHashesFromHeader_(true)
     , txMap_(SHAMapType::TRANSACTION, info.txHash, family)
     , stateMap_(SHAMapType::STATE, info.accountHash, family)
     , rules_(std::move(rules))
@@ -337,31 +338,29 @@ Ledger::Ledger(
 bool
 Ledger::setImmutable(bool rehash)
 {
-    // A map found structurally invalid during sync must never be made immutable: isValid() tests
-    // only for Invalid, and an immutable ledger is treated as persistable. Asked before anything is
-    // written, so a refusal leaves the header exactly as it was rather than half relabeled.
+    // An immutable ledger is treated as persistable, so an invalid map must never become
+    // immutable. The validity test runs before anything is written, so a refusal leaves
+    // the header as it was.
     if (!mapsValid())
         return false;
 
-    // Read here but written to the header below, once the maps are settled: getHash() can unshare a
-    // dirty tree, so it has to run while the map is still mutable, while a write to the header must
-    // wait until both maps have made it. Skipped once the ledger is immutable, since its maps can
-    // no longer change.
-    bool const deriveMapHashes = !immutable_ && rehash;
+    // Read now but written to the header below: getHash() can unshare a dirty tree, so it must run
+    // while the map is still mutable, while the header may only be written once both maps are
+    // settled. Skipped when the ledger is already immutable or when the header supplied these.
+    bool const deriveMapHashes = !immutable_ && !mapHashesFromHeader_ && rehash;
     UInt256 const txHash = deriveMapHashes ? txMap_.getHash().asUInt256() : UInt256{};
     UInt256 const accountHash = deriveMapHashes ? stateMap_.getHash().asUInt256() : UInt256{};
 
-    // Both were valid at the check above, but a concurrent walk can invalidate one in between (see
-    // SHAMap::state_), so the result is checked. setInvalid() outranks Immutable and can land
-    // after both maps have been settled, so this narrows the window it guards.
+    // A concurrent walk can invalidate a map between the check above and here (see
+    // SHAMap::state_), so the result is checked. That narrows the window rather than closing it,
+    // since setInvalid() outranks Immutable.
     bool const bothImmutable = setMapsImmutable();
     SOMETIMES(!bothImmutable, "xrpl::Ledger::setImmutable : map invalidated while going immutable");
     if (!bothImmutable)
-        return false;
+        return false;  // LCOV_EXCL_LINE: only the walk named above reaches this, so no test does
 
-    // Written only now, so losing the race above leaves the header describing what the ledger was
-    // built from rather than a map that has since been abandoned. Forced rather than conditional,
-    // since this is the only place the hash transitions to valid.
+    // Written only now, so failing the check above leaves the header describing what the ledger was
+    // built from. Forced, since this is the only place the hash transitions to valid.
     if (deriveMapHashes)
     {
         header_.txHash = txHash;

src/libxrpl/shamap/SHAMap.cpp

@@ -106,6 +106,18 @@ SHAMap::snapShot(bool isMutable) const
     return std::make_shared<SHAMap>(*this, isMutable);
 }
 
+void
+SHAMap::condemn(SHAMapTreeNode const& node, SHAMapHash const& hash, SHAMapNodeID const& position)
+    const
+{
+    // journal_ is shared by every map built from one Family, so name which map this is. The root
+    // node's own hash, since getHash() unshares the tree on a zero hash.
+    JLOG(journal_.warn()) << (node.isLeaf() ? "Leaf " : "Inner node ") << hash << " cannot sit at "
+                          << position << ", so the map with root hash " << root_->getHash()
+                          << " is invalid";
+    setInvalid();
+}
+
 void
 SHAMap::dirtyUp(NodePathStack& stack, UInt256 const& target, SHAMapTreeNodePtr child)
 {
@@ -121,14 +133,19 @@ SHAMap::dirtyUp(NodePathStack& stack, UInt256 const& target, SHAMapTreeNodePtr c
 
     while (!stack.empty())
     {
-        auto node = intr_ptr::dynamicPointerCast<SHAMapInnerNode>(stack.top().first);
-        SHAMapNodeID const nodeID = stack.top().second;
-        stack.pop();
-        XRPL_ASSERT(node, "xrpl::SHAMap::dirtyUp : non-null node");
+        auto const depth = stack.topDepth();
+        auto node = stack.releaseNodeAs<SHAMapInnerNode>();
+        if (!node)
+        {
+            // LCOV_EXCL_START
+            UNREACHABLE("xrpl::SHAMap::dirtyUp : node is not inner");
+            Throw<SHAMapMissingNode>(type_, target);
+            // LCOV_EXCL_STOP
+        }
 
-        auto const branch = selectBranch(nodeID, target);
+        auto const branch = selectBranch(depth, target);
 
-        node = unshareNode(std::move(node), nodeID);
+        node = unshareNode(std::move(node), depth);
         node->setChild(branch, std::move(child));
 
         child = std::move(node);
@@ -138,32 +155,62 @@ 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");
-    auto inNode = root_;
-    SHAMapNodeID nodeID;
+    if (stack != nullptr && !stack->empty())
+    {
+        // The path must start at the root, so a non-empty one is cleared rather than appended to.
+        // LCOV_EXCL_START
+        UNREACHABLE("xrpl::SHAMap::walkTowardsKey : non-empty stack input");
+        stack->clear();
+        return nullptr;
+        // LCOV_EXCL_STOP
+    }
 
-    // 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 const pushCurrent = [&] {
-        if (stack != nullptr)
-            stack->pushNode(inNode, id);
+    auto inNode = root_;
+    unsigned int noStackDepth = 0;
+
+    // Returns false when the path could not be extended, which is separate from `id` being absent
+    // (see pushChild). The path is cleared, so a caller reads an empty path as a refusal.
+    auto const 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 depth = stack != nullptr ? stack->topDepth() : noStackDepth;
+        auto const branch = selectBranch(depth, id);
         if (inner.isEmptyBranch(branch))
             return nullptr;
 
         inNode = descendThrow(inner, branch);
-        nodeID = nodeID.getChildNodeID(branch);
+        if (stack == nullptr)
+        {
+            // The same depth bound pushChild applies, so both modes refuse at the same depth. A
+            // node resolved from the local store still has its position and type to be judged.
+            bool const tooDeep = pastLeafDepth(depth, *inNode);
+            SOMETIMES(tooDeep, "xrpl::SHAMap::walkTowardsKey : child too deep");
+            if (tooDeep)
+            {
+                return nullptr;
+            }
+            ++noStackDepth;
+        }
     }
 
-    pushCurrent();
+    if (!pushCurrent())
+    {
+        return nullptr;
+    }
     return safeDowncast<SHAMapLeafNode*>(inNode.get());
 }
 
@@ -233,6 +280,7 @@ SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter const* filter) cons
     {
         try
         {
+            // Adopts the hash, which SHAMapSyncFilter::getNode()'s postcondition guarantees.
             auto node = SHAMapTreeNode::makeFromPrefix(makeSlice(*nodeData), hash);
             if (node)
             {
@@ -374,12 +422,23 @@ SHAMap::descend(
         !parent->isEmptyBranch(branch), "xrpl::SHAMap::descend : parent branch is non-empty");
 
     SHAMapTreeNode* child = parent->getChildPointer(branch);  // NOLINT(misc-const-correctness)
+    auto const childID = parentID.getChildNodeID(branch);
 
     if (child == nullptr)
     {
         auto const& childHash = parent->getChildHash(branch);
         SHAMapTreeNodePtr childNode = fetchNodeNT(childHash, filter);
 
+        if (childNode &&
+            (!belongsAt(childID, *childNode) || pastLeafDepth(parentID.getDepth(), *childNode)))
+        {
+            // A hash covers a node's contents, not its position, so position is judged before
+            // canonicalizeChild hooks the node in. The verdict lands on the map, since resolving
+            // the same blob again gives the same node.
+            condemn(*childNode, childHash, childID);
+            return std::make_pair(nullptr, childID);
+        }
+
         if (childNode)
         {
             childNode = parent->canonicalizeChild(branch, std::move(childNode));
@@ -387,7 +446,7 @@ SHAMap::descend(
         }
     }
 
-    return std::make_pair(child, parentID.getChildNodeID(branch));
+    return std::make_pair(child, childID);
 }
 
 SHAMapTreeNode*
@@ -434,7 +493,7 @@ SHAMap::descendAsync(
 
 template <class Node>
 intr_ptr::SharedPtr<Node>
-SHAMap::unshareNode(intr_ptr::SharedPtr<Node> node, SHAMapNodeID const& nodeID)
+SHAMap::unshareNode(intr_ptr::SharedPtr<Node> node, unsigned int depth)
 {
     // make sure the node is suitable for the intended operation (copy on write)
     XRPL_ASSERT(node->cowid() <= cowid_, "xrpl::SHAMap::unshareNode : node valid for cowid");
@@ -443,7 +502,7 @@ SHAMap::unshareNode(intr_ptr::SharedPtr<Node> node, SHAMapNodeID const& nodeID)
         // have a CoW
         XRPL_ASSERT(state() != SHAMapState::Immutable, "xrpl::SHAMap::unshareNode : not immutable");
         node = intr_ptr::staticPointerCast<Node>(node->clone(cowid_));
-        if (nodeID.isRoot())
+        if (depth == 0)
             root_ = node;
     }
     return node;
@@ -453,14 +512,19 @@ SHAMapLeafNode*
 SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const
 {
     XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack input");
-    if (auto const& top = stack.top().first; top->isLeaf())
+    if (stack.empty())
+    {
+        // LCOV_EXCL_START
+        return nullptr;
+        // LCOV_EXCL_STOP
+    }
+    if (auto const& top = stack.top(); top->isLeaf())
         return safeDowncast<SHAMapLeafNode*>(top.get());
 
-    // The stack owns the node/ID pairing, so descending is only ever "push the branch we took".
-    // `scanned` counts how many branches of the current node we have examined; the branch we look
-    // at is derived from it, so no index ever goes out of range. `inner` tracks the node on top of
-    // the stack, which keeps it alive, so it only needs recomputing after a push.
-    auto* inner = safeDowncast<SHAMapInnerNode*>(stack.top().first.get());
+    // `scanned` counts the branches of the current node already examined, and the branch looked at
+    // is derived from it, so the index stays inside the fan-out. `inner` points at the top of the
+    // path, which keeps it alive, so it only needs recomputing after a push.
+    auto* inner = safeDowncast<SHAMapInnerNode*>(stack.top().get());
     for (auto scanned = 0u; scanned < kBranchFactor;)
     {
         auto const childBranch =
@@ -472,9 +536,20 @@ SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const
             continue;
         }
 
-        stack.pushChild(descendThrow(*inner, childBranch), childBranch);
+        auto const parentDepth = stack.topDepth();
+        auto descended = descendThrow(*inner, childBranch);
+        if (!stack.pushChild(std::move(descended), childBranch))
+        {
+            // Throwing rather than returning nullptr keeps nullptr meaning a subtree with no leaf
+            // below it, which begin() and an iterator increment both rely on. Every caller here is
+            // a const read on a shared snapshot, so the verdict is left to the acquisition path
+            // (see SHAMap::descend and gmnProcessNodes).
+            JLOG(journal_.warn()) << "Cannot walk below depth " << parentDepth << " at branch "
+                                  << childBranch;
+            Throw<SHAMapMissingNode>(type_, inner->getChildHash(childBranch));
+        }
 
-        auto const& child = stack.top().first;
+        auto const& child = stack.top();
         if (child->isLeaf())
             return safeDowncast<SHAMapLeafNode*>(child.get());
 
@@ -529,7 +604,12 @@ 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)
     {
@@ -543,18 +623,28 @@ SHAMapLeafNode const*
 SHAMap::peekNextItem(UInt256 const& id, NodePathStack& stack) const
 {
     XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::peekNextItem : non-empty stack input");
-    XRPL_ASSERT(stack.top().first->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf");
+    if (stack.empty())
+    {
+        // LCOV_EXCL_START
+        return nullptr;
+        // LCOV_EXCL_STOP
+    }
+    XRPL_ASSERT(stack.top()->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf");
     stack.pop();
     while (!stack.empty())
     {
-        auto const [node, nodeID] = stack.top();
+        auto const& node = stack.top();
         XRPL_ASSERT(!node->isLeaf(), "xrpl::SHAMap::peekNextItem : another node is not leaf");
         auto& inner = safeDowncast<SHAMapInnerNode&>(*node);
-        for (auto i = selectBranch(nodeID, id) + 1; i < kBranchFactor; ++i)
+        for (auto i = selectBranch(stack.topDepth(), id) + 1; i < kBranchFactor; ++i)
         {
             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);
@@ -598,9 +688,15 @@ SHAMap::boundHelper(UInt256 const& id, BelowDirection direction) const
 
     NodePathStack stack;
     walkTowardsKey(id, &stack);
+
+    // An empty path means the walk refused a node. An empty map still leaves its root on the path,
+    // and end() would claim no key lies on the requested side of `id`, so a refusal throws.
+    if (stack.empty())
+        Throw<SHAMapMissingNode>(type_, id);
+
     while (!stack.empty())
     {
-        auto const [node, nodeID] = stack.top();
+        auto const& node = stack.top();
         if (node->isLeaf())
         {
             auto const& item = safeDowncast<SHAMapLeafNode const&>(*node).peekItem();
@@ -610,7 +706,7 @@ SHAMap::boundHelper(UInt256 const& id, BelowDirection direction) const
         else
         {
             auto& inner = safeDowncast<SHAMapInnerNode&>(*node);
-            auto const taken = selectBranch(nodeID, id);
+            auto const taken = selectBranch(stack.topDepth(), id);
             auto const remaining = searchingForward ? (kBranchFactor - 1u - taken) : taken;
 
             for (auto scanned = 0u; scanned < remaining; ++scanned)
@@ -620,7 +716,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);
@@ -662,10 +762,12 @@ SHAMap::delItem(UInt256 const& id)
     if (stack.empty())
         Throw<SHAMapMissingNode>(type_, id);
 
-    auto const leaf = intr_ptr::dynamicPointerCast<SHAMapLeafNode>(stack.top().first);
-    stack.pop();
+    // An absent id leaves an inner node on top, which is the "not found" answer.
+    auto const leaf = stack.releaseNodeAs<SHAMapLeafNode>();
+    if (!leaf)
+        return false;
 
-    if (!leaf || (leaf->peekItem()->key() != id))
+    if (leaf->peekItem()->key() != id)
         return false;
 
     SHAMapNodeType const type = leaf->getType();
@@ -675,19 +777,25 @@ SHAMap::delItem(UInt256 const& id)
 
     while (!stack.empty())
     {
-        auto node = intr_ptr::staticPointerCast<SHAMapInnerNode>(stack.top().first);
-        SHAMapNodeID const nodeID = stack.top().second;
-        stack.pop();
+        auto const depth = stack.topDepth();
+        auto node = stack.releaseNodeAs<SHAMapInnerNode>();
+        if (!node)
+        {
+            // LCOV_EXCL_START
+            UNREACHABLE("xrpl::SHAMap::delItem : node is not inner");
+            Throw<SHAMapMissingNode>(type_, id);
+            // LCOV_EXCL_STOP
+        }
 
-        node = unshareNode(std::move(node), nodeID);
+        node = unshareNode(std::move(node), depth);
         node->setChild(
-            selectBranch(nodeID, id), std::move(prevNode));  // NOLINT(bugprone-use-after-move)
+            selectBranch(depth, id), std::move(prevNode));  // NOLINT(bugprone-use-after-move)
 
         XRPL_ASSERT(
             not prevNode,  // NOLINT(bugprone-use-after-move)
             "xrpl::SHAMap::delItem : prevNode should be nullptr after std::move");
 
-        if (!nodeID.isRoot())
+        if (depth != 0)
         {
             // we may have made this a node with 1 or 0 children
             // And, if so, we need to remove this branch
@@ -748,8 +856,8 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
     if (stack.empty())
         Throw<SHAMapMissingNode>(type_, tag);
 
-    auto [node, nodeID] = stack.top();
-    stack.pop();
+    auto depth = stack.topDepth();
+    auto node = stack.releaseNode();
 
     if (node->isLeaf())
     {
@@ -757,12 +865,12 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
         if (leaf->peekItem()->key() == tag)
             return false;
     }
-    node = unshareNode(std::move(node), nodeID);
+    node = unshareNode(std::move(node), depth);
     if (node->isInner())
     {
         // easy case, we end on an inner node
         auto const inner = intr_ptr::staticPointerCast<SHAMapInnerNode>(node);
-        auto const branch = selectBranch(nodeID, tag);
+        auto const branch = selectBranch(depth, tag);
         XRPL_ASSERT(
             inner->isEmptyBranch(branch), "xrpl::SHAMap::addGiveItem : inner branch is empty");
         inner->setChild(branch, makeTypedLeaf(type, std::move(item), cowid_));
@@ -780,13 +888,20 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
 
         auto b1 = 0u, b2 = 0u;
 
-        while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key())))
+        while ((b1 = selectBranch(depth, tag)) == (b2 = selectBranch(depth, otherItem->key())))
         {
-            stack.pushNode(node, tag);
+            if (!stack.pushNode(node, tag))
+            {
+                // The loop advances only while the two keys agree at the current nibble, and keys
+                // agreeing at every nibble are equal, which returned false above.
+                // 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
-            nodeID = nodeID.getChildNodeID(b1);
+            ++depth;
             node = intr_ptr::makeShared<SHAMapInnerNode>(cowid_);
         }
 
@@ -835,16 +950,19 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem cons
     if (stack.empty())
         Throw<SHAMapMissingNode>(type_, tag);
 
-    auto node = intr_ptr::dynamicPointerCast<SHAMapLeafNode>(stack.top().first);
-    auto const nodeID = stack.top().second;
-    stack.pop();
+    auto const depth = stack.topDepth();
 
-    if (!node || (node->peekItem()->key() != tag))
+    // A tag absent from the map leaves an inner node on top, which the API permits.
+    auto node = stack.releaseNodeAs<SHAMapLeafNode>();
+    if (!node)
+    {
+        return false;
+    }
+
+    // Or the walk ended on a leaf holding some other key.
+    if (node->peekItem()->key() != tag)
     {
-        // LCOV_EXCL_START
-        UNREACHABLE("xrpl::SHAMap::updateGiveItem : invalid node");
         return false;
-        // LCOV_EXCL_STOP
     }
 
     if (node->getType() != type)
@@ -853,7 +971,7 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem cons
         return false;
     }
 
-    node = unshareNode(std::move(node), nodeID);
+    node = unshareNode(std::move(node), depth);
 
     if (node->setItem(item))
         dirtyUp(stack, tag, node);

src/libxrpl/shamap/SHAMapNodeID.cpp

@@ -50,14 +50,31 @@ maskedToDepth(UInt256 const& key, unsigned int depth)
     return key & depthMask(depth);
 }
 
-// Whether `id` at `depth` is what `key` looks like once masked down to that depth, i.e.
-// whether an ID with this depth and id names a subtree that `key` falls under.
+// Whether `key` masked to `depth` equals `id`. Asymmetric: `id` is not masked, so this also
+// refuses an id carrying bits below its own depth.
 static bool
 isPrefixOfAtDepth(UInt256 const& id, unsigned int depth, UInt256 const& key)
 {
     return maskedToDepth(key, depth) == id;
 }
 
+bool
+samePositionAtDepth(unsigned int depth, UInt256 const& lhs, UInt256 const& rhs)
+{
+    // The mask is chosen here, so an out-of-range depth would index depthMask's table past its
+    // last entry. kLeafDepth itself is in range, unlike in selectBranch: the position at the leaf
+    // depth is the whole key. A public helper has to hold its own bound.
+    if (depth > SHAMap::kLeafDepth)
+    {
+        // LCOV_EXCL_START
+        UNREACHABLE("xrpl::samePositionAtDepth : depth within tree");
+        depth = SHAMap::kLeafDepth;
+        // LCOV_EXCL_STOP
+    }
+
+    return maskedToDepth(lhs, depth) == maskedToDepth(rhs, depth);
+}
+
 // canonicalize the hash to a node ID for this depth
 SHAMapNodeID::SHAMapNodeID(unsigned int depth, UInt256 const& hash) : id_(hash), depth_(depth)
 {
@@ -90,6 +107,31 @@ SHAMapNodeID::getRawString() const
     return s.getString();
 }
 
+[[nodiscard]] UInt256
+childNodeID(UInt256 const& parentID, unsigned int parentDepth, unsigned int branch)
+{
+    XRPL_ASSERT(branch < SHAMap::kBranchFactor, "xrpl::childNodeID : valid branch input");
+    XRPL_ASSERT(parentDepth < SHAMap::kLeafDepth, "xrpl::childNodeID : parent above leaf depth");
+
+    // Only a depth below kLeafDepth names a child nibble, so clamp to the last one that does.
+    auto const clamped = std::min(parentDepth, SHAMap::kLeafDepth - 1u);
+
+    // The nibble is cleared before the branch is written, so an id already carrying one there is
+    // overwritten rather than merged. The byte index and both nibble choices read `clamped`, so
+    // they cannot disagree.
+    UInt256 id = parentID;
+    auto& byte = *(id.begin() + (clamped / 2));
+    if ((clamped & 1) != 0u)
+    {
+        byte = static_cast<unsigned char>((byte & 0xF0u) | branch);
+    }
+    else
+    {
+        byte = static_cast<unsigned char>((byte & 0x0Fu) | (branch << 4));
+    }
+    return id;
+}
+
 SHAMapNodeID
 SHAMapNodeID::getChildNodeID(unsigned int branch) const
 {
@@ -107,15 +149,13 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const
     XRPL_ASSERT(
         depth_ <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::getChildNodeID : maximum leaf depth");
 
-    if (depth_ >= SHAMap::kLeafDepth)
+    if (SHAMap::isLeafDepth(depth_))
         Throw<std::logic_error>(std::format("Request for child node ID of {}", to_string(*this)));
 
     if (!isPrefixOf(id_))
         Throw<std::logic_error>(std::format("Incorrect mask for {}", to_string(*this)));
 
-    SHAMapNodeID node{depth_ + 1, id_};
-    node.id_.begin()[depth_ / 2] |= ((depth_ & 1) != 0u) ? branch : (branch << 4);
-    return node;
+    return SHAMapNodeID{depth_ + 1, childNodeID(id_, depth_, branch)};
 }
 
 bool
@@ -145,16 +185,16 @@ deserializeSHAMapNodeID(void const* data, std::size_t size)
 }
 
 [[nodiscard]] unsigned int
-selectBranch(SHAMapNodeID const& id, UInt256 const& hash)
+selectBranch(unsigned int depth, UInt256 const& hash)
 {
-    XRPL_ASSERT(id.getDepth() < SHAMap::kLeafDepth, "xrpl::selectBranch : depth below leaf depth");
+    XRPL_ASSERT(depth < SHAMap::kLeafDepth, "xrpl::selectBranch : depth below leaf depth");
 
-    // A depth-64 ID has no nibble left to select. Callers must not ask, but clamp anyway to keep
-    // the read below the end of the 32-byte key.
-    auto const depth = std::min(id.getDepth(), SHAMap::kLeafDepth - 1u);
-    auto branch = static_cast<unsigned int>(*(hash.begin() + (depth / 2)));
+    // Only a depth below kLeafDepth has a nibble to select, so clamp to the last one that does.
+    auto const clamped = std::min(depth, SHAMap::kLeafDepth - 1u);
+    auto branch = static_cast<unsigned int>(*(hash.begin() + (clamped / 2)));
 
-    if ((depth & 1) != 0u)
+    // The byte index and the nibble choice both read `clamped`, so they cannot disagree.
+    if ((clamped & 1) != 0u)
     {
         branch &= 0xf;
     }

src/libxrpl/shamap/SHAMapSync.cpp

@@ -31,25 +31,6 @@
 
 namespace xrpl {
 
-namespace {
-
-/**
- * Whether a depth is at or past the deepest an inner node may occupy.
- *
- * Nibbles run out at SHAMap::kLeafDepth, so only a leaf may sit there. True for
- * every deeper position too, which lies past the end of a key.
- *
- * @param depth The depth to judge.
- * @return Whether an inner node at that depth makes the map impossible.
- */
-[[nodiscard]] bool
-isLeafDepth(unsigned int depth)
-{
-    return depth >= SHAMap::kLeafDepth;
-}
-
-}  // namespace
-
 void
 SHAMap::visitLeaves(
     std::function<void(boost::intrusive_ptr<SHAMapItem const> const& item)> const& leafFunction)
@@ -223,12 +204,12 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
             // we already know this child node is missing
             fullBelow = false;
         }
-        // The depth test precedes the cache lookup for the same reason it does in addKnownNode():
-        // the cache is keyed by node hash and shared across maps, and a hash covers a node's
-        // children but not its depth. Skipping the shortcut forgoes an optimization only.
+        // The cache key carries the child's position beside its hash, so a hit answers for this
+        // child at this position and no other. The depth test runs first, so a child at kLeafDepth
+        // is judged by the arm below rather than answered from the cache: only a leaf sits there.
         else if (
             !backed_ || isLeafDepth(nodeID.getDepth() + 1) ||
-            !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
+            !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256(), nodeID, branch))
         {
             bool pending = false;
             auto d = descendAsync(
@@ -259,18 +240,28 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
                 if (--mn.max <= 0)
                     return;
             }
-            else if (d->isInner() && isLeafDepth(nodeID.getDepth() + 1))
+            // Only a leaf has a position of its own to judge, so the type is tested first. The
+            // depth test bounds the branch this arm names, and the arm below bounds the descent.
+            else if (
+                d->isLeaf() && !isLeafDepth(nodeID.getDepth()) && !belongsAt(nodeID, branch, *d))
             {
-                // Only a leaf belongs that deep (see isLeafDepth and SHAMap::addKnownNode). A node
-                // resolved locally reaches the walk without passing through addKnownNode(), so the
-                // walk reaches this verdict itself. Ordered ahead of the full-below test below,
-                // which canonicalization shares across maps.
-                JLOG(journal_.warn()) << "Inner node at branch " << branch << " below " << nodeID
-                                      << " makes the map invalid";
-                setInvalid();
+                // The judgment SHAMap::descend makes, for the descendAsync path, which hooks what
+                // it resolves before this runs. `fullBelow` is cleared as on the missing path.
+                fullBelow = false;
+                condemn(*d, childHash, nodeID.getChildNodeID(branch));
                 return;
             }
-            else if (d->isInner() && !safeDowncast<SHAMapInnerNode*>(d)->isFullBelow(mn.generation))
+            else if (pastLeafDepth(nodeID.getDepth(), *d))
+            {
+                // A node resolved locally never passes through addKnownNode(), so the walk judges
+                // it here. The bound also covers nodeID, which the descent below writes.
+                condemn(*d, childHash, nodeID.getChildNodeID(branch));
+                return;
+            }
+            // The node's own full-below flag is not read here. The node object is shared by hash
+            // across maps and positions, so a flag another walk set says nothing about this
+            // position, and the position-keyed cache above has already missed for it.
+            else if (d->isInner())
             {
                 mn.stack.push(se);
 
@@ -289,10 +280,14 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
 
     if (fullBelow)
     {  // No partial node encountered below this node
-        node->setFullBelowGen(mn.generation);
+        // The node flag is written for the root alone, since the root position is the only one a
+        // shared node object can vouch for; see SHAMapInnerNode::fullBelowGen_.
+        if (nodeID.isRoot())
+            node->setFullBelowGen(mn.generation);
         if (backed_)
         {
-            f_.getFullBelowCache()->insert(node->getHash().asUInt256());
+            // Keyed by position as well as hash, so the entry answers only for this position.
+            f_.getFullBelowCache()->insert(node->getHash().asUInt256(), nodeID);
         }
     }
 
@@ -323,6 +318,25 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn)
         auto nodePtr = std::get<3>(deferredNode);
         auto const& nodeHash = parent->getChildHash(branch);
 
+        // A deferred entry carries the position the walk held when it posted the read, so a branch
+        // below it is named here only where the tree has room for one.
+        if (nodePtr && nodePtr->isLeaf() && !isLeafDepth(parentID.getDepth()) &&
+            !belongsAt(parentID, branch, *nodePtr))
+        {
+            // The judgment the synchronous paths make, for a node an async read resolved. Skips
+            // rather than returns, since every posted read must be drained while `mn` is alive.
+            condemn(*nodePtr, nodeHash, parentID.getChildNodeID(branch));
+            continue;
+        }
+
+        // The depth bound gmnProcessNodes carries beside its leaf test, for a node an async read
+        // resolved. Skips for the same reason as above.
+        if (nodePtr && pastLeafDepth(parentID.getDepth(), *nodePtr))
+        {
+            condemn(*nodePtr, nodeHash, parentID.getChildNodeID(branch));
+            continue;
+        }
+
         if (nodePtr)
         {  // Got the node
             nodePtr = parent->canonicalizeChild(branch, std::move(nodePtr));
@@ -438,12 +452,10 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
 
             if (mn.stack.empty() && !mn.resumes.empty())
             {
-                // Recheck nodes we could not finish before
+                // Recheck nodes we could not finish before. A node's own full-below flag is not
+                // consulted: it is written for a root only, and a resumed node sits below one.
                 for (auto const& [innerNode, nodeId] : mn.resumes)
-                {
-                    if (!innerNode->isFullBelow(mn.generation))
-                        mn.stack.emplace(innerNode, nodeId, randInt(255), 0, true);
-                }
+                    mn.stack.emplace(innerNode, nodeId, randInt(255), 0, true);
 
                 mn.resumes.clear();
             }
@@ -629,10 +641,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())
     {
@@ -640,13 +648,13 @@ SHAMap::addKnownNode(
         return SHAMapAddNode::duplicate();
     }
 
-    auto const generation = f_.getFullBelowCache()->getGeneration();
     SHAMapNodeID currNodeID;
     auto currNode = root_.get();
 
-    while (currNode->isInner() &&
-           !safeDowncast<SHAMapInnerNode*>(currNode)->isFullBelow(generation) &&
-           (currNodeID.getDepth() < nodeID.getDepth()))
+    // The descent reads no node's own full-below flag: a node object is shared by hash across
+    // maps and positions, so that flag answers for the root position alone. The position-keyed
+    // cache lookup in the loop is the memo that answers for a position below it.
+    while (currNode->isInner() && (currNodeID.getDepth() < nodeID.getDepth()))
     {
         auto const branch = selectBranch(currNodeID, nodeID.getNodeID());
         auto inner = safeDowncast<SHAMapInnerNode*>(currNode);
@@ -659,12 +667,11 @@ SHAMap::addKnownNode(
 
         auto childHash = inner->getChildHash(branch);
 
-        // Depth before the cache: the cache is keyed by node hash and shared across the family, and
-        // a hash covers a node's children but not its depth, so the same subtree can be cached as
-        // complete at one depth and reached at another. Skipping the shortcut only forgoes an
-        // optimization.
+        // The cache key carries the child's position beside its hash, so a hit answers for this
+        // child at this position and no other. The depth test runs first, so a node offered at
+        // kLeafDepth reaches the badDepth test below rather than being answered as a duplicate.
         if (!isLeafDepth(currNodeID.getDepth() + 1) &&
-            f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
+            f_.getFullBelowCache()->touchIfExists(childHash.asUInt256(), currNodeID, branch))
         {
             return SHAMapAddNode::duplicate();
         }
@@ -672,6 +679,14 @@ SHAMap::addKnownNode(
         auto prevNode = inner;
         std::tie(currNode, currNodeID) = descend(inner, currNodeID, branch, filter);
 
+        if (!isValid())
+        {
+            // descend condemned the map. `childHash` was read before that descent, so the
+            // comparison below would read a stale value.
+            JLOG(journal_.warn()) << "Node " << nodeID << " cannot be hooked into an invalid map";
+            return SHAMapAddNode::invalid();
+        }
+
         if (currNode != nullptr)
             continue;
 
@@ -682,25 +697,18 @@ SHAMap::addKnownNode(
             return SHAMapAddNode::invalid();
         }
 
-        // Only leaves may sit at kLeafDepth (see isLeafDepth), so an inner node there makes the map
-        // impossible. The node is reported as bad data.
-        //
-        // Every node from the root down hash-verified to get here, so it is the requested root hash
-        // itself that commits to a shape no valid tree can have. The verdict belongs to that hash
-        // rather than to our copy of the tree.
+        // Every node from the root down hash-verified to get here, so the requested root hash
+        // itself commits to a shape no tree can have. The verdict belongs to that hash.
         bool const badDepth = treeNode->isInner() && isLeafDepth(currNodeID.getDepth());
         SOMETIMES(badDepth, "xrpl::SHAMap::addKnownNode : map is invalid");
         if (badDepth)
         {
-            JLOG(journal_.warn()) << "Node " << nodeID << " makes the map invalid at "
-                                  << currNodeID;
-            setInvalid();
-            return SHAMapAddNode::invalid();
+            condemn(*treeNode, treeNode->getHash(), currNodeID);
+            return SHAMapAddNode::mapInvalidated();
         }
 
-        // The data hashes to the child at currNodeID but claims to belong at nodeID, so it is not
-        // the node that was asked for. Only the label is wrong, so the map stays sound and the node
-        // is still obtainable from another sender.
+        // The data hashes to the child at currNodeID but is labeled nodeID, so it is not the node
+        // asked for. Only the label is wrong, so the map stays sound.
         bool const badPosition = (currNodeID != nodeID);
         SOMETIMES(badPosition, "xrpl::SHAMap::addKnownNode : node ID does not match its position");
         if (badPosition)
@@ -710,6 +718,14 @@ SHAMap::addKnownNode(
             return SHAMapAddNode::invalid();
         }
 
+        // A leaf's own key names its position, and the hash test above ties this leaf to this
+        // parent, so the verdict belongs to the map. Below badPosition, since Invalid is terminal.
+        if (!belongsAt(nodeID, *treeNode))
+        {
+            condemn(*treeNode, treeNode->getHash(), nodeID);
+            return SHAMapAddNode::mapInvalidated();
+        }
+
         if (backed_)
             canonicalize(childHash, treeNode);
 
@@ -875,7 +891,7 @@ SHAMap::getProofPath(UInt256 const& key) const
         return {};
     }
 
-    if (auto const& node = stack.top().first; !node || node->isInner() ||
+    if (auto const& node = stack.top(); !node || node->isInner() ||
         intr_ptr::staticPointerCast<SHAMapLeafNode>(node)->peekItem()->key() != key)
     {
         JLOG(journal_.debug()) << "no path to " << key;
@@ -887,7 +903,7 @@ SHAMap::getProofPath(UInt256 const& key) const
     while (!stack.empty())
     {
         Serializer s;
-        stack.top().first->serializeForWire(s);
+        stack.top()->serializeForWire(s);
         path.emplace_back(std::move(s.modData()));
         stack.pop();
     }

src/libxrpl/shamap/SHAMapTreeNode.cpp

@@ -35,12 +35,12 @@ SHAMapTreeNode::makeTransaction(Slice data, SHAMapHash const& hash, bool hashVal
             "Short TXN node: {} bytes (minimum {} required)", data.size(), kMinShaMapItemBytes));
     }
 
-    auto item = makeShamapitem(sha512Half(HashPrefix::TransactionId, data), data);
+    // A transaction's key is also its node hash, so this digest serves as both.
+    auto const key = sha512Half(HashPrefix::TransactionId, data);
+    auto item = makeShamapitem(key, data);
 
-    if (hashValid)
-        return intr_ptr::makeShared<SHAMapTxLeafNode>(std::move(item), 0, hash);
-
-    return intr_ptr::makeShared<SHAMapTxLeafNode>(std::move(item), 0);
+    return intr_ptr::makeShared<SHAMapTxLeafNode>(
+        std::move(item), 0, hashValid ? hash : SHAMapHash{key});
 }
 
 SHAMapTreeNodePtr

src/test/app/AcquireTestHelpers.h

@@ -10,11 +10,11 @@
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/resource/Charge.h>
-#include <xrpl/shamap/SHAMapAddNode.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <tests/libxrpl/shamap/DeepChain.h>
+#include <tests/libxrpl/shamap/Tally.h>
 
 #include <xrpl.pb.h>
 
@@ -34,33 +34,34 @@
 
 namespace xrpl::test {
 
-// The chain builder needs only libxrpl, so it is shared with the gtest suites; see the header for
-// what keeps the protobuf reply builder below in this tree.
+// Both need only libxrpl, so the gtest suites share them.
 using tests::DeepChain;
+using tests::tallyIs;
 
-// A smallest-possible leaf plus its 4-byte HashPrefix lands one byte short of the floor
-// ConsensusTransSetSF::gotNode() parses at, so a chain's leaf stays below the parse threshold.
+// A smallest-possible leaf plus its 4-byte HashPrefix stays below the floor
+// ConsensusTransSetSF::gotNode() parses at.
 static_assert(
     sizeof(std::uint32_t) + DeepChain::kLeafItemBytes < ConsensusTransSetSF::kMinTxNodeBytesToParse,
     "a smallest-possible leaf must stay below the resubmission floor");
 
+/**
+ * A retry interval short enough that a whole timeout chain costs a fraction of
+ * a second. TimeoutCounter refuses anything at or below 10ms.
+ */
+inline constexpr auto kFastRetry = std::chrono::milliseconds{20};
+
 /**
  * A peer that records what it was charged. Every other method comes from
- * PeerStub.
- *
- * One instance per packet keeps charges() unambiguous about which packet was
- * charged what.
- *
- * charges_ is unguarded: charging happens on the packet path, so every charge
- * lands on the thread that fed the packet in.
+ * PeerStub. Use one instance per packet: that is what keeps charges()
+ * unambiguous about which packet was charged what. charges_ is also unguarded,
+ * and every charge lands on the thread that fed the packet in.
  */
 class ChargeRecordingPeer : public PeerStub
 {
 public:
     /**
      * @param hasTxSet What hasTxSet() reports, which is how an acquisition
-     *        decides whether this peer is worth asking. Defaults to true, so a
-     *        peer handed straight to takeNodes() needs no argument.
+     *        decides whether this peer is worth asking. Defaults to true.
      */
     explicit ChargeRecordingPeer(bool hasTxSet = true) : PeerStub(nextId()), hasTxSet_(hasTxSet)
     {
@@ -343,23 +344,4 @@ waitFor(
     return condition();
 }
 
-/**
- * Whether a batch verdict carries exactly the given counts.
- *
- * The counts, since get() is a log format. It is pinned once, in the
- * SHAMapAddNode tests, and is what to pass BEAST_EXPECTS() as the reason a
- * check here failed.
- *
- * @param san The verdict to check.
- * @param good How many nodes the batch should have hooked in.
- * @param bad How many it should have rejected.
- * @param duplicate How many it should have already held.
- * @return Whether the verdict matches.
- */
-[[nodiscard]] inline bool
-tallyIs(SHAMapAddNode const& san, int good, int bad, int duplicate)
-{
-    return san.getGood() == good && san.getBad() == bad && san.getDuplicate() == duplicate;
-}
-
 }  // namespace xrpl::test

src/test/app/ConsensusTransSetSF_test.cpp

@@ -0,0 +1,239 @@
+#include <test/jtx/Account.h>
+#include <test/jtx/Env.h>
+#include <test/jtx/amount.h>
+#include <test/jtx/pay.h>
+
+#include <xrpld/app/ledger/ConsensusTransSetSF.h>
+#include <xrpld/app/ledger/TransactionMaster.h>
+#include <xrpld/app/misc/Transaction.h>
+
+#include <xrpl/basics/Blob.h>
+#include <xrpl/basics/SHAMapHash.h>
+#include <xrpl/basics/Slice.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/basics/chrono.h>
+#include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/beast/utility/Journal.h>
+#include <xrpl/protocol/Serializer.h>
+#include <xrpl/shamap/Family.h>
+#include <xrpl/shamap/SHAMap.h>
+#include <xrpl/shamap/SHAMapMissingNode.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <utility>
+
+namespace xrpl::test {
+
+/**
+ * ConsensusTransSetSF must answer only for data that hashes to the key it was
+ * asked for. Its callers adopt that hash rather than recomputing one, and
+ * `canonicalize()` publishes the node to the tree node cache every SHAMap of
+ * the family reads.
+ */
+struct ConsensusTransSetSF_test : public beast::unit_test::Suite
+{
+    /**
+     * A node in the hash-prefixed form a filter serves. The hash returned is
+     * the one the parse computed from the contents, so a case checks the filter
+     * against the parse rather than against itself.
+     *
+     * @return The prefixed serialization and the node's own hash.
+     */
+    [[nodiscard]] static std::pair<Blob, SHAMapHash>
+    prefixedNode()
+    {
+        static constexpr auto kWordsPerTx = 4u;
+
+        Serializer wire;
+        for (auto word = 0u; word < kWordsPerTx; ++word)
+            wire.add32(0xDEADBEEFu + word);
+        wire.add8(kWireTypeTransaction);
+
+        auto const node = SHAMapTreeNode::makeFromWire(makeSlice(wire.peekData()));
+        if (!node)
+            return {};
+
+        Serializer prefixed;
+        node->serializeWithPrefix(prefixed);
+        return {prefixed.getData(), node->getHash()};
+    }
+
+    /**
+     * A cache of this filter's own, empty and independent of the application's.
+     *
+     * @param j Where the cache logs.
+     * @return The cache.
+     */
+    [[nodiscard]] static ConsensusTransSetSF::NodeCache
+    ownCache(beast::Journal j)
+    {
+        static constexpr auto kEntries = 64;
+        static constexpr auto kExpiry = std::chrono::seconds{300};
+        return ConsensusTransSetSF::NodeCache{"test", kEntries, kExpiry, stopwatch(), j};
+    }
+
+    /**
+     * A blob filed under a key it does not produce is refused.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testMisfiledCachedNodeIsRefused(jtx::Env& env)
+    {
+        testcase("A cached node that does not hash to its key is refused");
+
+        auto const [blob, realHash] = prefixedNode();
+        BEAST_EXPECT(!blob.empty());
+        if (blob.empty())
+            return;
+
+        // Any key the blob does not produce. The cache is filed under it anyway.
+        SHAMapHash const wrongHash{UInt256{7}};
+        BEAST_EXPECT(wrongHash != realHash);
+
+        auto cache = ownCache(env.app().getJournal("test"));
+        cache.insert(wrongHash, blob);
+
+        ConsensusTransSetSF const sf(env.app(), cache);
+        auto& family = env.app().getNodeFamily();
+
+        SHAMap map{SHAMapType::TRANSACTION, family};
+        BEAST_EXPECT(!map.fetchRoot(wrongHash, &sf));
+
+        // Every SHAMap of this family reads that cache by bare hash, so the node stays out of it.
+        BEAST_EXPECT(!family.getTreeNodeCache()->fetch(wrongHash.asUInt256()));
+
+        // The entry is dropped, which lets the refusal fall through to the transaction arm.
+        Blob stale;
+        BEAST_EXPECT(!cache.retrieve(wrongHash, stale));
+    }
+
+    /**
+     * The same blob under its own key is served, so the refusal above comes
+     * from the comparison rather than from the harness.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testCorrectlyFiledCachedNodeIsServed(jtx::Env& env)
+    {
+        testcase("A cached node that hashes to its key is served");
+
+        auto const [blob, realHash] = prefixedNode();
+        BEAST_EXPECT(!blob.empty());
+        if (blob.empty())
+            return;
+
+        auto cache = ownCache(env.app().getJournal("test"));
+        cache.insert(realHash, blob);
+
+        ConsensusTransSetSF const sf(env.app(), cache);
+        auto& family = env.app().getNodeFamily();
+
+        SHAMap map{SHAMapType::TRANSACTION, family};
+        BEAST_EXPECT(map.fetchRoot(realHash, &sf));
+        BEAST_EXPECT(map.getHash() == realHash);
+
+        // An accepted node is published, which gives the empty cache above its meaning.
+        BEAST_EXPECT(family.getTreeNodeCache()->fetch(realHash.asUInt256()) != nullptr);
+    }
+
+    /**
+     * The cache arm answers first, and an unknown key answers nothing.
+     *
+     * The transaction arm reads a different store, which holds nothing for the
+     * keys used here, so this case reaches the cache arm alone.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testCacheArmAnswersFirst(jtx::Env& env)
+    {
+        testcase("The cache arm answers first, and an unknown key answers nothing");
+
+        auto const [blob, realHash] = prefixedNode();
+        BEAST_EXPECT(!blob.empty());
+        if (blob.empty())
+            return;
+
+        // Filed in the cache, so a hit shows that arm is consulted before the transaction arm.
+        auto cache = ownCache(env.app().getJournal("test"));
+        cache.insert(realHash, blob);
+
+        ConsensusTransSetSF const sf(env.app(), cache);
+        BEAST_EXPECT(sf.getNode(realHash).has_value());
+
+        // A key neither arm holds answers nothing.
+        BEAST_EXPECT(!sf.getNode(SHAMapHash{UInt256{11}}).has_value());
+    }
+
+    /**
+     * A transaction filed under a key it does not serialize to is refused.
+     *
+     * The master transaction store is keyed by a transaction's own id, and
+     * `getCache()` hands that store out by reference, so a caller can file
+     * under any key. `TransactionMaster::fetch()` reaches the same state in
+     * production.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testMisfiledTransactionIsRefused(jtx::Env& env)
+    {
+        testcase("A cached transaction that does not serialize to its key is refused");
+
+        jtx::Account const alice{"alice"};
+        jtx::Account const bob{"bob"};
+        env.fund(jtx::XRP(10000), alice, bob);
+        env.close();
+
+        // A real transaction, so its id is the digest of exactly the bytes getNode() serializes.
+        auto const jt = env.jt(jtx::pay(alice, bob, jtx::XRP(1)));
+        BEAST_EXPECT(jt.stx != nullptr);
+        if (!jt.stx)
+            return;
+
+        std::string reason;
+        auto txn = std::make_shared<Transaction>(jt.stx, reason, env.app());
+        BEAST_EXPECT(txn->getID() == jt.stx->getTransactionID());
+
+        // Any key the transaction does not produce.
+        UInt256 const wrongKey{13};
+        BEAST_EXPECT(wrongKey != jt.stx->getTransactionID());
+
+        auto& txCache = env.app().getMasterTransaction().getCache();
+        static_cast<void>(txCache.canonicalizeReplaceClient(wrongKey, txn));
+
+        // The state the refusal is about, read back through the same call getNode() makes.
+        BEAST_EXPECT(env.app().getMasterTransaction().fetchFromCache(wrongKey) != nullptr);
+
+        // This filter's own cache is empty, so the transaction arm is the arm that answers.
+        auto cache = ownCache(env.app().getJournal("test"));
+        ConsensusTransSetSF const sf(env.app(), cache);
+        BEAST_EXPECT(!sf.getNode(SHAMapHash{wrongKey}).has_value());
+
+        // Control: the same transaction under its own id is served, so the refusal above comes
+        // from the comparison rather than from never reaching the arm.
+        auto const& realKey = jt.stx->getTransactionID();
+        static_cast<void>(txCache.canonicalizeReplaceClient(realKey, txn));
+        BEAST_EXPECT(sf.getNode(SHAMapHash{realKey}).has_value());
+    }
+
+    void
+    run() override
+    {
+        jtx::Env env{*this};
+
+        testMisfiledCachedNodeIsRefused(env);
+        testCorrectlyFiledCachedNodeIsServed(env);
+        testCacheArmAnswersFirst(env);
+        testMisfiledTransactionIsRefused(env);
+    }
+};
+
+BEAST_DEFINE_TESTSUITE(ConsensusTransSetSF, app, xrpl);
+
+}  // namespace xrpl::test

src/test/app/InboundLedger_test.cpp

@@ -5,6 +5,7 @@
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
 
+#include <xrpl/basics/Blob.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/chrono.h>
 #include <xrpl/beast/unit_test/suite.h>
@@ -14,11 +15,12 @@
 #include <xrpl/protocol/LedgerHeader.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/protocol/jss.h>
+#include <xrpl/resource/Fees.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <xrpl.pb.h>
 
-#include <chrono>
 #include <memory>
 #include <mutex>
 #include <set>
@@ -73,6 +75,50 @@ struct TestableInboundLedger final : InboundLedger
         trigger(nullptr, TriggerReason::Added);
     }
 
+    /**
+     * The same, as the timer chain does.
+     */
+    void
+    triggerTimeout()
+    {
+        trigger(nullptr, TriggerReason::Timeout);
+    }
+
+    /**
+     * Record how many timeouts have elapsed.
+     *
+     * @param timeouts The count to record.
+     */
+    void
+    setTimeouts(int timeouts)
+    {
+        ScopedLockType const sl(mtx_);
+        timeouts_ = timeouts;
+    }
+
+    /**
+     * Forget any recorded progress.
+     */
+    void
+    clearProgress()
+    {
+        ScopedLockType const sl(mtx_);
+        progress_ = false;
+    }
+
+    /**
+     * Whether a packet has advanced the acquisition since the flag was last
+     * cleared.
+     *
+     * @return Whether progress has been recorded.
+     */
+    [[nodiscard]] bool
+    madeProgress() const
+    {
+        ScopedLockType const sl(mtx_);
+        return progress_;
+    }
+
     /**
      * Record that every part has been fetched.
      */
@@ -110,17 +156,9 @@ mutableLedger(InboundLedger const& acquire)
 struct InboundLedger_test : public beast::unit_test::Suite
 {
     /**
-     * A retry interval short enough that a whole timeout chain costs a fraction
-     * of a second. TimeoutCounter refuses anything at or below 10ms.
-     */
-    static constexpr auto kFastRetry = std::chrono::milliseconds{20};
-
-    /**
-     * A seed unique to this chain within the suite.
-     *
-     * The Env below is shared, and its node store, fetch packs and remembered
-     * failures are all keyed by hash. A fresh seed per chain gives every chain
-     * distinct hashes, so each case resolves only its own nodes.
+     * A seed unique to this chain within the suite. The shared Env keys its
+     * node store, fetch packs and remembered failures by hash, so a fresh seed
+     * per chain keeps each case resolving only its own nodes.
      *
      * @return The seed.
      */
@@ -259,6 +297,23 @@ struct InboundLedger_test : public beast::unit_test::Suite
         return acquire;
     }
 
+    /**
+     * The chain's state-map nodes as a liAS_NODE reply for the given header.
+     *
+     * @param header The header whose hash and sequence the reply names.
+     * @param chain The chain supplying the nodes.
+     * @param data The nodes to include, each with its claimed position.
+     * @return The reply packet.
+     */
+    static std::shared_ptr<protocol::TMLedgerData>
+    stateNodePacket(
+        LedgerHeader const& header,
+        DeepChain const& chain,
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> const& data)
+    {
+        return packetFor(chain, data, protocol::liAS_NODE, header.hash, header.seq);
+    }
+
     /**
      * A ledger whose maps all resolve locally finishes on the spot, and the
      * finished ledger is immutable and handed on.
@@ -360,6 +415,60 @@ struct InboundLedger_test : public beast::unit_test::Suite
         BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash));
     }
 
+    /**
+     * A ledger completed by a walk rather than by tryDB() is settled before it
+     * is reported complete.
+     *
+     * The order is visible only to a reader of isComplete(), which is read
+     * without mtx_, so this case pins the invariant instead: every ledger the
+     * acquisition reports is immutable.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testWalkSettlesBeforeReportingComplete(jtx::Env& env)
+    {
+        testcase("A ledger completed by a walk is settled before it is reported");
+
+        auto const chain = DeepChain::toLeaf(2, nextSeed());
+        auto const header = makeHeader(chain);
+
+        // Everything except the leaf, so tryDB() can root the state map but its walk still has
+        // something to ask for, which leaves the completion to trigger().
+        storeHeader(env, header);
+        storeStateNodes(env, header, chain, chain.deepestDepth - 1);
+
+        auto const acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        BEAST_EXPECT(!acquire->checkLocal());
+        BEAST_EXPECT(!acquire->isComplete());
+        BEAST_EXPECT(!acquire->isFailed());
+
+        // The walk can finish only now, which places the completion in the walk.
+        storeStateNodes(env, header, chain, chain.deepestDepth);
+
+        acquire->triggerAdded();
+
+        BEAST_EXPECT(acquire->isComplete());
+        BEAST_EXPECT(!acquire->isFailed());
+
+        auto const settled = acquire->getLedger();
+        BEAST_EXPECT(settled != nullptr);
+        if (settled)
+            BEAST_EXPECT(settled->isImmutable());
+
+        // done()'s success arm ran, so the ledger reached LedgerMaster and the hash is absent
+        // from the failure list.
+        BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) != nullptr);
+        BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash));
+    }
+
     /**
      * A ledger whose map goes invalid on the way to being settled must be
      * discarded rather than delivered.
@@ -399,20 +508,18 @@ struct InboundLedger_test : public beast::unit_test::Suite
         if (!ledger)
             return;
 
-        // The state of affairs done() is handed: every part fetched, as far as the caller can tell.
+        // The state done() is handed: every part fetched, as far as the caller can tell.
         acquire->markComplete();
 
         // And the walk that has since reached the verdict.
         auto& stateMap = ledger->stateMap();
-        BEAST_EXPECT(stateMap.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
-        for (auto const& [nodeID, node] : chain.nodesBelowRoot())
-            stateMap.addKnownNode(nodeID, node, nullptr);
+        BEAST_EXPECT(chain.fill(stateMap));
+        BEAST_EXPECT(chain.addRejectedNode(stateMap).isInvalid());
         BEAST_EXPECT(!stateMap.isValid());
 
         acquire->signalDone();
 
-        // complete_ is withdrawn alongside the failure, or every guard that checks it before
-        // failed_ keeps treating this ledger as delivered.
+        // complete_ is withdrawn alongside the failure, since a guard may check it before failed_.
         BEAST_EXPECT(!acquire->isComplete());
         BEAST_EXPECT(acquire->isFailed());
 
@@ -426,13 +533,77 @@ struct InboundLedger_test : public beast::unit_test::Suite
     }
 
     /**
-     * An acquisition that fails on local data must still signal.
+     * A packet for an abandoned map must fail rather than be read as the map
+     * finishing.
      *
-     * Both entry points that reach tryDB() are covered. done() is what
-     * signals, runs logFailure(), and lands the hash in recentFailures_,
-     * which stops the next round asking for the same ledger.
-     * recentFailures_ is what the assertions watch, since it is the
-     * caller-visible consequence of having signaled.
+     * addKnownNode() reports every node a duplicate rather than invalid once
+     * the map is no longer Synching (see SHAMap::addKnownNode), so
+     * receiveNode() must read isValid() rather than isSynching() alone.
+     *
+     * The tx hash never resolves, so haveTransactions_ stays false and
+     * nothing but the check under test can make this packet report success.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testInvalidatedMapFailsReceiveNode(jtx::Env& env)
+    {
+        testcase("A packet for an abandoned map fails rather than completes");
+
+        // The fabricated chain, so feeding it to the state map invalidates the map.
+        DeepChain const chain{nextSeed()};
+
+        auto const header = makeHeader(UInt256{777}, chain.rootHash.asUInt256());
+        storeHeader(env, header);
+
+        auto const acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        BEAST_EXPECT(!acquire->checkLocal());
+        BEAST_EXPECT(!acquire->isFailed());
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        // Invalidate the state map directly, standing in for a trigger() walk that reached the
+        // same verdict.
+        auto const ledger = mutableLedger(*acquire);
+        BEAST_EXPECT(ledger != nullptr);
+        if (!ledger)
+            return;
+
+        auto& stateMap = ledger->stateMap();
+        BEAST_EXPECT(chain.fill(stateMap));
+        BEAST_EXPECT(chain.addRejectedNode(stateMap).isInvalid());
+        BEAST_EXPECT(!stateMap.isValid());
+
+        // An ordinary state-node packet. addKnownNode()'s "while not synching" branch reports it a
+        // duplicate, whatever it contains, since the map is no longer Synching.
+        auto const latePeer = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(
+            latePeer, stateNodePacket(header, chain, {{chain.idAt(1), chain.nodeAt(1)}})));
+        acquire->runData();
+
+        // The verdict is not this sender's, so it keeps its fee, and the duplicate verdict is not
+        // read as a finished map.
+        BEAST_EXPECT(latePeer->charges().empty());
+        BEAST_EXPECT(acquire->isFailed());
+        BEAST_EXPECT(!acquire->isComplete());
+
+        // Pins the isValid() check, which is what fails the acquisition while the map is abandoned.
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+    }
+
+    /**
+     * An acquisition that fails on local data must still signal. Both entry
+     * points that reach tryDB() are covered.
+     *
+     * done() signals and lands the hash in recentFailures_, which stops the
+     * next round asking for the same ledger. That list is what the assertions
+     * watch, since it is the caller-visible consequence of having signaled.
      *
      * @param env The environment to run in.
      */
@@ -502,6 +673,357 @@ struct InboundLedger_test : public beast::unit_test::Suite
             waitFor([&] { return env.app().getInboundLedgers().isFailure(otherHeader.hash); }));
     }
 
+    /**
+     * An acquisition whose state root names a shape no valid tree can have must
+     * fail, and must cost the sender the harsher tier.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testFabricatedChainFailsAcquire(jtx::Env& env)
+    {
+        testcase("A state-map chain reaching kLeafDepth fails the acquire");
+
+        DeepChain const chain{nextSeed()};
+        auto const header = makeHeader(chain);
+
+        auto const acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        // The header is accepted on its own terms, so the acquisition now chases this hash.
+        auto const headerPeer = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(headerPeer, headerPacket(header)));
+        acquire->runData();
+        BEAST_EXPECT(headerPeer->charges().empty());
+        BEAST_EXPECT(!acquire->isFailed());
+
+        // The root, then the rest of the chain ending in the inner node at kLeafDepth.
+        auto const data = chain.allNodes();
+
+        // The header counted as progress, so clear it to see what the packet below records.
+        acquire->clearProgress();
+
+        auto const chainPeer = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(chainPeer, stateNodePacket(header, chain, data)));
+        acquire->runData();
+
+        // The acquisition is over, and stays over: no peer can satisfy this hash.
+        BEAST_EXPECT(acquire->isFailed());
+        BEAST_EXPECT(!acquire->isComplete());
+
+        // The nodes ahead of the bad one belong to the same impossible tree, so the packet counts
+        // nothing at all.
+        BEAST_EXPECT(!acquire->madeProgress());
+
+        // A failed acquisition reports no ledger, though it still holds the partial one it built.
+        BEAST_EXPECT(acquire->getLedger() == nullptr);
+
+        BEAST_EXPECT(chainPeer->charges() == std::vector{resource::kFeeMalformedData});
+
+        // getJson() walks the same maps to report what is still needed, and comes back with an
+        // empty list, since the walk stops at the abandoned map.
+        auto const report = acquire->getJson(0);
+        BEAST_EXPECT(report[jss::failed].asBool());
+        BEAST_EXPECT(report[jss::have_header].asBool());
+        BEAST_EXPECT(!report[jss::have_state].asBool());
+        BEAST_EXPECT(report[jss::needed_state_hashes].size() == 0);
+    }
+
+    /**
+     * A merely-wrong state node must cost the recoverable tier and leave the
+     * acquisition alive. The fee split in receiveNode() turns on whether this
+     * node proved the map impossible, so a node that cannot be hooked but
+     * leaves the map sound is charged the lower tier.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testWrongStateNodeKeepsAcquireAlive(jtx::Env& env)
+    {
+        testcase("A merely-wrong state node leaves the acquire recoverable");
+
+        DeepChain const chain{nextSeed()};
+        auto const header = makeHeader(chain);
+
+        auto const acquire = std::make_shared<InboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        auto const headerPeer = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(headerPeer, headerPacket(header)));
+        acquire->runData();
+        BEAST_EXPECT(!acquire->isFailed());
+
+        auto const rootPeer = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(
+            rootPeer, stateNodePacket(header, chain, {{SHAMapNodeID{}, chain.nodeAt(0)}})));
+        acquire->runData();
+        BEAST_EXPECT(rootPeer->charges().empty());
+
+        // nodeAt(1) is the node the root is missing and its hash matches, but it is labeled as
+        // living at depth 2, so it cannot be hooked anywhere.
+        auto const wrongPeer = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(
+            wrongPeer,
+            stateNodePacket(header, chain, {{SHAMapNodeID{2, UInt256{}}, chain.nodeAt(1)}})));
+        acquire->runData();
+
+        BEAST_EXPECT(wrongPeer->charges() == std::vector{resource::kFeeInvalidData});
+
+        // The map is sound, so the acquisition is still going and still holds its ledger.
+        BEAST_EXPECT(!acquire->isFailed());
+        BEAST_EXPECT(!acquire->isComplete());
+        BEAST_EXPECT(acquire->getLedger() != nullptr);
+    }
+
+    /**
+     * A ledger assembled from local data must be judged even when only one map
+     * is settled.
+     *
+     * A fetch pack is checked against each node's own hash rather than the
+     * shape it implies, so a whole chain can resolve locally without passing
+     * through addKnownNode(). Here the transaction map is the chain, so its walk
+     * abandons it, while the state root is a hash no fetch pack supplies,
+     * leaving that map merely incomplete. tryDB() sets neither flag, so it has
+     * to reach the verdict itself.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testLocalChainFailsAcquire(jtx::Env& env)
+    {
+        testcase("A chain found locally fails the acquire");
+
+        DeepChain const chain{nextSeed()};
+
+        // The chain as the transaction root, and an arbitrary hash, seeded nowhere, as the state
+        // root.
+        auto const header = makeHeader(chain.rootHash.asUInt256(), UInt256{99});
+        auto& ledgerMaster = env.app().getLedgerMaster();
+
+        // The header, prefixed the way tryDB() expects to find it in a fetch pack.
+        Serializer hs;
+        hs.add32(HashPrefix::LedgerMaster);
+        addRaw(header, hs);
+        ledgerMaster.addFetchPack(header.hash, std::make_shared<Blob>(hs.modData()));
+
+        // Every node of the chain, keyed by its own hash. TransactionStateSF::getNode() reads
+        // these, so the transaction-map walk resolves the whole chain with no peer involved.
+        for (auto depth = 0u; depth <= SHAMap::kLeafDepth; ++depth)
+        {
+            ledgerMaster.addFetchPack(
+                chain.nodeAt(depth)->getHash().asUInt256(),
+                std::make_shared<Blob>(chain.prefixedNodeAt(depth)));
+        }
+
+        auto const acquire = std::make_shared<InboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        // checkLocal() routes into tryDB() with no peer data. It reports true only because the
+        // acquisition ended.
+        BEAST_EXPECT(acquire->checkLocal());
+
+        BEAST_EXPECT(acquire->isFailed());
+        BEAST_EXPECT(!acquire->isComplete());
+
+        // A failed acquisition reports no ledger, though it still holds the partial one it built.
+        BEAST_EXPECT(acquire->getLedger() == nullptr);
+    }
+
+    /**
+     * The aggressive-retry branch of trigger() must judge a map the walk
+     * abandoned.
+     *
+     * That branch reads an empty getNeededHashes() result as a complete map,
+     * and the walk it runs can reach the invalid verdict itself once nodes
+     * resolve from local storage. Only the root is local when tryDB() runs, so
+     * the state map holds a root and the walk stops one level down. The branch
+     * also needs a timeout count above kLedgerBecomeAggressiveThreshold, which
+     * the case records directly rather than waiting out the timer chain.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testAggressiveRetryJudgesLocalMap(jtx::Env& env)
+    {
+        testcase("An aggressive retry judges a map the walk abandoned");
+
+        DeepChain const chain{nextSeed()};
+
+        // The chain as the state root, and no transactions, so only the state map is in play.
+        auto const header = makeHeader(chain);
+        auto& ledgerMaster = env.app().getLedgerMaster();
+
+        Serializer hs;
+        hs.add32(HashPrefix::LedgerMaster);
+        addRaw(header, hs);
+        ledgerMaster.addFetchPack(header.hash, std::make_shared<Blob>(hs.modData()));
+
+        // Only the root, so the state map gets a root but the walk stops one level down.
+        ledgerMaster.addFetchPack(
+            chain.nodeAt(0)->getHash().asUInt256(),
+            std::make_shared<Blob>(chain.prefixedNodeAt(0)));
+
+        auto const acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        // The acquisition is alive: it has the header and a state root, and still wants the rest.
+        BEAST_EXPECT(!acquire->checkLocal());
+        BEAST_EXPECT(!acquire->isFailed());
+        BEAST_EXPECT(acquire->getJson(0)[jss::have_header].asBool());
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        auto const ledger = mutableLedger(*acquire);
+        BEAST_EXPECT(ledger != nullptr);
+        if (!ledger)
+            return;
+        BEAST_EXPECT(ledger->stateMap().isValid());
+
+        // The rest of the chain becomes resolvable only now, which places the verdict in this walk.
+        for (auto depth = 1u; depth <= SHAMap::kLeafDepth; ++depth)
+        {
+            ledgerMaster.addFetchPack(
+                chain.nodeAt(depth)->getHash().asUInt256(),
+                std::make_shared<Blob>(chain.prefixedNodeAt(depth)));
+        }
+
+        // kLedgerBecomeAggressiveThreshold is 4 and file-local, so name the requirement here.
+        acquire->setTimeouts(5);
+        acquire->clearProgress();
+        acquire->triggerTimeout();
+
+        // The walk resolved the chain locally and abandoned the map, and trigger() recorded that
+        // rather than reading the empty result as a finished acquisition.
+        BEAST_EXPECT(!ledger->stateMap().isValid());
+        BEAST_EXPECT(acquire->isFailed());
+        BEAST_EXPECT(!acquire->isComplete());
+
+        // have_state is the discriminating assertion: only this guard leaves it false, since every
+        // have-flag is set on the way to the setImmutable() backstop in done().
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        // The same branch with no header, which is the other arm of hasInvalidMap(): no map, so
+        // the arm answers false. getNeededHashes() has asked for the header, so the non-empty
+        // branch is the right one and the acquisition stays alive.
+        auto const headerless = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            UInt256{7},
+            0,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        headerless->setTimeouts(5);
+        headerless->clearProgress();
+        headerless->triggerTimeout();
+
+        BEAST_EXPECT(mutableLedger(*headerless) == nullptr);
+        BEAST_EXPECT(!headerless->isFailed());
+        BEAST_EXPECT(!headerless->isComplete());
+    }
+
+    /**
+     * The ordinary trigger() path must judge a map its own walk abandoned.
+     *
+     * Covers the state-map walk trigger() runs with mtx_ released, where the
+     * case above covers the empty getNeededHashes() branch. That verdict has to
+     * be read before the flags below it. Staged as that case is, with the
+     * timeout count left at zero to stay off the aggressive branch.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testWalkJudgesMapOnOrdinaryTrigger(jtx::Env& env)
+    {
+        testcase("An ordinary trigger judges a map its walk abandoned");
+
+        DeepChain const chain{nextSeed()};
+
+        auto const header = makeHeader(chain);
+        auto& ledgerMaster = env.app().getLedgerMaster();
+
+        Serializer hs;
+        hs.add32(HashPrefix::LedgerMaster);
+        addRaw(header, hs);
+        ledgerMaster.addFetchPack(header.hash, std::make_shared<Blob>(hs.modData()));
+
+        // Only the root, so the state map gets a root but the walk stops one level down.
+        ledgerMaster.addFetchPack(
+            chain.nodeAt(0)->getHash().asUInt256(),
+            std::make_shared<Blob>(chain.prefixedNodeAt(0)));
+
+        auto peerSet = std::make_unique<RequestCountingPeerSet>();
+        auto const* const peerSetPtr = peerSet.get();
+
+        auto const acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::move(peerSet));
+
+        BEAST_EXPECT(!acquire->checkLocal());
+        BEAST_EXPECT(!acquire->isFailed());
+        BEAST_EXPECT(acquire->getJson(0)[jss::have_header].asBool());
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        auto const ledger = mutableLedger(*acquire);
+        BEAST_EXPECT(ledger != nullptr);
+        if (!ledger)
+            return;
+        BEAST_EXPECT(ledger->stateMap().isValid());
+
+        // The rest of the chain becomes resolvable only now, which places the verdict in this walk.
+        for (auto depth = 1u; depth <= SHAMap::kLeafDepth; ++depth)
+        {
+            ledgerMaster.addFetchPack(
+                chain.nodeAt(depth)->getHash().asUInt256(),
+                std::make_shared<Blob>(chain.prefixedNodeAt(depth)));
+        }
+
+        // Sound going in, which is the first half of the tripwire below.
+        BEAST_EXPECT(ledger->stateMap().isValid());
+
+        int const requestsBefore = peerSetPtr->requests();
+        acquire->triggerAdded();
+
+        // The walk resolved the chain locally and abandoned the map, and trigger() recorded that
+        // rather than reading the empty node list as a finished state map.
+        BEAST_EXPECT(!ledger->stateMap().isValid());
+        BEAST_EXPECT(acquire->isFailed());
+
+        // have_state is the discriminating assertion. isComplete() is not: it reads false whether
+        // this guard fired or the setImmutable() backstop in done() did.
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        // A tripwire that keeps this case on the branch it names: the aggressive branch needs both
+        // a TIMEOUT reason and a count above kLedgerBecomeAggressiveThreshold.
+        BEAST_EXPECT(acquire->getJson(0)[jss::timeouts].asInt() == 0);
+
+        // The request count is unchanged after the verdict, so the guard ended the round.
+        BEAST_EXPECT(peerSetPtr->requests() == requestsBefore);
+    }
+
     /**
      * A peer-supplied header whose account hash is zero fails the acquisition
      * and signals, as a local one does. The peer is not charged, and
@@ -604,17 +1126,10 @@ struct InboundLedger_test : public beast::unit_test::Suite
     /**
      * The retry timer re-asks, then gives up and signals.
      *
-     * The only case that drives onTimer() rather than trigger() directly,
-     * which is what covers the give-up: past kLedgerTimeoutRetriesMax the
-     * acquisition fails itself and done() records that, so the same
-     * doomed ledger is not asked for again on the next round. It is also
-     * what the retry interval is a constructor parameter for, since the
-     * chain runs past kLedgerTimeoutRetriesMax ticks of three seconds
-     * apiece in production.
-     *
-     * The store is empty for this hash and no reply arrives, so every tick
-     * counts a timeout and the count climbs to the limit. A hash of its own,
-     * so this case owns its entry in the failure list.
+     * The only case that drives onTimer() rather than trigger() directly. Past
+     * kLedgerTimeoutRetriesMax the acquisition fails itself and done() records
+     * that, so the same ledger is not asked for again. The store is empty for
+     * this hash and no reply arrives, so every tick counts a timeout.
      *
      * @param env The environment to run in.
      */
@@ -662,24 +1177,108 @@ struct InboundLedger_test : public beast::unit_test::Suite
         BEAST_EXPECT(!acquire->isComplete());
         BEAST_EXPECT(peerSetPtr->requests() > requestsFromInit);
 
-        // done() remembered the hash, which is what stops the next round asking again.
+        // No ledger was ever built here, so both arms of the failed_ gate answer null. The
+        // local-chain case above covers the gate itself.
+        BEAST_EXPECT(acquire->getLedger() == nullptr);
         BEAST_EXPECT(
             waitFor([&] { return env.app().getInboundLedgers().isFailure(kUnknownLedger); }));
     }
 
+    /**
+     * A packet that leaves nothing to fetch must settle the ledger itself,
+     * rather than count on a later walk to notice.
+     *
+     * A map holding one item has that item's leaf for its root, so rooting it
+     * stops it synching on the spot. The packet here offers that root to a map
+     * that already holds it, which counts no good nodes, so runData() takes the
+     * sender for one that returned nothing and asks it nothing further. That
+     * leaves receiveNode() as the only thing that can settle this ledger.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testCompletesWhenNothingIsLeftToFetch(jtx::Env& env)
+    {
+        testcase("A packet that leaves nothing to fetch settles the ledger");
+
+        // A leaf at the root, so the state map holds exactly one item. No transactions, which
+        // leaves the state map the only part the acquisition needs.
+        auto const chain = DeepChain::toLeaf(0, nextSeed());
+        auto const header = makeHeader(chain);
+
+        // Only the header is local, so the state map starts with no root of its own.
+        storeHeader(env, header);
+
+        auto peerSet = std::make_unique<RequestCountingPeerSet>();
+        auto const* const peerSetPtr = peerSet.get();
+
+        auto const acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::move(peerSet));
+
+        BEAST_EXPECT(!acquire->checkLocal());
+        BEAST_EXPECT(!acquire->isComplete());
+        BEAST_EXPECT(!acquire->isFailed());
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        // Root the map directly, standing in for a trigger() walk that got that far. The flag is
+        // the acquisition's own, so it stays clear while the map is already whole.
+        auto const ledger = mutableLedger(*acquire);
+        BEAST_EXPECT(ledger != nullptr);
+        if (!ledger)
+            return;
+
+        auto& stateMap = ledger->stateMap();
+        BEAST_EXPECT(stateMap.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isUseful());
+        BEAST_EXPECT(!stateMap.isSynching());
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        // The same root again. A leaf carries its own key, so a depth of zero names the root and
+        // the node goes through addRootNode(), which reports a node already held.
+        auto const sender = std::make_shared<ChargeRecordingPeer>();
+        BEAST_EXPECT(acquire->gotData(sender, stateNodePacket(header, chain, chain.allNodes())));
+        acquire->runData();
+
+        // Nothing was asked of the sender afterwards, so no walk ran to reach the same verdict.
+        BEAST_EXPECT(!acquire->madeProgress());
+        BEAST_EXPECT(peerSetPtr->requests() == 0);
+
+        // The sender offered nothing bad, so it keeps its fee, and the map is now reported held.
+        BEAST_EXPECT(sender->charges().empty());
+        BEAST_EXPECT(acquire->getJson(0)[jss::have_state].asBool());
+
+        // Settled, reported complete and handed on, all from this one packet.
+        BEAST_EXPECT(acquire->isComplete());
+        BEAST_EXPECT(!acquire->isFailed());
+        BEAST_EXPECT(ledger->isImmutable());
+
+        BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) != nullptr);
+        BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash));
+    }
+
     void
     run() override
     {
-        // One Env for the suite, since building one costs far more than any case here. Safe
-        // because every chain is seeded through nextSeed(): the node store, the fetch packs and
-        // the remembered failures are all shared, and all three are keyed by hash.
+        // One Env for the suite, shared safely because every chain is seeded through nextSeed().
         jtx::Env env{*this};
 
         testLocalLedgerCompletesAcquire(env);
+        testWalkSettlesBeforeReportingComplete(env);
         testInvalidatedLedgerFailsInDone(env);
+        testInvalidatedMapFailsReceiveNode(env);
         testLocalFailureSignalsDone(env);
         testPeerZeroAccountHashFails(env);
         testPeerHeaderWithoutTransactionsCompletes(env);
+        testFabricatedChainFailsAcquire(env);
+        testWrongStateNodeKeepsAcquireAlive(env);
+        testLocalChainFailsAcquire(env);
+        testAggressiveRetryJudgesLocalMap(env);
+        testWalkJudgesMapOnOrdinaryTrigger(env);
+        testCompletesWhenNothingIsLeftToFetch(env);
 
         // Last: the only case that waits out a whole timeout chain.
         testTimerRetriesThenGivesUp(env);

src/tests/libxrpl/shamap/DeepChain.h

@@ -155,16 +155,26 @@ struct DeepChain
         return data;
     }
 
+    /**
+     * Every node in the chain, root first.
+     *
+     * @return The nodes, each with its claimed position.
+     */
+    [[nodiscard]] std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>>
+    allNodes() const
+    {
+        return nodesBelowRoot(0);
+    }
+
     /**
      * Fill a synching map, stopping one level short of the deepest node so the
      * caller offers that one itself.
      *
-     * Reports rather than asserts: the two binaries sharing this header use
-     * different test frameworks.
+     * Returns rather than asserts, since two test frameworks share this header.
      *
      * @param map The map to fill.
      * @return Whether the root and every node above the deepest one was
-     *         accepted, which is a property of the chain.
+     *         accepted.
      */
     [[nodiscard]] bool
     fill(SHAMap& map) const
@@ -182,7 +192,7 @@ struct DeepChain
     }
 
     /**
-     * Offer the deepest node, which for a fabricated chain is the inner node at
+     * Offer the deepest node, which without a leaf is the inner node at
      * SHAMap::kLeafDepth, a depth only a leaf may occupy.
      *
      * A chain whose only node is the root has nothing below it to offer, and
@@ -193,7 +203,7 @@ struct DeepChain
      * @return The verdict addKnownNode() reached.
      */
     [[nodiscard]] SHAMapAddNode
-    addOffendingNode(SHAMap& map) const
+    addRejectedNode(SHAMap& map) const
     {
         if (deepestDepth == 0)
             Throw<std::logic_error>("DeepChain: the root is not offered through addKnownNode()");
@@ -255,14 +265,14 @@ private:
     }
 
     /**
-     * Fill in inner nodes, each with one real child (and, under Decoy::Yes, an
-     * additional unresolvable second child), from the root down to the given depth,
-     * and record the root hash.
+     * Fill in inner nodes from the root down to the given depth and record the
+     * root hash. Each carries one real child and, under Decoy::Yes, an
+     * unresolvable second one.
      *
      * Bottom-up, since each node's hash covers the child hash below it.
      *
      * @param deepest The depth of the deepest inner node to build. May be
-     *        SHAMap::kLeafDepth, which is the fabricated chain's whole point.
+     *        SHAMap::kLeafDepth.
      * @param childHash What that deepest inner node points at.
      * @param decoy Whether to add an unresolvable second child at every level.
      */
@@ -277,10 +287,9 @@ private:
             --depth;
 
             // A key has 64 nibbles, so SHAMap::kLeafDepth is one past the last nibble
-            // selectBranch() reads from a 32-byte key. A fabricated chain's pathKey is zero, so
-            // branch 0 is the position such a node claims.
-            auto const branch =
-                depth == SHAMap::kLeafDepth ? 0u : selectBranch(idAt(depth), pathKey);
+            // selectBranch() reads from a 32-byte key. Such a chain has a zero pathKey, so branch
+            // 0 is the position it claims.
+            auto const branch = depth == SHAMap::kLeafDepth ? 0u : selectBranch(depth, pathKey);
 
             Serializer s;
             s.addBitString(childHash.asUInt256());

src/tests/libxrpl/shamap/SHAMap.cpp

@@ -7,7 +7,11 @@
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/Zero.h>
+#include <xrpl/nodestore/Database.h>
+#include <xrpl/nodestore/NodeObject.h>
+#include <xrpl/protocol/HashPrefix.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>
@@ -17,6 +21,8 @@
 
 #include <gtest/gtest.h>
 #include <helpers/TestSink.h>
+#include <shamap/InnerNode.h>
+#include <shamap/SyncFilter.h>
 #include <shamap/common.h>
 
 #include <algorithm>
@@ -25,6 +31,7 @@
 #include <cstdint>
 #include <format>
 #include <memory>
+#include <optional>
 #include <string>
 #include <string_view>
 #include <type_traits>
@@ -273,9 +280,12 @@ TEST_P(SHAMapTest, the_hash_after_each_add_is_undone_by_the_matching_delete)
         map.invariants();
     }
 
-    // Removing the items in reverse must walk back through the same hashes.
-    for (std::size_t k = kKeys.size(); k-- > 0;)
+    // Removing the items in reverse must walk back through the same hashes. The decrement is in
+    // the body, so the counter never steps below zero. UndefinedBehaviorSanitizer reports that
+    // wraparound.
+    for (std::size_t k = kKeys.size(); k > 0;)
     {
+        --k;
         EXPECT_EQ(map.getHash().asUInt256(), kHashes[k]);
         EXPECT_TRUE(map.delItem(kKeys[k]));
         map.invariants();
@@ -317,9 +327,8 @@ INSTANTIATE_TEST_SUITE_P(
     ::testing::Values(kBackedMode, kUnbackedMode),
     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.
+// Exercises the traversal paths built by belowHelper. A path names each node's position by its own
+// length, and every push requires a leaf's key to lie under the branch it was reached through.
 class SHAMapTraversal : public ::testing::Test
 {
 protected:
@@ -500,15 +509,64 @@ TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys)
     }
 }
 
+TEST_F(SHAMapTraversal, update_give_item_on_absent_key_returns_false)
+{
+    tests::TestNodeFamily f{j_};
+    auto keys = deepFanOutKeys();
+    SHAMap map{SHAMapType::FREE, f};
+    fillMap(map, keys);
+
+    // A key absent from a fanned-out map, so the walk ends on an inner node with an empty branch.
+    auto const absentKey = UInt256{std::string_view{std::string(64, '0')}};
+    Buffer vuc{32};
+    std::fill_n(vuc.data(), vuc.size(), std::uint8_t{2});
+    EXPECT_FALSE(map.updateGiveItem(
+        SHAMapNodeType::TnAccountState, makeShamapitem(absentKey, std::move(vuc))));
+
+    // A single-item map probed with a key that selects the same root branch, so the walk ends on
+    // the leaf it reached, whose key is not the one asked for.
+    SHAMap single{SHAMapType::FREE, f};
+    fillMap(single, {keys.front()});
+
+    auto probe = keys.front();
+    std::fill_n(probe.begin() + 1, probe.size() - 1, std::uint8_t{0});
+    ASSERT_NE(probe, keys.front());
+
+    Buffer other{32};
+    std::fill_n(other.data(), other.size(), std::uint8_t{3});
+    EXPECT_FALSE(single.updateGiveItem(
+        SHAMapNodeType::TnAccountState, makeShamapitem(probe, std::move(other))));
+}
+
+TEST_F(SHAMapTraversal, del_item_on_absent_key_returns_false)
+{
+    tests::TestNodeFamily f{j_};
+    auto const keys = deepFanOutKeys();
+    SHAMap map{SHAMapType::FREE, f};
+    fillMap(map, keys);
+
+    auto const before = map.getHash();
+
+    // Every key of this map shares its first nibble, so the root leaves the branch this key selects
+    // empty and the walk ends with an inner node on top of the path rather than a leaf.
+    auto const absentKey = UInt256{std::string_view{std::string(64, '0')}};
+    EXPECT_FALSE(map.delItem(absentKey));
+
+    // The refusal leaves the map as it was, which an answer of false alone does not say.
+    EXPECT_EQ(map.getHash(), before);
+    map.invariants();
+    for (auto const& key : keys)
+        EXPECT_TRUE(map.hasItem(key)) << to_string(key);
+}
+
 TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end)
 {
     tests::TestNodeFamily f{j_};
     SHAMap map{SHAMapType::FREE, f};
     map.setUnbacked();
 
-    // The root is a childless inner node, so boundHelper's inner-node branch scans every branch on
-    // the requested side of the one id selects, finds them all empty, and falls through to end()
-    // rather than dereference a child.
+    // An empty map still leaves its root on the path, so boundHelper reads a non-empty path and
+    // answers end() from it.
     EXPECT_EQ(map.upperBound(UInt256{}), map.end());
     EXPECT_EQ(map.lowerBound(UInt256{}), map.end());
 
@@ -849,7 +907,7 @@ TEST_F(SHAMapPathProof, verify_proof_path)
 
 // A legitimate proof path for two keys sharing all 63 leading nibbles is 65 elements: inner nodes
 // at depths 0..63 plus the leaf at depth 64. This pins that the 65 bound is real, so the fix for
-// the forged-path case below must not simply tighten the length limit.
+// the built-path case below must not simply tighten the length limit.
 TEST_F(SHAMapPathProof, legitimate_deep_path_is_sixty_five_elements)
 {
     tests::TestNodeFamily f{j_};
@@ -882,7 +940,7 @@ TEST_F(SHAMapPathProof, legitimate_deep_path_is_sixty_five_elements)
     // NOLINTEND(bugprone-unchecked-optional-access)
 }
 
-// A forged path of 65 hash-chained inner nodes reaches depth kLeafDepth, where only the leaf
+// A built path of 65 hash-chained inner nodes reaches depth kLeafDepth, where only the leaf
 // terminating the path may sit. Such a path must be rejected.
 TEST_F(SHAMapPathProof, all_inner_path_at_leaf_depth_is_rejected)
 {
@@ -894,8 +952,11 @@ TEST_F(SHAMapPathProof, all_inner_path_at_leaf_depth_is_rejected)
     std::vector<Blob> path;
     SHAMapHash childHash{UInt256{1}};
 
-    for (auto depth = SHAMap::kLeafDepth + 1u; depth-- > 0;)
+    // The decrement is in the body, so the counter never steps below zero.
+    // UndefinedBehaviorSanitizer reports that wraparound.
+    for (auto depth = SHAMap::kLeafDepth + 1u; depth > 0;)
     {
+        --depth;
         auto const id = SHAMapNodeID::createID(std::min(depth, SHAMap::kLeafDepth - 1u), kTestKey);
         auto const branch = selectBranch(id, kTestKey);
 
@@ -916,25 +977,25 @@ TEST_F(SHAMapPathProof, all_inner_path_at_leaf_depth_is_rejected)
 }
 
 /**
- * Wrap a leaf blob in a forged root inner node whose branch for `key` carries that leaf's hash.
+ * Wrap a leaf blob in a built root inner node whose branch for `key` carries that leaf's hash.
  *
  * The resulting two-element path hash-chains for `key` no matter which leaf sits at the bottom,
  * which is exactly the substitution a peer could attempt.
  *
  * @param leafBlob the wire form of the leaf to place at the bottom of the path.
- * @param key the key the forged path claims to prove.
- * @return the path (deepest element first) and the forged root hash, or an empty path if the leaf
+ * @param key the key the built path claims to prove.
+ * @return the path (deepest element first) and the built root hash, or an empty path if the leaf
  *         blob does not parse.
  */
 [[nodiscard]] static std::pair<std::vector<Blob>, UInt256>
-forgeRootOverLeaf(Blob const& leafBlob, UInt256 const& key)
+buildRootOverLeaf(Blob const& leafBlob, UInt256 const& key)
 {
     auto const leaf = SHAMapTreeNode::makeFromWire(makeSlice(leafBlob));
     if (!leaf || !leaf->isLeaf())
         return {};
     leaf->updateHash();
 
-    auto const branch = selectBranch(SHAMapNodeID::createID(0, key), key);
+    auto const branch = selectBranch(0u, key);
     Serializer s;
     for (auto i = 0u; i < SHAMap::kBranchFactor; ++i)
         s.addBitString(i == branch ? leaf->getHash().asUInt256() : UInt256{});
@@ -949,7 +1010,7 @@ forgeRootOverLeaf(Blob const& leafBlob, UInt256 const& key)
 }
 
 // The hash chain above a leaf proves nothing about which key that leaf holds, so a peer can graft a
-// genuine leaf from elsewhere in the map onto a path forged for another key. Comparing the terminal
+// genuine leaf from elsewhere in the map onto a path built for another key. Comparing the terminal
 // leaf's own key against the key being proved is what rejects it.
 TEST_F(SHAMapPathProof, substituted_leaf_for_other_key_is_rejected)
 {
@@ -979,17 +1040,765 @@ TEST_F(SHAMapPathProof, substituted_leaf_for_other_key_is_rejected)
     auto const& otherLeaf = otherPath->front();
     // NOLINTEND(bugprone-unchecked-optional-access)
 
-    // Control: the forged root is accepted when the leaf below it really is kKey's leaf, so the
+    // Control: the built root is accepted when the leaf below it really is kKey's leaf, so the
     // rejection below can only come from the leaf key comparison.
-    auto const [goodPath, goodRoot] = forgeRootOverLeaf(ownLeaf, kKey);
+    auto const [goodPath, goodRoot] = buildRootOverLeaf(ownLeaf, kKey);
     ASSERT_EQ(goodPath.size(), 2u);
     EXPECT_TRUE(SHAMap::verifyProofPath(goodRoot, kKey, goodPath));
 
-    // Same forged root, but kOtherKey's leaf substituted at the bottom: the hash chain still
+    // Same built root, but kOtherKey's leaf substituted at the bottom: the hash chain still
     // validates, yet the path does not prove anything about kKey.
-    auto const [badPath, badRoot] = forgeRootOverLeaf(otherLeaf, kKey);
+    auto const [badPath, badRoot] = buildRootOverLeaf(otherLeaf, kKey);
     ASSERT_EQ(badPath.size(), 2u);
     EXPECT_FALSE(SHAMap::verifyProofPath(badRoot, kKey, badPath));
 }
 
+/**
+ * Run `call` and check it throws SHAMapMissingNode naming a key, not a hash.
+ *
+ * A walk that decides it cannot go on names the key it was searching for, while
+ * a failure to fetch names the hash. Both throw the same type, so the payload
+ * is what tells the two apart.
+ *
+ * @param call the call expected to throw.
+ */
+template <class Call>
+void
+expectThrowNamingKey(Call&& call)
+{
+    try
+    {
+        call();
+        ADD_FAILURE() << "expected SHAMapMissingNode, nothing was thrown";
+    }
+    catch (SHAMapMissingNode const& e)
+    {
+        std::string const what{e.what()};
+        EXPECT_NE(what.find(": id "), std::string::npos) << what;
+        EXPECT_EQ(what.find(": hash "), std::string::npos) << what;
+    }
+}
+
+/**
+ * What a test needs to install a root inner node and resolve its children
+ * itself.
+ *
+ * A node's hash covers its contents rather than its position, so the fixtures
+ * below can record a correctly-built leaf under a branch of their choosing.
+ */
+class BuiltRootTest : 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"};
+
+    // The sequence a test writes stored nodes under, and sets on the map that reads them back. Any
+    // value serves: the nodestore is keyed by hash and takes this only as a lookup hint.
+    static constexpr std::uint32_t kStoredLedgerSeq = 11;
+
+    /**
+     * Assemble `map` as a root inner node holding the given children.
+     *
+     * The root is installed directly, so each child stays unresolved until a
+     * walk consults the filter for it. It takes a list rather than one hash
+     * because a walk reaches the sideways scans in `peekNextItem` and
+     * `boundHelper` only from a branch it can already resolve, which needs a
+     * second, well-placed child.
+     *
+     * @param map the map to assemble, which must be synching and empty.
+     * @param children the branch and hash of each child to record.
+     * @return whether the root was accepted.
+     */
+    static bool
+    buildRoot(SHAMap& map, std::vector<InnerChild> const& children)
+    {
+        auto root = makeFullInnerNode(children);
+        if (!root)
+            return false;
+
+        auto const rootHash = root->getHash();
+        return map.addRootNode(rootHash, std::move(root), nullptr).isGood();
+    }
+
+    /**
+     * A correctly-built leaf for the given key, in the form a sync filter
+     * serves.
+     *
+     * Serialized with its prefix rather than in wire form, since that is what
+     * checkFilter parses.
+     *
+     * @param f the family the source map belongs to.
+     * @param key the key to build the leaf for.
+     * @return the leaf's prefixed form and its hash, or an empty blob if the
+     *         map rejected the item.
+     */
+    static std::pair<Blob, SHAMapHash>
+    leafFor(Family& f, UInt256 const& key)
+    {
+        SHAMap source{SHAMapType::FREE, f};
+        source.setUnbacked();
+        if (!source.addItem(
+                SHAMapNodeType::TnAccountState, makeShamapitem(key, Slice{key.data(), key.size()})))
+        {
+            return {};
+        }
+
+        auto const path = source.getProofPath(key);
+        if (!path.has_value() || path->empty())
+            return {};
+
+        // NOLINTNEXTLINE(bugprone-unchecked-optional-access) has_value() checked above
+        auto const leaf = SHAMapTreeNode::makeFromWire(makeSlice(path->front()));
+        if (!leaf || !leaf->isLeaf())
+            return {};
+
+        Serializer s;
+        leaf->serializeWithPrefix(s);
+        return {s.getData(), leaf->getHash()};
+    }
+};
+
+// A leaf's key must lie under every branch on the path that reached it, not just the last one.
+class SHAMapMisplacedLeaf : public BuiltRootTest
+{
+protected:
+    // Any branch other than the one kKey selects at depth 0.
+    static constexpr unsigned int kWrongBranch = 5;
+
+    /**
+     * A key whose first nibble equals its last, so the branch it selects past
+     * kLeafDepth is the one the chain's deepest node occupies. That tells a
+     * refusal apart from a child that could not be fetched.
+     */
+    static constexpr UInt256 kChainKeyPastLeafDepth{
+        "b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6cab"};
+
+    /**
+     * Store a chain of single-child inner nodes running one level past
+     * kLeafDepth.
+     *
+     * Built bottom-up, so each parent records the hash of the node below it and
+     * the chain verifies at every level. The deepest node points at a child the
+     * store does not hold.
+     *
+     * @param f the family whose store the chain is written to.
+     * @param key the key whose nibbles choose the branch at every level.
+     * @return the chain's root and the hash of the node at kLeafDepth, or an
+     *         empty pair if a node could not be built.
+     */
+    static std::pair<SHAMapTreeNodePtr, SHAMapHash>
+    storeInnerChainPastLeafDepth(tests::TestNodeFamily& f, UInt256 const& key)
+    {
+        SHAMapHash childHash{UInt256{1}};
+        SHAMapTreeNodePtr node;
+        SHAMapHash deepestHash;
+
+        // The decrement is in the body, so the counter never steps below zero.
+        // UndefinedBehaviorSanitizer reports that wraparound.
+        for (auto depth = SHAMap::kLeafDepth + 1u; depth > 0;)
+        {
+            --depth;
+            auto const id = SHAMapNodeID::createID(std::min(depth, SHAMap::kLeafDepth - 1u), key);
+            node = makeFullInnerNode({{.branch = selectBranch(id, key), .hash = childHash}});
+            if (!node)
+                return {};
+
+            Serializer prefixed;
+            node->serializeWithPrefix(prefixed);
+            f.db().store(
+                NodeObjectType::AccountNode,
+                prefixed.getData(),
+                node->getHash().asUInt256(),
+                kStoredLedgerSeq);
+
+            childHash = node->getHash();
+            if (depth == SHAMap::kLeafDepth)
+                deepestHash = childHash;
+        }
+
+        return {std::move(node), deepestHash};
+    }
+};
+
+// Every branch above a leaf is judged, not only the last one, so a leaf under a subtree recorded on
+// a branch its key does not select is refused.
+TEST_F(SHAMapMisplacedLeaf, iterating_a_misplaced_subtree_throws)
+{
+    // A key greater than the probe below, so a greater key exists for that probe to find.
+    constexpr UInt256 kDeepKey{"a100000000000000000000000000000000000000000000000000000000000000"};
+
+    tests::TestNodeFamily sourceFamily{j_};
+    auto const [leafBlob, leafHash] = leafFor(sourceFamily, kDeepKey);
+    ASSERT_FALSE(leafBlob.empty());
+
+    // The subtree holds exactly one child, on the branch its key selects at depth 1, so the walk
+    // resolves the same node whichever branch the random scan starts from.
+    auto const leafBranch = selectBranch(1u, kDeepKey);
+    auto const inner = makeFullInnerNode({{.branch = leafBranch, .hash = leafHash}});
+    ASSERT_TRUE(inner);
+
+    Serializer innerPrefixed;
+    inner->serializeWithPrefix(innerPrefixed);
+
+    // The leaf is served as well, so the walk reaches it and the throw below is about position.
+    std::vector<std::pair<SHAMapHash, Blob>> served;
+    served.emplace_back(inner->getHash(), innerPrefixed.getData());
+    served.emplace_back(leafHash, leafBlob);
+
+    tests::TestNodeFamily targetFamily{j_};
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setUnbacked();
+    ASSERT_TRUE(buildRoot(map, {{.branch = kWrongBranch, .hash = inner->getHash()}}));
+
+    // The inner node carries no key, so only the leaf below it can be judged against the branches
+    // above.
+    ServingFilter const filter{std::move(served)};
+    map.getMissingNodes(4, &filter);
+
+    EXPECT_THROW(map.begin(), SHAMapMissingNode);
+
+    // The bounds refuse the same map, and a refusal throws where end() answers. The probe selects
+    // kWrongBranch at depth 0 and leafBranch at depth 1, so the descent reaches the misplaced leaf.
+    UInt256 probe;
+    probe.begin()[0] = static_cast<std::uint8_t>((kWrongBranch << 4) | leafBranch);
+    ASSERT_GT(kDeepKey, probe);
+
+    EXPECT_THROW(map.upperBound(probe), SHAMapMissingNode);
+    EXPECT_THROW(map.lowerBound(probe), SHAMapMissingNode);
+}
+
+// peekNextItem scans the branches beyond the one a key takes and judges each node it resolves
+// there. The root holds a well-placed leaf so the scan starts, and a misplaced one for it to reach.
+TEST_F(SHAMapMisplacedLeaf, iterating_past_a_leaf_into_a_misplaced_sibling_throws)
+{
+    // First nibble 10, so this leaf sits where it belongs. kKey's first nibble is 1, so its leaf
+    // does not belong on kStrayBranch, which is above 10 and so within the forward scan's reach.
+    constexpr UInt256 kPlacedKey{
+        "a000000000000000000000000000000000000000000000000000000000000000"};
+    static constexpr unsigned int kStrayBranch = 12;
+
+    tests::TestNodeFamily sourceFamily{j_};
+    auto const [placedBlob, placedHash] = leafFor(sourceFamily, kPlacedKey);
+    ASSERT_FALSE(placedBlob.empty());
+    auto const [strayBlob, strayHash] = leafFor(sourceFamily, kKey);
+    ASSERT_FALSE(strayBlob.empty());
+
+    // Both leaves go in the store rather than a filter, because the scan reaches them through
+    // descendThrow(), which consults no filter.
+    tests::TestNodeFamily targetFamily{j_};
+    targetFamily.db().store(
+        NodeObjectType::AccountNode, Blob{placedBlob}, placedHash.asUInt256(), kStoredLedgerSeq);
+    targetFamily.db().store(
+        NodeObjectType::AccountNode, Blob{strayBlob}, strayHash.asUInt256(), kStoredLedgerSeq);
+
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setLedgerSeq(kStoredLedgerSeq);
+
+    auto const placedBranch = selectBranch(0u, kPlacedKey);
+    ASSERT_LT(placedBranch, kStrayBranch);
+    ASSERT_TRUE(buildRoot(
+        map,
+        {{.branch = placedBranch, .hash = placedHash},
+         {.branch = kStrayBranch, .hash = strayHash}}));
+
+    // The store really holds both, so a refusal below is about position.
+    ASSERT_NE(targetFamily.db().fetchNodeObject(placedHash.asUInt256(), kStoredLedgerSeq), nullptr);
+    ASSERT_NE(targetFamily.db().fetchNodeObject(strayHash.asUInt256(), kStoredLedgerSeq), nullptr);
+
+    // The well-placed leaf resolves, so iteration starts.
+    auto it = map.begin();
+    ASSERT_NE(it, map.end());
+    EXPECT_EQ(it->key(), kPlacedKey);
+
+    // Advancing scans past placedBranch, resolves the stray leaf out of the store, and pushChild
+    // refuses it. The payload check narrows this to peekNextItem's two id-form throws, of which
+    // only the refused push can occur against this fixture.
+    expectThrowNamingKey([&] { ++it; });
+}
+
+// boundHelper runs the same sideways scan as peekNextItem and has its own throw for a node it
+// cannot push. Reached the same way, from a probe whose own branch resolves.
+TEST_F(SHAMapMisplacedLeaf, a_bound_scanning_into_a_misplaced_sibling_throws)
+{
+    constexpr UInt256 kPlacedKey{
+        "a000000000000000000000000000000000000000000000000000000000000000"};
+    static constexpr unsigned int kStrayBranch = 12;
+
+    tests::TestNodeFamily sourceFamily{j_};
+    auto const [placedBlob, placedHash] = leafFor(sourceFamily, kPlacedKey);
+    ASSERT_FALSE(placedBlob.empty());
+    auto const [strayBlob, strayHash] = leafFor(sourceFamily, kKey);
+    ASSERT_FALSE(strayBlob.empty());
+
+    tests::TestNodeFamily targetFamily{j_};
+    targetFamily.db().store(
+        NodeObjectType::AccountNode, Blob{placedBlob}, placedHash.asUInt256(), kStoredLedgerSeq);
+    targetFamily.db().store(
+        NodeObjectType::AccountNode, Blob{strayBlob}, strayHash.asUInt256(), kStoredLedgerSeq);
+
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setLedgerSeq(kStoredLedgerSeq);
+
+    auto const placedBranch = selectBranch(0u, kPlacedKey);
+    ASSERT_LT(placedBranch, kStrayBranch);
+    ASSERT_TRUE(buildRoot(
+        map,
+        {{.branch = placedBranch, .hash = placedHash},
+         {.branch = kStrayBranch, .hash = strayHash}}));
+
+    ASSERT_NE(targetFamily.db().fetchNodeObject(placedHash.asUInt256(), kStoredLedgerSeq), nullptr);
+    ASSERT_NE(targetFamily.db().fetchNodeObject(strayHash.asUInt256(), kStoredLedgerSeq), nullptr);
+
+    // upperBound from the well-placed key descends to its leaf, then scans above it and reaches the
+    // misplaced one, whose own key is greater than the probe. Narrowed to boundHelper's two id-form
+    // throws, of which only the refused push can occur against this fixture.
+    expectThrowNamingKey([&] { static_cast<void>(map.upperBound(kPlacedKey)); });
+}
+
+// walkTowardsKey refuses a node past kLeafDepth in both its modes. The mode that records a path for
+// its caller refuses through pushChild and clears the path, so the caller reads an empty path as a
+// refusal.
+TEST_F(SHAMapMisplacedLeaf, a_keyed_walk_recording_a_path_refuses_a_node_past_leaf_depth)
+{
+    tests::TestNodeFamily targetFamily{j_};
+    auto [root, deepestHash] = storeInnerChainPastLeafDepth(targetFamily, kChainKeyPastLeafDepth);
+    ASSERT_TRUE(root);
+    ASSERT_TRUE(deepestHash.isNonZero());
+
+    // The node the walk has to refuse is really in the store, so the refusal is about depth.
+    ASSERT_NE(
+        targetFamily.db().fetchNodeObject(deepestHash.asUInt256(), kStoredLedgerSeq), nullptr);
+
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setLedgerSeq(kStoredLedgerSeq);
+    auto const rootHash = root->getHash();
+    ASSERT_TRUE(map.addRootNode(rootHash, std::move(root), nullptr).isGood());
+
+    // delItem hands walkTowardsKey a path to fill, reads the cleared path as a refusal, and names
+    // the key it was walking to.
+    expectThrowNamingKey([&] { static_cast<void>(map.delItem(kChainKeyPastLeafDepth)); });
+}
+
+// The other mode: a walk with no path at all tracks a depth of its own and refuses at the same
+// bound, answering rather than throwing.
+TEST_F(SHAMapMisplacedLeaf, a_keyed_walk_with_no_path_refuses_a_node_past_leaf_depth)
+{
+    tests::TestNodeFamily targetFamily{j_};
+    auto [root, deepestHash] = storeInnerChainPastLeafDepth(targetFamily, kChainKeyPastLeafDepth);
+    ASSERT_TRUE(root);
+    ASSERT_TRUE(deepestHash.isNonZero());
+
+    ASSERT_NE(
+        targetFamily.db().fetchNodeObject(deepestHash.asUInt256(), kStoredLedgerSeq), nullptr);
+
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setLedgerSeq(kStoredLedgerSeq);
+    auto const rootHash = root->getHash();
+    ASSERT_TRUE(map.addRootNode(rootHash, std::move(root), nullptr).isGood());
+
+    // hasItem reaches findKey, which walks with no path. The walk reports the refusal and stops, so
+    // the key reads as absent.
+    bool found = true;
+    ASSERT_NO_THROW(found = map.hasItem(kChainKeyPastLeafDepth));
+    EXPECT_FALSE(found);
+}
+
+// A path of inner nodes running past kLeafDepth has no room for the leaf it leads to, so the
+// traversal refuses it. The chain is resolvable, so the throw is about depth.
+TEST_F(SHAMapMisplacedLeaf, a_path_of_inner_nodes_past_leaf_depth_is_refused)
+{
+    // An arbitrary key. Only the branches it selects matter here.
+    constexpr UInt256 kChainKey{"b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"};
+
+    tests::TestNodeFamily targetFamily{j_};
+
+    // Built bottom-up, so each parent records the hash of the node below it and the chain verifies
+    // at every level. The deepest node points at a child the store does not hold.
+    SHAMapHash childHash{UInt256{1}};
+    SHAMapTreeNodePtr node;
+
+    // The node at kLeafDepth, which is the one the traversal has to refuse. Recorded so the refusal
+    // can be told apart from a failure to fetch the sentinel the chain ends on.
+    SHAMapHash deepestHash;
+
+    // The decrement is in the body, so the counter never steps below zero.
+    // UndefinedBehaviorSanitizer reports that wraparound.
+    for (auto depth = SHAMap::kLeafDepth + 1u; depth > 0;)
+    {
+        --depth;
+        auto const id = SHAMapNodeID::createID(std::min(depth, SHAMap::kLeafDepth - 1u), kChainKey);
+        node = makeFullInnerNode({{.branch = selectBranch(id, kChainKey), .hash = childHash}});
+        ASSERT_TRUE(node);
+
+        Serializer prefixed;
+        node->serializeWithPrefix(prefixed);
+        targetFamily.db().store(
+            NodeObjectType::AccountNode,
+            prefixed.getData(),
+            node->getHash().asUInt256(),
+            kStoredLedgerSeq);
+
+        childHash = node->getHash();
+        if (depth == SHAMap::kLeafDepth)
+            deepestHash = childHash;
+    }
+    ASSERT_TRUE(deepestHash.isNonZero());
+
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setLedgerSeq(kStoredLedgerSeq);
+    ASSERT_TRUE(map.addRootNode(childHash, std::move(node), nullptr).isGood());
+
+    // The chain is really readable, so the refusal below is about depth.
+    ASSERT_NE(targetFamily.db().fetchNodeObject(childHash.asUInt256(), kStoredLedgerSeq), nullptr);
+
+    // Refused with SHAMapMissingNode, which is pushChild's other refusal shape: too deep rather
+    // than misplaced. The depth bound applies before getChildNodeID is asked for a child.
+    try
+    {
+        static_cast<void>(map.begin());
+        ADD_FAILURE() << "the traversal did not refuse the path";
+    }
+    catch (SHAMapMissingNode const& e)
+    {
+        // belowHelper's refusal reports the hash of the child it refused, so the node named here is
+        // the one at kLeafDepth.
+        std::string const what{e.what()};
+        EXPECT_NE(what.find(to_string(deepestHash)), std::string::npos) << what;
+    }
+}
+
+// addKnownNode reaches a filter through the synchronous descend on its way to the position it was
+// given. This pins the verdict it reports and that the map ends invalid.
+TEST_F(SHAMapMisplacedLeaf, hooking_a_known_node_invalidates_the_map)
+{
+    tests::TestNodeFamily sourceFamily{j_};
+    auto const [leafBlob, leafHash] = leafFor(sourceFamily, kKey);
+    ASSERT_FALSE(leafBlob.empty());
+
+    tests::TestNodeFamily targetFamily{j_};
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setUnbacked();
+    ASSERT_TRUE(buildRoot(map, {{.branch = kWrongBranch, .hash = 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, so the walk reaches the descent and judges what it resolves there.
+    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();
+
+    ServingFilter const filter{leafHash, leafBlob};
+    auto const result = map.addKnownNode(target, std::move(offered), &filter);
+
+    EXPECT_FALSE(map.isValid());
+
+    // The verdict is what the acquisition paths charge a peer on, so it is pinned alongside the
+    // state.
+    EXPECT_TRUE(result.isInvalid());
+    EXPECT_FALSE(result.isGood());
+}
+
+// The filter descent judges depth as well as position, and this case reaches the depth arm. It
+// serves an inner node, for which belongsAt holds by definition, so the verdict below comes from
+// pastLeafDepth alone.
+//
+// The whole chain is served through the filter and nothing is written to the store, because the
+// memory nodestore is shared by path across every test family in this binary.
+TEST_F(SHAMapMisplacedLeaf, a_filter_serving_an_inner_node_at_leaf_depth_invalidates_the_map)
+{
+    // A key of this case's own, so the chain it builds is its own too.
+    constexpr UInt256 kOwnChainKey{
+        "3f0e1d2c3b4a59687766554433221100ffeeddccbbaa99887766554433221100"};
+
+    // Built bottom-up, so each parent records the hash of the node below it. Every node goes to the
+    // filter, including the one at kLeafDepth, which is the one the descent has to refuse.
+    std::vector<std::pair<SHAMapHash, Blob>> served;
+    SHAMapHash childHash{UInt256{1}};
+    SHAMapTreeNodePtr node;
+
+    // The decrement is in the body, so the counter never steps below zero.
+    // UndefinedBehaviorSanitizer reports that wraparound.
+    for (auto depth = SHAMap::kLeafDepth + 1u; depth > 0;)
+    {
+        --depth;
+        auto const id =
+            SHAMapNodeID::createID(std::min(depth, SHAMap::kLeafDepth - 1u), kOwnChainKey);
+        node = makeFullInnerNode({{.branch = selectBranch(id, kOwnChainKey), .hash = childHash}});
+        ASSERT_TRUE(node);
+        ASSERT_TRUE(node->isInner());
+
+        Serializer prefixed;
+        node->serializeWithPrefix(prefixed);
+        served.emplace_back(node->getHash(), prefixed.getData());
+
+        childHash = node->getHash();
+    }
+    ASSERT_EQ(served.size(), SHAMap::kLeafDepth + 1u);
+
+    tests::TestNodeFamily targetFamily{j_};
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setUnbacked();
+    ASSERT_TRUE(map.addRootNode(childHash, std::move(node), nullptr).isGood());
+    ASSERT_TRUE(map.isValid());
+
+    // kLeafDepth is the deepest ID the tree allows, so the walk descends from kLeafDepth - 1 once
+    // more and consults the filter for a child at kLeafDepth.
+    auto const target = SHAMapNodeID::createID(SHAMap::kLeafDepth, kOwnChainKey);
+
+    Serializer offeredWire;
+    for (auto i = 0u; i < SHAMap::kBranchFactor; ++i)
+        offeredWire.addBitString(i == 0u ? UInt256{1} : UInt256{});
+    offeredWire.add8(kWireTypeInner);
+    auto offered = SHAMapTreeNode::makeFromWire(makeSlice(offeredWire.peekData()));
+    ASSERT_TRUE(offered);
+    offered->updateHash();
+
+    ServingFilter const filter{std::move(served)};
+    static_cast<void>(map.addKnownNode(target, std::move(offered), &filter));
+
+    EXPECT_FALSE(map.isValid());
+}
+
+// addKnownNode also hooks the very node it was handed, when the local store has nothing to resolve
+// for that slot. Its position is known only from the ID the caller supplied, so a leaf is judged
+// against its 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] = leafFor(sourceFamily, kKey);
+    ASSERT_FALSE(leafBlob.empty());
+
+    tests::TestNodeFamily targetFamily{j_};
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setUnbacked();
+    ASSERT_TRUE(buildRoot(map, {{.branch = kWrongBranch, .hash = leafHash}}));
+    ASSERT_TRUE(map.isValid());
+
+    // The branch the root records 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 hooked.
+    auto const result = map.addKnownNode(target, std::move(offered), nullptr);
+
+    EXPECT_FALSE(map.isValid());
+    EXPECT_TRUE(result.isInvalid());
+    EXPECT_FALSE(result.isGood());
+
+    // The verdict names which arm refused, not just that one did. This arm condemned the map, so
+    // it reports mapInvalidated(); the arm that only rejects a misplaced label reports the plain
+    // invalid() and leaves the map usable, which
+    // SHAMapSyncTest.only_the_map_invalidating_arm_reports_it holds the other side of. The two
+    // decide which fee InboundLedger::receiveNode() charges, so the names cannot be swapped.
+    EXPECT_TRUE(result.invalidatedMap());
+}
+
+// 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] = leafFor(sourceFamily, kKey);
+    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(buildRoot(map, {{.branch = kWrongBranch, .hash = leafHash}}));
+    ASSERT_TRUE(map.isValid());
+
+    ServingFilter const filter{leafHash, leafBlob};
+    map.getMissingNodes(1, &filter);
+
+    EXPECT_FALSE(map.isValid());
+}
+
+// The descendAsync walk leaves the leaf hooked, since it resolves the node before the position
+// is judged. Iterating it throws SHAMapMissingNode, which is how belowHelper reports a refusal.
+TEST_F(SHAMapMisplacedLeaf, iterating_a_hooked_misplaced_leaf_throws)
+{
+    tests::TestNodeFamily sourceFamily{j_};
+    auto const [leafBlob, leafHash] = leafFor(sourceFamily, kKey);
+    ASSERT_FALSE(leafBlob.empty());
+
+    tests::TestNodeFamily targetFamily{j_};
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setUnbacked();
+    ASSERT_TRUE(buildRoot(map, {{.branch = kWrongBranch, .hash = leafHash}}));
+
+    ServingFilter const filter{leafHash, leafBlob};
+    map.getMissingNodes(1, &filter);
+    ASSERT_FALSE(map.isValid());
+
+    EXPECT_THROW(map.begin(), SHAMapMissingNode);
+}
+
+// descendAsync is the production path, since a backed map posts asynchronous reads rather than
+// fetching inline. This case uses a backed map with the leaf in the store, so it takes that route.
+TEST_F(SHAMapMisplacedLeaf, an_async_read_resolving_a_misplaced_leaf_invalidates_the_map)
+{
+    tests::TestNodeFamily sourceFamily{j_};
+    auto const [leafBlob, leafHash] = leafFor(sourceFamily, kKey);
+    ASSERT_FALSE(leafBlob.empty());
+
+    // Backed and with the leaf in the store, so asyncFetch resolves it. No filter is passed, so the
+    // walk takes the asynchronous route rather than checkFilter.
+    tests::TestNodeFamily targetFamily{j_};
+    targetFamily.db().store(
+        NodeObjectType::AccountNode, Blob{leafBlob}, leafHash.asUInt256(), kStoredLedgerSeq);
+
+    SHAMap map{SHAMapType::FREE, UInt256{}, targetFamily};
+    map.setLedgerSeq(kStoredLedgerSeq);
+    ASSERT_TRUE(buildRoot(map, {{.branch = kWrongBranch, .hash = leafHash}}));
+
+    map.getMissingNodes(1, nullptr);
+
+    EXPECT_FALSE(map.isValid());
+}
+
+// A childless inner node carries no key, so every position rule holds for it by definition (see
+// belongsAt in SHAMapLeafNode.h). belowHelper finds no leaf below such a node, and both entry
+// points that reach it throw, which the cases below pin.
+class SHAMapChildlessInner : public BuiltRootTest
+{
+protected:
+    // A branch above the one kKey selects, so a scan reaches the leaf first and this second.
+    static constexpr unsigned int kEmptyInnerBranch = 5;
+
+    // The hash the root records for the childless inner node, and the key the store holds it under.
+    // Any non-zero value serves.
+    static constexpr UInt256 kEmptyInnerHash{
+        "00000000000000000000000000000000000000000000000000000000000000ff"};
+
+    /**
+     * The prefixed form of an inner node with no children.
+     *
+     * Assembled by hand, since serializeWithPrefix asserts the node has a
+     * branch.
+     *
+     * @return four prefix bytes followed by sixteen zero child hashes.
+     */
+    static Blob
+    childlessInnerBlob()
+    {
+        Serializer s;
+        s.add32(HashPrefix::InnerNode);
+        for (auto i = 0u; i < SHAMap::kBranchFactor; ++i)
+            s.addBitString(UInt256{});
+        return s.getData();
+    }
+
+    /**
+     * Assemble `map` as a root holding the leaf where it belongs and the
+     * childless inner above it.
+     *
+     * @param map the map to assemble, which must be synching and empty.
+     * @param leafHash the hash of the leaf, recorded under the branch kKey
+     *                 selects.
+     * @return whether the root was accepted.
+     */
+    static bool
+    buildRootWithEmptyInner(SHAMap& map, SHAMapHash const& leafHash)
+    {
+        return buildRoot(
+            map,
+            {
+                {.branch = selectBranch(0u, kKey), .hash = leafHash},
+                {.branch = kEmptyInnerBranch, .hash = SHAMapHash{kEmptyInnerHash}},
+            });
+    }
+
+    /**
+     * Build a map holding both nodes, hooked in through a getMissingNodes walk.
+     *
+     * @param f the family the map belongs to.
+     * @return the map, or nullptr if any step was refused.
+     */
+    std::shared_ptr<SHAMap>
+    buildMap(Family& f) const
+    {
+        tests::TestNodeFamily sourceFamily{j_};
+        auto const [leafBlob, leafHash] = leafFor(sourceFamily, kKey);
+        if (leafBlob.empty())
+            return nullptr;
+
+        // Backed, because the childless inner node reaches the map through the local store rather
+        // than through the filter, so the fixture depends on how finishFetch establishes a node's
+        // hash.
+        auto map = std::make_shared<SHAMap>(SHAMapType::FREE, UInt256{}, f);
+        map->setLedgerSeq(kStoredLedgerSeq);
+        if (!buildRootWithEmptyInner(*map, leafHash))
+            return nullptr;
+
+        f.db().store(
+            NodeObjectType::AccountNode, childlessInnerBlob(), kEmptyInnerHash, kStoredLedgerSeq);
+
+        // The leaf still comes from the filter, since its blob hashes to its own hash.
+        ServingFilter const filter{std::vector<std::pair<SHAMapHash, Blob>>{{leafHash, leafBlob}}};
+
+        // Both nodes are available, the leaf from the filter and the inner one from the store, so
+        // an empty list is what shows every blob resolved. isValid() stays true with an unresolved
+        // child, so this assertion is what settles it.
+        if (!map->getMissingNodes(4, &filter).empty())
+            return nullptr;
+
+        // Every node here sits where it belongs, so the position checks pass.
+        if (!map->isValid())
+            return nullptr;
+        return map;
+    }
+};
+
+// An iterator increment reaches the childless inner node through peekNextItem, which pushes it and
+// then asks belowHelper for a leaf below it. No leaf is found, so the increment throws.
+TEST_F(SHAMapChildlessInner, incrementing_past_a_childless_inner_node_throws)
+{
+    tests::TestNodeFamily f{j_};
+    auto const map = buildMap(f);
+    ASSERT_NE(map, nullptr);
+
+    auto it = map->begin();
+    ASSERT_NE(it, map->end());
+    EXPECT_EQ(it->key(), kKey);
+
+    // peekNextItem's second id-form throw, taken when the push succeeds and belowHelper finds no
+    // leaf below. The payload separates it from a node that was merely absent, which names a hash.
+    expectThrowNamingKey([&] { ++it; });
+}
+
+// upperBound reaches it through its own scan past the branch the probe takes, and throws rather
+// than answering end(), which would claim that no greater key exists.
+TEST_F(SHAMapChildlessInner, bounding_across_a_childless_inner_node_throws)
+{
+    tests::TestNodeFamily f{j_};
+    auto const map = buildMap(f);
+    ASSERT_NE(map, nullptr);
+
+    // Shares kKey's first nibble, so the walk ends on the leaf, but compares greater, so the leaf
+    // does not qualify and the scan moves up to the root and on to kEmptyInnerBranch.
+    UInt256 probe;
+    probe.begin()[0] = static_cast<std::uint8_t>(selectBranch(0u, kKey) << 4 | 0x0fu);
+    ASSERT_GT(probe, kKey);
+
+    // boundHelper's second id-form throw, reached the same way as in the test above.
+    expectThrowNamingKey([&] { static_cast<void>(map->upperBound(probe)); });
+}
+
 }  // namespace xrpl::tests

src/tests/libxrpl/shamap/SHAMapNodeID.cpp

@@ -90,6 +90,33 @@ TEST(SHAMapNodeIDTest, create_id_masks_key_to_depth)
     }
 }
 
+// A child's id is its parent's with the nibble at the parent's own depth set to the branch taken.
+// The reference comes from createID, so it is produced by the depthMask table rather than by the
+// arithmetic under test.
+TEST(SHAMapNodeIDTest, child_node_id_sets_the_nibble_the_next_depth_masks_in)
+{
+    for (auto depth = 0u; depth < SHAMap::kLeafDepth; ++depth)
+    {
+        auto const parent = SHAMapNodeID::createID(depth, kTestKey);
+
+        // The branch kTestKey's own nibble selects at this depth, so the child sits where createID
+        // puts that key one level down.
+        auto const branch = selectBranch(parent, kTestKey);
+        auto const child = SHAMapNodeID::createID(depth + 1, kTestKey);
+        auto const& expected = child.getNodeID();
+
+        EXPECT_EQ(childNodeID(parent.getNodeID(), depth, branch), expected) << "depth " << depth;
+
+        // Only that branch reaches that id, so the whole branch value lands in the nibble and
+        // sixteen branches name sixteen distinct positions.
+        for (auto other = 0u; other < SHAMap::kBranchFactor; ++other)
+        {
+            EXPECT_EQ(childNodeID(parent.getNodeID(), depth, other) == expected, other == branch)
+                << "depth " << depth << " branch " << other;
+        }
+    }
+}
+
 // The guards below must hold with XRPL_ASSERT compiled out (NDEBUG), so each one
 // has to be a real runtime check rather than an assert.
 
@@ -165,6 +192,43 @@ TEST(SHAMapNodeIDDeathTest, select_branch_clamps_leaf_depth)
 #endif
 }
 
+TEST(SHAMapNodeIDDeathTest, same_position_at_depth_holds_its_own_bound)
+{
+    // Differs from kTestKey in the last nibble only, so the two agree at every depth below
+    // kLeafDepth and only kLeafDepth itself tells them apart.
+    constexpr UInt256 kLastNibbleDiffers(
+        "b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca9");
+
+    // Depth 0 names no nibble, so every key shares the root position.
+    EXPECT_TRUE(samePositionAtDepth(0, kTestKey, kLastNibbleDiffers));
+    EXPECT_TRUE(samePositionAtDepth(0, kTestKey, UInt256{}));
+
+    // kLeafDepth is in range here, unlike in selectBranch, since the position at that depth is
+    // the whole key. A bound borrowed from selectBranch would clamp this to 63 and call the two
+    // keys equal, so the pair below is what holds the bound at the right value.
+    EXPECT_TRUE(samePositionAtDepth(SHAMap::kLeafDepth - 1u, kTestKey, kLastNibbleDiffers));
+    EXPECT_FALSE(samePositionAtDepth(SHAMap::kLeafDepth, kTestKey, kLastNibbleDiffers));
+    EXPECT_TRUE(samePositionAtDepth(SHAMap::kLeafDepth, kTestKey, kTestKey));
+
+    // Past kLeafDepth there is no mask in depthMask's 65-entry table, so the call clamps. That
+    // clamp is marked UNREACHABLE, which is an assert and therefore fatal wherever asserts are
+    // live, so only a build with them compiled out (or routed to Antithesis's non-fatal handler)
+    // reaches the clamp itself and can be asserted on.
+#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR)
+    for (auto const depth : {SHAMap::kLeafDepth + 1u, 100u, 255u, 256u, 320u})
+    {
+        // Answers as kLeafDepth does, rather than reading past the table or narrowing the depth
+        // to a byte: 256 would otherwise become 0 and call the two keys equal.
+        EXPECT_FALSE(samePositionAtDepth(depth, kTestKey, kLastNibbleDiffers)) << "depth " << depth;
+        EXPECT_TRUE(samePositionAtDepth(depth, kTestKey, kTestKey)) << "depth " << depth;
+    }
+#else
+    EXPECT_DEATH(
+        (void)samePositionAtDepth(SHAMap::kLeafDepth + 1u, kTestKey, kTestKey),
+        "depth within tree");
+#endif
+}
+
 TEST(SHAMapNodeIDTest, deserialize_rejects_out_of_range_depth)
 {
     // getRawString() only serializes a depth already accepted by the constructor's own

src/tests/libxrpl/shamap/SyncFilter.h

@@ -0,0 +1,95 @@
+#pragma once
+
+#include <xrpl/basics/Blob.h>
+#include <xrpl/basics/SHAMapHash.h>
+#include <xrpl/protocol/Serializer.h>
+#include <xrpl/shamap/SHAMapSyncFilter.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <cstdint>
+#include <map>
+#include <optional>
+#include <utility>
+#include <vector>
+
+namespace xrpl::tests {
+
+/**
+ * A sync filter that serves the nodes it was given, by hash, and records
+ * nothing.
+ *
+ * Serving is opt-in: a node absent from the set looks unavailable. Each node is
+ * held in its prefixed form, which is what SHAMap parses a filter's answer
+ * from.
+ */
+class ServingFilter : public SHAMapSyncFilter
+{
+public:
+    ServingFilter() = default;
+
+    /**
+     * @param hash The hash to serve the node under.
+     * @param blob The node's prefixed form.
+     */
+    ServingFilter(SHAMapHash const& hash, Blob blob)
+    {
+        serve(hash, std::move(blob));
+    }
+
+    /**
+     * @param nodes Each node's hash and prefixed form.
+     */
+    explicit ServingFilter(std::vector<std::pair<SHAMapHash, Blob>> nodes)
+    {
+        for (auto& [hash, blob] : nodes)
+            serve(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;
+    }
+
+    /**
+     * Offer a node back to the map, as a fetch pack does.
+     *
+     * @param node The node to serve, keyed by its own hash.
+     */
+    void
+    serve(SHAMapTreeNodePtr const& node)
+    {
+        Serializer s;
+        node->serializeWithPrefix(s);
+        serve(node->getHash(), s.modData());
+    }
+
+    /**
+     * Offer a node whose prefixed form the caller already holds.
+     *
+     * @param hash The hash to serve the node under.
+     * @param blob The node's prefixed form.
+     */
+    void
+    serve(SHAMapHash const& hash, Blob blob)
+    {
+        nodes_.emplace(hash, std::move(blob));
+    }
+
+private:
+    std::map<SHAMapHash, Blob> nodes_;
+};
+
+}  // namespace xrpl::tests

src/tests/libxrpl/shamap/Tally.h

@@ -0,0 +1,22 @@
+#pragma once
+
+#include <xrpl/shamap/SHAMapAddNode.h>
+
+namespace xrpl::tests {
+
+/**
+ * Whether a batch verdict carries exactly the given counts.
+ *
+ * @param san The verdict to check.
+ * @param good How many nodes the batch should have hooked in.
+ * @param bad How many it should have rejected.
+ * @param duplicate How many it should have already held.
+ * @return Whether the verdict matches.
+ */
+[[nodiscard]] inline bool
+tallyIs(SHAMapAddNode const& san, int good, int bad, int duplicate)
+{
+    return san.getGood() == good && san.getBad() == bad && san.getDuplicate() == duplicate;
+}
+
+}  // namespace xrpl::tests

src/xrpld/app/ledger/ConsensusTransSetSF.cpp

@@ -6,6 +6,7 @@
 #include <xrpl/basics/Blob.h>
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/SHAMapHash.h>
+#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/TaggedCache.ipp>  // IWYU pragma: keep
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/core/Job.h>
@@ -21,6 +22,7 @@
 #include <functional>
 #include <memory>
 #include <optional>
+#include <utility>
 
 namespace xrpl {
 
@@ -72,9 +74,22 @@ ConsensusTransSetSF::gotNode(
 std::optional<Blob>
 ConsensusTransSetSF::getNode(SHAMapHash const& nodeHash) const
 {
+    // Both arms answer for their own data, as getNode()'s postcondition requires. A caller adopts
+    // the hash this was asked for, so the digest is computed here.
     Blob nodeData;
     if (nodeCache_.retrieve(nodeHash, nodeData))
-        return nodeData;
+    {
+        // Only the cache's filing ties the blob to this key, so the digest is taken here.
+        if (sha512Half(makeSlice(nodeData)) == nodeHash.asUInt256())
+            return nodeData;
+
+        // No lock spans the read and the drop. insert() keeps a live entry, so no writer can
+        // replace this one, but a sweep or another drop may remove it and a writer refile the key
+        // before del() runs. That costs one cache miss and nothing else.
+        // The transaction arm below reads a different store, which may hold this key.
+        nodeCache_.del(nodeHash, false);
+        JLOG(j_.warn()) << "Cached node " << nodeHash << " does not hash to its key, dropped it";
+    }
 
     auto const txn = app_.getMasterTransaction().fetchFromCache(nodeHash.asUInt256());
 
@@ -82,14 +97,22 @@ ConsensusTransSetSF::getNode(SHAMapHash const& nodeHash) const
     {
         // this is a transaction, and we have it
         JLOG(j_.trace()) << "Node in our acquiring TX set is TXN we have";
+        auto const& stx = txn->getSTransaction();
+
+        // This serialization has to reproduce the node hash asked for, so the identity is tested
+        // here. A transaction's id is the digest of exactly the bytes serialized below and is held
+        // on the transaction, so the test reads it rather than digesting them again.
+        if (stx->getTransactionID() != nodeHash.asUInt256())
+        {
+            JLOG(j_.warn()) << "Transaction for node " << nodeHash
+                            << " does not serialize to it, discarding it";
+            return std::nullopt;
+        }
+
         Serializer s;
         s.add32(HashPrefix::TransactionId);
-        txn->getSTransaction()->add(s);
-        XRPL_ASSERT(
-            sha512Half(s.slice()) == nodeHash.asUInt256(),
-            "xrpl::ConsensusTransSetSF::getNode : transaction hash match");
-        nodeData = s.peekData();
-        return nodeData;
+        stx->add(s);
+        return std::move(s.modData());
     }
 
     return std::nullopt;

src/xrpld/app/ledger/InboundLedger.h

@@ -75,7 +75,11 @@ public:
     update(std::uint32_t seq);
 
     /**
-     * Returns true if we got all the data.
+     * Whether the acquisition succeeded and its ledger has been settled. Every
+     * path that sets this settles the ledger first, so a caller that sees it
+     * may use the ledger directly.
+     *
+     * @return Whether the ledger is complete and settled.
      */
     bool
     isComplete() const
@@ -84,7 +88,7 @@ public:
     }
 
     /**
-     * Returns false if we failed to get the data.
+     * @return Whether the acquisition has failed.
      */
     bool
     isFailed() const
@@ -161,8 +165,10 @@ protected:
     trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason);
 
     /**
-     * Settle the acquisition and signal whatever is waiting on it. Runs at most
-     * once. Call under mtx_, which the flags written here require.
+     * Settle the acquisition, publish its outcome, and signal whatever is
+     * waiting on it. Runs at most once. Call under mtx_, which the flags
+     * written here require. Settles before publishing, since isComplete() is
+     * read without mtx_.
      */
     void
     done();
@@ -180,6 +186,42 @@ private:
     void
     tryDB(node_store::Database& srcDB);
 
+    /**
+     * Whether either map of the ledger being acquired has been found invalid.
+     *
+     * A walk returns a bare list of hashes, so this is what tells a satisfied
+     * map from an abandoned one. See SHAMap::addKnownNode for why the verdict
+     * is final.
+     *
+     * @return Whether either map is Invalid, and false while there is no
+     *         ledger, since then there is no map to judge.
+     */
+    [[nodiscard]] bool
+    hasInvalidMap() const;
+
+    /**
+     * Whether nothing is left to fetch. Not the same as complete: done()
+     * settles the ledger before publishing that.
+     *
+     * @return Whether the header and both maps have been obtained.
+     */
+    [[nodiscard]] bool
+    haveEverything() const
+    {
+        return haveHeader_ && haveState_ && haveTransactions_;
+    }
+
+    /**
+     * Record what one peer's packet achieved, and report its yield. Only a
+     * useful node counts as progress. Call under mtx_.
+     *
+     * @param san What the packet's nodes achieved, accumulated over the whole
+     *        packet.
+     * @return How many good nodes the packet held.
+     */
+    [[nodiscard]] int
+    recordPacket(SHAMapAddNode const& san);
+
     void
     onTimer(bool progress, ScopedLockType& sl) override;
 

src/xrpld/app/ledger/InboundTransactions.h

@@ -39,7 +39,8 @@ public:
      *
      * @param setHash The transaction set ID (digest of the SHAMap root node).
      * @param acquire Whether to fetch the transaction set from the network if
-     * it is missing.
+     *        it is missing. The retention window is refreshed only while the
+     *        acquisition is still worth keeping.
      * @return The transaction set with ID setHash, or nullptr if it is
      * missing.
      */
@@ -50,7 +51,8 @@ public:
      * Add a transaction set from a LedgerData message.
      *
      * @param setHash The transaction set ID (digest of the SHAMap root node).
-     * @param peer The peer that sent the message.
+     * @param peer The peer that sent the message, charged here for a reply
+     *        outside its allowance.
      * @param message The LedgerData message.
      */
     virtual void
@@ -64,11 +66,13 @@ public:
      *
      * @param setHash The transaction set ID (should match set.getHash()).
      * @param set The transaction set.
-     * @param acquired Whether this transaction set was acquired from a peer,
-     * or constructed by ourself during consensus.
+     * @param fromAcquire Whether the acquisition for this hash supplied the set
+     *        itself. False cancels an acquisition still in flight. True leaves
+     *        it registered until newRound() sweeps it, so a late reply still
+     *        reaches the per-peer allowance wantsReplyFrom() judges.
      */
     virtual void
-    giveSet(UInt256 const& setHash, std::shared_ptr<SHAMap> const& set, bool acquired) = 0;
+    giveSet(UInt256 const& setHash, std::shared_ptr<SHAMap> const& set, bool fromAcquire) = 0;
 
     /**
      * Informs the container if a new consensus round

src/xrpld/app/ledger/OpenLedger.h

@@ -240,7 +240,20 @@ OpenLedger::apply(
         auto iter = retries.begin();
         while (iter != retries.end())
         {
-            switch (applyOne(app, view, iter->second, retry, flags, j))
+            // A transaction that cannot be applied counts as a failure, as it does in the pass
+            // above, so it leaves the set and counts as no change. Every entry gets a verdict on
+            // every pass, so the set shrinks.
+            auto result = Result::Failure;
+            try
+            {
+                result = applyOne(app, view, iter->second, retry, flags, j);
+            }
+            catch (std::exception const& e)
+            {
+                JLOG(j.error()) << "OpenLedger::apply: Caught exception: " << e.what();
+            }
+
+            switch (result)
             {
                 case Result::Success:
                     ++changes;

src/xrpld/app/ledger/detail/InboundLedger.cpp

@@ -113,8 +113,8 @@ InboundLedger::init(ScopedLockType& collectionLock)
     XRPL_ASSERT(
         ledger_->header().seq < kXrpLedgerEarliestFees || FeeSettingsEntryR(*ledger_),
         "xrpl::InboundLedger::init : valid ledger fees");
-    // tryDB() verified both maps before setting complete_ and mtx_ has been held since, so
-    // nothing can have invalidated them.
+    // tryDB() verified both maps before setting complete_ and mtx_ has been held since, so both
+    // maps are still sound here.
     if (!ledger_->setImmutable())
     {
         // LCOV_EXCL_START
@@ -237,6 +237,12 @@ InboundLedger::neededStateHashes(int max, SHAMapSyncFilter const* filter) const
     return neededHashes(ledger_->header().accountHash, ledger_->stateMap(), max, filter);
 }
 
+bool
+InboundLedger::hasInvalidMap() const
+{
+    return ledger_ && !ledger_->mapsValid();
+}
+
 // See how much of the ledger data is stored locally
 // Data found in a fetch pack will be stored
 void
@@ -340,18 +346,30 @@ InboundLedger::tryDB(node_store::Database& srcDB)
         }
     }
 
-    if (haveTransactions_ && haveState_)
+    // Judged here rather than at the setImmutable() below, which runs only once both flags are
+    // set: one map can be abandoned while the other is merely incomplete.
+    if (hasInvalidMap())
+    {
+        JLOG(journal_.warn()) << "Ledger " << hash_ << " found locally has an invalid map";
+        failed_ = true;
+        return;
+    }
+
+    if (haveEverything())
     {
         XRPL_ASSERT(
             ledger_->header().seq < kXrpLedgerEarliestFees || FeeSettingsEntryR(*ledger_),
             "xrpl::InboundLedger::tryDB : valid ledger fees");
-        // Settled before complete_ is published, so a caller that reads the flag never sees a
-        // ledger this function has not finished with.
+        // Settled before complete_ is published, so a caller that reads the flag sees a finished
+        // ledger. Reachable despite the guard above, since trigger() walks the state map with
+        // mtx_ released.
         if (!ledger_->setImmutable())
         {
+            // LCOV_EXCL_START: only the walk named above reaches this, so no test does
             JLOG(journal_.warn()) << "Ledger " << hash_ << " found locally is invalid";
             failed_ = true;
             return;
+            // LCOV_EXCL_STOP
         }
 
         JLOG(journal_.debug()) << "Had everything locally";
@@ -441,28 +459,29 @@ InboundLedger::done()
     signaled_ = true;
     touch();
 
-    // Settled before the outcome below is logged or acted on, so nothing reports a ledger this
-    // function has since refused.
-    if (complete_ && !failed_ && ledger_)
+    // complete_ is published only for a settled, valid ledger, since isComplete() is read without
+    // mtx_. The complete_ arm is for tryDB(), which settles its own result and sets the flag before
+    // calling, so that path arrives with only the reporting below left to do.
+    if (!failed_ && ledger_ && (complete_ || haveEverything()))
     {
         XRPL_ASSERT(
             ledger_->header().seq < kXrpLedgerEarliestFees || FeeSettingsEntryR(*ledger_),
             "xrpl::InboundLedger::done : valid ledger fees");
-        // Recovers rather than asserting: peer data produces this verdict, so a caller cannot know
-        // its map is still sound. Recovering rather than asserting is therefore the contract.
-        // Best-effort even so: setInvalid() outranks Immutable, so a walk that reaches the verdict
-        // after both maps have been settled leaves an immutable ledger with an invalid map. It
-        // narrows the window rather than closing it.
+        // trigger() walks the state map with mtx_ released, so the verdict can land after the
+        // flags said there was nothing left to fetch. A race rather than a broken invariant, so
+        // this recovers rather than asserts. setInvalid() outranks Immutable, so an immutable
+        // ledger can still hold an invalid map.
+        SOMETIMES(hasInvalidMap(), "xrpl::InboundLedger::done : map invalidated by a race");
         if (!ledger_->setImmutable())
         {
             JLOG(journal_.warn()) << "Acquired ledger " << hash_ << " is invalid";
-            // Withdrawn as well as failed, so a caller that already read complete_ - or that checks
-            // it before failed_ - cannot go on treating this ledger as delivered.
+            // Withdrawn as well as failed, for a caller that checks complete_ before failed_.
             complete_ = false;
             failed_ = true;
         }
         else
         {
+            complete_ = true;
             switch (reason_)
             {
                 case Reason::HISTORY:
@@ -485,9 +504,11 @@ InboundLedger::done()
 
     // mtx_ is held, so this may only post the work rather than do it.
     app_.getJobQueue().addJob(JtLedgerData, "AcqDone", [self = shared_from_this()] {
-        if (self->complete_ && !self->failed_)
+        // Read through getLedger(), which consults failed_ itself, so failure and a null ledger_
+        // are both refused by one check. checkAccept() requires a non-null ledger.
+        if (auto const ledger = self->getLedger(); self->complete_ && ledger)
         {
-            self->app_.getLedgerMaster().checkAccept(self->getLedger());
+            self->app_.getLedgerMaster().checkAccept(ledger);
             self->app_.getLedgerMaster().tryAdvance();
         }
         else
@@ -553,7 +574,20 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         {
             auto const need = getNeededHashes();
 
-            if (!need.empty())
+            // The validity check runs ahead of the emptiness test, since getNeededHashes() walks
+            // both maps and can reach the verdict itself. The result is read once, so the hint
+            // below and the test it feeds share one observation of a map another thread can be
+            // invalidating. The claim is withdrawn alongside the failure, so failed_ and
+            // complete_ never both read true.
+            bool const invalidMap = hasInvalidMap();
+            SOMETIMES(invalidMap, "xrpl::InboundLedger::trigger : map is invalid");
+            if (invalidMap)
+            {
+                JLOG(journal_.warn()) << "Acquire " << hash_ << " has an invalid map";
+                failed_ = true;
+                complete_ = false;
+            }
+            else if (!need.empty())
             {
                 protocol::TMGetObjectByHash tmBH;
                 bool typeSet = false;
@@ -590,11 +624,11 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
             }
             else
             {
+                // The tail of this function settles the ledger and reports it complete.
                 JLOG(journal_.info()) << "getNeededHashes says acquire is complete";
                 haveHeader_ = true;
                 haveTransactions_ = true;
                 haveState_ = true;
-                complete_ = true;
             }
         }
     }
@@ -657,27 +691,31 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         {
             AccountStateSF filter(ledger_->stateMap().family().db(), app_.getLedgerMaster());
 
-            // Release the lock while we process the large state map
+            // Release the lock while the state map is walked. mtx_ is recursive, so an sl.unlock()
+            // under onTimer() or the addPeers() callback leaves mtx_ held. The flags are re-read
+            // below because another packet can be handled while this walk runs.
             sl.unlock();
             auto nodes = ledger_->stateMap().getMissingNodes(kMissingNodesFind, &filter);
             sl.lock();
 
+            // The validity check runs outside the flags' guard below, since the verdict is about
+            // the map rather than this round: it holds even if another thread reported the ledger
+            // complete while the lock was released.
+            bool const walkAbandonedMap = hasInvalidMap();
+            SOMETIMES(walkAbandonedMap, "xrpl::InboundLedger::trigger : map abandoned by its walk");
+            if (walkAbandonedMap)
+            {
+                JLOG(journal_.warn()) << "Ledger " << hash_ << " has a map its walk abandoned";
+                failed_ = true;
+                complete_ = false;
+            }
             // Make sure nothing happened while we released the lock
-            if (!failed_ && !complete_ && !haveState_)
+            else if (!failed_ && !complete_ && !haveState_)
             {
                 if (nodes.empty())
                 {
-                    if (!ledger_->stateMap().isValid())
-                    {
-                        failed_ = true;
-                    }
-                    else
-                    {
-                        haveState_ = true;
-
-                        if (haveTransactions_)
-                            complete_ = true;
-                    }
+                    // The test above already caught a map the walk abandoned, so this one is sound.
+                    haveState_ = true;
                 }
                 else
                 {
@@ -739,9 +777,6 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
                 else
                 {
                     haveTransactions_ = true;
-
-                    if (haveState_)
-                        complete_ = true;
                 }
             }
             else
@@ -766,12 +801,12 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         }
     }
 
-    if (complete_ || failed_)
+    // done() settles the ledger before publishing complete_, and requires mtx_, which is still
+    // held here.
+    if (failed_ || haveEverything())
     {
-        JLOG(journal_.debug()) << "Done:" << (complete_ ? " complete" : "")
-                               << (failed_ ? " failed " : " ") << ledger_->header().seq;
-        // Called with mtx_ still held, so the flags done() writes are not written unlocked; mtx_
-        // is recursive, so a caller that already holds it further up is unaffected.
+        JLOG(journal_.debug()) << "Done:" << (failed_ ? " failed " : " have everything ")
+                               << ledger_->header().seq;
         done();
     }
 }
@@ -882,8 +917,17 @@ InboundLedger::takeHeader(std::string_view data)
 }
 
 /**
- * Process node data received from a peer
- * Call with a lock
+ * Judge one peer's map-node reply, hooking in what it carries.
+ *
+ * Call with mtx_ held. A node that cannot be hooked costs its sender the
+ * recoverable tier, and one that proves the map impossible costs the harsher
+ * one. The second verdict is read off the batch rather than off the map, since
+ * a getMissingNodes() walk can invalidate the map while this runs.
+ *
+ * @param peer The sender, charged at whichever site judged its data.
+ * @param packet The reply, which names the map it is for.
+ * @param san The running tally, added to as each node is judged, and reset to a
+ *        single rejection if the map turns out to be abandoned.
  */
 void
 InboundLedger::receiveNode(
@@ -963,7 +1007,32 @@ InboundLedger::receiveNode(
             {
                 JLOG(journal_.warn()) << "Got invalid node " << *nodeID << " for ledger " << hash_
                                       << " from peer " << peer->id();
-                peer->charge(resource::kFeeInvalidData, "ledger_node invalid");
+                // The charge test below reads the verdict rather than the map, because a
+                // getMissingNodes() walk runs with mtx_ released (see trigger()) and can
+                // invalidate the map while this packet is judged, and that verdict is not this
+                // sender's doing.
+                if (result.invalidatedMap())
+                {
+                    // This node commits to a shape no valid tree has. See SHAMap::addKnownNode for
+                    // why that verdict holds for every peer.
+                    peer->charge(resource::kFeeMalformedData, "ledger_node makes map invalid");
+                }
+                else
+                {
+                    peer->charge(resource::kFeeInvalidData, "ledger_node invalid");
+                }
+
+                if (!map.isValid())
+                {
+                    // The map is abandoned, whichever walk reached that verdict, so the
+                    // acquisition fails here.
+                    failed_ = true;
+                    done();
+
+                    // The whole packet is discarded, since the nodes ahead of the bad one belong to
+                    // the same impossible tree. Matches TransactionAcquire::takeNodesLocked().
+                    san = SHAMapAddNode::invalid();
+                }
                 return;
             }
         }
@@ -977,6 +1046,19 @@ InboundLedger::receiveNode(
         return;
     }
 
+    // The verdict belongs to whichever walk reached it rather than to this packet, so the
+    // acquisition fails here and the sender keeps its fee. The validity check runs ahead of
+    // isSynching() below, which reads an invalid map the same way as a finished one.
+    if (!map.isValid())
+    {
+        failed_ = true;
+        done();
+
+        // The whole packet is discarded, since its nodes belong to the abandoned map.
+        san = SHAMapAddNode::invalid();
+        return;
+    }
+
     if (!map.isSynching())
     {
         if (packet.type() == protocol::liTX_NODE)
@@ -988,11 +1070,10 @@ InboundLedger::receiveNode(
             haveState_ = true;
         }
 
-        if (haveTransactions_ && haveState_)
-        {
-            complete_ = true;
+        // done() settles the ledger before publishing complete_, so having every part is reported
+        // there rather than here.
+        if (haveEverything())
             done();
-        }
     }
 }
 
@@ -1202,11 +1283,7 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData co
             return -1;
         }
 
-        if (san.isUseful())
-            progress_ = true;
-
-        stats_ += san;
-        return san.getGood();
+        return recordPacket(san);
     }
 
     if ((packet.type() == protocol::liTX_NODE) || (packet.type() == protocol::liAS_NODE))
@@ -1227,21 +1304,24 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData co
                                << ((packet.type() == protocol::liTX_NODE) ? "TX" : "AS")
                                << " node stats: " << san.get();
 
-        // `san` accumulates across the whole packet, so `isInvalid()` (bad_ > 0) does not mean the
-        // packet had no useful nodes: credit whatever good/useful nodes were sent rather than
-        // discarding everything because one node in an otherwise-good packet was bad.
-        // Note: Peer charges for invalid/malformed data are issued from within receiveNode at the
-        // exact failure site, so the peer is only charged for problems they are responsible for.
-        if (san.isUseful())
-            progress_ = true;
-
-        stats_ += san;
-        return san.getGood();
+        // receiveNode() charges the peer at the site that judged the data, and discards the whole
+        // packet only for a node that leaves the map invalid.
+        return recordPacket(san);
     }
 
     return -1;
 }
 
+int
+InboundLedger::recordPacket(SHAMapAddNode const& san)
+{
+    if (san.isUseful())
+        progress_ = true;
+
+    stats_ += san;
+    return san.getGood();
+}
+
 namespace detail {
 // Track the amount of useful data that each peer returns
 struct PeerDataCounts

src/xrpld/app/ledger/detail/InboundTransactions.cpp

@@ -96,11 +96,10 @@ public:
             {
                 if (acquire)
                 {
-                    it->second.seq = seq_;
-                    if (it->second.acquire)
-                    {
-                        it->second.acquire->stillNeed();
-                    }
+                    // Refreshed only while stillNeed() says there is something to wait for, so an
+                    // acquisition whose map is invalid is left for newRound() to sweep.
+                    if (!it->second.acquire || it->second.acquire->stillNeed())
+                        it->second.seq = seq_;
                 }
                 return it->second.set;
             }
@@ -142,6 +141,12 @@ public:
             return;
         }
 
+        // The test runs before the loop below, which costs a hash per node, since a settled
+        // acquisition discards the result. It charges the peer itself when the reply is
+        // outside its allowance.
+        if (!ta->wantsReplyFrom(peer))
+            return;
+
         std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data;
         data.reserve(packet.nodes().size());
 
@@ -168,15 +173,8 @@ public:
             data.emplace_back(*nodeID, std::move(treeNode));
         }
 
-        auto const san = ta->takeNodes(std::move(data), peer);
-        if (san.isInvalid())
-        {
-            peer->charge(resource::kFeeInvalidData, "ledger_data invalid");
-        }
-        else if (!san.isUseful())
-        {
-            peer->charge(resource::kFeeUselessData, "ledger_data useless");
-        }
+        // takeNodes() charges the peer and records the batch itself, so the verdict is discarded.
+        static_cast<void>(ta->takeNodes(std::move(data), peer));
     }
 
     void
@@ -200,7 +198,16 @@ public:
                 inboundSet.set = set;
             }
 
-            inboundSet.acquire.reset();
+            // Reset only when something other than the acquisition supplied the set, since
+            // dropping the pointer cancels an acquisition still in flight. Keeping it lets a late
+            // reply for this hash still reach takeNodesLocked()'s allowance, and newRound() sweeps
+            // the entry.
+            //
+            // Keyed by the hash alone, not by which acquisition finished. addRootNode() refuses a
+            // root that does not hash to the hash asked for, so every acquisition for a hash ends
+            // with the same map, and installing it is correct whichever entry map_[hash] names.
+            if (!fromAcquire)
+                inboundSet.acquire.reset();
         }
 
         if (isNew)

src/xrpld/app/ledger/detail/TimeoutCounter.h

@@ -8,6 +8,7 @@
 
 #include <boost/asio/basic_waitable_timer.hpp>
 
+#include <atomic>
 #include <chrono>
 #include <cstdint>
 #include <memory>
@@ -50,6 +51,9 @@ namespace xrpl {
  * whether to postpone failure and reset the timeout. However, if it can
  * complete all its work in one synchronous step (while it holds the lock), then
  * it can ignore `progress_`.
+ *
+ * `isDone` is not terminal for every subtype: TransactionAcquire::stillNeed()
+ * clears `failed_` and calls `setTimer` again.
  */
 class TimeoutCounter
 {
@@ -131,10 +135,27 @@ protected:
      */
     UInt256 const hash_;
     int timeouts_{0};
-    bool complete_{false};
-    bool failed_{false};
+
+    // complete_ and failed_ are read without mtx_, so they are atomic rather than guarded: a
+    // reader that sees either flag set also sees the work the writer did before setting it.
+    static_assert(std::atomic<bool>::is_always_lock_free);
+
     /**
-     * Whether forward progress has been made.
+     * Whether the task finished successfully. Each subclass sets it only once
+     * the work it reports on is finished, so a reader may act on it without
+     * taking mtx_.
+     */
+    std::atomic<bool> complete_{false};
+
+    /**
+     * Whether the task gave up.
+     */
+    std::atomic<bool> failed_{false};
+
+    /**
+     * Whether forward progress has been made since invokeOnTimer() last ran.
+     * Each subtype defines what counts as progress, and may read this for
+     * decisions of its own.
      */
     bool progress_{false};
     /**

src/xrpld/app/ledger/detail/TransactionAcquire.cpp

@@ -10,6 +10,7 @@
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/core/Job.h>
+#include <xrpl/resource/Fees.h>
 #include <xrpl/server/NetworkOPs.h>
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapAddNode.h>
@@ -31,6 +32,11 @@ namespace xrpl {
 static constexpr auto kNormTimeouts = 4;
 static constexpr auto kMaxTimeouts = 20;
 
+// How many consecutive timer intervals a duplicate-only reply may postpone. A fan-out race is
+// settled in one or two, so the bound is generous for that case and the timeout count always
+// resumes.
+static constexpr auto kMaxDuplicateCredits = kNormTimeouts;
+
 TransactionAcquire::TransactionAcquire(
     Application& app,
     UInt256 const& hash,
@@ -52,7 +58,6 @@ void
 TransactionAcquire::done()
 {
     // mtx_ is held, so this may only post real work rather than do it.
-
     if (failed_)
     {
         JLOG(journal_.debug()) << "Failed to acquire TX set " << hash_;
@@ -111,6 +116,31 @@ TransactionAcquire::pmDowncast()
     return shared_from_this();
 }
 
+void
+TransactionAcquire::recordAsked(std::shared_ptr<Peer> const& peer)
+{
+    // Each request renews the pass chargeLateReply() reads, so a peer answering the request
+    // just sent to it is free whatever it answered in an earlier round.
+    if (peer)
+    {
+        requestedPeers_.insert(peer->id());
+        lateReplyGranted_.erase(peer->id());
+        return;
+    }
+
+    // A broadcast goes to every peer the set tracks, so each earns a late-reply pass.
+    auto const& ids = peerSet_->getPeerIds();
+    requestedPeers_.insert(ids.begin(), ids.end());
+    for (auto const id : ids)
+        lateReplyGranted_.erase(id);
+}
+
+bool
+TransactionAcquire::hasAsked(std::shared_ptr<Peer> const& peer) const
+{
+    return requestedPeers_.contains(peer->id());
+}
+
 void
 TransactionAcquire::trigger(std::shared_ptr<Peer> const& peer)
 {
@@ -138,6 +168,7 @@ TransactionAcquire::trigger(std::shared_ptr<Peer> const& peer)
             tmGL.set_querytype(protocol::qtINDIRECT);
 
         *tmGL.add_nodeids() = SHAMapNodeID().getRawString();
+        recordAsked(peer);
         peerSet_->sendRequest(tmGL, peer);
     }
     else if (!map_->isValid())
@@ -176,6 +207,7 @@ TransactionAcquire::trigger(std::shared_ptr<Peer> const& peer)
         {
             *tmGL.add_nodeids() = node.first.getRawString();
         }
+        recordAsked(peer);
         peerSet_->sendRequest(tmGL, peer);
     }
 }
@@ -185,24 +217,73 @@ TransactionAcquire::takeNodes(
     std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
     std::shared_ptr<Peer> const& peer)
 {
-    ScopedLockType const sl(mtx_);
+    ScopedLockType sl(mtx_);
 
-    if (complete_)
+    // A settled set accepts no further nodes, and spends this peer's late-reply allowance.
+    if (isDone())
     {
-        JLOG(journal_.trace()) << "TX set complete";
-        return SHAMapAddNode();
+        JLOG(journal_.trace()) << (complete_ ? "TX set complete" : "TX set failed");
+        chargeLateReply(peer, sl);
+
+        // An invalid map's verdict is final, so it outranks the duplicate report.
+        return map_->isValid() ? SHAMapAddNode::duplicate() : SHAMapAddNode::invalid();
     }
 
-    if (failed_)
+    // Read first: the call below can enroll this peer in requestedPeers_.
+    bool const wasAsked = hasAsked(peer);
+
+    auto const san = takeNodesLocked(std::move(data), peer, sl);
+
+    if (san.isUseful())
     {
-        JLOG(journal_.trace()) << "TX set failed";
-        return SHAMapAddNode();
+        // The set advanced, so the duplicate budget is earned back.
+        duplicateCredits_ = 0;
+        progress_ = true;
     }
+    else if (
+        san.getDuplicate() > 0 && san.getBad() == 0 && wasAsked && !progress_ &&
+        duplicateCredits_ < kMaxDuplicateCredits)
+    {
+        // A reply holding duplicates and no bad node, from a peer we asked, counts as an
+        // answer, bounded by kMaxDuplicateCredits and charged at most once per interval.
+        ++duplicateCredits_;
+        progress_ = true;
+    }
+
+    return san;
+}
+
+bool
+TransactionAcquire::wantsReplyFrom(std::shared_ptr<Peer> const& peer)
+{
+    ScopedLockType sl(mtx_);
+
+    if (!isDone())
+        return true;
+
+    JLOG(journal_.trace()) << (complete_ ? "TX set complete" : "TX set failed");
+    chargeLateReply(peer, sl);
+    return false;
+}
+
+SHAMapAddNode
+TransactionAcquire::takeNodesLocked(
+    std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
+    std::shared_ptr<Peer> const& peer,
+    ScopedLockType&)
+{
+    // Accumulated across the batch, so a packet ending in one bad node still counts the nodes
+    // hooked in ahead of it, as InboundLedger::receiveNode() already does.
+    SHAMapAddNode san;
 
     try
     {
         if (data.empty())
+        {
+            // Defensive: PeerImp rejects an empty node list before dispatch.
+            peer->charge(resource::kFeeInvalidData, "tx_set empty");
             return SHAMapAddNode::invalid();
+        }
 
         ConsensusTransSetSF sf(app_, app_.getTempNodeCache());
 
@@ -213,36 +294,70 @@ TransactionAcquire::takeNodes(
                 if (haveRoot_)
                 {
                     JLOG(journal_.debug()) << "Got root TXS node, already have it";
+                    san.incDuplicate();
+                    continue;
                 }
-                else if (!map_->addRootNode(SHAMapHash{hash_}, std::move(d.second), nullptr)
-                              .isGood())
+
+                auto const result =
+                    map_->addRootNode(SHAMapHash{hash_}, std::move(d.second), nullptr);
+                san += result;
+
+                if (!result.isGood())
                 {
                     JLOG(journal_.warn()) << "TX acquire got bad root node for TX set " << hash_
                                           << " from peer " << peer->id();
-                    return SHAMapAddNode::invalid();
-                }
-                else
-                {
-                    haveRoot_ = true;
+                    // addRootNode rejects a hash mismatch and leaves the map valid, so the timer
+                    // owns the retry and picks another peer.
+                    peer->charge(resource::kFeeInvalidData, "tx_set root hash mismatch");
+                    return san;
                 }
+
+                haveRoot_ = true;
+                continue;
             }
-            else if (!map_->addKnownNode(d.first, std::move(d.second), &sf).isGood())
+
+            auto const result = map_->addKnownNode(d.first, std::move(d.second), &sf);
+            san += result;
+
+            if (!result.isGood())
             {
                 JLOG(journal_.warn()) << "TX acquire got bad non-root node " << d.first
                                       << " for TX set " << hash_ << " from peer " << peer->id();
-                return SHAMapAddNode::invalid();
+                if (!map_->isValid())
+                {
+                    // The acquisition fails here, and stillNeed() keeps it failed. See
+                    // SHAMap::addKnownNode for why that verdict holds for every peer. Charged
+                    // kFeeMalformedData under the lock that reached the verdict, so the packet
+                    // that earned the fee is the one that pays it. A deterrent only, since the
+                    // same node reaches a map by paths with no sender to charge.
+                    peer->charge(resource::kFeeMalformedData, "tx_set node makes map invalid");
+                    failed_ = true;
+                    done();
+
+                    // The whole batch is discarded, since the nodes ahead of the bad one belong to
+                    // the same impossible tree, and the acquisition is over.
+                    return SHAMapAddNode::invalid();
+                }
+
+                // Any other bad node leaves the map sound, so the timer owns the retry and picks
+                // another peer.
+                peer->charge(resource::kFeeInvalidData, "tx_set node invalid");
+                return san;
             }
         }
 
         trigger(peer);
-        progress_ = true;
-        return SHAMapAddNode::useful();
+        return san;
     }
     catch (std::exception const& ex)
     {
-        JLOG(journal_.error()) << "Peer " << peer->id()
-                               << " sent us junky transaction node data: " << ex.what();
-        return SHAMapAddNode::invalid();
+        JLOG(journal_.error()) << "TX acquire threw while taking nodes for TX set " << hash_
+                               << " from peer " << peer->id() << ": " << ex.what();
+        // Whatever the batch hooked in before the throw stands, so the tally it reached is what
+        // the caller is told. The timer owns the retry. The sender keeps its fee, since the
+        // classes that reach here are raised by this code rather than by the data.
+        san.incInvalid();
+        return san;
     }
 }
 
@@ -266,12 +381,43 @@ TransactionAcquire::init(int numPeers)
 }
 
 void
+TransactionAcquire::chargeLateReply(std::shared_ptr<Peer> const& peer, ScopedLockType&)
+{
+    if (!hasAsked(peer) || !lateReplyGranted_.insert(peer->id()).second)
+        peer->charge(resource::kFeeUselessData, "tx_set data after the set was settled");
+}
+
+bool
 TransactionAcquire::stillNeed()
 {
-    ScopedLockType const sl(mtx_);
+    ScopedLockType sl(mtx_);
 
     timeouts_ = std::min<int>(timeouts_, kNormTimeouts);
+
+    // A running acquisition keeps the wait it has, rather than restarting it for every consensus
+    // round that asks for the set again.
+    if (!failed_)
+        return true;
+
+    // An invalid map keeps the acquisition failed, since that verdict holds for every peer (see
+    // SHAMap::addKnownNode). Reported so the caller stops refreshing this set's retention window.
+    if (!map_->isValid())
+        return false;
+
     failed_ = false;
+
+    // lateReplyGranted_ is left alone. The free allowance is earned one request at a time, so a
+    // peer keeps the pass it already spent until recordAsked() records another request to it. The
+    // timer restarted below is what sends those requests.
+
+    // The duplicate budget belongs to the round that just failed as well.
+    duplicateCredits_ = 0;
+
+    // Restarting the timer resumes the acquisition. expires_after() cancels whatever wait was
+    // outstanding, so at most one wait is held. A job already handed to the JobQueue still runs one
+    // invokeOnTimer(), which re-arms this same timer and folds back into the one chain.
+    setTimer(sl);
+    return true;
 }
 
 }  // namespace xrpl

src/xrpld/app/ledger/detail/TransactionAcquire.h

@@ -15,6 +15,7 @@
 #include <chrono>
 #include <cstddef>
 #include <memory>
+#include <set>
 #include <utility>
 #include <vector>
 
@@ -48,26 +49,117 @@ public:
         std::chrono::milliseconds retryInterval = kRetryInterval);
     ~TransactionAcquire() override = default;
 
-    SHAMapAddNode
+    /**
+     * Add nodes a peer sent us to the set we are acquiring.
+     *
+     * Takes mtx_, the lock that reaches the verdict, so the charge for
+     * declined data is issued here. A node that leaves the map invalid also
+     * fails the acquisition. A late reply is bounded by the per-peer allowance
+     * in lateReplyGranted_.
+     *
+     * @param data The nodes to add, each with its claimed position.
+     * @param peer The peer that sent them, charged here if the data is
+     *        declined.
+     * @return The tally of useful, duplicate, and bad nodes in the batch.
+     *         Useful and bad can both be nonzero.
+     */
+    [[nodiscard]] SHAMapAddNode
     takeNodes(
         std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
         std::shared_ptr<Peer> const& peer);
 
+    /**
+     * Whether a reply from this peer is worth deserializing.
+     *
+     * Takes mtx_ and spends the peer's late-reply allowance. The answer can go
+     * stale, since mtx_ is released before takeNodes(), which recognizes a late
+     * reply of its own accord.
+     *
+     * @param peer The peer that sent the reply, charged here when the reply is
+     *        outside the allowance.
+     * @return Whether the reply should be parsed and handed to takeNodes().
+     */
+    [[nodiscard]] bool
+    wantsReplyFrom(std::shared_ptr<Peer> const& peer);
+
     void
     init(int startPeers);
 
-    void
+    /**
+     * Resume a timed-out acquisition, or leave it alone.
+     *
+     * Takes mtx_. Clamps the timeout count, then clears the failed flag and
+     * restarts the timer if the map is still valid. An invalid map stays
+     * failed. See SHAMap::addKnownNode for why that verdict is final.
+     *
+     * @return Whether the set is still worth keeping, which is false once its
+     *         map is invalid.
+     */
+    [[nodiscard]] bool
     stillNeed();
 
 protected:
-    // Kept protected so a test subclass can read the map's state.
-    // Production callers reach a set through InboundTransactions.
+    // Protected so a test subclass can read the map's state.
     std::shared_ptr<SHAMap> map_;
 
 private:
     bool haveRoot_{false};
+
+    /**
+     * Every peer a request has actually been sent to, which is what earns a
+     * peer the free late-reply pass. trigger() records a peer where it builds
+     * the request, so selection alone does not enroll one. Survives a
+     * stillNeed() revival. Guarded by mtx_.
+     */
+    std::set<Peer::ID> requestedPeers_;
+
+    /**
+     * Peers in requestedPeers_ that have spent the free late reply their last
+     * request earned. One unspent pass per peer, renewed by each request sent
+     * to it, so overlapping requests to one peer share one pass. recordAsked()
+     * renews a pass wherever it records a request, for a targeted request and
+     * for a broadcast alike, so a revival on its own renews nothing. Guarded
+     * by mtx_.
+     */
+    std::set<Peer::ID> lateReplyGranted_;
+
+    /**
+     * Consecutive timer intervals a duplicate-only reply has postponed, with no
+     * useful node in between. Bounded by kMaxDuplicateCredits, so the timeout
+     * count always resumes. Reset by a batch that advances the set, and by
+     * stillNeed() on revival.
+     */
+    int duplicateCredits_{0};
+
     std::unique_ptr<PeerSet> peerSet_;
 
+    /**
+     * Add nodes a peer sent us, on the lock takeNodes() holds. Reached only for
+     * an acquisition still running.
+     *
+     * @param data The nodes to add, each with its claimed position.
+     * @param peer The peer that sent them, charged here if the data is
+     *        declined.
+     * @param sl Proof mtx_ is held.
+     * @return The tally of useful, duplicate, and bad nodes in the batch.
+     */
+    SHAMapAddNode
+    takeNodesLocked(
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
+        std::shared_ptr<Peer> const& peer,
+        ScopedLockType& sl);
+
+    /**
+     * Spend this peer's one free late reply, or charge it for replaying. Only
+     * one of wantsReplyFrom() and takeNodes() sees any given reply, so a reply
+     * is charged once.
+     *
+     * @param peer The peer that sent the reply.
+     * @param sl Proof mtx_ is held, which the allowance sets require.
+     */
+    void
+    chargeLateReply(std::shared_ptr<Peer> const& peer, ScopedLockType& sl);
+
     void
     onTimer(bool progress, ScopedLockType& sl) override;
 
@@ -82,6 +174,30 @@ private:
     void
     addPeers(std::size_t limit);
 
+    /**
+     * Record every peer a request has just gone to, and renew each one's free
+     * late reply. Call under mtx_.
+     *
+     * The renewal gives each peer one unspent pass, renewed by each request
+     * sent to it, so overlapping requests to one peer share one pass. See
+     * lateReplyGranted_.
+     *
+     * @param peer The peer a targeted request went to, or nullptr for a
+     *        broadcast, which reaches every peer the set tracks.
+     */
+    void
+    recordAsked(std::shared_ptr<Peer> const& peer);
+
+    /**
+     * Whether a request has ever gone to this peer, in any round. Call under
+     * mtx_.
+     *
+     * @param peer The peer to ask about, which must be non-null.
+     * @return Whether a request was sent to this peer.
+     */
+    [[nodiscard]] bool
+    hasAsked(std::shared_ptr<Peer> const& peer) const;
+
     void
     trigger(std::shared_ptr<Peer> const&);
     std::weak_ptr<TimeoutCounter>