From f2a9a3b2e91bf4ecbe61ecae115bd5147371c746 Mon Sep 17 00:00:00 2001
From: Leo Eichhorn <99166915+eichhorl@users.noreply.github.com>
Date: Tue, 21 Jan 2025 13:32:58 +0100
Subject: [PATCH] refactor: CON-1434 Serialize current and next transcripts
 into `pb::NiDkgTranscript` (#3039)

As the `NiDkgId` (and therefore the `NiDkgTag`) is already part of the
`NiDkgTranscript`, serializing the `NiDkgTag` separately as part of
`pb::TaggedNiDkgTranscript` is redundant.

This PR instead serializes the current and next transcripts directly as
`pb::NiDkgTranscript`.

https://github.com/dfinity/ic/pull/2838 introduced the new block payload
proto fields of type `pb::NiDkgTranscript`, and implemented the
deserialization function to consider both new and old fields. This
ensures the backwards/forwards compatibility of the change made by this
PR.

During upgrade:
- The old summary still contains `pb::TaggedNiDkgTranscript`s, but they
can still be read as the deserialization function supports both types.
- Newly created summary blocks will instead contain
`pb::NiDkgTranscript`s directly.

During downgrade:
- As the deserialization function supporting both types already exists
in the previous version, Summary blocks created by the version of this
PR (using `pb::NiDkgTranscript`) will be deserialized successfully.
- Newly created summary blocks on the old version will then again
contain `pb::TaggedNiDkgTranscript`s instead.

Once this change is deployed to all mainnet subnets, we may remove the
deprecated `pb::TaggedNiDkgTranscript` fields entirely.


Additionally, this change forces us to fix a state machine setup
function in `rs/state_machine_tests/src/lib.rs`, which incorrectly
marked both genesis NiDkg transcripts as "high threshold". Therefore,
this test setup function broke the (now explicit) invariant that for
each (tag, transcript) in `block.dkg_transcript_map` it holds that `tag
== transcript.tag`.
---
 rs/state_machine_tests/src/lib.rs   |  8 ++++++--
 rs/types/types/src/consensus/dkg.rs | 25 ++++++++-----------------
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/rs/state_machine_tests/src/lib.rs b/rs/state_machine_tests/src/lib.rs
index 89545047162..d05a36ab8a6 100644
--- a/rs/state_machine_tests/src/lib.rs
+++ b/rs/state_machine_tests/src/lib.rs
@@ -422,9 +422,13 @@ fn make_nodes_registry(
         .build();
 
     // Insert initial DKG transcripts
+    let mut high_threshold_transcript = ni_dkg_transcript.clone();
+    high_threshold_transcript.dkg_id.dkg_tag = NiDkgTag::HighThreshold;
+    let mut low_threshold_transcript = ni_dkg_transcript;
+    low_threshold_transcript.dkg_id.dkg_tag = NiDkgTag::LowThreshold;
     let cup_contents = CatchUpPackageContents {
-        initial_ni_dkg_transcript_high_threshold: Some(ni_dkg_transcript.clone().into()),
-        initial_ni_dkg_transcript_low_threshold: Some(ni_dkg_transcript.into()),
+        initial_ni_dkg_transcript_high_threshold: Some(high_threshold_transcript.into()),
+        initial_ni_dkg_transcript_low_threshold: Some(low_threshold_transcript.into()),
         ..Default::default()
     };
     registry_data_provider
diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs
index 11615ed5719..5447b27b5cf 100644
--- a/rs/types/types/src/consensus/dkg.rs
+++ b/rs/types/types/src/consensus/dkg.rs
@@ -293,19 +293,12 @@ impl Summary {
     }
 }
 
-fn build_tagged_transcripts_vec(
+fn build_transcripts_vec(
     transcripts: &BTreeMap<NiDkgTag, NiDkgTranscript>,
-) -> Vec<pb::TaggedNiDkgTranscript> {
+) -> Vec<pb::NiDkgTranscript> {
     transcripts
-        .iter()
-        .map(|(tag, transcript)| pb::TaggedNiDkgTranscript {
-            tag: pb::NiDkgTag::from(tag) as i32,
-            transcript: Some(pb::NiDkgTranscript::from(transcript)),
-            key_id: match tag {
-                NiDkgTag::HighThresholdForKey(k) => Some(pb::MasterPublicKeyId::from(k)),
-                _ => None,
-            },
-        })
+        .values()
+        .map(pb::NiDkgTranscript::from)
         .collect()
 }
 
@@ -355,12 +348,10 @@ impl From<&Summary> for pb::Summary {
                 .values()
                 .map(pb::NiDkgConfig::from)
                 .collect(),
-            current_transcripts_deprecated: build_tagged_transcripts_vec(
-                &summary.current_transcripts,
-            ),
-            current_transcripts_new: Vec::new(),
-            next_transcripts_deprecated: build_tagged_transcripts_vec(&summary.next_transcripts),
-            next_transcripts_new: Vec::new(),
+            current_transcripts_new: build_transcripts_vec(&summary.current_transcripts),
+            current_transcripts_deprecated: Vec::new(),
+            next_transcripts_new: build_transcripts_vec(&summary.next_transcripts),
+            next_transcripts_deprecated: Vec::new(),
             interval_length: summary.interval_length.get(),
             next_interval_length: summary.next_interval_length.get(),
             height: summary.height.get(),