fix: Count partial batch progress in a TX-set reply

TransactionAcquire::takeNodes() accumulates one SHAMapAddNode across the batch and hands it back, so a
packet ending on one bad node still counts the nodes hooked in ahead of it, as
InboundLedger::receiveNode() already does. A root the set already holds counts as a duplicate rather
than passing unremarked, which is what tells a batch of nodes we already have from one that was never
examined at all.

The body moves to takeNodesLocked() and takeNodes() becomes a thin wrapper that takes the lock and
records what the batch achieved on the one exit. Several of the inner exits stop the batch early, and
nothing in the compiler catches one that forgets to record progress. Progress turns on the batch being
useful rather than good, so a batch of nothing but duplicates records none: that flag is what keeps the
next timer tick from counting a timeout, and a reply carrying nothing new advanced nothing. It costs
retry budget on the ordinary second responder to a fan-out and nothing else.

InboundTransactions::gotData() asks isGood() rather than isUseful() for the useless-data charge, which
keeps that duplicate free. It is what an honest second responder to trigger()'s fan-out sends, and now
that the verdict distinguishes a duplicate from an empty tally, the useful test would charge for it
while the batch that was never examined still needs charging.

testPartialBatchIsCounted feeds two good nodes followed by one at a position it cannot occupy, and
checks the verdict names both halves and that the flag is set; then a batch of nothing but a root
already held, and checks it is good, not useful, and records nothing; then that root alongside a node
the set does need, and checks the duplicate is reported as one while the other records the progress.
It reads the flag rather than the returned tally, which only stands in for it, and clears it between
batches so each reading is about the batch just fed.

cmake/XrplSanitizers.cmake

@@ -77,19 +77,24 @@ if(is_clang)
     message(STATUS "  Ignorelist: ${ignorelist_path}")
 endif()
 
-# Define SANITIZERS macro for BuildInfo.cpp
+# Define SANITIZERS macro for BuildInfo.cpp, plus one XRPL_<NAME>SAN define per active
+# sanitizer, so other code can test for a specific one with #ifdef instead of parsing the
+# dot-joined SANITIZERS string.
 set(sanitizers_list)
 if(SANITIZERS MATCHES "address")
     set(enable_asan ON)
     list(APPEND sanitizers_list "ASAN")
+    target_compile_definitions(common INTERFACE XRPL_ASAN)
 endif()
 if(SANITIZERS MATCHES "thread")
     set(enable_tsan ON)
     list(APPEND sanitizers_list "TSAN")
+    target_compile_definitions(common INTERFACE XRPL_TSAN)
 endif()
 if(SANITIZERS MATCHES "undefinedbehavior")
     set(enable_ubsan ON)
     list(APPEND sanitizers_list "UBSAN")
+    target_compile_definitions(common INTERFACE XRPL_UBSAN)
 endif()
 
 if(sanitizers_list)

include/xrpl/ledger/Ledger.h

@@ -257,13 +257,43 @@ public:
         header_.validated = true;
     }
 
-    void
+    /**
+     * Mark this ledger as accepted and attempt to make it immutable.
+     *
+     * The close-time fields are recorded before the maps are settled, since the
+     * ledger hash covers them.
+     *
+     * @param closeTime The consensus-agreed close time.
+     * @param closeResolution The close time resolution.
+     * @param correctCloseTime Whether consensus agreed on the close time; if
+     *        false, kSLcfNoConsensusTime is recorded in closeFlags instead.
+     * @return What setImmutable() returned, so false means the ledger must be
+     *         discarded rather than retried.
+     */
+    [[nodiscard]] bool
     setAccepted(
         NetClock::time_point closeTime,
         NetClock::duration closeResolution,
         bool correctCloseTime);
 
-    void
+    /**
+     * Mark this ledger as immutable, so it can no longer be modified.
+     *
+     * A locally built or loaded ledger can never fail this call: only a map
+     * syncing against externally supplied hashes can become Invalid (see
+     * SHAMap::addKnownNode). A ledger assembled from peer data can fail it;
+     * there, false is an expected outcome, not an internal invariant break.
+     *
+     * @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 and only if the header
+     *        did not supply them.
+     * @return false if either map is Invalid, leaving the immutable flag
+     *         unset. A map invalidated partway through can leave the header
+     *         hashes written and one map immutable, so false means the
+     *         ledger must be discarded rather than retried.
+     */
+    [[nodiscard]] bool
     setImmutable(bool rehash = true);
 
     bool
@@ -272,23 +302,38 @@ public:
         return immutable_;
     }
 
-    /*  Mark this ledger as "should be full".
+    /**
+     * Whether neither map has been found invalid.
+     *
+     * Read by whatever assembles the ledger, which cannot tell a map that
+     * is merely incomplete from one that has been abandoned by looking at
+     * what a walk returned. See SHAMap::isValid().
+     *
+     * @return Whether both maps can still be the maps the header names.
+     */
+    [[nodiscard]] bool
+    mapsValid() const
+    {
+        return txMap_.isValid() && stateMap_.isValid();
+    }
 
-        "Full" is metadata property of the ledger, it indicates
-        that the local server wants all the corresponding nodes
-        in durable storage.
-
-        This is marked `const` because it reflects metadata
-        and not data that is in common with other nodes on the
-        network.
-    */
+    /**
+     * Mark this ledger as "should be full", indicating that the local server
+     * wants all the corresponding nodes in durable storage.
+     *
+     * Const because it reflects metadata, not data this ledger shares with
+     * other nodes on the network.
+     */
     void
     setFull() const
     {
-        txMap_.setFull();
+        // Sequence before flag, per map: setLedgerSeq() stores relaxed and setFull() stores
+        // release, so only this order publishes the sequence to SHAMap::finishFetch(), which
+        // reads it after winning the exchange on the flag.
         txMap_.setLedgerSeq(header_.seq);
-        stateMap_.setFull();
+        txMap_.setFull();
         stateMap_.setLedgerSeq(header_.seq);
+        stateMap_.setFull();
     }
 
     void
@@ -418,8 +463,37 @@ private:
     static std::pair<std::shared_ptr<STTx const>, std::shared_ptr<STObject const>>
     deserializeTxPlusMeta(SHAMapItem const& item);
 
+    /**
+     * Make both maps immutable, without short-circuiting.
+     *
+     * A concurrent walk can invalidate one map after the other is settled,
+     * so both calls are always made rather than one guarding the other:
+     * each map becomes Immutable or stays Invalid on its own, and neither
+     * is left mid-sync because the other refused.
+     *
+     * @return Whether both maps are immutable.
+     */
+    [[nodiscard]] bool
+    setMapsImmutable()
+    {
+        bool const txImmutable = txMap_.setImmutable();
+        bool const stateImmutable = stateMap_.setImmutable();
+        return txImmutable && stateImmutable;
+    }
+
     bool immutable_;
 
+    /**
+     * Whether the header's transaction and account hashes came from outside and
+     * so must not be derived from the maps.
+     *
+     * True only for a ledger built from a header, whose maps are then
+     * synced against the hashes it carries. Deriving them would turn a
+     * ledger verified against a hash we asked for into one that is merely
+     * self-consistent. Fixed at construction, unlike immutable_.
+     */
+    bool mapHashesFromHeader_ = false;
+
     // A SHAMap containing the transactions associated with this ledger.
     SHAMap mutable txMap_;
 

include/xrpl/shamap/SHAMap.h

@@ -2,6 +2,7 @@
 
 #include <xrpl/basics/Blob.h>
 #include <xrpl/basics/IntrusivePointer.h>
+#include <xrpl/basics/Log.h>
 #include <xrpl/basics/SHAMapHash.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/utility/Journal.h>
@@ -16,6 +17,7 @@
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
+#include <atomic>
 #include <condition_variable>
 #include <cstddef>
 #include <cstdint>
@@ -40,7 +42,7 @@ class SHAMapSyncFilter;
 /**
  * Describes the current state of a given SHAMap
  */
-enum class SHAMapState {
+enum class SHAMapState : std::uint8_t {
     /**
      * The map is in flux and objects can be added and removed.
      *
@@ -120,16 +122,43 @@ private:
      */
     std::uint32_t cowid_ = 1;
 
+    // ledgerSeq_, state_ and full_ are touched on the nodestore fetch path and on a
+    // getMissingNodes() walk, neither of which may block, so pin them lock-free.
+    static_assert(std::atomic<std::uint32_t>::is_always_lock_free);
+    static_assert(std::atomic<SHAMapState>::is_always_lock_free);
+    static_assert(std::atomic<bool>::is_always_lock_free);
+
     /**
      * 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 only serves as a lookup hint for a nodestore keyed by hash, so
+     * it orders nothing else.
      */
-    std::uint32_t ledgerSeq_ = 0;
+    std::atomic<std::uint32_t> ledgerSeq_ = 0;
 
     SHAMapTreeNodePtr root_;
-    mutable SHAMapState state_;
+
+    /**
+     * The map's state.
+     *
+     * A getMissingNodes() walk writes it, through setInvalid() and
+     * clearSynching(), while whatever drives the acquisition reads it.
+     * Nothing here requires the caller to hold a lock across the walk, and
+     * the acquisition code does not, so this is atomic rather than guarded.
+     */
+    std::atomic<SHAMapState> state_;
     SHAMapType const type_;
-    bool backed_ = true;         // Map is backed by the database
-    mutable bool full_ = false;  // Map is believed complete in database
+    bool backed_ = true;  // Map is backed by the database
+
+    /**
+     * Map is believed complete in database.
+     *
+     * finishFetch() clears it on whichever nodestore reader thread completes a
+     * read - several at once for the reads a getMissingNodes() walk posts.
+     */
+    mutable std::atomic<bool> full_ = false;
 
 public:
     /**
@@ -152,7 +181,14 @@ public:
     SHAMap&
     operator=(SHAMap const&) = delete;
 
-    // Take a snapshot of the given map:
+    /**
+     * Take a snapshot of the given map.
+     *
+     * @param other The map to snapshot. An Invalid source yields an Invalid
+     *        snapshot, since the two share the same node structure.
+     * @param isMutable Whether the snapshot may be modified. Ignored when other
+     *        is Invalid, since that state outranks both alternatives.
+     */
     SHAMap(SHAMap const& other, bool isMutable);
 
     // build new map
@@ -190,8 +226,15 @@ public:
 
     //--------------------------------------------------------------------------
 
-    // Returns a new map that's a snapshot of this one.
-    // Handles copy on write for mutable snapshots.
+    /**
+     * Return a new map that is a snapshot of this one.
+     *
+     * Handles copy on write for mutable snapshots. An invalid map yields an
+     * invalid snapshot, since the two share the same node structure.
+     *
+     * @param isMutable Whether the snapshot may be modified.
+     * @return The snapshot.
+     */
     std::shared_ptr<SHAMap>
     snapShot(bool isMutable) const;
 
@@ -296,9 +339,14 @@ public:
      * concurrency, to discover nodes referenced in the
      * SHAMap but not available locally.
      *
+     * Marks the map Invalid and abandons the traversal on meeting an inner
+     * node at kLeafDepth, a shape no valid tree can have, so callers must
+     * re-check isValid() before reading an empty result as "nothing left
+     * to fetch".
+     *
      * @param maxNodes The maximum number of found nodes to return
      * @param filter The filter to use when retrieving nodes
-     * @param return The nodes known to be missing
+     * @return The nodes known to be missing, or empty if the map is Invalid
      */
     std::vector<std::pair<SHAMapNodeID, uint256>>
     getMissingNodes(int maxNodes, SHAMapSyncFilter const* filter);
@@ -365,6 +413,11 @@ public:
      * @param filter Optional sync filter to track received nodes.
      * @return Status indicating whether the node was useful, duplicate, or invalid.
      *
+     * A node no valid tree could hold makes the map Invalid, which is
+     * terminal: the root hash committed to an impossible shape, so no peer
+     * can satisfy it. An acquisition reaching this verdict must give up
+     * rather than retry; nothing may promote the map back to a valid state.
+     *
      * @note This function expects the treeNode to be a valid, deserialized SHAMapTreeNode. The
      *       caller is responsible for deserialization and basic validation before calling this
      *       function. This also means that the nodeID must be consistent with the node's content.
@@ -375,16 +428,52 @@ public:
         SHAMapTreeNodePtr treeNode,
         SHAMapSyncFilter const* filter);
 
-    // status functions
-    void
+    /**
+     * Mark this map as immutable, so it can no longer be modified.
+     *
+     * @return false if the map is Invalid and was left unchanged, true
+     *         otherwise.
+     */
+    [[nodiscard]] bool
     setImmutable();
-    bool
+
+    /**
+     * Whether the map's hash is fixed while nodes may still be added to it.
+     *
+     * @return Whether the map is being synced against a hash it was given.
+     */
+    [[nodiscard]] bool
     isSynching() const;
+
+    /**
+     * Mark this map as syncing, fixing its hash while still allowing missing
+     * nodes to be added.
+     *
+     * Does nothing if the map is Invalid, which is terminal.
+     */
     void
     setSynching();
+
+    /**
+     * Mark this map as no longer syncing, so it can be modified again.
+     *
+     * Does nothing if the map is Invalid, which is terminal.
+     */
     void
     clearSynching();
-    bool
+
+    /**
+     * Whether the map can still be the map it claims to be.
+     *
+     * Not "complete" and not "self-consistent": a map that is merely missing
+     * nodes is valid, and stays valid until something proves the tree it is
+     * syncing against cannot exist. Only the map itself reaches that
+     * verdict, and only from a node the hashes it was given cannot
+     * accommodate.
+     *
+     * @return Whether the map has not been proven impossible.
+     */
+    [[nodiscard]] bool
     isValid() const;
 
     // caution: otherMap must be accessed only by this function
@@ -424,6 +513,46 @@ private:
     using DeltaRef =
         std::pair<boost::intrusive_ptr<SHAMapItem const>, boost::intrusive_ptr<SHAMapItem const>>;
 
+    /**
+     * The sequence of the ledger this map references, read atomically.
+     *
+     * @return The sequence, or zero if the map references no ledger.
+     */
+    [[nodiscard]] std::uint32_t
+    ledgerSeq() const;
+
+    /**
+     * The current state, read atomically.
+     *
+     * Orders state_ alone. The tree's nodes are still mutated without
+     * ordering guarantees, so this says nothing about whether the rest of
+     * the map is safe to read concurrently.
+     *
+     * @return The state as of the call, which a concurrent walk may
+     *         already have moved past.
+     */
+    [[nodiscard]] SHAMapState
+    state() const;
+
+    /**
+     * 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.
+     */
+    void
+    setInvalid();
+
+    /**
+     * Store a new state unless the map is Invalid, atomically.
+     *
+     * @param desired The state to store.
+     * @return false if the map is Invalid and was left unchanged, true
+     *         otherwise.
+     */
+    bool
+    trySetState(SHAMapState desired);
+
     // tree node cache operations
     SHAMapTreeNodePtr
     cacheLookup(SHAMapHash const& hash) const;
@@ -639,44 +768,100 @@ private:
 inline void
 SHAMap::setFull()
 {
-    full_ = true;
+    full_.store(true, std::memory_order_release);
 }
 
 inline void
 SHAMap::setLedgerSeq(std::uint32_t lseq)
 {
-    ledgerSeq_ = lseq;
+    ledgerSeq_.store(lseq, std::memory_order_relaxed);
 }
 
-inline void
+inline std::uint32_t
+SHAMap::ledgerSeq() const
+{
+    return ledgerSeq_.load(std::memory_order_relaxed);
+}
+
+inline SHAMapState
+SHAMap::state() const
+{
+    return state_.load(std::memory_order_acquire);
+}
+
+inline bool
+SHAMap::trySetState(SHAMapState desired)
+{
+    // Compare-exchange rather than check-then-store, so the refusal to leave Invalid holds no
+    // matter how this call interleaves with another thread's. Invalid is the only state this
+    // refuses to leave; the loop simply retries if another one is stored meanwhile. No load ahead
+    // of it, since a failed exchange both reports the state and refreshes expected.
+    auto expected = SHAMapState::Modifying;
+    while (expected != SHAMapState::Invalid)
+    {
+        if (state_.compare_exchange_weak(
+                expected, desired, std::memory_order_acq_rel, std::memory_order_acquire))
+        {
+            return true;
+        }
+    }
+    return false;
+}
+
+inline bool
 SHAMap::setImmutable()
 {
-    XRPL_ASSERT(state_ != SHAMapState::Invalid, "xrpl::SHAMap::setImmutable : state is valid");
-    state_ = SHAMapState::Immutable;
+    SOMETIMES(!isValid(), "xrpl::SHAMap::setImmutable : map is invalid");
+    return trySetState(SHAMapState::Immutable);
 }
 
 inline bool
 SHAMap::isSynching() const
 {
-    return state_ == SHAMapState::Synching;
+    return state() == SHAMapState::Synching;
 }
 
 inline void
 SHAMap::setSynching()
 {
-    state_ = SHAMapState::Synching;
+    // Guarded, so this is not a way out of Invalid, matching clearSynching().
+    if (!trySetState(SHAMapState::Synching))
+    {
+        // Unreachable today, though not because the map is Modifying: a ledger built from a header
+        // starts out with both maps Synching already. It is unreachable because this is only ever
+        // called on a map that has just been constructed, so nothing can have synced against it and
+        // reached a verdict on it yet.
+        // LCOV_EXCL_START
+        UNREACHABLE("xrpl::SHAMap::setSynching : map is invalid");
+        // LCOV_EXCL_STOP
+    }
 }
 
 inline void
 SHAMap::clearSynching()
 {
-    state_ = SHAMapState::Modifying;
+    // Guarded, so an invalid map stays invalid rather than being moved back to Modifying, which
+    // passes isValid(). Refusing is the contract rather than a broken invariant, so this reports
+    // instead of asserting: peer data produces the verdict, so an UNREACHABLE here would be an
+    // abort a peer could ask for.
+    SOMETIMES(!isValid(), "xrpl::SHAMap::clearSynching : map is invalid");
+    if (!trySetState(SHAMapState::Modifying))
+    {
+        JLOG(journal_.warn()) << "Refused to clear synching on an invalid map, root hash "
+                              << root_->getHash();
+    }
 }
 
 inline bool
 SHAMap::isValid() const
 {
-    return state_ != SHAMapState::Invalid;
+    return state() != SHAMapState::Invalid;
+}
+
+inline void
+SHAMap::setInvalid()
+{
+    state_.store(SHAMapState::Invalid, std::memory_order_release);
 }
 
 inline void

include/xrpl/shamap/SHAMapAddNode.h

@@ -14,35 +14,140 @@ private:
 
 public:
     SHAMapAddNode();
+
+    /**
+     * Record one node that was rejected.
+     *
+     * Counted rather than merely flagged, so a batch that carries on past a
+     * rejected node reports one per node instead of just that it happened.
+     */
     void
     incInvalid();
+
+    /**
+     * Record one node that was hooked into the map.
+     *
+     * Counted so a batch's tally can be read back through getGood().
+     */
     void
     incUseful();
+
+    /**
+     * Record one node the map already held.
+     *
+     * Counted separately from a useful node: it is not new data, but it is
+     * also not a rejection - see isGood().
+     */
     void
     incDuplicate();
-    void
-    reset();
+
+    /**
+     * How many nodes were hooked into the map, which isUseful() only reports
+     * the presence of.
+     *
+     * @return The count.
+     */
     [[nodiscard]] int
     getGood() const;
+    /**
+     * How many nodes were rejected, which isInvalid() only reports the presence
+     * of. A batch that stops on its first bad node counts one; one that carries
+     * on counts each.
+     *
+     * @return The count.
+     */
+    [[nodiscard]] int
+    getBad() const;
+
+    /**
+     * How many nodes the batch already held, which no other accessor reports: a
+     * duplicate counts as neither good nor bad.
+     *
+     * @return The count.
+     */
+    [[nodiscard]] int
+    getDuplicate() const;
+
+    /**
+     * Whether the batch overall was worth the exchange: nodes accepted or
+     * already held outnumber the ones rejected.
+     *
+     * A duplicate counts on the accepted side, since the peer answered a
+     * request rather than sent something unasked for; see incDuplicate().
+     *
+     * @return Whether the batch was good.
+     */
     [[nodiscard]] bool
     isGood() const;
+
+    /**
+     * Whether any node in the batch was rejected.
+     *
+     * @return Whether at least one node was bad.
+     */
     [[nodiscard]] bool
     isInvalid() const;
+
+    /**
+     * Whether any node in the batch was hooked into the map.
+     *
+     * @return Whether at least one node was useful.
+     */
     [[nodiscard]] bool
     isUseful() const;
+
+    /**
+     * A verdict recording one duplicate node.
+     *
+     * @return The verdict.
+     */
+    static SHAMapAddNode
+    duplicate();
+
+    /**
+     * A verdict recording one useful node.
+     *
+     * @return The verdict.
+     */
+    static SHAMapAddNode
+    useful();
+
+    /**
+     * A verdict recording one invalid node.
+     *
+     * @return The verdict.
+     */
+    static SHAMapAddNode
+    invalid();
+
+    /**
+     * Clear every count back to zero.
+     */
+    void
+    reset();
+
+    /**
+     * Render the tally as a log line.
+     *
+     * A format rather than an API: a caller that needs the counts themselves
+     * should read them through getGood(), getBad() and getDuplicate() instead
+     * of parsing this.
+     *
+     * @return The tally, e.g. "good:2 bad:1 dupe:1", or "no nodes processed" if
+     *         every count is zero.
+     */
     [[nodiscard]] std::string
     get() const;
 
+    /**
+     * Add another verdict's counts into this one.
+     *
+     * @param n The verdict to add.
+     * @return This verdict, updated.
+     */
     SHAMapAddNode&
     operator+=(SHAMapAddNode const& n);
 
-    static SHAMapAddNode
-    duplicate();
-    static SHAMapAddNode
-    useful();
-    static SHAMapAddNode
-    invalid();
-
 private:
     SHAMapAddNode(int good, int bad, int duplicate);
 };
@@ -74,18 +179,30 @@ SHAMapAddNode::incDuplicate()
     ++duplicate_;
 }
 
-inline void
-SHAMapAddNode::reset()
-{
-    good_ = bad_ = duplicate_ = 0;
-}
-
 inline int
 SHAMapAddNode::getGood() const
 {
     return good_;
 }
 
+inline int
+SHAMapAddNode::getBad() const
+{
+    return bad_;
+}
+
+inline int
+SHAMapAddNode::getDuplicate() const
+{
+    return duplicate_;
+}
+
+inline bool
+SHAMapAddNode::isGood() const
+{
+    return (good_ + duplicate_) > bad_;
+}
+
 inline bool
 SHAMapAddNode::isInvalid() const
 {
@@ -98,22 +215,6 @@ SHAMapAddNode::isUseful() const
     return good_ > 0;
 }
 
-inline SHAMapAddNode&
-SHAMapAddNode::operator+=(SHAMapAddNode const& n)
-{
-    good_ += n.good_;
-    bad_ += n.bad_;
-    duplicate_ += n.duplicate_;
-
-    return *this;
-}
-
-inline bool
-SHAMapAddNode::isGood() const
-{
-    return (good_ + duplicate_) > bad_;
-}
-
 inline SHAMapAddNode
 SHAMapAddNode::duplicate()
 {
@@ -132,6 +233,12 @@ SHAMapAddNode::invalid()
     return SHAMapAddNode(0, 1, 0);
 }
 
+inline void
+SHAMapAddNode::reset()
+{
+    good_ = bad_ = duplicate_ = 0;
+}
+
 inline std::string
 SHAMapAddNode::get() const
 {
@@ -160,4 +267,14 @@ SHAMapAddNode::get() const
     return ret;
 }
 
+inline SHAMapAddNode&
+SHAMapAddNode::operator+=(SHAMapAddNode const& n)
+{
+    good_ += n.good_;
+    bad_ += n.bad_;
+    duplicate_ += n.duplicate_;
+
+    return *this;
+}
+
 }  // namespace xrpl

include/xrpl/shamap/SHAMapInnerNode.h

@@ -31,7 +31,25 @@ private:
      */
     TaggedPointer hashesAndChildren_;
 
-    std::uint32_t fullBelowGen_ = 0;
+    // Inner nodes are allocated in the millions into a deliberately packed layout, so pin that
+    // wrapping fullBelowGen_ leaves every member at the offset it had, and that isFullBelow() does
+    // not take a mutex once per node of every walk.
+    static_assert(std::atomic<std::uint32_t>::is_always_lock_free);
+    static_assert(sizeof(std::atomic<std::uint32_t>) == sizeof(std::uint32_t));
+    static_assert(alignof(std::atomic<std::uint32_t>) == alignof(std::uint32_t));
+
+    /**
+     * Written from more than one thread: canonicalization shares nodes between
+     * maps, so concurrent walks of different maps reach the same node, and a
+     * single map's walk can run with the acquisition lock released (see
+     * SHAMap::state_).
+     *
+     * Relaxed both ways: a generation is only ever compared for equality
+     * and publishes nothing, since the children it vouches for are
+     * published through lock_. Ordering it would cost a barrier per inner
+     * node of every walk.
+     */
+    std::atomic<std::uint32_t> fullBelowGen_ = 0;
     std::uint16_t isBranch_ = 0;
 
     /**
@@ -204,13 +222,13 @@ SHAMapInnerNode::getBranchCount() const
 inline bool
 SHAMapInnerNode::isFullBelow(std::uint32_t generation) const
 {
-    return fullBelowGen_ == generation;
+    return fullBelowGen_.load(std::memory_order_relaxed) == generation;
 }
 
 inline void
 SHAMapInnerNode::setFullBelowGen(std::uint32_t gen)
 {
-    fullBelowGen_ = gen;
+    fullBelowGen_.store(gen, std::memory_order_relaxed);
 }
 
 }  // namespace xrpl

src/libxrpl/ledger/Ledger.cpp

@@ -202,7 +202,14 @@ Ledger::Ledger(
     }
 
     stateMap_.flushDirty(NodeObjectType::AccountNode);
-    setImmutable();
+    // Built locally; see Ledger::setImmutable(). Failed deterministically rather than handing back
+    // a mutable ledger that would abort later at a site that cannot explain why; logicError() logs.
+    if (!setImmutable())
+    {
+        // LCOV_EXCL_START
+        logicError("Ledger::Ledger(CreateGenesisT, ...): genesis ledger map is invalid");
+        // LCOV_EXCL_STOP
+    }
 }
 
 Ledger::Ledger(
@@ -213,7 +220,7 @@ Ledger::Ledger(
     Fees const& fees,
     Family& family,
     beast::Journal j)
-    : immutable_(true)
+    : immutable_(false)
     , txMap_(SHAMapType::TRANSACTION, info.txHash, family)
     , stateMap_(SHAMapType::STATE, info.accountHash, family)
     , fees_(fees)
@@ -236,8 +243,20 @@ Ledger::Ledger(
         JLOG(j.warn()) << "Don't have state data root for ledger" << header_.seq;
     }
 
-    txMap_.setImmutable();
-    stateMap_.setImmutable();
+    // Loaded locally; see Ledger::setImmutable().
+    if (setMapsImmutable())
+    {
+        immutable_ = true;
+    }
+    else
+    {
+        // LCOV_EXCL_START
+        JLOG(j.error()) << "Invalid map for ledger " << header_.seq;
+        UNREACHABLE("xrpl::Ledger::Ledger(LedgerHeader const&, ...) : map is invalid");
+        // Treat it as a damaged ledger: the code below recomputes the hash and re-acquires.
+        loaded = false;
+        // LCOV_EXCL_STOP
+    }
 
     if (!setup())
         loaded = false;
@@ -278,8 +297,12 @@ Ledger::Ledger(Ledger const& prevLedger, NetClock::time_point closeTime)
     }
 }
 
+// The maps start out Synching and are filled in afterwards, by an acquisition syncing against the
+// hashes the header carries or by a replay that only reads the header. So those hashes are input
+// rather than derived, and immutable_ stays false until setImmutable() finds both maps sound.
 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))
@@ -308,12 +331,20 @@ Ledger::Ledger(
     setup();
 }
 
-void
+bool
 Ledger::setImmutable(bool rehash)
 {
-    // Force update, since this is the only
-    // place the hash transitions to valid
-    if (!immutable_ && 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 relabelled.
+    if (!mapsValid())
+        return false;
+
+    // Force update, since this is the only place the hash transitions to valid. Skipped once the
+    // ledger is immutable, since its maps can no longer change, and skipped when the header
+    // supplied these hashes: deriving them from a map that fell short of its target would relabel
+    // the ledger instead of failing.
+    if (!immutable_ && !mapHashesFromHeader_ && rehash)
     {
         header_.txHash = txMap_.getHash().asUInt256();
         header_.accountHash = stateMap_.getHash().asUInt256();
@@ -322,13 +353,22 @@ Ledger::setImmutable(bool rehash)
     if (rehash)
         header_.hash = calculateLedgerHash(header_);
 
+    // Both were valid at the check above, but a concurrent walk can invalidate one in between (see
+    // SHAMap::state_), so the result is checked rather than assumed. Best-effort by nature: the
+    // guard narrows the window, it does not close it, since setInvalid() outranks Immutable and can
+    // land after both maps have been settled.
+    bool const bothImmutable = setMapsImmutable();
+    SOMETIMES(!bothImmutable, "xrpl::Ledger::setImmutable : map invalidated while going immutable");
+    if (!bothImmutable)
+        return false;
+
+    // Set last, so isImmutable() never reports a ledger whose maps are not both immutable.
     immutable_ = true;
-    txMap_.setImmutable();
-    stateMap_.setImmutable();
     setup();
+    return true;
 }
 
-void
+bool
 Ledger::setAccepted(
     NetClock::time_point closeTime,
     NetClock::duration closeResolution,
@@ -340,7 +380,17 @@ Ledger::setAccepted(
     header_.closeTime = closeTime;
     header_.closeTimeResolution = closeResolution;
     header_.closeFlags = correctCloseTime ? 0 : kSLcfNoConsensusTime;
-    setImmutable();
+    // Built locally; see Ledger::setImmutable().
+    if (!setImmutable())
+    {
+        // LCOV_EXCL_START
+        JLOG(j_.error()) << "Invalid map for accepted ledger " << header_.seq;
+        UNREACHABLE("xrpl::Ledger::setAccepted : map is invalid");
+        return false;
+        // LCOV_EXCL_STOP
+    }
+
+    return true;
 }
 
 bool

src/libxrpl/shamap/SHAMap.cpp

@@ -26,6 +26,7 @@
 
 #include <boost/smart_ptr/intrusive_ptr.hpp>
 
+#include <atomic>
 #include <cstdint>
 #include <exception>
 #include <functional>
@@ -77,14 +78,25 @@ SHAMap::SHAMap(SHAMap const& other, bool isMutable)
     : f_(other.f_)
     , journal_(other.f_.journal())
     , cowid_(other.cowid_ + 1)
-    , ledgerSeq_(other.ledgerSeq_)
+    , ledgerSeq_(other.ledgerSeq())
     , root_(other.root_)
-    , state_(isMutable ? SHAMapState::Modifying : SHAMapState::Immutable)
     , type_(other.type_)
     , backed_(other.backed_)
 {
+    // A snapshot shares the source's root, so Invalid carries over rather than being promoted to
+    // Modifying or Immutable, either of which would pass isValid(). Carried rather than refused,
+    // since a constructor cannot refuse. Read once into a local, or a concurrent walk could have
+    // the source report one state here and another below.
+    auto const otherState = other.state();
+    auto const ownState = [&] {
+        if (otherState == SHAMapState::Invalid)
+            return SHAMapState::Invalid;
+        return isMutable ? SHAMapState::Modifying : SHAMapState::Immutable;
+    }();
+    state_.store(ownState, std::memory_order_release);
+
     // If either map may change, they cannot share nodes
-    if ((state_ != SHAMapState::Immutable) || (other.state_ != SHAMapState::Immutable))
+    if ((ownState != SHAMapState::Immutable) || (otherState != SHAMapState::Immutable))
     {
         unshare();
     }
@@ -105,7 +117,7 @@ SHAMap::dirtyUp(SharedPtrNodeStack& stack, uint256 const& target, SHAMapTreeNode
     // child can be an inner node or a leaf
 
     XRPL_ASSERT(
-        (state_ != SHAMapState::Synching) && (state_ != SHAMapState::Immutable),
+        (state() != SHAMapState::Synching) && (state() != SHAMapState::Immutable),
         "xrpl::SHAMap::dirtyUp : valid state");
     XRPL_ASSERT(child && (child->cowid() == cowid_), "xrpl::SHAMap::dirtyUp : valid child input");
 
@@ -165,7 +177,7 @@ SHAMapTreeNodePtr
 SHAMap::fetchNodeFromDB(SHAMapHash const& hash) const
 {
     XRPL_ASSERT(backed_, "xrpl::SHAMap::fetchNodeFromDB : is backed");
-    auto obj = f_.db().fetchNodeObject(hash.asUInt256(), ledgerSeq_);
+    auto obj = f_.db().fetchNodeObject(hash.asUInt256(), ledgerSeq());
     return finishFetch(hash, obj);
 }
 
@@ -178,10 +190,15 @@ SHAMap::finishFetch(SHAMapHash const& hash, std::shared_ptr<NodeObject> const& o
     {
         if (!object)
         {
-            if (full_)
+            // A missing node disproves full_, so withdraw it and report the gap. The exchange
+            // rather than a test-then-clear pair is what leaves only one of the reader threads
+            // that miss reporting; the relaxed load ahead of it keeps a map that is already not
+            // full off the exclusive-write path, since full_ shares a cache line with state_ and
+            // ledgerSeq_ and a walk posts up to 512 reads per pass.
+            if (full_.load(std::memory_order_relaxed) &&
+                full_.exchange(false, std::memory_order_acq_rel))
             {
-                full_ = false;
-                f_.missingNodeAcquireBySeq(ledgerSeq_, hash.asUInt256());
+                f_.missingNodeAcquireBySeq(ledgerSeq(), hash.asUInt256());
             }
             return {};
         }
@@ -214,7 +231,7 @@ SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter const* filter) cons
             auto node = SHAMapTreeNode::makeFromPrefix(makeSlice(*nodeData), hash);
             if (node)
             {
-                filter->gotNode(true, hash, ledgerSeq_, std::move(*nodeData), node->getType());
+                filter->gotNode(true, hash, ledgerSeq(), std::move(*nodeData), node->getType());
                 if (backed_)
                     canonicalize(hash, node);
             }
@@ -394,7 +411,7 @@ SHAMap::descendAsync(
         {
             f_.db().asyncFetch(
                 hash.asUInt256(),
-                ledgerSeq_,
+                ledgerSeq(),
                 [this, hash, cb{std::move(callback)}](std::shared_ptr<NodeObject> const& object) {
                     auto node = finishFetch(hash, object);
                     cb(node, hash);
@@ -419,7 +436,7 @@ SHAMap::unshareNode(intr_ptr::SharedPtr<Node> node, SHAMapNodeID const& nodeID)
     if (node->cowid() != cowid_)
     {
         // have a CoW
-        XRPL_ASSERT(state_ != SHAMapState::Immutable, "xrpl::SHAMap::unshareNode : not immutable");
+        XRPL_ASSERT(state() != SHAMapState::Immutable, "xrpl::SHAMap::unshareNode : not immutable");
         node = intr_ptr::staticPointerCast<Node>(node->clone(cowid_));
         if (nodeID.isRoot())
             root_ = node;
@@ -673,7 +690,7 @@ bool
 SHAMap::delItem(uint256 const& id)
 {
     // delete the item with this ID
-    XRPL_ASSERT(state_ != SHAMapState::Immutable, "xrpl::SHAMap::delItem : not immutable");
+    XRPL_ASSERT(state() != SHAMapState::Immutable, "xrpl::SHAMap::delItem : not immutable");
 
     SharedPtrNodeStack stack;
     walkTowardsKey(id, &stack);
@@ -755,7 +772,7 @@ SHAMap::delItem(uint256 const& id)
 bool
 SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const> item)
 {
-    XRPL_ASSERT(state_ != SHAMapState::Immutable, "xrpl::SHAMap::addGiveItem : not immutable");
+    XRPL_ASSERT(state() != SHAMapState::Immutable, "xrpl::SHAMap::addGiveItem : not immutable");
     XRPL_ASSERT(type != SHAMapNodeType::TnInner, "xrpl::SHAMap::addGiveItem : valid type input");
 
     // add the specified item, does not update
@@ -846,7 +863,7 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem cons
     // can't change the tag but can change the hash
     uint256 const tag = item->key();
 
-    XRPL_ASSERT(state_ != SHAMapState::Immutable, "xrpl::SHAMap::updateGiveItem : not immutable");
+    XRPL_ASSERT(state() != SHAMapState::Immutable, "xrpl::SHAMap::updateGiveItem : not immutable");
 
     SharedPtrNodeStack stack;
     walkTowardsKey(tag, &stack);
@@ -937,7 +954,7 @@ SHAMap::writeNode(NodeObjectType t, SHAMapTreeNodePtr node) const
 
     Serializer s;
     node->serializeWithPrefix(s);
-    f_.db().store(t, std::move(s.modData()), node->getHash().asUInt256(), ledgerSeq_);
+    f_.db().store(t, std::move(s.modData()), node->getHash().asUInt256(), ledgerSeq());
     return node;
 }
 

src/libxrpl/shamap/SHAMapInnerNode.cpp

@@ -16,6 +16,7 @@
 #include <xrpl/shamap/detail/TaggedPointer.h>
 #include <xrpl/shamap/detail/TaggedPointer.ipp>
 
+#include <atomic>
 #include <cstddef>
 #include <cstdint>
 #include <mutex>
@@ -77,7 +78,10 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const
     auto p = intr_ptr::makeShared<SHAMapInnerNode>(cowid, branchCount);
     p->hash_ = hash_;
     p->isBranch_ = isBranch_;
-    p->fullBelowGen_ = fullBelowGen_;
+    // Relaxed, as everywhere: the generation is only ever compared for equality, and p is not
+    // reachable by another thread until this returns.
+    p->fullBelowGen_.store(
+        fullBelowGen_.load(std::memory_order_relaxed), std::memory_order_relaxed);
     SHAMapHash* cloneHashes = nullptr;
     SHAMapHash* thisHashes = nullptr;
     SHAMapTreeNodePtr* cloneChildren = nullptr;

src/libxrpl/shamap/SHAMapSync.cpp

@@ -31,6 +31,26 @@
 
 namespace xrpl {
 
+namespace {
+
+/**
+ * Whether a depth is one only a leaf may occupy.
+ *
+ * Nibbles run out at SHAMap::kLeafDepth, so an inner node there would need two
+ * keys agreeing in all 64 nibbles. Spelled once, since the sync path tests it
+ * for a node, for a node's child, and for a position a descent reached.
+ *
+ * @param depth The depth to judge.
+ * @return Whether an inner node at that depth would make the map impossible.
+ */
+[[nodiscard]] bool
+isLeafDepth(unsigned int depth)
+{
+    return depth >= SHAMap::kLeafDepth;
+}
+
+}  // namespace
+
 void
 SHAMap::visitLeaves(
     std::function<void(boost::intrusive_ptr<SHAMapItem const> const& item)> const& leafFunction)
@@ -193,7 +213,13 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
             // we already know this child node is missing
             fullBelow = false;
         }
-        else if (!backed_ || !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
+        // 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, so a hit would skip the depth guard below. Skipping the
+        // shortcut only forgoes an optimization.
+        else if (
+            !backed_ || isLeafDepth(nodeID.getDepth() + 1) ||
+            !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
         {
             bool pending = false;
             auto d = descendAsync(
@@ -224,6 +250,18 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
                 if (--mn.max <= 0)
                     return;
             }
+            else if (d->isInner() && isLeafDepth(nodeID.getDepth() + 1))
+            {
+                // Only a leaf belongs that deep (see isLeafDepth and SHAMap::addKnownNode). A node
+                // resolved locally never passes through addKnownNode(), so the walk has to reach
+                // this verdict itself. Ordered ahead of the full-below test below, which
+                // canonicalization shares across maps, or a node already marked full below would go
+                // unjudged.
+                JLOG(journal_.warn()) << "Inner node at branch " << branch << " below " << nodeID
+                                      << " makes the map invalid";
+                setInvalid();
+                return;
+            }
             else if (d->isInner() && !safeDowncast<SHAMapInnerNode*>(d)->isFullBelow(mn.generation))
             {
                 mn.stack.push(se);
@@ -297,17 +335,23 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn)
     mn.deferred = 0;
 }
 
-/**
- * Get a list of node IDs and hashes for nodes that are part of this SHAMap
- * but not available locally.  The filter can hold alternate sources of
- * nodes that are not permanently stored locally
- */
 std::vector<std::pair<SHAMapNodeID, uint256>>
 SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
 {
     XRPL_ASSERT(root_->getHash().isNonZero(), "xrpl::SHAMap::getMissingNodes : nonzero root hash");
     XRPL_ASSERT(max > 0, "xrpl::SHAMap::getMissingNodes : valid max input");
 
+    // An already-invalid map short-circuits here instead of re-deriving the verdict in the walk
+    // below, which reaches it on its own.
+    if (!isValid())
+    {
+        // journal_ is the family journal, shared by every map, so name which one this is. The root
+        // node's own hash rather than SHAMap::getHash(), which unshares the tree on a zero hash.
+        JLOG(journal_.warn()) << "getMissingNodes called on an invalid map, root hash "
+                              << root_->getHash() << " seq " << ledgerSeq();
+        return {};
+    }
+
     MissingNodes mn(
         max,
         filter,
@@ -340,6 +384,11 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
         {
             gmnProcessNodes(mn, pos);
 
+            // The walk just invalidated the map. Stop descending, but fall through to the drain
+            // below rather than returning, since posted reads hold a reference to mn.
+            if (!isValid())
+                break;
+
             if (mn.max <= 0)
                 break;
 
@@ -369,6 +418,11 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
         if (mn.deferred != 0)
             gmnProcessDeferredReads(mn);
 
+        // Reads are drained, so the map can now be abandoned. Whatever was collected belongs to
+        // a tree that cannot exist, so discard it.
+        if (!isValid())
+            return {};
+
         if (mn.max <= 0)
             return std::move(mn.missingNodes);
 
@@ -402,6 +456,12 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
 
     } while (node != nullptr);
 
+    // Tested once more, since an addKnownNode() on another thread can write the verdict after the
+    // loop's own test above, and an empty result would then be read as "satisfied". clearSynching()
+    // refuses either way, so this is about not asking rather than about the state it would leave.
+    if (!isValid())
+        return {};
+
     if (mn.missingNodes.empty())
         clearSynching();
 
@@ -541,7 +601,7 @@ SHAMap::addRootNode(
         Serializer s;
         root_->serializeWithPrefix(s);
         filter->gotNode(
-            false, root_->getHash(), ledgerSeq_, std::move(s.modData()), root_->getType());
+            false, root_->getHash(), ledgerSeq(), std::move(s.modData()), root_->getType());
     }
 
     return SHAMapAddNode::useful();
@@ -584,7 +644,15 @@ SHAMap::addKnownNode(
         }
 
         auto childHash = inner->getChildHash(branch);
-        if (f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
+
+        // The depth test precedes the cache lookup deliberately: the cache is keyed by node hash
+        // and shared across every map of this family, and a hash covers a node's children but not
+        // its depth, so the same subtree hash can be cached as complete at one depth and reached at
+        // another. Taking the shortcut first would make the verdict below depend on what an
+        // unrelated map cached, and the acquisition paths rely on it being deterministic. Skipping
+        // the shortcut only forgoes an optimization.
+        if (!isLeafDepth(currNodeID.getDepth() + 1) &&
+            f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
         {
             return SHAMapAddNode::duplicate();
         }
@@ -602,25 +670,37 @@ SHAMap::addKnownNode(
             return SHAMapAddNode::invalid();
         }
 
-        // Inner nodes must be at a level strictly less than 64
-        // but leaf nodes (while notionally at level 64) can be
-        // at any depth up to and including 64:
-        if ((currNodeID.getDepth() > kLeafDepth) ||
-            (treeNode->isInner() && currNodeID.getDepth() == kLeafDepth))
+        // Only leaves may sit at kLeafDepth (see isLeafDepth), so an inner node there makes the map
+        // impossible. Nothing is hooked in, so this is bad data rather than progress.
+        //
+        // Every node from the root down hash-verified to get here, so it is the requested root hash
+        // itself that commits to a shape no valid tree can have. The verdict therefore belongs to
+        // that hash rather than to our copy of the tree: no peer can satisfy it, retrying is
+        // futile, and it cannot arise by accident. The acquisition paths rely on all three.
+        //
+        // A charge is a deterrent rather than a control: the same node can reach a map through a
+        // fetch pack or an unsolicited object reply, neither of which comes through here, so
+        // nothing may assume the sender of such a node was made to pay for it.
+        bool const badDepth = treeNode->isInner() && isLeafDepth(currNodeID.getDepth());
+        SOMETIMES(badDepth, "xrpl::SHAMap::addKnownNode : map is invalid");
+        if (badDepth)
         {
-            // Map is provably invalid
-            state_ = SHAMapState::Invalid;
-            return SHAMapAddNode::useful();
+            JLOG(journal_.warn()) << "Node " << nodeID << " makes the map invalid at "
+                                  << currNodeID;
+            setInvalid();
+            return SHAMapAddNode::invalid();
         }
 
-        if (currNodeID != nodeID)
+        // The data hashes to the child we need at currNodeID but is labeled as belonging at nodeID,
+        // so it cannot be hooked anywhere. Only the label is wrong, so the map stays sound and the
+        // node is still obtainable from another sender.
+        bool const badPosition = (currNodeID != nodeID);
+        SOMETIMES(badPosition, "xrpl::SHAMap::addKnownNode : node ID does not match its position");
+        if (badPosition)
         {
-            // Either this node is broken or we didn't request it (yet)
-            JLOG(journal_.warn()) << "unable to hook node " << nodeID;
-            JLOG(journal_.info()) << " stuck at " << currNodeID;
-            JLOG(journal_.info()) << "got depth=" << nodeID.getDepth()
-                                  << ", walked to= " << currNodeID.getDepth();
-            return SHAMapAddNode::useful();
+            JLOG(journal_.warn()) << "Unable to hook node " << nodeID << ", stuck at "
+                                  << currNodeID;
+            return SHAMapAddNode::invalid();
         }
 
         if (backed_)
@@ -633,7 +713,7 @@ SHAMap::addKnownNode(
             Serializer s;
             treeNode->serializeWithPrefix(s);
             filter->gotNode(
-                false, childHash, ledgerSeq_, std::move(s.modData()), treeNode->getType());
+                false, childHash, ledgerSeq(), std::move(s.modData()), treeNode->getType());
         }
 
         return SHAMapAddNode::useful();
@@ -754,7 +834,7 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const
         // node claiming kLeafDepth, and getChildNodeID below throws in that case: reject rather
         // than let the throw escape uncaught. Not reachable through any public entry point,
         // since addKnownNode already marks such a map invalid, so no test can cover this.
-        if (nodeID.getDepth() >= kLeafDepth)
+        if (isLeafDepth(nodeID.getDepth()))
         {
             // LCOV_EXCL_START
             UNREACHABLE("xrpl::SHAMap::hasLeafNode : inner node at leaf depth");

src/test/app/AcquireTestHelpers.h

@@ -0,0 +1,465 @@
+#pragma once
+
+#include <xrpld/app/ledger/ConsensusTransSetSF.h>
+#include <xrpld/overlay/Message.h>
+#include <xrpld/overlay/Peer.h>
+#include <xrpld/overlay/PeerSet.h>
+
+#include <xrpl/basics/SHAMapHash.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/net/IPEndpoint.h>
+#include <xrpl/json/json_value.h>
+#include <xrpl/protocol/KeyType.h>
+#include <xrpl/protocol/PublicKey.h>
+#include <xrpl/protocol/SecretKey.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 <xrpl.pb.h>
+
+#include <atomic>
+#include <chrono>
+#include <cstddef>
+#include <cstdint>
+#include <functional>
+#include <memory>
+#include <mutex>
+#include <optional>
+#include <set>
+#include <string>
+#include <thread>
+#include <utility>
+#include <vector>
+
+namespace xrpl::test {
+
+// The chain builder needs only libxrpl, so it is shared with the gtest suites; see the header for
+// why the protobuf reply builder below cannot be.
+using tests::DeepChain;
+
+// 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 is never taken for a transaction and
+// resubmitted.
+static_assert(
+    sizeof(std::uint32_t) + DeepChain::kLeafItemBytes < ConsensusTransSetSF::kMinTxNodeBytesToParse,
+    "a smallest-possible leaf must stay below the resubmission floor");
+
+/**
+ * A peer that records what it was charged, and is otherwise inert.
+ *
+ * One instance per packet keeps charges() unambiguous about which packet was
+ * charged what.
+ *
+ * charges_ is unguarded: nothing on the retry timer's path charges a peer, so
+ * every charge lands on the thread that fed the packet in.
+ */
+class ChargeRecordingPeer : public Peer
+{
+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.
+     */
+    explicit ChargeRecordingPeer(bool hasTxSet = true) : id_(nextId()), hasTxSet_(hasTxSet)
+    {
+    }
+
+    void
+    charge(resource::Charge const& fee, std::string const& context = {}) override
+    {
+        charges_.push_back(fee);
+    }
+
+    [[nodiscard]] std::vector<resource::Charge> const&
+    charges() const
+    {
+        return charges_;
+    }
+
+    [[nodiscard]] id_t
+    id() const override
+    {
+        return id_;
+    }
+
+    [[nodiscard]] bool
+    hasTxSet(uint256 const&) const override
+    {
+        return hasTxSet_;
+    }
+
+    // Nothing below is consulted by the paths under test.
+
+    void
+    send(std::shared_ptr<Message> const&) override
+    {
+    }
+    [[nodiscard]] beast::ip::Endpoint
+    getRemoteAddress() const override
+    {
+        return {};
+    }
+    [[nodiscard]] bool
+    cluster() const override
+    {
+        return false;
+    }
+    [[nodiscard]] bool
+    isHighLatency() const override
+    {
+        return false;
+    }
+    [[nodiscard]] int
+    getScore(bool) const override
+    {
+        return 0;
+    }
+    [[nodiscard]] PublicKey const&
+    getNodePublic() const override
+    {
+        // Shared across instances: nothing tells these peers apart by key, and deriving one
+        // per instance runs a real Ed25519 keygen for every peer a case builds.
+        static PublicKey const kNodePublicKey =
+            derivePublicKey(KeyType::Ed25519, randomSecretKey());
+        return kNodePublicKey;
+    }
+    json::Value
+    json() override
+    {
+        return {};
+    }
+    [[nodiscard]] bool
+    supportsFeature(ProtocolFeature) const override
+    {
+        return false;
+    }
+    [[nodiscard]] std::optional<std::size_t>
+    publisherListSequence(PublicKey const&) const override
+    {
+        return {};
+    }
+    void
+    setPublisherListSequence(PublicKey const&, std::size_t const) override
+    {
+    }
+    [[nodiscard]] uint256
+    getClosedLedgerHash() const override
+    {
+        static uint256 const kHash{};
+        return kHash;
+    }
+    [[nodiscard]] bool
+    hasLedger(uint256 const&, std::uint32_t) const override
+    {
+        return true;
+    }
+    void
+    ledgerRange(std::uint32_t&, std::uint32_t&) const override
+    {
+    }
+    void
+    cycleStatus() override
+    {
+    }
+    bool
+    hasRange(std::uint32_t, std::uint32_t) override
+    {
+        return false;
+    }
+    [[nodiscard]] bool
+    compressionEnabled() const override
+    {
+        return false;
+    }
+    void
+    sendTxQueue() override
+    {
+    }
+    void
+    addTxQueue(uint256 const&) override
+    {
+    }
+    void
+    removeTxQueue(uint256 const&) override
+    {
+    }
+    [[nodiscard]] bool
+    txReduceRelayEnabled() const override
+    {
+        return false;
+    }
+    [[nodiscard]] std::string const&
+    fingerprint() const override
+    {
+        static std::string const kFingerprint;
+        return kFingerprint;
+    }
+
+private:
+    /**
+     * The next id to hand out, distinct per instance so a test with
+     * several peers can tell from a recorded id which one an acquisition
+     * picked.
+     *
+     * @return The id.
+     */
+    [[nodiscard]] static id_t
+    nextId()
+    {
+        static std::atomic<id_t> next{1};
+        return next++;
+    }
+
+    std::vector<resource::Charge> charges_;
+    id_t id_;
+    bool hasTxSet_;
+};
+
+/**
+ * A peer set that counts the requests sent through it, which is what shows
+ * whether an acquisition is still asking for nodes, and that offers peers to a
+ * hasItem/onPeerAdded callback pair like the real one, though it hard-filters by
+ * hasItem and never dedups a peer already selected (unlike the real peer set, which
+ * only scores by hasItem and dedups by tracked id).
+ *
+ * The retry timer drives addPeers() and sendRequest() from a job thread while
+ * the test reads the results, so everything recorded here is guarded.
+ */
+class RequestCountingPeerSet : public PeerSet
+{
+public:
+    /**
+     * @param candidates The peers addPeers() may offer, in the order they are
+     *        considered. Fixed at construction, so nothing can change them
+     *        while an acquisition is running. Empty for a case that never lets
+     *        addPeers() find anyone.
+     */
+    explicit RequestCountingPeerSet(std::vector<std::shared_ptr<Peer>> candidates = {})
+        : candidates_(std::move(candidates))
+    {
+    }
+
+    /**
+     * Offer the candidates to the caller, the way the real peer set offers the
+     * peers the overlay is tracking.
+     *
+     * @param limit The most peers to add, recorded so a test can check what was
+     *        asked for.
+     * @param hasItem Hard-filters the candidates worth asking. Selects rather than merely
+     *        scores, unlike the real peer set's use of the same parameter.
+     * @param onPeerAdded Called for each selected candidate.
+     */
+    void
+    addPeers(
+        std::size_t limit,
+        std::function<bool(std::shared_ptr<Peer> const&)> hasItem,
+        std::function<void(std::shared_ptr<Peer> const&)> onPeerAdded) override
+    {
+        std::vector<std::shared_ptr<Peer>> selected;
+        {
+            std::scoped_lock const lock(mutex_);
+
+            if (!firstLimit_)
+                firstLimit_ = limit;
+
+            for (auto const& candidate : candidates_)
+            {
+                if (selected.size() >= limit)
+                    break;
+                if (hasItem(candidate))
+                {
+                    addedPeers_.insert(candidate->id());
+                    selected.push_back(candidate);
+                }
+            }
+        }
+
+        // Outside the lock: onPeerAdded() calls back into the acquisition, which sends a
+        // request straight back through this object.
+        for (auto const& peer : selected)
+            onPeerAdded(peer);
+    }
+
+    void
+    sendRequest(
+        ::google::protobuf::Message const&,
+        protocol::MessageType,
+        std::shared_ptr<Peer> const& peer) override
+    {
+        std::scoped_lock const lock(mutex_);
+        ++requests_;
+    }
+
+    /**
+     * The ids of every peer addPeers() has selected, which is what an
+     * acquisition takes for the peers it is tracking.
+     *
+     * Unguarded, like the real peer set's: every caller of this and of
+     * addPeers() is an acquisition holding its own mtx_, so nothing can be
+     * added while a caller iterates. The by-value accessor below is what the
+     * test thread reads instead.
+     *
+     * A caveat for a case that wants a peer count rather than a set of ids:
+     * InboundLedger::getPeerCount() resolves each id through
+     * Overlay::findPeerByShortID(), which only knows peers that really
+     * connected, so it still reports zero however many ids are returned here.
+     *
+     * @return The ids.
+     */
+    [[nodiscard]] std::set<Peer::id_t> const&
+    getPeerIds() const override
+    {
+        return addedPeers_;
+    }
+
+    [[nodiscard]] int
+    requests() const
+    {
+        std::scoped_lock const lock(mutex_);
+        return requests_;
+    }
+
+    /**
+     * The limit the first addPeers() call asked for.
+     *
+     * The first rather than the last, because onTimer() keeps calling
+     * addPeers(1) for as long as an acquisition runs, which would overwrite
+     * what init() asked for.
+     */
+    [[nodiscard]] std::optional<std::size_t>
+    firstLimit() const
+    {
+        std::scoped_lock const lock(mutex_);
+        return firstLimit_;
+    }
+
+    /**
+     * The ids of every peer addPeers() selected, which is a set because
+     * onTimer() keeps re-offering the same candidates.
+     *
+     * @return The ids of every peer addPeers() has selected so far.
+     */
+    [[nodiscard]] std::set<Peer::id_t>
+    addedPeers() const
+    {
+        std::scoped_lock const lock(mutex_);
+        return addedPeers_;
+    }
+
+private:
+    std::vector<std::shared_ptr<Peer>> const candidates_;
+
+    mutable std::mutex mutex_;
+    int requests_{0};
+    std::optional<std::size_t> firstLimit_;
+    std::set<Peer::id_t> addedPeers_;
+};
+
+/**
+ * The given nodes of a chain as a TMLedgerData, so a test can go through the
+ * real dispatch rather than calling an acquisition directly.
+ *
+ * Not part of DeepChain itself: that header is shared with the gtest suites,
+ * whose binary has neither the protobuf types nor anything to send them to.
+ *
+ * @param chain The chain the nodes came from, which names the reply by default.
+ * @param data The nodes to include, each with its claimed position.
+ * @param type The reply type, which selects which map the receiver applies it
+ *        to.
+ * @param ledgerHash The hash the reply claims to be about, defaulting to the
+ *        chain root for a TX set. A ledger acquisition wants its header hash
+ *        here instead, since the chain root is only that ledger's account hash.
+ * @param ledgerSeq The sequence to name in the reply.
+ * @return The reply packet.
+ */
+[[nodiscard]] inline std::shared_ptr<protocol::TMLedgerData>
+packetFor(
+    DeepChain const& chain,
+    std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> const& data,
+    protocol::TMLedgerInfoType type = protocol::liTS_CANDIDATE,
+    std::optional<uint256> const& ledgerHash = std::nullopt,
+    std::uint32_t ledgerSeq = 0)
+{
+    auto packet = std::make_shared<protocol::TMLedgerData>();
+    auto const hash = ledgerHash.value_or(chain.rootHash.asUInt256());
+    packet->set_ledgerhash(hash.data(), uint256::size());
+    packet->set_ledgerseq(ledgerSeq);
+    packet->set_type(type);
+
+    for (auto const& [nodeID, node] : data)
+    {
+        Serializer s;
+        node->serializeForWire(s);
+
+        auto* const ledgerNode = packet->add_nodes();
+        ledgerNode->set_nodedata(s.peekData().data(), s.peekData().size());
+
+        // A leaf carries its own key, so the receiver rebuilds its position from
+        // that plus a depth; an inner node has no key and needs the full ID. The two
+        // fields are a oneof, so sending the wrong one is rejected outright.
+        if (node->isLeaf())
+        {
+            ledgerNode->set_depth(nodeID.getDepth());
+        }
+        else
+        {
+            ledgerNode->set_id(nodeID.getRawString());
+        }
+    }
+
+    return packet;
+}
+
+/**
+ * Poll until the condition holds, or give up.
+ *
+ * An acquisition's own timer and the jobs it hands finished work to both
+ * run on other threads, so a case cannot simply look once. The deadline
+ * is generous, so a loaded machine does not turn a pass into a failure.
+ *
+ * @param condition What to wait for.
+ * @param deadline The longest to wait.
+ * @return Whether the condition held before the deadline.
+ */
+[[nodiscard]] inline bool
+waitFor(
+    std::function<bool()> const& condition,
+    std::chrono::steady_clock::duration deadline = std::chrono::seconds{10})
+{
+    auto const giveUp = std::chrono::steady_clock::now() + deadline;
+    while (std::chrono::steady_clock::now() < giveUp)
+    {
+        if (condition())
+            return true;
+        std::this_thread::sleep_for(std::chrono::milliseconds{10});
+    }
+    return condition();
+}
+
+/**
+ * Whether a batch verdict carries exactly the given counts.
+ *
+ * The counts rather than get(): that string is a log format, not an API. It is
+ * pinned once, in the SHAMapAddNode tests, and is what to pass BEAST_EXPECTS()
+ * as the reason a check here failed. Same name and meaning as the gtest suites'
+ * tallyIs(), which returns an AssertionResult instead.
+ *
+ * @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/InboundLedger_test.cpp

@@ -0,0 +1,738 @@
+#include <test/app/AcquireTestHelpers.h>
+#include <test/jtx/Env.h>
+
+#include <xrpld/app/ledger/InboundLedger.h>
+#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>
+#include <xrpl/ledger/Ledger.h>
+#include <xrpl/nodestore/NodeObject.h>
+#include <xrpl/protocol/HashPrefix.h>
+#include <xrpl/protocol/LedgerHeader.h>
+#include <xrpl/protocol/Serializer.h>
+#include <xrpl/protocol/jss.h>
+
+#include <chrono>
+#include <memory>
+#include <mutex>
+#include <set>
+#include <utility>
+#include <vector>
+
+namespace xrpl::test {
+
+/**
+ * An acquisition that exposes the entry points its bases keep protected, so a
+ * case can reach them without the daemon's API growing.
+ */
+struct TestableInboundLedger final : InboundLedger
+{
+    using InboundLedger::InboundLedger;
+
+    /**
+     * Look for the ledger locally, ask the peers being tracked for the
+     * rest, and arm the timer, as InboundLedgers::acquire() does.
+     *
+     * That caller holds its collection lock across init(), which releases
+     * it, so this stands in with a lock of its own. Declared before the
+     * lock, so it outlives it.
+     */
+    void
+    startAcquire()
+    {
+        std::recursive_mutex collectionMutex;
+        ScopedLockType collectionLock(collectionMutex);
+        init(collectionLock);
+    }
+
+    /**
+     * Ask for more nodes, or judge what has been collected, as a fresh
+     * acquisition does.
+     */
+    void
+    triggerAdded()
+    {
+        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;
+    }
+
+    /**
+     * Record that nothing is left to fetch.
+     */
+    void
+    markComplete()
+    {
+        ScopedLockType const sl(mtx_);
+        complete_ = true;
+    }
+
+    /**
+     * Settle the acquisition and signal whatever is waiting on it.
+     */
+    void
+    signalDone()
+    {
+        ScopedLockType const sl(mtx_);
+        done();
+    }
+};
+
+/**
+ * The ledger an acquisition is assembling, as a pointer that can modify it.
+ *
+ * @param acquire The acquisition to read from.
+ * @return The ledger, or nullptr if there is none to report.
+ */
+[[nodiscard]] static std::shared_ptr<Ledger>
+mutableLedger(InboundLedger const& acquire)
+{
+    // Sound because the acquisition holds a non-const ledger and only hands out a const view.
+    return std::const_pointer_cast<Ledger>(acquire.getLedger());
+}
+
+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 no other chain in this suite has used.
+     *
+     * The Env below is shared, and its node store, fetch packs and remembered
+     * failures are all keyed by hash, so two cases building identically seeded
+     * chains would let one resolve or judge the other's. Handing out a fresh
+     * seed per chain makes that impossible rather than merely unlikely.
+     *
+     * @return The seed.
+     */
+    [[nodiscard]] unsigned int
+    nextSeed()
+    {
+        return ++seed_;
+    }
+
+    /**
+     * A ledger header naming the given map roots.
+     *
+     * The hash is derived from the fields, so an acquisition accepts the
+     * header as its own however the roots are chosen.
+     *
+     * @param txHash The transaction map root; zero means no transactions.
+     * @param accountHash The state map root; zero is a ledger no
+     *        acquisition can finish.
+     * @return The header, with its hash filled in.
+     */
+    static LedgerHeader
+    makeHeader(uint256 const& txHash, uint256 const& accountHash)
+    {
+        LedgerHeader header;
+        header.seq = 2;
+        header.parentCloseTime = NetClock::time_point{};
+        header.closeTime = NetClock::time_point{};
+        header.closeTimeResolution = NetClock::duration{10};
+        header.closeFlags = 0;
+        header.txHash = txHash;
+        header.accountHash = accountHash;
+        header.hash = calculateLedgerHash(header);
+        return header;
+    }
+
+    /**
+     * The common shape: no transactions, so only the state map is in play.
+     *
+     * @param chain The chain whose root to name as the state hash.
+     * @return The header, with its hash filled in.
+     */
+    static LedgerHeader
+    makeHeader(DeepChain const& chain)
+    {
+        return makeHeader(uint256{}, chain.rootHash.asUInt256());
+    }
+
+    /**
+     * Put the header in the local store, which is the first place tryDB()
+     * looks.
+     *
+     * Unlike a fetch pack, which hands each entry out once, the store
+     * keeps it, so more than one acquisition of the same ledger can find
+     * it.
+     *
+     * @param env The environment whose node store to seed.
+     * @param header The header to store, keyed by its own hash.
+     */
+    static void
+    storeHeader(jtx::Env& env, LedgerHeader const& header)
+    {
+        Serializer s;
+        s.add32(HashPrefix::LedgerMaster);
+        addRaw(header, s);
+
+        env.app().getNodeFamily().db().store(
+            NodeObjectType::Ledger, std::move(s.modData()), header.hash, header.seq);
+    }
+
+    /**
+     * Put every node of a chain in the local store, so a state-map walk
+     * resolves the whole map without a peer.
+     *
+     * @param env The environment whose node store to seed.
+     * @param header The header whose sequence the nodes are stored under.
+     * @param chain The chain supplying the nodes.
+     * @param maxDepth The deepest node to store, so a caller can leave a walk
+     *        something to ask for.
+     */
+    static void
+    storeStateNodes(
+        jtx::Env& env,
+        LedgerHeader const& header,
+        DeepChain const& chain,
+        unsigned int maxDepth)
+    {
+        auto& db = env.app().getNodeFamily().db();
+
+        for (auto depth = 0u; depth <= maxDepth; ++depth)
+        {
+            db.store(
+                NodeObjectType::AccountNode,
+                chain.prefixedNodeAt(depth),
+                chain.nodeAt(depth)->getHash().asUInt256(),
+                header.seq);
+        }
+    }
+
+    /**
+     * A ledger whose maps all resolve locally finishes on the spot, and the
+     * finished ledger is immutable and handed on.
+     *
+     * The case where tryDB() alone completes the acquisition, so it covers
+     * tryDB() reporting a ledger it found and done() taking its success arm
+     * on that path. Both entry points are driven: InboundLedgers::acquire()
+     * is the only caller of init(), and hands back the finished ledger
+     * itself, while checkLocal() is the route that reaches done().
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testLocalLedgerCompletesAcquire(jtx::Env& env)
+    {
+        testcase("A ledger found locally completes the acquire");
+
+        // A chain ending in a real leaf, so the state map is genuinely complete rather than merely
+        // rooted. No transactions, so only the state map is in play.
+        auto const chain = DeepChain::toLeaf(2, nextSeed());
+        auto const header = makeHeader(chain);
+
+        storeHeader(env, header);
+        storeStateNodes(env, header, chain, chain.deepestDepth);
+
+        // acquire() runs init() under its own collection lock and returns the ledger only once the
+        // acquisition is complete and unfailed, so a non-null result is what shows tryDB() found it
+        // without a peer ever being asked.
+        auto const acquired = env.app().getInboundLedgers().acquire(
+            header.hash, header.seq, InboundLedger::Reason::GENERIC);
+
+        BEAST_EXPECT(acquired != nullptr);
+        if (acquired)
+        {
+            BEAST_EXPECT(acquired->isImmutable());
+            BEAST_EXPECT(acquired->header().hash == header.hash);
+        }
+
+        // init() hands a ledger it completed to LedgerMaster itself, which is what makes it
+        // available to everything else.
+        BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) != nullptr);
+
+        // Nothing was logged as a failure, which is the other arm of done().
+        BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash));
+
+        // The same ledger through checkLocal(), which unlike init() reaches done(). Everything it
+        // needs is still in the store, since the first acquisition read rather than consumed it.
+        auto again = std::make_shared<InboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        // True because the acquisition ended, which here means it succeeded, and it reports that
+        // only after done() has run.
+        BEAST_EXPECT(again->checkLocal());
+        BEAST_EXPECT(again->isComplete());
+        BEAST_EXPECT(!again->isFailed());
+
+        auto const settled = again->getLedger();
+        BEAST_EXPECT(settled != nullptr);
+        if (settled)
+            BEAST_EXPECT(settled->isImmutable());
+
+        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 other order is what lets an unsettled ledger escape: isComplete()
+     * is read without mtx_, so a second thread can act on it while done()
+     * is still settling, and a mutable ledger reaching
+     * LedgerHistory::insert() calls logicError(). The order itself is only
+     * visible to a concurrent reader, so what this case pins is the
+     * consequence - the acquisition never reports a ledger that is not
+     * immutable - and that trigger() completes an acquisition without
+     * itself setting the completion flag.
+     *
+     * @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. That is what leaves the completion to trigger().
+        storeHeader(env, header);
+        storeStateNodes(env, header, chain, chain.deepestDepth - 1);
+
+        auto 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());
+
+        // Only now can the walk finish, so nothing but the walk can have completed this.
+        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 nothing was recorded as
+        // a failure.
+        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.
+     *
+     * done() settles the ledger before it logs or acts on the outcome, and a
+     * map abandoned by then makes settling refuse, so the acquisition has to
+     * record a failure instead. Reproduced by setting the flag and then
+     * invalidating the map, which is the order a walk on another thread
+     * produces without the second thread.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testInvalidatedLedgerFailsInDone(jtx::Env& env)
+    {
+        testcase("A ledger invalidated on its way to being settled fails");
+
+        // The fabricated chain, so feeding it to the state map invalidates the map.
+        DeepChain const chain{nextSeed()};
+
+        // Only the header is local, so the acquisition holds a ledger with an empty state map.
+        auto const header = makeHeader(chain);
+        storeHeader(env, header);
+
+        auto 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->isComplete());
+
+        auto const ledger = mutableLedger(*acquire);
+        BEAST_EXPECT(ledger != nullptr);
+        if (!ledger)
+            return;
+
+        // The state of affairs done() is handed: nothing left to fetch as far as the caller could
+        // 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(!stateMap.isValid());
+
+        acquire->signalDone();
+
+        // complete_ is withdrawn alongside the failure, or every guard that checks it before
+        // failed_ keeps treating this ledger as delivered.
+        BEAST_EXPECT(!acquire->isComplete());
+        BEAST_EXPECT(acquire->isFailed());
+
+        // Nothing was handed to LedgerMaster, and the hash is remembered as a failure so it is not
+        // immediately re-acquired.
+        BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) == nullptr);
+        BEAST_EXPECT(waitFor([&] { return env.app().getInboundLedgers().isFailure(header.hash); }));
+    }
+
+    /**
+     * An acquisition that fails on local data must still signal.
+     *
+     * Both entry points that reach tryDB() are covered, since without
+     * done() the object never signals, logFailure() never runs, and the
+     * hash never lands in recentFailures_ - so the same doomed ledger is
+     * asked for again on the next round. recentFailures_ is what the
+     * assertions watch, since it is the caller-visible consequence of
+     * having signalled.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testLocalFailureSignalsDone(jtx::Env& env)
+    {
+        testcase("An acquisition that fails locally still signals");
+
+        // A zero account hash is a ledger no acquisition can ever finish, and tryDB() says so as
+        // soon as it has the header.
+        auto const header = makeHeader(uint256{}, uint256{});
+        storeHeader(env, header);
+
+        BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash));
+
+        // acquire() is the only caller of init(), and hands back nothing for a failed acquisition.
+        BEAST_EXPECT(
+            env.app().getInboundLedgers().acquire(
+                header.hash, header.seq, InboundLedger::Reason::GENERIC) == nullptr);
+
+        // The failure reached recentFailures_, which is what stops the next round asking again.
+        BEAST_EXPECT(waitFor([&] { return env.app().getInboundLedgers().isFailure(header.hash); }));
+
+        // The other route into tryDB(): a trigger() on an acquisition that has no header yet. A
+        // hash of its own, so the entry above cannot answer for it.
+        auto const otherHeader = makeHeader(uint256{1}, uint256{});
+        storeHeader(env, otherHeader);
+
+        auto viaTrigger = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            otherHeader.hash,
+            otherHeader.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(otherHeader.hash));
+
+        viaTrigger->triggerAdded();
+
+        BEAST_EXPECT(viaTrigger->isFailed());
+        BEAST_EXPECT(!viaTrigger->isComplete());
+        BEAST_EXPECT(
+            waitFor([&] { return env.app().getInboundLedgers().isFailure(otherHeader.hash); }));
+    }
+
+    /**
+     * A ledger assembled from local data must be judged even when only
+     * one map is settled.
+     *
+     * tryDB() walks both maps to see what is on hand, and 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().
+     *
+     * The asymmetry is the point: 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() therefore
+     * sets neither flag and has to reach the verdict itself, since the
+     * setImmutable() call further down needs both.
+     *
+     * @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; 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 acquire = std::make_shared<InboundLedger>(
+            env.app(),
+            header.hash,
+            header.seq,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::make_unique<RequestCountingPeerSet>());
+
+        // checkLocal() routes into tryDB() without any peer data having arrived. It reports true
+        // only because the acquisition ended, which is what this case is about.
+        BEAST_EXPECT(acquire->checkLocal());
+
+        BEAST_EXPECT(acquire->isFailed());
+        BEAST_EXPECT(!acquire->isComplete());
+    }
+
+    /**
+     * The aggressive-retry branch of trigger() must judge a map the walk
+     * abandoned.
+     *
+     * That branch reads an empty getNeededHashes() result as "nothing
+     * left to fetch", and the walk it runs can reach the invalid verdict
+     * itself once nodes resolve from local storage rather than from a
+     * peer.
+     *
+     * The staging matters: tryDB() runs first and would shadow this guard
+     * if it could resolve the whole chain, so only the root is local to
+     * begin with - enough for the state map to hold a root, without which
+     * neededHashes() reports the root as missing and never walks, but not
+     * enough to reach the offending depth. Reaching the branch also needs
+     * a timeout count above kLedgerBecomeAggressiveThreshold, which the
+     * case records directly rather than waiting fifteen seconds for the
+     * timer chain to raise it.
+     *
+     * @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 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());
+
+        // Only now does the rest of the chain become resolvable, so tryDB() cannot have judged it.
+        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());
+
+        // haveState_ is what pins this guard rather than the setImmutable() backstop in done(),
+        // which also fails the acquire: without the guard the empty result reads as success, and
+        // every have-flag is set on the way to that backstop.
+        BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool());
+
+        // The same branch with no header yet, which is the other arm of hasInvalidMap(): there is
+        // no map to judge, and reading that as a verdict would fail an acquisition that has only
+        // just started. getNeededHashes() has asked for the header, so the non-empty branch is the
+        // right one and the acquisition stays alive.
+        auto 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 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.
+     *
+     * Nothing is local and no data ever arrives, so no tick can record progress
+     * and the count only climbs. A hash of its own, so no other case can have
+     * remembered it as a failure already.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testTimerRetriesThenGivesUp(jtx::Env& env)
+    {
+        testcase("The retry timer re-asks, then gives up");
+
+        uint256 const kUnknownLedger{8};
+
+        // One candidate, which onTimer() re-offers on every tick.
+        auto const candidate = std::make_shared<ChargeRecordingPeer>();
+        auto peerSet =
+            std::make_unique<RequestCountingPeerSet>(std::vector<std::shared_ptr<Peer>>{candidate});
+        auto* const peerSetPtr = peerSet.get();
+
+        auto acquire = std::make_shared<TestableInboundLedger>(
+            env.app(),
+            kUnknownLedger,
+            0,
+            InboundLedger::Reason::GENERIC,
+            stopwatch(),
+            std::move(peerSet),
+            kFastRetry);
+
+        BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(kUnknownLedger));
+
+        // init() finds nothing locally, so it asks the candidate and queues the first check-in,
+        // which is what arms the retry timer for every cycle after. Those first requests are not
+        // the ones under test, so count from here.
+        acquire->startAcquire();
+        BEAST_EXPECT(!acquire->isFailed());
+        int const requestsFromInit = peerSetPtr->requests();
+        BEAST_EXPECT(requestsFromInit > 0);
+        BEAST_EXPECT(peerSetPtr->addedPeers() == std::set<Peer::id_t>{candidate->id()});
+
+        // Every tick asks again, and past kLedgerTimeoutRetriesMax (6) the chain gives up.
+        BEAST_EXPECT(waitFor([&] { return acquire->isFailed(); }));
+        BEAST_EXPECT(!acquire->isComplete());
+        BEAST_EXPECT(peerSetPtr->requests() > requestsFromInit);
+
+        // done() remembered the hash, which is what stops the next round asking again.
+        BEAST_EXPECT(
+            waitFor([&] { return env.app().getInboundLedgers().isFailure(kUnknownLedger); }));
+    }
+
+    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.
+        jtx::Env env{*this};
+
+        testLocalLedgerCompletesAcquire(env);
+        testWalkSettlesBeforeReportingComplete(env);
+        testInvalidatedLedgerFailsInDone(env);
+        testLocalFailureSignalsDone(env);
+        testLocalChainFailsAcquire(env);
+        testAggressiveRetryJudgesLocalMap(env);
+
+        // Last: the only case that waits out a whole timeout chain.
+        testTimerRetriesThenGivesUp(env);
+    }
+
+private:
+    unsigned int seed_{0};
+};
+
+BEAST_DEFINE_TESTSUITE(InboundLedger, app, xrpl);
+
+}  // namespace xrpl::test

src/test/app/LedgerHistory_test.cpp

@@ -11,6 +11,7 @@
 
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/chrono.h>
+#include <xrpl/basics/contract.h>
 #include <xrpl/beast/insight/NullCollector.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/ledger/ApplyView.h>
@@ -22,6 +23,7 @@
 
 #include <cassert>
 #include <memory>
+#include <stdexcept>
 #include <vector>
 
 namespace xrpl::test {
@@ -70,11 +72,16 @@ public:
         }
         res->unshare();
 
-        // Accept ledger
-        res->setAccepted(
-            res->header().closeTime,
-            res->header().closeTimeResolution,
-            true /* close time correct*/);
+        // Accept ledger. Thrown rather than asserted because a failure leaves res unusable, and
+        // an assert would let a Release build hand every caller below a broken ledger. This helper
+        // is static, so BEAST_EXPECT is out of reach; the suite runner reports the throw.
+        if (!res->setAccepted(
+                res->header().closeTime,
+                res->header().closeTimeResolution,
+                true /* close time correct*/))
+        {
+            Throw<std::runtime_error>("makeLedger: ledger could not be accepted");
+        }
         lh.insert(res, false);
         return res;
     }

src/test/app/RCLValidations_test.cpp

@@ -100,7 +100,7 @@ class RCLValidations_test : public beast::unit_test::Suite
             BEAST_EXPECT(next->read(keylet::feeSettings()));
             if (forceHash)
             {
-                next->setImmutable();
+                BEAST_EXPECT(next->setImmutable());
                 forceHash = false;
             }
 

src/test/app/TransactionAcquire_test.cpp

@@ -0,0 +1,698 @@
+#include <test/app/AcquireTestHelpers.h>
+#include <test/jtx/Env.h>
+
+#include <xrpld/app/ledger/InboundTransactions.h>
+#include <xrpld/app/ledger/detail/TransactionAcquire.h>
+#include <xrpld/overlay/Peer.h>
+
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/resource/Fees.h>
+#include <xrpl/shamap/SHAMap.h>
+#include <xrpl/shamap/SHAMapAddNode.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <xrpl.pb.h>
+
+#include <chrono>
+#include <memory>
+#include <set>
+#include <thread>
+#include <utility>
+#include <vector>
+
+namespace xrpl::test {
+
+/**
+ * An acquisition that exposes the state its bases keep protected, so a case can
+ * reach it without the daemon's API growing.
+ */
+struct TestableTransactionAcquire final : TransactionAcquire
+{
+    using TransactionAcquire::TransactionAcquire;
+
+    /**
+     * Whether the set being acquired is still structurally coherent.
+     *
+     * @return Whether the map is still valid.
+     */
+    [[nodiscard]] bool
+    isMapValid() const
+    {
+        // Under the lock: a batch on another thread can reach the verdict.
+        ScopedLockType const sl(mtx_);
+        return map_->isValid();
+    }
+
+    /**
+     * Whether a batch has advanced the set since the flag was last cleared.
+     *
+     * @return Whether progress has been recorded.
+     */
+    [[nodiscard]] bool
+    madeProgress() const
+    {
+        // Under the lock: a timer tick clears the flag on a job thread.
+        ScopedLockType const sl(mtx_);
+        return progress_;
+    }
+
+    /**
+     * Forget any recorded progress.
+     */
+    void
+    clearProgress()
+    {
+        ScopedLockType const sl(mtx_);
+        progress_ = false;
+    }
+};
+
+struct TransactionAcquire_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. At this interval the
+     * window between the first retry (four timeouts in) and giving up (twenty)
+     * is still a third of a second, which is what the one case that watches
+     * both needs.
+     */
+    static constexpr auto kFastRetry = std::chrono::milliseconds{20};
+
+    /**
+     * A seed no other chain in this suite has used.
+     *
+     * The Env below is shared, and ConsensusTransSetSF::gotNode() puts
+     * every node it accepts into the application-wide NodeCache while
+     * InboundTransactions keys its acquisitions by set hash, so two cases
+     * building identically seeded chains would let one resolve or revive
+     * the other's. Handing out a fresh seed per chain makes that
+     * impossible rather than merely unlikely.
+     *
+     * @return The seed.
+     */
+    [[nodiscard]] unsigned int
+    nextSeed()
+    {
+        return ++seed_;
+    }
+
+    /**
+     * Whether takeNodes() declined to look at the data at all.
+     *
+     * TimeoutCounter::complete_ and failed_ are both protected, so this stands in
+     * for either: a done acquisition returns a verdict accounting for nothing.
+     *
+     * @param san The verdict a takeNodes() call returned.
+     * @return Whether that verdict shows the data was never looked at.
+     */
+    static bool
+    wasIgnored(SHAMapAddNode const& san)
+    {
+        return tallyIs(san, 0, 0, 0);
+    }
+
+    /**
+     * Wait for a finished set to reach InboundTransactions.
+     *
+     * done() hands the map over through a job, so this is what shows an
+     * acquisition completed rather than merely stopped.
+     *
+     * @param env The environment whose InboundTransactions to watch.
+     * @param setHash The set to wait for.
+     * @return The delivered map, or nullptr if none arrived.
+     */
+    [[nodiscard]] static std::shared_ptr<SHAMap>
+    waitForDeliveredSet(jtx::Env& env, uint256 const& setHash)
+    {
+        auto& inbound = env.app().getInboundTransactions();
+
+        // acquire=false: asking to acquire would spin up a second, unrelated acquisition for
+        // this hash (or, if one is already registered, needlessly poke stillNeed() on it).
+        //
+        // A shorter deadline than the polling default: the job is queued before takeNodes()
+        // returns, so anything beyond a few seconds means it is never coming.
+        std::shared_ptr<SHAMap> delivered;
+        if (!waitFor(
+                [&] { return (delivered = inbound.getSet(setHash, false)) != nullptr; },
+                std::chrono::seconds{5}))
+            return nullptr;
+        return delivered;
+    }
+
+    /**
+     * A chain ending in a real leaf completes the acquisition, and later
+     * replies for it are then left alone.
+     *
+     * Also pins the two things that follow from finishing: nothing more
+     * is asked for, and done() hands the map to InboundTransactions,
+     * which is what consensus is waiting on.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testHappyPathCompletesAcquisition(jtx::Env& env)
+    {
+        testcase("A chain ending in a leaf completes the acquire");
+
+        auto const chain = DeepChain::toLeaf(3, nextSeed());
+
+        auto peerSet = std::make_unique<RequestCountingPeerSet>();
+        auto* const peerSetPtr = peerSet.get();
+
+        uint256 const setHash = chain.rootHash.asUInt256();
+        auto const acquire =
+            std::make_shared<TransactionAcquire>(env.app(), setHash, std::move(peerSet));
+        auto const peer = std::make_shared<ChargeRecordingPeer>();
+
+        // The root alone leaves the set incomplete, so accepting it asks for the level
+        // below.
+        auto const rootResult = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer);
+        BEAST_EXPECT(rootResult.isUseful());
+        int const requestsWhileIncomplete = peerSetPtr->requests();
+        BEAST_EXPECT(requestsWhileIncomplete > 0);
+
+        // The rest of the chain, ending in the leaf.
+        auto const result = acquire->takeNodes(chain.nodesBelowRoot(), peer);
+        BEAST_EXPECT(result.isUseful());
+        BEAST_EXPECT(!result.isInvalid());
+
+        // Nothing more went out, which rules out the acquisition still asking for nodes.
+        // trigger() also sends nothing when it gives up, so the delivered set below is what
+        // shows it finished.
+        BEAST_EXPECT(peerSetPtr->requests() == requestsWhileIncomplete);
+
+        // done() hands the map over only when it has not failed, so this is what separates
+        // completion from failure.
+        auto const delivered = waitForDeliveredSet(env, setHash);
+        BEAST_EXPECT(delivered != nullptr);
+        if (delivered)
+        {
+            BEAST_EXPECT(delivered->getHash() == chain.rootHash);
+            BEAST_EXPECT(delivered->isValid());
+        }
+
+        // A reply arriving after the set is finished is not examined, and does not restart the
+        // asking - the ordinary fate of every responder to trigger()'s broadcast but the one that
+        // completed the set. init() was never called, so no timer can have failed the acquisition
+        // since the set above was delivered.
+        BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer)));
+        BEAST_EXPECT(peerSetPtr->requests() == requestsWhileIncomplete);
+    }
+
+    /**
+     * Two peers each answering with a different missing piece are both accepted
+     * without penalty, and the set completes from their combined replies.
+     *
+     * Driven through InboundTransactions::gotData() so the leaf goes over the
+     * real dispatch, which rebuilds a leaf's position from its own key rather
+     * than trusting the sender's label.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testTwoPeersEachSupplyPartOfTheSet(jtx::Env& env)
+    {
+        testcase("Two peers each supplying part of a set are both accepted without penalty");
+
+        auto const chain = DeepChain::toLeaf(3, nextSeed());
+        auto& inbound = env.app().getInboundTransactions();
+
+        // getSet() with acquire=true registers the TransactionAcquire that gotData() then
+        // looks up by hash.
+        uint256 const setHash = chain.rootHash.asUInt256();
+        BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr);
+
+        // The first peer answers with the root only.
+        auto const peerA = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, peerA, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}));
+        BEAST_EXPECT(peerA->charges().empty());
+
+        // The second answers with everything the first left out, and finishes the set.
+        auto const peerB = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, peerB, packetFor(chain, chain.nodesBelowRoot()));
+        BEAST_EXPECT(peerB->charges().empty());
+
+        // Completion does not depend on which peer sent which piece: the set finishes just
+        // as it does when one peer supplies the lot.
+        auto const delivered = waitForDeliveredSet(env, setHash);
+        BEAST_EXPECT(delivered != nullptr);
+        if (delivered)
+            BEAST_EXPECT(delivered->getHash() == chain.rootHash);
+    }
+
+    /**
+     * A root that does not hash to the set we asked for is a plain mismatch,
+     * and has to leave the acquisition able to try another peer.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testBadRootKeepsAcquireAlive(jtx::Env& env)
+    {
+        testcase("A mismatched root leaves the acquire recoverable");
+
+        DeepChain const chain{nextSeed()};
+
+        // Acquire an unrelated hash, so the chain's root cannot match it.
+        auto const acquire = std::make_shared<TestableTransactionAcquire>(
+            env.app(), uint256{42}, std::make_unique<RequestCountingPeerSet>());
+        auto const peer = std::make_shared<ChargeRecordingPeer>();
+
+        auto const result = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer);
+        BEAST_EXPECTS(tallyIs(result, 0, 1, 0), result.get());
+
+        // A mismatched root says nothing about the tree behind the hash we asked for, so the map
+        // is untouched.
+        BEAST_EXPECT(acquire->isMapValid());
+
+        // Still alive: the next packet is examined rather than waved through.
+        BEAST_EXPECT(!wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer)));
+    }
+
+    /**
+     * A second reply carrying a root we already have must stay free.
+     *
+     * This is what an honest second responder to the initial fan-out sends:
+     * trigger() broadcasts to every tracked peer, so several answer the same
+     * request and all but the first carry nothing new. Charging for that would
+     * penalize peers for answering.
+     *
+     * Covers the root specifically, which takeNodes() short-circuits on
+     * haveRoot_ without consulting the map. A repeated non-root node takes the
+     * other route, through addKnownNode() - see
+     * testDuplicateNonRootReplyIsFree().
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testDuplicateRootReplyIsFree(jtx::Env& env)
+    {
+        testcase("A reply of a root we already have is free");
+
+        DeepChain const chain{nextSeed()};
+        auto& inbound = env.app().getInboundTransactions();
+
+        // getSet() with acquire=true registers the TransactionAcquire that gotData() then looks up
+        // by hash.
+        uint256 const setHash = chain.rootHash.asUInt256();
+        BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr);
+
+        auto const rootPacket = packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}});
+
+        // The first responder supplies the root, which is genuinely useful.
+        auto const firstPeer = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, firstPeer, rootPacket);
+        BEAST_EXPECT(firstPeer->charges().empty());
+
+        // The second sends the same root. Nothing is added to the map, but the peer did what we
+        // asked, so it must not be charged.
+        auto const secondPeer = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, secondPeer, rootPacket);
+        BEAST_EXPECT(secondPeer->charges().empty());
+    }
+
+    /**
+     * A repeated non-root node must stay free too.
+     *
+     * The counterpart to testDuplicateRootReplyIsFree(), covering the
+     * route that does consult the map: addKnownNode() reports a node it
+     * already holds as a duplicate, and takeNodes() tests isGood(), which
+     * counts a duplicate as success. Testing isUseful() there instead
+     * would turn every honest second responder into a peer we charge for
+     * invalid data.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testDuplicateNonRootReplyIsFree(jtx::Env& env)
+    {
+        testcase("A repeated non-root node is free");
+
+        auto const chain = DeepChain::toLeaf(3, nextSeed());
+        auto& inbound = env.app().getInboundTransactions();
+
+        uint256 const setHash = chain.rootHash.asUInt256();
+        BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr);
+
+        auto const rootPeer = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}));
+        BEAST_EXPECT(rootPeer->charges().empty());
+
+        // Depth 1 alone, so the set stays incomplete and the acquisition keeps examining data
+        // rather than waving the second copy through as a late reply.
+        auto const level1 = packetFor(chain, {{chain.idAt(1), chain.nodeAt(1)}});
+
+        auto const firstPeer = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, firstPeer, level1);
+        BEAST_EXPECT(firstPeer->charges().empty());
+
+        auto const secondPeer = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, secondPeer, level1);
+        BEAST_EXPECT(secondPeer->charges().empty());
+    }
+
+    /**
+     * A reply whose node data cannot be deserialized is charged for.
+     *
+     * gotData() rejects the packet before the acquisition is handed
+     * anything, so this pins the charge on the dispatch layer rather than
+     * on takeNodes(). It also gives this suite's "was not charged"
+     * assertions their teeth: a harness that recorded no charge at all
+     * would satisfy all of them and fail only here.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testUndeserializableNodeIsCharged(jtx::Env& env)
+    {
+        testcase("A reply with undeserializable node data is charged");
+
+        DeepChain const chain{nextSeed()};
+        auto& inbound = env.app().getInboundTransactions();
+
+        uint256 const setHash = chain.rootHash.asUInt256();
+        BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr);
+
+        // A single byte naming a wire type that does not exist, so getTreeNode() rejects it
+        // before the acquisition is handed anything.
+        auto packet = std::make_shared<protocol::TMLedgerData>();
+        packet->set_ledgerhash(setHash.data(), uint256::size());
+        packet->set_ledgerseq(0);
+        packet->set_type(protocol::liTS_CANDIDATE);
+
+        auto* const node = packet->add_nodes();
+        node->set_nodedata("\xff", 1);
+        node->set_id(SHAMapNodeID{}.getRawString());
+
+        auto const peer = std::make_shared<ChargeRecordingPeer>();
+        inbound.gotData(setHash, peer, packet);
+
+        BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData});
+    }
+
+    /**
+     * A batch that ends on a bad node still counts the good nodes ahead of it,
+     * and a batch that achieved nothing records no progress.
+     *
+     * The recorded progress is what the verdict is for: it stops the
+     * next timer tick from counting a timeout, so reporting only the
+     * node the batch stopped on would push an acquisition that is
+     * genuinely advancing toward kMaxTimeouts, while recording progress
+     * for a batch we already had would hold a stalled one open. The flag
+     * is read rather than the returned tally, which only stands in for
+     * it, and cleared between batches so each reading is about the batch
+     * just fed.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testPartialBatchIsCounted(jtx::Env& env)
+    {
+        testcase("A batch ending on a bad node still counts the good nodes");
+
+        DeepChain const chain{nextSeed()};
+
+        auto const acquire = std::make_shared<TestableTransactionAcquire>(
+            env.app(), chain.rootHash.asUInt256(), std::make_unique<RequestCountingPeerSet>());
+        auto const peer = std::make_shared<ChargeRecordingPeer>();
+
+        // The root is useful, so it records progress.
+        acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer);
+        BEAST_EXPECT(acquire->madeProgress());
+        acquire->clearProgress();
+
+        // Good nodes at depths 1 and 2, then a further chain node mislabeled at a position it
+        // cannot occupy. The map stays sound, so only the last node is bad.
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> batch;
+        batch.emplace_back(SHAMapNodeID{1, uint256{}}, chain.nodeAt(1));
+        batch.emplace_back(SHAMapNodeID{2, uint256{}}, chain.nodeAt(2));
+        batch.emplace_back(SHAMapNodeID{9, uint256{}}, chain.nodeAt(3));
+
+        auto const san = acquire->takeNodes(batch, peer);
+
+        // The verdict names both halves rather than just the failure, and the two good nodes are
+        // what the next timer tick must not count as a timeout. The batch stops on the bad node,
+        // so exactly one is counted however many were left unexamined behind it.
+        BEAST_EXPECTS(tallyIs(san, 2, 1, 0), san.get());
+        BEAST_EXPECT(san.isUseful());
+        BEAST_EXPECT(acquire->madeProgress());
+        BEAST_EXPECT(acquire->isMapValid());
+
+        // A batch of nothing but the root we already have: counted as a duplicate rather than
+        // reaching the clean exit with nothing counted, so it is neither reported as useful nor
+        // allowed to postpone the timeout.
+        acquire->clearProgress();
+        auto const repeatedRoot = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer);
+
+        BEAST_EXPECTS(tallyIs(repeatedRoot, 0, 0, 1), repeatedRoot.get());
+        BEAST_EXPECT(repeatedRoot.isGood());
+        BEAST_EXPECT(!repeatedRoot.isUseful());
+        BEAST_EXPECT(!acquire->madeProgress());
+
+        // The same root alongside a node we do need: the duplicate is reported as one, and the
+        // node that did hook in is what records the progress.
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> mixed;
+        mixed.emplace_back(SHAMapNodeID{}, chain.nodeAt(0));
+        mixed.emplace_back(SHAMapNodeID{3, uint256{}}, chain.nodeAt(3));
+
+        auto const withDuplicateRoot = acquire->takeNodes(mixed, peer);
+
+        BEAST_EXPECTS(tallyIs(withDuplicateRoot, 1, 0, 1), withDuplicateRoot.get());
+        BEAST_EXPECT(withDuplicateRoot.isUseful());
+        BEAST_EXPECT(acquire->madeProgress());
+
+        // takeNodes() charges nobody: InboundTransactions::gotData() reads the verdict and decides.
+        BEAST_EXPECT(peer->charges().empty());
+    }
+
+    /**
+     * init() asks only the peers that claim to have the set.
+     *
+     * addPeers() passes hasTxSet(hash_) as its filter and trigger() as its
+     * callback, so a peer that says it has the set is asked and one that says
+     * it does not is left alone. Getting this wrong wastes a request on every
+     * peer in the overlay for every set.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testInitAsksOnlyPeersWithTheSet(jtx::Env& env)
+    {
+        testcase("init() asks only the peers that have the set");
+
+        DeepChain const chain{nextSeed()};
+
+        // Ordered with the useless peer first, so a filter that is ignored altogether
+        // shows up as the wrong peer being asked rather than as one extra request.
+        auto const withoutSet = std::make_shared<ChargeRecordingPeer>(false);
+        auto const withSet = std::make_shared<ChargeRecordingPeer>(true);
+
+        auto peerSet = std::make_unique<RequestCountingPeerSet>(
+            std::vector<std::shared_ptr<Peer>>{withoutSet, withSet});
+        auto* const peerSetPtr = peerSet.get();
+
+        auto const acquire = std::make_shared<TransactionAcquire>(
+            env.app(), chain.rootHash.asUInt256(), std::move(peerSet));
+
+        static constexpr int kStartPeers = 2;
+        acquire->init(kStartPeers);
+
+        // Stop the retry loop, which would otherwise keep offering the same candidates
+        // for as long as this case runs.
+        acquire->cancel();
+
+        BEAST_EXPECT(peerSetPtr->firstLimit() == kStartPeers);
+        BEAST_EXPECT(peerSetPtr->addedPeers() == std::set<Peer::id_t>{withSet->id()});
+
+        // The peer that was added is also asked, rather than merely tracked.
+        BEAST_EXPECT(peerSetPtr->requests() >= 1);
+    }
+
+    /**
+     * A timed-out acquisition asks again, and examines data again, once
+     * stillNeed() revives it.
+     *
+     * The pending timer is private to TimeoutCounter, so what stands in
+     * for observing it is a candidate peer the acquisition can only
+     * reach from onTimer(): nothing else in this case calls addPeers(),
+     * so a request going out after stillNeed() means the timer chain was
+     * restarted rather than just the failed flag cleared.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testRevivedAcquireCanRequestAgain(jtx::Env& env)
+    {
+        testcase("A revived acquire asks again and accepts data again");
+
+        DeepChain const chain{nextSeed()};
+
+        // One candidate, offered only by onTimer()'s addPeers(1), since init() is never called.
+        auto const candidate = std::make_shared<ChargeRecordingPeer>();
+        auto peerSet =
+            std::make_unique<RequestCountingPeerSet>(std::vector<std::shared_ptr<Peer>>{candidate});
+        auto* const peerSetPtr = peerSet.get();
+
+        // A short interval, since this case waits out one of them.
+        auto const acquire = std::make_shared<TransactionAcquire>(
+            env.app(), chain.rootHash.asUInt256(), std::move(peerSet), kFastRetry);
+        auto const peer = std::make_shared<ChargeRecordingPeer>();
+
+        // Time the acquisition out, which is what stillNeed() exists to undo.
+        acquire->cancel();
+        BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer)));
+
+        // No timer is pending: none was ever armed, and a packet a failed acquisition ignores does
+        // not arm one. Without this the wait below would prove nothing.
+        BEAST_EXPECT(peerSetPtr->requests() == 0);
+        BEAST_EXPECT(peerSetPtr->addedPeers().empty());
+
+        // Revived, so the timer chain restarts and its first tick offers the candidate.
+        acquire->stillNeed();
+        BEAST_EXPECT(waitFor([&] { return peerSetPtr->requests() > 0; }));
+        BEAST_EXPECT(peerSetPtr->addedPeers() == std::set<Peer::id_t>{candidate->id()});
+        int const requestsFromTheTimer = peerSetPtr->requests();
+
+        // Data is examined again too, and accepting the root asks for the next level. The wait
+        // above returns on the first tick, so the rest of the timeout chain has still to run
+        // before the acquisition could give up again.
+        auto const revived = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer);
+        BEAST_EXPECT(!wasIgnored(revived));
+        BEAST_EXPECT(revived.isUseful());
+        BEAST_EXPECT(peerSetPtr->requests() > requestsFromTheTimer);
+
+        // Stop the retry loop, which would otherwise keep asking for as long as this case runs.
+        acquire->cancel();
+    }
+
+    /**
+     * A running acquisition keeps the wait it already has.
+     *
+     * The other half of the same guard: consensus asks for a set it
+     * still needs once per round, so without the early return every ask
+     * would re-arm the timer and a set asked for more often than the
+     * interval would never tick at all. setTimer() cancels any pending
+     * wait, so calling stillNeed() faster than the interval is what
+     * makes that visible.
+     *
+     * Keeps the production interval, unlike the case above: the asking
+     * has to be clearly faster than the wait for a surviving tick to
+     * mean anything.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testStillNeedLeavesARunningAcquireAlone(jtx::Env& env)
+    {
+        testcase("A running acquire keeps the wait it has");
+
+        // One candidate, so every tick that survives produces a request.
+        auto const candidate = std::make_shared<ChargeRecordingPeer>();
+        auto peerSet =
+            std::make_unique<RequestCountingPeerSet>(std::vector<std::shared_ptr<Peer>>{candidate});
+        auto* const peerSetPtr = peerSet.get();
+
+        // An unrelated hash: nothing here feeds it data, so it stays incomplete and keeps asking.
+        auto const acquire =
+            std::make_shared<TransactionAcquire>(env.app(), uint256{43}, std::move(peerSet));
+
+        // init() asks the candidate once and arms the timer. That first request is not the one
+        // under test, so count from here.
+        acquire->init(1);
+        int const requestsFromInit = peerSetPtr->requests();
+
+        // Ask again far faster than the interval, the way a short consensus round would. Every ask
+        // clamps the timeout count, so the acquisition cannot give up while this runs.
+        auto const askAgainRepeatedly = [&] {
+            acquire->stillNeed();
+            std::this_thread::sleep_for(std::chrono::milliseconds{20});
+            return peerSetPtr->requests() > requestsFromInit;
+        };
+
+        // A tick gets through despite the asking, which it could not if each ask re-armed the wait.
+        BEAST_EXPECT(waitFor(askAgainRepeatedly));
+
+        acquire->cancel();
+    }
+
+    /**
+     * The retry timer re-asks with no peer of its own, then gives up on
+     * its own.
+     *
+     * Pins the two behaviors, not the thresholds they trip at: bounding those
+     * means asserting on wall clock. Both are read in one poll, so a fast
+     * interval cannot let the give-up land between the two readings.
+     *
+     * @param env The environment to run in.
+     */
+    void
+    testTimerRetriesThenGivesUp(jtx::Env& env)
+    {
+        testcase("The retry timer re-asks, then gives up");
+
+        DeepChain const chain{nextSeed()};
+
+        auto peerSet = std::make_unique<RequestCountingPeerSet>();
+        auto* const peerSetPtr = peerSet.get();
+
+        // An unrelated hash, so the probe below can never be accepted. See probe().
+        auto const acquire = std::make_shared<TransactionAcquire>(
+            env.app(), uint256{42}, std::move(peerSet), kFastRetry);
+
+        // No candidates, so nothing goes out until onTimer() decides to broadcast.
+        acquire->init(1);
+        BEAST_EXPECT(peerSetPtr->requests() == 0);
+
+        // A root that cannot hash to this acquisition's set is rejected without recording
+        // progress, so polling with it does not postpone the timeout being waited for. A fresh
+        // peer each time keeps the rejections from piling up on one.
+        auto const probe = [&] {
+            return wasIgnored(acquire->takeNodes(
+                {{SHAMapNodeID{}, chain.nodeAt(0)}}, std::make_shared<ChargeRecordingPeer>()));
+        };
+
+        // kNormTimeouts (4) intervals in, onTimer() starts asking again with no peer of its own to
+        // ask, and the acquisition is still examining data at that point.
+        BEAST_EXPECT(waitFor([&] { return peerSetPtr->requests() > 0 && !probe(); }));
+
+        // Past kMaxTimeouts (20) it fails itself, and stops examining data.
+        BEAST_EXPECT(waitFor(probe));
+
+        // Whichever poll saw the retry, the count it left behind is what records that it happened.
+        BEAST_EXPECT(peerSetPtr->requests() > 0);
+    }
+
+    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(): gotNode() puts every node it accepts
+        // into the application-wide NodeCache, so cases sharing an Env must not share a hash.
+        jtx::Env env{*this};
+
+        testHappyPathCompletesAcquisition(env);
+        testTwoPeersEachSupplyPartOfTheSet(env);
+        testBadRootKeepsAcquireAlive(env);
+        testDuplicateRootReplyIsFree(env);
+        testDuplicateNonRootReplyIsFree(env);
+        testUndeserializableNodeIsCharged(env);
+        testPartialBatchIsCounted(env);
+        testInitAsksOnlyPeersWithTheSet(env);
+        testRevivedAcquireCanRequestAgain(env);
+        testStillNeedLeavesARunningAcquireAlone(env);
+
+        // Last: the only case that waits out a whole timeout chain.
+        testTimerRetriesThenGivesUp(env);
+    }
+
+private:
+    unsigned int seed_{0};
+};
+
+BEAST_DEFINE_TESTSUITE(TransactionAcquire, app, xrpl);
+
+}  // namespace xrpl::test

src/tests/libxrpl/helpers/TxTest.cpp

@@ -207,7 +207,10 @@ TxTest::close()
         accum.apply(*newLedger);
     }
 
-    newLedger->setAccepted(ledgerCloseTime, newLedger->header().closeTimeResolution, true);
+    if (!newLedger->setAccepted(ledgerCloseTime, newLedger->header().closeTimeResolution, true))
+    {
+        Throw<std::runtime_error>("TxTest::close: ledger has an invalid map");
+    }
 
     closedLedger_ = newLedger;
 

src/tests/libxrpl/shamap/DeepChain.h

@@ -0,0 +1,322 @@
+#pragma once
+
+#include <xrpl/basics/Blob.h>
+#include <xrpl/basics/SHAMapHash.h>
+#include <xrpl/basics/Slice.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/basics/contract.h>
+#include <xrpl/protocol/Serializer.h>
+#include <xrpl/shamap/SHAMap.h>
+#include <xrpl/shamap/SHAMapAddNode.h>
+#include <xrpl/shamap/SHAMapLeafNode.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <cstddef>
+#include <optional>
+#include <stdexcept>
+#include <utility>
+#include <vector>
+
+namespace xrpl::tests {
+
+/**
+ * A chain of inner nodes, each with one real child (and, when built with a decoy, an
+ * additional unresolvable second child), from the root down to one deepest node, in
+ * wire form.
+ *
+ * Built bottom-up, so the root hash commits to the whole shape and every node
+ * hashes correctly, which lets a test supply one level at a time and have each
+ * accepted on its own merits. Every node sits on the branch pathKey selects at
+ * its depth, so a receiver descending towards that key walks the whole chain.
+ *
+ * Two shapes, separating a chain a peer could honestly send from one it could
+ * not:
+ *
+ * - the constructors run inner nodes all the way to SHAMap::kLeafDepth,
+ *   where no valid tree can hold one (see the badDepth check in
+ *   SHAMap::addKnownNode), so feeding it invalidates the map;
+ * - toLeaf() stops at a real transaction leaf, so feeding it completes an
+ *   acquisition.
+ *
+ * Shared by the gtest suites and the daemon's own, which is why nothing here
+ * reaches outside libxrpl: xrpl_tests links only xrpl.libxrpl, while Peer,
+ * PeerSet and the protobuf reply builder are all xrpld. The xrpld half of this
+ * helper is test::packetFor() in src/test/app/AcquireTestHelpers.h.
+ */
+struct DeepChain
+{
+    // nodes[d] is the deserialized node for depth d.
+    std::vector<SHAMapTreeNodePtr> nodes;
+    SHAMapHash rootHash;
+
+    // The key whose path through the tree this chain spells out. Zero for a
+    // fabricated chain, which therefore sits on branch 0 at every depth.
+    uint256 pathKey;
+
+    // The depth of the deepest node, which is the last one nodesBelowRoot() hands out.
+    unsigned int deepestDepth{SHAMap::kLeafDepth};
+
+    /**
+     * The payload size of the leaf toLeaf() builds, which is the smallest a
+     * SHAMap item may be. Published so a caller can relate it to a threshold of
+     * its own, as AcquireTestHelpers.h does.
+     */
+    static constexpr std::size_t kLeafItemBytes = kMinShaMapItemBytes;
+
+    /**
+     * A chain of inner nodes reaching SHAMap::kLeafDepth, which no valid tree
+     * can hold.
+     *
+     * @param seed Varies the whole chain, so two chains can coexist without one
+     *        resolving the other's nodes. Caches and fetch packs are keyed by
+     *        hash, so identically-seeded chains are the same chain.
+     */
+    explicit DeepChain(unsigned int seed = 1) : DeepChain(std::nullopt, seed, Decoy::No)
+    {
+    }
+
+    /**
+     * The same chain, with a second and unresolvable child at every level.
+     *
+     * On a backed map descendAsync() then posts a real asynchronous read at
+     * every level, which is what leaves reads in flight when a walk reaches
+     * kLeafDepth. Offered only for this shape: the decoy sits on branch 1,
+     * which is free only because a fabricated chain's pathKey is zero and
+     * so every real child sits on branch 0.
+     *
+     * @param seed Varies the whole chain. See the constructor.
+     * @return The chain.
+     */
+    [[nodiscard]] static DeepChain
+    withDecoys(unsigned int seed = 1)
+    {
+        return DeepChain{std::nullopt, seed, Decoy::Yes};
+    }
+
+    /**
+     * A chain ending in a real transaction leaf, which completes an
+     * acquisition.
+     *
+     * @param depth Where the leaf sits, at most SHAMap::kLeafDepth. Zero puts
+     *        the leaf at the root. Deeper is sparser than a one-transaction set
+     *        would really be, but the sync path judges nodes by their hashes
+     *        rather than by how sparse they are.
+     * @param seed Varies the leaf's contents, and so the whole chain. See the
+     *        constructor.
+     * @return The chain.
+     */
+    [[nodiscard]] static DeepChain
+    toLeaf(unsigned int depth, unsigned int seed = 1)
+    {
+        return DeepChain{std::optional{depth}, seed, Decoy::No};
+    }
+
+    /**
+     * The node the chain holds at the given depth, root first.
+     *
+     * @param depth The depth of the node to return, at most deepestDepth.
+     * @return The node.
+     */
+    [[nodiscard]] SHAMapTreeNodePtr
+    nodeAt(unsigned int depth) const
+    {
+        return nodes[depth];
+    }
+
+    /**
+     * Where the node at the given depth claims to belong, which is on the
+     * path to pathKey.
+     *
+     * @param depth The depth of the node to locate.
+     * @return The node's claimed position.
+     */
+    [[nodiscard]] SHAMapNodeID
+    idAt(unsigned int depth) const
+    {
+        return SHAMapNodeID::createID(depth, pathKey);
+    }
+
+    /**
+     * The same node in the prefixed form used for storage and fetch packs,
+     * which is what hashes to the node's own hash.
+     *
+     * @param depth The depth of the node to serialize.
+     * @return The node's prefixed serialized form.
+     */
+    [[nodiscard]] Blob
+    prefixedNodeAt(unsigned int depth) const
+    {
+        Serializer s;
+        nodeAt(depth)->serializeWithPrefix(s);
+        return s.modData();
+    }
+
+    /**
+     * Every node below the root, down to and including the deepest one.
+     *
+     * @param firstDepth The shallowest node to include, so a caller can feed
+     *        the chain in more than one batch.
+     * @return The nodes, each with its claimed position.
+     */
+    [[nodiscard]] std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>>
+    nodesBelowRoot(unsigned int firstDepth = 1) const
+    {
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data;
+        for (auto depth = firstDepth; depth <= deepestDepth; ++depth)
+            data.emplace_back(idAt(depth), nodeAt(depth));
+        return data;
+    }
+
+    /**
+     * Fill a synching map, stopping one level short of the deepest node so the
+     * caller offers that one itself.
+     *
+     * Reports rather than asserts, since this header is shared with a binary
+     * that has no test framework of its own to assert through.
+     *
+     * @param map The map to fill.
+     * @return Whether the root and every node above the deepest one was
+     *         accepted - a property of the chain, not of the map.
+     */
+    [[nodiscard]] bool
+    fill(SHAMap& map) const
+    {
+        if (!map.addRootNode(rootHash, nodeAt(0), nullptr).isGood())
+            return false;
+
+        for (auto depth = 1u; depth < deepestDepth; ++depth)
+        {
+            if (!map.addKnownNode(idAt(depth), nodeAt(depth), nullptr).isUseful())
+                return false;
+        }
+
+        return true;
+    }
+
+    /**
+     * Offer the deepest node, which for a fabricated chain is the inner node at
+     * SHAMap::kLeafDepth that no valid tree can hold.
+     *
+     * @param map The map to offer the node to, filled by fill() first.
+     * @return The verdict addKnownNode() reached.
+     */
+    [[nodiscard]] SHAMapAddNode
+    addOffendingNode(SHAMap& map) const
+    {
+        return map.addKnownNode(idAt(deepestDepth), nodeAt(deepestDepth), nullptr);
+    }
+
+private:
+    // Whether each level carries a second child that is never stored anywhere.
+    enum class Decoy { No, Yes };
+
+    /**
+     * Build any of the shapes.
+     *
+     * @param leafDepth Where a real transaction leaf sits, or nullopt to run
+     *        inner nodes all the way to SHAMap::kLeafDepth instead.
+     * @param seed Varies the chain's contents. See the public entry points.
+     * @param decoy Whether every level carries an unresolvable second child.
+     */
+    DeepChain(std::optional<unsigned int> leafDepth, unsigned int seed, Decoy decoy)
+        : nodes(leafDepth.value_or(SHAMap::kLeafDepth) + 1)
+    {
+        if (!leafDepth)
+        {
+            // No leaf, so the deepest inner node points at a child that is never fetched.
+            buildInnersDownTo(SHAMap::kLeafDepth, SHAMapHash{uint256{seed}}, decoy);
+            return;
+        }
+
+        // Exactly kLeafItemBytes of payload, the smallest a leaf item may be, which keeps the leaf
+        // below the size at which a receiver tries to parse one as a transaction. Checked rather
+        // than assumed, since a caller relates that constant to a threshold of its own.
+        Serializer payload;
+        payload.add32(seed);
+        payload.add32(0);
+        payload.add32(0);
+        if (payload.size() != kLeafItemBytes)
+            Throw<std::logic_error>("DeepChain: unexpected leaf payload size");
+
+        Serializer wire;
+        wire.addRaw(payload.peekData());
+        wire.add8(kWireTypeTransaction);
+
+        auto const leaf = SHAMapTreeNode::makeFromWire(makeSlice(wire.peekData()));
+
+        // A transaction leaf's key is the hash of its own contents, so the chain above it
+        // has no say in where it sits: it has to follow this key's nibbles.
+        pathKey = leafKey(*leaf);
+        deepestDepth = *leafDepth;
+        nodes[*leafDepth] = leaf;
+
+        if (*leafDepth == 0)
+        {
+            // The leaf is the root, so there is nothing above it to build.
+            rootHash = leaf->getHash();
+            return;
+        }
+
+        buildInnersDownTo(*leafDepth - 1, leaf->getHash(), decoy);
+    }
+
+    /**
+     * 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.
+     *
+     * 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.
+     * @param childHash What that deepest inner node points at.
+     * @param decoy Whether to add an unresolvable second child at every level.
+     */
+    void
+    buildInnersDownTo(unsigned int deepest, SHAMapHash childHash, Decoy decoy)
+    {
+        for (auto depth = deepest + 1; depth-- > 0;)
+        {
+            // A key has only 64 nibbles, so selectBranch() at SHAMap::kLeafDepth would index one
+            // byte past the end of the 32-byte key. A fabricated chain's pathKey is zero, so
+            // branch 0 is the position such a node claims anyway.
+            auto const branch =
+                depth == SHAMap::kLeafDepth ? 0u : selectBranch(idAt(depth), pathKey);
+
+            Serializer s;
+            s.addBitString(childHash.asUInt256());
+            s.add8(static_cast<unsigned char>(branch));
+
+            if (decoy == Decoy::Yes)
+            {
+                // The decoy sits at branch 1, which only stays free of the real child because
+                // every caller that passes Decoy::Yes leaves pathKey at its default of zero. If a
+                // caller ever combined a non-zero pathKey with a decoy, the compressed-inner-node
+                // parser would silently let the decoy overwrite the real child's hash instead of
+                // rejecting the duplicate branch, so guard the assumption rather than rely on it.
+                if (branch == 1)
+                    Throw<std::logic_error>("DeepChain: decoy branch collides with real child");
+
+                // Derived from the depth so it differs per level - each posts its own read - and
+                // is deterministic and cannot collide with a real node hash.
+                uint256 decoyHash;
+                decoyHash.begin()[0] = 0xDE;
+                decoyHash.begin()[1] = 0xC0;
+                decoyHash.begin()[2] = static_cast<unsigned char>(depth);
+                s.addBitString(decoyHash);
+                s.add8(1);  // the unresolvable decoy sits at branch 1
+            }
+
+            s.add8(kWireTypeCompressedInner);
+
+            auto node = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData()));
+            childHash = node->getHash();
+            nodes[depth] = std::move(node);
+        }
+
+        rootHash = childHash;
+    }
+};
+
+}  // namespace xrpl::tests

src/tests/libxrpl/shamap/SHAMapAddNode.cpp

@@ -0,0 +1,79 @@
+#include <xrpl/shamap/SHAMapAddNode.h>
+
+#include <gtest/gtest.h>
+
+using namespace xrpl;
+
+// get() is a log format rather than an API, so it is pinned here, once, instead of at every site
+// that has a verdict to check. Those check the tally through getGood()/getBad()/getDuplicate()
+// (see tallyIs()) or through isGood()/isUseful()/isInvalid(), which say the same thing without
+// depending on the wording.
+TEST(SHAMapAddNode, getNamesEveryNonEmptyCount)
+{
+    EXPECT_EQ(SHAMapAddNode{}.get(), "no nodes processed");
+    EXPECT_EQ(SHAMapAddNode::useful().get(), "good:1");
+    EXPECT_EQ(SHAMapAddNode::invalid().get(), "bad:1");
+    EXPECT_EQ(SHAMapAddNode::duplicate().get(), "dupe:1");
+
+    // Several of a kind are counted, and the counts are joined in a fixed order with a single
+    // space, whichever order they were recorded in.
+    SHAMapAddNode san;
+    san.incInvalid();
+    san.incUseful();
+    san.incUseful();
+    san.incDuplicate();
+    EXPECT_EQ(san.get(), "good:2 bad:1 dupe:1");
+
+    san.reset();
+    EXPECT_EQ(san.get(), "no nodes processed");
+}
+
+// The three counts the tests assert on, and the verdicts derived from them, so a tally check and
+// the log line cannot drift apart.
+TEST(SHAMapAddNode, countsAndVerdictsAgree)
+{
+    SHAMapAddNode san;
+    EXPECT_EQ(san.getGood(), 0);
+    EXPECT_EQ(san.getBad(), 0);
+    EXPECT_EQ(san.getDuplicate(), 0);
+    EXPECT_FALSE(san.isInvalid());
+    EXPECT_FALSE(san.isUseful());
+
+    // Good counts what was hooked in, and useful is that count being non-zero.
+    san.incUseful();
+    EXPECT_EQ(san.getGood(), 1);
+    EXPECT_TRUE(san.isUseful());
+    EXPECT_TRUE(san.isGood());
+
+    // A duplicate counts towards good without needing to be useful itself: isUseful() here still
+    // reflects the incUseful() above, not this increment.
+    san.incDuplicate();
+    EXPECT_EQ(san.getDuplicate(), 1);
+    EXPECT_FALSE(san.isInvalid());
+    EXPECT_TRUE(san.isGood());
+
+    // Bad is counted, not merely flagged: a batch that carries on past a rejected node reports one
+    // per node, so a test can tell "stopped on the first" from "rejected several".
+    san.incInvalid();
+    EXPECT_EQ(san.getBad(), 1);
+    EXPECT_TRUE(san.isInvalid());
+    EXPECT_TRUE(san.isGood()) << "one bad node among two accepted ones is still a good batch";
+
+    san.incInvalid();
+    san.incInvalid();
+    EXPECT_EQ(san.getGood(), 1);
+    EXPECT_EQ(san.getBad(), 3);
+    EXPECT_EQ(san.getDuplicate(), 1);
+    EXPECT_FALSE(san.isGood()) << "more bad nodes than accepted ones is not";
+
+    // Adding one verdict to another sums every count, which is how a batch's verdict is built up
+    // one node at a time.
+    SHAMapAddNode total;
+    total += SHAMapAddNode::useful();
+    total += SHAMapAddNode::invalid();
+    total += SHAMapAddNode::invalid();
+    total += SHAMapAddNode::duplicate();
+    EXPECT_EQ(total.getGood(), 1);
+    EXPECT_EQ(total.getBad(), 2);
+    EXPECT_EQ(total.getDuplicate(), 1);
+}

src/tests/libxrpl/shamap/SHAMapSync.cpp

@@ -1,30 +1,83 @@
+#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/basics/random.h>
+#include <xrpl/beast/hash/uhash.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/xor_shift_engine.h>
+#include <xrpl/ledger/Ledger.h>
+#include <xrpl/protocol/Fees.h>
+#include <xrpl/protocol/LedgerHeader.h>
+#include <xrpl/protocol/Rules.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/shamap/SHAMap.h>
+#include <xrpl/shamap/SHAMapAddNode.h>
 #include <xrpl/shamap/SHAMapItem.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapSyncFilter.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <boost/smart_ptr/intrusive_ptr.hpp>
 
 #include <gtest/gtest.h>
 #include <helpers/TestSink.h>
+#include <shamap/DeepChain.h>
 #include <shamap/common.h>
 
+#include <atomic>
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
 #include <list>
+#include <map>
+#include <optional>
+#include <thread>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
 namespace xrpl::tests {
 
+// The cap on how many nodes a walk reports, set well above what any test here expects.
+static constexpr int kMaxNodesPerRequest = 2048;
+
+/**
+ * Rules with no amendments enabled, which is all a ledger built here needs.
+ *
+ * @return The rules.
+ */
+[[nodiscard]] static Rules
+noAmendments()
+{
+    return Rules{std::unordered_set<uint256, beast::Uhash<>>{}};
+}
+
+/**
+ * Whether a verdict carries exactly the given counts.
+ *
+ * The counts rather than get(): that string is a log format, not an API. It is
+ * pinned once, in the SHAMapAddNode tests, and read here only to describe a
+ * failure.
+ *
+ * @param san The verdict to check.
+ * @param good How many nodes the batch should have hooked in.
+ * @param bad How many it should have rejected.
+ * @param duplicate How many it should have already held.
+ * @return Whether the verdict matches, naming the actual tally if it does not.
+ */
+[[nodiscard]] static ::testing::AssertionResult
+tallyIs(SHAMapAddNode const& san, int good, int bad, int duplicate)
+{
+    if (san.getGood() == good && san.getBad() == bad && san.getDuplicate() == duplicate)
+        return ::testing::AssertionSuccess();
+
+    return ::testing::AssertionFailure() << "tally is " << san.get() << ", expected good:" << good
+                                         << " bad:" << bad << " dupe:" << duplicate;
+}
+
 class SHAMapSyncTest : public ::testing::Test
 {
 protected:
@@ -81,8 +134,824 @@ protected:
 
         return true;
     }
+
+    /**
+     * A single-child inner node in wire form, pointing at the given child.
+     *
+     * The chain a caller builds from these is put together bottom-up,
+     * since each node's hash covers the child hash below it.
+     *
+     * @param childHash What the node points at, on branch 0.
+     * @return The deserialized node.
+     */
+    [[nodiscard]] static SHAMapTreeNodePtr
+    makeInnerNode(SHAMapHash const& childHash)
+    {
+        Serializer s;
+        s.addBitString(childHash.asUInt256());
+        s.add8(0);  // the chain continues at branch 0
+        s.add8(kWireTypeCompressedInner);
+
+        return SHAMapTreeNode::makeFromWire(makeSlice(s.peekData()));
+    }
+
+    /**
+     * A sync filter that records every node it is told about, and serves back
+     * only the ones it was explicitly asked to hold.
+     *
+     * Serving is opt-in: the sync path consults the filter before deciding
+     * a node is missing, so a filter that served everything it had seen
+     * would resolve the node a test is about to offer.
+     */
+    class RecordingFilter : public SHAMapSyncFilter
+    {
+    public:
+        // What one gotNode() call was told, in the order the calls arrived.
+        struct Report
+        {
+            bool fromFilter;
+            SHAMapHash hash;
+            std::uint32_t ledgerSeq;
+        };
+
+        void
+        gotNode(
+            bool fromFilter,
+            SHAMapHash const& hash,
+            std::uint32_t ledgerSeq,
+            Blob&&,  // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved)
+            SHAMapNodeType) const override
+        {
+            reports_.push_back({.fromFilter = fromFilter, .hash = hash, .ledgerSeq = ledgerSeq});
+        }
+
+        [[nodiscard]] std::optional<Blob>
+        getNode(SHAMapHash const& hash) const override
+        {
+            if (auto const it = served_.find(hash); it != served_.end())
+                return it->second;
+            return std::nullopt;
+        }
+
+        /**
+         * Offer a node back to the map, the way a fetch pack would.
+         *
+         * @param node The node to serve, keyed by its own hash.
+         */
+        void
+        serve(SHAMapTreeNodePtr const& node)
+        {
+            Serializer s;
+            node->serializeWithPrefix(s);
+            served_.emplace(node->getHash(), s.modData());
+        }
+
+        [[nodiscard]] std::vector<Report> const&
+        reports() const
+        {
+            return reports_;
+        }
+
+    private:
+        // Mutable because the whole interface is const: a filter is handed to the map by
+        // const pointer, so recording has to happen through one.
+        mutable std::vector<Report> reports_;
+        std::map<SHAMapHash, Blob> served_;
+    };
+
+    /**
+     * A sync filter that serves a range of a DeepChain's nodes, by hash.
+     *
+     * Stands in for a fetch pack, which is checked against each node's own
+     * hash and never structurally, so a walk can resolve nodes locally
+     * without any of them passing through addKnownNode(). Anything outside
+     * the range - including a decoy child - looks unavailable.
+     */
+    class ChainFilter : public SHAMapSyncFilter
+    {
+    public:
+        /**
+         * @param chain The chain whose nodes to serve.
+         * @param maxDepth The deepest node to serve.
+         * @param minDepth The shallowest node to serve.
+         */
+        explicit ChainFilter(
+            DeepChain const& chain,
+            unsigned int maxDepth = SHAMap::kLeafDepth,
+            unsigned int minDepth = 0)
+        {
+            for (auto depth = minDepth; depth <= maxDepth; ++depth)
+                nodes_.emplace(chain.nodeAt(depth)->getHash(), chain.prefixedNodeAt(depth));
+        }
+
+        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;
+        }
+
+    private:
+        std::map<SHAMapHash, Blob> nodes_;
+    };
+
+    /**
+     * A root inner node with all 16 branches occupied and not one of them
+     * resolvable.
+     *
+     * A walk of a backed map posts an asynchronous read for every branch in a
+     * single pass, so the nodestore reader threads run finishFetch() for the
+     * same map at the same time.
+     */
+    struct WideRoot
+    {
+        SHAMapTreeNodePtr node;
+        SHAMapHash hash;
+
+        WideRoot()
+        {
+            Serializer s;
+
+            for (auto branch = 0u; branch < SHAMap::kBranchFactor; ++branch)
+            {
+                // Derived from the branch so each posts its own read, and deterministic so it
+                // cannot collide with a real node hash.
+                uint256 childHash;
+                childHash.begin()[0] = 0xFA;
+                childHash.begin()[1] = 0xB1;
+                childHash.begin()[2] = static_cast<unsigned char>(branch);
+                s.addBitString(childHash);
+            }
+
+            s.add8(kWireTypeInner);
+
+            node = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData()));
+            hash = node->getHash();
+        }
+    };
 };
 
+// An inner node at kLeafDepth, where only leaves can live, leaves the map provably invalid. It
+// must be reported as bad data, and the map must then refuse to become immutable.
+TEST_F(SHAMapSyncTest, innerNodeAtLeafDepth)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(chain.fill(map));
+    ASSERT_TRUE(map.isValid());
+
+    auto const result = chain.addOffendingNode(map);
+
+    EXPECT_TRUE(tallyIs(result, 0, 1, 0));
+    EXPECT_FALSE(result.isGood());
+    EXPECT_FALSE(map.isValid());
+
+    // Invalid is terminal, so the map can no longer be made immutable and therefore cannot be
+    // persisted.
+    EXPECT_FALSE(map.setImmutable());
+}
+
+// A node that cannot be hooked anywhere is bad data, so the batch counts no progress, but the map
+// itself is unharmed and another sender can still complete it. All three ways of getting there are
+// covered, since they share that verdict.
+TEST_F(SHAMapSyncTest, nodeThatCannotBeHookedIsBadData)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
+
+    // nodeAt(1) is the node the root is missing and its hash matches, but we claim depth 2.
+    auto const wrongDepth = map.addKnownNode(SHAMapNodeID{2, uint256{}}, chain.nodeAt(1), nullptr);
+
+    EXPECT_TRUE(tallyIs(wrongDepth, 0, 1, 0));
+    EXPECT_FALSE(wrongDepth.isUseful());
+
+    // The chain sits on branch 0 at every depth, so a node claiming a position on branch 1 asks the
+    // descent to follow a branch the root does not have.
+    uint256 otherBranch;
+    otherBranch.begin()[0] = 0x10;
+    auto const emptyBranch =
+        map.addKnownNode(SHAMapNodeID{1, otherBranch}, chain.nodeAt(1), nullptr);
+
+    EXPECT_TRUE(tallyIs(emptyBranch, 0, 1, 0));
+
+    // The right position this time, but the data hashes to something other than the child the root
+    // says belongs there.
+    auto const corrupt = map.addKnownNode(SHAMapNodeID{1, uint256{}}, chain.nodeAt(2), nullptr);
+
+    EXPECT_TRUE(tallyIs(corrupt, 0, 1, 0));
+
+    // Nothing was hooked in and nothing was proven about the tree, so the map stays usable.
+    EXPECT_TRUE(map.isValid());
+}
+
+// The verdict must not be bypassable through the full-below cache. That cache is keyed by node hash
+// and shared by every map of a family, and a hash covers a node's children but not its depth, so an
+// earlier walk can mark the same subtree hash complete at one depth while this map reaches it at
+// kLeafDepth, with no collision involved. A hit there would report a duplicate and return before
+// the verdict, leaving what every later caller relies on dependent on what an unrelated map cached.
+TEST_F(SHAMapSyncTest, mapInvalidatingNodeIsJudgedOnCacheHit)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(chain.fill(map));
+    ASSERT_TRUE(map.isValid());
+
+    // The cache is keyed on the hash of the child being considered, so at the kLeafDepth boundary
+    // that is the offending node itself, and a hit is what would skip the whole branch. This test
+    // seeds that entry, unlike the cases above, which never touch the cache and so always miss.
+    f.getFullBelowCache()->insert(chain.nodeAt(SHAMap::kLeafDepth)->getHash().asUInt256());
+
+    auto const result = chain.addOffendingNode(map);
+
+    EXPECT_TRUE(tallyIs(result, 0, 1, 0));
+    EXPECT_FALSE(result.isGood());
+    EXPECT_FALSE(map.isValid());
+    EXPECT_FALSE(map.setImmutable());
+}
+
+// An invalid tx map must also stop the enclosing ledger from being marked immutable, since an
+// immutable ledger is treated as persistable.
+TEST_F(SHAMapSyncTest, invalidTxMapBlocksImmutableLedger)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    Ledger ledger{1, NetClock::time_point{}, noAmendments(), Fees{}, f};
+    ASSERT_FALSE(ledger.isImmutable());
+
+    ledger.txMap().setSynching();
+    ASSERT_TRUE(chain.fill(ledger.txMap()));
+
+    auto const result = chain.addOffendingNode(ledger.txMap());
+    ASSERT_TRUE(tallyIs(result, 0, 1, 0));
+    ASSERT_FALSE(ledger.txMap().isValid());
+
+    // The state map is untouched, so only the transaction map can be refusing.
+    ASSERT_TRUE(ledger.stateMap().isValid());
+
+    EXPECT_FALSE(ledger.setImmutable());
+    EXPECT_FALSE(ledger.isImmutable());
+}
+
+// The same for the state map. Ledger::setImmutable() tests both maps in one expression, so this
+// covers the second operand: a sound transaction map must not let an invalid state map through.
+TEST_F(SHAMapSyncTest, invalidStateMapBlocksImmutableLedger)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    Ledger ledger{1, NetClock::time_point{}, noAmendments(), Fees{}, f};
+    ASSERT_FALSE(ledger.isImmutable());
+
+    ledger.stateMap().setSynching();
+    ASSERT_TRUE(chain.fill(ledger.stateMap()));
+
+    auto const result = chain.addOffendingNode(ledger.stateMap());
+    ASSERT_TRUE(tallyIs(result, 0, 1, 0));
+    ASSERT_FALSE(ledger.stateMap().isValid());
+
+    // The transaction map is untouched, so only the state map can be refusing.
+    ASSERT_TRUE(ledger.txMap().isValid());
+
+    EXPECT_FALSE(ledger.setImmutable());
+    EXPECT_FALSE(ledger.isImmutable());
+}
+
+// A refusal must leave the header exactly as it was. setImmutable() derives the map hashes from the
+// maps and then the ledger hash from the header, so a check made only after those writes would
+// relabel the ledger on its way to failing, leaving a header that no longer describes what it was
+// built from. The later re-test cannot stand in for the early one for that reason: by the time it
+// runs, the header has been written.
+TEST_F(SHAMapSyncTest, refusedSettleLeavesTheHeaderAlone)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    // Not the header constructor: this one derives its map hashes, which is what must not happen.
+    Ledger ledger{1, NetClock::time_point{}, noAmendments(), Fees{}, f};
+    ASSERT_FALSE(ledger.isImmutable());
+    ASSERT_TRUE(ledger.header().txHash.isZero());
+    ASSERT_TRUE(ledger.header().accountHash.isZero());
+    auto const hashBefore = ledger.header().hash;
+
+    // A transaction map that hashes to something, so a derived header hash would differ from the
+    // one the ledger has now.
+    ASSERT_TRUE(ledger.txMap().addItem(SHAMapNodeType::TnTransactionNm, makeRandomAS()));
+    ASSERT_TRUE(ledger.txMap().getHash().isNonZero());
+
+    // And a state map the chain abandons, so settling has to refuse.
+    ledger.stateMap().setSynching();
+    ASSERT_TRUE(chain.fill(ledger.stateMap()));
+    ASSERT_TRUE(chain.addOffendingNode(ledger.stateMap()).isInvalid());
+    ASSERT_FALSE(ledger.stateMap().isValid());
+
+    EXPECT_FALSE(ledger.setImmutable());
+
+    // Nothing was written: not the map hashes, not the ledger hash, and not the flag.
+    EXPECT_FALSE(ledger.isImmutable());
+    EXPECT_TRUE(ledger.header().txHash.isZero());
+    EXPECT_TRUE(ledger.header().accountHash.isZero());
+    EXPECT_EQ(ledger.header().hash, hashBefore);
+}
+
+// A ledger built from a header must not claim to be immutable before setImmutable() has found both
+// maps sound: they start out Synching and are filled in afterwards, and LedgerHistory::insert() and
+// LedgerReplayMsgHandler both gate on that claim to catch exactly that case.
+TEST_F(SHAMapSyncTest, ledgerFromHeaderIsNotImmutableUntilSettled)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    LedgerHeader header;
+    header.seq = 2;
+    header.txHash = chain.rootHash.asUInt256();
+    header.hash = calculateLedgerHash(header);
+
+    Ledger ledger{header, noAmendments(), f};
+
+    // Both maps are still syncing, so nothing about this ledger is settled.
+    EXPECT_TRUE(ledger.txMap().isSynching());
+    EXPECT_TRUE(ledger.stateMap().isSynching());
+    EXPECT_FALSE(ledger.isImmutable());
+
+    ASSERT_TRUE(chain.fill(ledger.txMap()));
+    ASSERT_TRUE(chain.addOffendingNode(ledger.txMap()).isInvalid());
+    ASSERT_FALSE(ledger.txMap().isValid());
+
+    EXPECT_FALSE(ledger.setImmutable());
+    EXPECT_FALSE(ledger.isImmutable());
+}
+
+// The header's own map hashes are what the maps are synced against, so settling must not adopt
+// whatever the maps happen to hash to. The transaction map is left empty while the header names a
+// chain root, so deriving the hashes would rewrite that field and the ledger hash both.
+TEST_F(SHAMapSyncTest, ledgerFromHeaderKeepsTheMapHashesItWasGiven)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    LedgerHeader header;
+    header.seq = 2;
+    header.txHash = chain.rootHash.asUInt256();
+    header.hash = calculateLedgerHash(header);
+    auto const verifiedHash = header.hash;
+
+    Ledger ledger{header, noAmendments(), f};
+    ASSERT_FALSE(ledger.isImmutable());
+
+    // Nothing was ever synced, so the map is empty and hashes to zero. Both maps are still valid,
+    // so settling succeeds.
+    ASSERT_TRUE(ledger.txMap().getHash().isZero());
+    ASSERT_TRUE(ledger.setImmutable());
+    EXPECT_TRUE(ledger.isImmutable());
+
+    EXPECT_EQ(ledger.header().txHash, chain.rootHash.asUInt256());
+    EXPECT_EQ(ledger.header().hash, verifiedHash);
+}
+
+// Invalid is terminal: setImmutable() and clearSynching() offer no way back out of it, however
+// many times they are called. setSynching() is left alone, since it is unreachable on an invalid
+// map today and says so with an UNREACHABLE that aborts under -Dassert.
+TEST_F(SHAMapSyncTest, invalidStateIsTerminal)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(chain.fill(map));
+    ASSERT_TRUE(chain.addOffendingNode(map).isInvalid());
+    ASSERT_FALSE(map.isValid());
+
+    // Repeated attempts must each fail, and must not leave the map reporting a valid state.
+    for (auto attempt = 0; attempt < 3; ++attempt)
+    {
+        EXPECT_FALSE(map.setImmutable()) << "attempt " << attempt;
+        EXPECT_FALSE(map.isValid()) << "attempt " << attempt;
+    }
+
+    // Nor does clearSynching(), which keeps an abandoned map from being moved back to Modifying and
+    // passing isValid() again. It refuses rather than aborting, since a concurrent walk can
+    // invalidate a map between a caller's own check and this call.
+    for (auto attempt = 0; attempt < 3; ++attempt)
+    {
+        map.clearSynching();
+        EXPECT_FALSE(map.isValid()) << "attempt " << attempt;
+    }
+
+    // An invalid map is not synching either, so nothing reads it as mid-acquisition.
+    EXPECT_FALSE(map.isSynching());
+}
+
+// A snapshot shares the source map's root, so it inherits whatever the source was found to be.
+// Invalid carries over, since promoting it would hand back a map that passes isValid().
+TEST_F(SHAMapSyncTest, snapshotOfInvalidMapStaysInvalid)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(chain.fill(map));
+
+    auto const result = chain.addOffendingNode(map);
+    ASSERT_TRUE(tallyIs(result, 0, 1, 0));
+    ASSERT_FALSE(map.isValid());
+
+    // Both flavors: the immutable snapshot is the one that would be persisted,
+    // and the mutable one would otherwise launder the state back to Modifying.
+    for (bool const isMutable : {false, true})
+    {
+        auto const snapshot = map.snapShot(isMutable);
+        ASSERT_TRUE(snapshot != nullptr);
+        EXPECT_FALSE(snapshot->isValid()) << "isMutable " << isMutable;
+        EXPECT_FALSE(snapshot->setImmutable()) << "isMutable " << isMutable;
+    }
+
+    // A snapshot of a sound map is unaffected.
+    SHAMap valid{SHAMapType::FREE, f};
+    valid.addItem(SHAMapNodeType::TnAccountState, makeRandomAS());
+    EXPECT_TRUE(valid.snapShot(false)->isValid());
+    EXPECT_TRUE(valid.snapShot(true)->isValid());
+}
+
+// getMissingNodes() refuses an invalid map outright, before ever consulting a filter. The
+// offending node was never hooked into the tree by addKnownNode(), but without this guard a fetch
+// pack could still resolve it and let a walk reach the same verdict itself (see
+// getMissingNodesRejectsInnerNodeAtLeafDepth).
+TEST_F(SHAMapSyncTest, getMissingNodesRefusesInvalidMap)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    // Unbacked, like TransactionAcquire's map, so the walk resolves synchronously.
+    map.setUnbacked();
+    map.setSynching();
+
+    ASSERT_TRUE(chain.fill(map));
+
+    auto const offendingResult = chain.addOffendingNode(map);
+    ASSERT_TRUE(tallyIs(offendingResult, 0, 1, 0));
+    ASSERT_FALSE(map.isValid());
+
+    // Only the node the map rejected, offered back the way a fetch pack would.
+    ChainFilter const filter{chain, SHAMap::kLeafDepth, SHAMap::kLeafDepth};
+
+    EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty());
+    EXPECT_FALSE(map.isValid());
+    EXPECT_FALSE(map.setImmutable());
+}
+
+// A map can reach kLeafDepth without addKnownNode() ever being involved, since a fetch pack is
+// not checked structurally and the walk resolves every level locally. The map stays Modifying
+// throughout, so the isValid() guard never fires and the walk must reach the verdict itself.
+TEST_F(SHAMapSyncTest, getMissingNodesRejectsInnerNodeAtLeafDepth)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    // Unbacked so the walk resolves each level synchronously through the filter.
+    map.setUnbacked();
+    map.setSynching();
+
+    // Only the root goes in through the sync path; everything below comes from the filter, so
+    // nothing invalidates the map before the walk.
+    ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
+    ASSERT_TRUE(map.isValid());
+
+    ChainFilter const filter{chain};
+
+    EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty());
+
+    // The walk reaches the same verdict addKnownNode() does, so the map is now unusable and
+    // unpersistable.
+    EXPECT_FALSE(map.isValid());
+    EXPECT_FALSE(map.setImmutable());
+
+    // An empty result means "satisfied" for a valid map and clears the synching flag. Returning as
+    // soon as the map is abandoned keeps that call out of reach, and the map stays invalid across a
+    // second walk.
+    EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty());
+    EXPECT_FALSE(map.isValid());
+}
+
+// The depth guard must not be bypassable through the full-below cache. A node's hash covers its
+// child hashes but not its depth, so an earlier walk can mark the same subtree hash complete at one
+// depth and this one reach it at kLeafDepth, with no collision involved. This is the backed-map
+// case, which is what InboundLedger uses.
+TEST_F(SHAMapSyncTest, getMissingNodesRejectsInnerNodeAtLeafDepthOnCacheHit)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    // Backed, unlike the cases above, so the full-below cache is consulted at all.
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
+    ASSERT_TRUE(map.isValid());
+
+    // Mark the offending node itself as full below. The cache is keyed on the hash of the child
+    // being considered, so this is what the lookup at the kLeafDepth boundary asks about, and a hit
+    // is what would skip the whole branch - guard included.
+    f.getFullBelowCache()->insert(chain.nodeAt(SHAMap::kLeafDepth)->getHash().asUInt256());
+
+    ChainFilter const filter{chain};
+
+    EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty());
+    EXPECT_FALSE(map.isValid());
+    EXPECT_FALSE(map.setImmutable());
+}
+
+// Abandoning the walk must not abandon the reads it already posted. The MissingNodes block lives on
+// getMissingNodes()'s stack frame and every posted read holds a reference to it, so the verdict
+// breaks out of the descent but still falls through to the drain. Each level here has an
+// unresolvable second child, so reads are in flight when the verdict lands. The failure mode is a
+// use-after-free rather than a wrong answer, so it takes ASan to see.
+TEST_F(SHAMapSyncTest, getMissingNodesDrainsPostedReadsWhenInvalidated)
+{
+    TestNodeFamily f{j_};
+    auto const chain = DeepChain::withDecoys();
+
+    // Backed, so descendAsync() posts real asynchronous reads rather than resolving inline.
+    SHAMap map{SHAMapType::FREE, f};
+    map.setSynching();
+
+    ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
+    ASSERT_TRUE(map.isValid());
+
+    // Only the chain nodes are served, so the decoy at each level has to be read asynchronously.
+    ChainFilter const filter{chain};
+
+    // The walk descends the chain, posting a read per level for the decoy child, and marks the map
+    // invalid on reaching kLeafDepth. Returning empty is the visible part; draining first is the
+    // part only a sanitizer can see.
+    EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty());
+    EXPECT_FALSE(map.isValid());
+    EXPECT_FALSE(map.setImmutable());
+}
+
+// A walk that only meets legitimate depths must be left alone. Stopping one level short of
+// kLeafDepth leaves a deepest node whose child is genuinely missing, so the walk reports it and
+// the map stays valid.
+TEST_F(SHAMapSyncTest, getMissingNodesAcceptsInnerNodeAboveLeafDepth)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setUnbacked();
+    map.setSynching();
+
+    ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
+
+    // Everything except the node at kLeafDepth, which is the only one that would
+    // put the walk at a position no valid tree can occupy.
+    SHAMapHash const withheld = chain.nodeAt(SHAMap::kLeafDepth)->getHash();
+    ChainFilter const filter{chain, SHAMap::kLeafDepth - 1};
+
+    auto const missing = map.getMissingNodes(kMaxNodesPerRequest, &filter);
+
+    ASSERT_EQ(missing.size(), 1u);
+    EXPECT_EQ(missing[0].first.getDepth(), SHAMap::kLeafDepth);
+    EXPECT_EQ(missing[0].second, withheld.asUInt256());
+    EXPECT_TRUE(map.isValid());
+}
+
+// The clearSynching() call site in addRootNode() needs a leaf root, and so a zero root hash. An
+// invalid map always has an inner root with a non-zero hash, so the root is treated as a duplicate
+// and the flag stands.
+TEST_F(SHAMapSyncTest, addRootNodeLeavesInvalidMapInvalid)
+{
+    TestNodeFamily f{j_};
+    DeepChain const chain;
+
+    SHAMap map{SHAMapType::FREE, f};
+    map.setUnbacked();
+    map.setSynching();
+
+    ASSERT_TRUE(chain.fill(map));
+
+    auto const offendingResult = chain.addOffendingNode(map);
+    ASSERT_TRUE(tallyIs(offendingResult, 0, 1, 0));
+    ASSERT_FALSE(map.isValid());
+
+    // A duplicate: counted as good, but nothing new was taken from the peer.
+    auto const result = map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr);
+    EXPECT_TRUE(tallyIs(result, 0, 0, 1));
+    EXPECT_TRUE(result.isGood());
+    EXPECT_FALSE(result.isUseful());
+
+    EXPECT_FALSE(map.isValid());
+    EXPECT_FALSE(map.setImmutable());
+}
+
+// The concurrent half of the same contract: a walk writing Invalid while another thread calls
+// setImmutable(). The verdict must win, and the map must end up unable to become immutable.
+//
+// What is deliberately not asserted is that a setImmutable() returning true implies a valid map
+// when it returns. Nothing offers that: the compare-exchange can succeed and the walk can then
+// write Invalid, all before the caller's next statement. The guarantee is only that trySetState()
+// never leaves Invalid, which is what the post-join expectations below check.
+//
+// Only meaningful under ThreadSanitizer, which observes the collision this creates but cannot force
+// it. Skipped at run time rather than compiled out, so every build still parses the body. Under
+// SANITIZERS=thread, making state_ a plain member is reported as a data race here and
+// setImmutable() then succeeds on an invalid map. The compare-exchange in trySetState() is not
+// covered: an atomic load-then-store leaves a window too narrow to hit.
+TEST_F(SHAMapSyncTest, invalidStateSurvivesConcurrentSetImmutable)
+{
+#ifndef XRPL_TSAN
+    GTEST_SKIP() << "Only meaningful under ThreadSanitizer";
+#endif
+
+    static constexpr auto kRounds = 200uz;
+
+    for (auto round = 0uz; round < kRounds; ++round)
+    {
+        TestNodeFamily f{j_};
+        DeepChain const chain;
+
+        SHAMap map{SHAMapType::FREE, f};
+        map.setUnbacked();
+        map.setSynching();
+
+        // Only the root goes in through the sync path, so nothing has judged the map yet; the walk
+        // below resolves the rest through the filter and reaches the verdict itself.
+        ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood());
+        ChainFilter const filter{chain};
+
+        // One thread walks and invalidates; the other keeps calling setImmutable(). Started as
+        // close together as a latch allows, so the two collide somewhere in the middle rather than
+        // serializing.
+        std::atomic<bool> go{false};
+
+        std::thread walker([&] {
+            while (!go.load(std::memory_order_acquire))
+                std::this_thread::yield();
+            map.getMissingNodes(kMaxNodesPerRequest, &filter);
+        });
+
+        std::thread setter([&] {
+            while (!go.load(std::memory_order_acquire))
+                std::this_thread::yield();
+            for (auto attempt = 0uz; attempt < 64uz; ++attempt)
+                static_cast<void>(map.setImmutable());
+        });
+
+        go.store(true, std::memory_order_release);
+        walker.join();
+        setter.join();
+
+        // The walk always reaches the verdict, so the map must end up invalid and unable to become
+        // immutable however the two threads interleaved.
+        EXPECT_FALSE(map.isValid()) << "round " << round;
+        EXPECT_FALSE(map.setImmutable()) << "round " << round;
+    }
+}
+
+// A map marked complete in the database withdraws that claim the first time a read misses, and
+// reports the miss once so the ledger can be re-acquired. Sixteen unresolvable branches are posted
+// in one pass, so with four reader threads the misses overlap and finishFetch() runs concurrently
+// for a single map. The one-report count below is too narrow a window for an ordinary run to
+// police; what confirms it is the absence of a data race under ThreadSanitizer.
+TEST_F(SHAMapSyncTest, fullFlagIsWithdrawnOnceByConcurrentReaders)
+{
+    static constexpr auto kRounds = 8uz;
+    static constexpr auto kReadThreads = 4;
+
+    for (auto round = 0uz; round < kRounds; ++round)
+    {
+        TestNodeFamily f{j_, kReadThreads};
+        WideRoot const root;
+
+        // Backed, so descendAsync() posts real asynchronous reads rather than resolving inline.
+        SHAMap map{SHAMapType::FREE, f};
+        map.setSynching();
+
+        ASSERT_TRUE(map.addRootNode(root.hash, root.node, nullptr).isGood());
+
+        // The claim the first miss has to withdraw.
+        map.setFull();
+
+        // No filter, so every branch has to be read from a database that does not hold it.
+        EXPECT_EQ(map.getMissingNodes(kMaxNodesPerRequest, nullptr).size(), SHAMap::kBranchFactor)
+            << "round " << round;
+
+        EXPECT_EQ(f.missingBySeqReports(), 1uz) << "round " << round;
+    }
+}
+
+// Every node the sync path hands to a filter carries the map's ledger sequence, which is the hint
+// the filter passes on to a nodestore keyed by hash. All three call sites are covered: a root taken
+// from a peer, a node taken from a peer, and a node the walk resolved out of the filter itself.
+TEST_F(SHAMapSyncTest, syncFilterIsToldTheLedgerSequence)
+{
+    static constexpr std::uint32_t kLedgerSeq = 7;
+
+    TestNodeFamily f{j_};
+
+    // A three-level chain, built bottom-up so each hash covers the one below it. The deepest node
+    // points at a child that is never supplied, so the walk always has something to ask for.
+    auto const deepest = makeInnerNode(SHAMapHash{uint256{1}});
+    auto const middle = makeInnerNode(deepest->getHash());
+    auto const root = makeInnerNode(middle->getHash());
+
+    // Unbacked, so a node the filter does not hold is missing rather than looked up in a database.
+    SHAMap map{SHAMapType::FREE, f};
+    map.setUnbacked();
+    map.setSynching();
+    map.setLedgerSeq(kLedgerSeq);
+
+    RecordingFilter filter;
+
+    ASSERT_TRUE(map.addRootNode(root->getHash(), root, &filter).isGood());
+    ASSERT_TRUE(map.addKnownNode(SHAMapNodeID{1, uint256{}}, middle, &filter).isUseful());
+
+    // Only now is the deepest node resolvable, so nothing above it came out of the filter.
+    filter.serve(deepest);
+    auto const missing = map.getMissingNodes(kMaxNodesPerRequest, &filter);
+
+    // The walk resolved the deepest node through the filter and then asked for its child.
+    ASSERT_EQ(missing.size(), 1u);
+    EXPECT_EQ(missing[0].second, uint256{1});
+
+    ASSERT_EQ(filter.reports().size(), 3u);
+
+    // The two nodes taken from a peer, which the filter is told about so it can store them.
+    EXPECT_FALSE(filter.reports()[0].fromFilter);
+    EXPECT_EQ(filter.reports()[0].hash, root->getHash());
+    EXPECT_FALSE(filter.reports()[1].fromFilter);
+    EXPECT_EQ(filter.reports()[1].hash, middle->getHash());
+
+    // The one the walk read back out of the filter, which is reported as such.
+    EXPECT_TRUE(filter.reports()[2].fromFilter);
+    EXPECT_EQ(filter.reports()[2].hash, deepest->getHash());
+
+    for (auto const& report : filter.reports())
+        EXPECT_EQ(report.ledgerSeq, kLedgerSeq) << "hash " << report.hash;
+}
+
+// Ledger::setFull() has to publish each map's ledger sequence alongside the flag that lets the
+// first nodestore miss report a gap, or the report names the zero a map starts with and the lookup
+// it asks for cannot resolve.
+TEST_F(SHAMapSyncTest, ledgerSetFullPublishesTheLedgerSequence)
+{
+    static constexpr std::uint32_t kLedgerSeq = 7;
+
+    TestNodeFamily f{j_};
+
+    LedgerHeader header;
+    header.seq = kLedgerSeq;
+    // Non-zero, so the map has a root to look for and the lookup can miss.
+    header.txHash = uint256{1};
+    header.hash = calculateLedgerHash(header);
+
+    Ledger ledger{header, noAmendments(), f};
+
+    // The constructor already looked for that root and did not find it, but nothing had claimed the
+    // map was complete yet, so there was no gap to report.
+    ASSERT_EQ(f.missingBySeqReports(), 0uz);
+
+    ledger.setFull();
+
+    // Still missing, and now the map has a claim to withdraw, so the gap is reported.
+    // TestNodeFamily throws where the real family would start re-acquiring, which finishFetch()
+    // logs and swallows.
+    EXPECT_FALSE(ledger.txMap().fetchRoot(SHAMapHash{header.txHash}, nullptr));
+
+    EXPECT_EQ(f.missingBySeqReports(), 1uz);
+    EXPECT_EQ(f.missingBySeqRefNum(), kLedgerSeq);
+}
+
 TEST_F(SHAMapSyncTest, sync)
 {
     TestNodeFamily f{j_}, f2{j_};
@@ -92,7 +961,6 @@ TEST_F(SHAMapSyncTest, sync)
     static constexpr auto kItemCount = 10000uz;
     static constexpr auto kInvariantInterval = 100uz;
     static constexpr auto kNodesToConfuse = 500uz;
-    static constexpr auto kMaxNodesPerRequest = 2048;
 
     for (auto i = 0uz; i < kItemCount; ++i)
     {
@@ -105,7 +973,7 @@ TEST_F(SHAMapSyncTest, sync)
     ASSERT_TRUE(confuseMap(source, kNodesToConfuse));
     source.invariants();
 
-    source.setImmutable();
+    ASSERT_TRUE(source.setImmutable());
 
     std::size_t count = 0;
     source.visitLeaves([&count]([[maybe_unused]] auto const& item) { ++count; });

src/tests/libxrpl/shamap/common.h

@@ -14,7 +14,9 @@
 #include <xrpl/shamap/FullBelowCache.h>
 #include <xrpl/shamap/TreeNodeCache.h>
 
+#include <atomic>
 #include <chrono>
+#include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <stdexcept>
@@ -34,8 +36,18 @@ private:
 
     beast::Journal const j_;
 
+    // Written from whichever nodestore reader thread reports the miss, so read back atomically.
+    std::atomic<std::size_t> missingBySeqReports_ = 0;
+    std::atomic<std::uint32_t> missingBySeqRefNum_ = 0;
+
 public:
-    TestNodeFamily(beast::Journal j)
+    /**
+     * @param j The journal to log through.
+     * @param readThreads How many nodestore reader threads to run asynchronous
+     *        fetches on. More than one is needed only by a test that wants
+     *        several reads to complete on different threads at once.
+     */
+    explicit TestNodeFamily(beast::Journal j, int readThreads = 1)
         : fbCache_(std::make_shared<FullBelowCache>("App family full below cache", clock_, j))
         , tnCache_(
               std::make_shared<TreeNodeCache>(
@@ -50,7 +62,7 @@ public:
         testSection.set(Keys::kType, "memory");
         testSection.set(Keys::kPath, "SHAMap_test");
         db_ = node_store::Manager::instance().makeDatabase(
-            megabytes(4), scheduler_, 1, testSection, j);
+            megabytes(4), scheduler_, readThreads, testSection, j);
     }
 
     node_store::Database&
@@ -90,14 +102,29 @@ public:
         tnCache_->sweep();
     }
 
+    /**
+     * Record the report and throw, standing in for Family's real acquisition
+     * machinery.
+     *
+     * @param refNum Sequence of the ledger with the missing node, recorded so a
+     *        test can check which sequence the map published.
+     * @param nodeHash Hash of the missing node. Unused.
+     */
     void
-    missingNodeAcquireBySeq(
-        [[maybe_unused]] std::uint32_t refNum,
-        [[maybe_unused]] uint256 const& nodeHash) override
+    missingNodeAcquireBySeq(std::uint32_t refNum, [[maybe_unused]] uint256 const& nodeHash) override
     {
+        missingBySeqRefNum_.store(refNum, std::memory_order_release);
+        ++missingBySeqReports_;
         Throw<std::runtime_error>("missing node");
     }
 
+    /**
+     * Throw, standing in for Family's real acquisition machinery. Uncounted, as
+     * no test in this suite drives this path.
+     *
+     * @param refHash Hash of the ledger with the missing node. Unused.
+     * @param refNum Sequence of the ledger with the missing node. Unused.
+     */
     void
     missingNodeAcquireByHash(
         [[maybe_unused]] uint256 const& refHash,
@@ -106,6 +133,31 @@ public:
         Throw<std::runtime_error>("missing node");
     }
 
+    /**
+     * How many times a map of this family has withdrawn its claim of being
+     * complete in the database. Counted per family, so a test that wants the
+     * count for one map has to give that map a family of its own.
+     *
+     * @return The number of missingNodeAcquireBySeq() calls so far.
+     */
+    [[nodiscard]] std::size_t
+    missingBySeqReports() const
+    {
+        return missingBySeqReports_.load(std::memory_order_acquire);
+    }
+
+    /**
+     * The ledger sequence the most recent such report named, which is the hint
+     * the map published for the nodestore lookup that has to resolve the gap.
+     *
+     * @return The sequence, or zero if nothing has been reported yet.
+     */
+    [[nodiscard]] std::uint32_t
+    missingBySeqRefNum() const
+    {
+        return missingBySeqRefNum_.load(std::memory_order_acquire);
+    }
+
     void
     reset() override
     {

src/xrpld/app/ledger/ConsensusTransSetSF.cpp

@@ -42,7 +42,7 @@ ConsensusTransSetSF::gotNode(
 
     nodeCache_.insert(nodeHash, nodeData);
 
-    if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() > 16))
+    if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() >= kMinTxNodeBytesToParse))
     {
         // this is a transaction, and we didn't have it
         JLOG(j_.debug()) << "Node on our acquiring TX set is TXN we may not have";

src/xrpld/app/ledger/ConsensusTransSetSF.h

@@ -9,6 +9,7 @@
 #include <xrpl/shamap/SHAMapSyncFilter.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
+#include <cstddef>
 #include <cstdint>
 #include <optional>
 
@@ -24,6 +25,18 @@ class ConsensusTransSetSF : public SHAMapSyncFilter
 public:
     using NodeCache = TaggedCache<SHAMapHash, Blob>;
 
+    /**
+     * The size a node's hash-prefixed wire data must reach before gotNode()
+     * tries to parse and resubmit it as a transaction.
+     *
+     * A threshold rather than a derived bound: the smallest a hash-prefixed
+     * SHAMap leaf can be is the 4-byte HashPrefix plus kMinShaMapItemBytes, and
+     * nothing that size is a signed transaction either. The extra byte is the
+     * long-standing threshold this check has always used, kept as it was.
+     */
+    static constexpr std::size_t kMinTxNodeBytesToParse =
+        sizeof(std::uint32_t) + kMinShaMapItemBytes + 1;
+
     ConsensusTransSetSF(Application& app, NodeCache& nodeCache);
 
     // Note that the nodeData is overwritten by this call

src/xrpld/app/ledger/InboundLedger.h

@@ -30,9 +30,9 @@
 namespace xrpl {
 
 // A ledger we are trying to acquire
-class InboundLedger final : public TimeoutCounter,
-                            public std::enable_shared_from_this<InboundLedger>,
-                            public CountedObject<InboundLedger>
+class InboundLedger : public TimeoutCounter,
+                      public std::enable_shared_from_this<InboundLedger>,
+                      public CountedObject<InboundLedger>
 {
 public:
     using clock_type = beast::AbstractClock<std::chrono::steady_clock>;
@@ -44,13 +44,33 @@ public:
         CONSENSUS  // We believe the consensus round requires this ledger
     };
 
+    /**
+     * How long to wait between retries, and so how long each timeout counted
+     * against the acquisition takes. Long, since a ledger is worth chasing for
+     * far longer than a consensus round.
+     */
+    static constexpr std::chrono::milliseconds kRetryInterval{3000};
+
+    /**
+     * @param app The application to run in.
+     * @param hash The ledger to acquire.
+     * @param seq Its sequence, or zero if not known yet.
+     * @param reason Why it is being acquired.
+     * @param clock The clock touch() records against.
+     * @param peerSet Which peers to ask, and how to reach them.
+     * @param retryInterval How long to wait between retries. Defaulted in
+     *        production; InboundLedger_test passes a short one so a whole
+     *        timeout chain runs in a fraction of the time. TimeoutCounter
+     *        requires more than 10ms.
+     */
     InboundLedger(
         Application& app,
         uint256 const& hash,
         std::uint32_t seq,
         Reason reason,
-        clock_type&,
-        std::unique_ptr<PeerSet> peerSet);
+        clock_type& clock,
+        std::unique_ptr<PeerSet> peerSet,
+        std::chrono::milliseconds retryInterval = kRetryInterval);
 
     ~InboundLedger() override;
 
@@ -59,7 +79,13 @@ public:
     update(std::uint32_t seq);
 
     /**
-     * Returns true if we got all the data.
+     * Whether the acquisition succeeded and its ledger has been settled.
+     *
+     * Not merely "we got all the data": done() makes the ledger immutable
+     * before setting this, so a caller that sees it may use the ledger
+     * without checking anything else about it.
+     *
+     * @return Whether the ledger is complete and settled.
      */
     bool
     isComplete() const
@@ -119,15 +145,40 @@ public:
         return lastAction_;
     }
 
-private:
+protected:
+    // Kept protected, with the two entry points naming it, so a test subclass (see
+    // InboundLedger_test) can drive an acquisition the way the timer chain does, without routing
+    // through the JobQueue. Production callers reach an acquisition through InboundLedgers.
+
+    // Why trigger() is being run, which decides how deep a request goes and whether the
+    // aggressive-retry branch is eligible.
     enum class TriggerReason { Added, Reply, Timeout };
 
+    /**
+     * Ask for more nodes, or judge what has been collected.
+     *
+     * @param peer The peer to ask, or nullptr to ask everyone being tracked.
+     * @param reason Why the acquisition is being triggered.
+     */
+    void
+    trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason);
+
+    /**
+     * Settle the acquisition, publish its outcome, and signal whatever is
+     * waiting on it. Runs at most once.
+     *
+     * Settling comes first: isComplete() is read without mtx_, so an
+     * outcome published before the ledger is immutable could be acted on
+     * while the ledger is still mutable. Call under mtx_, which the flags
+     * written here require.
+     */
+    void
+    done();
+
+private:
     void
     filterNodes(std::vector<std::pair<SHAMapNodeID, uint256>>& nodes, TriggerReason reason);
 
-    void
-    trigger(std::shared_ptr<Peer> const&, TriggerReason);
-
     std::vector<neededHash_t>
     getNeededHashes();
 
@@ -137,11 +188,23 @@ private:
     void
     tryDB(node_store::Database& srcDB);
 
-    void
-    done();
+    /**
+     * Whether either map of the ledger being acquired has been found
+     * invalid.
+     *
+     * A walk returns a bare list of hashes, so an empty result does not
+     * tell a satisfied map from an abandoned one; callers that read
+     * emptiness as "nothing left to fetch" must ask this first. See
+     * SHAMap::addKnownNode for why the verdict is final.
+     *
+     * @return Whether either map is Invalid, and false while there is no ledger
+     *         yet, since then there is no map to judge.
+     */
+    [[nodiscard]] bool
+    hasInvalidMap() const;
 
     void
-    onTimer(bool progress, ScopedLockType& peerSetLock) override;
+    onTimer(bool progress, ScopedLockType& sl) override;
 
     std::size_t
     getPeerCount() const;

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

@@ -6,6 +6,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/chrono.h>
+#include <xrpl/basics/contract.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/ledger/ApplyView.h>
@@ -76,7 +77,22 @@ buildLedgerImpl(
     XRPL_ASSERT(
         built->header().seq < kXrpLedgerEarliestFees || built->read(keylet::feeSettings()),
         "xrpl::buildLedgerImpl : valid ledger fees");
-    built->setAccepted(closeTime, closeResolution, closeTimeCorrect);
+    // Built locally; see Ledger::setImmutable(). built's txMap_ is fresh and its stateMap_ is a
+    // snapshot of the parent's, which carries Invalid over, so an invalid parent is the only way
+    // this can fail - and reaching that needs a corrupt local nodestore rather than a peer, since a
+    // parent's root hashes are ones this node already adopted.
+    //
+    // logicError() rather than UNREACHABLE(), deliberately: this runs on the consensus hot path and
+    // so aborts a Release build, which is the harsher of the two tiers. It is chosen because a
+    // still-mutable ledger aborts a Release build anyway a moment later, at
+    // LedgerMaster::switchLCL() or LedgerHolder::set(), where the cause is no longer visible.
+    // Contrast loadLedgerFromFile(), which runs once at startup and can still return.
+    if (!built->setAccepted(closeTime, closeResolution, closeTimeCorrect))
+    {
+        // LCOV_EXCL_START
+        logicError("buildLedgerImpl: accepted ledger map is invalid");
+        // LCOV_EXCL_STOP
+    }
 
     return built;
 }

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

@@ -54,8 +54,6 @@
 
 namespace xrpl {
 
-using namespace std::chrono_literals;
-
 static constexpr auto kPeerCountStart = 5;           // Number of peers to start with
 static constexpr auto kPeerCountAdd = 3;             // Number of peers to add on a timeout
 static constexpr auto kLedgerTimeoutRetriesMax = 6;  // how many timeouts before we give up
@@ -65,20 +63,18 @@ static constexpr auto kMissingNodesFind = 256;  // Number of nodes to find initi
 static constexpr auto kReqNodesReply = 128;     // Number of nodes to request for a reply
 static constexpr auto kReqNodes = 12;           // Number of nodes to request blindly
 
-// millisecond for each ledger timeout
-constexpr auto kLedgerAcquireTimeout = 3000ms;
-
 InboundLedger::InboundLedger(
     Application& app,
     uint256 const& hash,
     std::uint32_t seq,
     Reason reason,
     clock_type& clock,
-    std::unique_ptr<PeerSet> peerSet)
+    std::unique_ptr<PeerSet> peerSet,
+    std::chrono::milliseconds retryInterval)
     : TimeoutCounter(
           app,
           hash,
-          kLedgerAcquireTimeout,
+          retryInterval,
           {.jobType = JtLedgerData, .jobName = "InboundLedger", .jobLimit = 5},
           app.getJournal("InboundLedger"))
     , clock_(clock)
@@ -97,8 +93,13 @@ InboundLedger::init(ScopedLockType& collectionLock)
     collectionLock.unlock();
 
     tryDB(app_.getNodeFamily().db());
+    // Matches checkLocal(): without done() this object never signals, so logFailure() never runs
+    // and the hash never lands in recentFailures_.
     if (failed_)
+    {
+        done();
         return;
+    }
 
     if (!complete_)
     {
@@ -112,7 +113,19 @@ InboundLedger::init(ScopedLockType& collectionLock)
     XRPL_ASSERT(
         ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()),
         "xrpl::InboundLedger::init : valid ledger fees");
-    ledger_->setImmutable();
+    // tryDB() verified both maps before setting complete_ and mtx_ has been held since, so
+    // nothing can have invalidated them.
+    if (!ledger_->setImmutable())
+    {
+        // LCOV_EXCL_START
+        // Recorded before the UNREACHABLE, which continues in a Release build, and paired with
+        // done() for the same reason as the tryDB() failure above.
+        failed_ = true;
+        done();
+        UNREACHABLE("xrpl::InboundLedger::init : map is invalid");
+        return;
+        // LCOV_EXCL_STOP
+    }
 
     if (reason_ == Reason::HISTORY)
         return;
@@ -222,6 +235,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
@@ -326,14 +345,34 @@ InboundLedger::tryDB(node_store::Database& srcDB)
         }
     }
 
+    // Judged here rather than left to the setImmutable() call below, which runs only once both
+    // flags are set: the two walks above set them independently, so 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 (haveTransactions_ && haveState_)
     {
-        JLOG(journal_.debug()) << "Had everything locally";
-        complete_ = true;
         XRPL_ASSERT(
             ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()),
             "xrpl::InboundLedger::tryDB : valid ledger fees");
-        ledger_->setImmutable();
+        // Settled before complete_ is published, so a caller that reads the flag never sees a
+        // ledger this function has not finished with. Reachable despite the guard above only
+        // because trigger() walks the state map with mtx_ released, so that walk can reach the
+        // verdict in between.
+        if (!ledger_->setImmutable())
+        {
+            JLOG(journal_.warn()) << "Ledger " << hash_ << " found locally is invalid";
+            failed_ = true;
+            return;
+        }
+
+        JLOG(journal_.debug()) << "Had everything locally";
+        complete_ = true;
     }
 }
 
@@ -419,6 +458,48 @@ InboundLedger::done()
     signaled_ = true;
     touch();
 
+    // Settled here, and complete_ published only once it is settled. isComplete() is read without
+    // mtx_, by InboundLedgers::acquire() among others, so a ledger published as complete before
+    // setImmutable() succeeded could be taken by another thread while still mutable - and a mutable
+    // ledger reaching LedgerHistory::insert() or LedgerHolder::set() calls logicError(), which
+    // aborts a Release build. tryDB() already settles its own result and sets complete_ itself, so
+    // that path arrives here with the ledger immutable and only the reporting below left to do.
+    bool const haveEverything = haveHeader_ && haveState_ && haveTransactions_;
+    if (!failed_ && ledger_ && (complete_ || haveEverything))
+    {
+        XRPL_ASSERT(
+            ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()),
+            "xrpl::InboundLedger::done : valid ledger fees");
+        // Recovers rather than asserting: trigger() walks the state map with mtx_ released, so that
+        // walk can reach the verdict after the flags said there was nothing left to fetch. A race
+        // rather than a broken invariant, and one peer data produces, so an abort here would be one
+        // a peer could ask for. 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.
+        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.
+            complete_ = false;
+            failed_ = true;
+        }
+        else
+        {
+            complete_ = true;
+            switch (reason_)
+            {
+                case Reason::HISTORY:
+                    app_.getInboundLedgers().onLedgerFetched();
+                    break;
+                default:
+                    app_.getLedgerMaster().storeLedger(ledger_);
+                    break;
+            }
+        }
+    }
+
     JLOG(journal_.debug()) << "Acquire " << hash_ << (failed_ ? " fail " : " ")
                            << ((timeouts_ == 0)
                                    ? std::string()
@@ -427,24 +508,7 @@ InboundLedger::done()
 
     XRPL_ASSERT(complete_ || failed_, "xrpl::InboundLedger::done : complete or failed");
 
-    if (complete_ && !failed_ && ledger_)
-    {
-        XRPL_ASSERT(
-            ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()),
-            "xrpl::InboundLedger::done : valid ledger fees");
-        ledger_->setImmutable();
-        switch (reason_)
-        {
-            case Reason::HISTORY:
-                app_.getInboundLedgers().onLedgerFetched();
-                break;
-            default:
-                app_.getLedgerMaster().storeLedger(ledger_);
-                break;
-        }
-    }
-
-    // We hold the PeerSet lock, so must dispatch
+    // 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_)
         {
@@ -497,6 +561,8 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         if (failed_)
         {
             JLOG(journal_.warn()) << " failed local for " << hash_;
+            // See init() for why done() must be called here rather than just returning.
+            done();
             return;
         }
     }
@@ -513,7 +579,17 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         {
             auto need = getNeededHashes();
 
-            if (!need.empty())
+            // Asked first, since getNeededHashes() walks both maps and can reach the verdict
+            // itself, and the branch below would otherwise read an empty result as "nothing left
+            // to fetch". Without a header there is no map to judge, but then getNeededHashes() has
+            // asked for the header, so the non-empty branch is the right one.
+            SOMETIMES(hasInvalidMap(), "xrpl::InboundLedger::trigger : map is invalid");
+            if (hasInvalidMap())
+            {
+                JLOG(journal_.warn()) << "Acquire " << hash_ << " has an invalid map";
+                failed_ = true;
+            }
+            else if (!need.empty())
             {
                 protocol::TMGetObjectByHash tmBH;
                 bool typeSet = false;
@@ -550,11 +626,12 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
             }
             else
             {
+                // The tail of this function turns having every part into a completed acquisition,
+                // once done() has settled the ledger.
                 JLOG(journal_.info()) << "getNeededHashes says acquire is complete";
                 haveHeader_ = true;
                 haveTransactions_ = true;
                 haveState_ = true;
-                complete_ = true;
             }
         }
     }
@@ -617,7 +694,11 @@ 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 we process the large state map. This is the only walk in this
+            // class that runs unlocked: onTimer() and the addPeers() callback both hold mtx_
+            // further up, and mtx_ is recursive, so their sl.unlock() releases nothing. So a
+            // second packet can be processed while this walk runs, which is why the map's own
+            // state is atomic and why the flags are re-read below.
             sl.unlock();
             auto nodes = ledger_->stateMap().getMissingNodes(kMissingNodesFind, &filter);
             sl.lock();
@@ -634,9 +715,6 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
                     else
                     {
                         haveState_ = true;
-
-                        if (haveTransactions_)
-                            complete_ = true;
                     }
                 }
                 else
@@ -699,9 +777,6 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
                 else
                 {
                     haveTransactions_ = true;
-
-                    if (haveState_)
-                        complete_ = true;
                 }
             }
             else
@@ -726,11 +801,14 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         }
     }
 
-    if (complete_ || failed_)
+    // Having every part is not yet a completed acquisition: done() settles the ledger first and
+    // only then publishes complete_. Called with mtx_ still held, as done() documents, so the flags
+    // it writes are not written unlocked; mtx_ is recursive, so a caller that already holds it is
+    // unaffected.
+    if (failed_ || (haveHeader_ && haveState_ && haveTransactions_))
     {
-        JLOG(journal_.debug()) << "Done:" << (complete_ ? " complete" : "")
-                               << (failed_ ? " failed " : " ") << ledger_->header().seq;
-        sl.unlock();
+        JLOG(journal_.debug()) << "Done:" << (failed_ ? " failed " : " have everything ")
+                               << ledger_->header().seq;
         done();
     }
 }
@@ -928,11 +1006,10 @@ InboundLedger::receiveNode(
             haveState_ = true;
         }
 
+        // done() settles the ledger before publishing complete_, so having every part is reported
+        // there rather than here.
         if (haveTransactions_ && haveState_)
-        {
-            complete_ = true;
             done();
-        }
     }
 }
 

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

@@ -173,8 +173,11 @@ public:
         {
             peer->charge(resource::kFeeInvalidData, "ledger_data invalid");
         }
-        else if (!san.isUseful())
+        else if (!san.isGood())
         {
+            // Good rather than useful: the verdict now tells a batch of nodes we already hold from
+            // one that was never examined, and a duplicate is what an honest second responder to
+            // trigger()'s fan-out sends.
             peer->charge(resource::kFeeUselessData, "ledger_data useless");
         }
     }

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

@@ -115,8 +115,15 @@ loadLedgerHelper(
     return ledger;
 }
 
+/**
+ * Settle a ledger just loaded from local storage, or discard it.
+ *
+ * @param ledger The ledger to settle; cleared on failure so the caller
+ *        cannot hand out one that is still mutable.
+ * @param j Where to log a refusal.
+ */
 static void
-finishLoadByIndexOrHash(std::shared_ptr<Ledger> const& ledger, beast::Journal j)
+finishLoadByIndexOrHash(std::shared_ptr<Ledger>& ledger, beast::Journal j)
 {
     if (!ledger)
         return;
@@ -124,7 +131,19 @@ finishLoadByIndexOrHash(std::shared_ptr<Ledger> const& ledger, beast::Journal j)
     XRPL_ASSERT(
         ledger->header().seq < kXrpLedgerEarliestFees || ledger->read(keylet::feeSettings()),
         "xrpl::finishLoadByIndexOrHash : valid ledger fees");
-    ledger->setImmutable();
+    // Loaded locally; see Ledger::setImmutable().
+    if (!ledger->setImmutable())
+    {
+        // LCOV_EXCL_START
+        JLOG(j.error()) << "Invalid map for ledger " << ledger->header().seq
+                        << "; not marking it as loaded";
+        UNREACHABLE("xrpl::finishLoadByIndexOrHash : map is invalid");
+        // Discarded rather than left un-full: nothing gates usability on the full flag, and a
+        // caller that took this ledger would abort in LedgerHistory::insert() instead.
+        ledger.reset();
+        return;
+        // LCOV_EXCL_STOP
+    }
 
     JLOG(j.trace()) << "Loaded ledger: " << to_string(ledger->header().hash);
 

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

@@ -98,9 +98,16 @@ protected:
 
     /**
      * Hook called from invokeOnTimer().
+     *
+     * @param progress Whether the subtype recorded progress since the
+     *        last call.
+     * @param sl Proof mtx_ is held, and held for the whole call. It is
+     *        this object's own mutex rather than anything belonging to a
+     *        PeerSet, and it is recursive, so a nested lock taken inside
+     *        this call releases nothing when it goes out of scope.
      */
     virtual void
-    onTimer(bool progress, ScopedLockType&) = 0;
+    onTimer(bool progress, ScopedLockType& sl) = 0;
 
     /**
      * Return a weak pointer to this.

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

@@ -8,6 +8,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/core/Job.h>
 #include <xrpl/server/NetworkOPs.h>
 #include <xrpl/shamap/SHAMap.h>
@@ -18,6 +19,7 @@
 #include <xrpl.pb.h>
 
 #include <algorithm>
+#include <chrono>
 #include <cstddef>
 #include <exception>
 #include <memory>
@@ -26,22 +28,18 @@
 
 namespace xrpl {
 
-using namespace std::chrono_literals;
-
-// Timeout interval in milliseconds
-constexpr auto kTxAcquireTimeout = 250ms;
-
 static constexpr auto kNormTimeouts = 4;
 static constexpr auto kMaxTimeouts = 20;
 
 TransactionAcquire::TransactionAcquire(
     Application& app,
     uint256 const& hash,
-    std::unique_ptr<PeerSet> peerSet)
+    std::unique_ptr<PeerSet> peerSet,
+    std::chrono::milliseconds retryInterval)
     : TimeoutCounter(
           app,
           hash,
-          kTxAcquireTimeout,
+          retryInterval,
           {.jobType = JtTxnData, .jobName = "TxAcq", .jobLimit = {}},
           app.getJournal("TransactionAcquire"))
     , peerSet_(std::move(peerSet))
@@ -53,16 +51,36 @@ TransactionAcquire::TransactionAcquire(
 void
 TransactionAcquire::done()
 {
-    // We hold a PeerSet lock and so cannot do real work here
+    // mtx_ is held, so this may only post real work rather than do it.
+
+    // Runs at most once per outcome, since every caller reaches here only after clearing
+    // TimeoutCounter's isDone() gate and setting complete_ or failed_. It can still run twice for
+    // two outcomes, since stillNeed() revives a timed-out set that can finish later - which is why
+    // this is unlatched, unlike InboundLedger::done() with its signaled_.
 
     if (failed_)
     {
         JLOG(journal_.debug()) << "Failed to acquire TX set " << hash_;
     }
+    else if (!map_->setImmutable())
+    {
+        // trigger() verified the map before setting complete_ and mtx_ has been held since, and
+        // unlike InboundLedger nothing walks this map with the lock released, so nothing can have
+        // invalidated it. Untestable for that reason, and left an UNREACHABLE rather than turned
+        // into a recovery: there is no interleaving that reaches it.
+        // LCOV_EXCL_START
+        // Withdraw complete_ alongside the failure, or trigger() and takeNodes() - which both check
+        // complete_ before failed_ - keep treating this as delivered while consensus waits on a set
+        // giveSet() never hands over.
+        complete_ = false;
+        failed_ = true;
+        JLOG(journal_.debug()) << "Failed to acquire TX set " << hash_;
+        UNREACHABLE("xrpl::TransactionAcquire::done : map is invalid");
+        // LCOV_EXCL_STOP
+    }
     else
     {
         JLOG(journal_.debug()) << "Acquired TX set " << hash_;
-        map_->setImmutable();
 
         uint256 const& hash(hash_);
         std::shared_ptr<SHAMap> const& map(map_);
@@ -79,7 +97,7 @@ TransactionAcquire::done()
 }
 
 void
-TransactionAcquire::onTimer(bool progress, ScopedLockType& psl)
+TransactionAcquire::onTimer(bool progress, ScopedLockType&)
 {
     if (timeouts_ > kMaxTimeouts)
     {
@@ -174,8 +192,29 @@ TransactionAcquire::takeNodes(
     std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
     std::shared_ptr<Peer> const& peer)
 {
-    ScopedLockType const sl(mtx_);
+    ScopedLockType sl(mtx_);
 
+    auto const san = takeNodesLocked(std::move(data), peer, sl);
+
+    // Recorded here rather than on each of takeNodesLocked()'s exits, several of which stop the
+    // batch early: a batch that advanced the map must keep the next timer tick from counting a
+    // timeout against it, and nothing in the compiler would catch an exit that forgot to say so.
+    //
+    // Useful rather than good: a batch of nothing but duplicates advanced nothing, so it records no
+    // progress even though it was not the sender's fault. That costs retry budget on the ordinary
+    // second responder to a fan-out and nothing else.
+    if (san.isUseful())
+        progress_ = true;
+
+    return san;
+}
+
+SHAMapAddNode
+TransactionAcquire::takeNodesLocked(
+    std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
+    std::shared_ptr<Peer> const& peer,
+    ScopedLockType&)
+{
     if (complete_)
     {
         JLOG(journal_.trace()) << "TX set complete";
@@ -188,6 +227,10 @@ TransactionAcquire::takeNodes(
         return SHAMapAddNode();
     }
 
+    // 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())
@@ -202,36 +245,45 @@ 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;
+                    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();
+                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();
+        san.incInvalid();
+        return san;
     }
 }
 
@@ -257,10 +309,20 @@ TransactionAcquire::init(int numPeers)
 void
 TransactionAcquire::stillNeed()
 {
-    ScopedLockType const sl(mtx_);
+    ScopedLockType sl(mtx_);
 
     timeouts_ = std::min<int>(timeouts_, kNormTimeouts);
+
+    // Nothing to revive: leave a running acquisition on the wait it has, rather than restarting it
+    // for every consensus round that asks for the set again.
+    if (!failed_)
+        return;
+
     failed_ = false;
+
+    // Restarting the timer is what resumes the acquisition. expires_after() cancels any pending
+    // wait, so this cannot leave two timer chains running.
+    setTimer(sl);
 }
 
 }  // namespace xrpl

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

@@ -11,6 +11,7 @@
 #include <xrpl/shamap/SHAMapAddNode.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
+#include <chrono>
 #include <cstddef>
 #include <memory>
 #include <utility>
@@ -20,16 +21,45 @@ namespace xrpl {
 
 // VFALCO TODO rename to PeerTxRequest
 // A transaction set we are trying to acquire
-class TransactionAcquire final : public TimeoutCounter,
-                                 public std::enable_shared_from_this<TransactionAcquire>,
-                                 public CountedObject<TransactionAcquire>
+class TransactionAcquire : public TimeoutCounter,
+                           public std::enable_shared_from_this<TransactionAcquire>,
+                           public CountedObject<TransactionAcquire>
 {
 public:
     using pointer = std::shared_ptr<TransactionAcquire>;
 
-    TransactionAcquire(Application& app, uint256 const& hash, std::unique_ptr<PeerSet> peerSet);
+    /**
+     * How long to wait between retries, and so how long each timeout counted
+     * against the acquisition takes. Short, since a set is wanted for the
+     * consensus round that asked for it or not at all.
+     */
+    static constexpr std::chrono::milliseconds kRetryInterval{250};
+
+    /**
+     * @param app The application to run in.
+     * @param hash The set to acquire.
+     * @param peerSet Which peers to ask, and how to reach them.
+     * @param retryInterval How long to wait between retries. Defaulted in
+     *        production; TransactionAcquire_test passes a short one so a whole
+     *        timeout chain runs in a fraction of the time. TimeoutCounter
+     *        requires more than 10ms.
+     */
+    TransactionAcquire(
+        Application& app,
+        uint256 const& hash,
+        std::unique_ptr<PeerSet> peerSet,
+        std::chrono::milliseconds retryInterval = kRetryInterval);
     ~TransactionAcquire() override = default;
 
+    /**
+     * Add nodes a peer sent us to the set we are acquiring.
+     *
+     * @param data The nodes to add, each with its claimed position.
+     * @param peer The peer that sent them.
+     * @return The tally of useful, unwanted, and bad nodes in the batch. Useful and
+     *         bad can both be nonzero, since only the node the batch stops on is
+     *         bad.
+     */
     SHAMapAddNode
     takeNodes(
         std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
@@ -38,18 +68,53 @@ public:
     void
     init(int startPeers);
 
+    /**
+     * Resume a timed-out acquisition, or leave a running one alone.
+     *
+     * Always clamps the timeout count. An acquisition that failed has its timer
+     * chain stopped, so this also clears the failed flag and restarts the timer;
+     * one that is still running already has a timer pending.
+     */
     void
     stillNeed();
 
-private:
+protected:
+    // Kept protected so a test subclass (see TransactionAcquire_test) can read the map's state,
+    // which nothing else publishes. Production callers reach a set through InboundTransactions.
     std::shared_ptr<SHAMap> map_;
+
+private:
     bool haveRoot_{false};
     std::unique_ptr<PeerSet> peerSet_;
 
-    void
-    onTimer(bool progress, ScopedLockType& peerSetLock) override;
+    /**
+     * Add nodes a peer sent us, on the lock takeNodes() holds.
+     *
+     * Split out so recording what the batch achieved happens on one exit rather
+     * than on each of the several this has, including the ones that stop the batch
+     * early.
+     *
+     * @param data The nodes to add, each with its claimed position.
+     * @param peer The peer that sent them.
+     * @return The tally of useful, unwanted, and bad nodes in the batch.
+     */
+    SHAMapAddNode
+    takeNodesLocked(
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
+        std::shared_ptr<Peer> const& peer,
+        ScopedLockType&);
 
     void
+    onTimer(bool progress, ScopedLockType& sl) override;
+
+    /**
+     * Settle the acquired set and hand it on, or report the failure. Call under
+     * mtx_.
+     *
+     * Runs at most once per outcome rather than once in total, since
+     * stillNeed() can revive a timed-out acquisition that then finishes.
+     */
+    void
     done();
 
     void

src/xrpld/app/main/Application.cpp

@@ -1700,7 +1700,14 @@ ApplicationImp::startGenesisLedger()
     XRPL_ASSERT(
         next->header().seq < kXrpLedgerEarliestFees || next->read(keylet::feeSettings()),
         "xrpl::ApplicationImp::startGenesisLedger : valid ledger fees");
-    next->setImmutable();
+    // Built locally; see Ledger::setImmutable(). Failed here rather than at storeLedger() below,
+    // which would name the wrong site.
+    if (!next->setImmutable())
+    {
+        // LCOV_EXCL_START
+        logicError("startGenesisLedger: genesis ledger map is invalid");
+        // LCOV_EXCL_STOP
+    }
     openLedger_.emplace(next, cachedSLEs_, logs_->journal("OpenLedger"));
     ledgerMaster_->storeLedger(next);
     ledgerMaster_->switchLCL(next);
@@ -1722,7 +1729,16 @@ ApplicationImp::getLastFullLedger()
         XRPL_ASSERT(
             ledger->header().seq < kXrpLedgerEarliestFees || ledger->read(keylet::feeSettings()),
             "xrpl::ApplicationImp::getLastFullLedger : valid ledger fees");
-        ledger->setImmutable();
+        // Loaded locally; see Ledger::setImmutable().
+        if (!ledger->setImmutable())
+        {
+            // LCOV_EXCL_START
+            JLOG(j.error()) << "Last full ledger " << seq << " has an invalid map; ignoring it";
+            UNREACHABLE("xrpl::ApplicationImp::getLastFullLedger : map is invalid");
+            // Must not fall through: that would mark a damaged ledger validated.
+            return {};
+            // LCOV_EXCL_STOP
+        }
 
         if (getLedgerMaster().haveLedger(seq))
             ledger->setValidated();
@@ -1874,7 +1890,16 @@ ApplicationImp::loadLedgerFromFile(std::string const& name)
             loadLedger->header().seq < kXrpLedgerEarliestFees ||
                 loadLedger->read(keylet::feeSettings()),
             "xrpl::ApplicationImp::loadLedgerFromFile : valid ledger fees");
-        loadLedger->setAccepted(closeTime, closeTimeResolution, !closeTimeEstimated);
+        // Built locally; see Ledger::setImmutable(). Unlike the genesis sites the caller handles a
+        // failure return, so bail out rather than abort.
+        if (!loadLedger->setAccepted(closeTime, closeTimeResolution, !closeTimeEstimated))
+        {
+            // LCOV_EXCL_START
+            JLOG(journal_.fatal()) << "Ledger from file has an invalid map";
+            UNREACHABLE("xrpl::ApplicationImp::loadLedgerFromFile : map is invalid");
+            return nullptr;
+            // LCOV_EXCL_STOP
+        }
 
         return loadLedger;
     }