From 17412d17e205c860af1d28bb9fc7ee0e101c5145 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Mon, 18 May 2020 17:29:06 -0700 Subject: [PATCH] Fix historical ledger acquisition when using online deletion: Commit e257a22 introduced changes in the logic used to acquire historical ledgers. The logic could cause historical ledgers to be acquired only since the last online deletion interval instead of the configured value to allow deletion. --- src/ripple/app/ledger/LedgerMaster.h | 4 ++ src/ripple/app/ledger/impl/LedgerMaster.cpp | 41 ++++++++------------- src/ripple/app/misc/SHAMapStore.h | 20 ++++++++++ src/ripple/app/misc/SHAMapStoreImp.cpp | 37 +++++++++++++------ src/ripple/app/misc/SHAMapStoreImp.h | 15 +++----- 5 files changed, 71 insertions(+), 46 deletions(-) diff --git a/src/ripple/app/ledger/LedgerMaster.h b/src/ripple/app/ledger/LedgerMaster.h index fadac2f9e..b82fce0bd 100644 --- a/src/ripple/app/ledger/LedgerMaster.h +++ b/src/ripple/app/ledger/LedgerMaster.h @@ -286,6 +286,10 @@ public: return !mValidLedger.empty(); } + // Returns the minimum ledger sequence in SQL database, if any. + boost::optional + minSqlSeq(); + private: void setValidLedger(std::shared_ptr const& l); diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index 3cee63b10..9a8f7dfe3 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -143,29 +143,17 @@ static constexpr std::chrono::minutes MAX_LEDGER_AGE_ACQUIRE{1}; // Don't acquire history if write load is too high static constexpr int MAX_WRITE_LOAD_ACQUIRE{8192}; -// Helper function for LedgerMaster::doAdvance() -// Returns the minimum ledger sequence in SQL database, if any. -static boost::optional -minSqlSeq(Application& app) -{ - boost::optional seq; - auto db = app.getLedgerDB().checkoutDb(); - *db << "SELECT MIN(LedgerSeq) FROM Ledgers", soci::into(seq); - return seq; -} - // Helper function for LedgerMaster::doAdvance() // Return true if candidateLedger should be fetched from the network. static bool shouldAcquire( std::uint32_t const currentLedger, std::uint32_t const ledgerHistory, - boost::optional minSeq, - std::uint32_t const lastRotated, + boost::optional const minimumOnline, std::uint32_t const candidateLedger, beast::Journal j) { - bool ret = [&]() { + bool const ret = [&]() { // Fetch ledger if it may be the current ledger if (candidateLedger >= currentLedger) return true; @@ -174,15 +162,9 @@ shouldAcquire( if (currentLedger - candidateLedger <= ledgerHistory) return true; - // Or it's greater than or equal to both: - // - the minimum persisted ledger or the maximum possible - // sequence value, if no persisted ledger, and - // - minimum ledger that will be persisted as of the next online - // deletion interval, or 1 if online deletion is disabled. - return candidateLedger >= - std::max( - minSeq.value_or(std::numeric_limits::max()), - lastRotated + 1); + // Or if greater than or equal to a specific minimum ledger. + // Do nothing if the minimum ledger to keep online is unknown. + return minimumOnline.has_value() && candidateLedger >= *minimumOnline; }(); JLOG(j.trace()) << "Missing ledger " << candidateLedger @@ -1899,8 +1881,7 @@ LedgerMaster::doAdvance(std::unique_lock& sl) shouldAcquire( mValidLedgerSeq, ledger_history_, - minSqlSeq(app_), - app_.getSHAMapStore().getLastRotated(), + app_.getSHAMapStore().minimumOnline(), *missing, m_journal)) { @@ -2149,4 +2130,14 @@ LedgerMaster::getFetchPackCacheSize() const return fetch_packs_.getCacheSize(); } +// Returns the minimum ledger sequence in SQL database, if any. +boost::optional +LedgerMaster::minSqlSeq() +{ + boost::optional seq; + auto db = app_.getLedgerDB().checkoutDb(); + *db << "SELECT MIN(LedgerSeq) FROM Ledgers", soci::into(seq); + return seq; +} + } // namespace ripple diff --git a/src/ripple/app/misc/SHAMapStore.h b/src/ripple/app/misc/SHAMapStore.h index d5f63ab42..8952453cb 100644 --- a/src/ripple/app/misc/SHAMapStore.h +++ b/src/ripple/app/misc/SHAMapStore.h @@ -24,6 +24,7 @@ #include #include #include +#include namespace ripple { @@ -74,6 +75,25 @@ public: /** Returns the number of file descriptors that are needed. */ virtual int fdRequired() const = 0; + + /** The minimum ledger to try and maintain in our database. + + This defines the lower bound for attempting to acquire historical + ledgers over the peer to peer network. + + If online_delete is enabled, then each time online_delete executes + and just prior to clearing SQL databases of historical ledgers, + move the value forward to one past the greatest ledger being deleted. + This minimizes fetching of ledgers that are in the process of being + deleted. Without online_delete or before online_delete is + executed, this value is always the minimum value persisted in the + ledger database, if any. + + @return The minimum ledger sequence to keep online based on the + description above. If not set, then an unseated optional. + */ + virtual boost::optional + minimumOnline() const = 0; }; //------------------------------------------------------------------------------ diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 3c9ccb942..94deae5d2 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -305,7 +305,7 @@ void SHAMapStoreImp::run() { beast::setCurrentThreadName("SHAMapStore"); - lastRotated_ = state_db_.getState().lastRotated; + LedgerIndex lastRotated = state_db_.getState().lastRotated; netOPs_ = &app_.getOPs(); ledgerMaster_ = &app_.getLedgerMaster(); fullBelowCache_ = &app_.family().fullbelow(); @@ -340,19 +340,19 @@ SHAMapStoreImp::run() } LedgerIndex const validatedSeq = validatedLedger->info().seq; - if (!lastRotated_) + if (!lastRotated) { - lastRotated_ = validatedSeq; - state_db_.setLastRotated(lastRotated_); + lastRotated = validatedSeq; + state_db_.setLastRotated(lastRotated); } - // will delete up to (not including) lastRotated_ - if (validatedSeq >= lastRotated_ + deleteInterval_ && - canDelete_ >= lastRotated_ - 1) + // will delete up to (not including) lastRotated + if (validatedSeq >= lastRotated + deleteInterval_ && + canDelete_ >= lastRotated - 1) { JLOG(journal_.warn()) - << "rotating validatedSeq " << validatedSeq << " lastRotated_ " - << lastRotated_ << " deleteInterval " << deleteInterval_ + << "rotating validatedSeq " << validatedSeq << " lastRotated " + << lastRotated << " deleteInterval " << deleteInterval_ << " canDelete_ " << canDelete_; switch (health()) @@ -366,7 +366,7 @@ SHAMapStoreImp::run() default:; } - clearPrior(lastRotated_); + clearPrior(lastRotated); switch (health()) { case Health::stopping: @@ -426,14 +426,14 @@ SHAMapStoreImp::run() default:; } - lastRotated_ = validatedSeq; + lastRotated = validatedSeq; dbRotating_->rotateWithLock( [&](std::string const& writableBackendName) { SavedState savedState; savedState.writableDb = newBackend->getName(); savedState.archiveDb = writableBackendName; - savedState.lastRotated = lastRotated_; + savedState.lastRotated = lastRotated; state_db_.setState(savedState); clearCaches(validatedSeq); @@ -624,6 +624,9 @@ SHAMapStoreImp::clearPrior(LedgerIndex lastRotated) if (health()) return; + // Do not allow ledgers to be acquired from the network + // that are about to be deleted. + minimumOnline_ = lastRotated + 1; ledgerMaster_->clearPriorLedgers(lastRotated); if (health()) return; @@ -715,6 +718,16 @@ SHAMapStoreImp::onChildrenStopped() } } +boost::optional +SHAMapStoreImp::minimumOnline() const +{ + // minimumOnline_ with 0 value is equivalent to unknown/not set. + // Don't attempt to acquire ledgers if that value is unknown. + if (deleteInterval_ && minimumOnline_) + return minimumOnline_.load(); + return app_.getLedgerMaster().minSqlSeq(); +} + //------------------------------------------------------------------------------ std::unique_ptr diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index 619e202d3..2fabf1a69 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -24,7 +24,6 @@ #include #include #include - #include #include #include @@ -86,13 +85,8 @@ private: static std::uint32_t const minimumDeletionInterval_ = 256; // minimum # of ledgers required for standalone mode. static std::uint32_t const minimumDeletionIntervalSA_ = 8; - // Ledger sequence at which the last deletion interval was triggered, - // or the current validated sequence as of first use - // if there have been no prior deletions. Deletion occurs up to (but - // not including) this value. All ledgers past this value are accumulated - // until the next online deletion. This value is persisted to SQLite - // nearly immediately after modification. - std::atomic lastRotated_{}; + // minimum ledger to maintain online. + std::atomic minimumOnline_{}; NodeStore::Scheduler& scheduler_; beast::Journal const journal_; @@ -168,7 +162,7 @@ public: LedgerIndex getLastRotated() override { - return lastRotated_; + return state_db_.getState().lastRotated; } // All ledgers before and including this are unprotected @@ -187,6 +181,9 @@ public: int fdRequired() const override; + boost::optional + minimumOnline() const override; + private: // callback for visitNodes bool