Merge develop

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

.github/actions/cargo-cache/action.yml

@@ -0,0 +1,38 @@
+name: Use cargo artifacts cache
+description: >
+  Cache the cargo build artifacts with rust-cache. Never caches ~/.cargo/bin:
+  when saving the cache, rust-cache deletes all binaries that were already
+  present there, which on persistent self-hosted runners wipes the tools
+  installed by prepare-runner. Harmless on ephemeral runners, but kept
+  consistent everywhere.
+
+inputs:
+  workspaces:
+    description: "Workspaces to cache, as 'workspace -> target' lines."
+    required: false
+    default: crates
+  key:
+    description: "Additional part of the cache key."
+    required: false
+    default: ""
+  cache-directories:
+    description: "Additional non-workspace directories to cache."
+    required: false
+    default: ""
+  save-if:
+    description: "Condition for saving the cache after the job."
+    required: false
+    default: "true"
+
+runs:
+  using: composite
+
+  steps:
+    - name: Use cargo artifacts cache
+      uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
+      with:
+        cache-bin: "false"
+        cache-directories: ${{ inputs.cache-directories }}
+        key: ${{ inputs.key }}
+        save-if: ${{ inputs.save-if }}
+        workspaces: ${{ inputs.workspaces }}

.github/dependabot.yml

@@ -4,6 +4,7 @@ updates:
     directories:
       - /
       - .github/actions/build-deps/
+      - .github/actions/cargo-cache/
       - .github/actions/release-info/
       - .github/actions/set-compiler-env/
       - .github/actions/setup-conan/

.github/workflows/build-nix-images.yml

@@ -58,7 +58,7 @@ jobs:
             base_image: debian:bookworm
           - name: rhel
             base_image: registry.access.redhat.com/ubi9/ubi:latest
-    uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@9e7e4e80af9e684c116b38369add8eea64451f32
+    uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@65d5a0bd72be4ecea95cff0673a6e0672ab5243a
     with:
       image_name: xrpld/nix-${{ matrix.distro.name }}
       dockerfile: nix/docker/Dockerfile

.github/workflows/build-packaging-images.yml

@@ -39,7 +39,7 @@ jobs:
           # AlmaLinux rather than UBI9, which does not ship rpm-sign.
           - name: rhel
             base_image: almalinux:9
-    uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@9e7e4e80af9e684c116b38369add8eea64451f32
+    uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@65d5a0bd72be4ecea95cff0673a6e0672ab5243a
     with:
       image_name: xrpld/packaging-${{ matrix.distro.name }}
       dockerfile: package/Dockerfile

.github/workflows/build-pre-commit-image.yml

@@ -30,7 +30,7 @@ jobs:
     permissions:
       contents: read
       packages: write
-    uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@9e7e4e80af9e684c116b38369add8eea64451f32
+    uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@65d5a0bd72be4ecea95cff0673a6e0672ab5243a
     with:
       image_name: xrpld/pre-commit
       dockerfile: bin/pre-commit/Dockerfile

.github/workflows/check-tools.yml

@@ -79,7 +79,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@51af40f99ea91a08c3528ddf16d98132dcc7e63c
+        uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13
         with:
           enable_ccache: false
 

.github/workflows/pre-commit.yml

@@ -14,7 +14,7 @@ on:
 jobs:
   # Call the workflow in the XRPLF/actions repo that runs the pre-commit hooks.
   run-hooks:
-    uses: XRPLF/actions/.github/workflows/pre-commit.yml@3ba08d6ddf114092891d48491fc2e26c3ba15552
+    uses: XRPLF/actions/.github/workflows/pre-commit.yml@f1952595d212e86169935135efc66294b4574131
     with:
       runs_on: ubuntu-latest
       container: '{ "image": "ghcr.io/xrplf/xrpld/pre-commit:sha-f56b79f" }'

.github/workflows/publish-docs.yml

@@ -47,7 +47,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@51af40f99ea91a08c3528ddf16d98132dcc7e63c
+        uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13
         with:
           enable_ccache: false
 

.github/workflows/reusable-build-test-config.yml

@@ -129,7 +129,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@51af40f99ea91a08c3528ddf16d98132dcc7e63c
+        uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13
         with:
           enable_ccache: ${{ inputs.ccache_enabled }}
 
@@ -163,7 +163,7 @@ jobs:
           compiler: ${{ inputs.compiler }}
 
       - name: Use cargo artifacts cache
-        uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
+        uses: ./.github/actions/cargo-cache
         with:
           cache-directories: ${{ env.BUILD_DIR }}/corrosion
           key: ${{ inputs.config_name }}

.github/workflows/reusable-clang-tidy.yml

@@ -27,7 +27,7 @@ jobs:
   determine-files:
     permissions:
       contents: read
-    uses: XRPLF/actions/.github/workflows/determine-tidy-files.yml@d041ac9f1fa9f07a4ba335eb4c1c82233fb3fef6
+    uses: XRPLF/actions/.github/workflows/determine-tidy-files.yml@70145243b905dc3e040a61d39c00e178cfb96f71
 
   run-clang-tidy:
     name: Run clang tidy
@@ -43,7 +43,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@51af40f99ea91a08c3528ddf16d98132dcc7e63c
+        uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13
         with:
           enable_ccache: false
 
@@ -60,7 +60,7 @@ jobs:
           compiler: ${{ env.COMPILER }}
 
       - name: Use cargo artifacts cache
-        uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
+        uses: ./.github/actions/cargo-cache
         with:
           cache-directories: ${{ env.BUILD_DIR }}/corrosion
           save-if: ${{ github.ref == 'refs/heads/develop' || startsWith(github.ref, 'refs/heads/release') }}

.github/workflows/reusable-rust.yml

@@ -33,9 +33,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Use cargo artifacts cache
-        uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
-        with:
-          workspaces: crates
+        uses: ./.github/actions/cargo-cache
 
       - name: Run clippy
         run: cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
@@ -48,9 +46,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Use cargo artifacts cache
-        uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
-        with:
-          workspaces: crates
+        uses: ./.github/actions/cargo-cache
 
       - name: Generate coverage report
         run: cargo llvm-cov nextest --workspace --all-features --locked --no-tests=warn --lcov --output-path lcov.info
@@ -76,9 +72,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Use cargo artifacts cache
-        uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
-        with:
-          workspaces: crates
+        uses: ./.github/actions/cargo-cache
 
       - name: Build documentation
         env:

.github/workflows/upload-conan-deps.yml

@@ -68,7 +68,7 @@ jobs:
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@c00c22ada3bd6bcda48fcb0d62fbbab49fec8a0f
+        uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13
         with:
           enable_ccache: false
 

CMakeLists.txt

@@ -114,6 +114,7 @@ find_package(OpenSSL REQUIRED)
 find_package(secp256k1 REQUIRED)
 find_package(SOCI REQUIRED)
 find_package(SQLite3 REQUIRED)
+find_package(wasmi REQUIRED)
 find_package(xxHash REQUIRED)
 
 target_link_libraries(

cmake/XrplCompiler.cmake

@@ -120,7 +120,10 @@ if(MSVC)
             _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS
             $<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CONFIG:Debug>>:_CRTDBG_MAP_ALLOC>
     )
-    target_link_libraries(common INTERFACE -errorreport:none -machine:X64)
+    target_link_libraries(
+        common
+        INTERFACE -errorreport:none -machine:X64 -ignore:4099
+    )
 else()
     target_compile_options(
         common

cmake/XrplCore.cmake

@@ -69,6 +69,7 @@ target_link_libraries(
         Xrpl::opts
         Xrpl::syslibs
         secp256k1::secp256k1
+        wasmi::wasmi
         xrpl.libpb
         xxHash::xxhash
         $<$<BOOL:${voidstar}>:antithesis-sdk-cpp>

cmake/XrplSanitizers.cmake

@@ -77,24 +77,19 @@ if(is_clang)
     message(STATUS "  Ignorelist: ${ignorelist_path}")
 endif()
 
-# 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.
+# Define SANITIZERS macro for BuildInfo.cpp
 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)

conan.lock

@@ -3,6 +3,7 @@
     "requires": [
         "zlib/1.3.2#1cb806da49011867778ffb6ac7190fcb%1782392402.122708",
         "xxhash/0.8.3#681d36a0a6111fc56e5e45ea182c19cc%1782392402.420688",
+        "wasmi/1.0.9#1fecdab9b90c96698eb35ea99ca4f5cb%1782307153.343419",
         "sqlite3/3.53.0#324ada52333108388a9a6108bfa96734%1782392403.185447",
         "soci/4.0.3#e726491a03468795453f7c83fc924a96%1782392402.679521",
         "snappy/1.1.10#968fef506ff261592ec30c574d4a7809%1782307151.633168",

conanfile.py

@@ -36,6 +36,7 @@ class Xrpl(ConanFile):
         "nudb/2.0.9",
         "openssl/3.6.3",
         "soci/4.0.3",
+        "wasmi/1.0.9",
         "zlib/1.3.2",
     ]
 
@@ -224,6 +225,7 @@ class Xrpl(ConanFile):
             "soci::soci",
             "secp256k1::secp256k1",
             "sqlite3::sqlite",
+            "wasmi::wasmi",
             "xxhash::xxhash",
             "zlib::zlib",
         ]

crates/Cargo.lock

@@ -57,9 +57,9 @@ dependencies = [
 
 [[package]]
 name = "cxx"
-version = "1.0.198"
+version = "1.0.199"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6fe442a792c7c736eea18b32a7f8a3b63cf8aafabda6760042dc2fdeda456291"
+checksum = "824894a4a85dca76d4c95c2b9098c036f5a29f627b30c12780774f6654e60974"
 dependencies = [
  "cc",
  "cxx-build",
@@ -72,9 +72,9 @@ dependencies = [
 
 [[package]]
 name = "cxx-build"
-version = "1.0.198"
+version = "1.0.199"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e3184a94384c663718698311a78a51ac00c484c10b4eeac06fb0a068c5f64fa2"
+checksum = "f1ae0b651ea5b0000b19513aef5a03f194d7e3486f2d9258b658da8677fe9036"
 dependencies = [
  "cc",
  "codespan-reporting",
@@ -87,9 +87,9 @@ dependencies = [
 
 [[package]]
 name = "cxxbridge-cmd"
-version = "1.0.198"
+version = "1.0.199"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0148d8fd1199329ddf1d157a5e134e51ceff37c6a7ddd38615c399d81cb05d8d"
+checksum = "fb05f91d3fb8435d9bab6ac5ce6ac1868be774325fb7fb2a91be39393b21388e"
 dependencies = [
  "clap",
  "codespan-reporting",
@@ -101,15 +101,15 @@ dependencies = [
 
 [[package]]
 name = "cxxbridge-flags"
-version = "1.0.198"
+version = "1.0.199"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "52850339faed2eaadd24e286dc1d8268cc6f8a7bd9524d713adc9099566b4c89"
+checksum = "bf293202e0e3e98495785745389e8d0755b217e66f19194a5c695c25e03282ef"
 
 [[package]]
 name = "cxxbridge-macro"
-version = "1.0.198"
+version = "1.0.199"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2c77c856545d886c9bd5215409ebb63b925e262135248b50c79e5a5f194ee47c"
+checksum = "ca001d746947c7249ed9d332a10f7a59daedbafeb0ec68c5c18a7db7a93f6ccc"
 dependencies = [
  "indexmap",
  "proc-macro2",

docs/install.md

@@ -65,7 +65,7 @@ wherever it appears in the repository configuration.
 4.  Add the repository, using the channel you picked in [Release channels](#release-channels):
 
     ```bash
-    echo "deb [signed-by=/etc/apt/keyrings/xrplf.asc] https://packages.xrplf.org/repository/deb-stable focal main" | \
+    echo "deb [signed-by=/etc/apt/keyrings/xrplf.asc] https://packages.xrplf.org/repository/deb-stable any main" | \
         sudo tee /etc/apt/sources.list.d/xrplf.list
     ```
 
@@ -98,13 +98,13 @@ wherever it appears in the repository configuration.
     enabled=1
     baseurl=https://packages.xrplf.org/repository/rpm-stable/
     gpgcheck=1
-    repo_gpgcheck=0
+    repo_gpgcheck=1
     gpgkey=https://packages.xrplf.org/xrplf.asc
     REPOFILE
     ```
 
     `gpgcheck=1` verifies each package against the key above.
-    `repo_gpgcheck` is off because the repository metadata is generated by the server and is not signed.
+    `repo_gpgcheck=1` verifies the repository metadata, which the server signs with the same key.
 
 3.  Install the `xrpld` package:
 

include/xrpl/ledger/Ledger.h

@@ -257,43 +257,13 @@ public:
         header_.validated = true;
     }
 
-    /**
-     * 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
+    void
     setAccepted(
         NetClock::time_point closeTime,
         NetClock::duration closeResolution,
         bool correctCloseTime);
 
-    /**
-     * 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
+    void
     setImmutable(bool rehash = true);
 
     bool
@@ -302,38 +272,23 @@ public:
         return immutable_;
     }
 
-    /**
-     * 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();
-    }
+    /*  Mark this ledger as "should be full".
 
-    /**
-     * 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.
-     */
+        "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.
+    */
     void
     setFull() const
     {
-        // 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);
         txMap_.setFull();
-        stateMap_.setLedgerSeq(header_.seq);
+        txMap_.setLedgerSeq(header_.seq);
         stateMap_.setFull();
+        stateMap_.setLedgerSeq(header_.seq);
     }
 
     void
@@ -463,37 +418,8 @@ 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,7 +2,6 @@
 
 #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>
@@ -17,7 +16,6 @@
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
-#include <atomic>
 #include <condition_variable>
 #include <cstddef>
 #include <cstdint>
@@ -42,7 +40,7 @@ class SHAMapSyncFilter;
 /**
  * Describes the current state of a given SHAMap
  */
-enum class SHAMapState : std::uint8_t {
+enum class SHAMapState {
     /**
      * The map is in flux and objects can be added and removed.
      *
@@ -122,43 +120,16 @@ 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::atomic<std::uint32_t> ledgerSeq_ = 0;
+    std::uint32_t ledgerSeq_ = 0;
 
     SHAMapTreeNodePtr root_;
-
-    /**
-     * The map's state.
-     *
-     * A getMissingNodes() walk writes it, through setInvalid() and
-     * clearSynching(), while whatever drives the acquisition reads it.
-     * 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_;
+    mutable SHAMapState state_;
     SHAMapType const type_;
-    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;
+    bool backed_ = true;         // Map is backed by the database
+    mutable bool full_ = false;  // Map is believed complete in database
 
 public:
     /**
@@ -181,14 +152,7 @@ public:
     SHAMap&
     operator=(SHAMap const&) = delete;
 
-    /**
-     * 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.
-     */
+    // Take a snapshot of the given map:
     SHAMap(SHAMap const& other, bool isMutable);
 
     // build new map
@@ -226,15 +190,8 @@ public:
 
     //--------------------------------------------------------------------------
 
-    /**
-     * 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.
-     */
+    // Returns a new map that's a snapshot of this one.
+    // Handles copy on write for mutable snapshots.
     std::shared_ptr<SHAMap>
     snapShot(bool isMutable) const;
 
@@ -339,14 +296,9 @@ 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
-     * @return The nodes known to be missing, or empty if the map is Invalid
+     * @param return The nodes known to be missing
      */
     std::vector<std::pair<SHAMapNodeID, uint256>>
     getMissingNodes(int maxNodes, SHAMapSyncFilter const* filter);
@@ -413,11 +365,6 @@ 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.
@@ -428,52 +375,16 @@ public:
         SHAMapTreeNodePtr treeNode,
         SHAMapSyncFilter const* filter);
 
-    /**
-     * 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
+    // status functions
+    void
     setImmutable();
-
-    /**
-     * 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
+    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();
-
-    /**
-     * 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
+    bool
     isValid() const;
 
     // caution: otherMap must be accessed only by this function
@@ -513,46 +424,6 @@ 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;
@@ -768,100 +639,44 @@ private:
 inline void
 SHAMap::setFull()
 {
-    full_.store(true, std::memory_order_release);
+    full_ = true;
 }
 
 inline void
 SHAMap::setLedgerSeq(std::uint32_t lseq)
 {
-    ledgerSeq_.store(lseq, std::memory_order_relaxed);
+    ledgerSeq_ = lseq;
 }
 
-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
+inline void
 SHAMap::setImmutable()
 {
-    SOMETIMES(!isValid(), "xrpl::SHAMap::setImmutable : map is invalid");
-    return trySetState(SHAMapState::Immutable);
+    XRPL_ASSERT(state_ != SHAMapState::Invalid, "xrpl::SHAMap::setImmutable : state is valid");
+    state_ = SHAMapState::Immutable;
 }
 
 inline bool
 SHAMap::isSynching() const
 {
-    return state() == SHAMapState::Synching;
+    return state_ == SHAMapState::Synching;
 }
 
 inline void
 SHAMap::setSynching()
 {
-    // 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
-    }
+    state_ = SHAMapState::Synching;
 }
 
 inline void
 SHAMap::clearSynching()
 {
-    // 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();
-    }
+    state_ = SHAMapState::Modifying;
 }
 
 inline bool
 SHAMap::isValid() const
 {
-    return state() != SHAMapState::Invalid;
-}
-
-inline void
-SHAMap::setInvalid()
-{
-    state_.store(SHAMapState::Invalid, std::memory_order_release);
+    return state_ != SHAMapState::Invalid;
 }
 
 inline void

include/xrpl/shamap/SHAMapAddNode.h

@@ -14,140 +14,35 @@ 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();
-
-    /**
-     * 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]] int
+    getGood() const;
+    [[nodiscard]] bool
+    isGood() const;
+    [[nodiscard]] bool
+    isInvalid() const;
+    [[nodiscard]] bool
+    isUseful() const;
     [[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);
 };
@@ -179,30 +74,18 @@ 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
 {
@@ -215,6 +98,22 @@ 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()
 {
@@ -233,12 +132,6 @@ SHAMapAddNode::invalid()
     return SHAMapAddNode(0, 1, 0);
 }
 
-inline void
-SHAMapAddNode::reset()
-{
-    good_ = bad_ = duplicate_ = 0;
-}
-
 inline std::string
 SHAMapAddNode::get() const
 {
@@ -267,14 +160,4 @@ 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,25 +31,7 @@ private:
      */
     TaggedPointer hashesAndChildren_;
 
-    // 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::uint32_t fullBelowGen_ = 0;
     std::uint16_t isBranch_ = 0;
 
     /**
@@ -222,13 +204,13 @@ SHAMapInnerNode::getBranchCount() const
 inline bool
 SHAMapInnerNode::isFullBelow(std::uint32_t generation) const
 {
-    return fullBelowGen_.load(std::memory_order_relaxed) == generation;
+    return fullBelowGen_ == generation;
 }
 
 inline void
 SHAMapInnerNode::setFullBelowGen(std::uint32_t gen)
 {
-    fullBelowGen_.store(gen, std::memory_order_relaxed);
+    fullBelowGen_ = gen;
 }
 
 }  // namespace xrpl

package/README.md

@@ -126,15 +126,15 @@ release defaults to 1 and is overridable with `-Dpkg_release=N`.
 
 Packages are published to the XRPLF repositories on Sonatype Nexus at
 `https://packages.xrplf.org`. The `release-info` action decides the channel from
-the event, and `publish_pkg.sh` maps that channel to a repository pair:
+the event, and `publish_pkg.sh` maps that channel to its repositories:
 
-| Event                    | Version           | Channel        | DEB repository     | RPM repository     |
-| ------------------------ | ----------------- | -------------- | ------------------ | ------------------ |
-| tag                      | `X.Y.Z`           | `stable`       | `deb-stable`       | `rpm-stable`       |
-| tag                      | `X.Y.Z-rcN`       | `unstable`     | `deb-unstable`     | `rpm-unstable`     |
-| tag                      | `X.Y.Z-bN`        | `experimental` | `deb-experimental` | `rpm-experimental` |
-| push to `develop`        | `xrpld --version` | `develop`      | `deb-develop`      | `rpm-develop`      |
-| tag, non-public codebase | _any_             | `private`      | `deb-private`      | `rpm-private`      |
+| Event                    | Version           | Channel        | DEB repository     | RPM upload repository     |
+| ------------------------ | ----------------- | -------------- | ------------------ | ------------------------- |
+| tag                      | `X.Y.Z`           | `stable`       | `deb-stable`       | `rpm-stable-hosted`       |
+| tag                      | `X.Y.Z-rcN`       | `unstable`     | `deb-unstable`     | `rpm-unstable-hosted`     |
+| tag                      | `X.Y.Z-bN`        | `experimental` | `deb-experimental` | `rpm-experimental-hosted` |
+| push to `develop`        | `xrpld --version` | `develop`      | `deb-develop`      | `rpm-develop-hosted`      |
+| tag, non-public codebase | _any_             | `private`      | `deb-private`      | `rpm-private-hosted`      |
 
 Only a tag names a channel — do not extend that to `develop`, where
 `BuildInfo.cpp`'s `versionString` moves through `-bN`, `-rcN` and even the final
@@ -155,12 +155,15 @@ Conan remote.
 
 Nexus owns the repository metadata; nothing here indexes anything. Worth knowing:
 
-- Each apt-hosted repository needs a distribution and a PGP signing keypair
-  configured in Nexus, which rejects one created without a keypair. Nexus signs
-  the apt metadata with it, never the packages.
-- Hosted yum repositories cannot be signed by Nexus at all, so `sign_rpm.sh`
-  signs the RPMs before they are uploaded, and rpm clients verify with
-  `gpgcheck=1` rather than `repo_gpgcheck=1`.
+- Each apt-hosted repository needs a distribution (ours use `any`) and a PGP
+  signing keypair configured in Nexus, which rejects one created without a
+  keypair. Nexus signs the apt metadata with it, never the packages.
+- Hosted yum repositories cannot be signed by Nexus, so each `rpm-<channel>-hosted`
+  repository sits behind a `rpm-<channel>` yum group repository whose metadata
+  Nexus signs. Uploads go to the hosted repository; clients point at the group
+  and verify the metadata with `repo_gpgcheck=1`. Nexus never signs the RPMs
+  themselves, so `sign_rpm.sh` signs them before they are uploaded, and clients
+  verify them with `gpgcheck=1`.
 - yum metadata is rebuilt asynchronously, so a successful publish is not
   immediately installable.
 - Each job uploads only what it built, and uploads are not transactional, so a

package/publish_pkg.sh

@@ -7,10 +7,13 @@ set -euo pipefail
 # Usage: publish_pkg.sh <channel> [package-dir]
 #
 #   channel      release channel, selecting the 'deb-<channel>' and
-#                'rpm-<channel>' repository pair
+#                'rpm-<channel>-hosted' repositories
 #   package-dir  searched recursively for *.deb, *.ddeb and *.rpm ('build' by
 #                default)
 #
+# RPMs are uploaded to the hosted repository, but yum clients install from the
+# 'rpm-<channel>' group repository in front of it, which serves signed metadata.
+#
 # NEXUS_USERNAME and NEXUS_PASSWORD are required. NEXUS_URL overrides the target
 # instance, and DRY_RUN=1 lists the uploads without performing them.
 
@@ -24,7 +27,7 @@ if [[ -z "${channel}" ]]; then
 fi
 
 deb_repo="deb-${channel}"
-rpm_repo="rpm-${channel}"
+rpm_repo="rpm-${channel}-hosted"
 
 if [[ -z "${DRY_RUN:-}" ]]; then
     : "${NEXUS_USERNAME:?is required}" "${NEXUS_PASSWORD:?is required}"

package/sign_rpm.sh

@@ -1,9 +1,10 @@
 #!/usr/bin/env bash
 set -euo pipefail
 
-# Sign the RPMs built by build_pkg.sh. Nexus cannot sign hosted yum metadata, so
-# the packages carry the signature themselves and rpm clients verify them with
-# gpgcheck=1.
+# Sign the RPMs built by build_pkg.sh. Nexus signs the yum repository metadata
+# (via the 'rpm-<channel>' group repository), but never the packages themselves,
+# so they carry their own signature. Clients verify the packages with gpgcheck=1
+# and the metadata with repo_gpgcheck=1.
 #
 # Usage: sign_rpm.sh [package-dir]
 #
@@ -12,8 +13,9 @@ set -euo pipefail
 # PKG_SIGNING_KEY must hold an armoured PGP private key. It has no flag, to keep
 # the key out of the process list.
 #
-# There is no DEB equivalent: apt trusts the repository metadata, which Nexus
-# signs, rather than the packages themselves.
+# The DEBs are deliberately not signed: embedded DEB signatures exist (debsigs),
+# but apt does not verify them by default and trusts the repository metadata,
+# which Nexus signs, instead.
 
 pkg_dir="${1:-build}"
 

src/libxrpl/ledger/Ledger.cpp

@@ -202,14 +202,7 @@ Ledger::Ledger(
     }
 
     stateMap_.flushDirty(NodeObjectType::AccountNode);
-    // 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
-    }
+    setImmutable();
 }
 
 Ledger::Ledger(
@@ -220,7 +213,7 @@ Ledger::Ledger(
     Fees const& fees,
     Family& family,
     beast::Journal j)
-    : immutable_(false)
+    : immutable_(true)
     , txMap_(SHAMapType::TRANSACTION, info.txHash, family)
     , stateMap_(SHAMapType::STATE, info.accountHash, family)
     , fees_(fees)
@@ -243,20 +236,8 @@ Ledger::Ledger(
         JLOG(j.warn()) << "Don't have state data root for ledger" << header_.seq;
     }
 
-    // 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
-    }
+    txMap_.setImmutable();
+    stateMap_.setImmutable();
 
     if (!setup())
         loaded = false;
@@ -297,12 +278,8 @@ 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_(false)
-    , mapHashesFromHeader_(true)
+    : immutable_(true)
     , txMap_(SHAMapType::TRANSACTION, info.txHash, family)
     , stateMap_(SHAMapType::STATE, info.accountHash, family)
     , rules_(std::move(rules))
@@ -331,20 +308,12 @@ Ledger::Ledger(
     setup();
 }
 
-bool
+void
 Ledger::setImmutable(bool rehash)
 {
-    // A map found structurally invalid during sync must never be made immutable: isValid() tests
-    // only for Invalid, and an immutable ledger is treated as persistable. Asked before anything is
-    // written, so a refusal leaves the header exactly as it was rather than half 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)
+    // Force update, since this is the only
+    // place the hash transitions to valid
+    if (!immutable_ && rehash)
     {
         header_.txHash = txMap_.getHash().asUInt256();
         header_.accountHash = stateMap_.getHash().asUInt256();
@@ -353,22 +322,13 @@ 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;
 }
 
-bool
+void
 Ledger::setAccepted(
     NetClock::time_point closeTime,
     NetClock::duration closeResolution,
@@ -380,17 +340,7 @@ Ledger::setAccepted(
     header_.closeTime = closeTime;
     header_.closeTimeResolution = closeResolution;
     header_.closeFlags = correctCloseTime ? 0 : kSLcfNoConsensusTime;
-    // 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;
+    setImmutable();
 }
 
 bool

src/libxrpl/shamap/SHAMap.cpp

@@ -26,7 +26,6 @@
 
 #include <boost/smart_ptr/intrusive_ptr.hpp>
 
-#include <atomic>
 #include <cstdint>
 #include <exception>
 #include <functional>
@@ -78,25 +77,14 @@ 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 ((ownState != SHAMapState::Immutable) || (otherState != SHAMapState::Immutable))
+    if ((state_ != SHAMapState::Immutable) || (other.state_ != SHAMapState::Immutable))
     {
         unshare();
     }
@@ -117,7 +105,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");
 
@@ -177,7 +165,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);
 }
 
@@ -190,15 +178,10 @@ SHAMap::finishFetch(SHAMapHash const& hash, std::shared_ptr<NodeObject> const& o
     {
         if (!object)
         {
-            // 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))
+            if (full_)
             {
-                f_.missingNodeAcquireBySeq(ledgerSeq(), hash.asUInt256());
+                full_ = false;
+                f_.missingNodeAcquireBySeq(ledgerSeq_, hash.asUInt256());
             }
             return {};
         }
@@ -231,7 +214,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);
             }
@@ -411,7 +394,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);
@@ -436,7 +419,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;
@@ -690,7 +673,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);
@@ -772,7 +755,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
@@ -863,7 +846,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);
@@ -954,7 +937,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,7 +16,6 @@
 #include <xrpl/shamap/detail/TaggedPointer.h>
 #include <xrpl/shamap/detail/TaggedPointer.ipp>
 
-#include <atomic>
 #include <cstddef>
 #include <cstdint>
 #include <mutex>
@@ -78,10 +77,7 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const
     auto p = intr_ptr::makeShared<SHAMapInnerNode>(cowid, branchCount);
     p->hash_ = hash_;
     p->isBranch_ = isBranch_;
-    // 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);
+    p->fullBelowGen_ = fullBelowGen_;
     SHAMapHash* cloneHashes = nullptr;
     SHAMapHash* thisHashes = nullptr;
     SHAMapTreeNodePtr* cloneChildren = nullptr;

src/libxrpl/shamap/SHAMapSync.cpp

@@ -31,26 +31,6 @@
 
 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)
@@ -213,13 +193,7 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se)
             // we already know this child node is missing
             fullBelow = false;
         }
-        // The depth test precedes the cache lookup for the same reason it does in addKnownNode():
-        // the cache is keyed by node hash and shared across maps, and a hash covers a node's
-        // children but not its depth, 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()))
+        else if (!backed_ || !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
         {
             bool pending = false;
             auto d = descendAsync(
@@ -250,18 +224,6 @@ 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);
@@ -335,23 +297,17 @@ 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,
@@ -384,11 +340,6 @@ 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;
 
@@ -418,11 +369,6 @@ 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);
 
@@ -456,12 +402,6 @@ 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();
 
@@ -601,7 +541,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();
@@ -644,15 +584,7 @@ SHAMap::addKnownNode(
         }
 
         auto childHash = inner->getChildHash(branch);
-
-        // 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()))
+        if (f_.getFullBelowCache()->touchIfExists(childHash.asUInt256()))
         {
             return SHAMapAddNode::duplicate();
         }
@@ -670,37 +602,25 @@ SHAMap::addKnownNode(
             return SHAMapAddNode::invalid();
         }
 
-        // 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)
+        // 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))
         {
-            JLOG(journal_.warn()) << "Node " << nodeID << " makes the map invalid at "
-                                  << currNodeID;
-            setInvalid();
-            return SHAMapAddNode::invalid();
+            // Map is provably invalid
+            state_ = SHAMapState::Invalid;
+            return SHAMapAddNode::useful();
         }
 
-        // 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)
+        if (currNodeID != nodeID)
         {
-            JLOG(journal_.warn()) << "Unable to hook node " << nodeID << ", stuck at "
-                                  << currNodeID;
-            return SHAMapAddNode::invalid();
+            // 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();
         }
 
         if (backed_)
@@ -713,7 +633,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();
@@ -834,7 +754,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 (isLeafDepth(nodeID.getDepth()))
+        if (nodeID.getDepth() >= kLeafDepth)
         {
             // LCOV_EXCL_START
             UNREACHABLE("xrpl::SHAMap::hasLeafNode : inner node at leaf depth");

src/test/app/AcquireTestHelpers.h

@@ -1,465 +0,0 @@
-#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

@@ -1,738 +0,0 @@
-#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,7 +11,6 @@
 
 #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>
@@ -23,7 +22,6 @@
 
 #include <cassert>
 #include <memory>
-#include <stdexcept>
 #include <vector>
 
 namespace xrpl::test {
@@ -72,16 +70,11 @@ public:
         }
         res->unshare();
 
-        // 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");
-        }
+        // Accept ledger
+        res->setAccepted(
+            res->header().closeTime,
+            res->header().closeTimeResolution,
+            true /* close time correct*/);
         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)
             {
-                BEAST_EXPECT(next->setImmutable());
+                next->setImmutable();
                 forceHash = false;
             }
 

src/test/app/TransactionAcquire_test.cpp

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

src/tests/libxrpl/shamap/DeepChain.h

@@ -1,322 +0,0 @@
-#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

@@ -1,79 +0,0 @@
-#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,83 +1,30 @@
-#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:
@@ -134,824 +81,8 @@ 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_};
@@ -961,6 +92,7 @@ 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)
     {
@@ -973,7 +105,7 @@ TEST_F(SHAMapSyncTest, sync)
     ASSERT_TRUE(confuseMap(source, kNodesToConfuse));
     source.invariants();
 
-    ASSERT_TRUE(source.setImmutable());
+    source.setImmutable();
 
     std::size_t count = 0;
     source.visitLeaves([&count]([[maybe_unused]] auto const& item) { ++count; });

src/tests/libxrpl/shamap/common.h

@@ -14,9 +14,7 @@
 #include <xrpl/shamap/FullBelowCache.h>
 #include <xrpl/shamap/TreeNodeCache.h>
 
-#include <atomic>
 #include <chrono>
-#include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <stdexcept>
@@ -36,18 +34,8 @@ 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:
-    /**
-     * @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)
+    TestNodeFamily(beast::Journal j)
         : fbCache_(std::make_shared<FullBelowCache>("App family full below cache", clock_, j))
         , tnCache_(
               std::make_shared<TreeNodeCache>(
@@ -62,7 +50,7 @@ public:
         testSection.set(Keys::kType, "memory");
         testSection.set(Keys::kPath, "SHAMap_test");
         db_ = node_store::Manager::instance().makeDatabase(
-            megabytes(4), scheduler_, readThreads, testSection, j);
+            megabytes(4), scheduler_, 1, testSection, j);
     }
 
     node_store::Database&
@@ -102,29 +90,14 @@ 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(std::uint32_t refNum, [[maybe_unused]] uint256 const& nodeHash) override
+    missingNodeAcquireBySeq(
+        [[maybe_unused]] 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,
@@ -133,31 +106,6 @@ 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() >= kMinTxNodeBytesToParse))
+    if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() > 16))
     {
         // 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,7 +9,6 @@
 #include <xrpl/shamap/SHAMapSyncFilter.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
-#include <cstddef>
 #include <cstdint>
 #include <optional>
 
@@ -25,18 +24,6 @@ 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 : public TimeoutCounter,
-                      public std::enable_shared_from_this<InboundLedger>,
-                      public CountedObject<InboundLedger>
+class InboundLedger final : public TimeoutCounter,
+                            public std::enable_shared_from_this<InboundLedger>,
+                            public CountedObject<InboundLedger>
 {
 public:
     using clock_type = beast::AbstractClock<std::chrono::steady_clock>;
@@ -44,33 +44,13 @@ 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& clock,
-        std::unique_ptr<PeerSet> peerSet,
-        std::chrono::milliseconds retryInterval = kRetryInterval);
+        clock_type&,
+        std::unique_ptr<PeerSet> peerSet);
 
     ~InboundLedger() override;
 
@@ -79,13 +59,7 @@ public:
     update(std::uint32_t seq);
 
     /**
-     * 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.
+     * Returns true if we got all the data.
      */
     bool
     isComplete() const
@@ -145,40 +119,15 @@ public:
         return lastAction_;
     }
 
-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.
+private:
     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();
 
@@ -188,23 +137,11 @@ private:
     void
     tryDB(node_store::Database& srcDB);
 
-    /**
-     * Whether either map of the ledger being acquired has been found
-     * invalid.
-     *
-     * A walk returns a bare list of hashes, so 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
+    done();
 
     void
-    onTimer(bool progress, ScopedLockType& sl) override;
+    onTimer(bool progress, ScopedLockType& peerSetLock) override;
 
     std::size_t
     getPeerCount() const;

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

@@ -6,7 +6,6 @@
 
 #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>
@@ -77,22 +76,7 @@ buildLedgerImpl(
     XRPL_ASSERT(
         built->header().seq < kXrpLedgerEarliestFees || built->read(keylet::feeSettings()),
         "xrpl::buildLedgerImpl : valid ledger fees");
-    // 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
-    }
+    built->setAccepted(closeTime, closeResolution, closeTimeCorrect);
 
     return built;
 }

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

@@ -54,6 +54,8 @@
 
 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
@@ -63,18 +65,20 @@ 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::chrono::milliseconds retryInterval)
+    std::unique_ptr<PeerSet> peerSet)
     : TimeoutCounter(
           app,
           hash,
-          retryInterval,
+          kLedgerAcquireTimeout,
           {.jobType = JtLedgerData, .jobName = "InboundLedger", .jobLimit = 5},
           app.getJournal("InboundLedger"))
     , clock_(clock)
@@ -93,13 +97,8 @@ 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_)
     {
@@ -113,19 +112,7 @@ InboundLedger::init(ScopedLockType& collectionLock)
     XRPL_ASSERT(
         ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()),
         "xrpl::InboundLedger::init : valid ledger fees");
-    // tryDB() verified both maps before setting complete_ and mtx_ has been held since, so
-    // nothing can have invalidated them.
-    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
-    }
+    ledger_->setImmutable();
 
     if (reason_ == Reason::HISTORY)
         return;
@@ -235,12 +222,6 @@ 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
@@ -345,34 +326,14 @@ 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");
-        // 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;
+        ledger_->setImmutable();
     }
 }
 
@@ -458,48 +419,6 @@ 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()
@@ -508,7 +427,24 @@ InboundLedger::done()
 
     XRPL_ASSERT(complete_ || failed_, "xrpl::InboundLedger::done : complete or failed");
 
-    // mtx_ is held, so this may only post the work rather than do it.
+    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
     app_.getJobQueue().addJob(JtLedgerData, "AcqDone", [self = shared_from_this()]() {
         if (self->complete_ && !self->failed_)
         {
@@ -561,8 +497,6 @@ 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;
         }
     }
@@ -579,17 +513,7 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         {
             auto need = getNeededHashes();
 
-            // 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())
+            if (!need.empty())
             {
                 protocol::TMGetObjectByHash tmBH;
                 bool typeSet = false;
@@ -626,12 +550,11 @@ 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;
             }
         }
     }
@@ -694,11 +617,7 @@ 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. 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.
+            // Release the lock while we process the large state map
             sl.unlock();
             auto nodes = ledger_->stateMap().getMissingNodes(kMissingNodesFind, &filter);
             sl.lock();
@@ -715,6 +634,9 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
                     else
                     {
                         haveState_ = true;
+
+                        if (haveTransactions_)
+                            complete_ = true;
                     }
                 }
                 else
@@ -777,6 +699,9 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
                 else
                 {
                     haveTransactions_ = true;
+
+                    if (haveState_)
+                        complete_ = true;
                 }
             }
             else
@@ -801,14 +726,11 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
         }
     }
 
-    // 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_))
+    if (complete_ || failed_)
     {
-        JLOG(journal_.debug()) << "Done:" << (failed_ ? " failed " : " have everything ")
-                               << ledger_->header().seq;
+        JLOG(journal_.debug()) << "Done:" << (complete_ ? " complete" : "")
+                               << (failed_ ? " failed " : " ") << ledger_->header().seq;
+        sl.unlock();
         done();
     }
 }
@@ -1006,10 +928,11 @@ 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/LedgerPersistence.cpp

@@ -115,15 +115,8 @@ 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>& ledger, beast::Journal j)
+finishLoadByIndexOrHash(std::shared_ptr<Ledger> const& ledger, beast::Journal j)
 {
     if (!ledger)
         return;
@@ -131,19 +124,7 @@ finishLoadByIndexOrHash(std::shared_ptr<Ledger>& ledger, beast::Journal j)
     XRPL_ASSERT(
         ledger->header().seq < kXrpLedgerEarliestFees || ledger->read(keylet::feeSettings()),
         "xrpl::finishLoadByIndexOrHash : valid ledger fees");
-    // 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
-    }
+    ledger->setImmutable();
 
     JLOG(j.trace()) << "Loaded ledger: " << to_string(ledger->header().hash);
 

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

@@ -98,16 +98,9 @@ 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& sl) = 0;
+    onTimer(bool progress, ScopedLockType&) = 0;
 
     /**
      * Return a weak pointer to this.

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

@@ -8,7 +8,6 @@
 
 #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>
@@ -19,7 +18,6 @@
 #include <xrpl.pb.h>
 
 #include <algorithm>
-#include <chrono>
 #include <cstddef>
 #include <exception>
 #include <memory>
@@ -28,18 +26,22 @@
 
 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::chrono::milliseconds retryInterval)
+    std::unique_ptr<PeerSet> peerSet)
     : TimeoutCounter(
           app,
           hash,
-          retryInterval,
+          kTxAcquireTimeout,
           {.jobType = JtTxnData, .jobName = "TxAcq", .jobLimit = {}},
           app.getJournal("TransactionAcquire"))
     , peerSet_(std::move(peerSet))
@@ -51,36 +53,16 @@ TransactionAcquire::TransactionAcquire(
 void
 TransactionAcquire::done()
 {
-    // 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_.
+    // We hold a PeerSet lock and so cannot do real work here
 
     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_);
@@ -97,7 +79,7 @@ TransactionAcquire::done()
 }
 
 void
-TransactionAcquire::onTimer(bool progress, ScopedLockType&)
+TransactionAcquire::onTimer(bool progress, ScopedLockType& psl)
 {
     if (timeouts_ > kMaxTimeouts)
     {
@@ -275,20 +257,10 @@ TransactionAcquire::init(int numPeers)
 void
 TransactionAcquire::stillNeed()
 {
-    ScopedLockType sl(mtx_);
+    ScopedLockType const 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,7 +11,6 @@
 #include <xrpl/shamap/SHAMapAddNode.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
-#include <chrono>
 #include <cstddef>
 #include <memory>
 #include <utility>
@@ -21,34 +20,14 @@ namespace xrpl {
 
 // VFALCO TODO rename to PeerTxRequest
 // A transaction set we are trying to acquire
-class TransactionAcquire : public TimeoutCounter,
-                           public std::enable_shared_from_this<TransactionAcquire>,
-                           public CountedObject<TransactionAcquire>
+class TransactionAcquire final : public TimeoutCounter,
+                                 public std::enable_shared_from_this<TransactionAcquire>,
+                                 public CountedObject<TransactionAcquire>
 {
 public:
     using pointer = std::shared_ptr<TransactionAcquire>;
 
-    /**
-     * 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(Application& app, uint256 const& hash, std::unique_ptr<PeerSet> peerSet);
     ~TransactionAcquire() override = default;
 
     SHAMapAddNode
@@ -59,35 +38,17 @@ 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();
 
-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:
+    std::shared_ptr<SHAMap> map_;
     bool haveRoot_{false};
     std::unique_ptr<PeerSet> peerSet_;
 
     void
-    onTimer(bool progress, ScopedLockType& sl) override;
+    onTimer(bool progress, ScopedLockType& peerSetLock) 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();
 

src/xrpld/app/main/Application.cpp

@@ -1700,14 +1700,7 @@ ApplicationImp::startGenesisLedger()
     XRPL_ASSERT(
         next->header().seq < kXrpLedgerEarliestFees || next->read(keylet::feeSettings()),
         "xrpl::ApplicationImp::startGenesisLedger : valid ledger fees");
-    // 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
-    }
+    next->setImmutable();
     openLedger_.emplace(next, cachedSLEs_, logs_->journal("OpenLedger"));
     ledgerMaster_->storeLedger(next);
     ledgerMaster_->switchLCL(next);
@@ -1729,16 +1722,7 @@ ApplicationImp::getLastFullLedger()
         XRPL_ASSERT(
             ledger->header().seq < kXrpLedgerEarliestFees || ledger->read(keylet::feeSettings()),
             "xrpl::ApplicationImp::getLastFullLedger : valid ledger fees");
-        // 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
-        }
+        ledger->setImmutable();
 
         if (getLedgerMaster().haveLedger(seq))
             ledger->setValidated();
@@ -1890,16 +1874,7 @@ ApplicationImp::loadLedgerFromFile(std::string const& name)
             loadLedger->header().seq < kXrpLedgerEarliestFees ||
                 loadLedger->read(keylet::feeSettings()),
             "xrpl::ApplicationImp::loadLedgerFromFile : valid ledger fees");
-        // 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
-        }
+        loadLedger->setAccepted(closeTime, closeTimeResolution, !closeTimeEstimated);
 
         return loadLedger;
     }