fix(ledger): archive after replying instead of before - #11149
Draft
mbjorkqvist wants to merge 7 commits into
Draft
fix(ledger): archive after replying instead of before#11149mbjorkqvist wants to merge 7 commits into
mbjorkqvist wants to merge 7 commits into
Conversation
The ICRC1 ledger applies a transfer synchronously — balances, total supply,
the block log and the certified data — and only then awaits `archive_blocks`.
That await is a commit point, so a trap in any later continuation of the call
cannot roll the transfer back, but it does turn the reply into a reject. The
caller is left unable to tell that reject apart from one where nothing
happened; the ck minters read it as "the mint did not happen" and retry, which
mints the same deposit twice (reported as ICPBB-369).
This adds a regression test for the invariant that is violated: a rejected
`icrc1_transfer` must not leave a committed block behind. It fails at this
commit:
committed_while_outstanding=true chain_length=9->10
balance=9000000->10000000 archives=[]
the ledger rejected the transfer with `IC0534: Canister cannot grow memory
by 262144 bytes due to its reserved cycles limit. The current limit
(100_000_000) would be exceeded by 1_462_346_208`, but had already
committed it
The trap is produced the way it can be produced on mainnet, by putting the
subnet under storage pressure while the ledger is suspended in the archive
await, so that the memory its archive continuation needs is refused with
IC0534. The reporter's proof of concept reached the mainnet 750 GiB
reservation threshold with 925 GiB of stable memory and hand-set the ledger's
reserved balance through a patched `StateMachine` API. Neither is necessary:
the thresholds are scaled down through the test's `HypervisorConfig` and the
ledger's reservation limit is an ordinary canister setting. The filler only
grows logical stable memory, which it never writes to, so the test uses no
meaningful host storage and runs in ~2s.
The test asserts its own preconditions — that the transfer was committed and
that the archive continuation was killed — so that it cannot pass vacuously
once archiving no longer fails the call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ledgers applied a transaction synchronously and then awaited `archive_blocks` before replying. That await is a commit point, so a trap in any continuation of the archiving — the replica refusing a memory growth under storage pressure, for instance — could not roll the transaction back, but it did turn the reply into a reject. Callers cannot tell such a reject apart from one where nothing happened; the ck minters read it as a failed mint and retry, minting the same deposit twice (ICPBB-369). Archiving is now spawned rather than awaited, so the reply is produced in the same message that commits the transaction and archiving continues on its own chain of messages, where a failure can no longer contradict a reply that was already sent. The blocks stay in the ledger until the next attempt, as they already did for archiving errors that did not trap. This covers `icrc1_transfer`, `icrc2_transfer_from`, `icrc2_approve`, `icrc152_mint` and `icrc152_burn` on the ICRC1 ledger and `send`, `icrc1_send`, `icrc2_approve` and `approve` on the ICP ledger. The ICP ledger already warned about this at one of the call sites — "Don't put anything that could ever trap after this call [...] the payment would appear to fail, but would actually succeed on chain" — directly above the awaited archiving that could do exactly that. This changes an observable contract: archiving is no longer complete when the call that triggered it replies. Tests asserting on archives right afterwards now wait for it through `await_archiving` in the shared state machine helpers, which returns immediately when nothing is in flight and so costs nothing on the calls that do not archive. Two tests were built on the old behaviour rather than incidentally affected by it. `test_archiving_in_chunks_returns_disjoint_block_range_locations` and `test_archiving_lots_of_blocks_after_enabling_archiving` ticked "until the transfer completes, meaning the archiving also completes", asserting that the transfer was still outstanding while the ledger archived. They now tick until the subnet is idle and keep asserting what they were about: that the ledger never reports the same block in two places while archiving proceeds. Because they run against both ledgers, the ICP ledger had to be changed together with the ICRC1 one. `archiving_recovers_after_a_trapped_attempt` covers the risk this introduces: a failure to archive is now invisible to callers, so a stuck archiving lock would be easy to miss. The lock is taken before the first await, and the test confirms it is released when the trapped task is canceled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds `test_transfer_when_subnet_is_out_of_memory`, run against both ledgers.
It fills the subnet, sends a transaction, and checks that the ledger recorded
nothing: no block, no balance change, no change in total supply. Both ledgers
trap the same way, while applying the transaction:
IC0503: Canister called `ic0.trap` with message: 'Panicked at 'Failed to
grow memory from 0 pages to 1 pages (delta = 1 pages).',
ic-stable-structures/src/lib.rs:182:9'
The stable memory grow returns -1, the block log panics, and the whole message
is discarded. This is the counterpart to the archiving atomicity test: a
failure *before* the ledger's first await is atomic, whereas one after it is
not, which is why archiving must not be awaited before replying. The test then
frees the memory and replays the same transaction, which succeeds — the ledger
is left usable and it really was the subnet being full.
Getting there is less obvious than it sounds. A full subnet is not by itself
enough to make transactions fail: the ledger allocates in large chunks and
serves most transactions out of memory it already holds. A first attempt filled
the subnet and then transferred for 900s without a single failure. Measured on
a fresh ICRC1 ledger:
after install: 4_416_971 bytes
after 1st mint: 29_648_331 bytes (+25 MB)
mints 2 to 9: unchanged
So the test uses a freshly installed ledger with no initial balances, whose
first transaction is also its first allocation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ledger_archiving_failures` only counted archiving that failed gracefully. A trap discards everything the failing message did, including any attempt to record it, so a ledger whose archiving trapped every time reported zero failures forever — the most misleading reading possible, and more so now that the failure no longer reaches the caller either. Destructors are the exception: they run while the task is being canceled, in the cleanup callback, whose state changes are kept. That is already how the archiving lock gets released, and `ic_cdk::futures::is_recovering_from_trap` distinguishes cancellation from an ordinary drop, so the archiving guard can count the failure on its way out. This is a signal of last resort. Blocks accumulating in the ledger while the archived block count stays flat remains the more robust one, and does not depend on the ledger surviving the trap at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The post-commit window the archiving atomicity test exercises is narrower than
"any archiving round". It needs the ledger to *grow* its memory, and archiving
only does that when it creates an archive canister: the `install_code` payload
carries the archive Wasm, which is where the reproduction traps, right after
`create_canister` returns.
A routine round hands the archive its next chunk of blocks using memory the
allocator already holds from the previous round. With production-shaped archive
options — 1000 blocks per round, the archive already holding 1000 — the ledger
does not grow at all, and the round completes under the same storage pressure
that breaks the creation round:
archive creation round: blocks 0..999, ledger memory 30041547->31352267
routine round: blocks 0..1999, ledger memory 31352267->31352267
The test asserts that, so that it fails if routine archiving starts allocating
too and the window turns out to be wider.
Note this is about the ledger's memory. The archive canister running out is
harmless: it answers with a reject, which the ledger handles rather than
trapping on. On mainnet the window is not a one-off either — a new archive node
is created whenever the current one fills up, which is the rollover the
reported proof of concept targeted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ledger_archiving_failures` counting trapped attempts was only asserted for the ICRC1 ledger. The counting itself lives in the shared archiving guard, but the counter does not: `increment_archiving_failure_metric` is a per-ledger trait impl over a per-crate `ARCHIVING_FAILURES` cell, so the ICP ledger's side of it was untested. Adds `test_trapped_archiving_is_counted` to the shared suite and runs it against both ledgers. It drives the ledger into a trapped archiving round the way the reported proof of concept does — pushing the subnet over its storage reservation threshold while the ledger is suspended in archiving — then checks that the transfer still succeeded, that no archive was created, and that the failure was counted. The last two assertions are both needed: a graceful archiving failure would also leave the counter at 1 with no archive, so without the trap the test could pass while measuring the wrong path. Verified by disabling the destructor, at which point both ledgers report 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A transfer is applied synchronously and then archived before the ledger replies. That await is a commit point: a trap while archiving cannot roll the transfer back, but it does turn the reply into a reject, and a caller cannot tell such a reject apart from one where nothing happened. The ck minters read it as a failed mint and retry, minting the same deposit twice.
Archiving now runs after the reply, on its own chain of messages, so a failure while archiving can no longer contradict a transfer that was already committed. The blocks stay in the ledger until the next attempt, as they already did for archiving errors that did not trap.
This changes an observable contract: archiving is no longer complete when the call that triggered it replies. Two existing tests were built on the old behaviour and now key off the subnet going idle instead; the rest wait for archiving through a shared helper. Both ledgers change together, because those tests are shared and cannot express two contracts.
A round that fails is now invisible to callers, so it is recorded rather than silent: the archiving failure metric counts rounds that trap, not just those that fail gracefully. Block accumulation remains the signal worth alerting on, since it does not depend on the ledger surviving the trap.