Merge branch 'develop' into ximinez/online-delete-gaps2

Co-authored-by: Copilot Autofix powered by AI <[email protected]>

cfg/xrpld-example.cfg

@@ -1123,7 +1123,8 @@
 #                           online deletion gives up on the current ledger and tries
 #                           again later. Note this only affects situations that cause
 #                           rotation to wait, such as going out of sync, or missing
-#                           ledgers. Forward progress is not penalized. Minimum is 64.
+#                           ledgers. Forward progress is not penalized. Minimum in
+#                           normal operation is 64. Minimum in standalone mode is 2.
 #                           Default is the online_delete value.
 #
 #   Notes:

include/xrpl/nodestore/DatabaseRotating.h

@@ -5,6 +5,7 @@
 #include <xrpl/nodestore/Database.h>
 #include <xrpl/nodestore/Scheduler.h>
 
+#include <cstdint>
 #include <functional>
 #include <memory>
 #include <string>
@@ -54,6 +55,12 @@ public:
      */
     virtual void
     setRotationInFlight(bool inFlight) = 0;
+    virtual bool
+    isRotationInFlight() const = 0;
+
+    [[nodiscard]]
+    virtual std::uint64_t
+    getAndResetDuplicationCount() = 0;
 };
 
 }  // namespace xrpl::node_store

include/xrpl/nodestore/detail/DatabaseRotatingImp.h

@@ -72,19 +72,28 @@ public:
 
     void
     setRotationInFlight(bool inFlight) override;
+    bool
+    isRotationInFlight() const override;
+
+    [[nodiscard]]
+    std::uint64_t
+    getAndResetDuplicationCount() 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
+    // Set to true during the entire SHAMapStore rotation process.
+    // 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};
+    // Duplication count tracks the number of nodes that are directly duplicated because they're in
+    // the target ledger or rescued from a cache.
+    std::atomic<std::uint64_t> duplicationCount_{0};
 
     std::shared_ptr<NodeObject>
     fetchNodeObject(UInt256 const& hash, std::uint32_t, FetchReport& fetchReport, bool duplicate)

src/libxrpl/nodestore/DatabaseRotatingImp.cpp

@@ -54,6 +54,7 @@ DatabaseRotatingImp::rotate(
     // deleted.
     std::shared_ptr<node_store::Backend> oldArchiveBackend;
     std::uint64_t copyForwards = 0;
+    std::uint64_t duplications = 0;
     {
         std::scoped_lock const lock(mutex_);
 
@@ -66,6 +67,7 @@ DatabaseRotatingImp::rotate(
         writableBackend_ = std::move(newBackend);
 
         copyForwards = copyForwardCount_.exchange(0, std::memory_order_relaxed);
+        duplications = duplicationCount_.exchange(0, std::memory_order_relaxed);
     }
 
     if (copyForwards > 0)
@@ -74,6 +76,11 @@ DatabaseRotatingImp::rotate(
                         << " archive-served reads into the writable backend "
                            "during the rotation window";
     }
+    if (duplications > 0)
+    {
+        JLOG(j_.warn()) << "Rotating: duplicated " << duplications
+                        << " nodes into the writable backend for the relevant cache.";
+    }
 
     f(newWritableBackendName, newArchiveBackendName);
 }
@@ -86,6 +93,22 @@ DatabaseRotatingImp::setRotationInFlight(bool inFlight)
                      << (inFlight ? "enabled" : "disabled");
 }
 
+bool
+DatabaseRotatingImp::isRotationInFlight() const
+{
+    return rotationInFlight_.load(std::memory_order_acquire);
+}
+
+[[nodiscard]]
+std::uint64_t
+DatabaseRotatingImp::getAndResetDuplicationCount()
+{
+    std::uint64_t duplications = 0;
+    duplications = duplicationCount_.exchange(0, std::memory_order_relaxed);
+
+    return duplications;
+}
+
 std::string
 DatabaseRotatingImp::getName() const
 {
@@ -186,26 +209,35 @@ DatabaseRotatingImp::fetchNodeObject(
     nodeObject = fetch(writable);
     if (!nodeObject)
     {
+        // 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.
+        auto const inFlight = isRotationInFlight();
+
         // Otherwise try to fetch from the archive backend
         nodeObject = fetch(archive);
         if (nodeObject)
         {
-            {
-                // Refresh the writable backend pointer
-                std::scoped_lock const lock(mutex_);
-                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))
+            // Check isRotationInFlight again, because it may have been set since the first check.
+            if (duplicate || inFlight || isRotationInFlight())
             {
-                if (!duplicate)
+                {
+                    // Refresh the writable backend pointer since we need to use it
+                    std::scoped_lock const lock(mutex_);
+                    writable = writableBackend_;
+                }
+
+                if (duplicate)
+                {
+                    duplicationCount_.fetch_add(1, std::memory_order_relaxed);
+                }
+                else
+                {
                     copyForwardCount_.fetch_add(1, std::memory_order_relaxed);
+                }
                 writable->store(nodeObject);
             }
         }

src/test/app/SHAMapStore_test.cpp

@@ -83,8 +83,8 @@ class SHAMapStore_test : public beast::unit_test::Suite
     // after the health check that gates it, and kFinished once a rotation has
     // run to completion. kExpired is logged by healthWait() when the circuit
     // breaker trips.
-    static constexpr char const* kRotating = "rotating";
-    static constexpr char const* kFinished = "finished rotation";
+    static constexpr char const* kRotating = "ROTATING:";
+    static constexpr char const* kFinished = "FINISHED ROTATION:";
     static constexpr char const* kExpired = "unable to make progress";
 
     // A Logs implementation that records every message the store's own

src/xrpld/app/misc/SHAMapStoreImp.cpp

@@ -280,6 +280,68 @@ SHAMapStoreImp::fdRequired() const
     return fdRequired_;
 }
 
+void
+SHAMapStoreImp::rescueNode(SHAMapTreeNode const& node, std::optional<NodeObjectType> expectedType)
+{
+    XRPL_ASSERT(node.cowid() == 0, "SHAMapStoreImp::rescueNode : rescued node must be clean");
+    // 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.
+
+    // Note that this function is only intended to be called for AccountState nodes. Transaction
+    // nodes and Inner (unknown) nodes are not expected to be present in the SHAMap or TreeNodeCache
+    // which are used to find missing nodes, and do not need to be rescued. The check below will
+    // prevent writing them if they are encountered, but it is not expected to happen in practice.
+
+    auto const nodeType = node.getType();
+    auto const objectType = std::invoke([nodeType, expectedType] {
+        switch (nodeType)
+        {
+            case SHAMapNodeType::TnAccountState:
+                return NodeObjectType::AccountNode;
+            // We don't expect to see transaction nodes. The check below will prevent writing them.
+            case SHAMapNodeType::TnTransactionNm:
+            case SHAMapNodeType::TnTransactionMd:
+                return NodeObjectType::TransactionNode;
+            case SHAMapNodeType::TnInner:
+                return expectedType.value_or(NodeObjectType::Unknown);
+            default:
+                return NodeObjectType::Unknown;
+        }
+    });
+
+    auto const hash = node.getHash().asUInt256();
+    XRPL_ASSERT_IF(
+        expectedType,
+        *expectedType == objectType,
+        "SHAMapStoreImp::rescueNode : expected node type");
+
+    if (objectType != NodeObjectType::AccountNode || (expectedType && *expectedType != objectType))
+    {
+        // LCOV_EXCL_START
+        JLOG(journal_.warn())
+            << "rescueNode: unable to re-store node with unsupported/unknown type, hash=" << hash
+            << " type=" << static_cast<int>(nodeType);
+        // We do not expect to see Inner nodes rescued without an expected type. Analysis and
+        // experimentation so far indicate that it just doesn't happen, specifically in
+        // freshenCaches. This UNREACHABLE is as much a developer alert as it is a safety check. If
+        // it does happen, we want to know about it. It won't affect production deployments.
+        UNREACHABLE("SHAMapStoreImp::rescueNode : unsupported node type");
+        return;
+        // LCOV_EXCL_STOP
+    }
+
+    Serializer s;
+    node.serializeWithPrefix(s);
+    dbRotating_->store(objectType, std::move(s.modData()), hash, 0);
+
+    JLOG(journal_.info()) << "rescueNode: re-stored node missing from both backends, hash=" << hash
+                          << " type=" << static_cast<int>(nodeType);
+}
+
 bool
 SHAMapStoreImp::copyNode(std::uint64_t& nodeCount, SHAMapTreeNode const& node)
 {
@@ -288,19 +350,7 @@ SHAMapStoreImp::copyNode(std::uint64_t& nodeCount, SHAMapTreeNode const& node)
         node.getHash().asUInt256(), 0, node_store::FetchType::Synchronous, true);
     if (!obj)
     {
-        XRPL_ASSERT(node.cowid() == 0, "SHAMapStoreImp::copyNode : rescued node must be clean");
-        // 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);
-        JLOG(journal_.warn()) << "copyNode: re-stored node missing from both backends, hash="
-                              << hash << " type=" << static_cast<int>(node.getType());
+        rescueNode(node, NodeObjectType::AccountNode);
     }
     if ((++nodeCount % checkHealthInterval_) == 0u)
     {
@@ -326,6 +376,11 @@ SHAMapStoreImp::run()
 
     while (true)
     {
+        XRPL_ASSERT(
+            !dbRotating_->isRotationInFlight(),
+            "SHAMapStoreImp::run : rotationInFlight_ must be false "
+            "outside rotation window");
+
         healthy_ = true;
         std::shared_ptr<Ledger const> validatedLedger;
 
@@ -387,13 +442,24 @@ SHAMapStoreImp::run()
         // will delete up to (not including) lastRotated
         if (readyToRotate)
         {
-            JLOG(journal_.warn()) << "rotating  validatedSeq " << validatedSeq << " lastRotated "
-                                  << lastRotated << " deleteInterval " << deleteInterval_
-                                  << " canDelete_ " << canDelete_ << " state "
+            auto const diff = validatedSeq - lastRotated;
+            JLOG(journal_.warn()) << "ROTATING: validatedSeq " << validatedSeq << " lastRotated "
+                                  << lastRotated << " diff " << diff << " deleteInterval "
+                                  << deleteInterval_ << " canDelete_ " << canDelete_ << " state "
                                   << app_.getOPs().strOperatingMode(false) << " age "
                                   << ledgerMaster_->getValidatedLedgerAge().count()
                                   << "s. Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
 
+            // Close the getKeys()->swap exposure window: from here until
+            // rotate() completes, an ordinary read for new ledgers 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. Use ScopeExit so the early returns and continues below (and any
+            // exceptions) also clear the flag.
+            ScopeExit const clearRotationInFlight{
+                [this] { dbRotating_->setRotationInFlight(false); }};
+            dbRotating_->setRotationInFlight(true);
+
             clearPrior(lastRotated);
             switch (healthWait())
             {
@@ -431,26 +497,12 @@ SHAMapStoreImp::run()
                 case HealthResult::KeepGoing:
                     break;
             }
-            // Only log if we completed without a "health" abort
-            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
             {
-                node_store::DatabaseRotating& db;
-                ~RotationExposureGuard()
-                {
-                    db.setRotationInFlight(false);
-                }
-            };
-            RotationExposureGuard const rotationExposureGuard{*dbRotating_};
-            dbRotating_->setRotationInFlight(true);
+                // Only log if we completed without a "health" abort
+                auto const copyDuplications = dbRotating_->getAndResetDuplicationCount();
+                JLOG(journal_.debug()) << "copied ledger " << validatedSeq << " duplicated "
+                                       << copyDuplications << " / " << nodeCount << " nodes";
+            }
 
             JLOG(journal_.debug()) << "freshening caches";
             freshenCaches();
@@ -495,9 +547,14 @@ SHAMapStoreImp::run()
                     clearCaches(validatedSeq);
                 });
 
-            JLOG(journal_.warn()) << "finished rotation. validatedSeq: " << validatedSeq
-                                  << ", lastRotated: " << lastRotated
-                                  << ". Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
+            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 process. Complete ledgers: "
+                << ledgerMaster_->getCompleteLedgers();
         }
     }
 }
@@ -692,8 +749,7 @@ SHAMapStoreImp::freshenCaches()
 {
     if (freshenCache(*treeNodeCache_))
         return;
-    if (freshenCache(app_.getMasterTransaction().getCache()))
-        return;
+    freshenCache(app_.getMasterTransaction().getCache());
 }
 
 void
@@ -767,7 +823,6 @@ SHAMapStoreImp::healthWait()
 
         buildingIndex = (numMissing == 1 && !haveIndex);
     };
-
     // Tracked server status properties
     LedgerIndex index = 0;
     bool buildingIndex = false;

src/xrpld/app/misc/SHAMapStoreImp.h

@@ -10,6 +10,7 @@
 #include <xrpl/nodestore/Backend.h>
 #include <xrpl/nodestore/Database.h>
 #include <xrpl/nodestore/DatabaseRotating.h>
+#include <xrpl/nodestore/NodeObject.h>
 #include <xrpl/nodestore/Scheduler.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/rdb/DatabaseCon.h>
@@ -21,6 +22,7 @@
 #include <algorithm>
 #include <atomic>
 #include <chrono>
+#include <concepts>
 #include <condition_variable>
 #include <cstdint>
 #include <functional>
@@ -120,7 +122,7 @@ private:
      * again later. This value represents the number of ledgers that must be validated without
      * making rotation progress before the process is aborted.
      */
-    std::uint32_t maxWaitingLedgers_ = deleteBatch_;
+    std::uint32_t maxWaitingLedgers_;
 
     // these do not exist upon SHAMapStore creation, but do exist
     // as of run() or before
@@ -186,6 +188,11 @@ public:
     minimumOnline() const override;
 
 private:
+    // Force write a node to the writable backend during rotation so it doesn't get lost
+    void
+    rescueNode(
+        SHAMapTreeNode const& node,
+        std::optional<NodeObjectType> expectedType = std::nullopt);
     // callback for visitNodes
     bool
     copyNode(std::uint64_t& nodeCount, SHAMapTreeNode const& node);
@@ -205,7 +212,18 @@ private:
 
         for (auto const& key : cache.getKeys())
         {
-            dbRotating_->fetchNodeObject(key, 0, node_store::FetchType::Synchronous, true);
+            [[maybe_unused]]
+            auto const obj =
+                dbRotating_->fetchNodeObject(key, 0, node_store::FetchType::Synchronous, true);
+            if constexpr (std::derived_from<typename CacheInstance::mapped_type, SHAMapTreeNode>)
+            {
+                if (!obj)
+                {
+                    auto const node = cache.fetch(key);
+                    if (node)
+                        rescueNode(*node);
+                }
+            }
             if (!(++check % checkHealthInterval_) && healthWait() != HealthResult::KeepGoing)
                 return true;
         }

src/xrpld/app/rdb/backend/detail/Node.cpp

@@ -274,9 +274,9 @@ saveValidatedLedger(
             app.getAcceptedLedgerCache().canonicalizeReplaceClient(ledger->header().hash, aLedger);
         }
     }
-    catch (std::exception const&)
+    catch (std::exception const& e)
     {
-        JLOG(j.warn()) << "An accepted ledger was missing nodes";
+        JLOG(j.warn()) << "An accepted ledger was missing nodes " << e.what();
         app.getLedgerMaster().failedSave(seq, ledger->header().hash);
         // Clients can now trust the database for information about this
         // ledger sequence.