diff --git a/include/xrpl/ledger/Ledger.h b/include/xrpl/ledger/Ledger.h index 8dcfc1d213..38b4e30972 100644 --- a/include/xrpl/ledger/Ledger.h +++ b/include/xrpl/ledger/Ledger.h @@ -257,13 +257,43 @@ public: header_.validated = true; } - void + /** + * Mark this ledger as accepted and attempt to make it immutable. + * + * The close-time fields are recorded before the maps are settled, since the + * ledger hash covers them. + * + * @param closeTime The consensus-agreed close time. + * @param closeResolution The close time resolution. + * @param correctCloseTime Whether consensus agreed on the close time; if + * false, kSLcfNoConsensusTime is recorded in closeFlags instead. + * @return What setImmutable() returned, so false means the ledger must be + * discarded rather than retried. + */ + [[nodiscard]] bool setAccepted( NetClock::time_point closeTime, NetClock::duration closeResolution, bool correctCloseTime); - void + /** + * Mark this ledger as immutable, so it can no longer be modified. + * + * A ledger built or loaded locally cannot have an invalid map, since only a + * map syncing against hashes from outside can be proven impossible (see + * SHAMap::addKnownNode), so a caller on such a path may treat a false return + * as a broken internal invariant. A caller assembling a ledger from peer data + * may not: for it, false is an outcome a peer can produce. + * + * @param rehash Whether to recompute the ledger hash from the header fields. + * The transaction and account hashes are recomputed from the maps too, + * but only the first time. + * @return false if either map is Invalid, leaving the immutable flag unset and + * the header untouched. A map invalidated partway through can still + * leave the other one immutable, so a false return means the ledger + * must be discarded rather than retried. + */ + [[nodiscard]] bool setImmutable(bool rehash = true); bool @@ -272,16 +302,28 @@ public: return immutable_; } - /* Mark this ledger as "should be full". + /** + * Whether neither map has been found invalid. + * + * Read by whatever assembles the ledger, which cannot tell a map that + * is merely incomplete from one that has been abandoned by looking at + * what a walk returned. See SHAMap::isValid(). + * + * @return Whether both maps can still be the maps the header names. + */ + [[nodiscard]] bool + mapsValid() const + { + return txMap_.isValid() && stateMap_.isValid(); + } - "Full" is metadata property of the ledger, it indicates - that the local server wants all the corresponding nodes - in durable storage. - - This is marked `const` because it reflects metadata - and not data that is in common with other nodes on the - network. - */ + /** + * Mark this ledger as "should be full", indicating that the local server + * wants all the corresponding nodes in durable storage. + * + * Const because it reflects metadata, not data this ledger shares with + * other nodes on the network. + */ void setFull() const { @@ -421,6 +463,24 @@ private: static std::pair, std::shared_ptr> deserializeTxPlusMeta(SHAMapItem const& item); + /** + * Make both maps immutable, without short-circuiting. + * + * A concurrent walk can invalidate one map after the other is settled, + * so both calls are always made rather than one guarding the other: + * each map becomes Immutable or stays Invalid on its own, and neither + * is left mid-sync because the other refused. + * + * @return Whether both maps are immutable. + */ + [[nodiscard]] bool + setMapsImmutable() + { + bool const txImmutable = txMap_.setImmutable(); + bool const stateImmutable = stateMap_.setImmutable(); + return txImmutable && stateImmutable; + } + bool immutable_; // A SHAMap containing the transactions associated with this ledger. diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 96ccb34643..8f712e0e21 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -180,7 +181,14 @@ public: SHAMap& operator=(SHAMap const&) = delete; - // Take a snapshot of the given map: + /** + * Take a snapshot of the given map. + * + * @param other The map to snapshot. An Invalid source yields an Invalid + * snapshot, since the two share the same node structure. + * @param isMutable Whether the snapshot may be modified. Ignored when other + * is Invalid, since that state outranks both alternatives. + */ SHAMap(SHAMap const& other, bool isMutable); // build new map @@ -218,8 +226,15 @@ public: //-------------------------------------------------------------------------- - // Returns a new map that's a snapshot of this one. - // Handles copy on write for mutable snapshots. + /** + * Return a new map that is a snapshot of this one. + * + * Handles copy on write for mutable snapshots. An invalid map yields an + * invalid snapshot, since the two share the same node structure. + * + * @param isMutable Whether the snapshot may be modified. + * @return The snapshot. + */ std::shared_ptr snapShot(bool isMutable) const; @@ -408,7 +423,13 @@ public: SHAMapTreeNodePtr treeNode, SHAMapSyncFilter const* filter); - void + /** + * Mark this map as immutable, so it can no longer be modified. + * + * @return false if the map is Invalid and was left unchanged, true + * otherwise. + */ + [[nodiscard]] bool setImmutable(); /** @@ -418,8 +439,24 @@ public: */ [[nodiscard]] bool isSynching() const; + + /** + * Mark this map as syncing, fixing its hash while still allowing missing + * nodes to be added. + * + * Left unchanged if the map is Invalid, which is terminal - though that + * case is itself treated as unreachable (and asserts in a build with + * assertions enabled), since nothing should call this on a map that has + * already been judged. + */ void setSynching(); + + /** + * Mark this map as no longer syncing, so it can be modified again. + * + * Does nothing if the map is Invalid, which is terminal. + */ void clearSynching(); @@ -499,11 +536,27 @@ private: * Record that the map is provably not the one it claims to be. * * Private because only the map itself can prove that, from a node that - * contradicts the hashes it is syncing against. + * contradicts the hashes it is syncing against. Cannot fail, since + * Invalid outranks every other state; see trySetState(). */ void setInvalid(); + /** + * Move the map to a new state, atomically. + * + * The only writer of state_ past construction, so the order between + * the states lives in one place: Invalid outranks all of them and is + * always stored, while every other transition is refused once the map + * is Invalid, which is what makes that verdict terminal. + * + * @param desired The state to move to. + * @return false if the map is Invalid and the requested state is not, + * leaving it unchanged; true otherwise. + */ + bool + trySetState(SHAMapState desired); + // tree node cache operations SHAMapTreeNodePtr cacheLookup(SHAMapHash const& hash) const; @@ -740,11 +793,41 @@ SHAMap::state() const return state_.load(std::memory_order_acquire); } -inline void +inline bool +SHAMap::trySetState(SHAMapState desired) +{ + // Stored outright rather than exchanged, since Invalid outranks every other state: only the map + // itself reaches that verdict, from a node that contradicts the hashes it is syncing against, + // so a walk that reaches it has to win however it interleaves with a thread settling the map. + // An exchange that refused to overwrite Immutable would leave a map proven impossible reporting + // itself sound, which is the one thing nothing downstream could recover from. + if (desired == SHAMapState::Invalid) + { + state_.store(SHAMapState::Invalid, std::memory_order_release); + return true; + } + + // Compare-exchange rather than check-then-store, so the refusal to leave Invalid holds no + // matter how this call interleaves with another thread's. Invalid is the only state this + // refuses to leave; the loop simply retries if another one is stored meanwhile. No load ahead + // of it, since a failed exchange both reports the state and refreshes expected. + auto expected = SHAMapState::Modifying; + while (expected != SHAMapState::Invalid) + { + if (state_.compare_exchange_weak( + expected, desired, std::memory_order_acq_rel, std::memory_order_acquire)) + { + return true; + } + } + return false; +} + +inline bool SHAMap::setImmutable() { - XRPL_ASSERT(isValid(), "xrpl::SHAMap::setImmutable : state is valid"); - state_.store(SHAMapState::Immutable, std::memory_order_release); + SOMETIMES(!isValid(), "xrpl::SHAMap::setImmutable : map is invalid"); + return trySetState(SHAMapState::Immutable); } inline bool @@ -756,13 +839,32 @@ SHAMap::isSynching() const inline void SHAMap::setSynching() { - state_.store(SHAMapState::Synching, std::memory_order_release); + // Guarded, so this is not a way out of Invalid, matching clearSynching(). + if (!trySetState(SHAMapState::Synching)) + { + // Unreachable today, though not because the map is Modifying: a ledger built from a header + // starts out with both maps Synching already. It is unreachable because this is only ever + // called on a map that has just been constructed, so nothing can have synced against it and + // reached a verdict on it yet. + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::setSynching : map is invalid"); + // LCOV_EXCL_STOP + } } inline void SHAMap::clearSynching() { - state_.store(SHAMapState::Modifying, std::memory_order_release); + // Guarded, so an invalid map stays invalid rather than being moved back to Modifying, which + // passes isValid(). Refusing is the contract rather than a broken invariant, so this reports + // instead of asserting: peer data produces the verdict, so an UNREACHABLE here would be an + // abort a peer could ask for. + SOMETIMES(!isValid(), "xrpl::SHAMap::clearSynching : map is invalid"); + if (!trySetState(SHAMapState::Modifying)) + { + JLOG(journal_.warn()) << "Refused to clear synching on an invalid map, root hash " + << root_->getHash(); + } } inline bool @@ -774,7 +876,9 @@ SHAMap::isValid() const inline void SHAMap::setInvalid() { - state_.store(SHAMapState::Invalid, std::memory_order_release); + // Through trySetState() like every other transition, so nothing writes state_ behind its back + // and the order between the states is stated once. Cannot fail: Invalid outranks all of them. + trySetState(SHAMapState::Invalid); } inline void diff --git a/src/libxrpl/ledger/Ledger.cpp b/src/libxrpl/ledger/Ledger.cpp index 188fb3cd2c..987f532320 100644 --- a/src/libxrpl/ledger/Ledger.cpp +++ b/src/libxrpl/ledger/Ledger.cpp @@ -202,7 +202,14 @@ Ledger::Ledger( } stateMap_.flushDirty(NodeObjectType::AccountNode); - setImmutable(); + // Built locally; see Ledger::setImmutable(). Failed deterministically rather than handing back + // a mutable ledger that would abort later at a site that cannot explain why; logicError() logs. + if (!setImmutable()) + { + // LCOV_EXCL_START + logicError("Ledger::Ledger(CreateGenesisT, ...): genesis ledger map is invalid"); + // LCOV_EXCL_STOP + } } Ledger::Ledger( @@ -213,7 +220,7 @@ Ledger::Ledger( Fees const& fees, Family& family, beast::Journal j) - : immutable_(true) + : immutable_(false) , txMap_(SHAMapType::TRANSACTION, info.txHash, family) , stateMap_(SHAMapType::STATE, info.accountHash, family) , fees_(fees) @@ -236,8 +243,20 @@ Ledger::Ledger( JLOG(j.warn()) << "Don't have state data root for ledger" << header_.seq; } - txMap_.setImmutable(); - stateMap_.setImmutable(); + // Loaded locally; see Ledger::setImmutable(). + if (setMapsImmutable()) + { + immutable_ = true; + } + else + { + // LCOV_EXCL_START + JLOG(j.error()) << "Invalid map for ledger " << header_.seq; + UNREACHABLE("xrpl::Ledger::Ledger(LedgerHeader const&, ...) : map is invalid"); + // Treat it as a damaged ledger: the code below recomputes the hash and re-acquires. + loaded = false; + // LCOV_EXCL_STOP + } if (!setup()) loaded = false; @@ -308,27 +327,51 @@ Ledger::Ledger( setup(); } -void +bool Ledger::setImmutable(bool rehash) { - // Force update, since this is the only - // place the hash transitions to valid - if (!immutable_ && rehash) + // A map found structurally invalid during sync must never be made immutable: isValid() tests + // only for Invalid, and an immutable ledger is treated as persistable. Asked before anything is + // written, so a refusal leaves the header exactly as it was rather than half relabelled. + if (!mapsValid()) + return false; + + // Read here but written to the header below, once the maps are settled: getHash() can unshare a + // dirty tree, so it has to run while the map is still mutable, while a write to the header must + // wait until both maps have made it. Skipped once the ledger is immutable, since its maps can + // no longer change. + bool const deriveMapHashes = !immutable_ && rehash; + uint256 const txHash = deriveMapHashes ? txMap_.getHash().asUInt256() : uint256{}; + uint256 const accountHash = deriveMapHashes ? stateMap_.getHash().asUInt256() : uint256{}; + + // Both were valid at the check above, but a concurrent walk can invalidate one in between (see + // SHAMap::state_), so the result is checked rather than assumed. Best-effort by nature: the + // guard narrows the window, it does not close it, since setInvalid() outranks Immutable and can + // land after both maps have been settled. + bool const bothImmutable = setMapsImmutable(); + SOMETIMES(!bothImmutable, "xrpl::Ledger::setImmutable : map invalidated while going immutable"); + if (!bothImmutable) + return false; + + // Written only now, so losing the race above leaves the header describing what the ledger was + // built from rather than a map that has since been abandoned. Forced rather than conditional, + // since this is the only place the hash transitions to valid. + if (deriveMapHashes) { - header_.txHash = txMap_.getHash().asUInt256(); - header_.accountHash = stateMap_.getHash().asUInt256(); + header_.txHash = txHash; + header_.accountHash = accountHash; } if (rehash) header_.hash = calculateLedgerHash(header_); + // Set last, so isImmutable() never reports a ledger whose maps are not both immutable. immutable_ = true; - txMap_.setImmutable(); - stateMap_.setImmutable(); setup(); + return true; } -void +bool Ledger::setAccepted( NetClock::time_point closeTime, NetClock::duration closeResolution, @@ -340,7 +383,17 @@ Ledger::setAccepted( header_.closeTime = closeTime; header_.closeTimeResolution = closeResolution; header_.closeFlags = correctCloseTime ? 0 : kSLcfNoConsensusTime; - setImmutable(); + // Built locally; see Ledger::setImmutable(). + if (!setImmutable()) + { + // LCOV_EXCL_START + JLOG(j_.error()) << "Invalid map for accepted ledger " << header_.seq; + UNREACHABLE("xrpl::Ledger::setAccepted : map is invalid"); + return false; + // LCOV_EXCL_STOP + } + + return true; } bool diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 01184ebd30..08a1feb91b 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -80,12 +80,23 @@ SHAMap::SHAMap(SHAMap const& other, bool isMutable) , cowid_(other.cowid_ + 1) , ledgerSeq_(other.ledgerSeq()) , root_(other.root_) - , state_(isMutable ? SHAMapState::Modifying : SHAMapState::Immutable) , type_(other.type_) , backed_(other.backed_) { + // A snapshot shares the source's root, so Invalid carries over rather than being promoted to + // Modifying or Immutable, either of which would pass isValid(). Carried rather than refused, + // since a constructor cannot refuse. Read once into a local, or a concurrent walk could have + // the source report one state here and another below. + auto const otherState = other.state(); + auto const ownState = [&] { + if (otherState == SHAMapState::Invalid) + return SHAMapState::Invalid; + return isMutable ? SHAMapState::Modifying : SHAMapState::Immutable; + }(); + state_.store(ownState, std::memory_order_release); + // If either map may change, they cannot share nodes - if ((state() != SHAMapState::Immutable) || (other.state() != SHAMapState::Immutable)) + if ((ownState != SHAMapState::Immutable) || (otherState != SHAMapState::Immutable)) { unshare(); } diff --git a/src/test/app/InboundLedger_test.cpp b/src/test/app/InboundLedger_test.cpp index e5091f5418..2e721cb880 100644 --- a/src/test/app/InboundLedger_test.cpp +++ b/src/test/app/InboundLedger_test.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -55,8 +56,41 @@ struct TestableInboundLedger final : InboundLedger { trigger(nullptr, TriggerReason::Added); } + + /** + * Record that nothing is left to fetch. + */ + void + markComplete() + { + ScopedLockType const sl(mtx_); + complete_ = true; + } + + /** + * Settle the acquisition and signal whatever is waiting on it. + */ + void + signalDone() + { + ScopedLockType const sl(mtx_); + done(); + } }; +/** + * The ledger an acquisition is assembling, as a pointer that can modify it. + * + * @param acquire The acquisition to read from. + * @return The ledger, or nullptr if there is none to report. + */ +[[nodiscard]] static std::shared_ptr +mutableLedger(InboundLedger const& acquire) +{ + // Sound because the acquisition holds a non-const ledger and only hands out a const view. + return std::const_pointer_cast(acquire.getLedger()); +} + struct InboundLedger_test : public beast::unit_test::Suite { /** @@ -239,6 +273,71 @@ struct InboundLedger_test : public beast::unit_test::Suite BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash)); } + /** + * A ledger whose map goes invalid on the way to being settled must be + * discarded rather than delivered. + * + * done() settles the ledger before it logs or acts on the outcome, and a + * map abandoned by then makes settling refuse, so the acquisition has to + * record a failure instead. Reproduced by setting the flag and then + * invalidating the map, which is the order a walk on another thread + * produces without the second thread. + * + * @param env The environment to run in. + */ + void + testInvalidatedLedgerFailsInDone(jtx::Env& env) + { + testcase("A ledger invalidated on its way to being settled fails"); + + // The fabricated chain, so feeding it to the state map invalidates the map. + DeepChain const chain{nextSeed()}; + + // Only the header is local, so the acquisition holds a ledger with an empty state map. + auto const header = makeHeader(chain); + storeHeader(env, header); + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + BEAST_EXPECT(!acquire->checkLocal()); + BEAST_EXPECT(!acquire->isFailed()); + BEAST_EXPECT(!acquire->isComplete()); + + auto const ledger = mutableLedger(*acquire); + BEAST_EXPECT(ledger != nullptr); + if (!ledger) + return; + + // The state of affairs done() is handed: nothing left to fetch as far as the caller could + // tell. + acquire->markComplete(); + + // And the walk that has since reached the verdict. + auto& stateMap = ledger->stateMap(); + BEAST_EXPECT(stateMap.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + for (auto const& [nodeID, node] : chain.nodesBelowRoot()) + stateMap.addKnownNode(nodeID, node, nullptr); + BEAST_EXPECT(!stateMap.isValid()); + + acquire->signalDone(); + + // complete_ is withdrawn alongside the failure, or every guard that checks it before + // failed_ keeps treating this ledger as delivered. + BEAST_EXPECT(!acquire->isComplete()); + BEAST_EXPECT(acquire->isFailed()); + + // Nothing was handed to LedgerMaster, and the hash is remembered as a failure so it is not + // immediately re-acquired. + BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) == nullptr); + BEAST_EXPECT(waitFor([&] { return env.app().getInboundLedgers().isFailure(header.hash); })); + } + /** * An acquisition that fails on local data must still signal. * @@ -363,6 +462,7 @@ struct InboundLedger_test : public beast::unit_test::Suite jtx::Env env{*this}; testLocalLedgerCompletesAcquire(env); + testInvalidatedLedgerFailsInDone(env); testLocalFailureSignalsDone(env); // Last: the only case that waits out a whole timeout chain. diff --git a/src/test/app/LedgerHistory_test.cpp b/src/test/app/LedgerHistory_test.cpp index f8688899e0..0360916960 100644 --- a/src/test/app/LedgerHistory_test.cpp +++ b/src/test/app/LedgerHistory_test.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -22,6 +23,7 @@ #include #include +#include #include namespace xrpl::test { @@ -70,11 +72,16 @@ public: } res->unshare(); - // Accept ledger - res->setAccepted( - res->header().closeTime, - res->header().closeTimeResolution, - true /* close time correct*/); + // Accept ledger. Thrown rather than asserted because a failure leaves res unusable, and + // an assert would let a Release build hand every caller below a broken ledger. This helper + // is static, so BEAST_EXPECT is out of reach; the suite runner reports the throw. + if (!res->setAccepted( + res->header().closeTime, + res->header().closeTimeResolution, + true /* close time correct*/)) + { + Throw("makeLedger: ledger could not be accepted"); + } lh.insert(res, false); return res; } diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index 9ace2e21af..9abca20e8c 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -100,7 +100,7 @@ class RCLValidations_test : public beast::unit_test::Suite BEAST_EXPECT(next->read(keylet::feeSettings())); if (forceHash) { - next->setImmutable(); + BEAST_EXPECT(next->setImmutable()); forceHash = false; } diff --git a/src/test/rpc/BookChanges_test.cpp b/src/test/rpc/BookChanges_test.cpp index f0b4a4e187..b7749770d5 100644 --- a/src/test/rpc/BookChanges_test.cpp +++ b/src/test/rpc/BookChanges_test.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -190,7 +191,7 @@ public: metadata->add(*metaSerializer); ledger->rawTxInsert(uint256{1}, txSerializer, metaSerializer); - ledger->setImmutable(); + BEAST_EXPECT(ledger->setImmutable()); ledger->setValidated(); try @@ -260,7 +261,12 @@ public: ledger->rawTxInsert(uint256{seq}, txSerializer, metaSerializer); } - ledger->setImmutable(); + // Thrown rather than asserted because a refusal leaves the ledger unusable, and this + // helper is static, so BEAST_EXPECT is out of reach; the suite runner reports the throw. + if (!ledger->setImmutable()) + { + Throw("bookChangesFor: ledger could not be made immutable"); + } ledger->setValidated(); return xrpl::rpc::computeBookChanges(std::static_pointer_cast(ledger)); diff --git a/src/tests/libxrpl/helpers/TxTest.cpp b/src/tests/libxrpl/helpers/TxTest.cpp index 4b4f407eeb..242286383d 100644 --- a/src/tests/libxrpl/helpers/TxTest.cpp +++ b/src/tests/libxrpl/helpers/TxTest.cpp @@ -207,7 +207,10 @@ TxTest::close() accum.apply(*newLedger); } - newLedger->setAccepted(ledgerCloseTime, newLedger->header().closeTimeResolution, true); + if (!newLedger->setAccepted(ledgerCloseTime, newLedger->header().closeTimeResolution, true)) + { + Throw("TxTest::close: ledger has an invalid map"); + } closedLedger_ = newLedger; diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index fa383f357a..e78a9ccd7e 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -2,11 +2,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -252,7 +254,7 @@ protected: }; // An inner node at kLeafDepth, where only leaves can live, leaves the map provably invalid. It -// must be reported as bad data rather than counted as useful. +// must be reported as bad data, and the map must then refuse to become immutable. TEST_F(SHAMapSyncTest, innerNodeAtLeafDepth) { TestNodeFamily f{j_}; @@ -269,6 +271,10 @@ TEST_F(SHAMapSyncTest, innerNodeAtLeafDepth) EXPECT_TRUE(tallyIs(result, 0, 1, 0)); EXPECT_FALSE(result.isGood()); EXPECT_FALSE(map.isValid()); + + // Invalid is terminal, so the map can no longer be made immutable and therefore cannot be + // persisted. + EXPECT_FALSE(map.setImmutable()); } // A node that cannot be hooked anywhere is bad data, so the batch counts no progress, but the map @@ -335,6 +341,160 @@ TEST_F(SHAMapSyncTest, mapInvalidatingNodeIsJudgedOnCacheHit) EXPECT_TRUE(tallyIs(result, 0, 1, 0)); EXPECT_FALSE(result.isGood()); EXPECT_FALSE(map.isValid()); + EXPECT_FALSE(map.setImmutable()); +} + +// An invalid tx map must also stop the enclosing ledger from being marked immutable, since an +// immutable ledger is treated as persistable. +TEST_F(SHAMapSyncTest, invalidTxMapBlocksImmutableLedger) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + Ledger ledger{1, NetClock::time_point{}, noAmendments(), Fees{}, f}; + ASSERT_FALSE(ledger.isImmutable()); + + ledger.txMap().setSynching(); + ASSERT_TRUE(chain.fill(ledger.txMap())); + + auto const result = chain.addOffendingNode(ledger.txMap()); + ASSERT_TRUE(tallyIs(result, 0, 1, 0)); + ASSERT_FALSE(ledger.txMap().isValid()); + + // The state map is untouched, so only the transaction map can be refusing. + ASSERT_TRUE(ledger.stateMap().isValid()); + + EXPECT_FALSE(ledger.setImmutable()); + EXPECT_FALSE(ledger.isImmutable()); +} + +// The same for the state map. Ledger::setImmutable() tests both maps in one expression, so this +// covers the second operand: a sound transaction map must not let an invalid state map through. +TEST_F(SHAMapSyncTest, invalidStateMapBlocksImmutableLedger) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + Ledger ledger{1, NetClock::time_point{}, noAmendments(), Fees{}, f}; + ASSERT_FALSE(ledger.isImmutable()); + + ledger.stateMap().setSynching(); + ASSERT_TRUE(chain.fill(ledger.stateMap())); + + auto const result = chain.addOffendingNode(ledger.stateMap()); + ASSERT_TRUE(tallyIs(result, 0, 1, 0)); + ASSERT_FALSE(ledger.stateMap().isValid()); + + // The transaction map is untouched, so only the state map can be refusing. + ASSERT_TRUE(ledger.txMap().isValid()); + + EXPECT_FALSE(ledger.setImmutable()); + EXPECT_FALSE(ledger.isImmutable()); +} + +// A refusal must leave the header exactly as it was. setImmutable() derives the map hashes from the +// maps and then the ledger hash from the header, so writes made before the refusal would relabel +// the ledger on its way to failing, leaving a header that no longer describes what it was built +// from. This case is caught by the check made up front; the same must hold of the re-test after the +// maps are settled, which is why the header is written only once that one has passed too. +TEST_F(SHAMapSyncTest, refusedSettleLeavesTheHeaderAlone) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + // Not the header constructor: this one derives its map hashes, which is what must not happen. + Ledger ledger{1, NetClock::time_point{}, noAmendments(), Fees{}, f}; + ASSERT_FALSE(ledger.isImmutable()); + ASSERT_TRUE(ledger.header().txHash.isZero()); + ASSERT_TRUE(ledger.header().accountHash.isZero()); + auto const hashBefore = ledger.header().hash; + + // A transaction map that hashes to something, so a derived header hash would differ from the + // one the ledger has now. + ASSERT_TRUE(ledger.txMap().addItem(SHAMapNodeType::TnTransactionNm, makeRandomAS())); + ASSERT_TRUE(ledger.txMap().getHash().isNonZero()); + + // And a state map the chain abandons, so settling has to refuse. + ledger.stateMap().setSynching(); + ASSERT_TRUE(chain.fill(ledger.stateMap())); + ASSERT_TRUE(chain.addOffendingNode(ledger.stateMap()).isInvalid()); + ASSERT_FALSE(ledger.stateMap().isValid()); + + EXPECT_FALSE(ledger.setImmutable()); + + // Nothing was written: not the map hashes, not the ledger hash, and not the flag. + EXPECT_FALSE(ledger.isImmutable()); + EXPECT_TRUE(ledger.header().txHash.isZero()); + EXPECT_TRUE(ledger.header().accountHash.isZero()); + EXPECT_EQ(ledger.header().hash, hashBefore); +} + +// Invalid is terminal: setImmutable() and clearSynching() offer no way back out of it, however +// many times they are called. setSynching() is left alone, since it is unreachable on an invalid +// map today and says so with an UNREACHABLE that aborts under -Dassert. +TEST_F(SHAMapSyncTest, invalidStateIsTerminal) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(chain.fill(map)); + ASSERT_TRUE(chain.addOffendingNode(map).isInvalid()); + ASSERT_FALSE(map.isValid()); + + // Repeated attempts must each fail, and must not leave the map reporting a valid state. + for (auto attempt = 0; attempt < 3; ++attempt) + { + EXPECT_FALSE(map.setImmutable()) << "attempt " << attempt; + EXPECT_FALSE(map.isValid()) << "attempt " << attempt; + } + + // Nor does clearSynching(), which keeps an abandoned map from being moved back to Modifying and + // passing isValid() again. It refuses rather than aborting, since a concurrent walk can + // invalidate a map between a caller's own check and this call. + for (auto attempt = 0; attempt < 3; ++attempt) + { + map.clearSynching(); + EXPECT_FALSE(map.isValid()) << "attempt " << attempt; + } + + // An invalid map is not synching either, so nothing reads it as mid-acquisition. + EXPECT_FALSE(map.isSynching()); +} + +// A snapshot shares the source map's root, so it inherits whatever the source was found to be. +// Invalid carries over, since promoting it would hand back a map that passes isValid(). +TEST_F(SHAMapSyncTest, snapshotOfInvalidMapStaysInvalid) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(chain.fill(map)); + + auto const result = chain.addOffendingNode(map); + ASSERT_TRUE(tallyIs(result, 0, 1, 0)); + ASSERT_FALSE(map.isValid()); + + // Both flavors: the immutable snapshot is the one that would be persisted, + // and the mutable one would otherwise launder the state back to Modifying. + for (bool const isMutable : {false, true}) + { + auto const snapshot = map.snapShot(isMutable); + ASSERT_TRUE(snapshot != nullptr); + EXPECT_FALSE(snapshot->isValid()) << "isMutable " << isMutable; + EXPECT_FALSE(snapshot->setImmutable()) << "isMutable " << isMutable; + } + + // A snapshot of a sound map is unaffected. + SHAMap valid{SHAMapType::FREE, f}; + valid.addItem(SHAMapNodeType::TnAccountState, makeRandomAS()); + EXPECT_TRUE(valid.snapShot(false)->isValid()); + EXPECT_TRUE(valid.snapShot(true)->isValid()); } // A map marked complete in the database withdraws that claim the first time a read misses, and @@ -472,7 +632,7 @@ TEST_F(SHAMapSyncTest, sync) ASSERT_TRUE(confuseMap(source, kNodesToConfuse)); source.invariants(); - source.setImmutable(); + ASSERT_TRUE(source.setImmutable()); std::size_t count = 0; source.visitLeaves([&count]([[maybe_unused]] auto const& item) { ++count; }); diff --git a/src/xrpld/app/ledger/detail/BuildLedger.cpp b/src/xrpld/app/ledger/detail/BuildLedger.cpp index e9c01c7133..31d0cb4a62 100644 --- a/src/xrpld/app/ledger/detail/BuildLedger.cpp +++ b/src/xrpld/app/ledger/detail/BuildLedger.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -76,7 +77,22 @@ buildLedgerImpl( XRPL_ASSERT( built->header().seq < kXrpLedgerEarliestFees || built->read(keylet::feeSettings()), "xrpl::buildLedgerImpl : valid ledger fees"); - built->setAccepted(closeTime, closeResolution, closeTimeCorrect); + // Built locally; see Ledger::setImmutable(). built's txMap_ is fresh and its stateMap_ is a + // snapshot of the parent's, which carries Invalid over, so an invalid parent is the only way + // this can fail - and reaching that needs a corrupt local nodestore rather than a peer, since a + // parent's root hashes are ones this node already adopted. + // + // logicError() rather than UNREACHABLE(), deliberately: this runs on the consensus hot path and + // so aborts a Release build, which is the harsher of the two tiers. It is chosen because a + // still-mutable ledger aborts a Release build anyway a moment later, at + // LedgerMaster::switchLCL() or LedgerHolder::set(), where the cause is no longer visible. + // Contrast loadLedgerFromFile(), which runs once at startup and can still return. + if (!built->setAccepted(closeTime, closeResolution, closeTimeCorrect)) + { + // LCOV_EXCL_START + logicError("buildLedgerImpl: accepted ledger map is invalid"); + // LCOV_EXCL_STOP + } return built; } diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 5fa1f96ba1..4311a68a51 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -113,7 +113,19 @@ InboundLedger::init(ScopedLockType& collectionLock) XRPL_ASSERT( ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()), "xrpl::InboundLedger::init : valid ledger fees"); - ledger_->setImmutable(); + // tryDB() verified both maps before setting complete_ and mtx_ has been held since, so + // nothing can have invalidated them. + if (!ledger_->setImmutable()) + { + // LCOV_EXCL_START + // Recorded before the UNREACHABLE, which continues in a Release build, and paired with + // done() for the same reason as the tryDB() failure above. + failed_ = true; + done(); + UNREACHABLE("xrpl::InboundLedger::init : map is invalid"); + return; + // LCOV_EXCL_STOP + } if (reason_ == Reason::HISTORY) return; @@ -329,12 +341,20 @@ InboundLedger::tryDB(node_store::Database& srcDB) if (haveTransactions_ && haveState_) { - JLOG(journal_.debug()) << "Had everything locally"; - complete_ = true; XRPL_ASSERT( ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()), "xrpl::InboundLedger::tryDB : valid ledger fees"); - ledger_->setImmutable(); + // Settled before complete_ is published, so a caller that reads the flag never sees a + // ledger this function has not finished with. + if (!ledger_->setImmutable()) + { + JLOG(journal_.warn()) << "Ledger " << hash_ << " found locally is invalid"; + failed_ = true; + return; + } + + JLOG(journal_.debug()) << "Had everything locally"; + complete_ = true; } } @@ -420,6 +440,40 @@ InboundLedger::done() signaled_ = true; touch(); + // Settled before the outcome below is logged or acted on, so nothing reports a ledger this + // function has since refused. + if (complete_ && !failed_ && ledger_) + { + 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. + if (!ledger_->setImmutable()) + { + JLOG(journal_.warn()) << "Acquired ledger " << hash_ << " is invalid"; + // Withdrawn as well as failed, so a caller that already read complete_ - or that checks + // it before failed_ - cannot go on treating this ledger as delivered. + complete_ = false; + failed_ = true; + } + else + { + switch (reason_) + { + case Reason::HISTORY: + app_.getInboundLedgers().onLedgerFetched(); + break; + default: + app_.getLedgerMaster().storeLedger(ledger_); + break; + } + } + } + JLOG(journal_.debug()) << "Acquire " << hash_ << (failed_ ? " fail " : " ") << ((timeouts_ == 0) ? std::string() @@ -428,24 +482,7 @@ InboundLedger::done() XRPL_ASSERT(complete_ || failed_, "xrpl::InboundLedger::done : complete or failed"); - if (complete_ && !failed_ && ledger_) - { - XRPL_ASSERT( - ledger_->header().seq < kXrpLedgerEarliestFees || ledger_->read(keylet::feeSettings()), - "xrpl::InboundLedger::done : valid ledger fees"); - ledger_->setImmutable(); - switch (reason_) - { - case Reason::HISTORY: - app_.getInboundLedgers().onLedgerFetched(); - break; - default: - app_.getLedgerMaster().storeLedger(ledger_); - break; - } - } - - // We hold the PeerSet lock, so must dispatch + // mtx_ is held, so this may only post the work rather than do it. app_.getJobQueue().addJob(JtLedgerData, "AcqDone", [self = shared_from_this()]() { if (self->complete_ && !self->failed_) { @@ -733,7 +770,8 @@ InboundLedger::trigger(std::shared_ptr const& peer, TriggerReason reason) { JLOG(journal_.debug()) << "Done:" << (complete_ ? " complete" : "") << (failed_ ? " failed " : " ") << ledger_->header().seq; - sl.unlock(); + // Called with mtx_ still held, so the flags done() writes are not written unlocked; mtx_ + // is recursive, so a caller that already holds it further up is unaffected. done(); } } diff --git a/src/xrpld/app/ledger/detail/LedgerPersistence.cpp b/src/xrpld/app/ledger/detail/LedgerPersistence.cpp index 6baf64e923..58b5be32ac 100644 --- a/src/xrpld/app/ledger/detail/LedgerPersistence.cpp +++ b/src/xrpld/app/ledger/detail/LedgerPersistence.cpp @@ -115,8 +115,15 @@ loadLedgerHelper( return ledger; } +/** + * Settle a ledger just loaded from local storage, or discard it. + * + * @param ledger The ledger to settle; cleared on failure so the caller + * cannot hand out one that is still mutable. + * @param j Where to log a refusal. + */ static void -finishLoadByIndexOrHash(std::shared_ptr const& ledger, beast::Journal j) +finishLoadByIndexOrHash(std::shared_ptr& ledger, beast::Journal j) { if (!ledger) return; @@ -124,7 +131,19 @@ finishLoadByIndexOrHash(std::shared_ptr const& ledger, beast::Journal j) XRPL_ASSERT( ledger->header().seq < kXrpLedgerEarliestFees || ledger->read(keylet::feeSettings()), "xrpl::finishLoadByIndexOrHash : valid ledger fees"); - ledger->setImmutable(); + // Loaded locally; see Ledger::setImmutable(). + if (!ledger->setImmutable()) + { + // LCOV_EXCL_START + JLOG(j.error()) << "Invalid map for ledger " << ledger->header().seq + << "; not marking it as loaded"; + UNREACHABLE("xrpl::finishLoadByIndexOrHash : map is invalid"); + // Discarded rather than left un-full: nothing gates usability on the full flag, and a + // caller that took this ledger would abort in LedgerHistory::insert() instead. + ledger.reset(); + return; + // LCOV_EXCL_STOP + } JLOG(j.trace()) << "Loaded ledger: " << to_string(ledger->header().hash); diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp index 5dd1b1907d..d2531355b5 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -50,16 +51,31 @@ TransactionAcquire::TransactionAcquire( void TransactionAcquire::done() { - // We hold a PeerSet lock and so cannot do real work here + // mtx_ is held, so this may only post real work rather than do it. if (failed_) { JLOG(journal_.debug()) << "Failed to acquire TX set " << hash_; } + else if (!map_->setImmutable()) + { + // trigger() verified the map before setting complete_ and mtx_ has been held since, and + // unlike InboundLedger nothing walks this map with the lock released, so nothing can have + // invalidated it. Untestable for that reason, and left an UNREACHABLE rather than turned + // into a recovery: there is no interleaving that reaches it. + // LCOV_EXCL_START + // Withdraw complete_ alongside the failure, or trigger() and takeNodes() - which both check + // complete_ before failed_ - keep treating this as delivered while consensus waits on a set + // giveSet() never hands over. + complete_ = false; + failed_ = true; + JLOG(journal_.debug()) << "Failed to acquire TX set " << hash_; + UNREACHABLE("xrpl::TransactionAcquire::done : map is invalid"); + // LCOV_EXCL_STOP + } else { JLOG(journal_.debug()) << "Acquired TX set " << hash_; - map_->setImmutable(); uint256 const& hash(hash_); std::shared_ptr const& map(map_); diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index d50637d98e..522d2775df 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1700,7 +1700,14 @@ ApplicationImp::startGenesisLedger() XRPL_ASSERT( next->header().seq < kXrpLedgerEarliestFees || next->read(keylet::feeSettings()), "xrpl::ApplicationImp::startGenesisLedger : valid ledger fees"); - next->setImmutable(); + // Built locally; see Ledger::setImmutable(). Failed here rather than at storeLedger() below, + // which would name the wrong site. + if (!next->setImmutable()) + { + // LCOV_EXCL_START + logicError("startGenesisLedger: genesis ledger map is invalid"); + // LCOV_EXCL_STOP + } openLedger_.emplace(next, cachedSLEs_, logs_->journal("OpenLedger")); ledgerMaster_->storeLedger(next); ledgerMaster_->switchLCL(next); @@ -1722,7 +1729,16 @@ ApplicationImp::getLastFullLedger() XRPL_ASSERT( ledger->header().seq < kXrpLedgerEarliestFees || ledger->read(keylet::feeSettings()), "xrpl::ApplicationImp::getLastFullLedger : valid ledger fees"); - ledger->setImmutable(); + // Loaded locally; see Ledger::setImmutable(). + if (!ledger->setImmutable()) + { + // LCOV_EXCL_START + JLOG(j.error()) << "Last full ledger " << seq << " has an invalid map; ignoring it"; + UNREACHABLE("xrpl::ApplicationImp::getLastFullLedger : map is invalid"); + // Must not fall through: that would mark a damaged ledger validated. + return {}; + // LCOV_EXCL_STOP + } if (getLedgerMaster().haveLedger(seq)) ledger->setValidated(); @@ -1874,7 +1890,16 @@ ApplicationImp::loadLedgerFromFile(std::string const& name) loadLedger->header().seq < kXrpLedgerEarliestFees || loadLedger->read(keylet::feeSettings()), "xrpl::ApplicationImp::loadLedgerFromFile : valid ledger fees"); - loadLedger->setAccepted(closeTime, closeTimeResolution, !closeTimeEstimated); + // Built locally; see Ledger::setImmutable(). Unlike the genesis sites the caller handles a + // failure return, so bail out rather than abort. + if (!loadLedger->setAccepted(closeTime, closeTimeResolution, !closeTimeEstimated)) + { + // LCOV_EXCL_START + JLOG(journal_.fatal()) << "Ledger from file has an invalid map"; + UNREACHABLE("xrpl::ApplicationImp::loadLedgerFromFile : map is invalid"); + return nullptr; + // LCOV_EXCL_STOP + } return loadLedger; }