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..9c83226544be 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,14 @@ 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. + 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..a854f77658af 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,15 @@ 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. + #[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,