Commit Graph

3 Commits

Author SHA1 Message Date
Bart
f588200c05 fix: Refuse to make an invalid SHAMap or Ledger immutable
An immutable map is treated as persistable, so SHAMap::setImmutable() returns [[nodiscard]] bool and
refuses a map that has been proven impossible. Every state change goes through trySetState(),
whose compare-exchange refuses to leave Invalid however it interleaves with another thread's, so
setSynching() and clearSynching() cannot launder an abandoned map back into a state that passes
isValid() either. clearSynching() reports its refusal and carries on, since peer data produces that
verdict and an abort there would be one a peer could ask for, while setSynching() keeps an UNREACHABLE
and says why it is out of reach: it only ever runs on a map that has just been constructed. The
snapshot constructor carries Invalid over rather than promoting it, since a snapshot shares the
source's root, and reads the source's state once into a local so a concurrent walk cannot have it
report two different things.

Ledger::setImmutable() and Ledger::setAccepted() do the same one level up. Both return [[nodiscard]]
bool, and setImmutable() asks mapsValid() before writing anything, so a refusal leaves the header
exactly as it was rather than relabelled on its way to failing. Past that check it settles both maps
through setMapsImmutable(), which is deliberately not short-circuited: each map becomes Immutable or
stays Invalid on its own, and neither is left mid-sync because the other refused. immutable_ is set
last, so isImmutable() never reports a ledger whose maps are not both immutable. The guard is
best-effort by nature, which the comments say: setInvalid() outranks Immutable, so a walk that reaches
the verdict after both maps are settled narrows the window rather than closing it.

All fourteen call sites branch on the result, and the rule that a ledger built or loaded locally
cannot have an invalid map is stated once, in Ledger::setImmutable()'s docstring, with each such site
pointing there. The tiers differ by what the caller can do: the two genesis paths and buildLedgerImpl()
call logicError(), the last of those explaining why it takes the harsher tier on the consensus hot
path; loadLedgerFromFile(), getLastFullLedger() and finishLoadByIndexOrHash() return instead, and the
last of those clears the pointer, since nothing gates usability on the full flag and a caller that
took the ledger would abort further on. InboundLedger and TransactionAcquire recover, since for them a
refusal is an outcome a peer can produce: each withdraws complete_ alongside the failure, so a guard
that reads that flag before failed_ cannot go on treating the result as delivered.

Six cases cover it. Five are gtest: an invalid map refuses repeatedly and is not synching either; a
refusal leaves both header map hashes and the ledger hash untouched; an invalid transaction map and an
invalid state map each block the enclosing ledger, covering both operands of the test in
setImmutable(); and a snapshot of an invalid map is invalid and unpersistable in both flavors. The
boost case drives InboundLedger::done() with a map invalidated after the have-flags were set, and
checks the acquisition reports neither complete nor delivered and remembers the hash as a failure.
2026-08-23 16:04:32 -04:00
Bart
dd24844f6e fix: Signal an InboundLedger that fails on local data
tryDB() decides an acquisition can never succeed on two paths: a header whose hash or sequence does
not match what was asked for, and a zero account hash. Both set failed_, and init() and trigger() now
call done() on that path, so the object signals whatever is waiting on it and logFailure() records
the hash in recentFailures_. That is what stops the next round asking for the same doomed ledger.
checkLocal() is the third route into tryDB() and does the same, so all three agree.

testLocalFailureSignalsDone drives both of the entry points that were silent. The first goes through
InboundLedgers::acquire(), the only caller of init(); the second constructs an acquisition with no
header and triggers it, which is the trigger() route. Each uses a hash of its own, since
recentFailures_ is shared and keyed by hash, and each asserts on recentFailures_ rather than on the
flags, since being remembered as a failure is the caller-visible consequence of having signalled.
2026-08-23 16:04:14 -04:00
Bart
aa75e50a94 test: Add a reusable peer harness for acquisition tests
src/tests/libxrpl/shamap/DeepChain.h builds the node chains both acquisition suites need. It offers
two shapes: inner nodes running all the way to SHAMap::kLeafDepth, a depth only a leaf may occupy,
so feeding one leaves the map provably impossible; and toLeaf(), which stops at a real transaction
leaf and so completes an acquisition. fill() and addOffendingNode() divide a chain at its deepest
node, so a caller populates a map and then offers that one node itself, and every entry point takes
a seed, since caches and fetch packs are keyed by hash and two chains must not resolve each other's
nodes. It sits under src/tests/libxrpl because building a chain needs nothing outside libxrpl, while
the peer harness that wraps it for these suites is xrpld-only.

src/test/app/AcquireTestHelpers.h holds the fakes both acquisition suites need: ChargeRecordingPeer,
which records what it was charged and is otherwise inert; RequestCountingPeerSet, which selects peers
the way the real one does and whose counters are safe to read while the retry timer runs, so a case
can see which peers an acquisition chose and what limit it asked for; packetFor(), which wraps a
DeepChain's nodes as a TMLedgerData reply so a case can go through the real gotData() dispatch rather
than reproducing it; waitFor(), for the paths that finish on another thread; and tallyIs(), for
reading a verdict as counts.

Both classes carry the seams the suites reach through, stated where the reader meets them.
TransactionAcquire and InboundLedger drop final and gain a defaulted retryInterval constructor
parameter beside a kRetryInterval constant, so a case can run a whole timeout chain in a fraction of
a second; TransactionAcquire::map_ is protected, since nothing else publishes the map's state; and
InboundLedger keeps TriggerReason, trigger() and done() protected, so a case can drive an acquisition
the way the timer chain does without routing through the JobQueue. No production call site
passes one, since TimeoutCounter already takes the interval, and its onTimer() hook documents that
the lock it is handed is this object's own recursive mutex rather than anything belonging to a
PeerSet.

Ten cases pin existing behavior across the two suites: a set completes, asks for nothing further and
reaches InboundTransactions; two peers each supplying a different piece are both accepted without
penalty; a root that does not hash-match leaves the acquisition able to try another peer and the map
untouched; a repeated root and a repeated non-root node are each free, while a reply whose node data
cannot be deserialized is charged, which is what gives the free-of-charge assertions their teeth;
init() asks only the peers claiming to have the set; a ledger that resolves locally completes on the
spot and is handed to LedgerMaster; and each suite's retry timer re-asks and then gives up. Each
suite shares one jtx::Env, which costs far more to build than any case, and hands out a fresh chain
seed per case so nothing one case fed can resolve another's nodes.

ConsensusTransSetSF::gotNode() also names its parse threshold kMinTxNodeBytesToParse, which the
helper asserts a chain's leaf payload stays below so a fabricated leaf is never parsed and
resubmitted as a transaction. It is sizeof(std::uint32_t) + kMinShaMapItemBytes + 1, or 17, and the
docstring says it is the long-standing threshold rather than a derived bound: the smallest
hash-prefixed leaf is 16 bytes and nothing that size is a signed transaction either.
2026-08-23 16:02:26 -04:00