From 98eceedbf924ece22d069cedebb535a31fb71985 Mon Sep 17 00:00:00 2001 From: Leo Eichhorn Date: Fri, 12 Jan 2024 15:18:02 +0000 Subject: [PATCH] feat(ecdsa): CON-1196 Purge unmatched quadruples referencing old key transcripts once certified height reaches the latest summary height --- rs/consensus/src/ecdsa/payload_builder.rs | 26 +++ .../src/ecdsa/payload_builder/quadruples.rs | 204 +++++++++++++++++- rs/consensus/src/ecdsa/test_utils.rs | 19 ++ 3 files changed, 241 insertions(+), 8 deletions(-) diff --git a/rs/consensus/src/ecdsa/payload_builder.rs b/rs/consensus/src/ecdsa/payload_builder.rs index 844e2ff4b7c..e0db12fb6ee 100644 --- a/rs/consensus/src/ecdsa/payload_builder.rs +++ b/rs/consensus/src/ecdsa/payload_builder.rs @@ -493,6 +493,11 @@ pub(crate) fn create_data_payload( Ok(new_payload) } +pub(crate) enum CertifiedHeight { + ReachedSummaryHeight, + BelowSummaryHeight, +} + pub(crate) fn create_data_payload_helper( subnet_id: SubnetId, context: &ValidationContext, @@ -551,6 +556,12 @@ pub(crate) fn create_data_payload_helper( .subnet_call_context_manager .ecdsa_dealings_contexts; + let certified_height = if context.certified_height >= summary_block.height() { + CertifiedHeight::ReachedSummaryHeight + } else { + CertifiedHeight::BelowSummaryHeight + }; + create_data_payload_helper_2( &mut ecdsa_payload, height, @@ -558,6 +569,7 @@ pub(crate) fn create_data_payload_helper( &ecdsa_config, &enabled_signing_keys, next_interval_registry_version, + certified_height, &receivers, all_signing_requests, ecdsa_dealings_contexts, @@ -577,6 +589,7 @@ pub(crate) fn create_data_payload_helper_2( ecdsa_config: &EcdsaConfig, enabled_signing_keys: &BTreeSet, next_interval_registry_version: RegistryVersion, + certified_height: CertifiedHeight, receivers: &[NodeId], all_signing_requests: &BTreeMap, ecdsa_dealings_contexts: &BTreeMap, @@ -614,6 +627,11 @@ pub(crate) fn create_data_payload_helper_2( ecdsa_payload, log, )?; + + if matches!(certified_height, CertifiedHeight::ReachedSummaryHeight) { + quadruples::purge_old_key_quadruples(ecdsa_payload, all_signing_requests); + } + quadruples::make_new_quadruples_if_needed(ecdsa_config, ecdsa_payload); let mut new_transcripts = @@ -1848,6 +1866,7 @@ mod tests { &EcdsaConfig::default(), &valid_keys, RegistryVersion::from(9), + CertifiedHeight::ReachedSummaryHeight, &[node_test_id(0)], &sign_with_ecdsa_contexts, &BTreeMap::default(), @@ -1871,6 +1890,7 @@ mod tests { &EcdsaConfig::default(), &valid_keys, RegistryVersion::from(9), + CertifiedHeight::ReachedSummaryHeight, &[node_test_id(0)], &sign_with_ecdsa_contexts, &BTreeMap::default(), @@ -2876,6 +2896,8 @@ mod tests { assert_eq!(metrics.critical_error_ecdsa_key_transcript_missing.get(), 1); // Now, quadruples and xnet reshares should be purged + // TODO(CON-1149): Extend once available quadruples are no longer immediately purged + // on key reshares. assert!(payload_4.available_quadruples.is_empty()); assert!(payload_4.quadruples_in_creation.is_empty()); assert!(payload_4.ongoing_xnet_reshares.is_empty()); @@ -2944,6 +2966,7 @@ mod tests { &ecdsa_config, &valid_keys, registry_version, + CertifiedHeight::ReachedSummaryHeight, &node_ids, &BTreeMap::default(), &BTreeMap::default(), @@ -3109,6 +3132,7 @@ mod tests { &ecdsa_config, &valid_keys, registry_version, + CertifiedHeight::ReachedSummaryHeight, &node_ids, &BTreeMap::default(), &BTreeMap::default(), @@ -3137,6 +3161,7 @@ mod tests { &ecdsa_config, &valid_keys, registry_version, + CertifiedHeight::ReachedSummaryHeight, &node_ids, &BTreeMap::default(), &BTreeMap::default(), @@ -3163,6 +3188,7 @@ mod tests { &ecdsa_config, &valid_keys, registry_version, + CertifiedHeight::ReachedSummaryHeight, &node_ids, &BTreeMap::default(), &BTreeMap::default(), diff --git a/rs/consensus/src/ecdsa/payload_builder/quadruples.rs b/rs/consensus/src/ecdsa/payload_builder/quadruples.rs index 01f1c6165cd..17786c46f08 100644 --- a/rs/consensus/src/ecdsa/payload_builder/quadruples.rs +++ b/rs/consensus/src/ecdsa/payload_builder/quadruples.rs @@ -3,13 +3,15 @@ use super::EcdsaPayloadError; use crate::ecdsa::pre_signer::EcdsaTranscriptBuilder; use ic_logger::{debug, error, ReplicaLogger}; use ic_registry_subnet_features::EcdsaConfig; +use ic_replicated_state::metadata_state::subnet_call_context_manager::SignWithEcdsaContext; use ic_types::{ consensus::ecdsa::{self, TranscriptAttributes}, crypto::{canister_threshold_sig::idkg::IDkgTranscript, AlgorithmId}, + messages::CallbackId, Height, NodeId, RegistryVersion, }; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; /// Update the quadruples in the payload by: /// - making new configs when pre-conditions are met; @@ -206,6 +208,32 @@ pub(super) fn update_quadruples_in_creation( Ok(new_transcripts) } +/// Purge all available but unmatched quadruples that are referencing a different key transcript +/// than the one currently used. +pub(super) fn purge_old_key_quadruples( + ecdsa_payload: &mut ecdsa::EcdsaPayload, + all_signing_requests: &BTreeMap, +) { + let Some(unmasked_transcript) = ecdsa_payload.key_transcript.current.as_ref() else { + return; + }; + let current_key_transcript_id = unmasked_transcript.transcript_id(); + + let matched_quadruples = all_signing_requests + .values() + .flat_map(|context| context.matched_quadruple.clone()) + .map(|(quadruple_id, _)| quadruple_id) + .collect::>(); + + ecdsa_payload.available_quadruples.retain(|id, quadruple| { + matched_quadruples.contains(id) + || match quadruple.key_unmasked_ref.as_ref() { + Some(transcript) => transcript.as_ref().transcript_id == current_key_transcript_id, + None => true, + } + }); +} + /// Creating new quadruples if necessary by updating quadruples_in_creation, /// considering currently available quadruples, quadruples in creation, and /// ecdsa configs. @@ -275,7 +303,7 @@ pub(super) mod test_utils { use std::collections::BTreeMap; use ic_types::{ - consensus::ecdsa::{self, EcdsaPayload, QuadrupleId}, + consensus::ecdsa::{self, EcdsaPayload, QuadrupleId, UnmaskedTranscript}, NodeId, RegistryVersion, }; @@ -295,12 +323,27 @@ pub(super) mod test_utils { } pub fn create_available_quadruple(ecdsa_payload: &mut EcdsaPayload, caller: u8) -> QuadrupleId { + create_available_quadruple_with_key_transcript(ecdsa_payload, caller, None) + } + + pub fn create_available_quadruple_with_key_transcript( + ecdsa_payload: &mut EcdsaPayload, + caller: u8, + key_transcript: Option, + ) -> QuadrupleId { let sig_inputs = create_sig_inputs(caller); let quadruple_id = ecdsa_payload.uid_generator.next_quadruple_id(); - let quadruple_ref = &sig_inputs.sig_inputs_ref.presig_quadruple_ref; + let mut quadruple_ref = sig_inputs.sig_inputs_ref.presig_quadruple_ref.clone(); + quadruple_ref.key_unmasked_ref = key_transcript; ecdsa_payload .available_quadruples - .insert(quadruple_id.clone(), quadruple_ref.clone()); + .insert(quadruple_id.clone(), quadruple_ref); + + for (t_ref, transcript) in sig_inputs.idkg_transcripts { + ecdsa_payload + .idkg_transcripts + .insert(t_ref.transcript_id, transcript); + } quadruple_id } @@ -312,15 +355,18 @@ pub(super) mod tests { use super::*; use crate::ecdsa::test_utils::{ - set_up_ecdsa_payload, EcdsaPayloadTestHelper, TestEcdsaBlockReader, - TestEcdsaTranscriptBuilder, + fake_sign_with_ecdsa_context_with_quadruple, set_up_ecdsa_payload, EcdsaPayloadTestHelper, + TestEcdsaBlockReader, TestEcdsaTranscriptBuilder, + }; + use ic_crypto_test_utils_canister_threshold_sigs::{ + generate_key_transcript, CanisterThresholdSigTestEnvironment, IDkgParticipants, }; - use ic_crypto_test_utils_canister_threshold_sigs::CanisterThresholdSigTestEnvironment; use ic_crypto_test_utils_reproducible_rng::{reproducible_rng, ReproducibleRng}; use ic_logger::replica_logger::no_op_logger; use ic_test_utilities::types::ids::subnet_test_id; use ic_types::{ - consensus::ecdsa::EcdsaPayload, crypto::canister_threshold_sig::idkg::IDkgTranscriptId, + consensus::ecdsa::{EcdsaPayload, UnmaskedTranscript}, + crypto::canister_threshold_sig::idkg::IDkgTranscriptId, SubnetId, }; @@ -581,4 +627,146 @@ pub(super) mod tests { assert_eq!(payload.peek_next_transcript_id().id(), 5); assert!(config_ids(&payload).is_empty()); } + + fn get_current_unmasked_key_transcript(payload: &EcdsaPayload) -> UnmaskedTranscript { + let transcript = payload.key_transcript.current.clone(); + transcript.unwrap().unmasked_transcript() + } + + #[test] + fn test_matched_quadruples_are_not_purged() { + let mut rng = reproducible_rng(); + let (mut payload, env, _) = set_up(&mut rng, subnet_test_id(1), Height::from(100)); + let key_id = payload.key_transcript.key_id.clone(); + let key_transcript = get_current_unmasked_key_transcript(&payload); + + let (dealers, receivers) = env.choose_dealers_and_receivers( + &IDkgParticipants::AllNodesAsDealersAndReceivers, + &mut rng, + ); + let transcript = generate_key_transcript( + &env, + &dealers, + &receivers, + AlgorithmId::ThresholdEcdsaSecp256k1, + &mut rng, + ); + let key_transcript2 = + UnmaskedTranscript::try_from((Height::from(200), &transcript)).unwrap(); + + // Create three quadruples, with the current, a different, no key transcript. + let quadruple_ids = vec![ + create_available_quadruple_with_key_transcript(&mut payload, 1, Some(key_transcript)), + create_available_quadruple_with_key_transcript(&mut payload, 2, Some(key_transcript2)), + create_available_quadruple_with_key_transcript(&mut payload, 3, None), + ]; + + // All three quadruples are matched with a context + let contexts = BTreeMap::from_iter(quadruple_ids.into_iter().map(|id| { + fake_sign_with_ecdsa_context_with_quadruple(id.id() as u8, key_id.clone(), Some(id)) + })); + + // None of them should be purged + assert_eq!(payload.available_quadruples.len(), 3); + purge_old_key_quadruples(&mut payload, &contexts); + assert_eq!(payload.available_quadruples.len(), 3); + } + + #[test] + fn test_unmatched_quadruples_of_current_key_are_not_purged() { + let mut rng = reproducible_rng(); + let (mut payload, _, _) = set_up(&mut rng, subnet_test_id(1), Height::from(100)); + let key_id = payload.key_transcript.key_id.clone(); + let key_transcript = get_current_unmasked_key_transcript(&payload); + + // Create three quadruples of the current key transcript + for i in 0..3 { + create_available_quadruple_with_key_transcript(&mut payload, i, Some(key_transcript)); + } + + // None of them are matched to a context + let contexts = BTreeMap::from_iter([fake_sign_with_ecdsa_context_with_quadruple( + 1, + key_id.clone(), + None, + )]); + + // None of them should be purged + assert_eq!(payload.available_quadruples.len(), 3); + purge_old_key_quadruples(&mut payload, &contexts); + assert_eq!(payload.available_quadruples.len(), 3); + } + + #[test] + fn test_unmatched_quadruples_without_key_are_not_purged() { + let mut rng = reproducible_rng(); + let (mut payload, _, _) = set_up(&mut rng, subnet_test_id(1), Height::from(100)); + let key_id = payload.key_transcript.key_id.clone(); + + // Create three quadruples without key transcript + for i in 0..3 { + create_available_quadruple_with_key_transcript(&mut payload, i, None); + } + + // None of them are matched to a context + let contexts = BTreeMap::from_iter([fake_sign_with_ecdsa_context_with_quadruple( + 1, + key_id.clone(), + None, + )]); + + // None of them should be purged + assert_eq!(payload.available_quadruples.len(), 3); + purge_old_key_quadruples(&mut payload, &contexts); + assert_eq!(payload.available_quadruples.len(), 3); + } + + #[test] + fn test_unmatched_quadruples_of_different_key_are_purged() { + let mut rng = reproducible_rng(); + let (mut payload, env, _) = set_up(&mut rng, subnet_test_id(1), Height::from(100)); + let key_id = payload.key_transcript.key_id.clone(); + + let (dealers, receivers) = env.choose_dealers_and_receivers( + &IDkgParticipants::AllNodesAsDealersAndReceivers, + &mut rng, + ); + let transcript = generate_key_transcript( + &env, + &dealers, + &receivers, + AlgorithmId::ThresholdEcdsaSecp256k1, + &mut rng, + ); + let other_key_transcript = + UnmaskedTranscript::try_from((Height::from(200), &transcript)).unwrap(); + + // Create two quadruples of the other key transcript + let quadruple_ids = (0..2) + .map(|i| { + create_available_quadruple_with_key_transcript( + &mut payload, + i, + Some(other_key_transcript), + ) + }) + .collect::>(); + + // The first one is matched to a context + let contexts = BTreeMap::from_iter([fake_sign_with_ecdsa_context_with_quadruple( + 1, + key_id.clone(), + Some(quadruple_ids[0].clone()), + )]); + + // The second one should be purged + assert_eq!(payload.available_quadruples.len(), 2); + purge_old_key_quadruples(&mut payload, &contexts); + assert_eq!(payload.available_quadruples.len(), 1); + + assert_eq!( + payload.available_quadruples.into_keys().next().unwrap(), + quadruple_ids[0] + ); + } } diff --git a/rs/consensus/src/ecdsa/test_utils.rs b/rs/consensus/src/ecdsa/test_utils.rs index a25d10ef8c0..818ab17dd5b 100644 --- a/rs/consensus/src/ecdsa/test_utils.rs +++ b/rs/consensus/src/ecdsa/test_utils.rs @@ -43,6 +43,7 @@ use ic_types::crypto::canister_threshold_sig::{ }; use ic_types::crypto::AlgorithmId; use ic_types::malicious_behaviour::MaliciousBehaviour; +use ic_types::messages::CallbackId; use ic_types::{signature::*, Time}; use ic_types::{Height, NodeId, PrincipalId, Randomness, RegistryVersion, SubnetId}; use rand::{CryptoRng, Rng}; @@ -88,6 +89,24 @@ pub fn fake_sign_with_ecdsa_context_with_batch_time( } } +pub fn fake_sign_with_ecdsa_context_with_quadruple( + id: u8, + key_id: EcdsaKeyId, + quadruple: Option, +) -> (CallbackId, SignWithEcdsaContext) { + let context = SignWithEcdsaContext { + request: RequestBuilder::new().build(), + message_hash: [0; 32], + derivation_path: vec![], + batch_time: mock_time(), + key_id, + pseudo_random_id: [id; 32], + matched_quadruple: quadruple.map(|qid| (qid, Height::from(1))), + nonce: None, + }; + (CallbackId::from(id as u64), context) +} + #[derive(Clone)] pub(crate) struct TestTranscriptParams { pub(crate) idkg_transcripts: BTreeMap,