Conversation
…hanges The cycles prepaid for a response execution are recorded in the callback under the cost schedule in effect when the call was performed, and settled when the response is executed against the cycles that execution requires, derived under the cost schedule in effect by then. The two amounts are only comparable if both were derived under the same cost schedule: an amount that is free under the free cost schedule has a zero real part but a non-zero nominal one, so settling across a switch either forfeits the real part of the prepayment or credits the canister with real cycles that were never withdrawn from its balance. Nothing enforced that today. Two paths could break it: - `prepare_canister_migration` validated that the source and the destination subnet have the same type and the same size, but not that they use the same cost schedule. A rental subnet is an application subnet on the free cost schedule, so a range could be migrated between it and a regular application subnet. That matters because a migrated canister carries its callbacks, and hence the prepayments recorded in them, to the destination subnet; - a subnet is assigned its cost schedule when it is created and no payload exposes a field to change it afterwards, so the cost schedule of a live subnet is immutable only by the absence of a way to mutate it, not by a check. Reject a migration between subnets whose cost schedules differ, and add the `check_subnet_cost_schedule_immutability` registry invariant, which rejects any mutation that would change the cost schedule of an existing subnet whichever path that mutation takes. Newly created subnets are free to pick either cost schedule. Both compare the cost schedule with `Unspecified` normalized to `Normal`, as a subnet record that predates the field leaves it unset and the replica reads that as `Normal`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The critical merge_subnets cost-schedule validation gap remains unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enforces stable cycles cost schedules for canisters across registry mutations and migrations.
Changes:
- Rejects migrations across differing normalized cost schedules.
- Adds schedule normalization and immutability checks.
- Adds regression tests.
File summaries
| File | Review |
|---|---|
rs/registry/canister/src/mutations/prepare_canister_migration.rs |
Critical: merge_subnets can still move canisters across differing schedules; add validation and regression coverage. |
rs/registry/canister/src/mutations/common.rs |
Adds cost-schedule normalization. |
rs/registry/canister/src/invariants/subnet/tests.rs |
Tests schedule immutability. |
rs/registry/canister/src/invariants/subnet.rs |
Implements the immutability invariant. |
rs/registry/canister/src/invariants/checks.rs |
Integrates the invariant into registry checks. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address the review of the previous commit: enforcing the invariant at the mutations that were known to move canisters between subnets keeps missing paths. `merge_subnets` moves every canister ID range of the source subnet to the destination subnet without touching either subnet record, and `do_migrate_canisters` reassigns individual canisters without validating anything at all, not even that the target subnet exists. Guard the property itself instead of enumerating its mutations. A canister is charged under the cost schedule of the subnet that the routing table assigns its canister ID to, so `check_canister_cost_schedule_invariants` compares, for every canister ID hosted both before and after the mutations under check, the cost schedule of the subnet that hosted it with the one of the subnet that hosts it now. That covers both ways of changing it — moving the canister and changing the cost schedule of its subnet — in any combination, and hence every mutation of the routing table or of a subnet record, present and future. `merge_subnets` also validates the two cost schedules itself, as `prepare_canister_migration` does, so that a proposal is rejected with an error rather than trapping in the invariant check. `reroute_canister_ranges` needs no check of its own: it only reroutes ranges covered by a canister migration, which `prepare_canister_migration` has already validated. Also use `get_key_family_iter` in `latest_subnet_cost_schedules`, which visits only the subnet record keys instead of scanning the whole store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`do_migrate_canisters` was left to the `check_canister_cost_schedule_invariants` registry invariant, which does catch it, but only by trapping in the invariant check without saying which canister is at fault. Validate the cost schedules of the subnet hosting each canister and of the target subnet up front, so that the trap names the canister and both subnets. The check mirrors the condition in `migrate_canisters_to_subnet`: it only applies when the target subnet already appears in the routing table, as the canisters are otherwise unassigned rather than moved, which leaves no cost schedule to preserve. A canister that no subnet hosts is skipped for the same reason. Also report only the intersection of the two overlapping canister ID ranges in the invariant's error message, as that is the part that actually changes its cost schedule: reporting the whole current range named canister IDs that were newly assigned or hosted by the same subnet all along, e.g. once two adjacent ranges got merged into one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical validation gaps still allow canisters to change cost schedules through unassigned routing intervals.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
The comparison of the cost schedule a canister is charged under only covers canister IDs that are routed both before and after the mutations under check, as an unrouted canister ID has no cost schedule to compare. Unrouting a canister ID does not destroy the canister state, callbacks included, so unrouting one and routing it to a subnet on a different cost schedule afterwards is not caught. Closing that would take either persistent knowledge of the cost schedule each unrouted canister ID last had, or making unrouting terminal. The latter is not an option: routing an unrouted canister ID to a subnet again is how a canister whose subnet was deleted is migrated, which `test_migrate_canisters_succeeds_if_source_subnet_deleted` covers. Document the gap rather than leave it implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Reassigning an unrouted canister can still bypass schedule compatibility checks and permit cross-schedule settlement.
Review details
Suppressed comments (1)
rs/registry/canister/src/mutations/do_migrate_canisters.rs:105
- This
continueleaves the financial invariant bypassable: an ID unassigned bydo_migrate_canisterswhen its target is absent can later be assigned by the same endpoint to any routed subnet, regardless of its former schedule. Because callbacks survive unrouting, that concrete two-call sequence can still settle a prepayment across Normal/Free schedules—the exact corruption this PR is intended to prevent. Preserve the last schedule for unrouted canisters and compare it here, or reject reassignment unless the source schedule can be proven compatible.
let Some((_range, source_subnet_id)) = routing_table.lookup_entry(*canister_id) else {
continue;
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…d change them CI caught a large instruction regression in the registry canbench benchmarks: the `migrate_canisters_*` benchmarks regressed by 71% to 74% and the `upgrade_with_routing_table_*` ones by 5% to 11%. Every mutation paid for reading the routing table twice over, once from the registry to get its pre-mutation state and once from the snapshot, and each read decodes every canister ranges shard. Charge those reads to the mutations that can actually change the cost schedule a canister is charged under, and only for the part of the routing table they touch: - a subnet record mutation can change the cost schedule of a subnet, so it pays for `check_subnet_cost_schedule_immutability`; - a canister ranges shard mutation can move a canister to a subnet on a different cost schedule, so it pays for `check_canister_cost_schedule_invariants`, which now compares only the shards that the mutation touches. A canister ID whose covering entry did not change is still assigned to the same subnet, and the cost schedule of that subnet cannot have changed either, which the invariant above enforces. Note that an entry can move between shards, e.g. when two adjacent ranges are merged into one, which mutates the shard it leaves as well as the shard it lands in, so both are compared; - every other mutation, `post_upgrade` with its empty mutation list included, pays for neither. Of the fourteen benchmarks, thirteen are back within the noise threshold. `migrate_canisters_10_times_100` remains 4.81% more expensive, which is the cost of the check itself on a routing table small enough for it to show: the 1k and the 10k variants of the same benchmark are within noise. Update the recorded results accordingly. Also add a test that a mutation of the routing table alone is checked, i.e. that the check is not skipped for want of a subnet record mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lves The list of mutations that can move a canister to a subnet on a different cost schedule claimed that `reroute_canister_ranges` and `merge_subnets` compare the cost schedules themselves. Neither half held: `reroute_canister_ranges` compares nothing of its own and relies on `prepare_canister_migration` having validated the canister migration it reroutes, and `do_migrate_canisters` does compare them, which it did not when that sentence was written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`check_global_state_invariants` collected the canister ranges shards that the mutations under check touch into a `Vec`, but a single batch can mutate the same shard more than once. The shard was then decoded once per mutation, and `RoutingTable` rejects the resulting duplicate entries, so the cost schedule check trapped with "There were N duplicate entries in the routing table" instead of checking anything. The SNS integration tests hit this. Collect the shards into a `BTreeSet` and take one in both helpers, so that decoding a shard twice is not expressible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The registry-wide migration invariant has broad operational impact and retains a documented unrouted-canister gap requiring human acceptance.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The cycles prepaid for a response execution are recorded in the callback under the cost schedule in effect when the call was performed, and settled when the response is executed against the cycles that execution requires, derived under the cost schedule in effect by then. The two amounts are only comparable if both were derived under the same cost schedule: an amount that is free under the free cost schedule has a zero real part but a non-zero nominal one, so settling across a switch either forfeits the real part of the prepayment or credits the canister with real cycles that were never withdrawn from its balance.
Nothing enforced that today. This PR enforces it for every canister that a subnet hosts, with one gap that it cannot close and documents instead: see Known gap.
The property, not its mutations
A canister is charged under the cost schedule of the subnet that the routing table assigns its canister ID to, so
check_canister_cost_schedule_invariantscompares, for every canister ID routed both before and after the mutations under check, the cost schedule of the subnet that hosted it with the one of the subnet that hosts it now. That covers both ways of changing it — moving the canister and changing the cost schedule of its subnet — in any combination, and hence every mutation of the routing table or of a subnet record, present and future.Enforcing this at the individual mutations instead would have kept missing paths:
merge_subnetsmoves every canister ID range of the source subnet to the destination subnet without touching either subnet record, anddo_migrate_canistersreassigns individual canisters without validating anything at all, not even that the target subnet exists.check_subnet_cost_schedule_immutabilityis kept alongside it as the stronger and simpler statement — no existing subnet ever changes its cost schedule, whichever mutation would change it; a newly created subnet is free to pick either. It also catches a change on a subnet that hosts no canister ID range yet.Validation at the mutations
An invariant failure traps, so the paths that a proposal or the canister migration orchestrator can reach validate the cost schedules themselves and report which subnets, or which canister, are at fault:
prepare_canister_migrationrejects preparing a migration between subnets on different cost schedules. This one is not covered by the invariant at all: it only writescanister_migrations, and the move happens later inreroute_canister_ranges;merge_subnetsrejects merging a source subnet into a destination on a different cost schedule;do_migrate_canistersrejects migrating a canister to a subnet on a cost schedule other than that of the subnet hosting it.reroute_canister_rangescompares nothing of its own: it only reroutes ranges covered by a canister migration, whichprepare_canister_migrationhas already validated.do_split_subnetneeds no check either, as the new subnet inherits the cost schedule of the source subnet.All comparisons normalize
UnspecifiedtoNormal, as a subnet record that predates the field leaves it unset and the replica reads that asNormal.What the checks cost
Reading the subnet records and the routing table is not free, and a first version of this PR had every mutation pay for reading the routing table twice over — once from the registry for its pre-mutation state and once from the snapshot, each read decoding every canister ranges shard. The canbench benchmarks caught it:
migrate_canisters_*regressed by 71% to 74% andupgrade_with_routing_table_*by 5% to 11%.Each check is now charged to the mutations that can actually break it:
check_subnet_cost_schedule_immutability;check_canister_cost_schedule_invariants, which compares only the shards that mutation touches. A canister ID whose covering entry did not change is still assigned to the same subnet, and the cost schedule of that subnet cannot have changed either, which the invariant above enforces. An entry can move between shards, e.g. when two adjacent ranges are merged into one, which mutates the shard it leaves as well as the shard it lands in, so both are compared. The shards are collected into a set, as a single batch can mutate one of them more than once and decoding a shard twice would produce duplicate entries, whichRoutingTablerejects;post_upgradewith its empty mutation list included, pays for neither.Thirteen of the fourteen benchmarks are within the noise threshold with that in place.
migrate_canisters_10_times_100remains 4.81% more expensive, which is the cost of the check itself on a routing table small enough for it to show — the 1k and the 10k variants of the same benchmark are within noise — and the recorded results are updated accordingly.Known gap: canister IDs that pass through an unrouted state
An unrouted canister ID has no cost schedule, so a canister ID that passes through an unrouted state is not covered by any of the above.
do_migrate_canistersunroutes a canister ID when the target subnet is absent from the routing table,remove_subnet_from_routing_tabledoes when its subnet is deleted, anddo_migrate_canistersthen supports routing it to a subnet again — that is how a canister whose subnet was deleted is migrated (test_migrate_canisters_succeeds_if_source_subnet_deleted). Two such calls in a row move a canister ID between cost schedules unchecked, and unrouting does not destroy the canister state, callbacks included.This was raised in review and is documented rather than fixed, as none of the available remedies fits in this PR:
Reaching the gap takes a canister migration orchestrator that unroutes a canister and then routes it to a subnet on a different cost schedule, which is not part of any current flow. The gap is documented on
check_canister_cost_schedule_invariantsand pointed at from the skip inRegistry::validate_cost_schedules, so it is not left implicit in the code either.Shipping with it is a call for a reviewer to make rather than mine: if it should be closed, the persistent record of the cost schedule each unrouted canister ID last had is the way to do it, and it belongs in its own change to the canister migration protocol.
Relation to #11431
Independent of #11431 (no shared files), but that PR relies on this assumption: it settles the prepayment on the nominal parts, which ignores a cost schedule switch entirely, whereas comparing the real parts happened to detect one. Merging this first means the assumption is enforced before anything depends on it.
🤖 Generated with Claude Code