fix(replay): don't execute the replay target height as a checkpoint round by mraszyk · Pull Request #11203 · dfinity/ic

mraszyk · 2026-08-18T16:18:51Z

Don't execute the last replayed height as a checkpoint round

Problem

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. That flag does not only decide whether a
checkpoint is written: it also selects ExecutionRoundType::CheckpointRound, which
changes execution — it charges every canister for resource allocation and aborts all
paused executions.

Only ic-replay bounds the delivery, and it always does. So the last replayed height was
executed differently from the way the subnet executed that very same height. Since the
certification version was bumped to V29, /subnet/<subnet_id>/metrics includes
CanisterStates::total_consumed_cycles(), so the difference now changes the certification
hash and ic-replay reports

Hash mismatch! State divergence detected for outstanding shares!

Recoveries replay to the highest certification share height, which is essentially never a
summary height, so every recovery is affected.

Changes

Consensus — requires_full_state_hash is derived from the block alone, so every
replayed round is executed exactly the way the subnet executed it.

Batches — the checkpoint ic-replay needs is created by an extra batch instead. New
BatchContent::Checkpointing, handled like BatchContent::Splitting: message routing
skips induction, execution and routing and only calls
checkpoint_round_with_no_execution(), so the checkpoint holds exactly the state the
subnet computed for the last replayed height. Extra batches no longer credit a blockmaker
(blockmaker_metrics: None), since no node proposed them.

ic-replay — persisting the replayed state is now an explicit decision:

new --create-checkpoint; without it nothing is written, so a replay can be re-run as
often as needed and only committed deliberately;
the extra batch is skipped when the state it would checkpoint is already checkpointed —
the target height is a CUP height, or no block was replayed at all;
the number of batches delivered on top of the replayed blocks is reported in the replay
output (StateParams::extra_batches);
restoring from a backup says explicitly when a non-CUP target height leaves no
checkpoint, instead of silently reporting the latest CUP's state params.

ic-recovery — passes --create-checkpoint only for the step whose checkpoint is
uploaded, not for the ones that only read the state or write the registry local store; no
longer refuses to replay without a consensus pool (it warns and asks for confirmation), so
a recovery that cannot reach any node over SSH can still execute its ic-replay
subcommand on top of the local checkpoint; ValidateReplayStep subtracts the extra batch
count reported by ic-replay instead of a constant the flows had to guess.

Behaviour changes

A bare ic-replay invocation no longer creates a checkpoint; pass
--create-checkpoint.
When a checkpoint is created, the replayed height is one above the subnet's — the
checkpointing batch — which the replay output now states.

Testing

requires_full_state_hash_ignores_max_batch_height_to_deliver in batch_delivery.rs
fails without the consensus fix. The ic-replay extra-batch paths have no unit coverage;
the sr_* subnet-recovery system tests exercise them end to end, replaying to a
certification-share height.

…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>

Without a consensus pool no batches are replayed, so the on-disk checkpoint is untouched and needs no extra batch to persist it; delivering one anyway would mutate the state using the wall clock time, producing a non-deterministic state hash. This restores the pre-existing no-op behavior of a plain `ic-replay` invocation over a state directory without a consensus pool (e.g. a state-only backup snapshot). Recovery flows, on the other hand, always download the consensus pool, so there a missing pool means the state was downloaded incorrectly: make ic-recovery's replay step fail loudly in that case instead of silently replaying no blocks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

- Make re-running the replay over the same data directory idempotent: once a previous invocation has delivered the extra batch and persisted the checkpoint above the replay target height, deliver no further extra batch. Otherwise every re-run would move the checkpoint (and thereby change the state hash) one height further, and only a run over pristine data would reproduce the recovery checkpoint. - Rework the --replay-until-height consent prompt: replaying a consensus pool now creates a deterministic checkpoint via the extra batch, so warn only when restoring from a backup, where a non-CUP target height yields no persistent progress beyond the latest CUP. - When restoring from a backup reaches a target height without a checkpoint, say so explicitly instead of silently reporting the state params of the latest CUP. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The extra batch that `ic-replay` delivers to persist the state it replayed does not correspond to any block, so its round should have no effect beyond what creating a checkpoint requires. Yet it used to be an ordinary data batch, i.e. its round inducted and executed messages (heartbeats, global timers, leftover queue traffic) that the subnet itself never executed at that point, and charged all canisters for resource allocation. Introduce `BatchContent::Checkpointing`, handled like `BatchContent::Splitting` in that message routing skips induction, execution and routing altogether and only calls `checkpoint_round_with_no_execution()`, which aborts paused executions and wipes the `SystemMetadata` caches. The resulting checkpoint contains exactly the state the subnet computed for the last replayed height. Note that not charging for resource allocation in this round loses nothing: charging is duration-based, so the first charging round after the subnet resumes covers the same interval. Also stop attributing the extra batches to a test blockmaker: their `blockmaker_metrics` are `None` now, so that no blockmaker is credited for a batch that no node ever proposed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Drop `round_type_decides_whether_a_non_charging_round_charges`: it only characterizes pre-existing scheduler behaviour and passes without the fix, so `requires_full_state_hash_ignores_max_batch_height_to_deliver` in `batch_delivery.rs` is the actual regression test. Fix two comments in `player.rs`: - delivering an extra batch on every re-run makes the state hash depend on how many times the replay was run; it is a run over *pristine* data whose hash would then no longer be reproduced (the claim was inverted). - the replayed height is executed the way the subnet executed it so that the resulting certified state is *identical* to the one the subnet certified, not merely comparable to its certification shares. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

zeropath-ai · 2026-08-19T13:31:24Z

✅ No security or compliance issues detected. Reviewed everything up to a6c4a15.

Security Overview

🔎 Scanned files: 15 changed file(s)
🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=04a14943-d719-41fe-ab54-f0cbe9ed2e78&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/consensus/src/consensus/batch_delivery.rs Derive requires_full_state_hash from block payload only; adjust logic and related tests ► rs/consensus/src/consensus/batch_delivery.rs Use Some(blockmaker_metrics) in blockmaker metrics setup ► rs/messaging/src/message_routing.rs Handle optional blockmaker_metrics and record metrics only when present ► rs/messaging/src/state_machine.rs Add handling for BatchContent::Checkpointing in state machine ► rs/messaging/src/state_machine/tests.rs Update tests to account for Checkpointing and optional metrics ► rs/recovery/src/app_subnet_recovery.rs Adjust replay validation step height to account for extra batch ► rs/recovery/src/nns_recovery_failover_nodes.rs Adjust replay validation step height to account for extra batch ► rs/recovery/src/nns_recovery_same_nodes.rs Update replay validation step to include extra batch behavior ► rs/recovery/src/steps.rs Fail when no consensus pool exists; ensure proper checkpoint handling ► rs/replay/src/lib.rs Commentary updates about extra batch and CUP-related checkpoint creation ► rs/replay/src/player.rs Introduce target_height handling; ensure extra batch yields checkpointing batch when appropriate ► rs/types/types/src/batch.rs Batch supports Optional blockmaker_metrics and new Checkpointing content; update related logic ► rs/test_utilities/types/src/batch/batch_builder.rs Default builder now uses Some(BlockmakerMetrics) for batch creation

pierugo-dfinity

In ic-replay, instead of predicting the operator's thought process and behave as what we think makes more sense today (i.e. execute an extra checkpointing batch, but actually not if there's no consensus pool, and actually not if we replayed beforehand), what about leaving the decision of adding an extra checkpointing batch as a CLI argument?
By default, we wouldn't include an extra batch and thus not create a checkpoint. The operator could safely replay as many times as they want without "committing" to/persisting anything. When they want to commit, they would pass the flag.
I think it would simplify the ic-replay implementation

During recoveries, I guess we can always enable that flag, even when no consensus pool was downloaded (maybe we weren't able to SSH in? See next comment), still displaying a confirmation prompt maybe.

pierugo-dfinity · 2026-08-25T08:43:16Z

+                        BatchContent::Checkpointing
                    };
                    extra_batch.batch_number = message_routing.expected_batch_height();
                    extra_batch.time += Duration::from_nanos(1);


I think this made sense before because when delivering a "proper" extra batch, it should indeed increase its batch time. But now, when delivering the a Checkpointing batch, this time is artificially increased, which mutates the system metadata, even though the subnet never did so. I think we could keep it the same as the previous batch and we would ignore it in the DSM implementation. Not sure if having a non-increasing batch time could have other undesirable consequences though.

even though the subnet never did so

the state is mutated in other ways, too, such as by charging canisters and aborting their DTS executions; so I'm not sure it's worthwhile to special case time here

pierugo-dfinity · 2026-08-25T08:43:39Z

                println!("Target height {height} reached.");
-                return Ok(self.get_latest_state_params(None, invalid_artifacts));
+                let state_params = self.get_latest_state_params(None, invalid_artifacts);
+                if state_params.height < last_batch_height {


When do we expect this condition to be true?

added a comment on that

Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>

- `deliver_extra_batch`: key the early return on the absence of a consensus pool instead of on `target_height` being `None`. - `restore_from_backup`: document when the reported state height can be below the last replayed height. - `FakeBatchProcessorImpl`: `unimplemented!` on `BatchContent::Checkpointing`, which only `ic-replay` delivers and always to the real batch processor. - `requires_full_state_hash_ignores_max_batch_height_to_deliver`: build the dependencies via `DependenciesBuilder::new`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…et-height-round-type # Conflicts: # rs/consensus/src/consensus/batch_delivery.rs # rs/determinism_test/src/lib.rs # rs/messaging/src/message_routing/tests.rs # rs/messaging/src/state_machine/tests.rs # rs/replay/src/player.rs # rs/test_utilities/types/src/batch/batch_builder.rs

…_step Keep the recovery flows free of the `ic-replay` implementation detail that every invocation delivers one extra batch at the end to create the checkpoint: their `extra_batches` arguments count only the batches the flow itself adds, and `get_validate_replay_step` adds the checkpointing one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`ReplayStep` refused to run without a consensus pool, on the grounds that a missing pool means the state was downloaded incorrectly. But the pool is downloaded over SSH, so a recovery that cannot reach any node has no pool and still wants to execute the extra batch of its `ic-replay` subcommand on top of the latest local checkpoint. Warn what will happen instead and let the operator confirm, and have `ic-replay` report the pool-less skip of the checkpointing batch so the no-op is visible in its output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Replaying up to a CUP height already creates the checkpoint, as the batch of a summary block requires a full state hash. Delivering the extra batch on top of it would move the checkpoint (and thereby the reported state hash) one height further for no reason, so skip it whenever the state it would checkpoint is already checkpointed. The number of extra batches is therefore no longer a constant the recovery flows can assume: `ic-replay` reports it in its output and `ValidateReplayStep` subtracts the reported number, which also fixes the count in the upgrade case, where the ingress batch is followed by a separate checkpointing batch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`ic-replay` decided by itself whether the replayed state should be persisted: it delivered an extra checkpointing batch, but not without a consensus pool, and not if a previous run had already delivered one. Replace that guesswork with `--create-checkpoint`. Without the flag nothing is persisted, so a replay can be re-run as often as needed; with it the state is committed. What remains is not a policy decision but a no-op check: the batch is skipped if the state it would checkpoint is already checkpointed, i.e. the last replayed block is a summary block or no block was replayed at all. The latter subsumes the former pool-less guard. Recoveries always pass the flag, except for the steps that only read the state (the recovery CUP) or only write the registry local store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Prevents bounded replay rounds from incorrectly executing as checkpoint rounds and adds explicit replay checkpoint creation.

Changes:

Derives checkpoint execution solely from block content.
Adds checkpoint-only batches and optional blockmaker metrics.
Adds --create-checkpoint and reports extra replay batches to recovery flows.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`rs/types/types/src/batch.rs`	Adds checkpoint-only batches and optional blockmaker metrics.
`rs/test_utilities/types/src/batch/batch_builder.rs`	Updates test batch construction.
`rs/state_machine_tests/src/lib.rs`	Wraps blockmaker metrics in `Some`.
`rs/replay/src/player.rs`	Implements explicit checkpoint batches and extra-batch reporting.
`rs/replay/src/lib.rs`	Propagates checkpoint configuration and replay results.
`rs/replay/src/cmd.rs`	Adds `--create-checkpoint`.
`rs/recovery/src/steps.rs`	Updates replay execution and validation.
`rs/recovery/src/replay_helper.rs`	Passes checkpoint configuration to replay.
`rs/recovery/src/nns_recovery_same_nodes.rs`	Uses reported extra-batch counts.
`rs/recovery/src/nns_recovery_failover_nodes.rs`	Updates replay validation construction.
`rs/recovery/src/lib.rs`	Simplifies validation-step API.
`rs/recovery/src/app_subnet_recovery.rs`	Updates replay validation construction.
`rs/messaging/src/state_machine/tests.rs`	Updates batch variants and metrics.
`rs/messaging/src/state_machine.rs`	Handles checkpoint-only rounds.
`rs/messaging/src/message_routing/tests.rs`	Updates test batches for optional metrics.
`rs/messaging/src/message_routing.rs`	Skips blockmaker accounting for synthetic batches.
`rs/determinism_test/src/lib.rs`	Updates deterministic test batches.
`rs/consensus/src/consensus/batch_delivery.rs`	Stops forcing bounded deliveries into checkpoint rounds.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+            if !self.skip_prompts
+                && !consent_given(&self.logger, "Continue without a consensus pool?")


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

`Player::new` panicked when neither a consensus pool nor `--replica-version` was available, so the pool-less replay the recovery now allows could not actually run: `replay_helper::replay` always passes `replica_version: None`, and no recovery flow has the right version to pass (`upgrade_version` is the version to upgrade *to*, applied after the replay via the CUP, whereas the extra batch executes under the version the subnet is currently on). Fall back to the subnet's replica version from the local registry, looked up the same way consensus stamps a block's version. This matches the pool-less `deliver_extra_batch`, which already takes its registry version from the latest local registry version. `--replica-version` stays an override, and the panic remains only as a last resort. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

mraszyk added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label Aug 18, 2026

github-actions Bot added the fix label Aug 18, 2026

mraszyk closed this Aug 19, 2026

mraszyk reopened this Aug 19, 2026

mraszyk and others added 4 commits August 19, 2026 08:46

mraszyk force-pushed the mraszyk/replay-target-height-round-type branch from 860b318 to a6c4a15 Compare August 19, 2026 11:26

mraszyk marked this pull request as ready for review August 19, 2026 13:30

mraszyk requested a review from a team as a code owner August 19, 2026 13:30

github-actions Bot added the @core-protocol label Aug 19, 2026

pierugo-dfinity reviewed Aug 25, 2026

View reviewed changes

mraszyk marked this pull request as draft August 26, 2026 15:13

mraszyk and others added 7 commits August 27, 2026 13:31

Update rs/replay/src/player.rs

286ba3c

Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>

mraszyk requested a balanced review from Copilot August 27, 2026 13:01

Copilot started reviewing on behalf of mraszyk August 27, 2026 13:01 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread rs/replay/src/cmd.rs

Comment thread rs/recovery/src/steps.rs

Comment on lines +474 to +475

if !self.skip_prompts

&& !consent_given(&self.logger, "Continue without a consensus pool?")

mraszyk and others added 2 commits August 27, 2026 15:13

Potential fix for pull request finding

83f999e

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Navigation Menu

Uh oh!

fix(replay): don't execute the replay target height as a checkpoint round - #11203

fix(replay): don't execute the replay target height as a checkpoint round#11203
mraszyk wants to merge 14 commits into
masterfrom
mraszyk/replay-target-height-round-type

mraszyk commented Aug 18, 2026 •

edited

Loading

Uh oh!

zeropath-ai Bot commented Aug 19, 2026

Uh oh!

pierugo-dfinity left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierugo-dfinity Aug 25, 2026

Uh oh!

mraszyk Aug 27, 2026

Uh oh!

pierugo-dfinity Aug 25, 2026

Uh oh!

mraszyk Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if !self.skip_prompts
		&& !consent_given(&self.logger, "Continue without a consensus pool?")

Uh oh!

Conversation

mraszyk commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Don't execute the last replayed height as a checkpoint round

Problem

Changes

Behaviour changes

Testing

Uh oh!

zeropath-ai Bot commented Aug 19, 2026

Uh oh!

pierugo-dfinity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierugo-dfinity Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

mraszyk Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

pierugo-dfinity Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

mraszyk Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mraszyk commented Aug 18, 2026 •

edited

Loading