From 736b88a8f56fcfdda49c6bda98884fc71e99c206 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 +++----- src/test/unit_test/SuiteJournal.h | 2 +- 6 files changed, 72 insertions(+), 47 deletions(-) diff --git a/src/ripple/app/ledger/LedgerMaster.h b/src/ripple/app/ledger/LedgerMaster.h index fadac2f9e46..b82fce0bd12 100644 --- a/src/ripple/app/ledger/LedgerMaster.h +++ b/src/ripple/app/ledger/LedgerMaster.h @@ -286,6 +286,10 @@ class LedgerMaster : public Stoppable, public AbstractFetchPackContainer 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 3cee63b1043..9a8f7dfe382 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 d5f63ab4219..8952453cba0 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 @@ class SHAMapStore : public Stoppable /** 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 3c9ccb94235..94deae5d276 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 619e202d335..2fabf1a6996 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 @@ class SHAMapStoreImp : public SHAMapStore 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 @@ class SHAMapStoreImp : public SHAMapStore LedgerIndex getLastRotated() override { - return lastRotated_; + return state_db_.getState().lastRotated; } // All ledgers before and including this are unprotected @@ -187,6 +181,9 @@ class SHAMapStoreImp : public SHAMapStore int fdRequired() const override; + boost::optional + minimumOnline() const override; + private: // callback for visitNodes bool diff --git a/src/test/unit_test/SuiteJournal.h b/src/test/unit_test/SuiteJournal.h index 74b03b71ade..6c7a0b54e64 100644 --- a/src/test/unit_test/SuiteJournal.h +++ b/src/test/unit_test/SuiteJournal.h @@ -98,7 +98,7 @@ class SuiteJournal : sink_(partition, threshold, suite), journal_(sink_) { } - operator beast::Journal &() + operator beast::Journal&() { return journal_; }