Refactor log checks for unittest and build logs

Reorganize log checks to streamline error reporting.

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

@@ -227,7 +227,8 @@ jobs:
               --build . \
               --config "${BUILD_TYPE}" \
               --parallel "${BUILD_NPROC}" \
-              --target "${CMAKE_TARGET}"
+              --target "${CMAKE_TARGET}" \
+              2>&1 | tee build.log
 
       - name: Show ccache statistics
         if: ${{ inputs.ccache_enabled }}
@@ -325,7 +326,7 @@ jobs:
           LD_PRELOAD="$PRELOAD" ./xrpld --unittest --unittest-jobs "${BUILD_NPROC}" 2>&1 | tee unittest.log
 
       - name: Show test failure summary
-        if: ${{ failure() && !inputs.build_only }}
+        if: ${{ failure() }}
         env:
           WORKING_DIR: ${{ runner.os == 'Windows' && format('{0}\{1}', env.BUILD_DIR, inputs.build_type) || env.BUILD_DIR }}
         run: |
@@ -336,13 +337,16 @@ jobs:
 
           cd "${WORKING_DIR}"
 
-          if [ ! -f unittest.log ]; then
-              echo "unittest.log not found; embedded tests may not have run."
-              exit 0
-          fi
-
-          if ! grep -E "failed" unittest.log; then
-              echo "Log present but no failure lines found in unittest.log."
+          if [ -f unittest.log ]; then
+              if ! grep -E "failed" unittest.log | grep -vE "^I[0-9]|^[0-9]+> (ERR:|FTL:)"; then
+                  echo "unittest.log present but no failure lines found."
+              fi
+          elif [ -f build.log ]; then
+              if ! grep -E "error:" build.log; then
+                  echo "build.log present but no compile errors found."
+              fi
+          else
+              echo "unittest.log/build.log not found; something went wrong."
           fi
       - name: Debug failure (Linux)
         if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }}

BUILD.md

@@ -41,10 +41,7 @@ Our Linux CI tooling is distro-independent and uses a Nix-based environment, so
 
 ### macOS
 
-Many `xrpld` engineers use macOS for development. The minimum supported version
-is macOS 26, which is also what our CI builds and tests against. The build
-defaults `CMAKE_OSX_DEPLOYMENT_TARGET` accordingly, so you do not need to pass it
-yourself.
+Many `xrpld` engineers use macOS for development.
 
 ### Windows
 

CMakeLists.txt

@@ -13,21 +13,6 @@ if(DEFINED CMAKE_MODULE_PATH)
 endif()
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
 
-# Default the macOS deployment target so it does not have to be passed on the
-# command line. Must be set before project() because project() consumes it when
-# configuring the compiler and SDK. A user-provided -DCMAKE_OSX_DEPLOYMENT_TARGET
-# still takes precedence.
-if(
-    CMAKE_HOST_SYSTEM_NAME STREQUAL "Darwin"
-    AND NOT DEFINED CMAKE_OSX_DEPLOYMENT_TARGET
-)
-    set(CMAKE_OSX_DEPLOYMENT_TARGET
-        "26.0"
-        CACHE STRING
-        "Minimum macOS deployment version"
-    )
-endif()
-
 project(xrpl)
 set(CMAKE_CXX_EXTENSIONS OFF)
 set(CMAKE_CXX_STANDARD 23)

cfg/xrpld-example.cfg

@@ -1059,11 +1059,10 @@
 #                           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, and that all
-#                           recent ledgers are available. If not, then continue
+#                           'age_threshold_seconds' old. If not, then continue
 #                           sleeping for this number of seconds and
 #                           checking until healthy.
-#                           Default is 2.
+#                           Default is 5.
 #
 #   Notes:
 #       The 'node_db' entry configures the primary, persistent storage.

include/xrpl/nodestore/DatabaseRotating.h

@@ -41,18 +41,6 @@ public:
         std::unique_ptr<NodeStore::Backend>&& newBackend,
         std::function<void(std::string const& writableName, std::string const& archiveName)> const&
             f) = 0;
-
-    /** Marks an online-delete rotation as in progress (or completed).
-
-        While in flight, a read served by the archive backend is copied
-        forward into the writable backend even for ordinary
-        (duplicate == false) fetches: the archive is about to be deleted,
-        and a node body canonicalized into caches during the rotation
-        window would otherwise survive only in RAM once the archive is
-        dropped.
-    */
-    virtual void
-    setRotationInFlight(bool inFlight) = 0;
 };
 
 }  // namespace xrpl::NodeStore

include/xrpl/nodestore/detail/DatabaseRotatingImp.h

@@ -9,7 +9,6 @@
 #include <xrpl/nodestore/NodeObject.h>
 #include <xrpl/nodestore/Scheduler.h>
 
-#include <atomic>
 #include <cstdint>
 #include <functional>
 #include <memory>
@@ -70,22 +69,11 @@ public:
     void
     sweep() override;
 
-    void
-    setRotationInFlight(bool inFlight) override;
-
 private:
     std::shared_ptr<Backend> writableBackend_;
     std::shared_ptr<Backend> archiveBackend_;
     mutable std::mutex mutex_;
 
-    // True between SHAMapStore starting the cache-freshen phase and the
-    // completion of rotate(). While true, archive hits on ordinary
-    // (duplicate == false) fetches are copied forward into the writable
-    // backend; copyForwardCount_ tallies them per rotation for the
-    // summary line logged at swap.
-    std::atomic<bool> rotationInFlight_{false};
-    std::atomic<std::uint64_t> copyForwardCount_{0};
-
     std::shared_ptr<NodeObject>
     fetchNodeObject(uint256 const& hash, std::uint32_t, FetchReport& fetchReport, bool duplicate)
         override;

include/xrpl/shamap/SHAMapMissingNode.h

@@ -35,15 +35,13 @@ to_string(SHAMapType t)
 class SHAMapMissingNode : public std::runtime_error
 {
 public:
-    SHAMapMissingNode(SHAMapType t, SHAMapHash const& hash, std::string const& location)
-        : std::runtime_error(
-              "Missing Node: " + to_string(t) + ": hash " + to_string(hash) + " in: " + location)
+    SHAMapMissingNode(SHAMapType t, SHAMapHash const& hash)
+        : std::runtime_error("Missing Node: " + to_string(t) + ": hash " + to_string(hash))
     {
     }
 
-    SHAMapMissingNode(SHAMapType t, uint256 const& id, std::string const& location)
-        : std::runtime_error(
-              "Missing Node: " + to_string(t) + ": id " + to_string(id) + " in: " + location)
+    SHAMapMissingNode(SHAMapType t, uint256 const& id)
+        : std::runtime_error("Missing Node: " + to_string(t) + ": id " + to_string(id))
     {
     }
 };

src/libxrpl/ledger/Ledger.cpp

@@ -746,8 +746,7 @@ Ledger::walkLedger(beast::Journal j, bool parallel) const
     if (stateMap_.getHash().isZero() && !header_.accountHash.isZero() &&
         !stateMap_.fetchRoot(SHAMapHash{header_.accountHash}, nullptr))
     {
-        missingNodes1.emplace_back(
-            SHAMapType::STATE, SHAMapHash{header_.accountHash}, "Ledger::walkLedger");
+        missingNodes1.emplace_back(SHAMapType::STATE, SHAMapHash{header_.accountHash});
     }
     else
     {
@@ -771,8 +770,7 @@ Ledger::walkLedger(beast::Journal j, bool parallel) const
     if (txMap_.getHash().isZero() && header_.txHash.isNonZero() &&
         !txMap_.fetchRoot(SHAMapHash{header_.txHash}, nullptr))
     {
-        missingNodes2.emplace_back(
-            SHAMapType::TRANSACTION, SHAMapHash{header_.txHash}, "Ledger::walkLedger");
+        missingNodes2.emplace_back(SHAMapType::TRANSACTION, SHAMapHash{header_.txHash});
     }
     else
     {

src/libxrpl/nodestore/DatabaseRotatingImp.cpp

@@ -52,7 +52,6 @@ DatabaseRotatingImp::rotate(
     // callback finishes. Only then will the archive directory be
     // deleted.
     std::shared_ptr<NodeStore::Backend> oldArchiveBackend;
-    std::uint64_t copyForwards = 0;
     {
         std::scoped_lock const lock(mutex_);
 
@@ -63,28 +62,11 @@ DatabaseRotatingImp::rotate(
         newArchiveBackendName = archiveBackend_->getName();
 
         writableBackend_ = std::move(newBackend);
-
-        copyForwards = copyForwardCount_.exchange(0, std::memory_order_acq_rel);
-    }
-
-    if (copyForwards > 0)
-    {
-        JLOG(j_.warn()) << "Rotating: copied forward " << copyForwards
-                        << " archive-served reads into the writable backend "
-                           "during the rotation window";
     }
 
     f(newWritableBackendName, newArchiveBackendName);
 }
 
-void
-DatabaseRotatingImp::setRotationInFlight(bool inFlight)
-{
-    rotationInFlight_.store(inFlight, std::memory_order_release);
-    JLOG(j_.debug()) << "Rotating: copy-forward on archive reads "
-                     << (inFlight ? "enabled" : "disabled");
-}
-
 std::string
 DatabaseRotatingImp::getName() const
 {
@@ -195,18 +177,9 @@ DatabaseRotatingImp::fetchNodeObject(
                 writable = writableBackend_;
             }
 
-            // Update writable backend with data from the archive backend.
-            // While a rotation is in flight, ordinary (duplicate == false)
-            // reads served by the archive are copied forward too: the
-            // archive is about to be deleted, and a body canonicalized
-            // into the cache after the freshen getKeys() snapshot would
-            // otherwise survive only in RAM once the archive is dropped.
-            if (duplicate || rotationInFlight_.load(std::memory_order_acquire))
-            {
-                if (!duplicate)
-                    ++copyForwardCount_;
+            // Update writable backend with data from the archive backend
+            if (duplicate)
                 writable->store(nodeObject);
-            }
         }
     }
 

src/libxrpl/shamap/SHAMap.cpp

@@ -272,7 +272,7 @@ SHAMap::fetchNode(SHAMapHash const& hash) const
     auto node = fetchNodeNT(hash);
 
     if (!node)
-        Throw<SHAMapMissingNode>(type_, hash, "fetchNode");
+        Throw<SHAMapMissingNode>(type_, hash);
 
     return node;
 }
@@ -283,7 +283,7 @@ SHAMap::descendThrow(SHAMapInnerNode* parent, int branch) const
     SHAMapTreeNode* ret = descend(parent, branch);  // NOLINT(misc-const-correctness)
 
     if ((ret == nullptr) && !parent->isEmptyBranch(branch))
-        Throw<SHAMapMissingNode>(type_, parent->getChildHash(branch), "descendThrow");
+        Throw<SHAMapMissingNode>(type_, parent->getChildHash(branch));
 
     return ret;
 }
@@ -294,7 +294,7 @@ SHAMap::descendThrow(SHAMapInnerNode& parent, int branch) const
     SHAMapTreeNodePtr ret = descend(parent, branch);
 
     if (!ret && !parent.isEmptyBranch(branch))
-        Throw<SHAMapMissingNode>(type_, parent.getChildHash(branch), "descendThrow");
+        Throw<SHAMapMissingNode>(type_, parent.getChildHash(branch));
 
     return ret;
 }
@@ -566,7 +566,7 @@ SHAMap::peekNextItem(uint256 const& id, SharedPtrNodeStack& stack) const
                 node = descendThrow(*inner, i);
                 auto leaf = firstBelow(node, stack, i);
                 if (leaf == nullptr)
-                    Throw<SHAMapMissingNode>(type_, id, "peekNextItem");
+                    Throw<SHAMapMissingNode>(type_, id);
                 XRPL_ASSERT(leaf->isLeaf(), "xrpl::SHAMap::peekNextItem : leaf is valid");
                 return leaf;
             }
@@ -624,7 +624,7 @@ SHAMap::upperBound(uint256 const& id) const
                     node = descendThrow(*inner, branch);
                     auto leaf = firstBelow(node, stack, branch);
                     if (leaf == nullptr)
-                        Throw<SHAMapMissingNode>(type_, id, "upperBound");
+                        Throw<SHAMapMissingNode>(type_, id);
                     return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
                 }
             }
@@ -657,7 +657,7 @@ SHAMap::lowerBound(uint256 const& id) const
                     node = descendThrow(*inner, branch);
                     auto leaf = lastBelow(node, stack, branch);
                     if (leaf == nullptr)
-                        Throw<SHAMapMissingNode>(type_, id, "lowerBound");
+                        Throw<SHAMapMissingNode>(type_, id);
                     return ConstIterator(this, leaf->peekItem().get(), std::move(stack));
                 }
             }
@@ -684,7 +684,7 @@ SHAMap::delItem(uint256 const& id)
     walkTowardsKey(id, &stack);
 
     if (stack.empty())
-        Throw<SHAMapMissingNode>(type_, id, "delItem");
+        Throw<SHAMapMissingNode>(type_, id);
 
     auto leaf = intr_ptr::dynamicPointerCast<SHAMapLeafNode>(stack.top().first);
     stack.pop();
@@ -770,7 +770,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
     walkTowardsKey(tag, &stack);
 
     if (stack.empty())
-        Throw<SHAMapMissingNode>(type_, tag, "addGiveItem");
+        Throw<SHAMapMissingNode>(type_, tag);
 
     auto [node, nodeID] = stack.top();
     stack.pop();
@@ -857,7 +857,7 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem cons
     walkTowardsKey(tag, &stack);
 
     if (stack.empty())
-        Throw<SHAMapMissingNode>(type_, tag, "updateGiveItem");
+        Throw<SHAMapMissingNode>(type_, tag);
 
     auto node = intr_ptr::dynamicPointerCast<SHAMapLeafNode>(stack.top().first);
     auto nodeID = stack.top().second;

src/libxrpl/shamap/SHAMapDelta.cpp

@@ -153,7 +153,7 @@ SHAMap::compare(SHAMap const& otherMap, Delta& differences, int maxCount) const
         {
             // LCOV_EXCL_START
             UNREACHABLE("xrpl::SHAMap::compare : missing a node");
-            Throw<SHAMapMissingNode>(type_, uint256(), "compare");
+            Throw<SHAMapMissingNode>(type_, uint256());
             // LCOV_EXCL_STOP
         }
 
@@ -270,7 +270,7 @@ SHAMap::walkMap(std::vector<SHAMapMissingNode>& missingNodes, int maxMissing) co
                 }
                 else
                 {
-                    missingNodes.emplace_back(type_, node->getChildHash(i), "walkMap");
+                    missingNodes.emplace_back(type_, node->getChildHash(i));
                     if (--maxMissing <= 0)
                         return;
                 }
@@ -344,8 +344,7 @@ SHAMap::walkMapParallel(std::vector<SHAMapMissingNode>& missingNodes, int maxMis
                             else
                             {
                                 std::scoped_lock const l{m};
-                                missingNodes.emplace_back(
-                                    type_, node->getChildHash(i), "walkMapParallel");
+                                missingNodes.emplace_back(type_, node->getChildHash(i));
                                 if (--maxMissing <= 0)
                                     return;
                             }

src/test/app/Invariants_test.cpp

@@ -3,6 +3,7 @@
 #include <test/jtx/Env.h>
 #include <test/jtx/TestHelpers.h>
 #include <test/jtx/amount.h>
+#include <test/jtx/envconfig.h>
 #include <test/jtx/fee.h>
 #include <test/jtx/mpt.h>
 #include <test/jtx/pay.h>
@@ -101,6 +102,12 @@ class Invariants_test : public beast::unit_test::Suite
         return xrpl::test::jtx::testableAmendments() | fixCleanup3_1_3 | fixCleanup3_2_0;
     }
 
+    test::jtx::Env
+    makeEnv(FeatureBitset features)
+    {
+        return {*this, test::jtx::envconfig(), features, nullptr, beast::Severity::Disabled};
+    }
+
     /**
      * Run a specific test case to put the ledger into a state that will be
      * detected by an invariant. Simulates the actions of a transaction that
@@ -130,7 +137,7 @@ class Invariants_test : public beast::unit_test::Suite
         TxAccount setTxAccount = TxAccount::None)
     {
         doInvariantCheck(
-            test::jtx::Env(*this, defaultAmendments()),
+            makeEnv(defaultAmendments()),
             expectLogs,
             precheck,
             fee,
@@ -1477,7 +1484,7 @@ class Invariants_test : public beast::unit_test::Suite
         testcase << "PermissionedDomain" + std::string(fixEnabled ? " fix" : "");
 
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain with no rules."}},
             [](Account const& a1, Account const& a2, ApplyContext& ac) {
                 return createPermissionedDomain(ac, a1, a2, 0).get();
@@ -1490,7 +1497,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         static constexpr auto kTooBig = kMaxPermissionedDomainCredentialsArraySize + 1;
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain bad credentials size " + std::to_string(kTooBig)}},
             [](Account const& a1, Account const& a2, ApplyContext& ac) {
                 return !!createPermissionedDomain(ac, a1, a2, kTooBig);
@@ -1501,7 +1508,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         testcase << "PermissionedDomain 3";
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain credentials aren't sorted"}},
             [](Account const& a1, Account const& a2, ApplyContext& ac) {
                 auto slePd = createPermissionedDomain(ac, a1, a2, 0);
@@ -1525,7 +1532,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         testcase << "PermissionedDomain 4";
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain credentials aren't unique"}},
             [](Account const& a1, Account const& a2, ApplyContext& ac) {
                 auto slePd = createPermissionedDomain(ac, a1, a2, 0);
@@ -1548,7 +1555,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         testcase << "PermissionedDomain Set 1";
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain with no rules."}},
             [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                 // create PD
@@ -1569,7 +1576,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         testcase << "PermissionedDomain Set 2";
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain bad credentials size " + std::to_string(kTooBig)}},
             [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                 // create PD
@@ -1600,7 +1607,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         testcase << "PermissionedDomain Set 3";
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain credentials aren't sorted"}},
             [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                 // create PD
@@ -1630,7 +1637,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         testcase << "PermissionedDomain Set 4";
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"permissioned domain credentials aren't unique"}},
             [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                 // create PD
@@ -1671,7 +1678,7 @@ class Invariants_test : public beast::unit_test::Suite
         {
             testcase << "PermissionedDomain set 2 domains ";
             doInvariantCheck(
-                Env(*this, features),
+                makeEnv(features),
                 fixEnabled ? badMoreThan1 : emptyV,
                 [](Account const& a1, Account const& a2, ApplyContext& ac) {
                     createPermissionedDomain(ac, a1, a2);
@@ -1717,7 +1724,7 @@ class Invariants_test : public beast::unit_test::Suite
         {
             testcase << "PermissionedDomain set 0 domains ";
             doInvariantCheck(
-                Env(*this, features),
+                makeEnv(features),
                 fixEnabled ? badNoDomains : emptyV,
                 [](Account const&, Account const&, ApplyContext&) { return true; },
                 XRPAmount{},
@@ -1740,7 +1747,7 @@ class Invariants_test : public beast::unit_test::Suite
             env1.close();
 
             doInvariantCheck(
-                Env(*this, features),
+                makeEnv(features),
                 a1,
                 a2,
                 fixEnabled ? badNoDomains : emptyV,
@@ -1781,7 +1788,7 @@ class Invariants_test : public beast::unit_test::Suite
         {
             testcase << "PermissionedDomain del, create domain ";
             doInvariantCheck(
-                Env(*this, features),
+                makeEnv(features),
                 fixEnabled ? badNotDeleted : emptyV,
                 [](Account const& a1, Account const& a2, ApplyContext& ac) {
                     createPermissionedDomain(ac, a1, a2);
@@ -1978,7 +1985,7 @@ class Invariants_test : public beast::unit_test::Suite
         testcase << "PermissionedDEX" + std::string(fixEnabled ? " fix" : "");
 
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"domain doesn't exist"}},
             [](Account const& a1, Account const&, ApplyContext& ac) {
                 Keylet const offerKey = keylet::offer(a1.id(), 10);
@@ -2005,7 +2012,7 @@ class Invariants_test : public beast::unit_test::Suite
 
         // missing domain ID in offer object
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             {{"hybrid offer is malformed"}},
             [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                 Keylet const offerKey = keylet::offer(a2.id(), 10);
@@ -4325,7 +4332,7 @@ class Invariants_test : public beast::unit_test::Suite
         };
 
         doInvariantCheck(
-            Env{*this, defaultAmendments() - fixCleanup3_2_0},
+            makeEnv(defaultAmendments() - fixCleanup3_2_0),
             {},
             [](Account const&, Account const&, ApplyContext&) { return true; },
             XRPAmount{},
@@ -4932,7 +4939,7 @@ class Invariants_test : public beast::unit_test::Suite
         // sfHighLimit issue, not the keylet currency).
         testcase << "overwrite: NoXRPTrustLines" + std::string(fixEnabled ? " fix" : "");
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             fixEnabled ? std::vector<std::string>{{"an XRP trust line was created"}}
                        : std::vector<std::string>{},
             [&insertOrderedTrustLinePair](Account const& a1, Account const& a2, ApplyContext& ac) {
@@ -4960,7 +4967,7 @@ class Invariants_test : public beast::unit_test::Suite
         // Regression: bad deep-freeze trust line followed by a valid one.
         testcase << "overwrite: NoDeepFreeze" + std::string(fixEnabled ? " fix" : "");
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             fixEnabled ? std::vector<std::string>{{"a trust line with deep freeze flag without "
                                                    "normal freeze was created"}}
                        : std::vector<std::string>{},
@@ -4994,7 +5001,7 @@ class Invariants_test : public beast::unit_test::Suite
         // still fires ("a MPT issuance was created").
         testcase << "overwrite: NoZeroEscrow MPT" + std::string(fixEnabled ? " fix" : "");
         doInvariantCheck(
-            Env(*this, features),
+            makeEnv(features),
             fixEnabled ? std::vector<std::string>{{"escrow specifies invalid amount"}}
                        : std::vector<std::string>{{"a MPT issuance was created"}},
             [](Account const& a1, Account const&, ApplyContext& ac) {

src/test/app/LedgerMaster_test.cpp

@@ -5,21 +5,17 @@
 #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 {
@@ -115,73 +111,6 @@ 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([](auto cfg) {
-                    return onlineDelete(std::move(cfg), 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(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);
-
-        // 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
@@ -195,7 +124,6 @@ public:
     testWithFeats(FeatureBitset features)
     {
         testTxnIdFromIndex(features);
-        testCompleteLedgerRange(features);
     }
 };
 

src/test/app/SHAMapStore_test.cpp

@@ -1,9 +1,7 @@
 #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>
@@ -11,7 +9,6 @@
 #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>
@@ -29,15 +26,12 @@
 #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 {
@@ -49,7 +43,10 @@ class SHAMapStore_test : public beast::unit_test::Suite
     static auto
     onlineDelete(std::unique_ptr<Config> cfg)
     {
-        return jtx::onlineDelete(std::move(cfg), kDeleteInterval);
+        cfg->ledgerHistory = kDeleteInterval;
+        auto& section = cfg->section(Sections::kNodeDatabase);
+        section.set(Keys::kOnlineDelete, std::to_string(kDeleteInterval));
+        return cfg;
     }
 
     static auto
@@ -147,11 +144,11 @@ class SHAMapStore_test : public beast::unit_test::Suite
         auto& store = env.app().getSHAMapStore();
 
         int ledgerSeq = 3;
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
         BEAST_EXPECT(!store.getLastRotated());
 
         env.close();
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         auto ledger = env.rpc("ledger", "validated");
         BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++)));
@@ -231,7 +228,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(kDeleteInterval + 4)));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         BEAST_EXPECT(store.getLastRotated() == kDeleteInterval + 3);
         lastRotated = store.getLastRotated();
@@ -258,7 +255,7 @@ public:
                 !getHash(ledgers[i]).empty());
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         BEAST_EXPECT(store.getLastRotated() == kDeleteInterval + lastRotated);
 
@@ -296,7 +293,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         // The database will always have back to ledger 2,
         // regardless of lastRotated.
@@ -311,7 +308,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
         BEAST_EXPECT(lastRotated != store.getLastRotated());
@@ -327,7 +324,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, kDeleteInterval + 1, lastRotated);
         BEAST_EXPECT(lastRotated != store.getLastRotated());
@@ -366,7 +363,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - 2, 2);
         BEAST_EXPECT(lastRotated == store.getLastRotated());
@@ -376,7 +373,7 @@ public:
         BEAST_EXPECT(!RPC::containsError(canDelete[jss::result]));
         BEAST_EXPECT(canDelete[jss::result][jss::can_delete] == ledgerSeq + (kDeleteInterval / 2));
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - 2, 2);
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
@@ -389,7 +386,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
 
@@ -405,7 +402,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
 
@@ -417,7 +414,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - firstBatch, firstBatch);
 
@@ -439,7 +436,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
 
@@ -451,7 +448,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
 
@@ -472,7 +469,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         BEAST_EXPECT(store.getLastRotated() == lastRotated);
 
@@ -484,7 +481,7 @@ public:
             BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true));
         }
 
-        BEAST_EXPECT(store.rendezvous());
+        store.rendezvous();
 
         ledgerCheck(env, ledgerSeq - lastRotated, lastRotated);
 
@@ -607,173 +604,6 @@ 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(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);
-
-        // 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 4
-                    // loops (400ms). If this test ever actually fails feel free to lower this
-                    // cutoff.
-                    BEAST_EXPECTS(iterations > 25, to_string(iterations));
-                }
-                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
     {
@@ -781,7 +611,6 @@ public:
         testAutomatic();
         testCanDelete();
         testRotate();
-        testLedgerGaps();
     }
 };
 

src/test/jtx/envconfig.h

@@ -3,7 +3,6 @@
 #include <xrpld/core/Config.h>
 
 #include <atomic>
-#include <cstdint>
 #include <map>
 #include <memory>
 #include <string>
@@ -63,19 +62,6 @@ 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,10 +7,8 @@
 #include <xrpl/config/Constants.h>
 
 #include <atomic>
-#include <cstdint>
 #include <map>
 #include <memory>
-#include <string>
 #include <vector>
 
 namespace xrpl::test {
@@ -62,15 +60,6 @@ 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/xrpld/app/ledger/LedgerMaster.h

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

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

@@ -57,7 +57,6 @@
 #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>
@@ -1577,25 +1576,12 @@ LedgerMaster::getPublishedLedger()
 }
 
 std::string
-LedgerMaster::getCompleteLedgers() const
+LedgerMaster::getCompleteLedgers()
 {
     std::scoped_lock const sl(completeLock_);
     return to_string(completeLedgers_);
 }
 
-std::size_t
-LedgerMaster::missingFromCompleteLedgerRange(LedgerIndex first, LedgerIndex last) const
-{
-    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,7 +8,6 @@
 #include <xrpl/nodestore/Scheduler.h>
 #include <xrpl/protocol/Protocol.h>
 
-#include <chrono>
 #include <cstdint>
 #include <memory>
 #include <optional>
@@ -35,8 +34,8 @@ public:
     virtual void
     start() = 0;
 
-    [[nodiscard]] virtual bool
-    rendezvous(std::optional<std::chrono::milliseconds> const& timeout = {}) const = 0;
+    virtual void
+    rendezvous() const = 0;
 
     virtual void
     stop() = 0;

src/xrpld/app/misc/SHAMapStoreImp.cpp

@@ -8,7 +8,6 @@
 #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>
@@ -17,11 +16,9 @@
 #include <xrpl/ledger/Ledger.h>
 #include <xrpl/nodestore/Database.h>
 #include <xrpl/nodestore/Manager.h>
-#include <xrpl/nodestore/NodeObject.h>
 #include <xrpl/nodestore/Scheduler.h>
 #include <xrpl/nodestore/detail/DatabaseRotatingImp.h>
 #include <xrpl/protocol/Protocol.h>
-#include <xrpl/protocol/Serializer.h>
 #include <xrpl/server/NetworkOPs.h>
 #include <xrpl/server/State.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
@@ -33,8 +30,6 @@
 #include <boost/filesystem/path.hpp>
 
 #include <algorithm>
-#include <chrono>
-#include <cstddef>
 #include <cstdint>
 #include <functional>
 #include <limits>
@@ -239,21 +234,14 @@ SHAMapStoreImp::onLedgerClosed(std::shared_ptr<Ledger const> const& ledger)
     cond_.notify_one();
 }
 
-bool
-SHAMapStoreImp::rendezvous(std::optional<std::chrono::milliseconds> const& timeout) const
+void
+SHAMapStoreImp::rendezvous() const
 {
     if (!working_)
-        return true;
-
-    auto notWorking = [&] { return !working_; };
+        return;
 
     std::unique_lock<std::mutex> lock(mutex_);
-    if (timeout)
-    {
-        return rendezvous_.wait_for(lock, *timeout, notWorking);
-    }
-    rendezvous_.wait(lock, notWorking);
-    return true;
+    rendezvous_.wait(lock, [&] { return !working_; });
 }
 
 int
@@ -266,28 +254,8 @@ bool
 SHAMapStoreImp::copyNode(std::uint64_t& nodeCount, SHAMapTreeNode const& node)
 {
     // Copy a single record from node to dbRotating_
-    auto obj = dbRotating_->fetchNodeObject(
+    dbRotating_->fetchNodeObject(
         node.getHash().asUInt256(), 0, NodeStore::FetchType::Synchronous, true);
-    if (!obj)
-    {
-        // Reachable from the validated state map in memory, but present in
-        // neither backend: its only on-disk copy lived in a backend removed by
-        // an earlier rotation, and it was never rewritten because it is clean
-        // (cowid == 0, so flushDirty skips it). Persist the in-memory body
-        // directly into the writable backend so it survives this rotation
-        // instead of later surfacing as an unresolvable SHAMapMissingNode.
-        auto const hash = node.getHash().asUInt256();
-        Serializer s;
-        node.serializeWithPrefix(s);
-        dbRotating_->store(NodeObjectType::AccountNode, std::move(s.modData()), hash, 0);
-        {
-            auto const& cached = treeNodeCache_->fetch(hash);
-            JLOG(journal_.warn()) << "copyNode: re-stored node missing from both backends, hash="
-                                  << hash << " type=" << static_cast<int>(node.getType())
-                                  << ". Node is " << (cached ? "" : "not ")
-                                  << "in the tree node cache";
-        }
-    }
     if ((++nodeCount % checkHealthInterval_) == 0u)
     {
         if (healthWait() == HealthResult::Stopping)
@@ -344,26 +312,14 @@ 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)
         {
-            auto const diff = validatedSeq - lastRotated;
             JLOG(journal_.warn()) << "rotating  validatedSeq " << validatedSeq << " lastRotated "
-                                  << lastRotated << " diff " << diff << " deleteInterval "
-                                  << deleteInterval_ << " canDelete_ " << canDelete_ << " state "
+                                  << lastRotated << " deleteInterval " << deleteInterval_
+                                  << " canDelete_ " << canDelete_ << " state "
                                   << app_.getOPs().strOperatingMode(false) << " age "
-                                  << ledgerMaster_->getValidatedLedgerAge().count()
-                                  << "s. Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
+                                  << ledgerMaster_->getValidatedLedgerAge().count() << 's';
 
             clearPrior(lastRotated);
             if (healthWait() == HealthResult::Stopping)
@@ -392,23 +348,6 @@ SHAMapStoreImp::run()
             JLOG(journal_.debug())
                 << "copied ledger " << validatedSeq << " nodecount " << nodeCount;
 
-            // Close the getKeys()->swap exposure window: from here until
-            // rotate() completes, an ordinary read served by the archive is
-            // copied forward into the writable backend, so a node fetched
-            // from the doomed archive cannot be left RAM-only when the
-            // archive is deleted. RAII so the early returns below (and any
-            // exception) also clear the flag.
-            struct RotationExposureGuard
-            {
-                NodeStore::DatabaseRotating& db;
-                ~RotationExposureGuard()
-                {
-                    db.setRotationInFlight(false);
-                }
-            };
-            RotationExposureGuard const rotationExposureGuard{*dbRotating_};
-            dbRotating_->setRotationInFlight(true);
-
             JLOG(journal_.debug()) << "freshening caches";
             freshenCaches();
             if (healthWait() == HealthResult::Stopping)
@@ -438,14 +377,7 @@ SHAMapStoreImp::run()
                     clearCaches(validatedSeq);
                 });
 
-            auto const currentValidatedSeq = ledgerMaster_->getValidLedgerIndex();
-            auto const processingDiff = currentValidatedSeq - validatedSeq;
-            JLOG(journal_.warn())
-                << "finished rotation. validatedSeq: " << validatedSeq
-                << ", lastRotated: " << lastRotated << " diff " << diff
-                << ". Updated validated seq is " << currentValidatedSeq << ", " << processingDiff
-                << " ledgers were validated during the rotation processs. Complete ledgers: "
-                << ledgerMaster_->getCompleteLedgers();
+            JLOG(journal_.warn()) << "finished rotation " << validatedSeq;
         }
     }
 }
@@ -689,86 +621,20 @@ SHAMapStoreImp::clearPrior(LedgerIndex lastRotated)
 SHAMapStoreImp::HealthResult
 SHAMapStoreImp::healthWait()
 {
-    // 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();
+    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_);
         age = ledgerMaster_->getValidatedLedgerAge();
         mode = netOPs_->getOperatingMode();
-
-        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);
-    }
-
-    auto healthy = [&]() {
-        // Special case: If the server is disconnected, it's not doing any ledger I/O, because
-        // it's focused on trying to get peers. A disconnected state is should never be caused by
-        // the activity of the server. It's usually limited to hardware or connectivity issues. Take
-        // advantage of that to run as much rotation I/O as possible before it comes back online.
-        if (mode == OperatingMode::DISCONNECTED)
-            return true;
-        if (age > ageThreshold)
-            return false;
-        if (numMissing > 0)
-            return false;
-        if (mode != OperatingMode::FULL)
-            return false;
-        return true;
-    };
-
-    while (!stop_ && !healthy())
-    {
-        // 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;
+        lock.lock();
     }
 
     return stop_ ? HealthResult::Stopping : HealthResult::KeepGoing;

src/xrpld/app/misc/SHAMapStoreImp.h

@@ -88,11 +88,6 @@ 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_;
@@ -107,12 +102,12 @@ private:
     std::chrono::milliseconds backOff_{100};
     std::chrono::seconds ageThreshold_{60};
     /**
-     * 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.
+     * If  the node is out of sync 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_{2};
+    std::chrono::seconds recoveryWaitTime_{5};
 
     // these do not exist upon SHAMapStore creation, but do exist
     // as of run() or before
@@ -168,8 +163,8 @@ public:
     void
     onLedgerClosed(std::shared_ptr<Ledger const> const& ledger) override;
 
-    bool
-    rendezvous(std::optional<std::chrono::milliseconds> const& timeout = {}) const override;
+    void
+    rendezvous() const override;
     int
     fdRequired() const override;