Repository navigation
Conversation
When a subnet is created or split off, it is assigned the canister IDs directly after the last range in the routing table. Deleting a subnet removes its ranges from the routing table, so if the deleted subnet held the last range, the next new subnet received exactly the same canister IDs. This has happened on mainnet, e.g., subnet jmfme received the range of the deleted subnet jimdo. The registry now stores a `CanisterIdAllocation` record under the key `canister_id_allocation`, holding one past the largest canister index ever allocated to a subnet. New ranges start at or above it, and creating or splitting a subnet bumps it in the same atomic mutation. If the record is absent, it is computed from the full history of the routing table (both the `canister_ranges_*` shards and the legacy `routing_table` key). A post-upgrade migration backfills the record from that history. New invariants ensure that, once present, the record is never deleted, never decreases, and that every routing table range ends below it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not validate the backfilled allocation against historical data, leaving an underestimated value that could permit reuse of deleted-subnet IDs undetected.
1 open issue
What changed in this PR
This PR prevents the registry from reallocating canister ID ranges belonging to deleted subnets by tracking the highest allocation across routing-table history.
Changes:
- Adds a persistent allocation record and updates it atomically during subnet creation and splitting.
- Backfills the record after upgrade and enforces nondecreasing allocation bounds.
- Adds tests for allocation, historical fallback, backfill, and invariants.
| File | Description |
|---|---|
rs/registry/routing_table/src/tests.rs |
Tests deleted-range reuse and allocation-aware insertion. |
rs/registry/routing_table/src/lib.rs |
Adds allocation-aware helpers and documents legacy reuse behavior. |
rs/registry/keys/src/lib.rs |
Defines the allocation record key. |
rs/registry/canister/src/registry_lifecycle.rs |
Runs allocation backfill before other upgrade migrations. |
rs/registry/canister/src/mutations/routing_table.rs |
Updates allocation bounds during subnet creation and splitting. |
rs/registry/canister/src/mutations/mod.rs |
Registers the allocation module. |
rs/registry/canister/src/mutations/canister_id_allocation.rs |
Implements allocation tracking, historical scanning, and backfill. |
rs/registry/canister/src/mutations/canister_id_allocation_tests.rs |
Tests historical allocation preservation and migration behavior. |
rs/registry/canister/src/invariants/routing_table.rs |
Adds allocation invariants and focused tests. |
rs/registry/canister/src/invariants/checks.rs |
Integrates allocation checks into registry mutation validation. |
rs/protobuf/src/gen/state/registry.routing_table.v1.rs |
Adds the generated state allocation type. |
rs/protobuf/src/gen/registry/registry.routing_table.v1.rs |
Adds the generated registry allocation type. |
rs/protobuf/def/registry/routing_table/v1/routing_table.proto |
Defines the allocation record schema. |
Review effort: Balanced
…ion backfill Very old routing table versions encoded `CanisterIdRange` ends as a plain `uint64` (field 2, now reserved), which `pb::RoutingTable` drops when decoding. The history scan skipped such ranges, assuming they cannot hold the historical maximum. Decode both encodings instead, so the backfill does not rely on that assumption, and test it with legacy-encoded bytes. Also check in the golden NNS state upgrade test that the backfilled `next_unallocated_canister_index` is at least 0xa000000, the value found by replaying mainnet registry history up to version 65131. An underestimate would let new subnets reuse canister IDs of deleted subnets without violating the registry invariants. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The u64 watermark cannot represent the one-past-end value for historical ranges ending at u64::MAX, causing post-upgrade backfill to trap or wrap and potentially allow canister ID reuse.
1 open issue
1 resolved since last review
Review effort: Balanced
- Detect the registry upgrade in the golden NNS state test by canister ID instead of by name. - Simplify the doc comment of check_canister_id_allocation_invariants. - Explain each skipped case in the routing table history scan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The backfill’s end + 1 overflows for valid historical ranges ending at u64::MAX, risking failed registry upgrades or an understated watermark that permits ID reuse.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ting table range only The invariant check decoded the whole routing table on every mutation, in addition to the existing routing table invariant checks, which made e.g. canister migrations ~27% more expensive. Since the routing table shards are sorted and disjoint, decoding the last non-empty shard suffices. Also update the canbench results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues remain: unordered routing-table shards can produce an incorrect allocation watermark, and ranges ending at u64::MAX can cause upgrade traps or watermark wraparound, risking canister ID reuse.
1 open issue
1 resolved since last review
Review effort: Balanced
…ting table The previous optimization only decoded the last routing table shard, assuming that shard keys are ordered like the ranges they hold. The routing table invariants do not check that, so a misordered shard could hide a range above the allocated canister IDs. Instead, decode the routing table once in check_global_state_invariants and pass it to both the canister migrations and the canister ID allocation checks. Add a regression test with misordered shards and update the canbench results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…allocation The existing upgrade_with_routing_table_* benchmarks never run the backfill, since their setup already creates the CanisterIdAllocation. Add variants that delete it before the upgrade, so that post_upgrade backfills it from the full history of the routing table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 6adb9c2. Security OverviewDetected Code Changes
|
| @@ -220,35 +220,79 @@ impl CanisterIdRanges { | |||
| } | |||
|
|
|||
| /// A helper function to help insert a new subnet to the routing table | |||
There was a problem hiding this comment.
This seems to under-sell what this does. What it really does is ALLOCATE a bunch of canister IDs (in a contiguous range), and record that in routing_table.
(Previously, the way it did that was to find the last currently routed canister ID, and take the next CANISTER_IDS_PER_SUBNET canister IDs, and assign those to the subnet. But this became incorrect once we added the ability to delete subnets, because then, this would re-allocate previously allocated canister IDs.)
Docs aside, in order to correctly allocate canister IDs, this NEEDS some additional information to avoid re-allocating previously allocated canister IDs. I like your idea of using a "high water mark" (simple and effective), but this function doesn't use that. Rather, this function is just being left behind, turning it into a land mine.
The simplest way to demine is to make next_unallocated_canister_index REQUIRED. That is, add that parameter to this function. Ofc, that would require updating call sites. It wouldn't be that bad, because they mostly build RoutingTable from scratch (mostly in tests), and in that case, it is acceptable to just pass 0 (for next_unallocated_canister_index), since in that case, there are no deletions that throw off the original way of finding fresh canister IDs (i.e. looking at max currently routed canister ID).
Another way to demine would be to add the high water mark to RoutingTable. Whether it really belongs there is debatable, but I submit it for your consideration.
(Tangent: I maybe would have originally called this the CanisterToSubnetMap or something like that, because this isn't just about how to direct requests. Rather, this tells us who (i.e. which subnet) runs what (i.e. which canisters). That then DRIVES how requests are routed, not the other way around. In that case, it is maybe clearer that this owns the high water mark, because then it is clearer that the job here is to ALLOCATE canister IDs (to subnets), not just route requests. But ofc, I'm not suggesting we actually rename this. My only point here is, this thing really WANTS the job of allocating, not just routing.)
There was a problem hiding this comment.
Agreed, the wrapper defaulting to 0 was a land mine. Addressed in 28bb113:
- There is now a single
routing_table_insert_subnetthat requiresnext_unallocated_canister_indexand returns the updated value; the zero-defaulting wrapper androuting_table_insert_subnet_after_allocated_canister_idsare gone. - Its docs now say that it allocates fresh canister IDs to the new subnet, and why the routing table alone is not enough once subnets can be deleted.
- Code that builds a routing table from scratch (tests, and ic-prep at genesis) passes
0or the value returned by the previous call, which is correct because no subnet has been deleted there. - The
StateMachinetest registry (make_registry_entries_for_subnet_split) does not track the allocation yet and passes0; I left that for a follow-up PR (noted in the PR description).
I kept the high water mark out of RoutingTable itself: replicas use that type for routing, and it is serialized into the registry shards and replicated state, so adding the field would change those formats for data replicas don't need. It lives in its own CanisterIdAllocation registry record instead, with invariants tying the two together.
… a subnet into the routing table `routing_table_insert_subnet` now requires `next_unallocated_canister_index` and returns the updated value, so callers can no longer allocate canister IDs without accounting for deleted subnets. This replaces the former zero-defaulting wrapper and `routing_table_insert_subnet_after_allocated_canister_ids`. Code that builds a routing table from scratch (tests, and ic-prep at genesis) passes 0 or the value returned by the previous call. The StateMachine test registry does not track the allocation yet and passes 0; that is left for a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 6adb9c2. Security OverviewDetected Code Changes
|
There was a problem hiding this comment.
🔵 Needs a closer look
The backfill overflows when computing end + 1 for valid ranges ending at u64::MAX, risking upgrade failure or loss of the historical maximum that permits ID reuse.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
pierugo-dfinity
left a comment
pierugo-dfinity
left a comment
There was a problem hiding this comment.
Approving core-protocol-owned files
| } | ||
|
|
||
| /// A helper function to help insert a new subnet to the routing table | ||
| /// Allocates `CANISTER_IDS_PER_SUBNET` fresh canister IDs (a contiguous range) |
There was a problem hiding this comment.
So, I was chatting with Claude about this, and it is telling me that other parts of the system assume that when a new subnet gets its initial canister ID range, that canister ID range is aligned to and of size CANISTER_IDS_PER_SUBNET.
That seems fragile. Ofc, this issue is not something that would be solved in this PR. I'm not saying to get rid of that assumption in this PR. I mostly want to be on the record that relying on such assumptions seems unnecessarily fragile.
One suggestion though: maybe we can make sure that start is a multiple of CANISTER_IDS_PER_SUBNET? Like, maybe we can round start up to such a multiple? I mean, if everything works the way we think it does, such rounding would not change the outcome, right?
There was a problem hiding this comment.
Agreed that relying on alignment is fragile, and I won't try to remove that assumption here. I did take the suggestion though: routing_table_insert_subnet now rounds start up to the next multiple of CANISTER_IDS_PER_SUBNET. As you say, if everything is aligned (as on mainnet and at genesis) this changes nothing, and if it isn't, we skip a few IDs and never reuse any. There's a new test for the misaligned case. Done in ad402a1.
| // Every canister ID whose index (the u64 encoded in its first 8 bytes) is | ||
| // below `next_unallocated_canister_index` has been assigned to some subnet at | ||
| // some point, even if that subnet has since been deleted and its range is no | ||
| // longer in the routing table. |
There was a problem hiding this comment.
Why say this? I mean, that might be true, but if you document it, then people will think they have permission to RELY on this property. The problem is, there may come a day when we want to skip some canister IDs for whatever reason. And in any case, I don't see how giving such permission helps anyone. Therefore, I would just leave this unsaid.
What we are currently relying on is that no canister ID >= next_unallocated_canister_index has ever been allocated. You could instead document that. I would mention that above the field though, since such a remark pertains specifically to the field.
There was a problem hiding this comment.
Good point. That property isn't needed, and promising it would get in the way if we ever want to skip IDs (which the alignment rounding from the other thread now does). I removed it and documented only what we rely on, above the field: no canister ID at or above next_unallocated_canister_index has ever been allocated. I also updated the Rust docs and the changelog that said "one past the largest index ever allocated". Done in ad402a1.
| /// This is read from the `CanisterIdAllocation`. If that does not exist yet (e.g., the | ||
| /// registry has not been upgraded past the release introducing it), it is computed from the |
There was a problem hiding this comment.
e.g. the registry has not been upgraded past the release introducing it
But if Registry has upgraded to a point where it has the definition of CanisterIdAllocation, then, the back fill must have run, right? So, in production, Registry would always have a CanisterIdAllocation record (not just the definition of that type), right?
Seems to me, the main reason such a record wouldn't exist is when there is a fresh Registry.
To get rid of that inconsistency, we could maybe insert a CanisterIdAllocation during installation. WDYT?
If we can make this consistent, then, things could be greatly simplified. In particular,
-
we just panic when
self.getreturns None. -
only back fill needs history, so once back fill is performed, we can delete the history scanner.
There was a problem hiding this comment.
I tried this: canister_init created the record, the backfill always wrote it, and a missing record panicked. It broke many test targets:
- Many setups (NNS test utils, registry integration tests, replay, root tests) install the registry with an empty init payload and fill it in later through
atomic_mutate. Creating the record at init then becomes the first mutation applied to an empty registry, and the global invariant checks reject it ("no system subnet"). Any mutation would fail the same way, so the problem is where the write happens, not the record itself. - A few tests assert specific registry versions after installation, and the extra mutation shifts them by one.
Both can be worked around: add the record to the last batch of a non-empty init payload, and have the test helpers that fill the registry later also write it. But that changes test setup in many places, which I'd rather not mix into this PR. I kept the fallback for now. Once mainnet has the backfill, the fallback only matters for fresh registries, and there "history" is just the setup mutations.
| // Tracks which canister IDs have ever been allocated to a subnet, so that | ||
| // subnet creation never hands out canister IDs again after the subnet that | ||
| // owned them has been deleted. |
There was a problem hiding this comment.
| // Tracks which canister IDs have ever been allocated to a subnet, so that | |
| // subnet creation never hands out canister IDs again after the subnet that | |
| // owned them has been deleted. | |
| // Whatever is needed to allocate canister IDs to new subnets. In particular, | |
| // we are trying to prevent a new canister from getting an ID used by an old | |
| // canister. |
Let's not rehash what bug motivated this. Rather, let's just describe what this is trying to do: support the "no recycling canister IDs" property.
There was a problem hiding this comment.
Applied, thanks. I also moved the precise guarantee to the field (see the other thread) and reworded the module doc in canister_id_allocation.rs the same way. Done in ad402a1.
…sterIdAllocation docs - `routing_table_insert_subnet` rounds the start of a new range up to the next multiple of `CANISTER_IDS_PER_SUBNET`, so that new ranges are aligned even if existing ranges are not. This changes nothing when everything is aligned. - Document only the guarantee that is relied on: no canister ID at or above `next_unallocated_canister_index` has ever been allocated to a subnet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Historical ranges ending at u64::MAX overflow the backfill counter, causing post-upgrade traps or allowing previously allocated canister IDs to be reused rather than explicitly handling address-space exhaustion.
No open issues 🎉
Review effort: Balanced
…ion-high-water-mark Resolve the conflict in routing_table_insert_subnet (keep the new next_unallocated_canister_index parameter, adopt next_back() from master), use next_back() in the routing table invariant check as well, and pass the new argument at a call site added on master. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The historical-range scan computes end + 1 for valid ranges ending at u64::MAX, risking an upgrade trap or a wrapped watermark that permits ID reuse after deletion, so exhaustion needs explicit handling and regression coverage.
No open issues 🎉
Review effort: Balanced
Problem
A new subnet (created or split off) gets the canister IDs right after the last range in the current routing table. Deleting a subnet removes its ranges, so if it held the last range, the next new subnet gets exactly the same canister IDs.
This has happened on mainnet: replaying registry versions 56000–65131 shows 9 reuses in 7 blocks, mostly of cloud engines. For example, block
[0x3100000, 0x31fffff]went to subnet jimdo, which was deleted, and then to subnet jmfme.Fix
CanisterIdAllocation { next_unallocated_canister_index }(keycanister_id_allocation): no canister index at or above it has ever been allocated to a subnet, including deleted subnets.CANISTER_IDS_PER_SUBNET(a no-op when existing ranges are aligned, as on mainnet), and updates the record in the same atomic mutation.canister_ranges_*shards and the legacyrouting_tablekey, in every encoding ever stored). The same computation is the fallback if the record is missing.Fixedentry in the registry'sunreleased_changelog.md.API:
routing_table_insert_subnetnow requiresnext_unallocated_canister_indexand returns the updated value, so callers can no longer allocate canister IDs without accounting for deleted subnets. Code that builds a routing table from scratch (tests, andic-prepat genesis) passes0or the value returned by the previous call, which is correct because no subnet has been deleted there.Follow-up: the
StateMachinetest registry (make_registry_entries_for_subnet_split) does not track the canister ID allocation yet and passes0, so it can still reuse the canister IDs of a deleted subnet. Making it track the allocation is left for a follow-up PR.Testing
upgrade_canisters_with_golden_nns_stateassertsnext_unallocated_canister_index >= 0xa000000, the value from replaying mainnet history up to registry version 65131.upgrade_with_routing_table_and_canister_id_allocation_backfill_*benchmarks measure the first upgrade, which runs the backfill: +9% to +16% instructions over a regular upgrade. Existing benchmarks change by at most +1.8% compared with master.🤖 Generated with Claude Code