Merge branch 'kpop/remove_option_quadruple_id_2' into 'master'

chore(ecdsa):[CON-1119] Replace `Option<EcdsaKeyId>` with just `EcdsaKeyId` in Quadruples

Also removed `key_id` from `QuadrupleId`, since it's safe now. 

See merge request dfinity-lab/public/ic!19015

rs/consensus/src/consensus/metrics.rs

@@ -158,8 +158,7 @@ impl BatchStats {
     }
 }
 
-// TODO(kpop): remove this Option eventually
-type CounterPerEcdsaKeyId = BTreeMap<Option<EcdsaKeyId>, usize>;
+type CounterPerEcdsaKeyId = BTreeMap<EcdsaKeyId, usize>;
 
 // Ecdsa payload stats
 pub struct EcdsaStats {
@@ -186,12 +185,12 @@ impl From<&EcdsaPayload> for EcdsaStats {
                 && payload.idkg_transcripts.get(transcript_id).is_some()
             {
                 *key_transcript_created
-                    .entry(Some(payload.key_transcript.key_id.clone()))
+                    .entry(payload.key_transcript.key_id.clone())
                     .or_default() += 1;
             }
         }
 
-        let keys = vec![None, Some(payload.key_transcript.key_id.clone())];
+        let keys = vec![payload.key_transcript.key_id.clone()];
 
         Self {
             key_transcript_created,
@@ -200,9 +199,12 @@ impl From<&EcdsaPayload> for EcdsaStats {
                 .values()
                 .filter(|status| matches!(status, CompletedSignature::Unreported(_)))
                 .count(),
-            available_quadruples: count_by_ecdsa_key_id(payload.available_quadruples.keys(), &keys),
+            available_quadruples: count_by_ecdsa_key_id(
+                payload.available_quadruples.values(),
+                &keys,
+            ),
             quadruples_in_creation: count_by_ecdsa_key_id(
-                payload.quadruples_in_creation.keys(),
+                payload.quadruples_in_creation.values(),
                 &keys,
             ),
             ongoing_xnet_reshares: count_by_ecdsa_key_id(
@@ -222,7 +224,7 @@ impl From<&EcdsaPayload> for EcdsaStats {
 
 fn count_by_ecdsa_key_id<T: HasEcdsaKeyId>(
     collection: impl Iterator<Item = T>,
-    expected_keys: &Vec<Option<EcdsaKeyId>>,
+    expected_keys: &[EcdsaKeyId],
 ) -> CounterPerEcdsaKeyId {
     let mut counter_per_key_id = CounterPerEcdsaKeyId::new();
 
@@ -233,9 +235,7 @@ fn count_by_ecdsa_key_id<T: HasEcdsaKeyId>(
     }
 
     for item in collection {
-        *counter_per_key_id
-            .entry(item.key_id().cloned())
-            .or_default() += 1;
+        *counter_per_key_id.entry(item.key_id().clone()).or_default() += 1;
     }
 
     counter_per_key_id
@@ -366,15 +366,15 @@ impl FinalizerMetrics {
             let set = |metric: &IntGaugeVec, counts: &CounterPerEcdsaKeyId| {
                 for (key_id, count) in counts.iter() {
                     metric
-                        .with_label_values(&[&key_id_label(key_id.as_ref())])
+                        .with_label_values(&[&key_id_label(Some(key_id))])
                         .set(*count as i64);
                 }
             };
 
             let inc_by = |metric: &IntCounterVec, counts: &CounterPerEcdsaKeyId| {
                 for (key_id, count) in counts.iter() {
                     metric
-                        .with_label_values(&[&key_id_label(key_id.as_ref())])
+                        .with_label_values(&[&key_id_label(Some(key_id))])
                         .inc_by(*count as u64);
                 }
             };
@@ -803,19 +803,19 @@ impl EcdsaPayloadMetrics {
     }
 
     pub(crate) fn report(&self, payload: &EcdsaPayload) {
-        let expected_keys = vec![None, Some(payload.key_transcript.key_id.clone())];
+        let expected_keys = vec![payload.key_transcript.key_id.clone()];
 
         self.payload_metrics_set_without_key_id_label(
             "signature_agreements",
             payload.signature_agreements.len(),
         );
         self.payload_metrics_set(
             "available_quadruples",
-            count_by_ecdsa_key_id(payload.available_quadruples.keys(), &expected_keys),
+            count_by_ecdsa_key_id(payload.available_quadruples.values(), &expected_keys),
         );
         self.payload_metrics_set(
             "quaruples_in_creation",
-            count_by_ecdsa_key_id(payload.quadruples_in_creation.keys(), &expected_keys),
+            count_by_ecdsa_key_id(payload.quadruples_in_creation.values(), &expected_keys),
         );
         self.payload_metrics_set(
             "ongoing_xnet_reshares",
@@ -836,7 +836,7 @@ impl EcdsaPayloadMetrics {
     fn payload_metrics_set(&self, label: &str, values: CounterPerEcdsaKeyId) {
         for (key_id, value) in values {
             self.payload_metrics
-                .with_label_values(&[label, &key_id_label(key_id.as_ref())])
+                .with_label_values(&[label, &key_id_label(Some(&key_id))])
                 .set(value as i64);
         }
     }

rs/consensus/src/ecdsa/payload_builder.rs

@@ -483,7 +483,7 @@ pub(crate) fn create_data_payload(
         {
             ecdsa_payload_metrics.payload_metrics_inc(
                 "key_transcripts_created",
-                ecdsa_payload.key_transcript.key_id(),
+                Some(ecdsa_payload.key_transcript.key_id()),
             );
         }
 

rs/consensus/src/ecdsa/payload_verifier.rs

@@ -1084,7 +1084,7 @@ mod test {
             curr_payload.available_quadruples.insert(
                 curr_payload.uid_generator.next_quadruple_id(),
                 PreSignatureQuadrupleRef {
-                    key_id: Some(curr_payload.key_transcript.key_id.clone()),
+                    key_id: curr_payload.key_transcript.key_id.clone(),
                     kappa_unmasked_ref: malicious_transcript_ref,
                     lambda_masked_ref: masked_transcript_1,
                     kappa_times_lambda_ref: masked_transcript_1,

rs/consensus/src/ecdsa/signer.rs

@@ -18,7 +18,7 @@ use ic_replicated_state::ReplicatedState;
 use ic_types::artifact::EcdsaMessageId;
 use ic_types::consensus::idkg::{
     ecdsa::ThresholdEcdsaSigInputsRef, sig_share_prefix, EcdsaBlockReader, EcdsaMessage,
-    EcdsaSigShare, EcdsaStats, HasEcdsaKeyId, RequestId,
+    EcdsaSigShare, EcdsaStats, RequestId,
 };
 use ic_types::crypto::canister_threshold_sig::{
     error::ThresholdEcdsaCombineSigSharesError, ThresholdEcdsaCombinedSignature,
@@ -516,7 +516,7 @@ impl<'a> EcdsaSignatureBuilderImpl<'a> {
             },
             |combined_signature| {
                 self.metrics
-                    .payload_metrics_inc("signatures_completed", request_id.key_id());
+                    .payload_metrics_inc("signatures_completed", None);
                 Some(combined_signature)
             },
         )

rs/consensus/src/ecdsa/test_utils.rs

@@ -264,7 +264,7 @@ impl From<&ThresholdEcdsaSigInputs> for TestSigInputs {
             hashed_message: inputs.hashed_message().try_into().unwrap(),
             nonce: *inputs.nonce(),
             presig_quadruple_ref: PreSignatureQuadrupleRef {
-                key_id: Some(fake_ecdsa_key_id()),
+                key_id: fake_ecdsa_key_id(),
                 kappa_unmasked_ref: UnmaskedTranscript::try_from((height, quad.kappa_unmasked()))
                     .unwrap(),
                 lambda_masked_ref: MaskedTranscript::try_from((height, quad.lambda_masked()))

rs/consensus/utils/src/lib.rs

@@ -833,7 +833,7 @@ mod tests {
         key_unmasked.transcript_id = fake_transcript_id(id + 4);
         let h = Height::from(0);
         PreSignatureQuadrupleRef {
-            key_id: Some(EcdsaKeyId::from_str("Secp256k1:some_key").unwrap()),
+            key_id: EcdsaKeyId::from_str("Secp256k1:some_key").unwrap(),
             kappa_unmasked_ref: UnmaskedTranscript::try_from((h, &kappa_unmasked)).unwrap(),
             lambda_masked_ref: MaskedTranscript::try_from((h, &lambda_masked)).unwrap(),
             kappa_times_lambda_ref: MaskedTranscript::try_from((h, &kappa_times_lambda)).unwrap(),

rs/protobuf/def/types/v1/idkg.proto

@@ -50,17 +50,17 @@ message OngoingSignature {
 }
 
 message AvailableQuadruple {
+  reserved 3;
+  reserved "key_id";
   uint64 quadruple_id = 1;
   PreSignatureQuadrupleRef quadruple = 2;
-  // Deprecated. Use `quadruple.key_id` instead.
-  registry.crypto.v1.EcdsaKeyId key_id = 3;
 }
 
 message QuadrupleInProgress {
+  reserved 3;
+  reserved "key_id";
   uint64 quadruple_id = 1;
   QuadrupleInCreation quadruple = 2;
-  // Deprecated. Use `quadruple.key_id` instead.
-  registry.crypto.v1.EcdsaKeyId key_id = 3;
 }
 
 message AvailablePreSignature {
@@ -86,10 +86,11 @@ message XnetReshareAgreement {
 }
 
 message RequestId {
+  reserved 4;
+  reserved "key_id";
   bytes pseudo_random_id = 1;
   uint64 quadruple_id = 2;
   uint64 height = 3;
-  registry.crypto.v1.EcdsaKeyId key_id = 4;
 }
 
 message TranscriptRef {

rs/protobuf/src/gen/types/types.v1.rs

@@ -536,9 +536,6 @@ pub struct AvailableQuadruple {
     pub quadruple_id: u64,
     #[prost(message, optional, tag = "2")]
     pub quadruple: ::core::option::Option<PreSignatureQuadrupleRef>,
-    /// Deprecated. Use `quadruple.key_id` instead.
-    #[prost(message, optional, tag = "3")]
-    pub key_id: ::core::option::Option<super::super::registry::crypto::v1::EcdsaKeyId>,
 }
 #[derive(serde::Serialize, serde::Deserialize)]
 #[allow(clippy::derive_partial_eq_without_eq)]
@@ -548,9 +545,6 @@ pub struct QuadrupleInProgress {
     pub quadruple_id: u64,
     #[prost(message, optional, tag = "2")]
     pub quadruple: ::core::option::Option<QuadrupleInCreation>,
-    /// Deprecated. Use `quadruple.key_id` instead.
-    #[prost(message, optional, tag = "3")]
-    pub key_id: ::core::option::Option<super::super::registry::crypto::v1::EcdsaKeyId>,
 }
 #[derive(serde::Serialize, serde::Deserialize)]
 #[allow(clippy::derive_partial_eq_without_eq)]
@@ -598,8 +592,6 @@ pub struct RequestId {
     pub quadruple_id: u64,
     #[prost(uint64, tag = "3")]
     pub height: u64,
-    #[prost(message, optional, tag = "4")]
-    pub key_id: ::core::option::Option<super::super::registry::crypto::v1::EcdsaKeyId>,
 }
 #[derive(serde::Serialize, serde::Deserialize)]
 #[allow(clippy::derive_partial_eq_without_eq)]

rs/types/types/src/consensus/idkg.rs

@@ -1462,7 +1462,6 @@ impl From<&EcdsaPayload> for pb::EcdsaPayload {
         for (quadruple_id, quadruple) in &payload.available_quadruples {
             available_quadruples.push(pb::AvailableQuadruple {
                 quadruple_id: quadruple_id.id(),
-                key_id: quadruple_id.key_id().map(Into::into),
                 quadruple: Some(quadruple.into()),
             });
         }
@@ -1472,7 +1471,6 @@ impl From<&EcdsaPayload> for pb::EcdsaPayload {
         for (quadruple_id, quadruple) in &payload.quadruples_in_creation {
             quadruples_in_creation.push(pb::QuadrupleInProgress {
                 quadruple_id: quadruple_id.id(),
-                key_id: quadruple_id.key_id().map(Into::into),
                 quadruple: Some(quadruple.into()),
             });
         }
@@ -1608,20 +1606,15 @@ impl TryFrom<&pb::EcdsaPayload> for EcdsaPayload {
         // available_quadruples
         let mut available_quadruples = BTreeMap::new();
         for available_quadruple in &payload.available_quadruples {
-            let key_id = available_quadruple
-                .key_id
-                .clone()
-                .map(TryInto::try_into)
-                .transpose()?;
-            let quadruple_id = QuadrupleId(available_quadruple.quadruple_id, key_id);
+            let quadruple_id = QuadrupleId(available_quadruple.quadruple_id);
             let quadruple: PreSignatureQuadrupleRef = try_from_option_field(
                 available_quadruple.quadruple.as_ref(),
                 "EcdsaPayload::available_quadruple::quadruple",
             )?;
             available_quadruples.insert(quadruple_id, quadruple);
         }
         for available_pre_signature in &payload.available_pre_signatures {
-            let pre_signature_id = QuadrupleId(available_pre_signature.pre_signature_id, None);
+            let pre_signature_id = QuadrupleId(available_pre_signature.pre_signature_id);
             let pre_signature: PreSignatureRef = try_from_option_field(
                 available_pre_signature.pre_signature.as_ref(),
                 "EcdsaPayload::available_pre_signature::pre_signature",
@@ -1637,20 +1630,15 @@ impl TryFrom<&pb::EcdsaPayload> for EcdsaPayload {
         // quadruples_in_creation
         let mut quadruples_in_creation = BTreeMap::new();
         for quadruple_in_creation in &payload.quadruples_in_creation {
-            let key_id = quadruple_in_creation
-                .key_id
-                .clone()
-                .map(TryInto::try_into)
-                .transpose()?;
-            let quadruple_id = QuadrupleId(quadruple_in_creation.quadruple_id, key_id);
+            let quadruple_id = QuadrupleId(quadruple_in_creation.quadruple_id);
             let quadruple: QuadrupleInCreation = try_from_option_field(
                 quadruple_in_creation.quadruple.as_ref(),
                 "EcdsaPayload::quadruple_in_creation::quadruple",
             )?;
             quadruples_in_creation.insert(quadruple_id, quadruple);
         }
         for pre_signature_in_creation in &payload.pre_signatures_in_creation {
-            let pre_signature_id = QuadrupleId(pre_signature_in_creation.pre_signature_id, None);
+            let pre_signature_id = QuadrupleId(pre_signature_in_creation.pre_signature_id);
             let pre_signature: PreSignatureInCreation = try_from_option_field(
                 pre_signature_in_creation.pre_signature.as_ref(),
                 "EcdsaPayload::pre_signature_in_creation::pre_signature",
@@ -1865,42 +1853,41 @@ impl From<&EcdsaMessage> for EcdsaArtifactId {
 
 pub trait HasEcdsaKeyId {
     /// Returns a reference to the [`EcdsaKeyId`] associated with the object.
-    // TODO(kpop): remove the Option once it's safe
-    fn key_id(&self) -> Option<&EcdsaKeyId>;
+    fn key_id(&self) -> &EcdsaKeyId;
 }
 
-impl HasEcdsaKeyId for QuadrupleId {
-    fn key_id(&self) -> Option<&EcdsaKeyId> {
-        self.key_id()
+impl HasEcdsaKeyId for QuadrupleInCreation {
+    fn key_id(&self) -> &EcdsaKeyId {
+        &self.key_id
     }
 }
 
-impl HasEcdsaKeyId for EcdsaReshareRequest {
-    fn key_id(&self) -> Option<&EcdsaKeyId> {
-        Some(&self.key_id)
+impl HasEcdsaKeyId for PreSignatureQuadrupleRef {
+    fn key_id(&self) -> &EcdsaKeyId {
+        &self.key_id
     }
 }
 
-impl HasEcdsaKeyId for RequestId {
-    fn key_id(&self) -> Option<&EcdsaKeyId> {
-        self.quadruple_id.key_id()
+impl HasEcdsaKeyId for EcdsaReshareRequest {
+    fn key_id(&self) -> &EcdsaKeyId {
+        &self.key_id
     }
 }
 
 impl HasEcdsaKeyId for EcdsaKeyTranscript {
-    fn key_id(&self) -> Option<&EcdsaKeyId> {
-        Some(&self.key_id)
+    fn key_id(&self) -> &EcdsaKeyId {
+        &self.key_id
     }
 }
 
 impl<T: HasEcdsaKeyId, U> HasEcdsaKeyId for (T, U) {
-    fn key_id(&self) -> Option<&EcdsaKeyId> {
+    fn key_id(&self) -> &EcdsaKeyId {
         self.0.key_id()
     }
 }
 
 impl<T: HasEcdsaKeyId> HasEcdsaKeyId for &T {
-    fn key_id(&self) -> Option<&EcdsaKeyId> {
+    fn key_id(&self) -> &EcdsaKeyId {
         (*self).key_id()
     }
 }