Skip to content

Commit

Permalink
Merge branch 'mathias-CRP-1993-remove-idkg-dealing-encryption-pubkeys…
Browse files Browse the repository at this point in the history 
…-count-method-from-keymanager-trait' into 'master'

chore(crypto): CRP-1993 remove idkg_dealing_encryption_pubkeys_count method from KeyManager trait

Remove the `idkg_dealing_encryption_pubkeys_count` method from the `KeyManager` trait. The method was initially introduces to facilitate testing, but the introduction of `ic_crypto_test_utils_metrics::assertions::MetricsObservationsAssert`, it is no longer necessary to keep the method in the trait. The functionality tested by the deleted tests are all covered by other existing tests (both the delegation from the crypto component to the CSP, and the key counting itself within the implementation of the CSP vault). 

See merge request dfinity-lab/public/ic!11761
  • Loading branch information
@mbjorkqvist
mbjorkqvist committed Apr 11, 2023
2 parents 4374be1 + 31509f5 commit 3d0b069
Show file tree
Hide file tree
Showing 8 changed files with 11 additions and 183 deletions.
12 changes: 1 addition & 11 deletions rs/crypto/internal/crypto_service_provider/src/api/keygen/errors.rs
View file
@@ -1,5 +1,5 @@
use crate::CspPublicKeyStoreError;
use ic_interfaces::crypto::{CurrentNodePublicKeysError, IdkgDealingEncPubKeysCountError};
use ic_interfaces::crypto::CurrentNodePublicKeysError;
use ic_types::crypto::CryptoError;
use serde::{Deserialize, Serialize};
use std::fmt;
Expand Down Expand Up @@ -59,13 +59,3 @@ impl From<NodePublicKeyDataError> for CryptoError {
}
}
}

impl From<NodePublicKeyDataError> for IdkgDealingEncPubKeysCountError {
fn from(e: NodePublicKeyDataError) -> Self {
match e {
NodePublicKeyDataError::TransientInternalError(internal_error) => {
IdkgDealingEncPubKeysCountError::TransientInternalError(internal_error)
}
}
}
}
13 changes: 3 additions & 10 deletions rs/crypto/src/keygen/mod.rs
View file
Expand Up @@ -13,7 +13,7 @@ use ic_crypto_internal_logmon::metrics::{
};
use ic_interfaces::crypto::{
CheckKeysWithRegistryError, CurrentNodePublicKeysError, IDkgDealingEncryptionKeyRotationError,
IDkgKeyRotationResult, IdkgDealingEncPubKeysCountError, KeyManager, KeyRotationOutcome,
IDkgKeyRotationResult, KeyManager, KeyRotationOutcome,
};
use ic_logger::{error, info, warn};
use ic_protobuf::registry::crypto::v1::{PublicKey as PublicKeyProto, X509PublicKeyCert};
Expand Down Expand Up @@ -114,13 +114,6 @@ impl<C: CryptoServiceProvider> KeyManager for CryptoComponentImpl<C> {
self.record_key_rotation_metrics(&key_rotation_result);
key_rotation_result
}

fn idkg_dealing_encryption_pubkeys_count(
&self,
) -> Result<usize, IdkgDealingEncPubKeysCountError> {
let result = self.csp.idkg_dealing_encryption_pubkeys_count()?;
Ok(result)
}
}

// Helpers for implementing `KeyManager`-trait.
Expand Down Expand Up @@ -455,14 +448,14 @@ impl<C: CryptoServiceProvider> CryptoComponentImpl<C> {
}

fn observe_number_of_idkg_dealing_encryption_public_keys(&self) {
match self.idkg_dealing_encryption_pubkeys_count() {
match self.csp.idkg_dealing_encryption_pubkeys_count() {
Ok(num_idkg_dealing_encryption_pubkeys) => {
self.metrics.observe_idkg_dealing_encryption_pubkey_count(
num_idkg_dealing_encryption_pubkeys,
MetricsResult::Ok,
);
}
Err(IdkgDealingEncPubKeysCountError::TransientInternalError(internal_error)) => {
Err(NodePublicKeyDataError::TransientInternalError(internal_error)) => {
warn!(
self.logger,
"Transient error retrieving local iDKG dealing encryption public key count: {}",
Expand Down
95 changes: 1 addition & 94 deletions rs/crypto/src/keygen/tests.rs
View file
Expand Up @@ -14,7 +14,7 @@ use ic_crypto_internal_csp::vault::api::PksAndSksContainsErrors;
use ic_crypto_internal_csp::vault::api::SecretKeyError;
use ic_crypto_internal_logmon::metrics::CryptoMetrics;
use ic_crypto_internal_threshold_sig_ecdsa::MEGaPublicKey;
use ic_crypto_temp_crypto::{EcdsaSubnetConfig, NodeKeysToGenerate, TempCryptoComponent};
use ic_crypto_temp_crypto::EcdsaSubnetConfig;
use ic_crypto_test_utils_csp::MockAllCryptoServiceProvider;
use ic_crypto_test_utils_keys::public_keys::{
valid_committee_signing_public_key, valid_dkg_dealing_encryption_public_key,
Expand Down Expand Up @@ -1214,37 +1214,6 @@ mod rotate_idkg_dealing_encryption_keys {
);
}

#[test]
fn should_correctly_get_idkg_dealing_encryption_pubkeys_count_for_multiple_keys() {
let setup = Setup::builder()
.with_csp_idkg_dealing_encryption_public_keys_count_result(Ok(2))
.build();

let idkg_dealing_encryption_pubkeys_count =
setup.crypto.idkg_dealing_encryption_pubkeys_count();
assert_matches!(idkg_dealing_encryption_pubkeys_count, Ok(2));
}

#[test]
fn should_correctly_return_idkg_dealing_encryption_pubkeys_count_error() {
const ERROR_MSG: &str = "error retrieving idkg dealing encryption key count";
let setup = Setup::builder()
.with_csp_idkg_dealing_encryption_public_keys_count_result(Err(
NodePublicKeyDataError::TransientInternalError(ERROR_MSG.to_string()),
))
.build();

let idkg_dealing_encryption_pubkeys_count =
setup.crypto.idkg_dealing_encryption_pubkeys_count();
assert_matches!(
idkg_dealing_encryption_pubkeys_count,
Err(IdkgDealingEncPubKeysCountError::TransientInternalError(
internal_error
))
if internal_error.contains(ERROR_MSG)
);
}

#[test]
fn should_return_error_when_registry_error() {
const ERROR_STR: &str = "registry client poll lock failed!";
Expand Down Expand Up @@ -1774,68 +1743,6 @@ mod rotate_idkg_dealing_encryption_keys {
}
}

mod idkg_dealing_encryption_pubkeys_count {
use super::*;
use ic_base_types::{NodeId, PrincipalId};

#[test]
fn should_correctly_count_idkg_dealing_encryption_pubkeys_when_all_keys_present() {
let crypto_component = TempCryptoComponent::builder()
.with_keys(NodeKeysToGenerate::all())
.build();
let key_counts = crypto_component
.idkg_dealing_encryption_pubkeys_count()
.expect("Error calling idkg_dealing_encryption_pubkeys_count");
assert_eq!(1, key_counts);
}

#[test]
fn should_correctly_count_idkg_dealing_encryption_pubkeys_when_no_keys_present() {
let crypto_component = TempCryptoComponent::builder()
.with_keys(NodeKeysToGenerate::none())
.build();
let key_counts = crypto_component
.idkg_dealing_encryption_pubkeys_count()
.expect("Error calling idkg_dealing_encryption_pubkeys_count");
assert_eq!(0, key_counts);
}

#[test]
fn should_have_idkg_dealing_encryption_pubkeys_count_returning_transient_error_if_csp_call_fails(
) {
use ic_crypto_internal_csp::api::NodePublicKeyDataError;
use ic_crypto_test_utils_csp::MockAllCryptoServiceProvider;
use ic_interfaces::crypto::KeyManager;
use ic_logger::replica_logger::no_op_logger;

let mut csp = MockAllCryptoServiceProvider::new();
const DETAILS_STR: &str = "test";
csp.expect_idkg_dealing_encryption_pubkeys_count()
.return_const(Err(NodePublicKeyDataError::TransientInternalError(
DETAILS_STR.to_string(),
)));

let registry_data = Arc::new(ProtoRegistryDataProvider::new());

let registry_client =
Arc::new(FakeRegistryClient::new(Arc::clone(&registry_data) as Arc<_>));

let crypto_component = CryptoComponentImpl::new_with_csp_and_fake_node_id(
csp,
no_op_logger(),
registry_client.clone(),
NodeId::from(PrincipalId::new_node_test_id(42)),
Arc::new(CryptoMetrics::none()),
None,
);
registry_client.reload();

let result = crypto_component.idkg_dealing_encryption_pubkeys_count();

assert_matches!(result, Err(IdkgDealingEncPubKeysCountError::TransientInternalError(details)) if details == DETAILS_STR);
}
}

struct Setup {
metrics_registry: MetricsRegistry,
crypto: CryptoComponentImpl<MockAllCryptoServiceProvider>,
Expand Down
13 changes: 3 additions & 10 deletions rs/crypto/temp_crypto/src/lib.rs
View file
Expand Up @@ -20,9 +20,9 @@ use ic_crypto_utils_time::CurrentSystemTimeSource;
use ic_interfaces::crypto::{
BasicSigVerifier, BasicSigVerifierByPublicKey, BasicSigner, CanisterSigVerifier,
CheckKeysWithRegistryError, CurrentNodePublicKeysError, IDkgDealingEncryptionKeyRotationError,
IDkgKeyRotationResult, IDkgProtocol, IdkgDealingEncPubKeysCountError, KeyManager,
LoadTranscriptResult, MultiSigVerifier, MultiSigner, NiDkgAlgorithm, ThresholdEcdsaSigVerifier,
ThresholdEcdsaSigner, ThresholdSigVerifier, ThresholdSigVerifierByPublicKey, ThresholdSigner,
IDkgKeyRotationResult, IDkgProtocol, KeyManager, LoadTranscriptResult, MultiSigVerifier,
MultiSigner, NiDkgAlgorithm, ThresholdEcdsaSigVerifier, ThresholdEcdsaSigner,
ThresholdSigVerifier, ThresholdSigVerifierByPublicKey, ThresholdSigner,
};
use ic_interfaces::time_source::TimeSource;
use ic_interfaces_registry::RegistryClient;
Expand Down Expand Up @@ -910,13 +910,6 @@ impl<C: CryptoServiceProvider> KeyManager for TempCryptoComponentGeneric<C> {
self.crypto_component
.rotate_idkg_dealing_encryption_keys(registry_version)
}

fn idkg_dealing_encryption_pubkeys_count(
&self,
) -> Result<usize, IdkgDealingEncPubKeysCountError> {
self.crypto_component
.idkg_dealing_encryption_pubkeys_count()
}
}

impl<C: CryptoServiceProvider> NiDkgAlgorithm for TempCryptoComponentGeneric<C> {
Expand Down
8 changes: 0 additions & 8 deletions rs/interfaces/src/crypto/keygen.rs
View file
Expand Up @@ -71,14 +71,6 @@ pub trait KeyManager {
&self,
registry_version: RegistryVersion,
) -> Result<IDkgKeyRotationResult, IDkgDealingEncryptionKeyRotationError>;

/// Returns the number of iDKG dealing encryption public keys stored locally.
///
/// # Errors
/// * if a transient error (e.g., RPC timeout) occurs when accessing the public key store
fn idkg_dealing_encryption_pubkeys_count(
&self,
) -> Result<usize, IdkgDealingEncPubKeysCountError>;
}

#[derive(Clone, Debug, PartialEq, Eq)]
Expand Down
36 changes: 0 additions & 36 deletions rs/interfaces/src/crypto/keygen/errors.rs
View file
Expand Up @@ -48,39 +48,3 @@ impl From<CurrentNodePublicKeysError> for CryptoError {
}
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub enum IdkgDealingEncPubKeysCountError {
TransientInternalError(String),
}

impl fmt::Display for IdkgDealingEncPubKeysCountError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
IdkgDealingEncPubKeysCountError::TransientInternalError(details) => {
let appendix = if details.is_empty() {
String::default()
} else {
format!(": {}", details)
};

write!(
f,
"Transient internal error occurred while retrieving iDKG dealing encryption public keys count{appendix}"
)
}
}
}
}

impl From<IdkgDealingEncPubKeysCountError> for CryptoError {
fn from(e: IdkgDealingEncPubKeysCountError) -> CryptoError {
match e {
IdkgDealingEncPubKeysCountError::TransientInternalError(details) => {
CryptoError::TransientInternalError {
internal_error: details,
}
}
}
}
}
5 changes: 0 additions & 5 deletions rs/orchestrator/src/registration.rs
View file
Expand Up @@ -710,7 +710,6 @@ mod tests {
use ic_crypto_tls_interfaces::TlsServerHandshakeError;
use ic_crypto_tls_interfaces::TlsStream;
use ic_interfaces::crypto::IDkgDealingEncryptionKeyRotationError;
use ic_interfaces::crypto::IdkgDealingEncPubKeysCountError;
use ic_interfaces::crypto::KeyManager;
use ic_interfaces::crypto::ThresholdSigVerifierByPublicKey;
use ic_interfaces::crypto::{BasicSigner, CheckKeysWithRegistryError};
Expand Down Expand Up @@ -759,10 +758,6 @@ mod tests {
&self,
registry_version: RegistryVersion,
) -> Result<IDkgKeyRotationResult, IDkgDealingEncryptionKeyRotationError>;

fn idkg_dealing_encryption_pubkeys_count(
&self,
) -> Result<usize, IdkgDealingEncPubKeysCountError>;
}

pub trait BasicSigner<MessageId> {
Expand Down
12 changes: 3 additions & 9 deletions rs/test_utilities/src/crypto.rs
View file
Expand Up @@ -11,9 +11,9 @@ use ic_crypto_test_utils_canister_threshold_sigs::{
use ic_interfaces::crypto::{
BasicSigVerifier, BasicSigVerifierByPublicKey, BasicSigner, CanisterSigVerifier,
CheckKeysWithRegistryError, CurrentNodePublicKeysError, IDkgDealingEncryptionKeyRotationError,
IDkgKeyRotationResult, IDkgProtocol, IdkgDealingEncPubKeysCountError, KeyManager,
LoadTranscriptResult, NiDkgAlgorithm, ThresholdEcdsaSigVerifier, ThresholdEcdsaSigner,
ThresholdSigVerifier, ThresholdSigVerifierByPublicKey, ThresholdSigner,
IDkgKeyRotationResult, IDkgProtocol, KeyManager, LoadTranscriptResult, NiDkgAlgorithm,
ThresholdEcdsaSigVerifier, ThresholdEcdsaSigner, ThresholdSigVerifier,
ThresholdSigVerifierByPublicKey, ThresholdSigner,
};
use ic_interfaces::crypto::{MultiSigVerifier, MultiSigner};
use ic_interfaces_registry::RegistryClient;
Expand Down Expand Up @@ -476,12 +476,6 @@ impl KeyManager for CryptoReturningOk {
) -> Result<IDkgKeyRotationResult, IDkgDealingEncryptionKeyRotationError> {
unimplemented!()
}

fn idkg_dealing_encryption_pubkeys_count(
&self,
) -> Result<usize, IdkgDealingEncPubKeysCountError> {
unimplemented!()
}
}

impl IDkgProtocol for CryptoReturningOk {
Expand Down

0 comments on commit 3d0b069

Please sign in to comment.