chore: CON-1453 Introduce a type for remote transcript results

rs/consensus/dkg/src/metrics.rs

@@ -115,9 +115,9 @@ impl From<&BlockPayload> for DkgPayloadStats {
                         .or_insert(1);
                 }
 
-                for (dkg_id, _, _) in &data_payload.dkg.transcripts_for_remote_subnets {
+                for transcript in &data_payload.dkg.transcripts_for_remote_subnets {
                     remote_transcripts_delivered
-                        .entry(dkg_id.dkg_tag.clone())
+                        .entry(transcript.dkg_id.dkg_tag.clone())
                         .and_modify(|count| *count += 1)
                         .or_insert(1);
                 }

rs/consensus/dkg/src/payload_builder.rs

@@ -1,5 +1,5 @@
 use crate::{
-    MAX_EARLY_REMOTE_TRANSCRIPTS, MAX_REMOTE_DKGS_PER_INTERVAL,
+    MAX_REMOTE_DKGS_PER_INTERVAL, MAX_REMOTE_TRANSCRIPTS_PER_PAYLOAD,
     metrics::{DkgPayloadMetrics, DkgPayloadMetricsOptionExt},
     remote::{
         ConfigResult, build_callback_id_config_map, get_updated_remote_dkg_attempts, merge_configs,
@@ -26,7 +26,10 @@ use ic_types::{
     batch::ValidationContext,
     consensus::{
         Block,
-        dkg::{DkgDataPayload, DkgPayload, DkgPayloadCreationError, DkgSummary, Message},
+        dkg::{
+            DkgDataPayload, DkgPayload, DkgPayloadCreationError, DkgSummary, Message,
+            RemoteTranscriptResult,
+        },
         get_faults_tolerated,
     },
     crypto::threshold_sig::ni_dkg::{
@@ -85,7 +88,7 @@ pub fn create_payload(
         .map(DkgPayload::Summary)
     } else {
         // If the height is not a start height, create a payload with new dealings,
-        // and possibly early remote transcripts.
+        // and possibly remote transcripts.
         create_data_payload(
             subnet_id,
             registry_client,
@@ -150,7 +153,7 @@ fn create_data_payload(
         max_dealings_per_block,
     );
 
-    let remote_dkg_transcripts = create_early_remote_transcripts(
+    let remote_dkg_transcripts = create_remote_transcripts(
         pool_reader,
         crypto,
         parent,
@@ -162,29 +165,27 @@ fn create_data_payload(
     if !remote_dkg_transcripts.is_empty() {
         info!(
             logger,
-            "Including {} early remote DKG transcripts in data block payload at height {}",
+            "Including {} remote DKG transcripts in data block payload at height {}",
             remote_dkg_transcripts.len(),
             parent.height.increment(),
         );
     }
 
-    // Include any early remote transcripts
     Ok(DkgDataPayload::new_with_remote_dkg_transcripts(
         last_summary_block.height,
         new_validated_dealings,
         remote_dkg_transcripts,
     ))
 }
 
-#[allow(clippy::type_complexity)]
-pub(crate) fn create_early_remote_transcripts(
+pub(crate) fn create_remote_transcripts(
     pool_reader: &PoolReader<'_>,
     crypto: &dyn ConsensusCrypto,
     parent: &Block,
     callback_id_map: BTreeMap<CallbackId, ConfigResult>,
     logger: &ReplicaLogger,
     dkg_payload_metrics: Option<&DkgPayloadMetrics>,
-) -> Result<Vec<(NiDkgId, CallbackId, Result<NiDkgTranscript, String>)>, DkgPayloadCreationError> {
+) -> Result<Vec<RemoteTranscriptResult>, DkgPayloadCreationError> {
     //  Since this function is relatively expensive, we simply return if there are no outstanding DKG contexts
     if callback_id_map.is_empty() {
         return Ok(vec![]);
@@ -207,8 +208,8 @@ pub(crate) fn create_early_remote_transcripts(
                 {
                     continue;
                 }
-                // Skip requests that would exceed the maximum number of early remote transcripts.
-                if selected_transcripts.len() + errs.len() > MAX_EARLY_REMOTE_TRANSCRIPTS {
+                // Skip requests that would exceed the maximum number of remote transcripts.
+                if selected_transcripts.len() + errs.len() > MAX_REMOTE_TRANSCRIPTS_PER_PAYLOAD {
                     continue;
                 }
                 // Reject contexts for which we failed to create configs.
@@ -223,16 +224,20 @@ pub(crate) fn create_early_remote_transcripts(
                     );
                     // Including the error in the payload will cause the context to receive
                     // a reject response.
-                    selected_transcripts.push((dkg_id, callback_id, Err(err)));
+                    selected_transcripts.push(RemoteTranscriptResult::new(
+                        dkg_id,
+                        callback_id,
+                        Err(err),
+                    ));
                 }
                 continue;
             }
         };
 
-        // Ensure that creating these transcripts would not exceed the maximum number of early
+        // Ensure that creating these transcripts would not exceed the maximum number of
         // remote transcripts. We continue with the next target_id in case it requires less
         // transcripts.
-        if selected_transcripts.len() + configs.len() > MAX_EARLY_REMOTE_TRANSCRIPTS {
+        if selected_transcripts.len() + configs.len() > MAX_REMOTE_TRANSCRIPTS_PER_PAYLOAD {
             continue;
         }
 
@@ -246,7 +251,7 @@ pub(crate) fn create_early_remote_transcripts(
             continue;
         }
 
-        // For each config, try to build the necessary (dkg_id, callback_id, transcript_result) triple
+        // For each config, try to build the necessary [`RemoteTranscriptResult`].
         for config in configs.iter() {
             let dealings = all_dealings.remove(config.dkg_id()).unwrap_or_else(|| {
                 dkg_payload_metrics
@@ -262,9 +267,7 @@ pub(crate) fn create_early_remote_transcripts(
             });
             // Generate the transcript. We need to retry transient errors, as a payload containing
             // transient errors may not be verifiable by peers.
-            let transcript_result = match NiDkgAlgorithm::create_transcript(
-                crypto, config, dealings,
-            ) {
+            let result = match NiDkgAlgorithm::create_transcript(crypto, config, dealings) {
                 Ok(transcript) => Ok(transcript),
                 // Note that we handled the reproducible error case of not having enough dealings
                 // already beforehand.
@@ -273,7 +276,7 @@ pub(crate) fn create_early_remote_transcripts(
                     // Including the error in the payload will cause the context to receive
                     // a reject response.
                     let error_message = format!(
-                        "Failed to create early remote transcript for dkg id {:?} at height {}: {}",
+                        "Failed to create remote transcript for dkg id {:?} at height {}: {}",
                         config.dkg_id(),
                         parent.height.increment(),
                         err
@@ -287,7 +290,11 @@ pub(crate) fn create_early_remote_transcripts(
                     return Err(DkgPayloadCreationError::DkgCreateTranscriptError(err));
                 }
             };
-            selected_transcripts.push((config.dkg_id().clone(), callback_id, transcript_result));
+            selected_transcripts.push(RemoteTranscriptResult::new(
+                config.dkg_id().clone(),
+                callback_id,
+                result,
+            ));
         }
     }
 
@@ -1185,9 +1192,11 @@ mod tests {
                     dkg_data
                         .transcripts_for_remote_subnets
                         .iter()
-                        .filter(|(dkg_id, _, result)| {
-                            dkg_id.target_subnet == NiDkgTargetSubnet::Remote(setup_target)
-                                && *result == Err(REMOTE_DKG_REPEATED_FAILURE_ERROR.to_string())
+                        .filter(|transcript| {
+                            transcript.dkg_id.target_subnet
+                                == NiDkgTargetSubnet::Remote(setup_target)
+                                && transcript.transcript_result
+                                    == Err(REMOTE_DKG_REPEATED_FAILURE_ERROR.to_string())
                         })
                         .count(),
                     2
@@ -1204,9 +1213,11 @@ mod tests {
                     dkg_data
                         .transcripts_for_remote_subnets
                         .iter()
-                        .filter(|(dkg_id, _, result)| {
-                            dkg_id.target_subnet == NiDkgTargetSubnet::Remote(reshare_target)
-                                && *result == Err(REMOTE_DKG_REPEATED_FAILURE_ERROR.to_string())
+                        .filter(|transcript| {
+                            transcript.dkg_id.target_subnet
+                                == NiDkgTargetSubnet::Remote(reshare_target)
+                                && transcript.transcript_result
+                                    == Err(REMOTE_DKG_REPEATED_FAILURE_ERROR.to_string())
                         })
                         .count(),
                     1

rs/consensus/dkg/src/payload_validator.rs

@@ -223,9 +223,9 @@ fn validate_dealings_payload(
         crypto_validate_dealing(crypto, config, message)?;
     }
 
-    // If we have early transcripts, we compare them
+    // If we have remote transcripts, we compare them
     if !dealings.transcripts_for_remote_subnets.is_empty() {
-        let expected_transcripts = payload_builder::create_early_remote_transcripts(
+        let expected_transcripts = payload_builder::create_remote_transcripts(
             pool_reader,
             crypto,
             parent,
@@ -237,16 +237,16 @@ fn validate_dealings_payload(
         if dealings.transcripts_for_remote_subnets != expected_transcripts {
             warn!(
                 log,
-                "Failed to validate {} early remote DKG transcripts in data block payload at height {}",
+                "Failed to validate {} remote DKG transcripts in data block payload at height {}",
                 dealings.transcripts_for_remote_subnets.len(),
                 parent.height.increment(),
             );
-            return Err(InvalidDkgPayloadReason::InvalidEarlyNiDkgTranscripts.into());
+            return Err(InvalidDkgPayloadReason::InvalidRemoteNiDkgTranscripts.into());
         }
 
         info!(
             log,
-            "Validated {} early remote DKG transcripts in data block payload at height {}",
+            "Validated {} remote DKG transcripts in data block payload at height {}",
             dealings.transcripts_for_remote_subnets.len(),
             parent.height.increment(),
         );
@@ -283,7 +283,7 @@ mod tests {
         batch::BatchPayload,
         consensus::{
             DataPayload, Payload,
-            dkg::{DealingContent, Message},
+            dkg::{DealingContent, Message, RemoteTranscriptResult},
             idkg,
         },
         crypto::threshold_sig::ni_dkg::{NiDkgId, NiDkgTag, NiDkgTargetSubnet},
@@ -506,8 +506,8 @@ mod tests {
     }
 
     #[test]
-    fn validate_dealings_payload_when_invalid_early_remote_transcripts_present_fails_test() {
-        // Data payloads with invalid early/remote transcripts are rejected.
+    fn validate_dealings_payload_when_invalid_remote_transcripts_present_fails_test() {
+        // Data payloads with invalid remote transcripts are rejected.
         ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| {
             let registry_version = 1;
             let committee = [NODE_1, NODE_2, NODE_3];
@@ -550,7 +550,7 @@ mod tests {
                 start_height: Height::from(0),
                 messages: vec![],
                 transcripts_for_remote_subnets: vec![
-                    (
+                    RemoteTranscriptResult::new(
                         NiDkgId {
                             start_block_height: Height::from(0),
                             dealer_subnet: SUBNET_1,
@@ -560,7 +560,7 @@ mod tests {
                         CallbackId::from(0),
                         Err("dummy".to_string()),
                     ),
-                    (
+                    RemoteTranscriptResult::new(
                         NiDkgId {
                             start_block_height: Height::from(0),
                             dealer_subnet: SUBNET_1,
@@ -594,7 +594,7 @@ mod tests {
                     &no_op_logger(),
                 ),
                 Err(DkgPayloadValidationError::InvalidArtifact(
-                    InvalidDkgPayloadReason::InvalidEarlyNiDkgTranscripts
+                    InvalidDkgPayloadReason::InvalidRemoteNiDkgTranscripts
                 ))
             );
         })

rs/consensus/dkg/src/test_utils.rs

@@ -16,13 +16,12 @@ use ic_types::{
     Height, NumberOfNodes, RegistryVersion,
     consensus::{
         BlockPayload,
-        dkg::{DealingContent, DealingMessages, Message},
+        dkg::{DealingContent, DealingMessages, Message, RemoteTranscriptResult},
     },
     crypto::threshold_sig::ni_dkg::{
-        NiDkgId, NiDkgTag, NiDkgTargetId, NiDkgTargetSubnet, NiDkgTranscript,
+        NiDkgId, NiDkgTag, NiDkgTargetId, NiDkgTargetSubnet,
         config::{NiDkgConfig, NiDkgConfigData},
     },
-    messages::CallbackId,
 };
 use std::{
     collections::{BTreeMap, BTreeSet},
@@ -103,7 +102,7 @@ pub(super) fn complement_state_manager_with_setup_initial_dkg_request(
 /// Extract the remote dkg transcripts from the current highest validated block
 pub(super) fn extract_remote_dkgs_from_highest_block(
     pool: &TestConsensusPool,
-) -> Vec<(NiDkgId, CallbackId, Result<NiDkgTranscript, String>)> {
+) -> Vec<RemoteTranscriptResult> {
     let block: ic_types::consensus::Block = pool
         .validated()
         .block_proposal()

rs/consensus/dkg/src/utils.rs

@@ -121,11 +121,11 @@ pub(super) fn get_dkg_dealings(
     {
         let payload = &block.payload.as_ref().as_data().dkg;
 
-        for (dkg_id, _, _) in payload.transcripts_for_remote_subnets.iter() {
+        for transcript in payload.transcripts_for_remote_subnets.iter() {
             // Add the finished DKG to excluded list
-            excluded.insert(dkg_id.clone());
+            excluded.insert(transcript.dkg_id.clone());
             // Remove already selected dealings
-            dealings.remove(dkg_id);
+            dealings.remove(&transcript.dkg_id);
         }
 
         for message in payload.messages.iter() {
@@ -206,7 +206,8 @@ mod tests {
         Height, RegistryVersion,
         batch::BatchPayload,
         consensus::{
-            Block, BlockPayload, BlockProposal, DataPayload, Payload, Rank, dkg::DkgDataPayload,
+            Block, BlockPayload, BlockProposal, DataPayload, Payload, Rank,
+            dkg::{DkgDataPayload, RemoteTranscriptResult},
             idkg,
         },
         crypto::{
@@ -371,7 +372,7 @@ mod tests {
                         dealings_included[0].clone(),
                         dealings_included[1].clone(),
                     ],
-                    transcripts_for_remote_subnets: vec![(
+                    transcripts_for_remote_subnets: vec![RemoteTranscriptResult::new(
                         dkg_id_with_transcript.clone(),
                         CallbackId::from(0),
                         Ok(dummy_transcript_for_tests()),