fix(replay): don't execute the replay target height as a checkpoint round - #11203
Draft
mraszyk wants to merge 1 commit into
Draft
fix(replay): don't execute the replay target height as a checkpoint round#11203mraszyk wants to merge 1 commit into
mraszyk wants to merge 1 commit into
Conversation
…ound
`deliver_batches()` derived `requires_full_state_hash` partly from its
`max_batch_height_to_deliver` argument, so the last batch of a bounded
delivery was always flagged as requiring a full state hash:
let persist_batch = Some(height) == max_batch_height_to_deliver;
let requires_full_state_hash = block.payload.is_summary() || persist_batch;
That flag does not only decide whether a checkpoint is written: it also
selects `ExecutionRoundType::CheckpointRound`, which *changes execution*.
A checkpoint round charges every canister for resource allocation and
usage, bypassing the `CHARGE_INTERVAL_ROUNDS` gate, and aborts all paused
executions instead of only those above a limit.
Only `ic-replay` passes `Some(..)` here, and it always does -- even
without `--replay-until-height`, it passes `Some(finalized_height)`. So
the last replayed height was executed differently from the way the subnet
executed that very same height, and the resulting state differed in the
canisters' cycle balances and consumed cycles.
That difference used to be invisible to the certified state. Since the
current certification version was bumped to `V29`, `/subnet/<subnet_id>/metrics`
includes `CanisterStates::total_consumed_cycles()`, so it now changes the
certification hash, and `ic-replay` reports
Hash mismatch! State divergence detected for outstanding shares!
against the subnet's certification shares at that height, refusing to
proceed without manual inspection. Subnet recoveries replay to the highest
certification share height, which is essentially never a summary height,
so every recovery is affected.
Derive `requires_full_state_hash` from the block alone, and have
`ic-replay` create the checkpoint it needs by always delivering an extra
batch at the end, one height above the last replayed block. The replayed
heights are then executed exactly as the subnet executed them, and the
checkpoint round happens at a height no node ever certified.
The replayed height is therefore one above the subnet's; account for it in
`ValidateReplayStep`, which already models this via `extra_batches`.
Both added tests fail without the corresponding change.
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
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.
deliver_batches()derivedrequires_full_state_hashpartly from itsmax_batch_height_to_deliverargument, so the last batch of a bounded delivery was always flagged as requiring a full state hash:That flag does not only decide whether a checkpoint is written: it also selects
ExecutionRoundType::CheckpointRound, which changes execution. A checkpoint round charges every canister for resource allocation and usage, bypassing theCHARGE_INTERVAL_ROUNDSgate, and aborts all paused executions instead of only those above a limit.Only
ic-replaypassesSome(..)here, and it always does -- even without--replay-until-height, it passesSome(finalized_height). So the last replayed height was executed differently from the way the subnet executed that very same height, and the resulting state differed in the canisters' cycle balances and consumed cycles.That difference used to be invisible to the certified state. If the current certification version was bumped to
V29,/subnet/<subnet_id>/metricsincludesCanisterStates::total_consumed_cycles(), so it changes the certification hash, andic-replayreportsagainst the subnet's certification shares at that height, refusing to proceed without manual inspection. Subnet recoveries replay to the highest certification share height, which is essentially never a summary height, so every recovery is affected.
Derive
requires_full_state_hashfrom the block alone, and haveic-replaycreate the checkpoint it needs by always delivering an extra batch at the end, one height above the last replayed block. The replayed heights are then executed exactly as the subnet executed them, and the checkpoint round happens at a height no node ever certified.The replayed height is therefore one above the subnet's; account for it in
ValidateReplayStep, which already models this viaextra_batches.Both added tests fail without the corresponding change.