diff --git a/src/test/app/InboundLedger_test.cpp b/src/test/app/InboundLedger_test.cpp index 2e721cb880..413339d058 100644 --- a/src/test/app/InboundLedger_test.cpp +++ b/src/test/app/InboundLedger_test.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -13,6 +14,7 @@ #include #include #include +#include #include #include @@ -57,6 +59,37 @@ struct TestableInboundLedger final : InboundLedger trigger(nullptr, TriggerReason::Added); } + /** + * The same, as the timer chain does. + */ + void + triggerTimeout() + { + trigger(nullptr, TriggerReason::Timeout); + } + + /** + * Record how many timeouts have elapsed. + * + * @param timeouts The count to record. + */ + void + setTimeouts(int timeouts) + { + ScopedLockType const sl(mtx_); + timeouts_ = timeouts; + } + + /** + * Forget any recorded progress. + */ + void + clearProgress() + { + ScopedLockType const sl(mtx_); + progress_ = false; + } + /** * Record that nothing is left to fetch. */ @@ -393,6 +426,171 @@ struct InboundLedger_test : public beast::unit_test::Suite waitFor([&] { return env.app().getInboundLedgers().isFailure(otherHeader.hash); })); } + /** + * A ledger assembled from local data must be judged even when only + * one map is settled. + * + * tryDB() walks both maps to see what is on hand, and a fetch pack is + * checked against each node's own hash rather than the shape it + * implies, so a whole chain can resolve locally without passing + * through addKnownNode(). + * + * The asymmetry is the point: the transaction map is the chain, so + * its walk abandons it, while the state root is a hash no fetch pack + * supplies, leaving that map merely incomplete. tryDB() therefore + * sets neither flag and has to reach the verdict itself, since the + * setImmutable() call further down needs both. + * + * @param env The environment to run in. + */ + void + testLocalChainFailsAcquire(jtx::Env& env) + { + testcase("A chain found locally fails the acquire"); + + DeepChain const chain{nextSeed()}; + + // The chain as the transaction root; an arbitrary hash, seeded nowhere, as the state root. + auto const header = makeHeader(chain.rootHash.asUInt256(), uint256{99}); + auto& ledgerMaster = env.app().getLedgerMaster(); + + // The header, prefixed the way tryDB() expects to find it in a fetch pack. + Serializer hs; + hs.add32(HashPrefix::LedgerMaster); + addRaw(header, hs); + ledgerMaster.addFetchPack(header.hash, std::make_shared(hs.modData())); + + // Every node of the chain, keyed by its own hash. TransactionStateSF::getNode() reads + // these, so the transaction-map walk resolves the whole chain with no peer involved. + for (auto depth = 0u; depth <= SHAMap::kLeafDepth; ++depth) + { + ledgerMaster.addFetchPack( + chain.nodeAt(depth)->getHash().asUInt256(), + std::make_shared(chain.prefixedNodeAt(depth))); + } + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + // checkLocal() routes into tryDB() without any peer data having arrived. It reports true + // only because the acquisition ended, which is what this case is about. + BEAST_EXPECT(acquire->checkLocal()); + + BEAST_EXPECT(acquire->isFailed()); + BEAST_EXPECT(!acquire->isComplete()); + } + + /** + * The aggressive-retry branch of trigger() must judge a map the walk + * abandoned. + * + * That branch reads an empty getNeededHashes() result as "nothing + * left to fetch", and the walk it runs can reach the invalid verdict + * itself once nodes resolve from local storage rather than from a + * peer. + * + * The staging matters: tryDB() runs first and would shadow this guard + * if it could resolve the whole chain, so only the root is local to + * begin with - enough for the state map to hold a root, without which + * neededHashes() reports the root as missing and never walks, but not + * enough to reach the offending depth. Reaching the branch also needs + * a timeout count above kLedgerBecomeAggressiveThreshold, which the + * case records directly rather than waiting fifteen seconds for the + * timer chain to raise it. + * + * @param env The environment to run in. + */ + void + testAggressiveRetryJudgesLocalMap(jtx::Env& env) + { + testcase("An aggressive retry judges a map the walk abandoned"); + + DeepChain const chain{nextSeed()}; + + // The chain as the state root, and no transactions, so only the state map is in play. + auto const header = makeHeader(chain); + auto& ledgerMaster = env.app().getLedgerMaster(); + + Serializer hs; + hs.add32(HashPrefix::LedgerMaster); + addRaw(header, hs); + ledgerMaster.addFetchPack(header.hash, std::make_shared(hs.modData())); + + // Only the root, so the state map gets a root but the walk stops one level down. + ledgerMaster.addFetchPack( + chain.nodeAt(0)->getHash().asUInt256(), + std::make_shared(chain.prefixedNodeAt(0))); + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + // The acquisition is alive: it has the header and a state root, and still wants the rest. + BEAST_EXPECT(!acquire->checkLocal()); + BEAST_EXPECT(!acquire->isFailed()); + BEAST_EXPECT(acquire->getJson(0)[jss::have_header].asBool()); + BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool()); + + auto const ledger = mutableLedger(*acquire); + BEAST_EXPECT(ledger != nullptr); + if (!ledger) + return; + BEAST_EXPECT(ledger->stateMap().isValid()); + + // Only now does the rest of the chain become resolvable, so tryDB() cannot have judged it. + for (auto depth = 1u; depth <= SHAMap::kLeafDepth; ++depth) + { + ledgerMaster.addFetchPack( + chain.nodeAt(depth)->getHash().asUInt256(), + std::make_shared(chain.prefixedNodeAt(depth))); + } + + // kLedgerBecomeAggressiveThreshold is 4 and file-local, so name the requirement here. + acquire->setTimeouts(5); + acquire->clearProgress(); + acquire->triggerTimeout(); + + // The walk resolved the chain locally and abandoned the map, and trigger() recorded that + // rather than reading the empty result as a finished acquisition. + BEAST_EXPECT(!ledger->stateMap().isValid()); + BEAST_EXPECT(acquire->isFailed()); + BEAST_EXPECT(!acquire->isComplete()); + + // haveState_ is what pins this guard rather than the setImmutable() backstop in done(), + // which also fails the acquire: without the guard the empty result reads as success, and + // every have-flag is set on the way to that backstop. + BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool()); + + // The same branch with no header yet, which is the other arm of hasInvalidMap(): there is + // no map to judge, and reading that as a verdict would fail an acquisition that has only + // just started. getNeededHashes() has asked for the header, so the non-empty branch is the + // right one and the acquisition stays alive. + auto headerless = std::make_shared( + env.app(), + uint256{7}, + 0, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + headerless->setTimeouts(5); + headerless->clearProgress(); + headerless->triggerTimeout(); + + BEAST_EXPECT(mutableLedger(*headerless) == nullptr); + BEAST_EXPECT(!headerless->isFailed()); + BEAST_EXPECT(!headerless->isComplete()); + } + /** * The retry timer re-asks, then gives up and signals. * @@ -464,6 +662,8 @@ struct InboundLedger_test : public beast::unit_test::Suite testLocalLedgerCompletesAcquire(env); testInvalidatedLedgerFailsInDone(env); testLocalFailureSignalsDone(env); + testLocalChainFailsAcquire(env); + testAggressiveRetryJudgesLocalMap(env); // Last: the only case that waits out a whole timeout chain. testTimerRetriesThenGivesUp(env); diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 5add2afc98..a85fb9047f 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -177,6 +177,24 @@ private: void tryDB(node_store::Database& srcDB); + /** + * Whether either map of the ledger being acquired has been found + * invalid. + * + * A walk returns a bare list of hashes, so an empty result does not + * tell a satisfied map from an abandoned one; callers that read + * emptiness as "nothing left to fetch" must ask this first. Asked + * regardless of this acquisition's own flags after the one walk that + * runs with the lock released, whose verdict can land after another + * thread has reported the ledger complete. See SHAMap::addKnownNode + * for why the verdict is final. + * + * @return Whether either map is Invalid, and false while there is no ledger + * yet, since then there is no map to judge. + */ + [[nodiscard]] bool + hasInvalidMap() const; + void onTimer(bool progress, ScopedLockType& sl) override; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 4311a68a51..0204ce8139 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -235,6 +235,12 @@ InboundLedger::neededStateHashes(int max, SHAMapSyncFilter const* filter) const return neededHashes(ledger_->header().accountHash, ledger_->stateMap(), max, filter); } +bool +InboundLedger::hasInvalidMap() const +{ + return ledger_ && !ledger_->mapsValid(); +} + // See how much of the ledger data is stored locally // Data found in a fetch pack will be stored void @@ -339,13 +345,25 @@ InboundLedger::tryDB(node_store::Database& srcDB) } } + // Judged here rather than left to the setImmutable() call below, which runs only once both + // flags are set: the two walks above set them independently, so one map can be abandoned while + // the other is merely incomplete. + if (hasInvalidMap()) + { + JLOG(journal_.warn()) << "Ledger " << hash_ << " found locally has an invalid map"; + failed_ = true; + return; + } + if (haveTransactions_ && haveState_) { XRPL_ASSERT( ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()), "xrpl::InboundLedger::tryDB : valid ledger fees"); // Settled before complete_ is published, so a caller that reads the flag never sees a - // ledger this function has not finished with. + // ledger this function has not finished with. Reachable despite the guard above only + // because trigger() walks the state map with mtx_ released, so that walk can reach the + // verdict in between. if (!ledger_->setImmutable()) { JLOG(journal_.warn()) << "Ledger " << hash_ << " found locally is invalid"; @@ -447,11 +465,13 @@ InboundLedger::done() XRPL_ASSERT( ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()), "xrpl::InboundLedger::done : valid ledger fees"); - // Recovers rather than asserting: peer data produces this verdict, so a caller cannot know - // its map is still sound, and an abort here would be one a peer could ask for. Best-effort - // even so: setInvalid() outranks Immutable, so a walk that reaches the verdict after both - // maps have been settled leaves an immutable ledger with an invalid map. It narrows the - // window rather than closing it. + // Recovers rather than asserting: trigger() walks the state map with mtx_ released, so that + // walk can reach the verdict after the flags said there was nothing left to fetch. A race + // rather than a broken invariant, and one peer data produces, so an abort here would be one + // a peer could ask for. Best-effort even so: setInvalid() outranks Immutable, so a walk + // that reaches the verdict after both maps have been settled leaves an immutable ledger + // with an invalid map. It narrows the window rather than closing it. + SOMETIMES(hasInvalidMap(), "xrpl::InboundLedger::done : map invalidated by a race"); if (!ledger_->setImmutable()) { JLOG(journal_.warn()) << "Acquired ledger " << hash_ << " is invalid"; @@ -553,7 +573,20 @@ InboundLedger::trigger(std::shared_ptr const& peer, TriggerReason reason) { auto need = getNeededHashes(); - if (!need.empty()) + // Asked first, since getNeededHashes() walks both maps and can reach the verdict + // itself, and the branch below would otherwise read an empty result as "nothing left + // to fetch". Without a header there is no map to judge, but then getNeededHashes() has + // asked for the header, so the non-empty branch is the right one. + // Read once, so the telemetry below and the branch it feeds cannot land on two + // different observations of a map another thread is invalidating right now. + bool const invalidMap = hasInvalidMap(); + SOMETIMES(invalidMap, "xrpl::InboundLedger::trigger : map is invalid"); + if (invalidMap) + { + JLOG(journal_.warn()) << "Acquire " << hash_ << " has an invalid map"; + failed_ = true; + } + else if (!need.empty()) { protocol::TMGetObjectByHash tmBH; bool typeSet = false; @@ -662,22 +695,28 @@ InboundLedger::trigger(std::shared_ptr const& peer, TriggerReason reason) auto nodes = ledger_->stateMap().getMissingNodes(kMissingNodesFind, &filter); sl.lock(); + // Asked ahead of the flags below and outside their guard, since the verdict is about + // the map rather than about this round: it holds even when another thread reported this + // ledger complete while the lock was released, and that guard would drop it in exactly + // that case, leaving a ledger reported complete whose map cannot be the one the header + // names. The claim is withdrawn alongside the failure for the same reason. + if (hasInvalidMap()) + { + JLOG(journal_.warn()) << "Ledger " << hash_ << " has a map its walk abandoned"; + failed_ = true; + complete_ = false; + } // Make sure nothing happened while we released the lock - if (!failed_ && !complete_ && !haveState_) + else if (!failed_ && !complete_ && !haveState_) { if (nodes.empty()) { - if (!ledger_->stateMap().isValid()) - { - failed_ = true; - } - else - { - haveState_ = true; + // Sound rather than merely finished: the walk above cannot have abandoned the + // map without the test ahead of this one having caught it. + haveState_ = true; - if (haveTransactions_) - complete_ = true; - } + if (haveTransactions_) + complete_ = true; } else {