From 3ee3f3740ce0c7cc6b3def618e9bfbcbc542d34e Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 22 Aug 2026 22:26:03 -0400 Subject: [PATCH] fix: Refuse to make an invalid SHAMap or Ledger immutable SHAMap::setImmutable() now returns [[nodiscard]] bool and refuses a map already proven impossible. Every state change goes through trySetState(), the only writer of state_ past construction, so the order between the states is stated once: its compare-exchange can't leave Invalid however it interleaves with another thread's, and setInvalid() stores through the same funnel rather than behind its back. Invalid is stored unconditionally there, since only the map itself reaches that verdict and a walk that reaches it has to win against a thread settling the map; refusing to overwrite Immutable would leave a map proven impossible reporting itself sound, which is what nothing downstream could recover from. Ledger::setImmutable()/setAccepted() do the same one level up, checking mapsValid() before touching anything and settling both maps independently so neither is left mid-sync because the other refused. The map hashes are read before the maps are settled, since getHash() can unshare a dirty tree, but written to the header only once both maps have made it. A walk that invalidates a map in between therefore leaves the header describing what the ledger was built from rather than a map that has since been abandoned. Every call site now branches on the result. The two genesis paths and buildLedgerImpl() call logicError(), since consensus can't tolerate an invalid ledger; the load paths return early instead; InboundLedger and TransactionAcquire withdraw complete_ alongside the failure, since for them a refusal is an outcome a peer can produce. A test helper that cannot reach BEAST_EXPECT throws instead, so a refusal cannot hand a broken ledger to the assertions below. --- include/xrpl/ledger/Ledger.h | 82 +++++++-- include/xrpl/shamap/SHAMap.h | 126 ++++++++++++-- src/libxrpl/ledger/Ledger.cpp | 81 +++++++-- src/libxrpl/shamap/SHAMap.cpp | 15 +- src/test/app/InboundLedger_test.cpp | 100 +++++++++++ src/test/app/LedgerHistory_test.cpp | 17 +- src/test/app/RCLValidations_test.cpp | 2 +- src/test/rpc/BookChanges_test.cpp | 10 +- src/tests/libxrpl/helpers/TxTest.cpp | 5 +- src/tests/libxrpl/shamap/SHAMapSync.cpp | 164 +++++++++++++++++- src/xrpld/app/ledger/detail/BuildLedger.cpp | 18 +- src/xrpld/app/ledger/detail/InboundLedger.cpp | 84 ++++++--- .../app/ledger/detail/LedgerPersistence.cpp | 23 ++- .../app/ledger/detail/TransactionAcquire.cpp | 20 ++- src/xrpld/app/main/Application.cpp | 31 +++- 15 files changed, 698 insertions(+), 80 deletions(-) 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; }