feat(registry): add merge_subnets endpoint - #11366
Conversation
Adds a `merge_subnets` endpoint to the registry canister. It takes a source and a destination subnet ID and merges the canister ID ranges of the source subnet into the canister ID range set of the destination subnet, so that the canisters hosted by the source subnet are routed to the destination subnet afterwards. The routing table is updated via `modify_routing_table`, which assigns the source subnet's ranges to the destination subnet and coalesces the resulting adjacent ranges. Only the routing table is touched: neither subnet record is modified and the source subnet is not deleted, so decommissioning a subnet is `merge_subnets` followed by `delete_subnet`. The endpoint is restricted to governance and rejects payloads where the two subnet IDs are equal, either subnet is unknown, the source subnet hosts no canister ID range, or an ongoing canister migration overlaps the source subnet's ranges (which would leave those migrations with a host that is not on their recorded trace). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a governance-only Registry endpoint for merging one subnet’s canister ranges into another.
Changes:
- Implements validation and routing-table reassignment.
- Exposes the endpoint through Candid.
- Adds unit, integration, and authorization tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
rs/registry/canister/unreleased_changelog.md |
Documents the endpoint. |
rs/registry/canister/tests/merge_subnets.rs |
Adds end-to-end coverage. |
rs/registry/canister/src/mutations/routing_table.rs |
Adds the routing mutation. |
rs/registry/canister/src/mutations/mod.rs |
Registers the mutation module. |
rs/registry/canister/src/mutations/merge_subnets.rs |
Implements validation and tests. |
rs/registry/canister/canister/registry.did |
Adds the production Candid API. |
rs/registry/canister/canister/registry_test.did |
Adds the test Candid API. |
rs/registry/canister/canister/canister.rs |
Exposes the governance-only update endpoint. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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 c189291. Security Overview
Detected Code Changes
|
…merge
Add the `MergeSubnets` NNS function, mapped to the `merge_subnets` method of the
registry canister under the `SubnetManagement` topic, and extend that method so
that a single such proposal performs the whole registry-side part of a subnet
merge, all in one registry version, so that the destination subnet never
observes a state where only some of it took effect:
1. the canister ID ranges of the source subnet are merged into the canister ID
range set of the destination subnet, so that all canisters that used to be
hosted by the source subnet are routed to the destination subnet;
2. a recovery catch-up package is created for the destination subnet, at the
height, time and state hash of the merged state, running a fresh DKG for
the destination subnet's membership; and
3. the destination subnet is brought back online.
The payload therefore grows the `height`, `time_ns` and `state_hash` of the
merged state, plus an `initial_dkg_subnet_id` naming the subnet that handles the
`setup_initial_dkg` call: it must not be the destination subnet, which is
offline while the merge is in progress.
The caller is expected to have taken both subnets offline and to have extended
the state of the destination subnet with the state of the canisters of the source
subnet beforehand; `state_hash` is the hash of the manifest of the result. The
source subnet record is not modified and the source subnet is not deleted: it
merely does not host any canister ID range anymore.
The recovery catch-up package needs fresh DKG transcripts because its
`initial_ni_dkg_transcript_{low,high}_threshold` become the current transcripts
of the DKG summary that bootstraps consensus at the recovery height. The registry
only holds the destination subnet's genesis (or last recovery) catch-up package,
whose transcripts its nodes may no longer have the secret key shares for, so
reusing those would risk restarting a subnet that cannot sign.
Merging into a subnet holding chain keys is rejected: recovering such a subnet
requires resharing its keys onto the recovery catch-up package, which this
method does not do, and silently leaving the destination subnet unable to sign
would be worse than refusing.
As the method now makes an inter-canister call half way through, it follows
`do_recover_subnet` in checking that none of the records it is about to
overwrite -- and none of the canister ID ranges it validated -- changed while
that call was in flight. Unlike `do_recover_subnet`, it reports the reject code
and message if that call fails, rather than an opaque `unwrap` panic.
`StateMachine` only answered `setup_initial_dkg` requests from
`do_execute_round`, which `tick()` (and hence `await_ingress`) does not go
through, so any canister awaiting such a call hung there forever. Factor the
fake responses out into `setup_initial_dkg_responses` and produce them from
`tick_with_config` as well, next to the threshold signing requests it already
answers. Without this the integration test of the success path cannot run, which
is why it is part of this commit.
The two success unit tests of `merge_subnets`, which can no longer drive the
method to completion, now exercise the routing table part directly; the success
path as a whole, including the recovery catch-up package and the unhalting, is
covered by the integration test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # rs/registry/canister/unreleased_changelog.md
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
rs/registry/canister/src/mutations/merge_subnets.rs:274
- This API documentation is inaccurate when
default_initial_dkg_subnet_idis configured: an omitted subnet is routed to that configured default and only falls back to NNS when no default exists.
/// unset, the request is handled by the NNS subnet.
Revalidate, after `setup_initial_dkg` returns, that no canister migration overlapping the canister ID ranges of the source subnet was prepared while the call was in flight: `prepare_canister_migration` does not touch the routing table, so the existing post-call check could not catch it. Also fix the low/high threshold transcript records being swapped in the `SetupInitialDKGResponse` that `StateMachine` synthesizes for pending `setup_initial_dkg` requests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
rs/registry/canister/src/mutations/merge_subnets.rs:135
- When
initial_dkg_subnet_idis omitted, the call is routed to the configured default initial-DKG subnet, not necessarily the NNS subnet. If that default is the destination, this validation passes and then waits for DKG from the offline subnet being recovered. Mirrorvalidate_recover_subnet_payloadand reject this case before making the call.
if let Some(initial_dkg_subnet_id) = initial_dkg_subnet_id {
if initial_dkg_subnet_id == destination_subnet {
return Err(format!(
"initial DKG subnet {initial_dkg_subnet_id} must be different from the destination subnet"
));
}
self.get(
&make_subnet_record_key(initial_dkg_subnet_id).into_bytes(),
pre_call_registry_version,
)
.ok_or_else(|| {
format!("initial DKG subnet {initial_dkg_subnet_id} is not a known subnet")
})?;
}
rs/registry/canister/src/mutations/merge_subnets.rs:287
- This fallback description is incorrect when
default_initial_dkg_subnet_idis configured: an omitted subnet ID is routed there and only falls back to the NNS subnet when no default exists. Documenting the actual routing is important because callers must explicitly override a default that equals the offline destination.
/// The subnet that should handle the `setup_initial_dkg` call producing the
/// DKG transcripts of the recovery CUP. Must be different from
/// `destination_subnet`, which is offline while the merge is in progress. If
/// unset, the request is handled by the NNS subnet.
pub initial_dkg_subnet_id: Option<SubnetId>,
Chain key initializations in a CUP take precedence over the chain key configuration of the subnet record, so carrying over the ones of the CUP being replaced would make the destination subnet bootstrap obsolete key material -- contradicting the endpoint's refusal to merge into a subnet holding chain keys. Clear both initialization fields, as recovering a subnet without an initial chain key configuration does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Revert `merge_subnets` to what it originally did: merge the canister ID ranges of the source subnet into the canister ID range set of the destination subnet, and nothing else. The recovery catch-up package, the fresh DKG for the destination subnet's membership and the unhalting are dropped again, and with them the `height`, `time_ns`, `state_hash` and `initial_dkg_subnet_id` payload fields, the inter-canister call and the guards around it. Recovering the destination subnet at the merged state stays with `recover_subnet`. The `MergeSubnets` NNS function is kept -- without it the endpoint would be unreachable -- with its description narrowed accordingly. The `StateMachine` change that answered `setup_initial_dkg` requests from `tick_with_config` goes away with the inter-canister call that needed it. Its fix of the swapped `SetupInitialDKGResponse` fields stays, as that is a bug of its own: the `HighThreshold`-tagged transcript was encoded as the low-threshold record and vice versa. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pierugo-dfinity
left a comment
pierugo-dfinity
left a comment
There was a problem hiding this comment.
Had a quick look, will continue tomorrow
| // migration out of those ranges: the migrated ranges would end up being hosted by the | ||
| // destination subnet, which is not on the recorded migration trace. | ||
| if let Some(canister_migrations) = self.get_canister_migrations(version) | ||
| && !are_disjoint(canister_migrations.ranges(), source_ranges.iter()) |
There was a problem hiding this comment.
Is it fine to have canister migrations from destination_ranges?
There was a problem hiding this comment.
Yes, because subnet merging does not (concurrently) move any ranges away from destination_ranges. But actually I'm gonna drop this check since
- the registry key
canister_migrationsis used for subnet splitting and we are definitely not gonna merge and split subnets at the same time; - streams from/to the source subnet must be empty during subnet splitting and thus the registry key
canister_migrationsis a no-op for the source subnet anyway.
There was a problem hiding this comment.
canister_migrationsis used for subnet splitting
I see it's also used in prepare_canister_migration though I'm not sure in what context is this mutation called.
There was a problem hiding this comment.
This is the subnet splitting flow.
There was a problem hiding this comment.
This is the subnet splitting flow.
Ah yes, the old manual flow
we are definitely not gonna merge and split subnets at the same time
Well, if we ever accidentally did, at least this check would prevent us from nuking the IC 👀
There was a problem hiding this comment.
While I agree that if everything works according to plan we should never have non-empty streams to or from the source subnet at this point, I believe there is value in defense in depth.
If under normal circumstances this is going to be a no-op anyway, then I'd rather keep it than drop it. Also for documentation purposes: you could simply replace the comment above (since it's obvious we don't want to mix merges and splits) with "Checks before the merge should ensure that this never triggers", where "this" is documented by the error message.
daniel-wong-dfinity-org-twin
left a comment
daniel-wong-dfinity-org-twin
left a comment
There was a problem hiding this comment.
I would have split this between registry and Governance. I mean, the governance changes are small, so I guess this is ok.
It would be nice if Governance did some validation, like, does the payload even decode to the right type, but this is a general flaw with NnsFunctions, and since split is an NnsFunction, it probably makes sense for this also to be one...
| destination subnet. Only the routing table is updated: neither subnet record | ||
| is modified and the source subnet is not deleted. |
There was a problem hiding this comment.
So, basically, this migrates all canisters from one subnet to another, right? I don't think that's what "merge" calls to mind, at least, for me it doesn't. I mean, I do not hate the name, so it's probably not worth changing it at this point, but if you are willing to entertain suggestions, how about "migrate all subnet canisters", or "evacuate subnet", or something? This would make it clearer that the two subnets survive; it's just the contents of one are dumped into another.
There was a problem hiding this comment.
The word "migrate" is already overloaded to refer to (i) subnet splitting via the registry key canister_migrations and (ii) single canister ID migration (routing table change) orchestrated by a dedicated NNS canister. So I'd prefer to not use it in yet another context.
I suppose your confusion comes from the fact that the source subnet survives (it must be cleaned up by a separate proposal) so how about MergeSubnetCanisterRanges?
There was a problem hiding this comment.
Do as you see fit.
Yes, the fact that the source subnet remains afterwards is what makes me think that "merge" is not so apt. When you merge two companies, the employees do not simply move from one to the other. Rather, you are left with just one company.
The existing "migrate" operation moves a canister from one subnet to another, right? Isn't that what's going on here, just a different number of canisters? To me, that makes "migrate" an even better choice. I don't see it as conflation; I see it as consistency.
There's also my other suggestion, "evacuate", which does not conjure images of the existing thing called "migrate". This more emphasizes that ALL canisters are moved out (but does not mention that they will all go to the same destination), which to me, is not as important as the fact that existing canisters are finding a new home, have an actual DESTINATION, not just left stranded in the middle of the street, as might happen if you evacuate a building due to fire. "migrate" is less chaotic. It implies a planned destination, not just a departure.
| check_subnet_for_canisters( | ||
| ®istry, | ||
| vec![ | ||
| (CanisterId::from(0), subnet_id_2), |
There was a problem hiding this comment.
What about non-endpoint canister IDs? I mean, since everything from 0 to 511 is taken, you could concisely check all of (0..=511), right? In fact, it would probably be even less code than this!
There was a problem hiding this comment.
Done in c189291: the test now checks all of 0..=511. It still runs in ~11s.
There was a problem hiding this comment.
tangent: maybe add one negative case? Sorry, I should have thought of this before.
Drops the canister migrations check: the registry key `canister_migrations` is only used for subnet splitting, which is never concurrent with a subnet merge, and the streams of a splitting subnet must be empty anyway. The rest is test and comment cleanup: the two-subnet fixture registry becomes a lazy_static pseudo-constant with its routing table as a constant, the failing merges now all assert that the routing table is unchanged (through a new `assert_routing_table_unchanged_vs_fixture` helper), and the integration test checks the subnet of every canister ID in [0, 511]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Source ranges in active canister migrations must be rejected to prevent invariant failures and unsafe rerouting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
| let routing_table = self.get_routing_table_or_panic(version); | ||
| if routing_table.ranges(source_subnet).is_empty() { | ||
| return Err(format!( | ||
| "source subnet {source_subnet} does not host any canister ID range" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Subnet merging comes with a set of manually checked requirements (see design doc), the subnets not being split being one of them. Hence, omitting this particular check makes sense to me since it'd only result in a failed NNS governance proposal.
There was a problem hiding this comment.
As long as the check does nor conflict with the manual checks, I would add it.
In particular, a canister appearing in a canister_migrations entry means that there may still exist messages addressed to that canister that were routed into the "wrong" stream. Since subnet merging does not touch canister_migrations (and thus the actual host subnet after the merge is never added to the migration path) such messages might end up rerouted forever or dropped.
| let routing_table = self.get_routing_table_or_panic(version); | ||
| if routing_table.ranges(source_subnet).is_empty() { | ||
| return Err(format!( | ||
| "source subnet {source_subnet} does not host any canister ID range" | ||
| )); | ||
| } |
There was a problem hiding this comment.
As long as the check does nor conflict with the manual checks, I would add it.
In particular, a canister appearing in a canister_migrations entry means that there may still exist messages addressed to that canister that were routed into the "wrong" stream. Since subnet merging does not touch canister_migrations (and thus the actual host subnet after the merge is never added to the migration path) such messages might end up rerouted forever or dropped.
| // migration out of those ranges: the migrated ranges would end up being hosted by the | ||
| // destination subnet, which is not on the recorded migration trace. | ||
| if let Some(canister_migrations) = self.get_canister_migrations(version) | ||
| && !are_disjoint(canister_migrations.ranges(), source_ranges.iter()) |
There was a problem hiding this comment.
While I agree that if everything works according to plan we should never have non-empty streams to or from the source subnet at this point, I believe there is value in defense in depth.
If under normal circumstances this is going to be a no-op anyway, then I'd rather keep it than drop it. Also for documentation purposes: you could simply replace the comment above (since it's obvious we don't want to mix merges and splits) with "Checks before the merge should ensure that this never triggers", where "this" is documented by the error message.
| /// Merges the canister ID ranges of the source subnet into the canister ID | ||
| /// range set of the destination subnet. | ||
| /// | ||
| /// After this operation, all canisters that used to be hosted by the source | ||
| /// subnet are routed to the destination subnet and the source subnet does | ||
| /// not host any canister ID range anymore. | ||
| /// | ||
| /// Note that only the routing table is updated: neither subnet record is | ||
| /// modified and, in particular, the source subnet is not deleted. |
There was a problem hiding this comment.
I was going to comment below, on MergeSubnetsPayload that it would be cleaner to talk about moving canisters than merging canister ranges. But then, the registry mutation is specifically the "merging canister ranges" part of the canister migration, so this is the appropriate vocabulary.
What may be missing from this doc comment though, is a very high level (read, one or two short sentences) description of the merging process pointing out that both subnets are halted at this point, with all streams to and from the source subnet empty; and that the destination subnet (with all canister states) will only be unhalted after this registry change is applied (I was going to say "after it has been observed by all subnets", but that turns out to not be necessary).
| /// Merges the canister ID ranges of the source subnet into the canister ID | ||
| /// range set of the destination subnet. | ||
| /// | ||
| /// After this operation, all canisters that used to be hosted by the source | ||
| /// subnet are routed to the destination subnet and the source subnet does | ||
| /// not host any canister ID range anymore. | ||
| /// | ||
| /// Note that only the routing table is updated: neither subnet record is | ||
| /// modified and, in particular, the source subnet is not deleted. |
There was a problem hiding this comment.
Taking a step back, do we have (or plan to have) a description of the subnet merging process anywhere in the code? (Essentially an abbreviated version of the process described in the design doc.)
If not, then this (or, probably better, the state tool command) would be a good place to have it.
| /// Note that only the routing table is updated: neither subnet record is | ||
| /// modified and, in particular, the source subnet is not deleted. | ||
| pub fn merge_subnets(&mut self, payload: MergeSubnetsPayload) -> Result<(), String> { | ||
| println!("{LOG_PREFIX}merge_subnets: {payload:?}"); |
There was a problem hiding this comment.
Ignorant question: Is this intended as a canister debug log or is it just a leftover?
| } | ||
|
|
||
| #[test] | ||
| fn test_merge_subnets_into_subnet_without_canister_id_ranges() { |
There was a problem hiding this comment.
Is there any use case for "merging" canisters onto an empty subnet?
I suppose you could e.g. move all canisters from an application subnet to a cloud engine (or the other way around) this way, but wouldn't it be simpler to add a "change subnet type" proposal instead?
| /// Merge a subnet into another subnet: the canister ID ranges of the source | ||
| /// subnet are merged into the canister ID range set of the destination subnet, | ||
| /// so that all canisters that used to be hosted by the source subnet are routed | ||
| /// to the destination subnet. |
There was a problem hiding this comment.
A briefer description would be:
| /// Merge a subnet into another subnet: the canister ID ranges of the source | |
| /// subnet are merged into the canister ID range set of the destination subnet, | |
| /// so that all canisters that used to be hosted by the source subnet are routed | |
| /// to the destination subnet. | |
| /// Merge a subnet into another subnet: in the routing table, reassigns all | |
| /// canister ranges hosted by the source subnet to the destination subnet. |
Or "remaps".
| // Merge a subnet into another subnet: the canister ID ranges of the source | ||
| // subnet are merged into the canister ID range set of the destination subnet, | ||
| // so that all canisters that used to be hosted by the source subnet are routed | ||
| // to the destination subnet. |
| ValidNnsFunction::MergeSubnets => { | ||
| "Merge a subnet into another subnet. The canister ID ranges of the source subnet \ | ||
| are merged into the canister ID range set of the destination subnet, so that all \ | ||
| canisters that used to be hosted by the source subnet are routed to the \ | ||
| destination subnet. Only the routing table is updated: neither subnet record is \ | ||
| modified and the source subnet is not deleted." | ||
| } |
| * Added a new `NnsFunction` variant `MergeSubnets`, which proposes to merge a | ||
| subnet into another subnet: the canister ID ranges of the source subnet are | ||
| merged into the canister ID range set of the destination subnet, so that all | ||
| canisters that used to be hosted by the source subnet are routed to the | ||
| destination subnet. Only the routing table is updated: neither subnet record |
| assert_eq!( | ||
| get_routing_table_entries(registry), | ||
| FIXTURE_ROUTING_TABLE_ENTRIES.to_vec(), | ||
| ); |
There was a problem hiding this comment.
Do as you see fit.
Ah. This is so concise now that FIXTURE_ROUTING_TABLE_ENTRIES can serve as a reference value. Inlining might make more sense.
(My suggestion to spin out made more sense when the reference value was a multi-line "literal".)
The registry side of a subnet merge: a
merge_subnetsendpoint on the registry canister, and aMergeSubnetsNNS function mapped to it under theSubnetManagementtopic, so that a single proposal performs the range reassignment.The endpoint takes a source and a destination subnet ID and merges the canister ID ranges of the source subnet into the canister ID range set of the destination subnet, so that the canisters hosted by the source subnet are routed to the destination subnet afterwards. The routing table is updated via
modify_routing_table, which assigns the source subnet's ranges to the destination subnet and coalesces the resulting adjacent ranges. Only the routing table is touched: neither subnet record is modified and the source subnet is not deleted, so decommissioning a subnet ismerge_subnetsfollowed bydelete_subnet— the source subnet merely does not host any canister ID range anymore.The endpoint is restricted to governance and rejects payloads where the two subnet IDs are equal, either subnet is unknown, or the source subnet hosts no canister ID range.
Recovering the destination subnet at the merged state — the recovery catch-up package, the fresh DKG and the unhalting — is deliberately out of scope here and stays with
recover_subnet.One unrelated
StateMachinefix comes along: the fakeSetupInitialDKGResponseit produces for pendingsetup_initial_dkgrequests had its two fields swapped, putting theHighThreshold-tagged transcript intolow_threshold_transcript_recordand vice versa.