fix: CON-1466 Don't panic in get_dkg_summary_from_cup_contents (#3974)

This function is called by the orchestrator to make the latest registry CUP every ~10 seconds. Instead of panicking when the NiDKG summary cannot be created, we should return an error and handle it in the caller.
dfinity · Feb 17, 2025 · 20b675c · 20b675c
1 parent 67f5ede
commit 20b675c
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 46 deletions.
diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs
@@ -464,7 +464,7 @@ pub fn get_dkg_summary_from_cup_contents(
     subnet_id: SubnetId,
     registry: &dyn RegistryClient,
     registry_version: RegistryVersion,
-) -> Summary {
+) -> Result<Summary, String> {
     // If we're in a NNS subnet recovery case with failover nodes, we extract the registry of the
     // NNS we're recovering.
     let registry_version_of_original_registry = cup_contents
@@ -478,55 +478,51 @@ pub fn get_dkg_summary_from_cup_contents(
         NiDkgTag::LowThreshold,
         cup_contents
             .initial_ni_dkg_transcript_low_threshold
+            .ok_or("Missing initial low-threshold DKG transcript".to_string())
             .map(|dkg_transcript_record| {
-                initial_ni_dkg_transcript_from_registry_record(dkg_transcript_record)
-                    .expect("Decoding initial low-threshold DKG transcript failed.")
-            })
-            .expect("Missing initial low-threshold DKG transcript"),
+                initial_ni_dkg_transcript_from_registry_record(dkg_transcript_record).map_err(
+                    |err| format!("Decoding initial low-threshold DKG transcript failed: {err}"),
+                )
+            })??,
     );
     transcripts.insert(
         NiDkgTag::HighThreshold,
         cup_contents
             .initial_ni_dkg_transcript_high_threshold
+            .ok_or("Missing initial high-threshold DKG transcript".to_string())
             .map(|dkg_transcript_record| {
-                initial_ni_dkg_transcript_from_registry_record(dkg_transcript_record)
-                    .expect("Decoding initial high-threshold DKG transcript failed.")
-            })
-            .expect("Missing initial high-threshold DKG transcript"),
+                initial_ni_dkg_transcript_from_registry_record(dkg_transcript_record).map_err(
+                    |err| format!("Decoding initial high-threshold DKG transcript failed: {err}"),
+                )
+            })??,
     );
 
     // Get the transcripts for vetkeys from the `chain_key_initializations`
-    let mut vet_key_transcripts = cup_contents
-        .chain_key_initializations
-        .into_iter()
-        .filter_map(|init| {
-            let key_id = init.key_id.expect("Initialization without a key id");
-            let init = init.initialization.expect("Empty initialization");
-
-            // IDkg initializations are handled in a different place. This is to include NiDkgTranscripts into the Summary only
-            let Initialization::TranscriptRecord(record) = init else {
-                return None;
-            };
-
-            let key_id = NiDkgMasterPublicKeyId::try_from(key_id)
-                .expect("IDkg key combined with NiDkg initialization");
-
-            let transcript =
-                initial_ni_dkg_transcript_from_registry_record(record).unwrap_or_else(|_| {
-                    panic!("Decoding high-threshold DKG for key-id {} failed.", key_id)
-                });
-
-            Some((NiDkgTag::HighThresholdForKey(key_id), transcript))
-        })
-        .collect::<BTreeMap<_, _>>();
+    for init in cup_contents.chain_key_initializations.into_iter() {
+        let key_id = init
+            .key_id
+            .ok_or("Initialization without a key id".to_string())?;
+        let init = init
+            .initialization
+            .ok_or("Empty initialization".to_string())?;
+        // IDkg initializations are handled in a different place. This is to include NiDkgTranscripts into the Summary only
+        let Initialization::TranscriptRecord(record) = init else {
+            continue;
+        };
+        let key_id = NiDkgMasterPublicKeyId::try_from(key_id)
+            .map_err(|err| format!("IDkg key combined with NiDkg initialization: {err}"))?;
+        let transcript = initial_ni_dkg_transcript_from_registry_record(record).map_err(|err| {
+            format!(
+                "Decoding high-threshold DKG for key-id {} failed: {}",
+                key_id, err
+            )
+        })?;
+        transcripts.insert(NiDkgTag::HighThresholdForKey(key_id), transcript);
+    }
 
     // Extract vet key ids
-
     let vet_key_ids = vetkd_key_ids_for_subnet(subnet_id, registry, registry_version)
-        .expect("Failed to get vetkeys");
-
-    // Add vet key transcripts to the summary block
-    transcripts.append(&mut vet_key_transcripts);
+        .map_err(|err| format!("Failed to get vetKD key IDs: {err:?}"))?;
 
     // If we're in a NNS subnet recovery with failover nodes, we set the transcript versions to the
     // registry version of the recovered NNS, otherwise the oldest registry version used in a CUP is
@@ -538,7 +534,7 @@ pub fn get_dkg_summary_from_cup_contents(
     }
 
     let committee = get_node_list(subnet_id, registry, registry_version)
-        .expect("Could not retrieve committee list");
+        .map_err(|err| format!("Could not retrieve committee list: {err:?}"))?;
 
     let height = Height::from(cup_contents.height);
     let configs = get_configs_for_local_transcripts(
@@ -552,13 +548,16 @@ pub fn get_dkg_summary_from_cup_contents(
         registry_version_of_original_registry.unwrap_or(registry_version),
         &vet_key_ids,
     )
-    .expect("Couldn't generate configs for the genesis summary");
+    .map_err(|err| format!("Couldn't generate configs for the genesis summary: {err:?}"))?;
+
     // For the first 2 intervals we use the length value contained in the
     // genesis subnet record.
-    let interval_length = get_dkg_interval_length(registry, registry_version, subnet_id)
-        .expect("Could not retrieve the interval length for the genesis summary.");
+    let interval_length =
+        get_dkg_interval_length(registry, registry_version, subnet_id).map_err(|err| {
+            format!("Could not retrieve the interval length for the genesis summary: {err:?}")
+        })?;
     let next_interval_length = interval_length;
-    Summary::new(
+    Ok(Summary::new(
         configs,
         transcripts,
         BTreeMap::new(), // next transcripts
@@ -570,7 +569,7 @@ pub fn get_dkg_summary_from_cup_contents(
         next_interval_length,
         height,
         BTreeMap::new(), // initial_dkg_attempts
-    )
+    ))
 }
 
 /// Creates DKG configs for the local subnet for the next DKG intervals.
@@ -875,7 +874,8 @@ pub fn make_genesis_summary(
                     subnet_id,
                     registry,
                     summary_registry_version,
-                );
+                )
+                .expect("Failed to get NiDKG summary from CUP contents");
             }
             _ => {
                 if backoff > max_backoff {

diff --git a/rs/consensus/src/cup_utils.rs b/rs/consensus/src/cup_utils.rs
@@ -46,12 +46,22 @@ pub fn make_registry_cup_from_cup_contents(
             return None;
         }
     };
-    let dkg_summary = get_dkg_summary_from_cup_contents(
+    let dkg_summary = match get_dkg_summary_from_cup_contents(
         cup_contents.clone(),
         subnet_id,
         registry,
         registry_version,
-    );
+    ) {
+        Ok(summary) => summary,
+        Err(err) => {
+            warn!(
+                logger,
+                "Failed constructing NiDKG summary block from CUP contents: {}.", err
+            );
+
+            return None;
+        }
+    };
     let cup_height = Height::new(cup_contents.height);
 
     let idkg_summary = match bootstrap_idkg_summary(