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.
This commit is contained in:
Bart
2026-08-22 22:26:03 -04:00
parent 20d3a9b488
commit 3ee3f3740c
15 changed files with 698 additions and 80 deletions

View File

@@ -8,6 +8,7 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/basics/chrono.h>
#include <xrpl/beast/unit_test/suite.h>
#include <xrpl/ledger/Ledger.h>
#include <xrpl/nodestore/NodeObject.h>
#include <xrpl/protocol/HashPrefix.h>
#include <xrpl/protocol/LedgerHeader.h>
@@ -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<Ledger>
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<Ledger>(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<TestableInboundLedger>(
env.app(),
header.hash,
header.seq,
InboundLedger::Reason::GENERIC,
stopwatch(),
std::make_unique<RequestCountingPeerSet>());
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.