From 30bf45e80e6b5c1660cd12c6b554d4f1e85a2d11 Mon Sep 17 00:00:00 2001 From: Leo Eichhorn Date: Mon, 13 May 2024 15:28:03 +0000 Subject: [PATCH] fix(schnorr): Revert 'CON-1255 Make `MasterPublicKey` in `EcdsaReshareRequest` mandatory' --- rs/consensus/src/ecdsa/payload_builder.rs | 6 ++--- .../src/ecdsa/payload_builder/resharing.rs | 12 ++++----- rs/consensus/src/ecdsa/test_utils.rs | 10 +++---- rs/types/types/src/consensus/idkg.rs | 27 ++++++++----------- rs/types/types/src/exhaustive.rs | 4 +-- 5 files changed, 25 insertions(+), 34 deletions(-) diff --git a/rs/consensus/src/ecdsa/payload_builder.rs b/rs/consensus/src/ecdsa/payload_builder.rs index 16f20c7eee9..3b47a7b4b81 100644 --- a/rs/consensus/src/ecdsa/payload_builder.rs +++ b/rs/consensus/src/ecdsa/payload_builder.rs @@ -288,9 +288,9 @@ fn create_summary_payload_helper( .retain(|_, pre_sig| !new_key_transcripts.contains(&pre_sig.key_id())); // This will clear the current ongoing reshares, and the execution requests will be restarted // with the new key and different transcript IDs. - ecdsa_summary - .ongoing_xnet_reshares - .retain(|request, _| !new_key_transcripts.contains(&request.master_key_id)); + ecdsa_summary.ongoing_xnet_reshares.retain(|request, _| { + !new_key_transcripts.contains(&MasterPublicKeyId::Ecdsa(request.key_id.clone())) + }); ecdsa_summary.uid_generator.update_height(height)?; update_summary_refs(height, &mut ecdsa_summary, block_reader)?; diff --git a/rs/consensus/src/ecdsa/payload_builder/resharing.rs b/rs/consensus/src/ecdsa/payload_builder/resharing.rs index 20e6edaac3c..202cf9e4030 100644 --- a/rs/consensus/src/ecdsa/payload_builder/resharing.rs +++ b/rs/consensus/src/ecdsa/payload_builder/resharing.rs @@ -159,8 +159,8 @@ fn reshare_request_from_dealings_context( context: &EcdsaDealingsContext, ) -> idkg::EcdsaReshareRequest { idkg::EcdsaReshareRequest { - key_id: Some(context.key_id.clone()), - master_key_id: MasterPublicKeyId::Ecdsa(context.key_id.clone()), + key_id: context.key_id.clone(), + master_key_id: Some(MasterPublicKeyId::Ecdsa(context.key_id.clone())), receiving_node_ids: context.nodes.iter().copied().collect(), registry_version: context.registry_version, } @@ -180,8 +180,8 @@ pub mod test_utils { registry_version: u64, ) -> idkg::EcdsaReshareRequest { idkg::EcdsaReshareRequest { - key_id: Some(key_id.clone()), - master_key_id: MasterPublicKeyId::Ecdsa(key_id), + key_id: key_id.clone(), + master_key_id: Some(MasterPublicKeyId::Ecdsa(key_id)), receiving_node_ids: (0..num_nodes).map(node_test_id).collect::>(), registry_version: RegistryVersion::from(registry_version), } @@ -250,8 +250,8 @@ mod tests { let make_key_id = |i: u64| EcdsaKeyId::from_str(&format!("Secp256k1:some_key_{i}")).unwrap(); let make_reshare_request = |i| EcdsaReshareRequest { - key_id: Some(make_key_id(i)), - master_key_id: MasterPublicKeyId::Ecdsa(make_key_id(i)), + key_id: make_key_id(i), + master_key_id: Some(MasterPublicKeyId::Ecdsa(make_key_id(i))), receiving_node_ids: vec![node_test_id(i)], registry_version: RegistryVersion::from(i), }; diff --git a/rs/consensus/src/ecdsa/test_utils.rs b/rs/consensus/src/ecdsa/test_utils.rs index 4f8e5e519d6..1a31408b788 100644 --- a/rs/consensus/src/ecdsa/test_utils.rs +++ b/rs/consensus/src/ecdsa/test_utils.rs @@ -32,7 +32,6 @@ use ic_test_utilities_types::messages::RequestBuilder; use ic_types::artifact::EcdsaMessageId; use ic_types::consensus::certification::Certification; use ic_types::consensus::idkg::common::PreSignatureRef; -use ic_types::consensus::idkg::HasMasterPublicKeyId; use ic_types::consensus::idkg::{ self, ecdsa::{PreSignatureQuadrupleRef, ThresholdEcdsaSigInputsRef}, @@ -67,12 +66,9 @@ use super::utils::get_context_request_id; pub(crate) fn dealings_context_from_reshare_request( request: idkg::EcdsaReshareRequest, ) -> EcdsaDealingsContext { - let MasterPublicKeyId::Ecdsa(key_id) = request.key_id() else { - panic!("Expected ECDSA key Id"); - }; EcdsaDealingsContext { request: RequestBuilder::new().build(), - key_id, + key_id: request.key_id, nodes: request.receiving_node_ids.into_iter().collect(), registry_version: request.registry_version, time: time::UNIX_EPOCH, @@ -1485,8 +1481,8 @@ pub(crate) fn fake_ecdsa_key_id() -> EcdsaKeyId { pub(crate) fn create_reshare_request(num_nodes: u64, registry_version: u64) -> EcdsaReshareRequest { let key_id = fake_ecdsa_key_id(); EcdsaReshareRequest { - key_id: Some(key_id.clone()), - master_key_id: MasterPublicKeyId::Ecdsa(key_id), + key_id: key_id.clone(), + master_key_id: Some(MasterPublicKeyId::Ecdsa(key_id)), receiving_node_ids: (0..num_nodes).map(node_test_id).collect::>(), registry_version: RegistryVersion::from(registry_version), } diff --git a/rs/types/types/src/consensus/idkg.rs b/rs/types/types/src/consensus/idkg.rs index 1d35f66ca35..515818b0c16 100644 --- a/rs/types/types/src/consensus/idkg.rs +++ b/rs/types/types/src/consensus/idkg.rs @@ -779,8 +779,8 @@ impl TryFrom<&pb::KeyTranscriptCreation> for KeyTranscriptCreation { /// Internal format of the resharing request from execution. #[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Serialize, Deserialize)] pub struct EcdsaReshareRequest { - pub key_id: Option, - pub master_key_id: MasterPublicKeyId, + pub key_id: EcdsaKeyId, + pub master_key_id: Option, pub receiving_node_ids: Vec, pub registry_version: RegistryVersion, } @@ -793,10 +793,10 @@ impl Hash for EcdsaReshareRequest { receiving_node_ids, registry_version, } = self; - if let Some(key_id) = key_id { - key_id.hash(state); + key_id.hash(state); + if let Some(master_key_id) = master_key_id { + master_key_id.hash(state); } - master_key_id.hash(state); receiving_node_ids.hash(state); registry_version.hash(state); } @@ -809,8 +809,8 @@ impl From<&EcdsaReshareRequest> for pb::EcdsaReshareRequest { receiving_node_ids.push(node_id_into_protobuf(*node)); } Self { - key_id: request.key_id.as_ref().map(|key_id| key_id.into()), - master_key_id: Some(request.master_key_id.clone().into()), + key_id: Some((&request.key_id).into()), + master_key_id: request.master_key_id.clone().map(|key_id| key_id.into()), receiving_node_ids, registry_version: request.registry_version.get(), } @@ -826,17 +826,12 @@ impl TryFrom<&pb::EcdsaReshareRequest> for EcdsaReshareRequest { .map(|node| node_id_try_from_option(Some(node.clone()))) .collect::, ProxyDecodeError>>()?; - let key_id = request - .key_id + let key_id = try_from_option_field(request.key_id.clone(), "EcdsaReshareRequest::key_id")?; + let master_key_id = request + .master_key_id .clone() .map(|key_id| key_id.try_into()) .transpose()?; - - let master_key_id = try_from_option_field( - request.master_key_id.clone(), - "EcdsaReshareRequest::master_key_id", - )?; - Ok(Self { key_id, master_key_id, @@ -2148,7 +2143,7 @@ impl HasMasterPublicKeyId for PreSignatureRef { impl HasMasterPublicKeyId for EcdsaReshareRequest { fn key_id(&self) -> MasterPublicKeyId { - self.master_key_id.clone() + MasterPublicKeyId::Ecdsa(self.key_id.clone()) } } diff --git a/rs/types/types/src/exhaustive.rs b/rs/types/types/src/exhaustive.rs index 0359762cd2b..118c841a5ae 100644 --- a/rs/types/types/src/exhaustive.rs +++ b/rs/types/types/src/exhaustive.rs @@ -738,8 +738,8 @@ impl ExhaustiveSet for EcdsaReshareRequest { DerivedEcdsaReshareRequest::exhaustive_set(rng) .into_iter() .map(|r| EcdsaReshareRequest { - key_id: Some(r.key_id.clone()), - master_key_id: MasterPublicKeyId::Ecdsa(r.key_id), + key_id: r.key_id.clone(), + master_key_id: Some(MasterPublicKeyId::Ecdsa(r.key_id)), receiving_node_ids: r.receiving_node_ids, registry_version: r.registry_version, })