refactor(state_manager): stop checking the legacy canister log index on checkpoint load - #11013
Open
mraszyk wants to merge 1 commit into
Open
refactor(state_manager): stop checking the legacy canister log index on checkpoint load#11013mraszyk wants to merge 1 commit into
mraszyk wants to merge 1 commit into
Conversation
…on checkpoint load `load_canister_state` compared `log_memory_store.next_idx()` against the `canister_log.next_idx()` decoded from `next_canister_log_record_idx`, and reported a `state_manager_checkpoint_soft_invariant_broken` critical error on mismatch. That check is what makes dropping the legacy `canister_log` fields from `CanisterStateBits` a downgrade-breaking change: a replica that no longer writes `next_canister_log_record_idx` produces checkpoints on which every older replica reports one critical error per canister that has ever logged (observed as 62 errors in `upgrade_downgrade_nns_subnet_test`). Removing the check here first means that once this version is on mainnet, the legacy fields can be dropped from the checkpoint without tripping the downgrade path. The log memory store is authoritative for `fetch_canister_logs` and the delta log index is seeded from `log_memory_store.next_idx()`, so nothing on the read path depends on the legacy index; after this change it is only written, in `tip.rs`, for backward compatibility. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes a soft-invariant check during checkpoint load that compared the legacy canister_log.next_idx() (decoded from next_canister_log_record_idx) against the authoritative log_memory_store.next_idx(). This helps make it safe to later drop the legacy canister log fields from checkpoints without causing older replicas to emit critical errors when loading new checkpoints.
Changes:
- Removed the
log_memory_store.next_idx()vscanister_log.next_idx()mismatch check inload_canister_state. - Eliminated the associated
state_manager_checkpoint_soft_invariant_brokencritical-error emission for this legacy-index mismatch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✅ No security or compliance issues detected. Reviewed everything up to 19bd2ce. Security Overview
Detected Code Changes
|
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.
load_canister_statecomparedlog_memory_store.next_idx()against thecanister_log.next_idx()decoded fromnext_canister_log_record_idx, and reported astate_manager_checkpoint_soft_invariant_brokencritical error on mismatch.That check is what makes dropping the legacy
canister_logfields fromCanisterStateBitsa downgrade-breaking change: a replica that no longer writesnext_canister_log_record_idxproduces checkpoints on which every older replica reports one critical error per canister that has ever logged.Removing the check here first means that once this version is on mainnet, the legacy fields can be dropped from the checkpoint without tripping the downgrade path.
The log memory store is authoritative for
fetch_canister_logsand the delta log index is seeded fromlog_memory_store.next_idx(), so nothing on the read path depends on the legacy index; after this change it is only written, intip.rs, for backward compatibility.