chore(dkg): remove summary's transcripts for remote subnets (1/4)

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),

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(),
     }
 }

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::<Vec<_>>()
             );
-            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) => {

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 {

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<SubnetId>,
 }
-/// 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<NiDkgTranscript>,
     #[prost(message, repeated, tag = "12")]
     pub next_transcripts: ::prost::alloc::vec::Vec<NiDkgTranscript>,
+    /// 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<bool>,
     #[prost(oneof = "summary::SubnetSplittingStatus", tags = "13, 14, 15")]
     pub subnet_splitting_status: ::core::option::Option<summary::SubnetSplittingStatus>,
 }

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<T, true>` 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<T, true>`, 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<T, false>`, 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<T, const SETTABLE: bool>(Option<T>);
 
@@ -89,9 +106,18 @@ impl<T, const SETTABLE: bool> BackwardsCompatible<T, SETTABLE> {
     /// populated the field is rolled back to a version that does not populate the field.
     pub fn try_from_proto<Proto: TryInto<T, Error = ProxyDecodeError>>(
         proto: Option<Proto>,
+    ) -> Result<Self, ProxyDecodeError> {
+        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>(
+        proto: Option<Proto>,
+        convert: impl FnOnce(Proto) -> Result<T, ProxyDecodeError>,
     ) -> Result<Self, ProxyDecodeError> {
         match proto {
-            Some(value) => Ok(Self(Some(value.try_into()?))),
+            Some(value) => Ok(Self(Some(convert(value)?))),
             None => Ok(Self(None)),
         }
     }

rs/types/types/src/consensus/dkg.rs

@@ -268,7 +268,7 @@ pub struct DkgSummary {
     #[serde_as(as = "Vec<(_, _)>")]
     next_transcripts: BTreeMap<NiDkgTag, NiDkgTranscript>,
     /// Transcripts that are computed for remote subnets.
-    pub transcripts_for_remote_subnets: Vec<RemoteTranscriptResult>,
+    pub transcripts_for_remote_subnets: BackwardsCompatible<Vec<RemoteTranscriptResult>, 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<pb::Summary> 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,