Skip to content

Commit

Permalink
Merge branch 'arshavir/post-ecdsa-cleanup-2' into 'master'
Browse files Browse the repository at this point in the history 
chore(registry): Remove redundant ECDSA-specific functions

This MR removes some dead code and slightly refactors the last part in `validate_create_subnet_payload` to ensure that `initial_chain_key_config` is validated using the same code regardless of whether it comes from the legacy or the new source. 

See merge request dfinity-lab/public/ic!20091
  • Loading branch information
@aterga
aterga committed Jun 27, 2024
2 parents 4f36ced + b86b9aa commit 32d2c91
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 209 deletions.
79 changes: 42 additions & 37 deletions rs/registry/canister/src/mutations/do_create_subnet.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl Registry {
LOG_PREFIX, payload, subnet_id
);

// 2b. Invoke compute_initial_ecdsa_dealings on ic_00
// 2b. Invoke compute_initial_i_dkg_dealings on ic_00

// TODO[NNS1-3022]: Stop reading `payload.ecdsa_config` and mutating `payload`.

Expand Down Expand Up @@ -249,32 +249,41 @@ impl Registry {
}
});

let own_subnet_id = None;
match (&payload.ecdsa_config, &payload.chain_key_config) {
(Some(_), Some(_)) => {
panic!("Deprecated field ecdsa_config cannot be specified with chain_key_config.");
}
(Some(ecdsa_initial_config), None) => {
if let Err(message) =
self.validate_ecdsa_initial_config(ecdsa_initial_config, own_subnet_id)
{
panic!("{}Cannot create subnet: {}", LOG_PREFIX, message);
let prevalidated_initial_chain_key_config =
match (&payload.ecdsa_config, &payload.chain_key_config) {
(Some(_), Some(_)) => {
panic!(
"Deprecated field ecdsa_config cannot be specified with chain_key_config."
);
}
}
(None, Some(initial_chain_key_config)) => {
let validation_result =
InitialChainKeyConfigInternal::try_from(initial_chain_key_config.clone())
.and_then(|initial_chain_key_config| {
self.validate_initial_chain_key_config(
&initial_chain_key_config,
own_subnet_id,
)
});
if let Err(message) = validation_result {
panic!("{}Cannot create subnet: {}", LOG_PREFIX, message);
(Some(ecdsa_initial_config), None) => {
let initial_chain_key_config_from_legacy_source =
InitialChainKeyConfigInternal::try_from(ecdsa_initial_config.clone())
.unwrap_or_else(|err| {
panic!(
"{}Cannot prevalidate ChainKeyConfig derived from EcdsaInitialConfig: \
{}", LOG_PREFIX, err
);
});
Some(initial_chain_key_config_from_legacy_source)
}
}
(None, None) => (), // Nothing to do.
(None, Some(initial_chain_key_config)) => {
let initial_chain_key_config_from_new_source =
InitialChainKeyConfigInternal::try_from(initial_chain_key_config.clone())
.unwrap_or_else(|err| {
panic!("{}Cannot prevalidate ChainKeyConfig: {}", LOG_PREFIX, err);
});
Some(initial_chain_key_config_from_new_source)
}
(None, None) => None,
};
if let Some(prevalidated_initial_chain_key_config) = prevalidated_initial_chain_key_config {
let own_subnet_id = None;
self.validate_initial_chain_key_config(
&prevalidated_initial_chain_key_config,
own_subnet_id,
)
.unwrap_or_else(|err| panic!("{}Cannot validate ChainKeyConfig: {}", LOG_PREFIX, err));
}
}
}
Expand Down Expand Up @@ -663,7 +672,7 @@ mod test {
// for DKG + ECDSA
#[test]
#[should_panic(
expected = "The requested ECDSA key 'ecdsa:Secp256k1:fake_key_id' was not found in any subnet"
expected = "requested chain key 'ecdsa:Secp256k1:fake_key_id' was not found in any subnet"
)]
fn should_panic_if_ecdsa_keys_non_existing() {
let mut registry = invariant_compliant_registry(0);
Expand All @@ -676,7 +685,7 @@ mod test {
curve: EcdsaCurve::Secp256k1,
name: "fake_key_id".to_string(),
},
subnet_id: None,
subnet_id: Some(*TEST_USER2_PRINCIPAL),
}],
max_queue_size: Some(DEFAULT_ECDSA_MAX_QUEUE_SIZE),
signature_request_timeout_ns: None,
Expand All @@ -689,9 +698,7 @@ mod test {
}

#[test]
#[should_panic(
expected = "EcdsaKeyRequest for key 'ecdsa:Secp256k1:fake_key_id' did not specify subnet_id."
)]
#[should_panic(expected = "EcdsaKeyRequest.subnet_id must be set")]
fn should_panic_if_ecdsa_keys_subnet_not_specified() {
// Set up a subnet that has the key but fail to specify subnet_id in request
let key_id = EcdsaKeyId {
Expand Down Expand Up @@ -748,8 +755,8 @@ mod test {

#[test]
#[should_panic(
expected = "The requested ECDSA key 'ecdsa:Secp256k1:fake_key_id' is not available in targeted \
subnet 'l5ckc-b6p6l-4o5gj-fkfvl-3sq56-7vw6s-d6nof-q4j4j-jzead-nnwim-vqe'"
expected = "The requested chain key 'ecdsa:Secp256k1:fake_key_id' is not available \
in targeted subnet 'l5ckc-b6p6l-4o5gj-fkfvl-3sq56-7vw6s-d6nof-q4j4j-jzead-nnwim-vqe'"
)]
fn should_panic_if_ecdsa_keys_non_existing_from_requested_subnet() {
let key_id = EcdsaKeyId {
Expand Down Expand Up @@ -805,11 +812,9 @@ mod test {
}

#[test]
#[should_panic(
expected = "[Registry] Cannot create subnet: The requested ECDSA key ids [EcdsaKeyId { curve: \
Secp256k1, name: \"fake_key_id\" }, EcdsaKeyId { curve: Secp256k1, name: \"fake_key_id\" }] \
have duplicates"
)]
#[should_panic(expected = "The requested chain keys [\
Ecdsa(EcdsaKeyId { curve: Secp256k1, name: \"fake_key_id\" }), \
Ecdsa(EcdsaKeyId { curve: Secp256k1, name: \"fake_key_id\" })] have duplicates")]
fn test_disallow_duplicate_ecdsa_keys() {
// Step 1.1: prepare registry.
let mut registry = invariant_compliant_registry(0);
Expand Down
178 changes: 6 additions & 172 deletions rs/registry/canister/src/mutations/subnet.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
use crate::chain_key::{InitialChainKeyConfigInternal, KeyConfigRequestInternal};
use crate::{
common::LOG_PREFIX,
mutations::{
common::{
decode_registry_value, encode_or_panic, get_subnet_ids_from_subnet_list, has_duplicates,
},
do_create_subnet::EcdsaInitialConfig,
mutations::common::{
decode_registry_value, encode_or_panic, get_subnet_ids_from_subnet_list, has_duplicates,
},
registry::{Registry, Version},
};
Expand All @@ -15,20 +12,15 @@ use ic_base_types::{
subnet_id_into_protobuf, CanisterId, NodeId, PrincipalId, RegistryVersion, SubnetId,
};
use ic_management_canister_types::{
ComputeInitialEcdsaDealingsArgs, ComputeInitialEcdsaDealingsResponse,
ComputeInitialIDkgDealingsArgs, ComputeInitialIDkgDealingsResponse, EcdsaKeyId,
MasterPublicKeyId,
ComputeInitialIDkgDealingsArgs, ComputeInitialIDkgDealingsResponse, MasterPublicKeyId,
};
use ic_protobuf::registry::{
crypto::v1::{ChainKeySigningSubnetList, EcdsaSigningSubnetList},
subnet::v1::{
CatchUpPackageContents, ChainKeyInitialization, EcdsaInitialization, SubnetListRecord,
SubnetRecord,
},
crypto::v1::ChainKeySigningSubnetList,
subnet::v1::{CatchUpPackageContents, ChainKeyInitialization, SubnetListRecord, SubnetRecord},
};
use ic_registry_keys::{
make_catch_up_package_contents_key, make_chain_key_signing_subnet_list_key,
make_ecdsa_signing_subnet_list_key, make_subnet_list_record_key, make_subnet_record_key,
make_subnet_list_record_key, make_subnet_record_key,
};
use ic_registry_subnet_features::ChainKeyConfig;
use ic_registry_transport::{
Expand Down Expand Up @@ -269,96 +261,6 @@ impl Registry {
}
}

/// Get the initial ECDSA dealings via a call to IC00 for a given EcdsaInitialConfig and a set of
/// nodes to receive them.
pub async fn get_all_initial_ecdsa_dealings_from_ic00(
&self,
ecdsa_initial_config: &Option<EcdsaInitialConfig>,
receiver_nodes: Vec<NodeId>,
) -> Vec<EcdsaInitialization> {
let initial_ecdsa_dealings_futures = ecdsa_initial_config
.as_ref()
.map(|config| {
self.get_compute_ecdsa_args_from_initial_config(config, receiver_nodes)
.into_iter()
.map(|request| self.get_ecdsa_initializations_from_ic00(request))
.collect::<Vec<_>>()
})
.unwrap_or_default();

futures::future::join_all(initial_ecdsa_dealings_futures).await
}

/// Helper function to build the request objects to send to IC00 for
/// `compute_initial_ecdsa_dealings`
fn get_compute_ecdsa_args_from_initial_config(
&self,
ecdsa_initial_config: &EcdsaInitialConfig,
receiver_nodes: Vec<NodeId>,
) -> Vec<ComputeInitialEcdsaDealingsArgs> {
let latest_version = self.latest_version();
ecdsa_initial_config
.keys
.iter()
.map(|key_request| {
// create requests outside of async move context to avoid ownership problems
let key_id = key_request.key_id.clone();
let target_subnet = key_request
.subnet_id
.map(SubnetId::new)
.expect("subnet_id is required for EcdsaKeyRequests");
ComputeInitialEcdsaDealingsArgs::new(
key_id,
target_subnet,
receiver_nodes.iter().copied().collect(),
RegistryVersion::new(latest_version),
)
})
.collect()
}

/// Helper function to make the request and decode the response for
/// `compute_initial_ecdsa_dealings`.
async fn get_ecdsa_initializations_from_ic00(
&self,
dealing_request: ComputeInitialEcdsaDealingsArgs,
) -> EcdsaInitialization {
let response_bytes = call(
CanisterId::ic_00(),
"compute_initial_ecdsa_dealings",
bytes,
Encode!(&dealing_request).unwrap(),
)
.await
.unwrap();

let response = ComputeInitialEcdsaDealingsResponse::decode(&response_bytes).unwrap();
println!(
"{}response from compute_initial_ecdsa_dealings successfully received",
LOG_PREFIX
);

EcdsaInitialization {
key_id: Some((&dealing_request.key_id).into()),
dealings: Some(response.initial_dkg_dealings),
}
}

/// Get the list of subnets that can sign for a given EcdsaKeyId.
pub fn get_ecdsa_signing_subnet_list(
&self,
key_id: &EcdsaKeyId,
) -> Option<EcdsaSigningSubnetList> {
let ecdsa_signing_subnet_list_key_id = make_ecdsa_signing_subnet_list_key(key_id);
self.get(
ecdsa_signing_subnet_list_key_id.as_bytes(),
self.latest_version(),
)
.map(|registry_value| {
decode_registry_value::<EcdsaSigningSubnetList>(registry_value.value.to_vec())
})
}

/// Get the list of subnets that can sign for a given MasterPublicKeyId.
pub fn get_chain_key_signing_subnet_list(
&self,
Expand Down Expand Up @@ -454,74 +356,6 @@ impl Registry {
requested_keys.difference(&current_keys).cloned().collect()
}

/// Validates EcdsaInitialConfig. If own_subnet_id is supplied, this also validates that all
/// requested keys are available on a different subnet (for the case of recovering a subnet)
pub fn validate_ecdsa_initial_config(
&self,
ecdsa_initial_config: &EcdsaInitialConfig,
own_subnet_id: Option<PrincipalId>,
) -> Result<(), String> {
let keys_to_subnets = self.get_master_public_keys_to_subnets_map();

for key_request in &ecdsa_initial_config.keys {
// Requested key must be a known key.
let key_id = MasterPublicKeyId::Ecdsa(key_request.key_id.clone());
if !keys_to_subnets.contains_key(&key_id) {
return Err(format!(
"The requested ECDSA key '{}' was not found in any subnet.",
key_id
));
}

let subnets_for_key = keys_to_subnets.get(&key_id).unwrap();

// Require that a subnet is targeted.
let subnet_id_principal = match key_request.subnet_id.as_ref() {
None => {
return Err(format!(
"EcdsaKeyRequest for key '{}' did not specify subnet_id.",
key_id
))
}
Some(id) => id,
};

// Ensure the subnet being targeted is not the same as the subnet being recovered.
if let Some(own_subnet_principal) = own_subnet_id {
if subnet_id_principal == &own_subnet_principal {
return Err(format!(
"Attempted to recover ECDSA key '{}' by requesting it from itself. \
Subnets cannot recover ECDSA keys from themselves.",
key_id,
));
}
}

// Ensure that the targeted subnet actually holds the key.
let subnet_id = SubnetId::new(*subnet_id_principal);
if !subnets_for_key.contains(&subnet_id) {
return Err(format!(
"The requested ECDSA key '{}' is not available in targeted subnet '{}'.",
key_id, subnet_id_principal
));
}
}

let ecdsa_key_ids: Vec<_> = ecdsa_initial_config
.keys
.iter()
.map(|key| key.key_id.clone())
.collect();
if has_duplicates(&ecdsa_key_ids) {
return Err(format!(
"The requested ECDSA key ids {:?} have duplicates",
ecdsa_key_ids
));
}

Ok(())
}

/// Validates InitialChainKeyConfig. If own_subnet_id is supplied, this also validates that all
/// requested keys are available on a different subnet (for the case of recovering a subnet)
pub(crate) fn validate_initial_chain_key_config(
Expand Down

0 comments on commit 32d2c91

Please sign in to comment.