feat(delegations): reactively fetch delegations on new public key and/or canister ranges - #10699
Conversation
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 566488e. Security Overview
Detected Code Changes
|
|
✅ No security or compliance issues detected. Reviewed everything up to b1534a2. Security Overview
Detected Code Changes
|
- No behavioral changes
- No
- No
- No
There was a problem hiding this comment.
Pull request overview
This PR reduces delegation-related downtime during subnet public-key / canister-range changes by adding a reactive delegation-refresh path driven by the latest certified state, and by extracting/refactoring the delegation read/validation logic into a dedicated ic_nns_delegation_reader crate for broader reuse (e.g., by HTTP handlers).
Changes:
- Add a new
ic_nns_delegation_readercrate that builds filtered delegations and validates delegation certificates against certified-state subnet topology (public key + canister ranges). - Update the NNS delegation manager to (a) proactively refresh on a fixed interval while holding back delegations that are “too new” for the replica’s certified state, and (b) reactively refetch quickly when the cached delegation mismatches the certified state.
- Wire the state reader into replica stack construction and extend metrics to distinguish fetch vs. state-comparison/reactive behavior.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rs/replica/src/setup_ic_stack.rs | Passes the state manager/reader into the delegation manager so it can compare delegations against certified state. |
| rs/http_endpoints/nns_delegation_reader/test_utils/src/lib.rs | Adds reader-focused test utilities for constructing fake certificate delegations. |
| rs/http_endpoints/nns_delegation_reader/test_utils/Cargo.toml | Renames the test-utils crate to align with the new reader crate. |
| rs/http_endpoints/nns_delegation_reader/test_utils/BUILD.bazel | Updates Bazel visibility and crate name for the renamed test-utils crate. |
| rs/http_endpoints/nns_delegation_reader/src/validation.rs | Introduces delegation-vs-state validation logic (public key + canister ranges) and unit tests. |
| rs/http_endpoints/nns_delegation_reader/src/reader.rs | Refactors delegation builder/reader API; adds verified build path and state-consistency checks. |
| rs/http_endpoints/nns_delegation_reader/src/lib.rs | Exposes the new reader/validation API surface. |
| rs/http_endpoints/nns_delegation_reader/Cargo.toml | Defines the new ic-nns-delegation-reader crate and its dependencies/bench config. |
| rs/http_endpoints/nns_delegation_reader/BUILD.bazel | Adds Bazel targets for the new crate, tests, and benchmarks. |
| rs/http_endpoints/nns_delegation_reader/benches/nns_delegation_reader.rs | Moves the benchmark to use the new reader crate/test utils. |
| rs/http_endpoints/nns_delegation_manager/src/nns_delegation_manager.rs | Adds state-driven reactive refresh + proactive hold-back logic; updates tests accordingly. |
| rs/http_endpoints/nns_delegation_manager/src/metrics.rs | Splits metrics into fetch/state-comparison/held-back/reactive counters. |
| rs/http_endpoints/nns_delegation_manager/src/lib.rs | Re-exports reader types from ic_nns_delegation_reader and removes the old internal reader module. |
| rs/http_endpoints/nns_delegation_manager/Cargo.toml | Adds state-manager + replicated-state + reader deps; adjusts dev-deps for updated tests. |
| rs/http_endpoints/nns_delegation_manager/BUILD.bazel | Updates Bazel deps to include the new reader crate and state-manager mocks; removes old bench target. |
| Cargo.lock | Records the new crates/dependency graph changes (reader crate + renamed test utils, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r/reader split Upstream PR #10699 split NNSDelegationReader/Builder and the certificate validation logic out of ic-nns-delegation-manager into a new ic-nns-delegation-reader crate (plus its ic-nns-delegation-reader-test-utils helper crate), and added proactive/reactive delegation fetching driven by consistency checks against the latest certified state.
During canister migrations and soon subnet splitting, the public key of a subnet and its canister ranges are modified. Because the delegation manager fetches new delegations every 5 minutes independently from the state, there can be up to 5 minutes of additional downtime during those operations just to let each replica fetch a new delegation that matches their state.
This PR proposes to add a new asynchronous path to the delegation manager to fetch new delegations. To enable this, the PR also refactors the delegation reader.
Delegation manager changes
The current "fetch every 5 minutes" logic stays with one important difference: if the fetched delegation does not match the latest certified state, hold it back, i.e. ignore it and try again 5 minutes later. This is to avoid the case where we would have fetched a delegation too early.
Asynchronously, the delegation manager locally checks every 10 seconds whether its latest delegation matches its latest certified state. In almost all cases, they will, and it won't do anything. Though if they do mismatch, then it reactively fetches a new delegation. This allows to reduce the maximum downtime from 5 minutes to 10 seconds.
Delegation reader refactor
The
NNSDelegationBuilder/Readerare moved to their own crateic_nns_delegation_reader, which contains the relevant code for comparing the delegations with the replicated state. By putting the comparison/validation logic directly inNNSDelegationBuilder, this will allow the HTTP handler to check whether its delegation matches the latest certified state and return a 503 error if not instead of serving an inconsistent delegation.For now, "delegation matching the state" means the same subnet public key and the same canister ranges for one's own subnet, but we can add more variants to
CanisterRangesCheckto check whether a specific canister ID is present in both the delegation and state (instead of checking for all ranges).The motivation for moving it to a different crate is that the query handler will need to depend on it to perform the comparison and it would be better not to also depend on all the dependencies of
ic_nns_delegation_manager. In particular,ic_nns_delegation_readercannot depend onReplicatedStateas it would otherwise create a circular dependency.