AI review feedback: clean up, enforce parameter validity, tests

Co-authored-by: Marek Foss <marek.foss@neti-soft.com>
Co-authored-by: Alex Kremer <akremer@ripple.com>

.cspell.config.yaml

@@ -262,6 +262,8 @@ words:
   - Rohrs
   - roundings
   - rustc
+  - rustfmt
+  - rustup
   - sahyadri
   - Satoshi
   - scons
@@ -305,6 +307,7 @@ words:
   - takerpays
   - ters
   - TMEndpointv2
+  - toolchain
   - tparam
   - trixie
   - tx

.gersemi/definitions.cmake

@@ -11,6 +11,9 @@ endfunction()
 function(create_symbolic_link target link)
 endfunction()
 
+function(xrpl_add_benchmark name)
+endfunction()
+
 macro(exclude_from_default target_)
 endmacro()
 

.github/scripts/levelization/results/ordering.txt

@@ -1,3 +1,6 @@
+benchmarks.libxrpl > xrpl.basics
+benchmarks.libxrpl > xrpl.config
+benchmarks.libxrpl > xrpl.nodestore
 libxrpl.basics > xrpl.basics
 libxrpl.conditions > xrpl.basics
 libxrpl.conditions > xrpl.conditions

.github/scripts/strategy-matrix/linux.json

@@ -1,5 +1,5 @@
 {
-  "image_tag": "sha-e29b523",
+  "image_tag": "sha-2e25435",
   "configs": {
     "ubuntu": [
       {

.github/workflows/publish-docs.yml

@@ -41,13 +41,13 @@ env:
 jobs:
   build:
     runs-on: ubuntu-latest
-    container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-e29b523
+    container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-2e25435
     steps:
       - name: Checkout repository
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c
+        uses: XRPLF/actions/prepare-runner@ad188deb3dae79dc39816e16ddfdad1e06c6fab2
         with:
           enable_ccache: false
 

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

@@ -113,7 +113,7 @@ jobs:
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c
+        uses: XRPLF/actions/prepare-runner@ad188deb3dae79dc39816e16ddfdad1e06c6fab2
         with:
           enable_ccache: ${{ inputs.ccache_enabled }}
 
@@ -324,6 +324,23 @@ jobs:
 
           LD_PRELOAD="$PRELOAD" ./xrpld --unittest --unittest-jobs "${BUILD_NPROC}" 2>&1 | tee unittest.log
 
+      # Smoke-run every benchmark module with a single repetition to confirm the
+      # benchmarks still build and execute. This is a correctness check, not a
+      # performance measurement, so it is skipped for instrumented builds
+      # (sanitizers/coverage/voidstar), where it would be slow and meaningless,
+      # and on Windows, where the `install` target does not build them.
+      - name: Run the benchmarks
+        if: ${{ !inputs.build_only && runner.os != 'Windows' && env.SANITIZERS_ENABLED == 'false' && env.COVERAGE_ENABLED != 'true' && env.VOIDSTAR_ENABLED != 'true' }}
+        working-directory: ${{ env.BUILD_DIR }}
+        run: |
+          rc=0
+          while IFS= read -r bench; do
+              echo "::group::${bench}"
+              "./${bench}" --benchmark_repetitions=1 || rc=1
+              echo "::endgroup::"
+          done < <(find src/benchmarks -type f -perm -u+x -name 'xrpl.bench.*')
+          exit "${rc}"
+
       - name: Show test failure summary
         if: ${{ failure() && !inputs.build_only }}
         env:

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

@@ -34,7 +34,7 @@ jobs:
     needs: [determine-files]
     if: ${{ needs.determine-files.outputs.cpp_changed_files != '' || needs.determine-files.outputs.need_full_run == 'true' }}
     runs-on: ["self-hosted", "Linux", "X64", "heavy"]
-    container: "ghcr.io/xrplf/xrpld/nix-debian:sha-e29b523"
+    container: "ghcr.io/xrplf/xrpld/nix-debian:sha-2e25435"
     permissions:
       contents: read
       issues: write
@@ -43,7 +43,7 @@ jobs:
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c
+        uses: XRPLF/actions/prepare-runner@ad188deb3dae79dc39816e16ddfdad1e06c6fab2
         with:
           enable_ccache: false
 

.github/workflows/reusable-upload-recipe.yml

@@ -40,7 +40,7 @@ defaults:
 jobs:
   upload:
     runs-on: ubuntu-latest
-    container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-e29b523
+    container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-2e25435
     env:
       REMOTE_NAME: ${{ inputs.remote_name }}
       CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.remote_username }}

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

@@ -68,7 +68,7 @@ jobs:
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c
+        uses: XRPLF/actions/prepare-runner@ad188deb3dae79dc39816e16ddfdad1e06c6fab2
         with:
           enable_ccache: false
 

CMakeLists.txt

@@ -131,6 +131,10 @@ else()
 endif()
 target_link_libraries(xrpl_libs INTERFACE ${nudb})
 
+if(benchmark)
+    find_package(benchmark REQUIRED)
+endif()
+
 if(coverage)
     include(XrplCov)
 endif()
@@ -145,3 +149,7 @@ if(tests)
     include(CTest)
     add_subdirectory(src/tests/libxrpl)
 endif()
+
+if(benchmark)
+    add_subdirectory(src/benchmarks/libxrpl)
+endif()

CONTRIBUTING.md

@@ -83,6 +83,7 @@ If you create new source files, they must be organized as follows:
   `src/libxrpl`.
 - All other non-test files must go under `src/xrpld`.
 - All test source files must go under `src/test`.
+- All benchmark source files must go under `src/benchmarks`.
 
 The source must be formatted according to the style guide below. The easiest
 way to satisfy this is to install the [`pre-commit`](#pre-commit-hooks) hooks,

cfg/xrpld-example.cfg

@@ -1059,10 +1059,11 @@
 #                           The online delete process checks periodically
 #                           that xrpld is still in sync with the network,
 #                           and that the validated ledger is less than
-#                           'age_threshold_seconds' old. If not, then continue
+#                           'age_threshold_seconds' old, and that all
+#                           recent ledgers are available. If not, then continue
 #                           sleeping for this number of seconds and
 #                           checking until healthy.
-#                           Default is 5.
+#                           Default is 2.
 #
 #   Notes:
 #       The 'node_db' entry configures the primary, persistent storage.

cmake/XrplAddBenchmark.cmake

@@ -0,0 +1,36 @@
+include(isolate_headers)
+
+# Define a benchmark executable for the module `name`.
+#
+# This follows the same general pattern as other build helpers in this repo
+# (e.g. `add_module`): create a target and isolate headers, but here the target
+# is a benchmark executable and no `add_test(...)` is registered.
+#
+# `isolate_headers` exposes only `${CMAKE_CURRENT_SOURCE_DIR}/${name}` on the
+# include path, rooted at `src`, so a benchmark's own headers are reached as
+# `<benchmarks/.../${name}/...>` and nothing else in the tree leaks in.
+function(xrpl_add_benchmark name)
+    set(target ${PROJECT_NAME}.bench.${name})
+
+    file(
+        GLOB_RECURSE sources
+        CONFIGURE_DEPENDS
+        "${CMAKE_CURRENT_SOURCE_DIR}/${name}/*.cpp"
+        "${CMAKE_CURRENT_SOURCE_DIR}/${name}.cpp"
+    )
+    add_executable(${target} ${ARGN} ${sources})
+
+    # Benchmark sources register cases through Google Benchmark's static
+    # registrars (anonymous-namespace lambdas). Merging several such files into
+    # one unity translation unit collides those internal-linkage entities, so
+    # keep benchmarks out of the unity build - mirroring xrpl.libpb in
+    # XrplCore.cmake. Each file compiles fine on its own.
+    set_target_properties(${target} PROPERTIES UNITY_BUILD OFF)
+
+    isolate_headers(
+        ${target}
+        "${CMAKE_SOURCE_DIR}/src"
+        "${CMAKE_CURRENT_SOURCE_DIR}/${name}"
+        PRIVATE
+    )
+endfunction()

cmake/XrplSettings.cmake

@@ -30,6 +30,8 @@ if(tests)
     endif()
 endif()
 
+option(benchmark "Build benchmarks" ON)
+
 # Enabled by default so every header is compiled on its own as the main file of
 # its own compile_commands.json entry - this is what lets clang-tidy (and clangd
 # and IDEs) analyse a header's own includes directly. The per-header objects are

conan.lock

@@ -25,6 +25,7 @@
         "c-ares/1.34.6#545240bb1c40e2cacd4362d6b8967650%1782392402.681654",
         "bzip2/1.0.8#c470882369c2d95c5c77e970c0c7e321%1782392402.296732",
         "boost/1.91.0#ea540ca2133d831b560036aa24dece3c%1782392419.475605",
+        "benchmark/1.9.5#b885dc73ad67b40a55d45684d1c88ad1%1782736613.864841",
         "abseil/20250127.0#9ef01c1451a8340f9022e46238c0fbb6%1783945159.651047"
     ],
     "build_requires": [

conanfile.py

@@ -15,6 +15,7 @@ class Xrpl(ConanFile):
     settings = "os", "compiler", "build_type", "arch"
     options = {
         "assertions": [True, False],
+        "benchmark": [True, False],
         "coverage": [True, False],
         "fPIC": [True, False],
         "jemalloc": [True, False],
@@ -46,6 +47,7 @@ class Xrpl(ConanFile):
 
     default_options = {
         "assertions": False,
+        "benchmark": True,
         "coverage": False,
         "fPIC": True,
         "jemalloc": False,
@@ -129,6 +131,8 @@ class Xrpl(ConanFile):
             self.options["boost"].without_cobalt = True
 
     def requirements(self):
+        if self.options.benchmark:
+            self.requires("benchmark/1.9.5")
         self.requires("boost/1.91.0", force=True, transitive_headers=True)
         self.requires("date/3.0.4", transitive_headers=True)
         if self.options.jemalloc:
@@ -162,6 +166,7 @@ class Xrpl(ConanFile):
     def generate(self):
         tc = CMakeToolchain(self)
         tc.variables["tests"] = self.options.tests
+        tc.variables["benchmark"] = self.options.benchmark
         tc.variables["assert"] = self.options.assertions
         tc.variables["coverage"] = self.options.coverage
         tc.variables["jemalloc"] = self.options.jemalloc

include/xrpl/ledger/helpers/LendingHelpers.h

@@ -263,10 +263,11 @@ constructLoanState(
     Number const& principalOutstanding,
     Number const& managementFeeOutstanding);
 
-// Constructs a valid LoanState object from a Loan object, which always has
-// rounded values
+// Overload of constructLoanState() that reads the three tracked fields
+// directly from a Loan ledger object, which always holds rounded values,
+// rather than taking them as separate Number arguments.
 LoanState
-constructRoundedLoanState(SLE::const_ref loan);
+constructLoanState(SLE::const_ref loan);
 
 Number
 computeManagementFee(

rust-toolchain.toml

@@ -0,0 +1,8 @@
+# Rust toolchain pin for rustup-based CI runners and local development.
+# rustup reads this file and installs the pinned toolchain (see the
+# prepare-runner action in XRPLF/actions, which runs `rustup toolchain install`).
+# NOTE: the Nix CI image and development shell ignore this file; its rustc comes from flake.lock.
+[toolchain]
+channel = "1.95"
+components = ["rustfmt", "clippy"]
+profile = "minimal"

src/benchmarks/libxrpl/CMakeLists.txt

@@ -0,0 +1,22 @@
+include(XrplAddBenchmark)
+
+# Benchmark requirements.
+find_package(benchmark REQUIRED)
+
+# Custom target for all benchmarks defined in this file.
+add_custom_target(xrpl.benchmarks)
+
+# Common library dependencies for every benchmark module. `benchmark_main`
+# supplies a `main()` that parses the standard Google Benchmark CLI flags
+# (`--benchmark_filter`, `--benchmark_format`, ...), so no per-module main.cpp
+# is needed.
+add_library(xrpl.imports.bench INTERFACE)
+target_link_libraries(
+    xrpl.imports.bench
+    INTERFACE benchmark::benchmark_main xrpl.libxrpl
+)
+
+# One benchmark executable for each module.
+xrpl_add_benchmark(nodestore)
+target_link_libraries(xrpl.bench.nodestore PRIVATE xrpl.imports.bench)
+add_dependencies(xrpl.benchmarks xrpl.bench.nodestore)

src/benchmarks/libxrpl/nodestore/Backend.cpp

@@ -0,0 +1,329 @@
+#include <xrpl/nodestore/Backend.h>
+
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/nodestore/NodeObject.h>
+#include <xrpl/nodestore/Types.h>
+
+#include <benchmark/benchmark.h>
+#include <benchmarks/libxrpl/nodestore/NodeStoreBench.h>
+
+#include <array>
+#include <cstddef>
+#include <cstdint>
+#include <functional>
+#include <memory>
+#include <string>
+#include <string_view>
+#include <utility>
+#include <vector>
+
+namespace xrpl::NodeStore {
+namespace {
+
+constexpr std::size_t kPoolSizes[] = {1000, 10000, 100000};
+constexpr int kThreadCounts[] = {1, 4, 8};
+constexpr std::size_t kBatchSize = 256;
+
+constexpr std::string_view kNamePrefix = "BM_Backend_";
+constexpr std::string_view kNameSeparator = "/";
+
+struct RunState
+{
+    std::unique_ptr<BackendHarness> harness;
+    Batch present;                     // prefix-1 objects, eligible to be stored
+    Batch recent;                      // prefix-1 objects in the "future" key space
+    std::vector<uint256> missing;      // prefix-2 keys that are never stored
+    std::vector<std::size_t> shuffle;  // [0, poolSize) permutation for random-like access
+    std::size_t avgPayload = 0;        // mean getData().size() over `present`
+
+    void
+    release()
+    {
+        harness.reset();
+        Batch{}.swap(present);
+        Batch{}.swap(recent);
+        std::vector<uint256>{}.swap(missing);
+        std::vector<std::size_t>{}.swap(shuffle);
+    }
+};
+
+struct SetupContext
+{
+    RunState& rs;
+    Backend& backend;
+    std::size_t poolSize;
+};
+
+struct IterateContext
+{
+    RunState& rs;
+    Backend& backend;
+    std::size_t index;
+    std::size_t poolSize;
+};
+
+struct Workload
+{
+    std::string_view name;
+    std::function<void(SetupContext const&)> setup;
+    std::function<void(IterateContext const&)> iterate;
+    bool reportBytes = false;  // SetBytesProcessed from rs.avgPayload
+    bool clobber = true;       // ClobberMemory after the loop (false for pure stores)
+    bool pinToPool = false;    // pin iterations to one pool sweep instead of autotuning
+};
+
+// One store() per iteration. Iterations are pinned to one pool sweep (per
+// thread) so the index never wraps past the pool - otherwise NuDB::doInsert
+// swallows key_exists and the workload degenerates into duplicate-detection
+// no-ops.
+Workload const kInsert{
+    .name = "Insert",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.avgPayload = averagePayload(ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, backend, index, poolSize] = ctx;
+            backend.store(rs.present[index % poolSize]);
+        },
+    .reportBytes = true,
+    .clobber = false,
+    .pinToPool = true,
+};
+
+// One fetch() of a present key (a hit) per iteration.
+Workload const kFetch{
+    .name = "Fetch",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.avgPayload = averagePayload(ctx.rs.present);
+            prepopulate(ctx.backend, ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, backend, index, poolSize] = ctx;
+            std::shared_ptr<NodeObject> result;
+            backend.fetch(rs.present[index % poolSize]->getHash(), &result);
+            benchmark::DoNotOptimize(result);
+        },
+    .reportBytes = true,
+};
+
+// One fetch() of a never-stored key (a miss); the backend is left empty.
+Workload const kMissing{
+    .name = "Missing",
+    .setup = [](SetupContext const& ctx) { ctx.rs.missing = makeMissingKeys(ctx.poolSize); },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, backend, index, poolSize] = ctx;
+            std::shared_ptr<NodeObject> result;
+            backend.fetch(rs.missing[index % poolSize], &result);
+            benchmark::DoNotOptimize(result);
+        },
+};
+
+// 80% hits / 20% misses. The fetch index comes from a shuffle table so access
+// is random-like without per-iteration RNG cost; sequential `index % poolSize`
+// would be artificially cache-friendly to RocksDB's block cache.
+Workload const kMixed{
+    .name = "Mixed",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.missing = makeMissingKeys(ctx.poolSize);
+            ctx.rs.shuffle = makeShuffle(ctx.poolSize, /*seed=*/1);
+            prepopulate(ctx.backend, ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, backend, index, poolSize] = ctx;
+            std::shared_ptr<NodeObject> result;
+            auto const pick = rs.shuffle[index % poolSize];
+            if (index % 5 == 0)
+            {
+                backend.fetch(rs.missing[pick], &result);
+            }
+            else
+            {
+                backend.fetch(rs.present[pick]->getHash(), &result);
+            }
+            benchmark::DoNotOptimize(result);
+        },
+};
+
+// An xrpld-like cycle: a hit, a maybe-miss recent fetch, and a store. The
+// recent fetch uses the shuffle table (not `slot`) so it doesn't fetch the item
+// it's about to store this iteration - which would give an all-miss-then-hit
+// step instead of a smooth ramp. The store walks sequentially so each recent
+// object is stored once.
+Workload const kWork{
+    .name = "Work",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.recent = makePool(1, ctx.poolSize, ctx.poolSize);
+            ctx.rs.shuffle = makeShuffle(ctx.poolSize, /*seed=*/2);
+            prepopulate(ctx.backend, ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, backend, index, poolSize] = ctx;
+            auto const slot = index % poolSize;
+            auto const pick = rs.shuffle[slot];
+
+            std::shared_ptr<NodeObject> historical;
+            backend.fetch(rs.present[pick]->getHash(), &historical);
+            benchmark::DoNotOptimize(historical);
+
+            std::shared_ptr<NodeObject> recent;
+            backend.fetch(rs.recent[pick]->getHash(), &recent);
+            benchmark::DoNotOptimize(recent);
+
+            backend.store(rs.recent[slot]);
+        },
+    .clobber = true,
+    .pinToPool = true,
+};
+
+auto
+makeRunner(Workload w, std::string cfg, std::shared_ptr<RunState> rs)
+{
+    return [w = std::move(w), cfg = std::move(cfg), rs = std::move(rs)](benchmark::State& state) {
+        auto const poolSize = static_cast<std::size_t>(state.range(0));
+        if (state.thread_index() == 0)
+        {
+            rs->harness = std::make_unique<BackendHarness>(cfg);
+            w.setup(
+                SetupContext{.rs = *rs, .backend = *rs->harness->backend, .poolSize = poolSize});
+        }
+
+        std::size_t index = state.thread_index();
+        for (auto _ : state)
+        {
+            w.iterate(
+                IterateContext{
+                    .rs = *rs,
+                    .backend = *rs->harness->backend,
+                    .index = index,
+                    .poolSize = poolSize});
+            index += state.threads();
+        }
+
+        if (w.clobber)
+            benchmark::ClobberMemory();
+
+        state.SetItemsProcessed(state.iterations());
+        if (w.reportBytes)
+            state.SetBytesProcessed(static_cast<std::int64_t>(state.iterations() * rs->avgPayload));
+
+        if (state.thread_index() == 0)
+            rs->release();
+    };
+}
+
+// Register workload `w` against backend `bc`, choosing the registration shape
+// from `w.pinToPool`.
+void
+registerWorkload(BackendConfig const& bc, Workload const& w)
+{
+    std::string const cfg = bc.config;
+    std::string name{kNamePrefix};
+    name += w.name;
+    name += kNameSeparator;
+    name += bc.name;
+
+    if (!w.pinToPool)
+    {
+        auto rs = std::make_shared<RunState>();
+        auto* b = benchmark::RegisterBenchmark(name, makeRunner(w, cfg, rs));
+        b->RangeMultiplier(10)->Range(kPoolSizes[0], kPoolSizes[std::size(kPoolSizes) - 1]);
+        b->Threads(1)->Threads(4)->Threads(8)->UseRealTime();
+
+        return;
+    }
+
+    for (auto const poolSize : kPoolSizes)
+    {
+        for (auto const threads : kThreadCounts)
+        {
+            if (poolSize % static_cast<std::size_t>(threads) != 0)
+                continue;
+
+            auto rs = std::make_shared<RunState>();
+            benchmark::RegisterBenchmark(name, makeRunner(w, cfg, rs))
+                ->Arg(poolSize)
+                ->Iterations(poolSize / static_cast<std::size_t>(threads))
+                ->Threads(threads)
+                ->UseRealTime();
+        }
+    }
+}
+
+// One storeBatch() of kBatchSize objects per iteration. Single-threaded:
+// Backend::storeBatch must not run concurrently with itself or store().
+// Iterations are pinned to the batch count so the index never wraps into
+// key_exists no-ops. Kept separate from Workload: batch slicing and the
+// per-batch item/byte accounting don't fit the thread-axis mold.
+void
+registerStoreBatch(BackendConfig const& bc)
+{
+    std::string const cfg = bc.config;
+    std::string name{kNamePrefix};
+    name += "StoreBatch";
+    name += kNameSeparator;
+    name += bc.name;
+    for (auto const poolSize : kPoolSizes)
+    {
+        auto const numBatches = poolSize / kBatchSize;
+        if (numBatches == 0)
+            continue;
+
+        auto rs = std::make_shared<RunState>();
+        benchmark::RegisterBenchmark(
+            name,
+            [rs, cfg](benchmark::State& state) {
+                auto const poolSize = static_cast<std::size_t>(state.range(0));
+                rs->harness = std::make_unique<BackendHarness>(cfg);
+                rs->present = makePool(1, poolSize);
+                rs->avgPayload = averagePayload(rs->present);
+                std::vector<Batch> const batches = sliceBatches(rs->present, kBatchSize);
+                if (batches.empty())
+                {
+                    state.SkipWithError("pool smaller than one batch");
+                    return;
+                }
+
+                std::size_t index = 0;
+                for (auto _ : state)
+                {
+                    rs->harness->backend->storeBatch(batches[index % batches.size()]);
+                    ++index;
+                }
+
+                state.SetItemsProcessed(static_cast<std::int64_t>(state.iterations() * kBatchSize));
+                state.SetBytesProcessed(
+                    static_cast<std::int64_t>(state.iterations() * kBatchSize * rs->avgPayload));
+                rs->release();
+            })
+            ->Arg(poolSize)
+            ->Iterations(numBatches);
+    }
+}
+
+[[maybe_unused]] bool const kRegistered = [] {
+    auto const workloads = std::to_array({&kInsert, &kFetch, &kMissing, &kMixed, &kWork});
+    for (auto const& bc : backendConfigs())
+    {
+        for (auto const* w : workloads)
+            registerWorkload(bc, *w);
+
+        registerStoreBatch(bc);
+    }
+    return true;
+}();
+
+}  // namespace
+}  // namespace xrpl::NodeStore

src/benchmarks/libxrpl/nodestore/Database.cpp

@@ -0,0 +1,243 @@
+#include <xrpl/nodestore/Database.h>
+
+#include <xrpl/basics/Blob.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/nodestore/NodeObject.h>
+#include <xrpl/nodestore/Types.h>
+
+#include <benchmark/benchmark.h>
+#include <benchmarks/libxrpl/nodestore/NodeStoreBench.h>
+
+#include <array>
+#include <cstddef>
+#include <cstdint>
+#include <functional>
+#include <memory>
+#include <string>
+#include <string_view>
+#include <utility>
+#include <vector>
+
+namespace xrpl::NodeStore {
+namespace {
+
+// Number of distinct objects pre-generated per run.
+constexpr std::size_t kDefaultPoolSize = 100000;
+
+// Async read threads the Database spawns. Unused by the synchronous fetch path
+// these benchmarks take; kept fixed so runs are comparable.
+constexpr int kReadThreads = 4;
+
+constexpr std::string_view kNamePrefix = "BM_Database_";
+constexpr std::string_view kNameSeparator = "/";
+
+struct RunState
+{
+    std::unique_ptr<DatabaseHarness> harness;
+    Batch present;                     // prefix-1 objects, eligible to be stored
+    Batch recent;                      // prefix-1 objects in the "future" key space
+    std::vector<uint256> missing;      // prefix-2 keys that are never stored
+    std::vector<std::size_t> shuffle;  // [0, poolSize) permutation for random-like access
+    std::size_t avgPayload = 0;        // mean getData().size() over `present`
+};
+
+struct SetupContext
+{
+    RunState& rs;
+    Database& db;
+    std::size_t poolSize;
+};
+
+struct IterateContext
+{
+    RunState& rs;
+    Database& db;
+    std::uint32_t seq;
+    std::size_t index;
+    std::size_t poolSize;
+};
+
+struct Workload
+{
+    std::string_view name;
+    std::function<void(SetupContext const&)> setup;
+    std::function<void(IterateContext const&)> iterate;
+    bool reportBytes = false;
+    bool pinIterations = false;
+};
+
+void
+prepopulate(Database& db, Batch const& objects)
+{
+    auto const seq = db.earliestLedgerSeq();
+    for (auto const& obj : objects)
+    {
+        Blob data(obj->getData());
+        db.store(obj->getType(), std::move(data), obj->getHash(), seq);
+    }
+    db.sync();
+}
+
+// One store() per iteration; a fresh Blob copy is handed over each time.
+Workload const kStore{
+    .name = "Store",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.avgPayload = averagePayload(ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, db, seq, index, poolSize] = ctx;
+            auto const& obj = rs.present[index % poolSize];
+            Blob data(obj->getData());
+            db.store(obj->getType(), std::move(data), obj->getHash(), seq);
+        },
+    .reportBytes = true,
+    .pinIterations = true,
+};
+
+// One fetchNodeObject() of a stored key (a hit) per iteration.
+Workload const kFetch{
+    .name = "Fetch",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.avgPayload = averagePayload(ctx.rs.present);
+            prepopulate(ctx.db, ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, db, seq, index, poolSize] = ctx;
+            auto obj = db.fetchNodeObject(rs.present[index % poolSize]->getHash(), seq);
+            benchmark::DoNotOptimize(obj);
+        },
+    .reportBytes = true,
+};
+
+// One fetchNodeObject() of a never-stored key (a miss) per iteration.
+Workload const kMissing{
+    .name = "Missing",
+    .setup = [](SetupContext const& ctx) { ctx.rs.missing = makeMissingKeys(ctx.poolSize); },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, db, seq, index, poolSize] = ctx;
+            auto obj = db.fetchNodeObject(rs.missing[index % poolSize], seq);
+            benchmark::DoNotOptimize(obj);
+        },
+};
+
+// 80% hits / 20% misses. The fetch index comes from a shuffle table so access
+// is random-like without per-iteration RNG cost; sequential `index % poolSize`
+// would be artificially cache-friendly.
+Workload const kMixed{
+    .name = "Mixed",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.missing = makeMissingKeys(ctx.poolSize);
+            ctx.rs.shuffle = makeShuffle(ctx.poolSize, /*seed=*/1);
+            prepopulate(ctx.db, ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, db, seq, index, poolSize] = ctx;
+            auto const pick = rs.shuffle[index % poolSize];
+            std::shared_ptr<NodeObject> obj;
+            if (index % 5 == 0)
+            {
+                obj = db.fetchNodeObject(rs.missing[pick], seq);
+            }
+            else
+            {
+                obj = db.fetchNodeObject(rs.present[pick]->getHash(), seq);
+            }
+            benchmark::DoNotOptimize(obj);
+        },
+};
+
+// An xrpld-like cycle: a hit, a maybe-miss recent fetch, and a store. The
+// recent fetch uses the shuffle table (not `slot`) so it doesn't fetch the item
+// it's about to store this iteration - which would give an all-miss-then-hit
+// step instead of a smooth ramp. The store walks sequentially so each recent
+// object is stored once.
+Workload const kWork{
+    .name = "Work",
+    .setup =
+        [](SetupContext const& ctx) {
+            ctx.rs.present = makePool(1, ctx.poolSize);
+            ctx.rs.recent = makePool(1, ctx.poolSize, ctx.poolSize);
+            ctx.rs.shuffle = makeShuffle(ctx.poolSize, /*seed=*/2);
+            prepopulate(ctx.db, ctx.rs.present);
+        },
+    .iterate =
+        [](IterateContext const& ctx) {
+            auto& [rs, db, seq, index, poolSize] = ctx;
+            auto const slot = index % poolSize;
+            auto const pick = rs.shuffle[slot];
+
+            auto historical = db.fetchNodeObject(rs.present[pick]->getHash(), seq);
+            benchmark::DoNotOptimize(historical);
+
+            auto recent = db.fetchNodeObject(rs.recent[pick]->getHash(), seq);
+            benchmark::DoNotOptimize(recent);
+
+            auto const& obj = rs.recent[slot];
+            Blob data(obj->getData());
+            db.store(obj->getType(), std::move(data), obj->getHash(), seq);
+        },
+    .pinIterations = true,
+};
+
+void
+registerWorkload(BackendConfig const& bc, Workload const& w)
+{
+    auto rs = std::make_shared<RunState>();
+    std::string const cfg = bc.config;
+    std::string name{kNamePrefix};
+    name += w.name;
+    name += kNameSeparator;
+    name += bc.name;
+    auto* b = benchmark::RegisterBenchmark(name, [rs, cfg, w](benchmark::State& state) {
+        auto const poolSize = static_cast<std::size_t>(state.range(0));
+        rs->harness = std::make_unique<DatabaseHarness>(cfg, kReadThreads);
+        auto& db = *rs->harness->db;
+        w.setup(SetupContext{.rs = *rs, .db = db, .poolSize = poolSize});
+        auto const seq = db.earliestLedgerSeq();
+
+        std::size_t index = 0;
+        for (auto _ : state)
+        {
+            w.iterate(
+                IterateContext{
+                    .rs = *rs, .db = db, .seq = seq, .index = index, .poolSize = poolSize});
+            ++index;
+        }
+        benchmark::ClobberMemory();
+
+        state.SetItemsProcessed(state.iterations());
+        if (w.reportBytes)
+        {
+            state.SetBytesProcessed(static_cast<std::int64_t>(state.iterations() * rs->avgPayload));
+        }
+        rs->harness.reset();
+    });
+
+    b->Arg(kDefaultPoolSize);
+
+    if (w.pinIterations)
+        b->Iterations(kDefaultPoolSize);
+}
+
+[[maybe_unused]] bool const kRegistered = [] {
+    auto const workloads = std::to_array({&kStore, &kFetch, &kMissing, &kMixed, &kWork});
+    for (auto const& bc : backendConfigs())
+    {
+        for (auto const* w : workloads)
+            registerWorkload(bc, *w);
+    }
+    return true;
+}();
+
+}  // namespace
+}  // namespace xrpl::NodeStore

src/benchmarks/libxrpl/nodestore/NodeStoreBench.h

@@ -0,0 +1,318 @@
+#pragma once
+
+#include <xrpl/basics/Blob.h>
+#include <xrpl/basics/ByteUtilities.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/basics/safe_cast.h>
+#include <xrpl/beast/utility/Journal.h>
+#include <xrpl/beast/utility/temp_dir.h>
+#include <xrpl/beast/xor_shift_engine.h>
+#include <xrpl/config/BasicConfig.h>
+#include <xrpl/nodestore/Backend.h>
+#include <xrpl/nodestore/Database.h>
+#include <xrpl/nodestore/DummyScheduler.h>
+#include <xrpl/nodestore/Manager.h>
+#include <xrpl/nodestore/NodeObject.h>
+#include <xrpl/nodestore/Scheduler.h>
+#include <xrpl/nodestore/Types.h>
+
+#include <boost/algorithm/string/classification.hpp>
+#include <boost/algorithm/string/split.hpp>
+
+#include <algorithm>
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <memory>
+#include <numeric>
+#include <random>
+#include <string>
+#include <utility>
+#include <vector>
+
+// Shared helpers for the NodeStore benchmarks.
+//
+namespace xrpl::NodeStore {
+
+// Fill `bytes` of memory at `buffer` with random bits drawn from `g`.
+template <class Generator>
+inline void
+rngcpy(void* buffer, std::size_t bytes, Generator& g)
+{
+    using result_type = typename Generator::result_type;
+    while (bytes >= sizeof(result_type))
+    {
+        auto const v = g();
+        std::memcpy(buffer, &v, sizeof(v));
+        buffer = reinterpret_cast<std::uint8_t*>(buffer) + sizeof(v);
+        bytes -= sizeof(v);
+    }
+
+    if (bytes > 0)
+    {
+        auto const v = g();
+        std::memcpy(buffer, &v, bytes);
+    }
+}
+
+/**
+ * @brief Deterministic generator of a reproducible sequence of random NodeObjects.
+ *
+ * Indexing is stable: `obj(n)` and `key(n)` always return the same value for a
+ * given `n`, regardless of call order, because the engine is reseeded from `n`
+ * on every call.
+ *
+ * Using different prefixes guarantees the two key spaces are disjoint for the fetch-miss
+ * workloads.
+ */
+class Sequence
+{
+private:
+    static constexpr auto kMinSize = 250;
+    static constexpr auto kMaxSize = 1250;
+
+    beast::xor_shift_engine gen_;
+    std::uint8_t prefix_;
+    std::discrete_distribution<std::uint32_t> dType_;
+    std::uniform_int_distribution<std::uint32_t> dSize_;
+
+public:
+    explicit Sequence(std::uint8_t prefix)
+        : prefix_(prefix)
+        // uniform distribution over hotLEDGER - hotTRANSACTION_NODE
+        // but exclude hotTRANSACTION = 2 (removed)
+        , dType_({1, 1, 0, 1, 1})
+        , dSize_(kMinSize, kMaxSize)
+    {
+    }
+
+    // Returns the n-th key. Used to generate keys that are never stored.
+    // The layout mirrors obj()'s: prefix at byte 0, RNG over the rest, so the
+    // two key spaces stay disjoint by construction (not by coincidence).
+    uint256
+    key(std::size_t n)
+    {
+        gen_.seed(n + 1);
+        uint256 result;
+        auto const data = static_cast<std::uint8_t*>(&*result.begin());
+        *data = prefix_;
+        rngcpy(data + 1, result.size() - 1, gen_);
+        return result;
+    }
+
+    // Returns the n-th complete NodeObject.
+    std::shared_ptr<NodeObject>
+    obj(std::size_t n)
+    {
+        gen_.seed(n + 1);
+        uint256 key;
+        auto const data = static_cast<std::uint8_t*>(&*key.begin());
+        *data = prefix_;
+        rngcpy(data + 1, key.size() - 1, gen_);
+        Blob value(dSize_(gen_));
+        rngcpy(&value[0], value.size(), gen_);
+        return NodeObject::createObject(
+            safeCast<NodeObjectType>(dType_(gen_)), std::move(value), key);
+    }
+
+    // Fills `b` with `size` consecutive NodeObjects starting at index `n`.
+    void
+    batch(std::size_t n, Batch& b, std::size_t size)
+    {
+        b.clear();
+        b.reserve(size);
+        while ((size--) != 0u)
+            b.push_back(obj(n++));
+    }
+};
+
+// Parse a comma-separated "key=value,key=value" string into a config Section.
+inline Section
+parseConfig(std::string const& s)
+{
+    Section section;
+    std::vector<std::string> values;
+    boost::split(values, s, boost::algorithm::is_any_of(","));
+    section.append(values);
+    return section;
+}
+
+// Pre-generate `count` distinct objects from key space `prefix`, starting at
+// sequence index `start`.
+inline Batch
+makePool(std::uint8_t prefix, std::size_t count, std::size_t start = 0)
+{
+    Sequence seq(prefix);
+    Batch pool;
+    pool.reserve(count);
+    for (std::size_t i = 0; i < count; ++i)
+        pool.push_back(seq.obj(start + i));
+    return pool;
+}
+
+// Pre-generate `count` keys disjoint from every `makePool(...)` object, for
+// measuring fetches that miss.
+inline std::vector<uint256>
+makeMissingKeys(std::size_t count)
+{
+    Sequence seq(2);
+    std::vector<uint256> keys;
+    keys.reserve(count);
+    for (std::size_t i = 0; i < count; ++i)
+        keys.push_back(seq.key(i));
+    return keys;
+}
+
+// Mean payload size across a pool, used for SetBytesProcessed throughput.
+inline std::size_t
+averagePayload(Batch const& pool)
+{
+    if (pool.empty())
+        return 0;
+    std::size_t total = 0;
+    for (auto const& obj : pool)
+        total += obj->getData().size();
+    return total / pool.size();
+}
+
+// Store every object and flush, so a following fetch exercises the real read
+// path rather than an in-memory write buffer.
+//
+// We chunk the write at kBatchWriteLimitSize because Types.h documents that as
+// the maximum allowed batch size. NuDB happens to tolerate larger batches
+// today, but the benchmark should not rely on that.
+//
+// sync() is a no-op for both NuDB and RocksDB at the moment (NuDB has a small
+// internal burst buffer that the timed loop will warm up). That is a contract
+// hint, not a guarantee; if either backend ever grows a real flush we get it
+// here for free.
+inline void
+prepopulate(Backend& backend, Batch const& objects)
+{
+    for (std::size_t i = 0; i < objects.size(); i += kBatchWriteLimitSize)
+    {
+        auto const end = std::min(i + kBatchWriteLimitSize, objects.size());
+        backend.storeBatch(Batch(objects.begin() + i, objects.begin() + end));
+    }
+    backend.sync();
+}
+
+// A deterministic permutation of [0, size). Lets the timed loop visit the
+// pre-generated pool in a random-like order with zero RNG cost per iteration -
+// the Timing_test workloads it replaces used uniform_int_distribution per
+// fetch, and a shuffle table reproduces that access pattern without paying for
+// the distribution inside the timed region.
+inline std::vector<std::size_t>
+makeShuffle(std::size_t size, std::uint64_t seed)
+{
+    std::vector<std::size_t> v(size);
+    std::iota(v.begin(), v.end(), std::size_t{0});
+    beast::xor_shift_engine gen(seed);
+    std::shuffle(v.begin(), v.end(), gen);
+    return v;
+}
+
+// Partition a pool into fixed-size batches. Any trailing remainder shorter than
+// `batchSize` is dropped, so every returned batch has exactly `batchSize`.
+inline std::vector<Batch>
+sliceBatches(Batch const& pool, std::size_t batchSize)
+{
+    std::vector<Batch> batches;
+    if (batchSize == 0)
+        return batches;
+    batches.reserve(pool.size() / batchSize);
+    for (std::size_t i = 0; i + batchSize <= pool.size(); i += batchSize)
+        batches.emplace_back(pool.begin() + i, pool.begin() + i + batchSize);
+    return batches;
+}
+
+/**
+ * @brief RAII owner of a NodeStore Backend opened on a private temporary directory.
+ *
+ * Member declaration order matters: `tempDir` is declared first so it is
+ * destroyed last, after the backend has closed and released its files.
+ */
+struct BackendHarness
+{
+    beast::TempDir tempDir;
+    DummyScheduler scheduler;
+    beast::Journal journal{beast::Journal::getNullSink()};
+    std::unique_ptr<Backend> backend;
+
+    explicit BackendHarness(std::string const& configString)
+    {
+        Section config = parseConfig(configString);
+        // A private, unique path per harness, so concurrent or repeated runs
+        // never share on-disk state.
+        config.set("path", tempDir.path());
+        backend =
+            Manager::instance().makeBackend(config, megabytes(std::size_t{4}), scheduler, journal);
+        backend->setDeletePath();
+        backend->open();
+    }
+
+    ~BackendHarness()
+    {
+        if (backend)
+            backend->close();
+    }
+};
+
+/**
+ * RAII owner of a NodeStore Database - the application-facing wrapper around a
+ * Backend, which adds fetch/store accounting and the async read-thread pool.
+ */
+struct DatabaseHarness
+{
+    beast::TempDir tempDir;
+    DummyScheduler scheduler;
+    beast::Journal journal{beast::Journal::getNullSink()};
+    std::unique_ptr<Database> db;
+
+    DatabaseHarness(std::string const& configString, int readThreads)
+    {
+        Section config = parseConfig(configString);
+        config.set("path", tempDir.path());
+        db = Manager::instance().makeDatabase(
+            megabytes(std::size_t{4}), scheduler, readThreads, config, journal);
+    }
+
+    ~DatabaseHarness()
+    {
+        if (db)
+            db->stop();
+    }
+};
+
+// A NodeStore backend to benchmark, named for the --benchmark_filter CLI flag.
+struct BackendConfig
+{
+    char const* name;    // short label, e.g. "nudb"
+    char const* config;  // parseConfig() string, e.g. "type=nudb"
+};
+
+// The backends every workload is registered against.
+//
+// The in-memory backend is intentionally excluded. It keeps its table in a
+// process-global map keyed by path, with no removal API, so building a fresh
+// backend per run - as a microbenchmark must - would leak the whole dataset on
+// every run. Timing_test, the suite this benchmark replaces, excluded it for
+// the same reason. NuDB and RocksDB are the production backends worth timing.
+//
+// RocksDB is included only when it was compiled in (xrpl.libxrpl carries
+// XRPL_ROCKSDB_AVAILABLE transitively).
+inline std::vector<BackendConfig> const&
+backendConfigs()
+{
+    static std::vector<BackendConfig> const kConfigs = {
+        {.name = "nudb", .config = "type=nudb"},
+#if XRPL_ROCKSDB_AVAILABLE
+        {.name = "rocksdb",
+         .config = "type=rocksdb,open_files=2000,filter_bits=12,cache_mb=256,"
+                   "file_size_mb=8,file_size_mult=2"},
+#endif
+    };
+    return kConfigs;
+}
+
+}  // namespace xrpl::NodeStore

src/libxrpl/ledger/helpers/LendingHelpers.cpp

@@ -312,6 +312,25 @@ computeInterestAndFeeParts(
     return std::make_pair(interest - fee, fee);
 }
 
+/* Rounds a raw (unrounded) interest amount to the loan's scale, then splits
+ * the rounded amount into net interest (to the vault) and management fee (to
+ * the broker).
+ *
+ * This is the common "round then split" step shared by late payment, full
+ * payment, and overpayment interest calculations.
+ */
+std::pair<Number, Number>
+roundAndSplitInterest(
+    Asset const& asset,
+    Number const& rawInterest,
+    TenthBips16 managementFeeRate,
+    std::int32_t loanScale,
+    Number::RoundingMode mode = Number::getround())
+{
+    auto const interest = roundToAsset(asset, rawInterest, loanScale, mode);
+    return computeInterestAndFeeParts(asset, interest, managementFeeRate, loanScale);
+}
+
 /* Calculates penalty interest accrued on overdue payments.
  * Returns 0 if payment is not late.
  *
@@ -387,22 +406,18 @@ loanAccruedInterest(
  *
  * This is the core function that updates the Loan ledger object fields based on
  * a computed payment.
-
- * The function is templated to work with both direct Number/uint32_t values
- * (for testing/simulation) and ValueProxy types (for actual ledger updates).
  */
-template <class NumberProxy, class UInt32Proxy, class UInt32OptionalProxy>
 LoanPaymentParts
-doPayment(
-    ExtendedPaymentComponents const& payment,
-    NumberProxy& totalValueOutstandingProxy,
-    NumberProxy& principalOutstandingProxy,
-    NumberProxy& managementFeeOutstandingProxy,
-    UInt32Proxy& paymentRemainingProxy,
-    UInt32Proxy& prevPaymentDateProxy,
-    UInt32OptionalProxy& nextDueDateProxy,
-    std::uint32_t paymentInterval)
+doPayment(ExtendedPaymentComponents const& payment, SLE::ref loan)
 {
+    auto totalValueOutstandingProxy = loan->at(sfTotalValueOutstanding);
+    auto principalOutstandingProxy = loan->at(sfPrincipalOutstanding);
+    auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding);
+    auto paymentRemainingProxy = loan->at(sfPaymentRemaining);
+    auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDueDate);
+    auto nextDueDateProxy = loan->at(sfNextPaymentDueDate);
+    std::uint32_t const paymentInterval = loan->at(sfPaymentInterval);
+
     XRPL_ASSERT_PARTS(nextDueDateProxy, "xrpl::detail::doPayment", "Next due date proxy set");
 
     if (payment.specialCase == PaymentSpecialCase::Final)
@@ -470,16 +485,12 @@ doPayment(
 
     // Principal can never exceed total value (principal is part of total value)
     XRPL_ASSERT_PARTS(
-        // Use an explicit cast because the template parameter can be
-        // ValueProxy<Number> or Number
         static_cast<Number>(principalOutstandingProxy) <=
             static_cast<Number>(totalValueOutstandingProxy),
         "xrpl::detail::doPayment",
         "principal does not exceed total");
 
     XRPL_ASSERT_PARTS(
-        // Use an explicit cast because the template parameter can be
-        // ValueProxy<Number> or Number
         static_cast<Number>(managementFeeOutstandingProxy) >= beast::kZero,
         "xrpl::detail::doPayment",
         "fee outstanding stays valid");
@@ -717,22 +728,23 @@ tryOverpayment(
  * overpayment would leave the loan in an invalid state, we can reject it
  * gracefully without corrupting the ledger data.
  */
-template <class NumberProxy>
 std::expected<LoanPaymentParts, TER>
 doOverpayment(
     Rules const& rules,
     Asset const& asset,
     std::int32_t loanScale,
     ExtendedPaymentComponents const& overpaymentComponents,
-    NumberProxy& totalValueOutstandingProxy,
-    NumberProxy& principalOutstandingProxy,
-    NumberProxy& managementFeeOutstandingProxy,
-    NumberProxy& periodicPaymentProxy,
+    SLE::ref loan,
     Number const& periodicRate,
-    std::uint32_t const paymentRemaining,
     TenthBips16 const managementFeeRate,
     beast::Journal j)
 {
+    auto totalValueOutstandingProxy = loan->at(sfTotalValueOutstanding);
+    auto principalOutstandingProxy = loan->at(sfPrincipalOutstanding);
+    auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding);
+    auto periodicPaymentProxy = loan->at(sfPeriodicPayment);
+    auto const paymentsRemaining = loan->at(sfPaymentRemaining);
+
     auto const loanState = constructLoanState(
         totalValueOutstandingProxy, principalOutstandingProxy, managementFeeOutstandingProxy);
     auto const periodicPayment = periodicPaymentProxy;
@@ -744,7 +756,7 @@ doOverpayment(
                     << ", interestPart: " << overpaymentComponents.trackedInterestPart()
                     << ", untrackedInterest: " << overpaymentComponents.untrackedInterest
                     << ", totalDue: " << overpaymentComponents.totalDue
-                    << ", payments remaining :" << paymentRemaining;
+                    << ", payments remaining :" << paymentsRemaining;
 
     // Attempt to re-amortize the loan with the overpayment applied.
     // This modifies the temporary copies, leaving the proxies unchanged.
@@ -756,7 +768,7 @@ doOverpayment(
         loanState,
         periodicPayment,
         periodicRate,
-        paymentRemaining,
+        paymentsRemaining,
         managementFeeRate,
         j);
     if (!ret)
@@ -864,16 +876,15 @@ std::expected<ExtendedPaymentComponents, TER>
 computeLatePayment(
     Asset const& asset,
     ApplyView const& view,
-    Number const& principalOutstanding,
-    std::int32_t nextDueDate,
+    SLE::const_ref loan,
     ExtendedPaymentComponents const& periodic,
-    TenthBips32 lateInterestRate,
-    std::int32_t loanScale,
-    Number const& latePaymentFee,
     STAmount const& amount,
     TenthBips16 managementFeeRate,
     beast::Journal j)
 {
+    std::int32_t const nextDueDate = loan->at(sfNextPaymentDueDate);
+    std::int32_t const loanScale = loan->at(sfLoanScale);
+
     // Check if the due date has passed. If not, reject the payment as
     // being too soon
     if (!hasExpired(view, nextDueDate))
@@ -881,15 +892,15 @@ computeLatePayment(
 
     // Calculate the penalty interest based on how long the payment is overdue.
     auto const latePaymentInterest = loanLatePaymentInterest(
-        principalOutstanding, lateInterestRate, view.parentCloseTime(), nextDueDate);
+        loan->at(sfPrincipalOutstanding),
+        TenthBips32{loan->at(sfLateInterestRate)},
+        view.parentCloseTime(),
+        nextDueDate);
 
     // Round the late interest and split it between the vault (net interest)
-    // and the broker (management fee portion). This lambda ensures we
-    // round before splitting to maintain precision.
-    auto const [roundedLateInterest, roundedLateManagementFee] = [&]() {
-        auto const interest = roundToAsset(asset, latePaymentInterest, loanScale);
-        return computeInterestAndFeeParts(asset, interest, managementFeeRate, loanScale);
-    }();
+    // and the broker (management fee portion).
+    auto const [roundedLateInterest, roundedLateManagementFee] =
+        roundAndSplitInterest(asset, latePaymentInterest, managementFeeRate, loanScale);
 
     XRPL_ASSERT(roundedLateInterest >= 0, "xrpl::detail::computeLatePayment : valid late interest");
     XRPL_ASSERT_PARTS(
@@ -908,7 +919,7 @@ computeLatePayment(
         // 1. Regular service fee (from periodic.untrackedManagementFee)
         // 2. Late payment fee (fixed penalty)
         // 3. Management fee portion of late interest
-        periodic.untrackedManagementFee + latePaymentFee + roundedLateManagementFee,
+        periodic.untrackedManagementFee + loan->at(sfLatePaymentFee) + roundedLateManagementFee,
 
         // Untracked interest includes:
         // 1. Any untracked interest from the regular payment (usually 0)
@@ -958,22 +969,15 @@ std::expected<ExtendedPaymentComponents, TER>
 computeFullPayment(
     Asset const& asset,
     ApplyView& view,
-    Number const& principalOutstanding,
-    Number const& managementFeeOutstanding,
-    Number const& periodicPayment,
-    std::uint32_t paymentRemaining,
-    std::uint32_t prevPaymentDate,
-    std::uint32_t const startDate,
-    std::uint32_t const paymentInterval,
-    TenthBips32 const closeInterestRate,
-    std::int32_t loanScale,
-    Number const& totalInterestOutstanding,
+    SLE::const_ref loan,
     Number const& periodicRate,
-    Number const& closePaymentFee,
     STAmount const& amount,
     TenthBips16 managementFeeRate,
     beast::Journal j)
 {
+    std::uint32_t const paymentRemaining = loan->at(sfPaymentRemaining);
+    std::int32_t const loanScale = loan->at(sfLoanScale);
+
     // Full payment must be made before the final scheduled payment.
     if (paymentRemaining <= 1)
     {
@@ -986,7 +990,7 @@ computeFullPayment(
     // This theoretical (unrounded) value is used to compute interest and
     // penalties accurately.
     Number const theoreticalPrincipalOutstanding = loanPrincipalFromPeriodicPayment(
-        view.rules(), periodicPayment, periodicRate, paymentRemaining);
+        view.rules(), loan->at(sfPeriodicPayment), periodicRate, paymentRemaining);
 
     // Full payment interest includes both accrued interest (time since last
     // payment) and prepayment penalty (for closing early).
@@ -994,18 +998,21 @@ computeFullPayment(
         theoreticalPrincipalOutstanding,
         periodicRate,
         view.parentCloseTime(),
-        paymentInterval,
-        prevPaymentDate,
-        startDate,
-        closeInterestRate);
+        loan->at(sfPaymentInterval),
+        loan->at(sfPreviousPaymentDueDate),
+        loan->at(sfStartDate),
+        TenthBips32{loan->at(sfCloseInterestRate)});
 
-    // Split the full payment interest into net interest (to vault) and
-    // management fee (to broker), applying proper rounding.
-    auto const [roundedFullInterest, roundedFullManagementFee] = [&]() {
-        auto const interest =
-            roundToAsset(asset, fullPaymentInterest, loanScale, Number::RoundingMode::Downward);
-        return computeInterestAndFeeParts(asset, interest, managementFeeRate, loanScale);
-    }();
+    // Split the full payment interest into net interest (to vault) and management fee (to broker),
+    // applying proper rounding.
+    auto const [roundedFullInterest, roundedFullManagementFee] = roundAndSplitInterest(
+        asset, fullPaymentInterest, managementFeeRate, loanScale, Number::RoundingMode::Downward);
+
+    LoanState const loanState = constructLoanState(loan);
+    Number const principalOutstanding = loanState.principalOutstanding;
+    Number const managementFeeOutstanding = loanState.managementFeeDue;
+    Number const totalInterestOutstanding = loanState.interestDue;
+    Number const closePaymentFee = roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale);
 
     ExtendedPaymentComponents const full{
         PaymentComponents{
@@ -1046,8 +1053,7 @@ computeFullPayment(
         "xrpl::detail::computeFullPayment",
         "total due is rounded");
 
-    JLOG(j.trace()) << "computeFullPayment result: periodicPayment: " << periodicPayment
-                    << ", periodicRate: " << periodicRate
+    JLOG(j.trace()) << "computeFullPayment result: periodicRate: " << periodicRate
                     << ", paymentRemaining: " << paymentRemaining
                     << ", theoreticalPrincipalOutstanding: " << theoreticalPrincipalOutstanding
                     << ", fullPaymentInterest: " << fullPaymentInterest
@@ -1298,6 +1304,34 @@ computePaymentComponents(
     };
 }
 
+/* Thin overload of computePaymentComponents() that unwraps the tracked
+ * fields directly from the Loan ledger object. `periodicRate` is derived
+ * rather than stored, and `managementFeeRate` comes from the LoanBroker, not
+ * the Loan, so both remain explicit parameters. Kept separate from the
+ * value-based overload above, which is exercised directly by unit tests
+ * against simulated (non-ledger) loan states.
+ */
+PaymentComponents
+computePaymentComponents(
+    Rules const& rules,
+    Asset const& asset,
+    SLE::ref loan,
+    Number const& periodicRate,
+    TenthBips16 managementFeeRate)
+{
+    return computePaymentComponents(
+        rules,
+        asset,
+        loan->at(sfLoanScale),
+        loan->at(sfTotalValueOutstanding),
+        loan->at(sfPrincipalOutstanding),
+        loan->at(sfManagementFeeOutstanding),
+        loan->at(sfPeriodicPayment),
+        periodicRate,
+        loan->at(sfPaymentRemaining),
+        managementFeeRate);
+}
+
 /* Computes payment components for an overpayment scenario.
  *
  * An overpayment occurs when a borrower pays more than the scheduled periodic
@@ -1342,11 +1376,12 @@ computeOverpaymentComponents(
     // This interest doesn't follow the normal amortization schedule - it's
     // a one-time charge for paying early.
     // Equation (20) and (21) from XLS-66 spec, Section A-2 Equation Glossary
-    auto const [roundedOverpaymentInterest, roundedOverpaymentManagementFee] = [&]() {
-        auto const interest =
-            roundToAsset(asset, tenthBipsOfValue(overpayment, overpaymentInterestRate), loanScale);
-        return detail::computeInterestAndFeeParts(asset, interest, managementFeeRate, loanScale);
-    }();
+    auto const [roundedOverpaymentInterest, roundedOverpaymentManagementFee] =
+        roundAndSplitInterest(
+            asset,
+            tenthBipsOfValue(overpayment, overpaymentInterestRate),
+            managementFeeRate,
+            loanScale);
 
     auto const result = detail::ExtendedPaymentComponents{
         // Build the payment components, after fees and penalty
@@ -1373,6 +1408,265 @@ computeOverpaymentComponents(
     return result;
 }
 
+/* Derives the two rate values every make*Payment() helper needs: the
+ * broker's management fee rate, and the loan's periodic (per-payment-period)
+ * interest rate.
+ */
+std::pair<TenthBips16, Number>
+loanRatesFor(SLE::const_ref loan, SLE::const_ref brokerSle)
+{
+    TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)};
+    TenthBips32 const interestRate{loan->at(sfInterestRate)};
+    Number const periodicRate = loanPeriodicRate(interestRate, loan->at(sfPaymentInterval));
+    XRPL_ASSERT(interestRate == 0 || periodicRate > 0, "xrpl::detail::loanRatesFor : valid rate");
+    return {managementFeeRate, periodicRate};
+}
+
+/* Handles a full (early payoff) payment. Implements the "full payment"
+ * branch of the make_payment function from the XLS-66 spec, Section
+ * 3.2.4.4.
+ */
+std::expected<LoanPaymentParts, TER>
+makeFullPayment(
+    Asset const& asset,
+    ApplyView& view,
+    SLE::ref loan,
+    SLE::const_ref brokerSle,
+    STAmount const& amount,
+    beast::Journal j)
+{
+    auto const [managementFeeRate, periodicRate] = loanRatesFor(loan, brokerSle);
+
+    auto const fullPaymentComponents =
+        computeFullPayment(asset, view, loan, periodicRate, amount, managementFeeRate, j);
+
+    // computeFullPayment only ever fails with a genuine error TER (never
+    // tesSUCCESS), so there is no separate "no-op" outcome to handle here.
+    if (fullPaymentComponents.has_value())
+        return doPayment(*fullPaymentComponents, loan);
+    return std::unexpected(fullPaymentComponents.error());
+}
+
+/* Handles a late payment (past due date, with the late-payment flag set).
+ * Implements the "late payment" branch of the make_payment function from
+ * the XLS-66 spec, Section 3.2.4.4.
+ */
+std::expected<LoanPaymentParts, TER>
+makeLatePayment(
+    Asset const& asset,
+    ApplyView const& view,
+    SLE::ref loan,
+    SLE::const_ref brokerSle,
+    STAmount const& amount,
+    beast::Journal j)
+{
+    auto const [managementFeeRate, periodicRate] = loanRatesFor(loan, brokerSle);
+
+    Number const serviceFee = loan->at(sfLoanServiceFee);
+    ExtendedPaymentComponents const periodic{
+        computePaymentComponents(view.rules(), asset, loan, periodicRate, managementFeeRate),
+        serviceFee};
+    XRPL_ASSERT_PARTS(
+        periodic.trackedPrincipalDelta >= 0,
+        "xrpl::detail::makeLatePayment",
+        "regular payment valid principal");
+
+    auto const latePaymentComponents =
+        computeLatePayment(asset, view, loan, periodic, amount, managementFeeRate, j);
+
+    // computeLatePayment only ever fails with a genuine error TER (never
+    // tesSUCCESS), so there is no separate "no-op" outcome to handle here.
+    if (latePaymentComponents.has_value())
+        return doPayment(*latePaymentComponents, loan);
+    return std::unexpected(latePaymentComponents.error());
+}
+
+/* Handles regular scheduled payments, including an optional overpayment tail.
+ * Implements the "regular" and "overpayment" branches of the make_payment
+ * function from the XLS-66 spec, Section 3.2.4.4.
+ */
+std::expected<LoanPaymentParts, TER>
+makeRegularPayment(
+    Asset const& asset,
+    ApplyView const& view,
+    SLE::ref loan,
+    SLE::const_ref brokerSle,
+    STAmount const& amount,
+    LoanPaymentType const paymentType,
+    beast::Journal j)
+{
+    using namespace Lending;
+
+    XRPL_ASSERT_PARTS(
+        paymentType == LoanPaymentType::Regular || paymentType == LoanPaymentType::Overpayment,
+        "xrpl::detail::makeRegularPayment",
+        "regular payment type");
+
+    auto const [managementFeeRate, periodicRate] = loanRatesFor(loan, brokerSle);
+
+    std::int32_t const loanScale = loan->at(sfLoanScale);
+    Number const serviceFee = loan->at(sfLoanServiceFee);
+
+    ExtendedPaymentComponents periodic{
+        computePaymentComponents(view.rules(), asset, loan, periodicRate, managementFeeRate),
+        serviceFee};
+    XRPL_ASSERT_PARTS(
+        periodic.trackedPrincipalDelta >= 0,
+        "xrpl::detail::makeRegularPayment",
+        "regular payment valid principal");
+
+    // Keep a running total of the actual parts paid
+    LoanPaymentParts totalParts;
+    Number totalPaid = kNumZero;
+    std::size_t numPayments = 0;
+
+    // Cached here (rather than re-looking up loan->at(sfPaymentRemaining) at each use) since it's
+    // read multiple times below. It's a write-through proxy, so it still reflects doPayment's
+    // mutations each iteration.
+    auto paymentRemainingProxy = loan->at(sfPaymentRemaining);
+
+    while ((amount >= (totalPaid + periodic.totalDue)) && paymentRemainingProxy > 0 &&
+           numPayments < kLoanMaximumPaymentsPerTransaction)
+    {
+        // Try to make more payments
+        XRPL_ASSERT_PARTS(
+            periodic.trackedPrincipalDelta >= 0,
+            "xrpl::detail::makeRegularPayment",
+            "payment pays non-negative principal");
+
+        totalPaid += periodic.totalDue;
+        totalParts += doPayment(periodic, loan);
+        ++numPayments;
+
+        XRPL_ASSERT_PARTS(
+            (periodic.specialCase == PaymentSpecialCase::Final) == (paymentRemainingProxy == 0),
+            "xrpl::detail::makeRegularPayment",
+            "final payment is the final payment");
+
+        // Don't compute the next payment if this was the last payment
+        if (periodic.specialCase == PaymentSpecialCase::Final)
+            break;
+
+        periodic = ExtendedPaymentComponents{
+            computePaymentComponents(view.rules(), asset, loan, periodicRate, managementFeeRate),
+            serviceFee};
+    }
+
+    if (numPayments == 0)
+    {
+        JLOG(j.warn()) << "Regular loan payment amount is insufficient. Due: " << periodic.totalDue
+                       << ", paid: " << amount;
+        return std::unexpected(tecINSUFFICIENT_PAYMENT);
+    }
+
+    XRPL_ASSERT_PARTS(
+        totalParts.principalPaid + totalParts.interestPaid + totalParts.feePaid == totalPaid,
+        "xrpl::detail::makeRegularPayment",
+        "payment parts add up");
+    XRPL_ASSERT_PARTS(
+        totalParts.valueChange == 0, "xrpl::detail::makeRegularPayment", "no value change");
+
+    // -------------------------------------------------------------
+    // overpayment handling
+    //
+    // If the "fixCleanup3_1_3" amendment is enabled, truncate "amount",
+    // at the loan scale. If the raw value is used, the overpayment
+    // amount could be meaningless dust. Trying to process such a small
+    // amount will, at best, waste time when all the result values round
+    // to zero. At worst, it can cause logical errors with tiny amounts
+    // of interest that don't add up correctly.
+    auto const roundedAmount = view.rules().enabled(fixCleanup3_1_3)
+        ? roundToAsset(asset, amount, loanScale, Number::RoundingMode::TowardsZero)
+        : amount;
+
+    bool const overpaymentSupported =
+        paymentType == LoanPaymentType::Overpayment && loan->isFlag(lsfLoanOverpayment);
+
+    bool const overpaymentAllowed =   //
+        paymentRemainingProxy > 0 &&  //
+        totalPaid < roundedAmount &&  //
+        numPayments < kLoanMaximumPaymentsPerTransaction;
+
+    if (overpaymentSupported && overpaymentAllowed)
+    {
+        TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)};
+        TenthBips32 const overpaymentFeeRate{loan->at(sfOverpaymentFee)};
+
+        // It shouldn't be possible for the overpayment to be greater than
+        // totalValueOutstanding, because that would have been processed as
+        // another normal payment. But cap it just in case.
+        Number const overpaymentRaw =
+            std::min(roundedAmount - totalPaid, *loan->at(sfTotalValueOutstanding));
+
+        bool const fixEnabled = view.rules().enabled(fixCleanup3_2_0);
+        Number const overpayment = fixEnabled
+            ? roundToAsset(asset, overpaymentRaw, loanScale, Number::RoundingMode::Downward)
+            : overpaymentRaw;
+
+        // Post-amendment, the rounded overpayment can be zero; pre-amendment
+        // it's always positive given the surrounding guards.
+        if (!fixEnabled || overpayment > 0)
+        {
+            ExtendedPaymentComponents const overpaymentComponents = computeOverpaymentComponents(
+                view.rules(),
+                asset,
+                loanScale,
+                overpayment,
+                overpaymentInterestRate,
+                overpaymentFeeRate,
+                managementFeeRate);
+
+            // Don't process an overpayment if the whole amount (or more!)
+            // gets eaten by fees and interest.
+            if (overpaymentComponents.trackedPrincipalDelta > 0)
+            {
+                XRPL_ASSERT_PARTS(
+                    overpaymentComponents.untrackedInterest >= beast::kZero,
+                    "xrpl::detail::makeRegularPayment",
+                    "overpayment penalty did not reduce value of loan");
+                if (auto const overResult = doOverpayment(
+                        view.rules(),
+                        asset,
+                        loanScale,
+                        overpaymentComponents,
+                        loan,
+                        periodicRate,
+                        managementFeeRate,
+                        j))
+                {
+                    totalParts += *overResult;
+                }
+                else if (overResult.error())
+                {
+                    // error() will be the TER returned if a payment is not
+                    // made. It will only evaluate to true if it's unsuccessful.
+                    // Otherwise, tesSUCCESS means nothing was done, so
+                    // continue.
+                    return std::unexpected(overResult.error());
+                }
+            }
+        }
+    }
+
+    // Check the final results are rounded, to double-check that the
+    // intermediate steps were rounded.
+    XRPL_ASSERT(
+        isRounded(asset, totalParts.principalPaid, loanScale) &&
+            totalParts.principalPaid >= beast::kZero,
+        "xrpl::detail::makeRegularPayment : total principal paid is valid");
+    XRPL_ASSERT(
+        isRounded(asset, totalParts.interestPaid, loanScale) &&
+            totalParts.interestPaid >= beast::kZero,
+        "xrpl::detail::makeRegularPayment : total interest paid is valid");
+    XRPL_ASSERT(
+        isRounded(asset, totalParts.valueChange, loanScale),
+        "xrpl::detail::makeRegularPayment : loan value change is valid");
+    XRPL_ASSERT(
+        isRounded(asset, totalParts.feePaid, loanScale) && totalParts.feePaid >= beast::kZero,
+        "xrpl::detail::makeRegularPayment : fee paid is valid");
+    return totalParts;
+}
+
 }  // namespace detail
 
 detail::LoanStateDeltas
@@ -1632,8 +1926,10 @@ constructLoanState(
 }
 
 LoanState
-constructRoundedLoanState(SLE::const_ref loan)
+constructLoanState(SLE::const_ref loan)
 {
+    XRPL_ASSERT(loan && loan->getType() == ltLOAN, "xrpl::constructLoanState : valid loan SLE");
+
     return constructLoanState(
         loan->at(sfTotalValueOutstanding),
         loan->at(sfPrincipalOutstanding),
@@ -1790,12 +2086,7 @@ loanMakePayment(
     LoanPaymentType const paymentType,
     beast::Journal j)
 {
-    using namespace Lending;
-
-    auto principalOutstandingProxy = loan->at(sfPrincipalOutstanding);
-    auto paymentRemainingProxy = loan->at(sfPaymentRemaining);
-
-    if (paymentRemainingProxy == 0 || principalOutstandingProxy == 0)
+    if (loan->at(sfPaymentRemaining) == 0 || loan->at(sfPrincipalOutstanding) == 0)
     {
         // Loan complete this is already checked in LoanPay::preclaim()
         // LCOV_EXCL_START
@@ -1804,9 +2095,6 @@ loanMakePayment(
         // LCOV_EXCL_STOP
     }
 
-    auto totalValueOutstandingProxy = loan->at(sfTotalValueOutstanding);
-    auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding);
-
     // Next payment due date must be set unless the loan is complete
     auto nextDueDateProxy = loan->at(sfNextPaymentDueDate);
     if (*nextDueDateProxy == 0)
@@ -1815,26 +2103,8 @@ loanMakePayment(
         return std::unexpected(tecINTERNAL);
     }
 
-    std::int32_t const loanScale = loan->at(sfLoanScale);
-
-    TenthBips32 const interestRate{loan->at(sfInterestRate)};
-
-    Number const serviceFee = loan->at(sfLoanServiceFee);
-    TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)};
-
-    Number const periodicPayment = loan->at(sfPeriodicPayment);
-
-    auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDueDate);
-    std::uint32_t const startDate = loan->at(sfStartDate);
-
-    std::uint32_t const paymentInterval = loan->at(sfPaymentInterval);
-
-    // Compute the periodic rate that will be used for calculations
-    // throughout
-    Number const periodicRate = loanPeriodicRate(interestRate, paymentInterval);
-    XRPL_ASSERT(interestRate == 0 || periodicRate > 0, "xrpl::loanMakePayment : valid rate");
-
-    XRPL_ASSERT(*totalValueOutstandingProxy > 0, "xrpl::loanMakePayment : valid total value");
+    XRPL_ASSERT(
+        *loan->at(sfTotalValueOutstanding) > 0, "xrpl::loanMakePayment : valid total value");
 
     view.update(loan);
 
@@ -1844,311 +2114,29 @@ loanMakePayment(
     {
         // If the payment is late, and the late flag was not set, it's not
         // valid
-        JLOG(j.warn()) << "Loan payment is overdue. Use the tfLoanLatePayment "
-                          "transaction "
-                          "flag to make a late payment. Loan was created on "
-                       << startDate << ", prev payment due date is " << prevPaymentDateProxy
-                       << ", next payment due date is " << nextDueDateProxy << ", ledger time is "
+        JLOG(j.warn()) << "Loan payment is overdue. Use the tfLoanLatePayment transaction flag to "
+                          "make a late payment. Loan was created on "
+                       << loan->at(sfStartDate) << ", prev payment due date is "
+                       << loan->at(sfPreviousPaymentDueDate) << ", next payment due date is "
+                       << nextDueDateProxy << ", ledger time is "
                        << view.parentCloseTime().time_since_epoch().count();
         return std::unexpected(tecEXPIRED);
     }
 
-    // -------------------------------------------------------------
-    // full payment handling
-    if (paymentType == LoanPaymentType::Full)
+    switch (paymentType)
     {
-        TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)};
-        Number const closePaymentFee = roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale);
-
-        LoanState const roundedLoanState = constructLoanState(
-            totalValueOutstandingProxy, principalOutstandingProxy, managementFeeOutstandingProxy);
-
-        auto const fullPaymentComponents = detail::computeFullPayment(
-            asset,
-            view,
-            principalOutstandingProxy,
-            managementFeeOutstandingProxy,
-            periodicPayment,
-            paymentRemainingProxy,
-            prevPaymentDateProxy,
-            startDate,
-            paymentInterval,
-            closeInterestRate,
-            loanScale,
-            roundedLoanState.interestDue,
-            periodicRate,
-            closePaymentFee,
-            amount,
-            managementFeeRate,
-            j);
-
-        if (fullPaymentComponents.has_value())
-        {
-            return doPayment(
-                *fullPaymentComponents,
-                totalValueOutstandingProxy,
-                principalOutstandingProxy,
-                managementFeeOutstandingProxy,
-                paymentRemainingProxy,
-                prevPaymentDateProxy,
-                nextDueDateProxy,
-                paymentInterval);
-        }
-
-        if (fullPaymentComponents.error())
-        {
-            // error() will be the TER returned if a payment is not made. It
-            // will only evaluate to true if it's unsuccessful. Otherwise,
-            // tesSUCCESS means nothing was done, so continue.
-            return std::unexpected(fullPaymentComponents.error());
-        }
-
-        // LCOV_EXCL_START
-        UNREACHABLE("xrpl::loanMakePayment : invalid full payment result");
-        JLOG(j.error()) << "Full payment computation failed unexpectedly.";
-        return std::unexpected(tecINTERNAL);
-        // LCOV_EXCL_STOP
+        case LoanPaymentType::Full:
+            return detail::makeFullPayment(asset, view, loan, brokerSle, amount, j);
+        case LoanPaymentType::Late:
+            return detail::makeLatePayment(asset, view, loan, brokerSle, amount, j);
+        case LoanPaymentType::Regular:
+        case LoanPaymentType::Overpayment:
+            return detail::makeRegularPayment(asset, view, loan, brokerSle, amount, paymentType, j);
     }
 
-    // -------------------------------------------------------------
-    // compute the periodic payment info that will be needed whether the
-    // payment is late or regular
-    detail::ExtendedPaymentComponents periodic{
-        detail::computePaymentComponents(
-            view.rules(),
-            asset,
-            loanScale,
-            totalValueOutstandingProxy,
-            principalOutstandingProxy,
-            managementFeeOutstandingProxy,
-            periodicPayment,
-            periodicRate,
-            paymentRemainingProxy,
-            managementFeeRate),
-        serviceFee};
-    XRPL_ASSERT_PARTS(
-        periodic.trackedPrincipalDelta >= 0,
-        "xrpl::loanMakePayment",
-        "regular payment valid principal");
-
-    // -------------------------------------------------------------
-    // late payment handling
-    if (paymentType == LoanPaymentType::Late)
-    {
-        TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)};
-        Number const latePaymentFee = loan->at(sfLatePaymentFee);
-
-        auto const latePaymentComponents = detail::computeLatePayment(
-            asset,
-            view,
-            principalOutstandingProxy,
-            nextDueDateProxy,
-            periodic,
-            lateInterestRate,
-            loanScale,
-            latePaymentFee,
-            amount,
-            managementFeeRate,
-            j);
-
-        if (latePaymentComponents.has_value())
-        {
-            return doPayment(
-                *latePaymentComponents,
-                totalValueOutstandingProxy,
-                principalOutstandingProxy,
-                managementFeeOutstandingProxy,
-                paymentRemainingProxy,
-                prevPaymentDateProxy,
-                nextDueDateProxy,
-                paymentInterval);
-        }
-
-        if (latePaymentComponents.error())
-        {
-            // error() will be the TER returned if a payment is not made. It
-            // will only evaluate to true if it's unsuccessful.
-            return std::unexpected(latePaymentComponents.error());
-        }
-
-        // LCOV_EXCL_START
-        UNREACHABLE("xrpl::loanMakePayment : invalid late payment result");
-        JLOG(j.error()) << "Late payment computation failed unexpectedly.";
-        return std::unexpected(tecINTERNAL);
-        // LCOV_EXCL_STOP
-    }
-
-    // -------------------------------------------------------------
-    // regular periodic payment handling
-
-    XRPL_ASSERT_PARTS(
-        paymentType == LoanPaymentType::Regular || paymentType == LoanPaymentType::Overpayment,
-        "xrpl::loanMakePayment",
-        "regular payment type");
-
-    // Keep a running total of the actual parts paid
-    LoanPaymentParts totalParts;
-    Number totalPaid;
-    std::size_t numPayments = 0;
-
-    while ((amount >= (totalPaid + periodic.totalDue)) && paymentRemainingProxy > 0 &&
-           numPayments < kLoanMaximumPaymentsPerTransaction)
-    {
-        // Try to make more payments
-        XRPL_ASSERT_PARTS(
-            periodic.trackedPrincipalDelta >= 0,
-            "xrpl::loanMakePayment",
-            "payment pays non-negative principal");
-
-        totalPaid += periodic.totalDue;
-        totalParts += detail::doPayment(
-            periodic,
-            totalValueOutstandingProxy,
-            principalOutstandingProxy,
-            managementFeeOutstandingProxy,
-            paymentRemainingProxy,
-            prevPaymentDateProxy,
-            nextDueDateProxy,
-            paymentInterval);
-        ++numPayments;
-
-        XRPL_ASSERT_PARTS(
-            (periodic.specialCase == detail::PaymentSpecialCase::Final) ==
-                (paymentRemainingProxy == 0),
-            "xrpl::loanMakePayment",
-            "final payment is the final payment");
-
-        // Don't compute the next payment if this was the last payment
-        if (periodic.specialCase == detail::PaymentSpecialCase::Final)
-            break;
-
-        periodic = detail::ExtendedPaymentComponents{
-            detail::computePaymentComponents(
-                view.rules(),
-                asset,
-                loanScale,
-                totalValueOutstandingProxy,
-                principalOutstandingProxy,
-                managementFeeOutstandingProxy,
-                periodicPayment,
-                periodicRate,
-                paymentRemainingProxy,
-                managementFeeRate),
-            serviceFee};
-    }
-
-    if (numPayments == 0)
-    {
-        JLOG(j.warn()) << "Regular loan payment amount is insufficient. Due: " << periodic.totalDue
-                       << ", paid: " << amount;
-        return std::unexpected(tecINSUFFICIENT_PAYMENT);
-    }
-
-    XRPL_ASSERT_PARTS(
-        totalParts.principalPaid + totalParts.interestPaid + totalParts.feePaid == totalPaid,
-        "xrpl::loanMakePayment",
-        "payment parts add up");
-    XRPL_ASSERT_PARTS(totalParts.valueChange == 0, "xrpl::loanMakePayment", "no value change");
-
-    // -------------------------------------------------------------
-    // overpayment handling
-    //
-    // If the "fixCleanup3_1_3" amendment is enabled, truncate "amount",
-    // at the loan scale. If the raw value is used, the overpayment
-    // amount could be meaningless dust. Trying to process such a small
-    // amount will, at best, waste time when all the result values round
-    // to zero. At worst, it can cause logical errors with tiny amounts
-    // of interest that don't add up correctly.
-    auto const roundedAmount = view.rules().enabled(fixCleanup3_1_3)
-        ? roundToAsset(asset, amount, loanScale, Number::RoundingMode::TowardsZero)
-        : amount;
-    if (paymentType == LoanPaymentType::Overpayment && loan->isFlag(lsfLoanOverpayment) &&
-        paymentRemainingProxy > 0 && totalPaid < roundedAmount &&
-        numPayments < kLoanMaximumPaymentsPerTransaction)
-    {
-        TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)};
-        TenthBips32 const overpaymentFeeRate{loan->at(sfOverpaymentFee)};
-
-        // It shouldn't be possible for the overpayment to be greater than
-        // totalValueOutstanding, because that would have been processed as
-        // another normal payment. But cap it just in case.
-        Number const overpaymentRaw =
-            std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy);
-
-        bool const fixEnabled = view.rules().enabled(fixCleanup3_2_0);
-        Number const overpayment = fixEnabled
-            ? roundToAsset(asset, overpaymentRaw, loanScale, Number::RoundingMode::Downward)
-            : overpaymentRaw;
-
-        // Post-amendment, the rounded overpayment can be zero; pre-amendment
-        // it's always positive given the surrounding guards.
-        if (!fixEnabled || overpayment > 0)
-        {
-            detail::ExtendedPaymentComponents const overpaymentComponents =
-                detail::computeOverpaymentComponents(
-                    view.rules(),
-                    asset,
-                    loanScale,
-                    overpayment,
-                    overpaymentInterestRate,
-                    overpaymentFeeRate,
-                    managementFeeRate);
-
-            // Don't process an overpayment if the whole amount (or more!)
-            // gets eaten by fees and interest.
-            if (overpaymentComponents.trackedPrincipalDelta > 0)
-            {
-                XRPL_ASSERT_PARTS(
-                    overpaymentComponents.untrackedInterest >= beast::kZero,
-                    "xrpl::loanMakePayment",
-                    "overpayment penalty did not reduce value of loan");
-                // Can't just use `periodicPayment` here, because it might
-                // change
-                auto periodicPaymentProxy = loan->at(sfPeriodicPayment);
-                if (auto const overResult = detail::doOverpayment(
-                        view.rules(),
-                        asset,
-                        loanScale,
-                        overpaymentComponents,
-                        totalValueOutstandingProxy,
-                        principalOutstandingProxy,
-                        managementFeeOutstandingProxy,
-                        periodicPaymentProxy,
-                        periodicRate,
-                        paymentRemainingProxy,
-                        managementFeeRate,
-                        j))
-                {
-                    totalParts += *overResult;
-                }
-                else if (overResult.error())
-                {
-                    // error() will be the TER returned if a payment is not
-                    // made. It will only evaluate to true if it's unsuccessful.
-                    // Otherwise, tesSUCCESS means nothing was done, so
-                    // continue.
-                    return std::unexpected(overResult.error());
-                }
-            }
-        }
-    }
-
-    // Check the final results are rounded, to double-check that the
-    // intermediate steps were rounded.
-    XRPL_ASSERT(
-        isRounded(asset, totalParts.principalPaid, loanScale) &&
-            totalParts.principalPaid >= beast::kZero,
-        "xrpl::loanMakePayment : total principal paid is valid");
-    XRPL_ASSERT(
-        isRounded(asset, totalParts.interestPaid, loanScale) &&
-            totalParts.interestPaid >= beast::kZero,
-        "xrpl::loanMakePayment : total interest paid is valid");
-    XRPL_ASSERT(
-        isRounded(asset, totalParts.valueChange, loanScale),
-        "xrpl::loanMakePayment : loan value change is valid");
-    XRPL_ASSERT(
-        isRounded(asset, totalParts.feePaid, loanScale) && totalParts.feePaid >= beast::kZero,
-        "xrpl::loanMakePayment : fee paid is valid");
-    return totalParts;
+    // LCOV_EXCL_START
+    UNREACHABLE("xrpl::loanMakePayment : invalid payment type");
+    return std::unexpected(tecINTERNAL);
+    // LCOV_EXCL_STOP
 }
 }  // namespace xrpl

src/test/app/LedgerMaster_test.cpp

@@ -5,17 +5,21 @@
 #include <test/jtx/noop.h>
 
 #include <xrpld/app/ledger/LedgerMaster.h>
+#include <xrpld/app/misc/SHAMapStore.h>
 #include <xrpld/core/Config.h>
 
+#include <xrpl/basics/ToString.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STObject.h>
 #include <xrpl/protocol/STTx.h>
 
 #include <cstdint>
 #include <memory>
+#include <sstream>
 #include <vector>
 
 namespace xrpl::test {
@@ -111,6 +115,71 @@ class LedgerMaster_test : public beast::unit_test::Suite
         }
     }
 
+    void
+    testCompleteLedgerRange(FeatureBitset features)
+    {
+        // Note that this test is intentionally very similar to
+        // SHAMapStore_test::testLedgerGaps, but has a different
+        // focus.
+
+        testcase("Complete Ledger operations");
+
+        using namespace test::jtx;
+
+        auto const deleteInterval = 8;
+
+        Env env{*this, envconfig(onlineDelete, deleteInterval)};
+
+        auto const alice = Account("alice");
+        env.fund(XRP(1000), alice);
+        env.close();
+
+        auto& lm = env.app().getLedgerMaster();
+        LedgerIndex minSeq = 2;
+        LedgerIndex maxSeq = env.closed()->header().seq;
+        auto& store = env.app().getSHAMapStore();
+        BEAST_EXPECT(store.rendezvous());
+        LedgerIndex lastRotated = store.getLastRotated();
+        BEAST_EXPECTS(maxSeq == 3, to_string(maxSeq));
+        BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers());
+        BEAST_EXPECTS(lastRotated == 3, to_string(lastRotated));
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
+        BEAST_EXPECT(minSeq + 1 > maxSeq - 1);
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
+
+        // Close enough ledgers to rotate a few times
+        for (int i = 0; i < 24; ++i)
+        {
+            for (int t = 0; t < 3; ++t)
+            {
+                env(noop(alice));
+            }
+            env.close();
+            BEAST_EXPECT(store.rendezvous());
+
+            ++maxSeq;
+
+            if (maxSeq == lastRotated + deleteInterval)
+            {
+                minSeq = lastRotated;
+                lastRotated = maxSeq;
+            }
+            BEAST_EXPECTS(
+                env.closed()->header().seq == maxSeq, to_string(env.closed()->header().seq));
+            BEAST_EXPECTS(store.getLastRotated() == lastRotated, to_string(store.getLastRotated()));
+            std::stringstream expectedRange;
+            expectedRange << minSeq << "-" << maxSeq;
+            BEAST_EXPECTS(lm.getCompleteLedgers() == expectedRange.str(), lm.getCompleteLedgers());
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
+        }
+    }
+
 public:
     void
     run() override
@@ -124,6 +193,7 @@ public:
     testWithFeats(FeatureBitset features)
     {
         testTxnIdFromIndex(features);
+        testCompleteLedgerRange(features);
     }
 };
 

src/test/app/Loan_test.cpp

@@ -446,7 +446,7 @@ protected:
                 env.test.BEAST_EXPECT(loan->at(sfPeriodicPayment) == periodicPayment);
                 env.test.BEAST_EXPECT(loan->at(sfFlags) == flags);
 
-                auto const ls = constructRoundedLoanState(loan);
+                auto const ls = constructLoanState(loan);
 
                 auto const interestRate = TenthBips32{loan->at(sfInterestRate)};
                 auto const paymentInterval = loan->at(sfPaymentInterval);
@@ -1119,7 +1119,7 @@ protected:
                     // No reason for this not to exist
                     return;
                 }
-                auto const current = constructRoundedLoanState(loanSle);
+                auto const current = constructLoanState(loanSle);
                 auto const errors = nextTrueState - current;
                 log << currencyLabel << " Loan balances: "
                     << "\n\tAmount taken: " << paymentComponents.trackedValueDelta
@@ -6056,7 +6056,7 @@ protected:
             auto const loanSle = env.le(loanKeylet);
             if (!BEAST_EXPECT(loanSle))
                 return;
-            auto const state = constructRoundedLoanState(loanSle);
+            auto const state = constructLoanState(loanSle);
 
             log << "Loan state:" << std::endl;
             log << "  ValueOutstanding: " << state.valueOutstanding << std::endl;

src/test/app/SHAMapStore_test.cpp

@@ -1,7 +1,9 @@
 #include <test/jtx/Env.h>
 #include <test/jtx/amount.h>
 #include <test/jtx/envconfig.h>
+#include <test/jtx/noop.h>
 
+#include <xrpld/app/ledger/LedgerMaster.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/app/main/NodeStoreScheduler.h>
 #include <xrpld/app/misc/SHAMapStore.h>
@@ -9,6 +11,7 @@
 #include <xrpld/core/Config.h>
 
 #include <xrpl/basics/ByteUtilities.h>
+#include <xrpl/basics/Number.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/config/BasicConfig.h>
@@ -26,12 +29,15 @@
 #include <boost/filesystem/path.hpp>
 
 #include <atomic>
+#include <cstddef>
 #include <cstdint>
 #include <limits>
 #include <map>
 #include <memory>
 #include <optional>
+#include <sstream>
 #include <string>
+#include <thread>
 #include <utility>
 
 namespace xrpl::test {
@@ -43,10 +49,7 @@ class SHAMapStore_test : public beast::unit_test::Suite
     static auto
     onlineDelete(std::unique_ptr<Config> cfg)
     {
-        cfg->ledgerHistory = kDeleteInterval;
-        auto& section = cfg->section(Sections::kNodeDatabase);
-        section.set(Keys::kOnlineDelete, std::to_string(kDeleteInterval));
-        return cfg;
+        return jtx::onlineDelete(std::move(cfg), kDeleteInterval);
     }
 
     static auto
@@ -144,11 +147,11 @@ class SHAMapStore_test : public beast::unit_test::Suite
         auto& store = env.app().getSHAMapStore();
 
         int ledgerSeq = 3;
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
         BEAST_EXPECT(!store.getLastRotated());
 
         env.close();
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         auto ledger = env.rpc("ledger", "validated");
         BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++)));
@@ -228,7 +231,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(kDeleteInterval + 4)));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         BEAST_EXPECT(store.getLastRotated() == kDeleteInterval + 3);
         lastRotated = store.getLastRotated();
@@ -255,7 +258,7 @@ public:
                 !getHash(ledgers[i]).empty());
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         BEAST_EXPECT(store.getLastRotated() == kDeleteInterval + lastRotated);
 
@@ -293,7 +296,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         // The database will always have back to ledger 2,
         // regardless of lastRotated.
@@ -308,7 +311,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
         BEAST_EXPECT(lastRotated != store.getLastRotated());
@@ -324,7 +327,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, kDeleteInterval + 1, lastRotated);
         BEAST_EXPECT(lastRotated != store.getLastRotated());
@@ -363,7 +366,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - 2, 2);
         BEAST_EXPECT(lastRotated == store.getLastRotated());
@@ -373,7 +376,7 @@ public:
         BEAST_EXPECT(!RPC::containsError(canDelete[jss::result]));
         BEAST_EXPECT(canDelete[jss::result][jss::can_delete] == ledgerSeq + (kDeleteInterval / 2));
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - 2, 2);
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
@@ -386,7 +389,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
 
@@ -402,7 +405,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
 
@@ -414,7 +417,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - firstBatch, firstBatch);
 
@@ -436,7 +439,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
 
@@ -448,7 +451,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
 
@@ -469,7 +472,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
 
@@ -481,7 +484,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        store.rendezvous();
+        BEAST_EXPECT(store.rendezvous());
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
 
@@ -604,6 +607,176 @@ public:
         BEAST_EXPECT(dbr->getName() == "3");
     }
 
+    void
+    testLedgerGaps()
+    {
+        // Note that this test is intentionally very similar to
+        // LedgerMaster_test::testCompleteLedgerRange, but has a different
+        // focus.
+
+        testcase("Wait for ledger gaps to fill in");
+
+        using namespace test::jtx;
+
+        Env env{*this, envconfig(onlineDelete)};
+
+        auto failureMessage = [&](char const* label, auto expected, auto actual) {
+            std::stringstream ss;
+            ss << label << ": Expected: " << expected << ", Got: " << actual;
+            return ss.str();
+        };
+
+        auto const alice = Account("alice");
+        env.fund(XRP(1000), alice);
+        env.close();
+
+        auto& lm = env.app().getLedgerMaster();
+        LedgerIndex minSeq = 2;
+        LedgerIndex maxSeq = env.closed()->header().seq;
+        auto& store = env.app().getSHAMapStore();
+        LedgerIndex lastRotated = store.getLastRotated();
+        while (lastRotated != 3)
+        {
+            BEAST_EXPECT(store.rendezvous());
+            lastRotated = store.getLastRotated();
+        }
+        BEAST_EXPECTS(maxSeq == 3, std::to_string(maxSeq));
+        BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers());
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
+        BEAST_EXPECT(minSeq + 1 > maxSeq - 1);
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
+        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
+
+        // Close enough ledgers to rotate a few times
+        while (maxSeq < 20)
+        {
+            for (int t = 0; t < 3; ++t)
+            {
+                env(noop(alice));
+            }
+            env.close();
+            BEAST_EXPECT(store.rendezvous());
+
+            ++maxSeq;
+
+            if (maxSeq + 1 == lastRotated + kDeleteInterval)
+            {
+                using namespace std::chrono_literals;
+
+                // The next ledger will trigger a rotation. Delete the
+                // current ledger from LedgerMaster.
+                std::this_thread::sleep_for(100ms);
+                LedgerIndex const deleteSeq = maxSeq;
+                {
+                    std::size_t iterations = 30;
+                    while (!lm.haveLedger(deleteSeq) && --iterations > 0)
+                    {
+                        std::this_thread::sleep_for(100ms);
+                    }
+                    // Even the slowest machines should be able to finalize deleteSeq within 10
+                    // loops (400ms). If this test ever actually fails feel free to lower this
+                    // cutoff. The intent of this test is to flag if the loop takes a very long
+                    // time, but still allow the rest of this function to finish.
+                    BEAST_EXPECTS(iterations > 20, std::to_string(iterations));
+                    if (!BEAST_EXPECT(lm.haveLedger(deleteSeq)))
+                        return;
+                }
+                lm.clearLedger(deleteSeq);
+
+                auto expectedRange = [](auto minSeq, auto deleteSeq, auto maxSeq) {
+                    std::stringstream expectedRange;
+                    expectedRange << minSeq << "-" << (deleteSeq - 1);
+                    if (deleteSeq + 1 == maxSeq)
+                    {
+                        expectedRange << "," << maxSeq;
+                    }
+                    else if (deleteSeq < maxSeq)
+                    {
+                        expectedRange << "," << (deleteSeq + 1) << "-" << maxSeq;
+                    }
+                    return expectedRange.str();
+                };
+                BEAST_EXPECTS(
+                    lm.getCompleteLedgers() == expectedRange(minSeq, deleteSeq, maxSeq),
+                    failureMessage(
+                        "Complete ledgers",
+                        expectedRange(minSeq, deleteSeq, maxSeq),
+                        lm.getCompleteLedgers()));
+                BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 1);
+
+                // Close another ledger, which will trigger a rotation, but the
+                // rotation will be stuck until the missing ledger is filled in.
+                env.close();
+                // Do not call rendezvous() here without a timeout; it will block until the missing
+                // ledger is backfilled. That will not happen automatically. It's a manual step that
+                // is done later in this test.
+                ++maxSeq;
+
+                // Nothing has changed
+                BEAST_EXPECTS(
+                    store.getLastRotated() == lastRotated,
+                    failureMessage("lastRotated", lastRotated, store.getLastRotated()));
+                BEAST_EXPECTS(
+                    lm.getCompleteLedgers() == expectedRange(minSeq, deleteSeq, maxSeq),
+                    failureMessage(
+                        "Complete ledgers",
+                        expectedRange(minSeq, deleteSeq, maxSeq),
+                        lm.getCompleteLedgers()));
+
+                // Close 5 more ledgers, waiting a little bit in between to
+                // simulate the ledger making progress while online delete waits
+                // for the missing ledger to be filled in.
+                // This ensures the healthWait check has time to run and
+                // detect the gap.
+                for (int l = 0; l < 5; ++l)
+                {
+                    env.close();
+                    ++maxSeq;
+                    // Nothing has changed
+                    BEAST_EXPECTS(
+                        store.getLastRotated() == lastRotated,
+                        failureMessage("lastRotated", lastRotated, store.getLastRotated()));
+                    BEAST_EXPECTS(
+                        lm.getCompleteLedgers() == expectedRange(minSeq, deleteSeq, maxSeq),
+                        failureMessage(
+                            "Complete Ledgers",
+                            expectedRange(minSeq, deleteSeq, maxSeq),
+                            lm.getCompleteLedgers()));
+                    // The Store is "stuck" in healthWait() and won't finish the run() loop until
+                    // it's backfilled
+                    BEAST_EXPECT(!store.rendezvous(100ms));
+                }
+
+                // Put the missing ledger back in LedgerMaster
+                lm.setLedgerRangePresent(deleteSeq, deleteSeq);
+
+                // Wait for the rotation to finish
+                BEAST_EXPECT(store.rendezvous());
+
+                minSeq = lastRotated;
+                lastRotated = deleteSeq + 1;
+            }
+            BEAST_EXPECT(maxSeq != lastRotated + kDeleteInterval);
+            BEAST_EXPECTS(
+                env.closed()->header().seq == maxSeq,
+                failureMessage("maxSeq", maxSeq, env.closed()->header().seq));
+            BEAST_EXPECTS(
+                store.getLastRotated() == lastRotated,
+                failureMessage("lastRotated", lastRotated, store.getLastRotated()));
+            std::stringstream expectedRange;
+            expectedRange << minSeq << "-" << maxSeq;
+            BEAST_EXPECTS(
+                lm.getCompleteLedgers() == expectedRange.str(),
+                failureMessage("CompleteLedgers", expectedRange.str(), lm.getCompleteLedgers()));
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
+        }
+    }
+
     void
     run() override
     {
@@ -611,6 +784,7 @@ public:
         testAutomatic();
         testCanDelete();
         testRotate();
+        testLedgerGaps();
     }
 };
 

src/test/jtx/envconfig.h

@@ -3,6 +3,7 @@
 #include <xrpld/core/Config.h>
 
 #include <atomic>
+#include <cstdint>
 #include <map>
 #include <memory>
 #include <string>
@@ -62,6 +63,19 @@ envconfig(F&& modfunc, Args&&... args)
     return modfunc(envconfig(), std::forward<Args>(args)...);
 }
 
+/**
+ * @brief adjust config to enable online_delete
+ *
+ * @param cfg config instance to be modified
+ *
+ * @param deleteInterval how many new ledgers should be available before
+ * rotating. Defaults to 8, because the standalone minimum is 8.
+ *
+ * @return unique_ptr to Config instance
+ */
+std::unique_ptr<Config>
+onlineDelete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval = 8);
+
 /**
  * @brief adjust config so no admin ports are enabled
  *

src/test/jtx/impl/envconfig.cpp

@@ -7,8 +7,10 @@
 #include <xrpl/config/Constants.h>
 
 #include <atomic>
+#include <cstdint>
 #include <map>
 #include <memory>
+#include <string>
 #include <vector>
 
 namespace xrpl::test {
@@ -60,6 +62,15 @@ setupConfigForUnitTests(Config& cfg)
 
 namespace jtx {
 
+std::unique_ptr<Config>
+onlineDelete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval)
+{
+    cfg->ledgerHistory = deleteInterval;
+    auto& section = cfg->section(Sections::kNodeDatabase);
+    section.set(Keys::kOnlineDelete, std::to_string(deleteInterval));
+    return cfg;
+}
+
 std::unique_ptr<Config>
 noAdmin(std::unique_ptr<Config> cfg)
 {

src/test/nodestore/Timing_test.cpp

@@ -1,729 +0,0 @@
-#include <test/nodestore/TestBase.h>
-#include <test/unit_test/SuiteJournal.h>
-
-#include <xrpl/basics/Blob.h>
-#include <xrpl/basics/ByteUtilities.h>
-#include <xrpl/basics/base_uint.h>
-#include <xrpl/basics/contract.h>
-#include <xrpl/basics/safe_cast.h>
-#include <xrpl/beast/unit_test/suite.h>
-#include <xrpl/beast/unit_test/thread.h>
-#include <xrpl/beast/utility/Journal.h>
-#include <xrpl/beast/utility/temp_dir.h>
-#include <xrpl/beast/xor_shift_engine.h>
-#include <xrpl/config/BasicConfig.h>
-#include <xrpl/config/Constants.h>
-#include <xrpl/nodestore/Backend.h>
-#include <xrpl/nodestore/DummyScheduler.h>
-#include <xrpl/nodestore/Manager.h>
-#include <xrpl/nodestore/NodeObject.h>
-#include <xrpl/nodestore/Scheduler.h>
-#include <xrpl/nodestore/Types.h>
-
-#include <boost/algorithm/string/classification.hpp>
-#include <boost/algorithm/string/split.hpp>
-
-#include <algorithm>
-#include <atomic>
-#include <chrono>
-#include <cstddef>
-#include <cstdint>
-#include <exception>
-#include <functional>
-#include <iomanip>
-#include <ios>
-#include <memory>
-#include <ostream>
-#include <random>
-#include <sstream>
-#include <string>
-#include <utility>
-#include <vector>
-
-#ifndef NODESTORE_TIMING_DO_VERIFY
-#define NODESTORE_TIMING_DO_VERIFY 0
-#endif
-
-namespace xrpl::NodeStore {
-
-std::unique_ptr<Backend>
-makeBackend(Section const& config, Scheduler& scheduler, beast::Journal journal)
-{
-    return Manager::instance().makeBackend(config, megabytes(4), scheduler, journal);
-}
-
-// Fill memory with random bits
-template <class Generator>
-static void
-rngcpy(void* buffer, std::size_t bytes, Generator& g)
-{
-    using result_type = Generator::result_type;
-    while (bytes >= sizeof(result_type))
-    {
-        auto const v = g();
-        memcpy(buffer, &v, sizeof(v));
-        buffer = reinterpret_cast<std::uint8_t*>(buffer) + sizeof(v);
-        bytes -= sizeof(v);
-    }
-
-    if (bytes > 0)
-    {
-        auto const v = g();
-        memcpy(buffer, &v, bytes);
-    }
-}
-
-// Instance of node factory produces a deterministic sequence
-// of random NodeObjects within the given
-class Sequence
-{
-private:
-    static constexpr auto kMinLedger = 1;
-    static constexpr auto kMaxLedger = 1000000;
-    static constexpr auto kMinSize = 250;
-    static constexpr auto kMaxSize = 1250;
-
-    beast::xor_shift_engine gen_;
-    std::uint8_t prefix_;
-    std::discrete_distribution<std::uint32_t> dType_;
-    std::uniform_int_distribution<std::uint32_t> dSize_;
-
-public:
-    explicit Sequence(std::uint8_t prefix)
-        : prefix_(prefix)
-        // uniform distribution over hotLEDGER - hotTRANSACTION_NODE
-        // but exclude  hotTRANSACTION = 2 (removed)
-        , dType_({1, 1, 0, 1, 1})
-        , dSize_(kMinSize, kMaxSize)
-    {
-    }
-
-    // Returns the n-th key
-    uint256
-    key(std::size_t n)
-    {
-        gen_.seed(n + 1);
-        uint256 result;
-        rngcpy(&*result.begin(), result.size(), gen_);
-        return result;
-    }
-
-    // Returns the n-th complete NodeObject
-    std::shared_ptr<NodeObject>
-    obj(std::size_t n)
-    {
-        gen_.seed(n + 1);
-        uint256 key;
-        auto const data = static_cast<std::uint8_t*>(&*key.begin());
-        *data = prefix_;
-        rngcpy(data + 1, key.size() - 1, gen_);
-        Blob value(dSize_(gen_));
-        rngcpy(&value[0], value.size(), gen_);
-        return NodeObject::createObject(
-            safeCast<NodeObjectType>(dType_(gen_)), std::move(value), key);
-    }
-
-    // returns a batch of NodeObjects starting at n
-    void
-    batch(std::size_t n, Batch& b, std::size_t size)
-    {
-        b.clear();
-        b.reserve(size);
-        while ((size--) != 0u)
-            b.emplace_back(obj(n++));
-    }
-};
-
-//----------------------------------------------------------------------------------
-
-class Timing_test : public beast::unit_test::Suite
-{
-public:
-    static constexpr auto kMissingNodePercent = 20;  // percent of fetches for missing nodes
-
-    std::size_t const defaultRepeat = 3;
-#ifndef NDEBUG
-    std::size_t const defaultItems = 10000;
-#else
-    std::size_t const defaultItems = 100000;  // release
-#endif
-
-    using clock_type = std::chrono::steady_clock;
-    using duration_type = std::chrono::milliseconds;
-
-    struct Params
-    {
-        std::size_t items;
-        std::size_t threads;
-    };
-
-    static std::string
-    toString(Section const& config)
-    {
-        std::string s;
-        for (auto iter = config.begin(); iter != config.end(); ++iter)
-            s += (iter != config.begin() ? "," : "") + iter->first + "=" + iter->second;
-        return s;
-    }
-
-    static std::string
-    toString(duration_type const& d)
-    {
-        std::stringstream ss;
-        ss << std::fixed << std::setprecision(3) << (d.count() / 1000.) << "s";
-        return ss.str();
-    }
-
-    static Section
-    parse(std::string s)
-    {
-        Section section;
-        std::vector<std::string> v;
-        boost::split(v, s, boost::algorithm::is_any_of(","));
-        section.append(v);
-        return section;
-    }
-
-    //--------------------------------------------------------------------------
-
-    // Workaround for GCC's parameter pack expansion in lambdas
-    // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47226
-    template <class Body>
-    class ParallelForLambda
-    {
-    private:
-        std::size_t const n_;
-        std::atomic<std::size_t>& c_;
-
-    public:
-        ParallelForLambda(std::size_t n, std::atomic<std::size_t>& c) : n_(n), c_(c)
-        {
-        }
-
-        template <class... Args>
-        void
-        operator()(Args&&... args)
-        {
-            Body body(args...);
-            for (;;)
-            {
-                auto const i = c_++;
-                if (i >= n_)
-                    break;
-                body(i);
-            }
-        }
-    };
-
-    /*  Execute parallel-for loop.
-
-        Constructs `number_of_threads` instances of `Body`
-        with `args...` parameters and runs them on individual threads
-        with unique loop indexes in the range [0, n).
-    */
-    template <class Body, class... Args>
-    void
-    parallelFor(std::size_t const n, std::size_t numberOfThreads, Args const&... args)
-    {
-        std::atomic<std::size_t> c(0);
-        std::vector<beast::unit_test::Thread> t;
-        t.reserve(numberOfThreads);
-        for (std::size_t id = 0; id < numberOfThreads; ++id)
-            t.emplace_back(*this, ParallelForLambda<Body>(n, c), args...);
-        for (auto& _ : t)
-            _.join();
-    }
-
-    template <class Body, class... Args>
-    void
-    parallelForId(std::size_t const n, std::size_t numberOfThreads, Args const&... args)
-    {
-        std::atomic<std::size_t> c(0);
-        std::vector<beast::unit_test::Thread> t;
-        t.reserve(numberOfThreads);
-        for (std::size_t id = 0; id < numberOfThreads; ++id)
-            t.emplace_back(*this, ParallelForLambda<Body>(n, c), id, args...);
-        for (auto& _ : t)
-            _.join();
-    }
-
-    //--------------------------------------------------------------------------
-
-    // Insert only
-    void
-    doInsert(Section const& config, Params const& params, beast::Journal journal)
-    {
-        DummyScheduler scheduler;
-        auto backend = makeBackend(config, scheduler, journal);
-        BEAST_EXPECT(backend != nullptr);
-        backend->open();
-
-        class Body
-        {
-        private:
-            Suite& suite_;
-            Backend& backend_;
-            Sequence seq_;
-
-        public:
-            explicit Body(Suite& s, Backend& backend) : suite_(s), backend_(backend), seq_(1)
-            {
-            }
-
-            void
-            operator()(std::size_t i)
-            {
-                try
-                {
-                    backend_.store(seq_.obj(i));
-                }
-                catch (std::exception const& e)
-                {
-                    suite_.fail(e.what());
-                }
-            }
-        };
-
-        try
-        {
-            parallelFor<Body>(params.items, params.threads, std::ref(*this), std::ref(*backend));
-        }
-        catch (std::exception const&)
-        {
-#if NODESTORE_TIMING_DO_VERIFY
-            backend->verify();
-#endif
-            rethrow();
-        }
-        backend->close();
-    }
-
-    // Fetch existing keys
-    void
-    doFetch(Section const& config, Params const& params, beast::Journal journal)
-    {
-        DummyScheduler scheduler;
-        auto backend = makeBackend(config, scheduler, journal);
-        BEAST_EXPECT(backend != nullptr);
-        backend->open();
-
-        class Body
-        {
-        private:
-            Suite& suite_;
-            Backend& backend_;
-            Sequence seq1_;
-            beast::xor_shift_engine gen_;
-            std::uniform_int_distribution<std::size_t> dist_;
-
-        public:
-            Body(std::size_t id, Suite& s, Params const& params, Backend& backend)
-                : suite_(s), backend_(backend), seq1_(1), gen_(id + 1), dist_(0, params.items - 1)
-            {
-            }
-
-            void
-            operator()(std::size_t i)
-            {
-                try
-                {
-                    std::shared_ptr<NodeObject> obj;
-                    std::shared_ptr<NodeObject> result;
-                    obj = seq1_.obj(dist_(gen_));
-                    backend_.fetch(obj->getHash(), &result);
-                    suite_.expect(result && isSame(result, obj));
-                }
-                catch (std::exception const& e)
-                {
-                    suite_.fail(e.what());
-                }
-            }
-        };
-        try
-        {
-            parallelForId<Body>(
-                params.items,
-                params.threads,
-                std::ref(*this),
-                std::ref(params),
-                std::ref(*backend));
-        }
-        catch (std::exception const&)
-        {
-#if NODESTORE_TIMING_DO_VERIFY
-            backend->verify();
-#endif
-            rethrow();
-        }
-        backend->close();
-    }
-
-    // Perform lookups of non-existent keys
-    void
-    doMissing(Section const& config, Params const& params, beast::Journal journal)
-    {
-        DummyScheduler scheduler;
-        auto backend = makeBackend(config, scheduler, journal);
-        BEAST_EXPECT(backend != nullptr);
-        backend->open();
-
-        class Body
-        {
-        private:
-            Suite& suite_;
-            // Params const& params_;
-            Backend& backend_;
-            Sequence seq2_;
-            beast::xor_shift_engine gen_;
-            std::uniform_int_distribution<std::size_t> dist_;
-
-        public:
-            Body(std::size_t id, Suite& s, Params const& params, Backend& backend)
-                : suite_(s)
-                //, params_ (params)
-                , backend_(backend)
-                , seq2_(2)
-                , gen_(id + 1)
-                , dist_(0, params.items - 1)
-            {
-            }
-
-            void
-            operator()(std::size_t i)
-            {
-                try
-                {
-                    auto const hash = seq2_.key(i);
-                    std::shared_ptr<NodeObject> result;
-                    backend_.fetch(hash, &result);
-                    suite_.expect(!result);
-                }
-                catch (std::exception const& e)
-                {
-                    suite_.fail(e.what());
-                }
-            }
-        };
-
-        try
-        {
-            parallelForId<Body>(
-                params.items,
-                params.threads,
-                std::ref(*this),
-                std::ref(params),
-                std::ref(*backend));
-        }
-        catch (std::exception const&)
-        {
-#if NODESTORE_TIMING_DO_VERIFY
-            backend->verify();
-#endif
-            rethrow();
-        }
-        backend->close();
-    }
-
-    // Fetch with present and missing keys
-    void
-    doMixed(Section const& config, Params const& params, beast::Journal journal)
-    {
-        DummyScheduler scheduler;
-        auto backend = makeBackend(config, scheduler, journal);
-        BEAST_EXPECT(backend != nullptr);
-        backend->open();
-
-        class Body
-        {
-        private:
-            Suite& suite_;
-            // Params const& params_;
-            Backend& backend_;
-            Sequence seq1_;
-            Sequence seq2_;
-            beast::xor_shift_engine gen_;
-            std::uniform_int_distribution<std::uint32_t> rand_;
-            std::uniform_int_distribution<std::size_t> dist_;
-
-        public:
-            Body(std::size_t id, Suite& s, Params const& params, Backend& backend)
-                : suite_(s)
-                //, params_ (params)
-                , backend_(backend)
-                , seq1_(1)
-                , seq2_(2)
-                , gen_(id + 1)
-                , rand_(0, 99)
-                , dist_(0, params.items - 1)
-            {
-            }
-
-            void
-            operator()(std::size_t i)
-            {
-                try
-                {
-                    if (rand_(gen_) < kMissingNodePercent)
-                    {
-                        auto const hash = seq2_.key(dist_(gen_));
-                        std::shared_ptr<NodeObject> result;
-                        backend_.fetch(hash, &result);
-                        suite_.expect(!result);
-                    }
-                    else
-                    {
-                        std::shared_ptr<NodeObject> obj;
-                        std::shared_ptr<NodeObject> result;
-                        obj = seq1_.obj(dist_(gen_));
-                        backend_.fetch(obj->getHash(), &result);
-                        suite_.expect(result && isSame(result, obj));
-                    }
-                }
-                catch (std::exception const& e)
-                {
-                    suite_.fail(e.what());
-                }
-            }
-        };
-
-        try
-        {
-            parallelForId<Body>(
-                params.items,
-                params.threads,
-                std::ref(*this),
-                std::ref(params),
-                std::ref(*backend));
-        }
-        catch (std::exception const&)
-        {
-#if NODESTORE_TIMING_DO_VERIFY
-            backend->verify();
-#endif
-            rethrow();
-        }
-        backend->close();
-    }
-
-    // Simulate an xrpld workload:
-    // Each thread randomly:
-    //      inserts a new key
-    //      fetches an old key
-    //      fetches recent, possibly non existent data
-    void
-    doWork(Section const& config, Params const& params, beast::Journal journal)
-    {
-        DummyScheduler scheduler;
-        auto backend = makeBackend(config, scheduler, journal);
-        BEAST_EXPECT(backend != nullptr);
-        backend->setDeletePath();
-        backend->open();
-
-        class Body
-        {
-        private:
-            Suite& suite_;
-            Params const& params_;
-            Backend& backend_;
-            Sequence seq1_;
-            beast::xor_shift_engine gen_;
-            std::uniform_int_distribution<std::uint32_t> rand_;
-            std::uniform_int_distribution<std::size_t> recent_;
-            std::uniform_int_distribution<std::size_t> older_;
-
-        public:
-            Body(std::size_t id, Suite& s, Params const& params, Backend& backend)
-                : suite_(s)
-                , params_(params)
-                , backend_(backend)
-                , seq1_(1)
-                , gen_(id + 1)
-                , rand_(0, 99)
-                , recent_(params.items, (params.items * 2) - 1)
-                , older_(0, params.items - 1)
-            {
-            }
-
-            void
-            operator()(std::size_t i)
-            {
-                try
-                {
-                    if (rand_(gen_) < 200)
-                    {
-                        // historical lookup
-                        std::shared_ptr<NodeObject> obj;
-                        std::shared_ptr<NodeObject> result;
-                        auto const j = older_(gen_);
-                        obj = seq1_.obj(j);
-                        backend_.fetch(obj->getHash(), &result);
-                        suite_.expect(result != nullptr);
-                        suite_.expect(isSame(result, obj));
-                    }
-
-                    char p[2];
-                    p[0] = rand_(gen_) < 50 ? 0 : 1;
-                    p[1] = 1 - p[0];
-                    for (char const op : p)
-                    {
-                        // NOLINTNEXTLINE(bugprone-switch-missing-default-case)
-                        switch (op)
-                        {
-                            case 0: {
-                                // fetch recent
-                                std::shared_ptr<NodeObject> obj;
-                                std::shared_ptr<NodeObject> result;
-                                auto const j = recent_(gen_);
-                                obj = seq1_.obj(j);
-                                backend_.fetch(obj->getHash(), &result);
-                                suite_.expect(!result || isSame(result, obj));
-                                break;
-                            }
-
-                            case 1: {
-                                // insert new
-                                auto const j = i + params_.items;
-                                backend_.store(seq1_.obj(j));
-                                break;
-                            }
-                        }
-                    }
-                }
-                catch (std::exception const& e)
-                {
-                    suite_.fail(e.what());
-                }
-            }
-        };
-
-        try
-        {
-            parallelForId<Body>(
-                params.items,
-                params.threads,
-                std::ref(*this),
-                std::ref(params),
-                std::ref(*backend));
-        }
-        catch (std::exception const&)
-        {
-#if NODESTORE_TIMING_DO_VERIFY
-            backend->verify();
-#endif
-            rethrow();
-        }
-        backend->close();
-    }
-
-    //--------------------------------------------------------------------------
-
-    using test_func = void (Timing_test::*)(Section const&, Params const&, beast::Journal);
-    using test_list = std::vector<std::pair<std::string, test_func>>;
-
-    duration_type
-    doTest(test_func f, Section const& config, Params const& params, beast::Journal journal)
-    {
-        auto const start = clock_type::now();
-        (this->*f)(config, params, journal);
-        return std::chrono::duration_cast<duration_type>(clock_type::now() - start);
-    }
-
-    void
-    doTests(
-        std::size_t threads,
-        test_list const& tests,
-        std::vector<std::string> const& configStrings)
-    {
-        using std::setw;
-        int w = 8;
-        for (auto const& test : tests)
-        {
-            w = std::max<std::basic_string<char>::size_type>(w, test.first.size());
-        }
-        log << threads << " Thread" << (threads > 1 ? "s" : "") << ", " << defaultItems
-            << " Objects" << std::endl;
-        {
-            std::stringstream ss;
-            ss << std::left << setw(10) << "Backend" << std::right;
-            for (auto const& test : tests)
-                ss << " " << setw(w) << test.first;
-            log << ss.str() << std::endl;
-        }
-
-        using beast::Severity;
-        test::SuiteJournal journal("Timing_test", *this);
-
-        for (auto const& configString : configStrings)
-        {
-            Params params{};
-            params.items = defaultItems;
-            params.threads = threads;
-            for (auto i = defaultRepeat; (i--) != 0u;)
-            {
-                beast::TempDir const tempDir;
-                Section config = parse(configString);
-                config.set(Keys::kPath, tempDir.path());
-                std::stringstream ss;
-                ss << std::left << setw(10) << get(config, Keys::kType, std::string())
-                   << std::right;
-                for (auto const& test : tests)
-                {
-                    ss << " " << setw(w) << toString(doTest(test.second, config, params, journal));
-                }
-                ss << "   " << toString(config);
-                log << ss.str() << std::endl;
-            }
-        }
-    }
-
-    void
-    run() override
-    {
-        testcase("Timing", beast::unit_test::AbortT::AbortOnFail);
-
-        /*  Parameters:
-
-            repeat          Number of times to repeat each test
-            items           Number of objects to create in the database
-
-        */
-        std::string const defaultArgs =
-            "type=nudb"
-#if XRPL_ROCKSDB_AVAILABLE
-            ";type=rocksdb,open_files=2000,filter_bits=12,cache_mb=256,"
-            "file_size_mb=8,file_size_mult=2"
-#endif
-            ;
-
-        test_list const tests = {
-            {"Insert", &Timing_test::doInsert},
-            {"Fetch", &Timing_test::doFetch},
-            {"Missing", &Timing_test::doMissing},
-            {"Mixed", &Timing_test::doMixed},
-            {"Work", &Timing_test::doWork}};
-
-        auto args = arg().empty() ? defaultArgs : arg();
-        std::vector<std::string> configStrings;
-        boost::split(configStrings, args, boost::algorithm::is_any_of(";"));
-        for (auto iter = configStrings.begin(); iter != configStrings.end();)
-        {
-            if (iter->empty())
-            {
-                iter = configStrings.erase(iter);
-            }
-            else
-            {
-                ++iter;
-            }
-        }
-
-        doTests(1, tests, configStrings);
-        doTests(4, tests, configStrings);
-        doTests(8, tests, configStrings);
-        // do_tests (16, tests, config_strings);
-    }
-};
-
-BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(Timing, nodestore, xrpl, 1);
-
-}  // namespace xrpl::NodeStore

src/xrpld/app/ledger/LedgerMaster.h

@@ -123,7 +123,10 @@ public:
     failedSave(std::uint32_t seq, uint256 const& hash);
 
     std::string
-    getCompleteLedgers();
+    getCompleteLedgers() const;
+
+    std::size_t
+    missingFromCompleteLedgerRange(LedgerIndex first, LedgerIndex last) const;
 
     /**
      * Apply held transactions to the open ledger
@@ -348,7 +351,7 @@ private:
     // A set of transactions to replay during the next close
     std::unique_ptr<LedgerReplay> replayData_;
 
-    std::recursive_mutex completeLock_;
+    std::recursive_mutex mutable completeLock_;
     RangeSet<std::uint32_t> completeLedgers_;
 
     // Publish thread is running.

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

@@ -57,6 +57,7 @@
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
+#include <boost/icl/concept/interval_associator.hpp>
 #include <boost/icl/concept/interval_set.hpp>
 
 #include <xrpl.pb.h>
@@ -1576,12 +1577,33 @@ LedgerMaster::getPublishedLedger()
 }
 
 std::string
-LedgerMaster::getCompleteLedgers()
+LedgerMaster::getCompleteLedgers() const
 {
     std::scoped_lock const sl(completeLock_);
     return to_string(completeLedgers_);
 }
 
+std::size_t
+LedgerMaster::missingFromCompleteLedgerRange(LedgerIndex first, LedgerIndex last) const
+{
+    if (first > last)
+    {
+        // LCOV_EXCL_START
+        UNREACHABLE("xrpl::LedgerMaster::missingFromCompleteLedgerRange : invalid parameters");
+        return 0;
+        // LCOV_EXCL_STOP
+    }
+
+    RangeSet<LedgerIndex> const target{range(first, last)};
+
+    auto const missing = [&target, this] {
+        std::scoped_lock const sl(completeLock_);
+        return target - completeLedgers_;
+    }();
+
+    return boost::icl::size(missing);
+}
+
 std::optional<NetClock::time_point>
 LedgerMaster::getCloseTimeBySeq(LedgerIndex ledgerIndex)
 {

src/xrpld/app/misc/SHAMapStore.h

@@ -8,6 +8,7 @@
 #include <xrpl/nodestore/Scheduler.h>
 #include <xrpl/protocol/Protocol.h>
 
+#include <chrono>
 #include <cstdint>
 #include <memory>
 #include <optional>
@@ -34,8 +35,8 @@ public:
     virtual void
     start() = 0;
 
-    virtual void
-    rendezvous() const = 0;
+    [[nodiscard]] virtual bool
+    rendezvous(std::optional<std::chrono::milliseconds> const& timeout = {}) const = 0;
 
     virtual void
     stop() = 0;

src/xrpld/app/misc/SHAMapStoreImp.cpp

@@ -8,6 +8,7 @@
 #include <xrpl/basics/ByteUtilities.h>
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/contract.h>
+#include <xrpl/basics/scope.h>
 #include <xrpl/beast/core/CurrentThreadName.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/instrumentation.h>
@@ -32,6 +33,8 @@
 #include <boost/filesystem/path.hpp>
 
 #include <algorithm>
+#include <chrono>
+#include <cstddef>
 #include <cstdint>
 #include <functional>
 #include <limits>
@@ -236,14 +239,21 @@ SHAMapStoreImp::onLedgerClosed(std::shared_ptr<Ledger const> const& ledger)
     cond_.notify_one();
 }
 
-void
-SHAMapStoreImp::rendezvous() const
+bool
+SHAMapStoreImp::rendezvous(std::optional<std::chrono::milliseconds> const& timeout) const
 {
     if (!working_)
-        return;
+        return true;
+
+    auto notWorking = [&] { return !working_; };
 
     std::unique_lock<std::mutex> lock(mutex_);
-    rendezvous_.wait(lock, [&] { return !working_; });
+    if (timeout)
+    {
+        return rendezvous_.wait_for(lock, *timeout, notWorking);
+    }
+    rendezvous_.wait(lock, notWorking);
+    return true;
 }
 
 int
@@ -330,6 +340,16 @@ SHAMapStoreImp::run()
         bool const readyToRotate = validatedSeq >= lastRotated + deleteInterval_ &&
             canDelete_ >= lastRotated - 1 && healthWait() == HealthResult::KeepGoing;
 
+        {
+            JLOG(journal_.trace()) << "run: Setting lastGoodValidatedLedger_ to " << validatedSeq;
+            // Note that this is set after the healthWait() check, so that we
+            // don't start the rotation until the validated ledger is fully
+            // processed. It is not guaranteed to be done at this point. It also
+            // allows the testLedgerGaps unit test to work.
+            std::unique_lock<std::mutex> const lock(mutex_);
+            lastGoodValidatedLedger_ = validatedSeq;
+        }
+
         // will delete up to (not including) lastRotated
         if (readyToRotate)
         {
@@ -337,7 +357,8 @@ SHAMapStoreImp::run()
                                   << lastRotated << " deleteInterval " << deleteInterval_
                                   << " canDelete_ " << canDelete_ << " state "
                                   << app_.getOPs().strOperatingMode(false) << " age "
-                                  << ledgerMaster_->getValidatedLedgerAge().count() << 's';
+                                  << ledgerMaster_->getValidatedLedgerAge().count()
+                                  << "s. Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
 
             clearPrior(lastRotated);
             if (healthWait() == HealthResult::Stopping)
@@ -412,7 +433,9 @@ SHAMapStoreImp::run()
                     clearCaches(validatedSeq);
                 });
 
-            JLOG(journal_.warn()) << "finished rotation " << validatedSeq;
+            JLOG(journal_.warn()) << "finished rotation. validatedSeq: " << validatedSeq
+                                  << ", lastRotated: " << lastRotated
+                                  << ". Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
         }
     }
 }
@@ -656,20 +679,70 @@ SHAMapStoreImp::clearPrior(LedgerIndex lastRotated)
 SHAMapStoreImp::HealthResult
 SHAMapStoreImp::healthWait()
 {
-    auto age = ledgerMaster_->getValidatedLedgerAge();
-    OperatingMode mode = netOPs_->getOperatingMode();
-    std::unique_lock lock(mutex_);
-    while (!stop_ && (mode != OperatingMode::FULL || age > ageThreshold_))
-    {
-        lock.unlock();
-        JLOG(journal_.warn()) << "Waiting " << recoveryWaitTime_.count()
-                              << "s for node to stabilize. state: "
-                              << app_.getOPs().strOperatingMode(mode, false) << ". age "
-                              << age.count() << 's';
-        std::this_thread::sleep_for(recoveryWaitTime_);
+    // Gets the current status of the server from ledgerMaster_ and netOPs_. Must be called
+    // while mutex_ is unlocked to avoid unlikely, but possible, deadlock with ledgerMaster_'s
+    // completeLock_.
+    // Releasing the lock may mean that status will be slightly out of date when the lock is
+    // reacquired, but it's close enough. In a normal rotation, healthWait() is called frequently,
+    // so a false positive will be detected on the next call, and a false negative will be detected
+    // in the next loop iteration. Database rotation is important, but not timely, so an extra
+    // delay is fine.
+    auto readServerStatus = [this](
+                                LedgerIndex& index,
+                                std::chrono::seconds& age,
+                                OperatingMode& mode,
+                                std::size_t& numMissing,
+                                LedgerIndex const lowerBound,
+                                ScopeUnlock<decltype(mutex_)> const&) {
+        index = ledgerMaster_->getValidLedgerIndex();
         age = ledgerMaster_->getValidatedLedgerAge();
         mode = netOPs_->getOperatingMode();
-        lock.lock();
+
+        numMissing =
+            lowerBound == 0 ? 0 : ledgerMaster_->missingFromCompleteLedgerRange(lowerBound, index);
+    };
+
+    // Tracked server status properties
+    LedgerIndex index = 0;
+    std::chrono::seconds age;
+    OperatingMode mode = OperatingMode::DISCONNECTED;
+    std::size_t numMissing = 0;
+    LedgerIndex lastLedger = 0;
+
+    std::unique_lock lock(mutex_);
+
+    auto const waitTime = recoveryWaitTime_;
+    auto const ageThreshold = ageThreshold_;
+    {
+        auto const lowerBound = lastGoodValidatedLedger_;
+
+        ScopeUnlock const unlock(lock);
+
+        readServerStatus(index, age, mode, numMissing, lowerBound, unlock);
+    }
+    while (!stop_ && (mode != OperatingMode::FULL || age > ageThreshold || numMissing > 0))
+    {
+        // this value shouldn't change, so grab it while we have the
+        // lock
+        auto const lowerBound = lastGoodValidatedLedger_;
+
+        ScopeUnlock const unlock(lock);
+
+        auto const stream = std::invoke([mode, age, ageThreshold, index, lastLedger, this]() {
+            if (mode != OperatingMode::FULL || age > ageThreshold)
+                return journal_.warn();
+            if (index != lastLedger)
+                return journal_.trace();
+            return journal_.info();
+        });
+        JLOG(stream) << "Waiting " << waitTime.count() << "s for node to stabilize. state: "
+                     << app_.getOPs().strOperatingMode(mode, false) << ". age " << age.count()
+                     << "s. Missing ledgers: " << numMissing << ".  Expect: " << lowerBound << "-"
+                     << index << ". Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
+        std::this_thread::sleep_for(waitTime);
+
+        readServerStatus(index, age, mode, numMissing, lowerBound, unlock);
+        lastLedger = index;
     }
 
     return stop_ ? HealthResult::Stopping : HealthResult::KeepGoing;

src/xrpld/app/misc/SHAMapStoreImp.h

@@ -88,6 +88,11 @@ private:
     std::thread thread_;
     bool stop_ = false;
     bool healthy_ = true;
+    // Used to prevent ledger gaps from forming during online deletion. Keeps
+    // track of the last validated ledger that was processed without gaps. There
+    // are no guarantees about gaps while online delete is not running. For
+    // that, use advisory_delete and check for gaps externally.
+    LedgerIndex lastGoodValidatedLedger_ = 0;
     mutable std::condition_variable cond_;
     mutable std::condition_variable rendezvous_;
     mutable std::mutex mutex_;
@@ -102,12 +107,12 @@ private:
     std::chrono::milliseconds backOff_{100};
     std::chrono::seconds ageThreshold_{60};
     /**
-     * If  the node is out of sync during an online_delete healthWait()
-     * call, sleep the thread for this time, and continue checking until
-     * recovery.
+     * If  the node is out of sync, or any recent ledgers are not
+     * available during an online_delete healthWait() call, sleep
+     * the thread for this time, and continue checking until recovery.
      * See also: "recovery_wait_seconds" in xrpld-example.cfg
      */
-    std::chrono::seconds recoveryWaitTime_{5};
+    std::chrono::seconds recoveryWaitTime_{2};
 
     // these do not exist upon SHAMapStore creation, but do exist
     // as of run() or before
@@ -163,8 +168,8 @@ public:
     void
     onLedgerClosed(std::shared_ptr<Ledger const> const& ledger) override;
 
-    void
-    rendezvous() const override;
+    bool
+    rendezvous(std::optional<std::chrono::milliseconds> const& timeout = {}) const override;
     int
     fdRequired() const override;