Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

Draft
wants to merge 36 commits into
base: franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
a4e5d35
feat(crypto): CRP-2597 make NiDkgTag non-Copy
fspreiss Oct 31, 2024
808aa1d
More fixes
fspreiss Oct 31, 2024
9cb80e8
Take NiDkgId as ref in combine_threshold_sig_shares
fspreiss Nov 1, 2024
4461616
Take NiDkgId as ref in verify_threshold_sig_combined
fspreiss Nov 1, 2024
0ae9cdc
Take NiDkgId as ref at more places
fspreiss Nov 1, 2024
3a8675b
Fmt
fspreiss Nov 1, 2024
8c447fd
Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-make-…
fspreiss Nov 1, 2024
5e466de
Remove unused dkg_id param of NiDkgCspClient::load_threshold_signing_key
fspreiss Nov 1, 2024
b5343d6
Cleanup benches
fspreiss Nov 1, 2024
64a7b7b
NiDkgConfig::dkg_id now returns ref
fspreiss Nov 1, 2024
e0e60b9
Remove obsolete dkg_id param in NiDkgCspClient::create_dealing
fspreiss Nov 1, 2024
26e2b6a
Remove obsolete dkg_id param in NiDkgCspClient::verify_dealing
fspreiss Nov 1, 2024
14fec1d
Remove obsolete dkg_id param in NiDkgCspClient::verify_resharing_dealing
fspreiss Nov 1, 2024
7339b90
Cleanup
fspreiss Nov 1, 2024
7fda226
Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-make-…
fspreiss Nov 4, 2024
e98b7d8
Merge branch 'franzstefan/CRP-2597-make-nidkgtag-non-copy' into franz…
fspreiss Nov 4, 2024
dbaf13a
feat(crypto): CRP-2597-extend-nidkg-tag-with-high-threshold-for-keyid…
fspreiss Nov 5, 2024
c51dc23
Fix deps
fspreiss Nov 5, 2024
fce1c63
Remove obsolete conversion i32->NiDkgTag
fspreiss Nov 5, 2024
ac819a5
Merge branch 'franzstefan/CRP-2597-move-masterpublickeyid-proto-to-ty…
fspreiss Nov 6, 2024
06f027f
Merge branch 'franzstefan/CRP-2597-move-masterpublickeyid-proto-to-ty…
fspreiss Nov 6, 2024
6a5a578
Cleanup
fspreiss Nov 6, 2024
07fb339
Tests
fspreiss Nov 6, 2024
0c1c7a7
Do not benchmark NiDkgTag::HighThresholdForKey for now
fspreiss Nov 6, 2024
b17d8f9
Do not support new tag in new_with_inverted_threshold
fspreiss Nov 6, 2024
ac37697
Add test should_return_error_if_ni_dkg_tag_invalid_with_invalid_maste…
fspreiss Nov 6, 2024
7cfe2f7
Extend TODO description
fspreiss Nov 6, 2024
14cfb31
Add comment
fspreiss Nov 7, 2024
4fadc02
Store: rename capacity and improve docu
fspreiss Nov 7, 2024
3ad52e4
Typo in docu
fspreiss Nov 7, 2024
770c4e4
Typo
fspreiss Nov 7, 2024
cd99051
Custom ExhaustiveSet impl for NiDkgTag
fspreiss Nov 7, 2024
df71d3a
Rename to high_threshold_for_key_dkg_id_insertion_order
fspreiss Nov 7, 2024
caccc55
Fix store invariant
fspreiss Nov 7, 2024
4d167a8
Update ThresholdSigDataStore tests
fspreiss Nov 7, 2024
348d4a0
Merge branch 'franzstefan/CRP-2597-move-masterpublickeyid-proto-to-ty…
fspreiss Nov 7, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion rs/consensus/src/consensus/batch_delivery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ pub fn generate_responses_to_setup_initial_dkg_calls(
for (id, callback_id, transcript) in transcripts_for_remote_subnets.iter() {
let add_transcript = |transcript_results: &mut TranscriptResults| {
let value = Some(transcript.clone());
match id.dkg_tag {
match &id.dkg_tag {
NiDkgTag::LowThreshold => {
if transcript_results.low_threshold.is_some() {
error!(
Expand All @@ -327,6 +327,12 @@ pub fn generate_responses_to_setup_initial_dkg_calls(
}
transcript_results.high_threshold = value;
}
NiDkgTag::HighThresholdForKey(master_public_key_id) => {
error!(
log,
"Implementation error: NiDkgTag::HighThresholdForKey({master_public_key_id}) used in SetupInitialDKG for callback ID {callback_id}",
);
}
Comment on lines +330 to +335
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea here is to reuse the transcripts_for_remote_subnets field also for xnet reshares of VetKD keys. So then we would generalize this function such that it can generate the correct ConsensusResponse, depending on whether the callback ID is for a SetupInitialDKG context or a VetKD reshare context.

For now, logging an error and linking the ticket to generalize this function (CON-1416) should be fine.

}
};
match transcripts.get_mut(callback_id) {
Expand Down
9 changes: 6 additions & 3 deletions rs/consensus/src/consensus/dkg_key_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,9 +535,12 @@ impl Drop for DkgKeyManager {

/// Print the information about a [`NiDkgId`] in a concise way for logging
fn dkg_id_log_msg(id: &NiDkgId) -> String {
let tag = match id.dkg_tag {
NiDkgTag::LowThreshold => "low",
NiDkgTag::HighThreshold => "high",
let tag = match &id.dkg_tag {
NiDkgTag::LowThreshold => "low".to_string(),
NiDkgTag::HighThreshold => "high".to_string(),
NiDkgTag::HighThresholdForKey(master_public_key_id) => {
format!("highForKey({master_public_key_id})")
}
};

// If the target is local (which it is usually), we don't log the target
Expand Down
27 changes: 25 additions & 2 deletions rs/consensus/src/dkg/payload_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,21 @@ pub(crate) fn get_configs_for_local_transcripts(
resharing_transcript.cloned(),
)
}
////////////////////////////////////////////////////////////////////////////////
// TODO: how to behave here? The code below is copied+adapted from the HighThreshold case. However,
// this code currently won't be executed because we iterate over TAGS, which is a const that does not
// and cannot contain an NiDkgTag::HighThresholdForKey entry
///////////////////////////////////////////////////////////////////////////////
NiDkgTag::HighThresholdForKey(master_public_key_id) => {
let resharing_transcript = reshared_transcripts
.get(&NiDkgTag::HighThresholdForKey(master_public_key_id.clone()));
(
resharing_transcript
.map(|transcript| transcript.committee.get().clone())
.unwrap_or_else(|| node_ids.clone()),
resharing_transcript.cloned(),
)
}
Comment on lines +510 to +524
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behavior is correct (maybe it could even be combined with the NiDkgTag::HighThreshold branch). We can link ticket CON-1413 above, reminding us to iterate over all vetKD keys that were requested by registry in addition to the tags in TAGS.

};
let threshold =
NumberOfNodes::from(tag.threshold_for_subnet_of_size(node_ids.len()) as u32);
Expand Down Expand Up @@ -1206,9 +1221,13 @@ mod tests {
assert_eq!(conf.max_corrupt_dealers().get(), 2);
assert_eq!(
conf.threshold().get().get(),
match *tag {
match tag {
NiDkgTag::LowThreshold => 3,
NiDkgTag::HighThreshold => 5,
/////////////////////////////////////////
// TODO: check with Consensus team
NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"),
/////////////////////////////////////////
Comment on lines +1227 to +1230
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can link CON-1417 to extend this test once we have support for vet key transcripts in registry CUPs

}
);
}
Expand Down Expand Up @@ -1311,9 +1330,13 @@ mod tests {
assert_eq!(conf.max_corrupt_dealers().get(), 2);
assert_eq!(
conf.threshold().get().get(),
match *tag {
match tag {
NiDkgTag::LowThreshold => 3,
NiDkgTag::HighThreshold => 5,
/////////////////////////////////////////
// TODO: check with Consensus team
NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"),
/////////////////////////////////////////
}
Comment on lines +1336 to 1340
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can link CON-1413 to extend this test once we can create local transcript configs for vetKeys that were requested by registry

);

Expand Down
1 change: 1 addition & 0 deletions rs/crypto/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ DEPENDENCIES = [
"//rs/registry/helpers",
"//rs/registry/keys",
"//rs/types/base_types",
"//rs/types/management_canister_types",
"//rs/types/types",
"@crate_index//:bincode",
"@crate_index//:hex",
Expand Down
1 change: 1 addition & 0 deletions rs/crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ ic-crypto-tls-cert-validation = { path = "node_key_validation/tls_cert_validatio
ic-crypto-tls-interfaces = { path = "tls_interfaces" }
ic-crypto-utils-basic-sig = { path = "utils/basic_sig" }
ic-crypto-utils-tls = { path = "utils/tls" }
ic-management-canister-types = { path = "../types/management_canister_types" }
ic-interfaces = { path = "../interfaces" }
ic-interfaces-registry = { path = "../interfaces/registry" }
ic-logger = { path = "../monitoring/logger" }
Expand Down
2 changes: 2 additions & 0 deletions rs/crypto/benches/ni_dkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ impl TestCase {
let tag_name = match self.dkg_tag {
NiDkgTag::LowThreshold => "low",
NiDkgTag::HighThreshold => "high",
NiDkgTag::HighThresholdForKey(_) => unimplemented!(),
};
format!(
"crypto_nidkg_{}_nodes_{}_dealers_{}",
Expand All @@ -281,6 +282,7 @@ impl TestCase {
num_of_dealers: num_of_nodes,
dkg_tag,
},
NiDkgTag::HighThresholdForKey(_) => unimplemented!(),
}
}
}
Expand Down
1 change: 1 addition & 0 deletions rs/crypto/internal/crypto_service_provider/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ DEV_DEPENDENCIES = [
"//rs/crypto/utils/basic_sig",
"//rs/test_utilities/in_memory_logger",
"//rs/test_utilities/time",
"//rs/types/management_canister_types",
"//rs/types/types_test_utils",
"@crate_index//:assert_matches",
"@crate_index//:lazy_static",
Expand Down
1 change: 1 addition & 0 deletions rs/crypto/internal/crypto_service_provider/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ ic-crypto-test-utils-local-csp-vault = { path = "../../../crypto/test_utils/loca
ic-crypto-test-utils-metrics = { path = "../../../crypto/test_utils/metrics" }
ic-crypto-test-utils-reproducible-rng = { path = "../../../crypto/test_utils/reproducible_rng" }
ic-crypto-utils-basic-sig = { path = "../../utils/basic_sig" }
ic-management-canister-types = { path = "../../../types/management_canister_types" }
ic-test-utilities-compare-dirs = { path = "../../../test_utilities/compare_dirs" }
ic-test-utilities-in-memory-logger = { path = "../../../test_utilities/in_memory_logger" }
ic-test-utilities-time = { path = "../../../test_utilities/time" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ use ic_crypto_internal_types::sign::threshold_sig::ni_dkg::{
CspNiDkgDealing, CspNiDkgTranscript, Epoch,
};
use ic_crypto_internal_types::sign::threshold_sig::public_key::CspThresholdSigPublicKey;
use ic_management_canister_types::{
EcdsaCurve, EcdsaKeyId, MasterPublicKeyId, SchnorrAlgorithm, SchnorrKeyId, VetKdCurve,
VetKdKeyId,
};
use ic_types::crypto::threshold_sig::ni_dkg::config::dealers::NiDkgDealers;
use ic_types::crypto::threshold_sig::ni_dkg::config::receivers::NiDkgReceivers;
use ic_types::crypto::threshold_sig::ni_dkg::config::NiDkgThreshold;
Expand All @@ -22,7 +26,7 @@ use rand::Rng;
use rand_chacha::ChaCha20Rng;
use std::collections::BTreeMap;
use std::sync::Arc;
use strum::IntoEnumIterator;
use strum::{EnumCount, IntoEnumIterator};

// Generate random data structures:
// Alternatively we could implement Distribution for all of these types.
Expand All @@ -33,10 +37,45 @@ pub fn random_height(rng: &mut ChaCha20Rng) -> Height {
pub fn random_subnet_id(rng: &mut ChaCha20Rng) -> SubnetId {
subnet_test_id(rng.gen::<u64>())
}
pub fn random_master_public_key_id(rng: &mut ChaCha20Rng) -> MasterPublicKeyId {
assert_eq!(MasterPublicKeyId::COUNT, 3);
assert_eq!(EcdsaCurve::iter().count(), 1);
assert_eq!(SchnorrAlgorithm::iter().count(), 2);
assert_eq!(VetKdCurve::iter().count(), 1);

use rand::prelude::SliceRandom;
[
MasterPublicKeyId::Ecdsa(EcdsaKeyId {
curve: EcdsaCurve::Secp256k1,
name: "some key".to_string(),
}),
MasterPublicKeyId::Schnorr(SchnorrKeyId {
algorithm: SchnorrAlgorithm::Bip340Secp256k1,
name: "some key".to_string(),
}),
MasterPublicKeyId::Schnorr(SchnorrKeyId {
algorithm: SchnorrAlgorithm::Ed25519,
name: "some key".to_string(),
}),
MasterPublicKeyId::VetKd(VetKdKeyId {
curve: VetKdCurve::Bls12_381_G2,
name: "some key".to_string(),
}),
]
.choose(rng)
.cloned()
.expect("Could not choose a MasterPublicKeyId")
}
pub fn random_ni_dkg_tag(rng: &mut ChaCha20Rng) -> NiDkgTag {
NiDkgTag::iter()
.choose(rng)
.expect("Could not choose a NiDkgTag")
use rand::prelude::SliceRandom;
[
NiDkgTag::LowThreshold,
NiDkgTag::HighThreshold,
NiDkgTag::HighThresholdForKey(random_master_public_key_id(rng)),
]
.choose(rng)
.cloned()
.expect("Could not choose a NiDkgTag")
}
pub fn random_ni_dkg_id(rng: &mut ChaCha20Rng) -> NiDkgId {
NiDkgId {
Expand Down
22 changes: 10 additions & 12 deletions rs/crypto/src/sign/threshold_sig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ mod tests;
pub struct ThresholdSignerInternal {}

impl ThresholdSignerInternal {
////////////////////////////////////////
#[allow(clippy::result_large_err)]
////////////////////////////////////////
pub fn sign_threshold<C: ThresholdSignatureCspClient, H: Signable>(
lockable_threshold_sig_data_store: &LockableThresholdSigDataStore,
threshold_sig_csp_client: &C,
Expand Down Expand Up @@ -70,6 +73,9 @@ fn sig_data_not_found_error(dkg_id: NiDkgId) -> ThresholdSigDataNotFoundError {
ThresholdSigDataNotFoundError::ThresholdSigDataNotFound { dkg_id }
}

////////////////////////////////////////
#[allow(clippy::result_large_err)]
////////////////////////////////////////
fn threshold_sig_share_or_panic<H: Signable>(
csp_signature: CspSignature,
) -> Result<ThresholdSigShareOf<H>, ThresholdSignError> {
Expand Down Expand Up @@ -441,12 +447,8 @@ impl ThresholdSigVerifierInternal {
H: Signable,
{
let csp_signature = CspSignature::try_from(signature)?;
let transcript = initial_ni_dkg_transcript_from_registry(
registry,
subnet_id,
version,
NiDkgTag::HighThreshold,
)?;
let transcript =
initial_high_threshold_ni_dkg_transcript_from_registry(registry, subnet_id, version)?;
let csp_pub_coeffs = CspPublicCoefficients::from(&transcript);
threshold_sig_csp_client
.threshold_verify_combined_signature(
Expand All @@ -459,20 +461,16 @@ impl ThresholdSigVerifierInternal {
}
}

fn initial_ni_dkg_transcript_from_registry(
fn initial_high_threshold_ni_dkg_transcript_from_registry(
registry: &dyn RegistryClient,
subnet_id: SubnetId,
registry_version: RegistryVersion,
dkg_tag: NiDkgTag,
) -> CryptoResult<NiDkgTranscript> {
let maybe_transcripts = registry
.get_initial_dkg_transcripts(subnet_id, registry_version)
.map_err(CryptoError::RegistryClient)?;
match maybe_transcripts.value {
Some(transcripts) => Ok(match dkg_tag {
NiDkgTag::LowThreshold => transcripts.low_threshold,
NiDkgTag::HighThreshold => transcripts.high_threshold,
}),
Some(transcripts) => Ok(transcripts.high_threshold),
None => Err(CryptoError::DkgTranscriptNotFound {
subnet_id,
registry_version,
Expand Down
Loading