diff --git a/Cargo.lock b/Cargo.lock index 3de39c22ef4b..85beca7f7806 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7618,6 +7618,7 @@ dependencies = [ "ic-artifact-pool", "ic-canonical-state", "ic-canonical-state-tree-hash", + "ic-config", "ic-consensus-mocks", "ic-consensus-utils", "ic-crypto-test-utils-crypto-returning-ok", @@ -7629,9 +7630,11 @@ dependencies = [ "ic-metrics", "ic-registry-subnet-type", "ic-replicated-state", + "ic-test-artifact-pool", "ic-test-utilities", "ic-test-utilities-consensus", "ic-test-utilities-logger", + "ic-test-utilities-registry", "ic-test-utilities-types", "ic-types", "ic-types-test-utils", diff --git a/rs/consensus/certification/BUILD.bazel b/rs/consensus/certification/BUILD.bazel index 513fc9ca0388..574c6f36bc72 100644 --- a/rs/consensus/certification/BUILD.bazel +++ b/rs/consensus/certification/BUILD.bazel @@ -39,6 +39,7 @@ rust_test( "//rs/artifact_pool", "//rs/canonical_state", "//rs/canonical_state/tree_hash", + "//rs/config", "//rs/consensus/mocks", "//rs/consensus/utils", "//rs/crypto/test_utils/crypto_returning_ok", @@ -51,8 +52,10 @@ rust_test( "//rs/registry/subnet_type", "//rs/replicated_state", "//rs/test_utilities", + "//rs/test_utilities/artifact_pool", "//rs/test_utilities/consensus", "//rs/test_utilities/logger", + "//rs/test_utilities/registry", "//rs/test_utilities/types", "//rs/types/types", "//rs/types/types_test_utils", diff --git a/rs/consensus/certification/Cargo.toml b/rs/consensus/certification/Cargo.toml index ea337a24d4a0..6163c3b3bfff 100644 --- a/rs/consensus/certification/Cargo.toml +++ b/rs/consensus/certification/Cargo.toml @@ -25,11 +25,14 @@ slog = { workspace = true } assert_matches = { workspace = true } ic-artifact-pool = { path = "../../artifact_pool" } ic-consensus-mocks = { path = "../mocks" } +ic-config = { path = "../../config" } ic-crypto-test-utils-crypto-returning-ok = { path = "../../crypto/test_utils/crypto_returning_ok" } ic-registry-subnet-type = { path = "../../registry/subnet_type" } +ic-test-artifact-pool = { path = "../../test_utilities/artifact_pool" } ic-test-utilities = { path = "../../test_utilities" } ic-test-utilities-consensus = { path = "../../test_utilities/consensus" } ic-test-utilities-logger = { path = "../../test_utilities/logger" } +ic-test-utilities-registry = { path = "../../test_utilities/registry" } ic-test-utilities-types = { path = "../../test_utilities/types" } ic-types-test-utils = { path = "../../types/types_test_utils" } mockall = { workspace = true } diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index a02af098f609..a14ac83919cd 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -4,6 +4,7 @@ use ic_canonical_state_tree_hash::lazy_tree::materialize::materialize; use ic_consensus_utils::{ MINIMUM_CHAIN_LENGTH, active_high_threshold_nidkg_id, aggregate, bouncer_metrics::BouncerMetrics, membership::Membership, registry_version_at_height, + subnet_splitting_status_at_height, }; use ic_crypto_tree_hash::{Witness, recompute_digest}; use ic_interfaces::{ @@ -14,7 +15,7 @@ use ic_interfaces::{ }; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::{StateHashMetadata, StateManager}; -use ic_logger::{ReplicaLogger, debug, error, trace}; +use ic_logger::{ReplicaLogger, debug, error, info, trace, warn}; use ic_metrics::{MetricsRegistry, buckets::decimal_buckets}; use ic_replicated_state::ReplicatedState; use ic_types::{ @@ -25,6 +26,7 @@ use ic_types::{ certification::{ Certification, CertificationContent, CertificationMessage, CertificationShare, }, + dkg::{PostSplitArgs, SubnetSplittingStatus}, }, crypto::{CryptoHash, Signed}, replica_config::ReplicaConfig, @@ -340,6 +342,18 @@ impl CertifierImpl { .shares_at_height(state_hash_metadata.height) .all(|share| share.signed.signature.signer != self.replica_config.node_id) }) + // Filter out all heights, where the subnet splitting is taking place + .filter(|state_hash_metadata| { + self.should_skip_due_to_subnet_splitting(state_hash_metadata.height) + .inspect_err(|err| { + warn!( + self.log, + "Failed to check the subnet splitting status: {err}. \ + Skipping creation of the certificate share" + ) + }) + .is_ok_and(|should_skip| !should_skip) + }) .cloned() .filter_map(|state_hash_metadata| { let content = CertificationContent::new(state_hash_metadata.hash); @@ -479,6 +493,32 @@ impl CertifierImpl { let registry_version = registry_version_at_height(self.consensus_pool_cache.as_ref(), certification.height)?; + // If a subnet splitting is taking place, we need to skip validating certifications (and + // shares). In particular because after a split, before replicas get restarted, they are + // still under the same P2P network and can gossip certifications for states of different + // subnets. + match self.should_skip_due_to_subnet_splitting(certification.height) { + Ok(true) => { + info!( + every_n_seconds => 30, + self.log, + "Skipping the validation of a certification at height {} because a \ + subnet splitting is taking place", + certification.height + ); + return None; + } + Ok(false) => {} + Err(err) => { + warn!( + self.log, + "Failed to check the subnet splitting status: {err}. \ + Skipping validation of the certificate" + ); + return None; + } + } + // check if the certification is indeed valid for the specified height. If // not, we consider the certification invalid. if let Err(e) = validate_height_witness( @@ -517,6 +557,32 @@ impl CertifierImpl { let msg = CertificationMessage::CertificationShare(share.clone()); let content = &share.signed.content; + // If a subnet splitting is taking place, we need to skip validating certifications (and + // shares). In particular because after a split, before replicas get restarted, they are + // still under the same P2P network and can gossip certifications for states of different + // subnets. + match self.should_skip_due_to_subnet_splitting(share.height) { + Ok(true) => { + info!( + every_n_seconds => 30, + self.log, + "Skipping the validation of a certification share at height {} because a \ + subnet splitting is taking place", + share.height + ); + return None; + } + Ok(false) => {} + Err(err) => { + warn!( + self.log, + "Failed to check the subnet splitting status: {err}. \ + Skipping validation of the certificate share" + ); + return None; + } + } + // If the share has an invalid content or does not belong to the // committee if let Err(e) = validate_height_witness( @@ -582,6 +648,24 @@ impl CertifierImpl { } } } + + /// Checks if we should skip the creation and/or validation of certifications/shares + /// at the given height, due to an ongoing subnet splitting. + fn should_skip_due_to_subnet_splitting(&self, height: Height) -> Result { + match subnet_splitting_status_at_height(self.consensus_pool_cache.as_ref(), height) { + None => Err(format!( + "Missing finalized summary block for height {height}" + )), + Some(SubnetSplittingStatus::NotScheduled) => Ok(false), + // Don't produce certifications in the dkg interval where the subnet splitting is + // happening as it will be skipped by consensus anyways + Some(SubnetSplittingStatus::Scheduled(..)) => Ok(true), + // Wait for the replica to be restarted with the new `subnet_id` + Some(SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id })) => { + Ok(new_subnet_id != self.replica_config.subnet_id) + } + } + } } fn validate_height_witness( @@ -616,7 +700,8 @@ mod tests { use ic_canonical_state::lazy_tree_conversion::replicated_state_as_lazy_tree; use ic_canonical_state_tree_hash::hash_tree::hash_lazy_tree; use ic_canonical_state_tree_hash::lazy_tree::materialize::materialize_partial; - use ic_consensus_mocks::{Dependencies, dependencies}; + use ic_config::artifact_pool::ArtifactPoolConfig; + use ic_consensus_mocks::{Dependencies, dependencies, dependencies_with_subnet_params}; use ic_crypto_tree_hash::{Digest, Witness, sparse_labeled_tree_from_paths}; use ic_interfaces::{ certification::CertificationPool, @@ -624,9 +709,13 @@ mod tests { }; use ic_interfaces_state_manager::StateHashMetadata; use ic_registry_subnet_type::SubnetType; + use ic_test_artifact_pool::consensus_pool::TestConsensusPool; use ic_test_utilities_consensus::fake::*; use ic_test_utilities_logger::with_test_replica_logger; + use ic_test_utilities_registry::SubnetRecordBuilder; use ic_test_utilities_types::ids::{node_test_id, subnet_test_id}; + use ic_types::backwards_compatibility::BackwardsCompatible; + use ic_types::consensus::{BlockPayload, HashedBlock, Payload, dkg::SplittingArgs}; use ic_types::{ CryptoHashOfPartialState, Height, artifact::CertificationMessageId, @@ -1557,4 +1646,305 @@ mod tests { }) }) } + + // DKG interval length used for subnet-splitting tests. + const TEST_DKG_INTERVAL: u64 = 9; + + fn dependencies_for_splitting_tests( + pool_config: ArtifactPoolConfig, + nodes: u64, + ) -> Dependencies { + let committee = (0..nodes).map(node_test_id).collect::>(); + dependencies_with_subnet_params( + pool_config, + subnet_test_id(0), + vec![( + 1, + SubnetRecordBuilder::from(&committee) + .with_dkg_interval_length(TEST_DKG_INTERVAL) + .build(), + )], + ) + } + + // Advances `pool` by TEST_DKG_INTERVAL rounds so the next block is a DKG + // summary block, then inserts and finalizes that summary block after setting + // its subnet-splitting status to `status`. + // + // Returns the height of the newly finalized summary block. Heights in + // [split_height, split_height + TEST_DKG_INTERVAL] are covered by this + // summary, so `subnet_splitting_status_at_height` will return `status` for + // any of those heights. + fn advance_to_splitting_interval( + pool: &mut TestConsensusPool, + status: SubnetSplittingStatus, + ) -> Height { + pool.advance_round_normal_operation_n(TEST_DKG_INTERVAL); + + let mut proposal = pool.make_next_block(); + let block = proposal.content.as_mut(); + let mut payload = block.payload.as_ref().as_summary().clone(); + payload.dkg.subnet_splitting_status = BackwardsCompatible::new_for_test_only(Some(status)); + block.payload = Payload::new( + ic_types::crypto::crypto_hash, + BlockPayload::Summary(payload), + ); + proposal.content = HashedBlock::new(ic_types::crypto::crypto_hash, block.clone()); + + pool.advance_round_with_block(&proposal); + + proposal.height() + } + + fn not_scheduled_splitting() -> SubnetSplittingStatus { + SubnetSplittingStatus::NotScheduled + } + + fn scheduled_splitting() -> SubnetSplittingStatus { + SubnetSplittingStatus::Scheduled(SplittingArgs { + source_subnet_id: subnet_test_id(0), + destination_subnet_id: subnet_test_id(1), + }) + } + + fn done_splitting_different_subnet() -> SubnetSplittingStatus { + SubnetSplittingStatus::PostSplit(PostSplitArgs { + new_subnet_id: subnet_test_id(1), + }) + } + + fn done_splitting_same_subnet() -> SubnetSplittingStatus { + SubnetSplittingStatus::PostSplit(PostSplitArgs { + new_subnet_id: subnet_test_id(0), + }) + } + + fn assert_for_all_subnet_splitting_statuses( + pool: &mut TestConsensusPool, + mut test: impl FnMut(SubnetSplittingStatus, Height), + ) { + for status in [ + not_scheduled_splitting(), + scheduled_splitting(), + done_splitting_different_subnet(), + done_splitting_same_subnet(), + ] { + let splitting_height = advance_to_splitting_interval(pool, status); + for test_height in splitting_height.get()..=splitting_height.get() + TEST_DKG_INTERVAL { + let test_height = Height::from(test_height); + + test(status, test_height); + } + } + } + + /// Signing should be skipped for heights covered by a `Scheduled` or `Done` with different + /// subnet ID splitting interval. + /// In a `Done` interval with same subnet ID, signing should proceed as normal. + #[test] + fn test_sign_skips_during_subnet_splitting() { + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + with_test_replica_logger(|log| { + let Dependencies { + mut pool, + replica_config, + registry, + crypto, + state_manager, + .. + } = dependencies_for_splitting_tests(pool_config.clone(), 4); + + let metrics_registry = MetricsRegistry::new(); + let cert_pool = CertificationPoolImpl::new( + replica_config.node_id, + pool_config, + ic_logger::replica_logger::no_op_logger(), + metrics_registry.clone(), + ); + let certifier = CertifierImpl::new( + replica_config, + registry, + crypto, + state_manager, + pool.get_cache(), + metrics_registry, + log, + ); + + assert_for_all_subnet_splitting_statuses(&mut pool, |status, test_height| { + let shares = certifier.sign( + &cert_pool, + &[StateHashMetadata { + height: test_height, + hash: CryptoHashOfPartialState::from(CryptoHash(vec![1, 2, 3])), + height_witness: Witness::new_for_testing_with_height(), + }], + ); + + match status { + SubnetSplittingStatus::Scheduled(..) => { + assert!( + shares.is_empty(), + "Expected no shares during subnet splitting, got: {shares:?}" + ); + } + SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id }) + if new_subnet_id != subnet_test_id(0) => + { + assert!( + shares.is_empty(), + "Expected no shares after Done splitting with different subnet ID, got: {shares:?}" + ); + } + SubnetSplittingStatus::NotScheduled + | SubnetSplittingStatus::PostSplit(..) => { + assert!( + !shares.is_empty(), + "Expected shares when not splitting or splitting with same subnet ID" + ); + } + } + }); + }) + }) + } + + /// An incoming share at a height inside a `Scheduled` or `Done` with different subnet ID + /// splitting interval should be ignored and not validated, as it could be from the other + /// subnet. + /// In a `Done` interval with same subnet ID, shares should be validated as normal. + #[test] + fn test_validate_share_handles_invalid_during_scheduled_subnet_splitting() { + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + with_test_replica_logger(|log| { + let Dependencies { + mut pool, + replica_config, + registry, + crypto, + state_manager, + .. + } = dependencies_for_splitting_tests(pool_config.clone(), 4); + + let metrics_registry = MetricsRegistry::new(); + let cert_pool = CertificationPoolImpl::new( + replica_config.node_id, + pool_config, + ic_logger::replica_logger::no_op_logger(), + metrics_registry.clone(), + ); + let certifier = CertifierImpl::new( + replica_config, + registry, + crypto, + state_manager, + pool.get_cache(), + metrics_registry, + log, + ); + + assert_for_all_subnet_splitting_statuses(&mut pool, |status, test_height| { + let content = gen_content(test_height); + let share = CertificationShare { + height: test_height, + height_witness: Witness::new_for_testing_with_height(), + signed: Signed { + content, + signature: ThresholdSignatureShare::fake(node_test_id(1)), + }, + }; + + let result = certifier.validate_share(&cert_pool, &share); + match status { + SubnetSplittingStatus::Scheduled(..) => { + assert_eq!(result, None, "Expected None during subnet splitting"); + } + SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id }) + if new_subnet_id != subnet_test_id(0) => + { + assert_eq!( + result, None, + "Expected None after Done splitting with different subnet ID" + ); + } + SubnetSplittingStatus::NotScheduled + | SubnetSplittingStatus::PostSplit(..) => { + assert_eq!( + result, + Some(ChangeAction::MoveToValidated( + CertificationMessage::CertificationShare(share) + )), + "Expected MoveToValidated when not splitting or splitting with same subnet ID" + ); + } + } + }); + }) + }) + } + + /// Full certifications received during a `Scheduled` or `Done` with different subnet ID + /// splitting interval should be ignored and not validated, as they could be from the other + #[test] + fn test_validate_certification_validates_despite_scheduled_subnet_splitting() { + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + with_test_replica_logger(|log| { + let Dependencies { + mut pool, + replica_config, + registry, + crypto, + state_manager, + .. + } = dependencies_for_splitting_tests(pool_config.clone(), 1); + + let certifier = CertifierImpl::new( + replica_config, + registry, + crypto, + state_manager, + pool.get_cache(), + MetricsRegistry::new(), + log, + ); + + assert_for_all_subnet_splitting_statuses(&mut pool, |status, test_height| { + let content = gen_content(test_height); + let cert = Certification { + height: test_height, + height_witness: Some(Witness::new_for_testing_with_height()), + signed: Signed { + content, + signature: ThresholdSignature::fake(), + }, + }; + + let result = certifier.validate_certification(&cert); + match status { + SubnetSplittingStatus::Scheduled(..) => { + assert_eq!(result, None, "Expected None during subnet splitting"); + } + SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id }) + if new_subnet_id != subnet_test_id(0) => + { + assert_eq!( + result, None, + "Expected None after Done splitting with different subnet ID" + ); + } + SubnetSplittingStatus::NotScheduled + | SubnetSplittingStatus::PostSplit(..) => { + assert_eq!( + result, + Some(ChangeAction::MoveToValidated( + CertificationMessage::Certification(cert.clone()) + )), + "Expected MoveToValidated when not splitting or splitting with same subnet ID" + ); + } + } + }); + }) + }) + } } diff --git a/rs/consensus/utils/src/lib.rs b/rs/consensus/utils/src/lib.rs index f0bbec0bdb0d..f425551a0f55 100644 --- a/rs/consensus/utils/src/lib.rs +++ b/rs/consensus/utils/src/lib.rs @@ -12,7 +12,10 @@ use ic_registry_client_helpers::subnet::{NotarizationDelaySettings, SubnetRegist use ic_replicated_state::ReplicatedState; use ic_types::{ Height, NodeId, RegistryVersion, ReplicaVersion, SubnetId, - consensus::{Block, BlockProposal, HasCommittee, HasHeight, HasRank, Threshold}, + consensus::{ + Block, BlockProposal, HasCommittee, HasHeight, HasRank, Threshold, + dkg::SubnetSplittingStatus, + }, crypto::{ Signed, threshold_sig::ni_dkg::{NiDkgId, NiDkgReceivers, NiDkgTag, NiDkgTranscript}, @@ -325,6 +328,14 @@ pub fn active_high_threshold_committee( }) } +/// Return the current high transcript for the given height if it was found. +pub fn subnet_splitting_status_at_height( + reader: &dyn ConsensusPoolCache, + height: Height, +) -> Option { + get_active_data_at(reader, height, get_subnet_splitting_status_at_given_summary) +} + /// Return the active DKGData active at the given height if it was found. fn get_active_data_at( reader: &dyn ConsensusPoolCache, @@ -401,6 +412,19 @@ fn get_transcript_data_at_given_summary( } } +fn get_subnet_splitting_status_at_given_summary( + summary_block: &Block, + height: Height, +) -> Option { + let dkg_summary = &summary_block.payload.as_ref().as_summary().dkg; + + if dkg_summary.current_interval_includes(height) { + Some(dkg_summary.subnet_splitting_status()) + } else { + None + } +} + /// Check if the [`ReplicaVersion`] is the current version /// /// # Arguments