diff --git a/src/test/app/LedgerMaster_test.cpp b/src/test/app/LedgerMaster_test.cpp index d41868c76c..e0800f0d81 100644 --- a/src/test/app/LedgerMaster_test.cpp +++ b/src/test/app/LedgerMaster_test.cpp @@ -123,6 +123,10 @@ 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; diff --git a/src/test/app/SHAMapStore_test.cpp b/src/test/app/SHAMapStore_test.cpp index ea4aff70e4..06ba645cdb 100644 --- a/src/test/app/SHAMapStore_test.cpp +++ b/src/test/app/SHAMapStore_test.cpp @@ -642,6 +642,150 @@ 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 const alice = Account("alice"); + env.fund(XRP(1000), alice); + env.close(); + + auto& lm = env.app().getLedgerMaster(); + LedgerIndex minSeq = 2; + LedgerIndex maxSeq = env.closed()->info().seq; + auto& store = env.app().getSHAMapStore(); + 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 + while (maxSeq < 20) + { + for (int t = 0; t < 3; ++t) + { + env(noop(alice)); + } + env.close(); + store.rendezvous(); + + ++maxSeq; + + if (maxSeq + 1 == lastRotated + deleteInterval) + { + // The next ledger will trigger a rotation. Delete an arbitrary + // ledger from LedgerMaster. + LedgerIndex const deleteSeq = maxSeq; + 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), + 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()! You'll end up with a deadlock. + ++maxSeq; + + // Nothing has changed + BEAST_EXPECTS( + store.getLastRotated() == lastRotated, + to_string(store.getLastRotated())); + BEAST_EXPECTS( + lm.getCompleteLedgers() == + expectedRange(minSeq, deleteSeq, maxSeq), + lm.getCompleteLedgers()); + + using namespace std::chrono_literals; + // Close 5 more ledgers, waiting one second 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(); + // DO NOT CALL rendezvous()! You'll end up with a deadlock. + ++maxSeq; + // Nothing has changed + BEAST_EXPECTS( + store.getLastRotated() == lastRotated, + to_string(store.getLastRotated())); + BEAST_EXPECTS( + lm.getCompleteLedgers() == + expectedRange(minSeq, deleteSeq, maxSeq), + lm.getCompleteLedgers()); + std::this_thread::sleep_for(1s); + } + + // Put the missing ledger back in LedgerMaster + lm.setLedgerRangePresent(deleteSeq, deleteSeq); + + // Wait for the rotation to finish + store.rendezvous(); + + minSeq = lastRotated; + lastRotated = deleteSeq + 1; + } + BEAST_EXPECT(maxSeq != lastRotated + deleteInterval); + BEAST_EXPECTS( + env.closed()->info().seq == maxSeq, + to_string(env.closed()->info().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); + } + } + void run() override { @@ -649,6 +793,7 @@ public: testAutomatic(); testCanDelete(); testRotate(); + testLedgerGaps(); } }; diff --git a/src/xrpld/app/misc/SHAMapStoreImp.cpp b/src/xrpld/app/misc/SHAMapStoreImp.cpp index 5a279cdea8..476af3d451 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -308,6 +308,12 @@ SHAMapStoreImp::run() validatedSeq >= lastRotated + deleteInterval_ && canDelete_ >= lastRotated - 1 && healthWait() == keepGoing; + // 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. + lastGoodValidatedLedger_ = validatedSeq; + // will delete up to (not including) lastRotated if (readyToRotate) { @@ -639,12 +645,6 @@ SHAMapStoreImp::healthWait() OperatingMode mode = netOPs_->getOperatingMode(); std::unique_lock lock(mutex_); - if (lastGoodValidatedLedger_ == 0) - { - // TODO: Do we want to initialze lastGoodValidatedLedger_ to the value - // of lastRotated in run()? - lastGoodValidatedLedger_ = index; - } auto numMissing = ledgerMaster_->missingFromCompleteLedgerRange( lastGoodValidatedLedger_, index); while ( diff --git a/src/xrpld/app/misc/SHAMapStoreImp.h b/src/xrpld/app/misc/SHAMapStoreImp.h index 7fdb7a9c17..6fe37bb611 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.h +++ b/src/xrpld/app/misc/SHAMapStoreImp.h @@ -89,6 +89,10 @@ 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_;