feat: Fast upgrades: upgrade permit shares, pool, and pool manager - #11548
Open
frankdavid wants to merge 2 commits into
Open
frankdavid wants to merge 2 commits into
frankdavid wants to merge 2 commits into
Conversation
First step of the Phase-2 rolling-reboot consensus protocol, which adds the data types and the artifact mechanism. * Add `UpgradePermitAction` (`Request` / `Authorize` / `Return`). `UpgradePermitAuthorizationRequest` is the signed content, `UpgradePermitAuthorizationShare` is the gossiped artifact. * Add the in-memory `UpgradePermitAuthPoolImpl` which is modeled after other pools. * Add the `ic-consensus-upgrade` crate with `UpgradePermitAuthPoolManager`. It signs shares for requests in finalized blocks and validates gossiped shares. * Register the new proto file with the generator and add `UpgradePermitAuthorizationRequest` (signing) and `UpgradePermitAuthorizationShare` (pool-ID hashing) domain separators.
|
✅ No security or compliance issues detected. Reviewed everything up to f8f1910. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Consensus hash incompatibility, incomplete payload validation and production wiring, and incorrect retry and share-validation behavior are blocking issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces Phase-2 upgrade-permit types, protobuf encoding, cryptographic domains, artifact pooling, and authorization-share management.
Changes:
- Adds upgrade request, authorization, return, and signature-share types.
- Adds payload serialization, artifact IDs, pool interfaces, and an in-memory pool.
- Adds a pool manager for signing, validating, gossiping, and expiring shares.
File summaries
| File | Description |
|---|---|
rs/types/types/src/crypto/sign.rs |
Adds request signing domain. |
rs/types/types/src/crypto/hash/tests.rs |
Updates consensus hash vectors. |
rs/types/types/src/crypto/hash/domain_separator.rs |
Adds upgrade domains. |
rs/types/types/src/crypto/hash.rs |
Adds request/share hash domains. |
rs/types/types/src/consensus/upgrade.rs |
Defines permit actions. |
rs/types/types/src/consensus.rs |
Adds request/share types and protobuf conversion. |
rs/types/types/src/batch/upgrade.rs |
Encodes and decodes upgrade payloads. |
rs/types/types/src/batch.rs |
Extends batch payloads and messages. |
rs/types/types/src/artifact.rs |
Defines authorization-share IDs. |
rs/test_utilities/types/src/batch/payload.rs |
Initializes test payload field. |
rs/state_machine_tests/src/lib.rs |
Initializes delivered upgrade actions. |
rs/protobuf/src/gen/types/types.v1.rs |
Regenerates protobuf bindings. |
rs/protobuf/generator/src/lib.rs |
Registers upgrade protobuf generation. |
rs/protobuf/def/types/v1/upgrade.proto |
Defines upgrade wire messages. |
rs/protobuf/def/types/v1/consensus.proto |
Adds block upgrade bytes. |
rs/protobuf/def/types/v1/artifact.proto |
Adds upgrade artifact IDs. |
rs/interfaces/src/upgrade.rs |
Adds pool and validation interfaces. |
rs/interfaces/src/p2p/consensus.rs |
Corrects documentation spelling. |
rs/interfaces/src/lib.rs |
Exports upgrade interfaces. |
rs/interfaces/src/crypto.rs |
Extends the crypto interface. |
rs/interfaces/src/consensus.rs |
Adds upgrade validation errors. |
rs/interfaces/mocks/src/crypto.rs |
Adds upgrade crypto mocks. |
rs/consensus/utils/src/crypto.rs |
Extends consensus crypto bounds. |
rs/consensus/upgrade/src/pool_manager.rs |
Implements share lifecycle management. |
rs/consensus/upgrade/src/lib.rs |
Implements membership and share validation. |
rs/consensus/upgrade/Cargo.toml |
Defines the new crate. |
rs/consensus/upgrade/BUILD.bazel |
Adds Bazel targets. |
rs/consensus/src/consensus/payload_builder.rs |
Initializes upgrade test payloads. |
rs/consensus/mocks/src/lib.rs |
Adds the pool to test dependencies. |
rs/artifact_pool/src/upgrade_permit_auth_pool.rs |
Implements the in-memory pool. |
rs/artifact_pool/src/lib.rs |
Exports the pool module. |
Cargo.toml |
Registers the workspace crate. |
Cargo.lock |
Locks the new crate dependencies. |
Review details
Suppressed comments (2)
rs/consensus/upgrade/src/pool_manager.rs:77
- Advancing
last_scannedto the tip before processing makes every skipped or failed request permanent. In particular, a transient signing failure logged below is never retried on later polls, so this node may never contribute its required authorization share. Advance the cursor only past successfully processed work, or retain failed requests for retry.
*last = tip;
rs/consensus/upgrade/src/lib.rs:77
- This maps every signature-verification failure to
AuthorizeInvalidShare, including transient crypto errors. The manager then permanently removes the share; established consensus code instead defers non-reproducible errors (for examplers/consensus/idkg/src/pre_signer.rs:834-845). Preserve the crypto error classification so only reproducible failures invalidate the artifact and transient failures are retried.
.map_err(|_| InvalidUpgradePayloadReason::AuthorizeInvalidShare { signer })?;
- Files reviewed: 32/33 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+32
to
+35
| let registry_at_height: BTreeSet<NodeId> = membership | ||
| .get_nodes_at_version(block_registry_version) | ||
| .map(|nodes| nodes.into_iter().collect()) | ||
| .unwrap_or_default(); |
Comment on lines
+163
to
+167
| match validate_share( | ||
| share, | ||
| share.content.requestor, | ||
| share.content.request_height, | ||
| &membership, |
| pub canister_http: Vec<u8>, | ||
| pub query_stats: Vec<u8>, | ||
| pub chain_key: Vec<u8>, | ||
| pub upgrade: Vec<u8>, |
| assert_eq!( | ||
| hex::encode(hash.get_ref().0.as_slice()), | ||
| "764535296841f3db421a928cfadff3460be406d0182da64034eee623a9a97e99", | ||
| "c20a87578beb94df369dabfefc30c0d47d170c75d68236aae3b16335c0f21c4a", |
Contributor
There was a problem hiding this comment.
No, it should be fine, please don't do that. The reason this fired seems to be because test_block above returns a data block, when it should really be a summary block (which is unaffected by this change).
|
|
||
| /// Signs shares for requests in finalized blocks, validates gossiped shares, | ||
| /// and purges expired ones. | ||
| pub struct UpgradePermitAuthPoolManager { |
Comment on lines
+31
to
+32
| /// Requests we've already signed (node, request_height). | ||
| signed_requests: Mutex<BTreeSet<(NodeId, Height)>>, |
Comment on lines
+82
to
+86
| for height_num in start.get()..=tip.get() { | ||
| let height = Height::from(height_num); | ||
| let Ok(block) = chain.get_block_by_height(height) else { | ||
| continue; | ||
| }; |
| //! | ||
| //! 1. **Request**: A block maker includes `UpgradePermitAction::Request` in | ||
| //! its block when it wants to reboot. Validators check outstanding requests | ||
| //! the allowed max parallel reboots. |
| for Signed<UpgradePermitAuthorizationRequest, BasicSignature<UpgradePermitAuthorizationRequest>> | ||
| { | ||
| fn domain(&self) -> String { | ||
| DomainSeparator::UpgradePermitAuthorizationShare.to_string() |
eichhorl
reviewed
Sep 20, 2026
Comment on lines
+235
to
+240
| upgrade: if self.upgrade.is_empty() { | ||
| Vec::new() | ||
| } else { | ||
| bytes_to_upgrade_payload(&self.upgrade) | ||
| .map_err(IntoMessagesError::UpgradePayloadError)? | ||
| }, |
Contributor
There was a problem hiding this comment.
Don't think we need the condition here
| canister_http, | ||
| query_stats, | ||
| chain_key, | ||
| upgrade: _, |
Contributor
There was a problem hiding this comment.
We should also add a comment here if this test isn't applicable (like for xnet above)
| consensus_pool_cache: Arc<dyn ConsensusBlockCache>, | ||
| membership: Arc<Membership>, | ||
| /// Requests we've already signed (node, request_height). | ||
| signed_requests: Mutex<BTreeSet<(NodeId, Height)>>, |
Contributor
There was a problem hiding this comment.
Couldn't we just check the pool if a certain signature already exists?
Comment on lines
+87
to
+98
| let payload = block.payload.as_ref(); | ||
| if payload.is_summary() { | ||
| continue; | ||
| } | ||
| let upgrade_bytes = &payload.as_data().batch.upgrade; | ||
| if upgrade_bytes.is_empty() { | ||
| continue; | ||
| } | ||
| let Ok(actions) = bytes_to_upgrade_payload(upgrade_bytes) else { | ||
| continue; | ||
| }; | ||
| let membership = self.block_membership(block); |
Contributor
There was a problem hiding this comment.
Wasn't the plan to also add some data to the replicated state? I think that might simplify everything here. Usually we shouldn't need to look back at the blockchain and deserialize the payloads in the client (see for example the HTTP outcalls or chain-key signer implementations). There, the data flow is the following:
- A block maker adds some data to a block
- When the block is executed, this data triggers some changes to the replicated state
- The client/pool manager creates/validates new shares based on data in the replicated state
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.
First step of the Phase-2 rolling-reboot consensus protocol, which adds the data types and the artifact mechanism.
UpgradePermitAction(Request/Authorize/Return).UpgradePermitAuthorizationRequestis the signed content,UpgradePermitAuthorizationShareis the gossiped artifact.UpgradePermitAuthPoolImplwhich is modeled after other pools.ic-consensus-upgradecrate withUpgradePermitAuthPoolManager. It signs shares for requests in finalized blocks and validates gossiped shares.UpgradePermitAuthorizationRequest(signing) andUpgradePermitAuthorizationShare(pool-ID hashing) domain separators.