From a60f1abd69dd93d87495373f834b0499b4bbd7ee Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 19 Aug 2026 11:08:27 +0000 Subject: [PATCH 1/5] chore: add removed marker --- rs/consensus/dkg/src/lib.rs | 5 ++- rs/consensus/dkg/src/test_utils.rs | 2 +- rs/consensus/src/consensus/batch_delivery.rs | 8 ++-- rs/protobuf/def/types/v1/dkg.proto | 11 +++++- rs/protobuf/src/gen/types/types.v1.rs | 12 +++++- rs/types/types/src/backwards_compatibility.rs | 28 ++++++++++++- rs/types/types/src/consensus/dkg.rs | 39 ++++++++++++++----- 7 files changed, 87 insertions(+), 18 deletions(-) diff --git a/rs/consensus/dkg/src/lib.rs b/rs/consensus/dkg/src/lib.rs index be56990a6079..45e089324304 100644 --- a/rs/consensus/dkg/src/lib.rs +++ b/rs/consensus/dkg/src/lib.rs @@ -995,7 +995,10 @@ mod tests { for dkg_id in summary.dkg.configs.keys() { assert_eq!(dkg_id.target_subnet, NiDkgTargetSubnet::Local); } - assert_eq!(summary.dkg.transcripts_for_remote_subnets.len(), 0); + assert_eq!( + summary.dkg.transcripts_for_remote_subnets.as_ref(), + Some(&vec![]) + ); // Verify that the remote_dkg_attempts are set to `Completed`. assert_eq!( summary.dkg.remote_dkg_attempts.get(&target_id), diff --git a/rs/consensus/dkg/src/test_utils.rs b/rs/consensus/dkg/src/test_utils.rs index 7632acc8228c..2406cfc9752e 100644 --- a/rs/consensus/dkg/src/test_utils.rs +++ b/rs/consensus/dkg/src/test_utils.rs @@ -112,7 +112,7 @@ pub(super) fn extract_remote_dkgs_from_highest_block( .into_inner(); match block.payload.as_ref() { - BlockPayload::Summary(summary) => summary.dkg.transcripts_for_remote_subnets.clone(), + BlockPayload::Summary(_) => vec![], BlockPayload::Data(data) => data.dkg.transcripts_for_remote_subnets.clone(), } } diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 6773ccc1bba6..347d0713425b 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -331,10 +331,10 @@ fn generate_responses_to_subnet_calls( "New DKG summary with config ids created: {:?}", summary_payload.dkg.configs.keys().collect::>() ); - consensus_responses.append(&mut generate_responses_to_remote_dkgs( - &summary_payload.dkg.transcripts_for_remote_subnets, - log, - )); + if let Some(transcripts) = summary_payload.dkg.transcripts_for_remote_subnets.as_ref() { + consensus_responses + .append(&mut generate_responses_to_remote_dkgs(transcripts, log)); + } CanisterHttpSpent::default() } BlockPayload::Data(data_payload) => { diff --git a/rs/protobuf/def/types/v1/dkg.proto b/rs/protobuf/def/types/v1/dkg.proto index 217cf4536bbb..52d34e06a6af 100644 --- a/rs/protobuf/def/types/v1/dkg.proto +++ b/rs/protobuf/def/types/v1/dkg.proto @@ -38,7 +38,7 @@ message PostSplitArgs { SubnetId new_subnet_id = 1; } -// next id: 16 +// next id: 17 message Summary { reserved 5, 6, 8; reserved "transcripts_for_new_subnets"; @@ -56,6 +56,15 @@ message Summary { SplittingArgs scheduled = 14; PostSplitArgs post_split = 15; } + // Set by replica versions that no longer maintain `transcripts_for_remote_subnets` (field 10). + // + // When set, field 10 must be ignored entirely, including when hashing the summary. This is what + // allows the field to be removed without changing the hash of a summary: replica versions that + // still maintain the field and versions that have dropped it derive the same hash from the same + // wire bytes, because both read this marker. `repeated` fields cannot express the difference + // between "absent" and "empty" on the wire, hence the separate marker. See the documentation of + // `ic_types::backwards_compatibility::BackwardsCompatible` for the rollout this is part of. + optional bool transcripts_for_remote_subnets_removed = 16; } message CallbackIdedNiDkgTranscript { diff --git a/rs/protobuf/src/gen/types/types.v1.rs b/rs/protobuf/src/gen/types/types.v1.rs index 166ffe39f1b2..d895e1dbfe33 100644 --- a/rs/protobuf/src/gen/types/types.v1.rs +++ b/rs/protobuf/src/gen/types/types.v1.rs @@ -381,7 +381,7 @@ pub struct PostSplitArgs { #[prost(message, optional, tag = "1")] pub new_subnet_id: ::core::option::Option, } -/// next id: 16 +/// next id: 17 #[derive(Clone, PartialEq, ::prost::Message)] pub struct Summary { #[prost(uint64, tag = "1")] @@ -402,6 +402,16 @@ pub struct Summary { pub current_transcripts: ::prost::alloc::vec::Vec, #[prost(message, repeated, tag = "12")] pub next_transcripts: ::prost::alloc::vec::Vec, + /// Set by replica versions that no longer maintain `transcripts_for_remote_subnets` (field 10). + /// + /// When set, field 10 must be ignored entirely, including when hashing the summary. This is what + /// allows the field to be removed without changing the hash of a summary: replica versions that + /// still maintain the field and versions that have dropped it derive the same hash from the same + /// wire bytes, because both read this marker. `repeated` fields cannot express the difference + /// between "absent" and "empty" on the wire, hence the separate marker. See the documentation of + /// `ic_types::backwards_compatibility::BackwardsCompatible` for the rollout this is part of. + #[prost(bool, optional, tag = "16")] + pub transcripts_for_remote_subnets_removed: ::core::option::Option, #[prost(oneof = "summary::SubnetSplittingStatus", tags = "13, 14, 15")] pub subnet_splitting_status: ::core::option::Option, } diff --git a/rs/types/types/src/backwards_compatibility.rs b/rs/types/types/src/backwards_compatibility.rs index b2da4671f726..f1658f30caff 100644 --- a/rs/types/types/src/backwards_compatibility.rs +++ b/rs/types/types/src/backwards_compatibility.rs @@ -32,6 +32,23 @@ use std::hash::{Hash, Hasher}; /// 2. When the change is deployed to all replicas, we can switch the type to /// `BackwardsCompatible` and the field can begin to be populated. /// 3. When the change is deployed to all replicas, we can replace the type with `T`. +/// +/// Lifecycle of removing an existing field in a backwards compatible way, i.e. the above in +/// reverse: +/// 1. Replace the field's type `T` with `BackwardsCompatible`, keeping it populated. This +/// step is hash neutral, because `Some(v)` hashes like `v`. +/// 2. When the change is deployed to all replicas, switch the type to +/// `BackwardsCompatible`, so that the field is no longer populated. +/// 3. When the change is deployed to all replicas, remove the field. +/// +/// Step 2 is only hash neutral if every replica version derives the same `Option` from the same +/// protobuf. That holds when the protobuf can tell an absent field from a present one, which is +/// where the `None` of a newly added field comes from in the first place. A `repeated` field cannot: +/// empty and absent are the same bytes on the wire, and an empty collection is not hash invisible +/// the way `None` is, since `<[T]>::hash` writes a length prefix even for an empty slice. Removing a +/// collection field therefore needs an explicit presence marker on the wire, introduced in step 1 +/// and only set from step 2 onwards, so that both versions read the presence from the bytes rather +/// than deciding it by version. #[derive(Clone, Eq, PartialEq, Debug, Deserialize, Serialize)] pub struct BackwardsCompatible(Option); @@ -89,9 +106,18 @@ impl BackwardsCompatible { /// populated the field is rolled back to a version that does not populate the field. pub fn try_from_proto>( proto: Option, + ) -> Result { + Self::try_from_proto_with(proto, |p| p.try_into()) + } + + /// Like [`try_from_proto`](Self::try_from_proto), for protobuf values whose conversion is not + /// expressed as a `TryFrom` impl. + pub fn try_from_proto_with( + proto: Option, + convert: impl FnOnce(Proto) -> Result, ) -> Result { match proto { - Some(value) => Ok(Self(Some(value.try_into()?))), + Some(value) => Ok(Self(Some(convert(value)?))), None => Ok(Self(None)), } } diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index 8abf967bd22d..a3d62917a0c1 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -268,7 +268,7 @@ pub struct DkgSummary { #[serde_as(as = "Vec<(_, _)>")] next_transcripts: BTreeMap, /// Transcripts that are computed for remote subnets. - pub transcripts_for_remote_subnets: Vec, + pub transcripts_for_remote_subnets: BackwardsCompatible, true>, /// The length of the current interval in rounds (following the start /// block). pub interval_length: Height, @@ -301,7 +301,7 @@ impl DkgSummary { .collect(), current_transcripts, next_transcripts, - transcripts_for_remote_subnets: vec![], + transcripts_for_remote_subnets: BackwardsCompatible::new(vec![]), registry_version, interval_length, next_interval_length, @@ -453,9 +453,22 @@ impl From<&DkgSummary> for pb::Summary { interval_length: summary.interval_length.get(), next_interval_length: summary.next_interval_length.get(), height: summary.height.get(), - transcripts_for_remote_subnets: build_callback_ided_transcripts_vec( - summary.transcripts_for_remote_subnets.as_slice(), - ), + transcripts_for_remote_subnets: summary + .transcripts_for_remote_subnets + .as_ref() + .map(|t| build_callback_ided_transcripts_vec(t.as_slice())) + // `None` -> empty vector + .unwrap_or_default(), + // Relay the marker instead of only ever setting it for our own summaries: `prost` + // drops unknown fields, so a replica version that decodes a summary coming from a + // version which no longer maintains the field and re-encodes it would otherwise strip + // the marker, turning `None` back into `Some(vec![])` downstream and thereby changing + // the hash of that summary. + transcripts_for_remote_subnets_removed: summary + .transcripts_for_remote_subnets + .as_ref() + .is_none() + .then_some(true), remote_dkg_attempts: build_remote_dkg_attempts_vec(&summary.remote_dkg_attempts), subnet_splitting_status: summary .subnet_splitting_status @@ -614,10 +627,18 @@ impl TryFrom for DkgSummary { interval_length: Height::from(summary.interval_length), next_interval_length: Height::from(summary.next_interval_length), height: Height::from(summary.height), - transcripts_for_remote_subnets: build_transcripts_vec_from_pb( - summary.transcripts_for_remote_subnets, - ) - .map_err(ProxyDecodeError::Other)?, + transcripts_for_remote_subnets: BackwardsCompatible::try_from_proto_with( + // A set marker means the summary was produced by a replica version that no longer + // maintains the field, in which case the repeated field must be ignored entirely, + // including for hashing. Without the marker the repeated field is authoritative, + // even when empty: an empty vector still contributes its length prefix to the hash + // preimage, exactly as it did before the field became `BackwardsCompatible`. + (!summary + .transcripts_for_remote_subnets_removed + .unwrap_or_default()) + .then_some(summary.transcripts_for_remote_subnets), + |t| build_transcripts_vec_from_pb(t).map_err(ProxyDecodeError::Other), + )?, remote_dkg_attempts: build_remote_dkg_attempts_map(&summary.remote_dkg_attempts), subnet_splitting_status: BackwardsCompatible::try_from_proto( summary.subnet_splitting_status, From e334f074d8aaf751a8e3db1a884b4b4199443423 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 19 Aug 2026 15:54:36 +0000 Subject: [PATCH 2/5] docs --- rs/protobuf/def/types/v1/dkg.proto | 3 +-- rs/protobuf/src/gen/types/types.v1.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/rs/protobuf/def/types/v1/dkg.proto b/rs/protobuf/def/types/v1/dkg.proto index 52d34e06a6af..9c83226544be 100644 --- a/rs/protobuf/def/types/v1/dkg.proto +++ b/rs/protobuf/def/types/v1/dkg.proto @@ -62,8 +62,7 @@ message Summary { // allows the field to be removed without changing the hash of a summary: replica versions that // still maintain the field and versions that have dropped it derive the same hash from the same // wire bytes, because both read this marker. `repeated` fields cannot express the difference - // between "absent" and "empty" on the wire, hence the separate marker. See the documentation of - // `ic_types::backwards_compatibility::BackwardsCompatible` for the rollout this is part of. + // between "absent" and "empty" on the wire, hence the separate marker. optional bool transcripts_for_remote_subnets_removed = 16; } diff --git a/rs/protobuf/src/gen/types/types.v1.rs b/rs/protobuf/src/gen/types/types.v1.rs index d895e1dbfe33..a854f77658af 100644 --- a/rs/protobuf/src/gen/types/types.v1.rs +++ b/rs/protobuf/src/gen/types/types.v1.rs @@ -408,8 +408,7 @@ pub struct Summary { /// allows the field to be removed without changing the hash of a summary: replica versions that /// still maintain the field and versions that have dropped it derive the same hash from the same /// wire bytes, because both read this marker. `repeated` fields cannot express the difference - /// between "absent" and "empty" on the wire, hence the separate marker. See the documentation of - /// `ic_types::backwards_compatibility::BackwardsCompatible` for the rollout this is part of. + /// between "absent" and "empty" on the wire, hence the separate marker. #[prost(bool, optional, tag = "16")] pub transcripts_for_remote_subnets_removed: ::core::option::Option, #[prost(oneof = "summary::SubnetSplittingStatus", tags = "13, 14, 15")] From 398bad143d1935220caf585fc399fc490cc53339 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 19 Aug 2026 14:45:21 +0000 Subject: [PATCH 3/5] chore: `None` transcripts --- rs/consensus/dkg/src/lib.rs | 5 +---- rs/types/types/src/consensus/dkg.rs | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/rs/consensus/dkg/src/lib.rs b/rs/consensus/dkg/src/lib.rs index 45e089324304..1dbb90c71b7f 100644 --- a/rs/consensus/dkg/src/lib.rs +++ b/rs/consensus/dkg/src/lib.rs @@ -995,10 +995,7 @@ mod tests { for dkg_id in summary.dkg.configs.keys() { assert_eq!(dkg_id.target_subnet, NiDkgTargetSubnet::Local); } - assert_eq!( - summary.dkg.transcripts_for_remote_subnets.as_ref(), - Some(&vec![]) - ); + assert_eq!(summary.dkg.transcripts_for_remote_subnets.as_ref(), None); // Verify that the remote_dkg_attempts are set to `Completed`. assert_eq!( summary.dkg.remote_dkg_attempts.get(&target_id), diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index a3d62917a0c1..b0bfa590a014 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -268,7 +268,7 @@ pub struct DkgSummary { #[serde_as(as = "Vec<(_, _)>")] next_transcripts: BTreeMap, /// Transcripts that are computed for remote subnets. - pub transcripts_for_remote_subnets: BackwardsCompatible, true>, + pub transcripts_for_remote_subnets: BackwardsCompatible, false>, /// The length of the current interval in rounds (following the start /// block). pub interval_length: Height, @@ -301,7 +301,7 @@ impl DkgSummary { .collect(), current_transcripts, next_transcripts, - transcripts_for_remote_subnets: BackwardsCompatible::new(vec![]), + transcripts_for_remote_subnets: BackwardsCompatible::empty(), registry_version, interval_length, next_interval_length, From 6ea579536973b0fbfa566890d92162cb420c9dfc Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 19 Aug 2026 14:51:46 +0000 Subject: [PATCH 4/5] chore: remove field --- rs/consensus/dkg/src/lib.rs | 1 - rs/consensus/src/consensus/batch_delivery.rs | 4 --- rs/protobuf/def/types/v1/dkg.proto | 4 +-- rs/protobuf/src/gen/types/types.v1.rs | 2 -- rs/types/types/src/consensus/dkg.rs | 37 ++++---------------- 5 files changed, 8 insertions(+), 40 deletions(-) diff --git a/rs/consensus/dkg/src/lib.rs b/rs/consensus/dkg/src/lib.rs index 1dbb90c71b7f..b298c7d132b7 100644 --- a/rs/consensus/dkg/src/lib.rs +++ b/rs/consensus/dkg/src/lib.rs @@ -995,7 +995,6 @@ mod tests { for dkg_id in summary.dkg.configs.keys() { assert_eq!(dkg_id.target_subnet, NiDkgTargetSubnet::Local); } - assert_eq!(summary.dkg.transcripts_for_remote_subnets.as_ref(), None); // Verify that the remote_dkg_attempts are set to `Completed`. assert_eq!( summary.dkg.remote_dkg_attempts.get(&target_id), diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 347d0713425b..c5648c236df9 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -331,10 +331,6 @@ fn generate_responses_to_subnet_calls( "New DKG summary with config ids created: {:?}", summary_payload.dkg.configs.keys().collect::>() ); - if let Some(transcripts) = summary_payload.dkg.transcripts_for_remote_subnets.as_ref() { - consensus_responses - .append(&mut generate_responses_to_remote_dkgs(transcripts, log)); - } CanisterHttpSpent::default() } BlockPayload::Data(data_payload) => { diff --git a/rs/protobuf/def/types/v1/dkg.proto b/rs/protobuf/def/types/v1/dkg.proto index 9c83226544be..5942a164359c 100644 --- a/rs/protobuf/def/types/v1/dkg.proto +++ b/rs/protobuf/def/types/v1/dkg.proto @@ -40,15 +40,15 @@ message PostSplitArgs { // next id: 17 message Summary { - reserved 5, 6, 8; + reserved 5, 6, 8, 10; reserved "transcripts_for_new_subnets"; + reserved "transcripts_for_remote_subnets"; uint64 registry_version = 1; uint64 interval_length = 2; uint64 next_interval_length = 3; uint64 height = 4; repeated NiDkgConfig configs = 7; repeated RemoteDkgAttemptCount remote_dkg_attempts = 9; - repeated CallbackIdedNiDkgTranscript transcripts_for_remote_subnets = 10; repeated NiDkgTranscript current_transcripts = 11; repeated NiDkgTranscript next_transcripts = 12; oneof subnet_splitting_status { diff --git a/rs/protobuf/src/gen/types/types.v1.rs b/rs/protobuf/src/gen/types/types.v1.rs index a854f77658af..eaa7e4a65693 100644 --- a/rs/protobuf/src/gen/types/types.v1.rs +++ b/rs/protobuf/src/gen/types/types.v1.rs @@ -396,8 +396,6 @@ pub struct Summary { pub configs: ::prost::alloc::vec::Vec, #[prost(message, repeated, tag = "9")] pub remote_dkg_attempts: ::prost::alloc::vec::Vec, - #[prost(message, repeated, tag = "10")] - pub transcripts_for_remote_subnets: ::prost::alloc::vec::Vec, #[prost(message, repeated, tag = "11")] pub current_transcripts: ::prost::alloc::vec::Vec, #[prost(message, repeated, tag = "12")] diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index b0bfa590a014..dc37567246af 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -267,8 +267,6 @@ pub struct DkgSummary { /// corresponding to this tag. #[serde_as(as = "Vec<(_, _)>")] next_transcripts: BTreeMap, - /// Transcripts that are computed for remote subnets. - pub transcripts_for_remote_subnets: BackwardsCompatible, false>, /// The length of the current interval in rounds (following the start /// block). pub interval_length: Height, @@ -301,7 +299,6 @@ impl DkgSummary { .collect(), current_transcripts, next_transcripts, - transcripts_for_remote_subnets: BackwardsCompatible::empty(), registry_version, interval_length, next_interval_length, @@ -453,22 +450,12 @@ impl From<&DkgSummary> for pb::Summary { interval_length: summary.interval_length.get(), next_interval_length: summary.next_interval_length.get(), height: summary.height.get(), - transcripts_for_remote_subnets: summary - .transcripts_for_remote_subnets - .as_ref() - .map(|t| build_callback_ided_transcripts_vec(t.as_slice())) - // `None` -> empty vector - .unwrap_or_default(), - // Relay the marker instead of only ever setting it for our own summaries: `prost` - // drops unknown fields, so a replica version that decodes a summary coming from a - // version which no longer maintains the field and re-encodes it would otherwise strip - // the marker, turning `None` back into `Some(vec![])` downstream and thereby changing - // the hash of that summary. - transcripts_for_remote_subnets_removed: summary - .transcripts_for_remote_subnets - .as_ref() - .is_none() - .then_some(true), + // The marker must keep being set even though this replica version no longer reads it: + // replica versions that still carry the field derive it from this marker, and would + // otherwise decode the empty repeated field above into `Some(vec![])` and hash its + // length prefix, where this version hashes nothing. It may only stop being set once no + // replica version that reads it is deployed any more. + transcripts_for_remote_subnets_removed: Some(true), remote_dkg_attempts: build_remote_dkg_attempts_vec(&summary.remote_dkg_attempts), subnet_splitting_status: summary .subnet_splitting_status @@ -627,18 +614,6 @@ impl TryFrom for DkgSummary { interval_length: Height::from(summary.interval_length), next_interval_length: Height::from(summary.next_interval_length), height: Height::from(summary.height), - transcripts_for_remote_subnets: BackwardsCompatible::try_from_proto_with( - // A set marker means the summary was produced by a replica version that no longer - // maintains the field, in which case the repeated field must be ignored entirely, - // including for hashing. Without the marker the repeated field is authoritative, - // even when empty: an empty vector still contributes its length prefix to the hash - // preimage, exactly as it did before the field became `BackwardsCompatible`. - (!summary - .transcripts_for_remote_subnets_removed - .unwrap_or_default()) - .then_some(summary.transcripts_for_remote_subnets), - |t| build_transcripts_vec_from_pb(t).map_err(ProxyDecodeError::Other), - )?, remote_dkg_attempts: build_remote_dkg_attempts_map(&summary.remote_dkg_attempts), subnet_splitting_status: BackwardsCompatible::try_from_proto( summary.subnet_splitting_status, From e46b18f89523aec54419656779e1c04ac140174c Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 19 Aug 2026 16:07:00 +0000 Subject: [PATCH 5/5] docs --- rs/consensus/src/consensus/batch_delivery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index c5648c236df9..d4a724653b2f 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -315,7 +315,7 @@ pub(crate) fn deliver_batches_with_result_processor( /// This function creates responses to the system calls that are redirected to /// consensus. There are two types of calls being handled here: -/// - Initial NiDKG transcript creation, where a response may come from summary or data payloads. +/// - Initial NiDKG transcript creation, where a response may come from data payloads. /// - Canister threshold signature creation, where a response may come from from data payloads. /// - CanisterHttpResponse handling, where a response to a canister http request may come from data payloads. fn generate_responses_to_subnet_calls(