From 9652b5c10a9c138ed6a71da320e65f8324edf93d Mon Sep 17 00:00:00 2001 From: Leo Eichhorn Date: Fri, 8 May 2026 09:19:54 +0000 Subject: [PATCH 1/3] update --- .../canister/canbench/canbench_results.yml | 40 +- rs/registry/canister/src/invariants/crypto.rs | 11 +- .../canister/src/invariants/crypto/tests.rs | 11 +- .../canister/src/registry_lifecycle.rs | 342 +----------------- rs/registry/subnet_features/src/lib.rs | 114 +++++- 5 files changed, 146 insertions(+), 372 deletions(-) diff --git a/rs/registry/canister/canbench/canbench_results.yml b/rs/registry/canister/canbench/canbench_results.yml index 0fb17d314077..d5c6d47ae5ca 100644 --- a/rs/registry/canister/canbench/canbench_results.yml +++ b/rs/registry/canister/canbench/canbench_results.yml @@ -2,129 +2,129 @@ benches: get_subnet_for_canister_100_times_100: total: calls: 1 - instructions: 31025759 + instructions: 31018983 heap_increase: 0 stable_memory_increase: 0 scopes: {} get_subnet_for_canister_100_times_10k: total: calls: 1 - instructions: 694110012 + instructions: 702761588 heap_increase: 0 stable_memory_increase: 0 scopes: {} get_subnet_for_canister_100_times_1k: total: calls: 1 - instructions: 90967747 + instructions: 91772955 heap_increase: 0 stable_memory_increase: 0 scopes: {} measure_routing_table_invariant_checks_shards_and_unsharded: total: calls: 1 - instructions: 268353620 + instructions: 266793620 heap_increase: 138 stable_memory_increase: 0 scopes: {} measure_snapshot_creation_with_1000_individual_entries: total: calls: 1 - instructions: 20682683 + instructions: 20682680 heap_increase: 2 stable_memory_increase: 0 scopes: {} measure_snapshot_creation_with_100_000_individual_entries: total: calls: 1 - instructions: 3581447942 + instructions: 3581447939 heap_increase: 177 stable_memory_increase: 0 scopes: {} measure_snapshot_creation_with_100_segments_of_1000_entries: total: calls: 1 - instructions: 6435427 + instructions: 6435424 heap_increase: 30 stable_memory_increase: 0 scopes: {} measure_snapshot_creation_with_1_segment_of_1000_entries: total: calls: 1 - instructions: 53955 + instructions: 53952 heap_increase: 0 stable_memory_increase: 0 scopes: {} migrate_canisters_10_times_100: total: calls: 1 - instructions: 156361795 + instructions: 153037656 heap_increase: 0 stable_memory_increase: 0 scopes: {} migrate_canisters_10_times_10k: total: calls: 1 - instructions: 10367776782 + instructions: 10274465354 heap_increase: 0 stable_memory_increase: 0 scopes: {} migrate_canisters_10_times_1k: total: calls: 1 - instructions: 1032517212 + instructions: 1021342939 heap_increase: 3 stable_memory_increase: 0 scopes: {} upgrade_with_routing_table_100: total: calls: 1 - instructions: 186143660 + instructions: 185869684 heap_increase: 7 stable_memory_increase: 0 scopes: post_upgrade: calls: 1 - instructions: 185263063 + instructions: 184987710 heap_increase: 4 stable_memory_increase: 0 pre_upgrade: calls: 1 - instructions: 669662 + instructions: 671039 heap_increase: 3 stable_memory_increase: 0 upgrade_with_routing_table_10k: total: calls: 1 - instructions: 5626018276 + instructions: 5619485071 heap_increase: 342 stable_memory_increase: 0 scopes: post_upgrade: calls: 1 - instructions: 5603899441 + instructions: 5597365201 heap_increase: 217 stable_memory_increase: 0 pre_upgrade: calls: 1 - instructions: 16603376 + instructions: 16604363 heap_increase: 125 stable_memory_increase: 0 upgrade_with_routing_table_1k: total: calls: 1 - instructions: 686509309 + instructions: 685664368 heap_increase: 40 stable_memory_increase: 0 scopes: post_upgrade: calls: 1 - instructions: 683633916 + instructions: 682787584 heap_increase: 25 stable_memory_increase: 0 pre_upgrade: calls: 1 - instructions: 2201525 + instructions: 2202916 heap_increase: 15 stable_memory_increase: 0 version: 0.4.1 diff --git a/rs/registry/canister/src/invariants/crypto.rs b/rs/registry/canister/src/invariants/crypto.rs index 5e5460e075c6..423218a43d6e 100644 --- a/rs/registry/canister/src/invariants/crypto.rs +++ b/rs/registry/canister/src/invariants/crypto.rs @@ -290,7 +290,8 @@ fn check_chain_key_configs(snapshot: &RegistrySnapshot) -> Result<(), InvariantC for key_config in chain_key_config.key_configs { let key_id = key_config.key_id.clone(); let pre_sigs = key_config.pre_signatures_to_create_in_advance; - if key_id.requires_pre_signatures() && (pre_sigs.is_none() || pre_sigs == Some(0)) { + let requires_pre_signatures = key_id.requires_pre_signatures(); + if requires_pre_signatures && (pre_sigs.is_none() || pre_sigs == Some(0)) { return Err(InvariantCheckError { msg: format!( "pre_signatures_to_create_in_advance for key {key_id} of subnet {subnet_id:} must be non-zero", @@ -298,6 +299,14 @@ fn check_chain_key_configs(snapshot: &RegistrySnapshot) -> Result<(), InvariantC source: None, }); } + if !requires_pre_signatures && pre_sigs.is_some() { + return Err(InvariantCheckError { + msg: format!( + "pre_signatures_to_create_in_advance for key {key_id} of subnet {subnet_id:} must be None", + ), + source: None, + }); + } if !key_ids.insert(key_id) { return Err(InvariantCheckError { msg: format!( diff --git a/rs/registry/canister/src/invariants/crypto/tests.rs b/rs/registry/canister/src/invariants/crypto/tests.rs index 0874d76d86ba..bf6eb1bd1617 100644 --- a/rs/registry/canister/src/invariants/crypto/tests.rs +++ b/rs/registry/canister/src/invariants/crypto/tests.rs @@ -678,7 +678,7 @@ mod chain_key_enabled_subnet_lists { #[test] #[should_panic( - expected = "Missing required struct field: KeyConfig::pre_signatures_to_create_in_advance" + expected = "KeyConfig::pre_signatures_to_create_in_advance for key schnorr:Bip340Secp256k1:schnorr_key should be non-zero" )] fn should_fail_if_missing_pre_signatures_for_key_that_requires_pre_signatures() { let mut config = invariant_compliant_chain_key_config(); @@ -688,7 +688,7 @@ mod chain_key_enabled_subnet_lists { #[test] #[should_panic( - expected = "pre_signatures_to_create_in_advance for key ecdsa:Secp256k1:ecdsa_key of subnet ya35z-hhham-aaaaa-aaaap-yai must be non-zero" + expected = "KeyConfig::pre_signatures_to_create_in_advance for key ecdsa:Secp256k1:ecdsa_key should be non-zero" )] fn should_fail_if_pre_signatures_is_zero_for_key_that_requires_pre_signatures() { let mut config = invariant_compliant_chain_key_config(); @@ -709,14 +709,17 @@ mod chain_key_enabled_subnet_lists { } #[test] - fn should_succeed_if_pre_signatures_is_zero_for_key_that_does_not_require_pre_signatures() { + #[should_panic( + expected = "KeyConfig::pre_signatures_to_create_in_advance for key vetkd:Bls12_381_G2:vetkd_key should be None, but got Some(1)." + )] + fn should_fail_if_pre_signatures_is_set_for_key_that_does_not_require_pre_signatures() { let mut config = invariant_compliant_chain_key_config(); let key_config = &mut config.key_configs[2]; assert!(matches!( key_config.key_id.as_ref().unwrap().key_id, Some(master_public_key_id::KeyId::Vetkd(_)) ),); - key_config.pre_signatures_to_create_in_advance = Some(0); + key_config.pre_signatures_to_create_in_advance = Some(1); check_chain_key_config_invariant(config); } diff --git a/rs/registry/canister/src/registry_lifecycle.rs b/rs/registry/canister/src/registry_lifecycle.rs index e9b11419e55a..e74c33cdc371 100644 --- a/rs/registry/canister/src/registry_lifecycle.rs +++ b/rs/registry/canister/src/registry_lifecycle.rs @@ -3,11 +3,8 @@ use crate::{pb::v1::RegistryCanisterStableStorage, registry::Registry}; use ic_base_types::PrincipalId; use ic_protobuf::registry::node::v1::NodeRewardType; use ic_protobuf::registry::node_operator::v1::NodeOperatorRecord; -use ic_protobuf::registry::subnet::v1::SubnetRecord as SubnetRecordPb; -use ic_protobuf::types::v1::master_public_key_id::KeyId; -use ic_registry_keys::{make_node_operator_record_key, make_subnet_record_key}; +use ic_registry_keys::make_node_operator_record_key; use ic_registry_transport::{pb::v1::RegistryMutation, update}; -use ic_types::SubnetId; use maplit::btreemap; use prost::Message; use std::str::FromStr; @@ -36,12 +33,6 @@ pub fn canister_post_upgrade( total_batches += 1; } - let mutations = fix_vetkd_pre_signatures_field(registry); - if !mutations.is_empty() { - registry.maybe_apply_mutation_internal(mutations); - total_batches += 1; - } - total_batches }; // @@ -189,121 +180,16 @@ fn fix_node_operators_corrupted(registry: &Registry) -> Vec { mutations } -fn fix_vetkd_pre_signatures_field(registry: &Registry) -> Vec { - let mut mutations = Vec::new(); - - let subnets_with_vetkeys: Vec<&str> = vec![ - // Subnet holding vetkd:Bls12_381_G2:key_1 - "pzp6e-ekpqk-3c5x7-2h6so-njoeq-mt45d-h3h6c-q3mxf-vpeq5-fk5o7-yae", - // Subnet holding vetkd:Bls12_381_G2:key_1 backup - "uzr34-akd3s-xrdag-3ql62-ocgoh-ld2ao-tamcv-54e7j-krwgb-2gm4z-oqe", - // Subnet holding vetkd:Bls12_381_G2:test_key_1 - "fuqsr-in2lc-zbcjj-ydmcw-pzq7h-4xm2z-pto4i-dcyee-5z4rz-x63ji-nae", - // Subnet holding vetkd:Bls12_381_G2:key_1 backup - "2fq7c-slacv-26cgz-vzbx2-2jrcs-5edph-i5s2j-tck77-c3rlz-iobzx-mqe", - ]; - - for subnet_id_str in subnets_with_vetkeys { - let subnet_id_principal = match PrincipalId::from_str(subnet_id_str) { - Ok(pid) => pid, - Err(e) => { - ic_cdk::println!( - "Warning: Failed to parse subnet ID '{subnet_id_str}': {e}. Skipping.", - ); - continue; - } - }; - let subnet_id = SubnetId::new(subnet_id_principal); - - let subnet_key = make_subnet_record_key(subnet_id); - let registry_value = match registry.get(subnet_key.as_bytes(), registry.latest_version()) { - Some(value) => value, - None => { - ic_cdk::println!("Warning: Subnet {subnet_id} not found in registry, skipping",); - continue; - } - }; - - let mut subnet_record_pb = match SubnetRecordPb::decode(®istry_value.value[..]) { - Ok(record) => record, - Err(e) => { - ic_cdk::println!("Error decoding SubnetRecord for subnet {subnet_id}: {e}",); - continue; - } - }; - - // Check if chain_key_config exists and needs modification - let mut subnet_record_needs_update = false; - if let Some(ref mut chain_key_config) = subnet_record_pb.chain_key_config { - for key_config in &mut chain_key_config.key_configs { - // Skip if not a valid vetKD key. - match &key_config.key_id { - Some(key_id) => { - match &key_id.key_id { - Some(KeyId::Vetkd(_)) => { /* proceed */ } - Some(_) => continue, - None => { - ic_cdk::println!( - "Warning: KeyConfig::key_id.key_id in \ - subnet {subnet_id} is unexpectedly `None`. \ - Skipping" - ); - continue; - } - } - } - None => { - ic_cdk::println!( - "Warning: KeyConfig::key_id in subnet {subnet_id} \ - is unexpectedly `None`. Skipping." - ); - continue; - } - }; - - if key_config.pre_signatures_to_create_in_advance == Some(0) { - key_config.pre_signatures_to_create_in_advance = None; - subnet_record_needs_update = true; - ic_cdk::println!( - "Migrating vetKD key in subnet {subnet_id}: changing pre_signatures_to_create_in_advance from Some(0) to None", - ); - } - } - } - - if subnet_record_needs_update { - mutations.push(update(subnet_key, subnet_record_pb.encode_to_vec())); - } - } - - mutations -} - #[cfg(test)] mod test { use super::*; use crate::{ - common::test_helpers::{ - add_fake_subnet, empty_mutation, get_invariant_compliant_subnet_record, - invariant_compliant_registry, prepare_registry_with_nodes, - }, + common::test_helpers::{empty_mutation, invariant_compliant_registry}, registry::{EncodedVersion, Version}, registry_lifecycle::Registry, }; use ic_base_types::PrincipalId; - use ic_protobuf::registry::subnet::v1::{ - ChainKeyConfig as ChainKeyConfigPb, KeyConfig as KeyConfigPb, - SubnetRecord as SubnetRecordPb, - }; - use ic_protobuf::types::v1::{ - EcdsaCurve as EcdsaCurvePb, EcdsaKeyId as EcdsaKeyIdPb, - MasterPublicKeyId as MasterPublicKeyIdPb, VetKdCurve as VetKdCurvePb, - VetKdKeyId as VetKdKeyIdPb, master_public_key_id::KeyId, - }; - use ic_registry_keys::make_subnet_record_key; - use ic_registry_subnet_features::DEFAULT_ECDSA_MAX_QUEUE_SIZE; use ic_registry_transport::insert; - use ic_test_utilities_types::ids::subnet_test_id; use maplit::btreemap; use std::str::FromStr; @@ -642,228 +528,4 @@ mod test { "Assertion for NodeOperator {node_operator_2rqo7} failed" ); } - - #[test] - fn test_fix_vetkd_pre_signatures_field() { - let mut registry = invariant_compliant_registry(0); - let (mutate_request, node_ids_and_dkg_pks) = prepare_registry_with_nodes(1, 4); - registry.maybe_apply_mutation_internal(mutate_request.mutations); - - // Create a subnet that is in the migration list with a vetKD key that has pre_signatures_to_create_in_advance == Some(0) - let subnet_id_1 = SubnetId::new( - PrincipalId::from_str( - "pzp6e-ekpqk-3c5x7-2h6so-njoeq-mt45d-h3h6c-q3mxf-vpeq5-fk5o7-yae", - ) - .unwrap(), - ); - let mut subnet_list_record = registry.get_subnet_list_record(); - let mut subnet_record_1 = get_invariant_compliant_subnet_record( - node_ids_and_dkg_pks.keys().take(1).cloned().collect(), - ); - subnet_record_1.chain_key_config = Some(ChainKeyConfigPb { - key_configs: vec![KeyConfigPb { - key_id: Some(MasterPublicKeyIdPb { - key_id: Some(KeyId::Vetkd(VetKdKeyIdPb { - curve: VetKdCurvePb::Bls12381G2 as i32, - name: "key_1".to_string(), - })), - }), - pre_signatures_to_create_in_advance: Some(0), // This should be migrated to None - max_queue_size: Some(50), - }], - signature_request_timeout_ns: None, - idkg_key_rotation_period_ms: None, - max_parallel_pre_signature_transcripts_in_creation: None, - }); - registry.maybe_apply_mutation_internal(add_fake_subnet( - subnet_id_1, - &mut subnet_list_record, - subnet_record_1.clone(), - &node_ids_and_dkg_pks - .iter() - .take(1) - .map(|(k, v)| (*k, v.clone())) - .collect(), - )); - - // Create a subnet that is NOT in the migration list (should not be affected) - let subnet_id_2 = subnet_test_id(2000); - let mut subnet_record_2 = get_invariant_compliant_subnet_record( - node_ids_and_dkg_pks - .keys() - .skip(1) - .take(1) - .cloned() - .collect(), - ); - subnet_record_2.chain_key_config = Some(ChainKeyConfigPb { - key_configs: vec![KeyConfigPb { - key_id: Some(MasterPublicKeyIdPb { - key_id: Some(KeyId::Vetkd(VetKdKeyIdPb { - curve: VetKdCurvePb::Bls12381G2 as i32, - name: "other_key".to_string(), - })), - }), - pre_signatures_to_create_in_advance: Some(0), // This should NOT be migrated - max_queue_size: Some(50), - }], - signature_request_timeout_ns: None, - idkg_key_rotation_period_ms: None, - max_parallel_pre_signature_transcripts_in_creation: None, - }); - registry.maybe_apply_mutation_internal(add_fake_subnet( - subnet_id_2, - &mut subnet_list_record, - subnet_record_2.clone(), - &node_ids_and_dkg_pks - .iter() - .skip(1) - .take(1) - .map(|(k, v)| (*k, v.clone())) - .collect(), - )); - - // Create a subnet in the migration list with a vetKD key that has a different value (not 0) - let subnet_id_3 = SubnetId::new( - PrincipalId::from_str( - "uzr34-akd3s-xrdag-3ql62-ocgoh-ld2ao-tamcv-54e7j-krwgb-2gm4z-oqe", - ) - .unwrap(), - ); - let mut subnet_record_3 = get_invariant_compliant_subnet_record( - node_ids_and_dkg_pks - .keys() - .skip(2) - .take(1) - .cloned() - .collect(), - ); - subnet_record_3.chain_key_config = Some(ChainKeyConfigPb { - key_configs: vec![KeyConfigPb { - key_id: Some(MasterPublicKeyIdPb { - key_id: Some(KeyId::Vetkd(VetKdKeyIdPb { - curve: VetKdCurvePb::Bls12381G2 as i32, - name: "key_1".to_string(), - })), - }), - pre_signatures_to_create_in_advance: Some(10), // This should NOT be migrated (not 0) - max_queue_size: Some(50), - }], - signature_request_timeout_ns: None, - idkg_key_rotation_period_ms: None, - max_parallel_pre_signature_transcripts_in_creation: None, - }); - registry.maybe_apply_mutation_internal(add_fake_subnet( - subnet_id_3, - &mut subnet_list_record, - subnet_record_3.clone(), - &node_ids_and_dkg_pks - .iter() - .skip(2) - .take(1) - .map(|(k, v)| (*k, v.clone())) - .collect(), - )); - - // Create a subnet in the migration list with a non-vetKD key (should not be affected) - let subnet_id_4 = SubnetId::new( - PrincipalId::from_str( - "fuqsr-in2lc-zbcjj-ydmcw-pzq7h-4xm2z-pto4i-dcyee-5z4rz-x63ji-nae", - ) - .unwrap(), - ); - let mut subnet_record_4 = get_invariant_compliant_subnet_record( - node_ids_and_dkg_pks - .keys() - .skip(3) - .take(1) - .cloned() - .collect(), - ); - subnet_record_4.chain_key_config = Some(ChainKeyConfigPb { - key_configs: vec![KeyConfigPb { - key_id: Some(MasterPublicKeyIdPb { - key_id: Some(KeyId::Ecdsa(EcdsaKeyIdPb { - curve: EcdsaCurvePb::Secp256k1 as i32, - name: "test_key".to_string(), - })), - }), - pre_signatures_to_create_in_advance: Some(10), // This should NOT be migrated (not vetKD, and not zero) - max_queue_size: Some(DEFAULT_ECDSA_MAX_QUEUE_SIZE), - }], - signature_request_timeout_ns: None, - idkg_key_rotation_period_ms: None, - max_parallel_pre_signature_transcripts_in_creation: None, - }); - registry.maybe_apply_mutation_internal(add_fake_subnet( - subnet_id_4, - &mut subnet_list_record, - subnet_record_4.clone(), - &node_ids_and_dkg_pks - .iter() - .skip(3) - .take(1) - .map(|(k, v)| (*k, v.clone())) - .collect(), - )); - - // Run the migration - let mutations = fix_vetkd_pre_signatures_field(®istry); - // We expect 1 mutation: subnet_id_1 has a vetKD key with pre_signatures_to_create_in_advance == Some(0) - assert_eq!(mutations.len(), 1); - registry.apply_mutations_for_test(mutations); - - // Verify subnet_id_1 was migrated (pre_signatures_to_create_in_advance changed from Some(0) to None) - let subnet_key_1 = make_subnet_record_key(subnet_id_1); - let registry_value_1 = registry - .get(subnet_key_1.as_bytes(), registry.latest_version()) - .unwrap(); - let subnet_record_1_after = SubnetRecordPb::decode(®istry_value_1.value[..]).unwrap(); - let expected_subnet_record_1 = { - let mut subnet_record_1_clone = subnet_record_1.clone(); - subnet_record_1_clone - .chain_key_config - .as_mut() - .unwrap() - .key_configs - .get_mut(0) - .unwrap() - .pre_signatures_to_create_in_advance = None; - subnet_record_1_clone - }; - assert_eq!(subnet_record_1_after, expected_subnet_record_1); - - // Verify subnet_id_2 was NOT migrated (not in the migration list) - let subnet_key_2 = make_subnet_record_key(subnet_id_2); - let registry_value_2 = registry - .get(subnet_key_2.as_bytes(), registry.latest_version()) - .unwrap(); - let subnet_record_2_after = SubnetRecordPb::decode(®istry_value_2.value[..]).unwrap(); - assert_eq!( - subnet_record_2_after, subnet_record_2, - "Subnet not in migration list should not be affected" - ); - - // Verify subnet_id_3 was NOT migrated (pre_signatures_to_create_in_advance is not 0) - let subnet_key_3 = make_subnet_record_key(subnet_id_3); - let registry_value_3 = registry - .get(subnet_key_3.as_bytes(), registry.latest_version()) - .unwrap(); - let subnet_record_3_after = SubnetRecordPb::decode(®istry_value_3.value[..]).unwrap(); - assert_eq!( - subnet_record_3_after, subnet_record_3, - "Subnet with vetKD key having non-zero value should not be affected" - ); - - // Verify subnet_id_4 was NOT migrated (not a vetKD key) - let subnet_key_4 = make_subnet_record_key(subnet_id_4); - let registry_value_4 = registry - .get(subnet_key_4.as_bytes(), registry.latest_version()) - .unwrap(); - let subnet_record_4_after = SubnetRecordPb::decode(®istry_value_4.value[..]).unwrap(); - assert_eq!( - subnet_record_4_after, subnet_record_4, - "Subnet with non-vetKD key should not be affected" - ); - } } diff --git a/rs/registry/subnet_features/src/lib.rs b/rs/registry/subnet_features/src/lib.rs index b9d0dfb2a157..81a69f7c2d8f 100644 --- a/rs/registry/subnet_features/src/lib.rs +++ b/rs/registry/subnet_features/src/lib.rs @@ -118,13 +118,19 @@ impl TryFrom for KeyConfig { fn try_from(value: pb::KeyConfig) -> Result { let key_id: MasterPublicKeyId = try_from_option_field(value.key_id, "KeyConfig::key_id")?; - if key_id.requires_pre_signatures() && value.pre_signatures_to_create_in_advance.is_none() { - return Err(ProxyDecodeError::MissingField( - "KeyConfig::pre_signatures_to_create_in_advance", - )); + let pre_sigs = value.pre_signatures_to_create_in_advance; + let requires_pre_signatures = key_id.requires_pre_signatures(); + if requires_pre_signatures && (pre_sigs.is_none() || pre_sigs == Some(0)) { + return Err(ProxyDecodeError::Other(format!( + "KeyConfig::pre_signatures_to_create_in_advance for key {key_id} should be non-zero, but got {pre_sigs:?}.", + ))); + } else if !requires_pre_signatures && pre_sigs.is_some() { + return Err(ProxyDecodeError::Other(format!( + "KeyConfig::pre_signatures_to_create_in_advance for key {key_id} should be None, but got {pre_sigs:?}.", + ))); } Ok(KeyConfig { - pre_signatures_to_create_in_advance: value.pre_signatures_to_create_in_advance, + pre_signatures_to_create_in_advance: pre_sigs, key_id, max_queue_size: try_from_option_field( value.max_queue_size, @@ -242,7 +248,7 @@ mod tests { curve: VetKdCurve::Bls12_381_G2, name: "test_key2".to_string(), }), - pre_signatures_to_create_in_advance: Some(0), + pre_signatures_to_create_in_advance: None, max_queue_size: 30, }, ], @@ -277,7 +283,7 @@ mod tests { }, )), }), - pre_signatures_to_create_in_advance: Some(0), + pre_signatures_to_create_in_advance: None, max_queue_size: Some(30), }, ], @@ -293,4 +299,98 @@ mod tests { assert_eq!(chain_key_config, chain_key_config_after_deser,); } + + #[test] + fn test_key_config_try_from_errors() { + let ecdsa_key_id = Some(pb_types::MasterPublicKeyId { + key_id: Some(pb_types::master_public_key_id::KeyId::Ecdsa( + pb_types::EcdsaKeyId { + curve: 1, + name: "ecdsa_test_key".to_string(), + }, + )), + }); + let vetkd_key_id = Some(pb_types::MasterPublicKeyId { + key_id: Some(pb_types::master_public_key_id::KeyId::Vetkd( + pb_types::VetKdKeyId { + curve: 1, + name: "vetkd_test_key".to_string(), + }, + )), + }); + + // Branch: missing key_id => MissingField. + let err = KeyConfig::try_from(pb::KeyConfig { + key_id: None, + pre_signatures_to_create_in_advance: Some(1), + max_queue_size: Some(10), + }) + .unwrap_err(); + assert!(matches!( + err, + ProxyDecodeError::MissingField("KeyConfig::key_id") + )); + + // Branch: key requires pre-signatures and field is missing => Other. + let err = KeyConfig::try_from(pb::KeyConfig { + key_id: ecdsa_key_id.clone(), + pre_signatures_to_create_in_advance: None, + max_queue_size: Some(10), + }) + .unwrap_err(); + assert!(matches!(err, ProxyDecodeError::Other(err) if err.contains("should be non-zero"))); + + // Branch: key requires pre-signatures and field is zero => Other. + let err = KeyConfig::try_from(pb::KeyConfig { + key_id: ecdsa_key_id.clone(), + pre_signatures_to_create_in_advance: Some(0), + max_queue_size: Some(10), + }) + .unwrap_err(); + assert!(matches!(err, ProxyDecodeError::Other(err) if err.contains("should be non-zero"))); + + // Branch: key does not require pre-signatures and field is present => Other. + let err = KeyConfig::try_from(pb::KeyConfig { + key_id: vetkd_key_id.clone(), + pre_signatures_to_create_in_advance: Some(1), + max_queue_size: Some(10), + }) + .unwrap_err(); + assert!(matches!(err, ProxyDecodeError::Other(err) if err.contains("should be None"))); + + // Branch: missing max_queue_size => MissingField. + let err = KeyConfig::try_from(pb::KeyConfig { + key_id: ecdsa_key_id.clone(), + pre_signatures_to_create_in_advance: Some(1), + max_queue_size: None, + }) + .unwrap_err(); + assert!(matches!( + err, + ProxyDecodeError::MissingField("KeyConfig::max_queue_size") + )); + + // Success branch: key requiring pre-signatures with a present value. + let ecdsa_key_config = KeyConfig::try_from(pb::KeyConfig { + key_id: ecdsa_key_id, + pre_signatures_to_create_in_advance: Some(5), + max_queue_size: Some(25), + }) + .expect("ECDSA key config should decode."); + assert_eq!( + ecdsa_key_config.pre_signatures_to_create_in_advance, + Some(5) + ); + assert_eq!(ecdsa_key_config.max_queue_size, 25); + + // Success branch: key not requiring pre-signatures with no value. + let vetkd_key_config = KeyConfig::try_from(pb::KeyConfig { + key_id: vetkd_key_id, + pre_signatures_to_create_in_advance: None, + max_queue_size: Some(30), + }) + .expect("VetKD key config should decode."); + assert_eq!(vetkd_key_config.pre_signatures_to_create_in_advance, None); + assert_eq!(vetkd_key_config.max_queue_size, 30); + } } From 3936055bb6841d79822d570797886843dd9029c9 Mon Sep 17 00:00:00 2001 From: Leo Eichhorn Date: Fri, 8 May 2026 11:06:47 +0000 Subject: [PATCH 2/3] fix --- rs/consensus/idkg/src/utils.rs | 35 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/rs/consensus/idkg/src/utils.rs b/rs/consensus/idkg/src/utils.rs index 6f6647fc2506..ae1aa9f8bce7 100644 --- a/rs/consensus/idkg/src/utils.rs +++ b/rs/consensus/idkg/src/utils.rs @@ -342,13 +342,6 @@ pub fn get_idkg_chain_key_config_if_enabled( .iter() // Skip keys that don't need to run IDKG protocol .filter(|key_config| key_config.key_id.is_idkg_key()) - // A key that has `presignatures_to_create_in_advance` set to 0 is not active - .filter(|key_config| { - key_config - .pre_signatures_to_create_in_advance - .unwrap_or_default() - != 0 - }) .count(); if num_active_key_ids == 0 { @@ -807,11 +800,11 @@ mod tests { let (subnet_id, registry, version) = set_up_get_chain_key_config_test(&malformed_chain_key_config, pool_config); - let config = - get_idkg_chain_key_config_if_enabled(subnet_id, version, registry.as_ref()) - .expect("Should successfully get the config"); - - assert!(config.is_none()); + let result = + get_idkg_chain_key_config_if_enabled(subnet_id, version, registry.as_ref()); + assert_matches!(result, Err(RegistryClientError::DecodeError { error }) + if error.contains("KeyConfig::pre_signatures_to_create_in_advance") + && error.contains("should be non-zero")); }) } @@ -836,10 +829,8 @@ mod tests { get_idkg_chain_key_config_if_enabled(subnet_id, version, registry.as_ref()); assert_matches!(result, Err(RegistryClientError::DecodeError{ error }) - if error.contains("\ - failed with Missing required struct field: \ - KeyConfig::pre_signatures_to_create_in_advance\ - ") + if error.contains("KeyConfig::pre_signatures_to_create_in_advance") + && error.contains("should be non-zero") ); }) } @@ -861,10 +852,12 @@ mod tests { let (subnet_id, registry, version) = set_up_get_chain_key_config_test(&malformed_chain_key_config, pool_config); - let config = + let result = get_idkg_chain_key_config_if_enabled(subnet_id, version, registry.as_ref()); - assert_matches!(config, Ok(None)); + assert_matches!(result, Err(RegistryClientError::DecodeError { error }) + if error.contains("KeyConfig::pre_signatures_to_create_in_advance") + && error.contains("should be None")); }) } @@ -885,10 +878,12 @@ mod tests { let (subnet_id, registry, version) = set_up_get_chain_key_config_test(&malformed_chain_key_config, pool_config); - let config = + let result = get_idkg_chain_key_config_if_enabled(subnet_id, version, registry.as_ref()); - assert_matches!(config, Ok(None)); + assert_matches!(result, Err(RegistryClientError::DecodeError { error }) + if error.contains("KeyConfig::pre_signatures_to_create_in_advance") + && error.contains("should be None")); }) } From 1db0d89c237fe23a9ff00581113942803a4a0a85 Mon Sep 17 00:00:00 2001 From: Leo Eichhorn Date: Fri, 8 May 2026 12:16:30 +0000 Subject: [PATCH 3/3] changelog --- rs/registry/canister/unreleased_changelog.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rs/registry/canister/unreleased_changelog.md b/rs/registry/canister/unreleased_changelog.md index b3ecec255b16..160c8b91eff4 100644 --- a/rs/registry/canister/unreleased_changelog.md +++ b/rs/registry/canister/unreleased_changelog.md @@ -14,10 +14,15 @@ on the process that this file is part of, see ## Changed * Updated the response text of some failed registry mutations. "Blessed" -> "Elected". +* Tightened chain-key config validation and invariants: + `pre_signatures_to_create_in_advance` must be non-zero for keys that require pre-signatures, + and must be `None` for keys that do not. ## Deprecated ## Removed +* Removed the completed `fix_vetkd_pre_signatures_field` post-upgrade data migration and its + migration-specific unit test. ## Fixed