From c951daa00dab1149742809cbd7b9d5e89a638834 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 30 Jul 2026 13:30:24 +0000 Subject: [PATCH 1/8] feat: do not validate certifications of other subnet --- Cargo.lock | 3 + rs/consensus/certification/BUILD.bazel | 3 + rs/consensus/certification/Cargo.toml | 3 + rs/consensus/certification/src/certifier.rs | 394 +++++++++++++++++++- rs/consensus/utils/src/lib.rs | 26 +- 5 files changed, 426 insertions(+), 3 deletions(-) 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 From 5df6dc72bddb003435755d4105ef51337570a390 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 6 Aug 2026 09:55:45 +0000 Subject: [PATCH 2/8] docs: typos --- rs/consensus/certification/src/certifier.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index a14ac83919cd..65598ba8651d 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -1810,11 +1810,10 @@ mod tests { } /// 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. + /// 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() { + fn test_validate_share_handles_invalid_during_subnet_splitting() { ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { with_test_replica_logger(|log| { let Dependencies { @@ -1884,9 +1883,10 @@ mod tests { } /// 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 + /// should be ignored and not validated, as they could be from the other subnet. + /// In a `Done` interval with same subnet ID, certifications should be validated as normal #[test] - fn test_validate_certification_validates_despite_scheduled_subnet_splitting() { + fn test_validate_certification_handles_invalid_during_subnet_splitting() { ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { with_test_replica_logger(|log| { let Dependencies { From 3416609e9ad1c979388f920af89022bdd01e4f47 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 6 Aug 2026 14:50:47 +0000 Subject: [PATCH 3/8] docs: typo --- rs/consensus/utils/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/consensus/utils/src/lib.rs b/rs/consensus/utils/src/lib.rs index f425551a0f55..928a881aac1c 100644 --- a/rs/consensus/utils/src/lib.rs +++ b/rs/consensus/utils/src/lib.rs @@ -328,7 +328,7 @@ pub fn active_high_threshold_committee( }) } -/// Return the current high transcript for the given height if it was found. +/// Return the subnet splitting status for the given height if it was found. pub fn subnet_splitting_status_at_height( reader: &dyn ConsensusPoolCache, height: Height, From 94f807908a358f8d06415305d5fee086267c2c64 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 6 Aug 2026 14:51:12 +0000 Subject: [PATCH 4/8] chore: non-ASCII space --- rs/consensus/certification/src/certifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 65598ba8651d..6babcd54d148 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -502,7 +502,7 @@ impl CertifierImpl { info!( every_n_seconds => 30, self.log, - "Skipping the validation of a certification at height {} because a \ + "Skipping the validation of a certification at height {} because a \ subnet splitting is taking place", certification.height ); From 0a14357d4dac5d790f92d4d42d06fdfd0616a780 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Mon, 10 Aug 2026 14:47:14 +0000 Subject: [PATCH 5/8] refactor: deduplicate by moving logging --- rs/consensus/certification/src/certifier.rs | 103 +++++++++----------- 1 file changed, 47 insertions(+), 56 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 6babcd54d148..5c24ba9898da 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -344,15 +344,7 @@ impl CertifierImpl { }) // 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) + !self.should_skip_due_to_subnet_splitting(state_hash_metadata.height) }) .cloned() .filter_map(|state_hash_metadata| { @@ -497,26 +489,8 @@ impl CertifierImpl { // 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; - } + if self.should_skip_due_to_subnet_splitting(certification.height) { + return None; } // check if the certification is indeed valid for the specified height. If @@ -561,26 +535,8 @@ impl CertifierImpl { // 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 self.should_skip_due_to_subnet_splitting(share.height) { + return None; } // If the share has an invalid content or does not belong to the @@ -651,18 +607,53 @@ 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 { + fn should_skip_due_to_subnet_splitting(&self, height: Height) -> bool { 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), + None => { + warn!( + every_n_seconds => 30, + self.log, + "Missing finalized summary block for height {height}. \ + Skipping creation/validation of certifications/shares" + ); + + true + } + Some(SubnetSplittingStatus::NotScheduled) => 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), + Some(SubnetSplittingStatus::Scheduled(..)) => { + info!( + every_n_seconds => 30, + self.log, + "Skipping creation/validation of certifications/shares at height {height} \ + because a subnet splitting is taking place at the current interval" + ); + + 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) + if new_subnet_id != self.replica_config.subnet_id { + info!( + every_n_seconds => 30, + self.log, + "Skipping creation/validation of certifications/shares at height {height} \ + because a subnet splitting has taken place and the replica is still running \ + with the old subnet_id" + ); + + true + } else { + info!( + every_n_seconds => 30, + self.log, + "A subnet splitting has just taken place and the replica is running with the \ + new subnet_id. Creating/validating certifications/shares at height {height}" + ); + + false + } } } } From c26eb4f2f652a1e5855dde6bfca252d4918d459c Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Mon, 10 Aug 2026 14:48:45 +0000 Subject: [PATCH 6/8] refactor: pass `should_skip` to test closure --- rs/consensus/certification/src/certifier.rs | 147 +++++++++----------- 1 file changed, 63 insertions(+), 84 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 5c24ba9898da..275a07265096 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -1712,19 +1712,19 @@ mod tests { fn assert_for_all_subnet_splitting_statuses( pool: &mut TestConsensusPool, - mut test: impl FnMut(SubnetSplittingStatus, Height), + mut test: impl FnMut(SubnetSplittingStatus, bool, Height), ) { - for status in [ - not_scheduled_splitting(), - scheduled_splitting(), - done_splitting_different_subnet(), - done_splitting_same_subnet(), + for (status, should_skip) in [ + (not_scheduled_splitting(), false), + (scheduled_splitting(), true), + (done_splitting_different_subnet(), true), + (done_splitting_same_subnet(), false), ] { 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); + test(status, should_skip, test_height); } } } @@ -1762,40 +1762,31 @@ mod tests { 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(), - }], - ); + assert_for_all_subnet_splitting_statuses( + &mut pool, + |status, should_skip, 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) => - { + if should_skip { assert!( shares.is_empty(), - "Expected no shares after Done splitting with different subnet ID, got: {shares:?}" + "Expected shares to be empty for status {status:?}, got: {shares:?}" ); - } - SubnetSplittingStatus::NotScheduled - | SubnetSplittingStatus::PostSplit(..) => { + } else { assert!( !shares.is_empty(), - "Expected shares when not splitting or splitting with same subnet ID" + "Expected shares to be non-empty for status {status:?}" ); } - } - }); + }, + ); }) }) } @@ -1833,42 +1824,36 @@ mod tests { 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)), - }, - }; + assert_for_all_subnet_splitting_statuses( + &mut pool, + |status, should_skip, 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) => - { + let result = certifier.validate_share(&cert_pool, &share); + if should_skip { assert_eq!( result, None, - "Expected None after Done splitting with different subnet ID" + "Expected no change action for status {status:?}, got: {result:?}" ); - } - SubnetSplittingStatus::NotScheduled - | SubnetSplittingStatus::PostSplit(..) => { + } else { assert_eq!( result, Some(ChangeAction::MoveToValidated( CertificationMessage::CertificationShare(share) )), - "Expected MoveToValidated when not splitting or splitting with same subnet ID" + "Expected MoveToValidated for status {status:?}, got: {result:?}" ); } - } - }); + }, + ); }) }) } @@ -1899,42 +1884,36 @@ mod tests { 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(), - }, - }; + assert_for_all_subnet_splitting_statuses( + &mut pool, + |status, should_skip, 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) => - { + let result = certifier.validate_certification(&cert); + if should_skip { assert_eq!( result, None, - "Expected None after Done splitting with different subnet ID" + "Expected no change action for status {status:?}, got: {result:?}" ); - } - SubnetSplittingStatus::NotScheduled - | SubnetSplittingStatus::PostSplit(..) => { + } else { assert_eq!( result, Some(ChangeAction::MoveToValidated( - CertificationMessage::Certification(cert.clone()) + CertificationMessage::Certification(cert) )), - "Expected MoveToValidated when not splitting or splitting with same subnet ID" + "Expected MoveToValidated for status {status:?}, got: {result:?}" ); } - } - }); + }, + ); }) }) } From 3fdeff68df247a297e46b9e5778e812393cc94ed Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Mon, 10 Aug 2026 14:58:36 +0000 Subject: [PATCH 7/8] test: unknown status --- rs/consensus/certification/src/certifier.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 275a07265096..69886d36567c 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -1712,7 +1712,7 @@ mod tests { fn assert_for_all_subnet_splitting_statuses( pool: &mut TestConsensusPool, - mut test: impl FnMut(SubnetSplittingStatus, bool, Height), + mut test: impl FnMut(Option, bool, Height), ) { for (status, should_skip) in [ (not_scheduled_splitting(), false), @@ -1720,13 +1720,24 @@ mod tests { (done_splitting_different_subnet(), true), (done_splitting_same_subnet(), false), ] { - let splitting_height = advance_to_splitting_interval(pool, status); - for test_height in splitting_height.get()..=splitting_height.get() + TEST_DKG_INTERVAL { + let splitting_height = advance_to_splitting_interval(pool, status).get(); + for test_height in splitting_height..=splitting_height + TEST_DKG_INTERVAL { let test_height = Height::from(test_height); - test(status, should_skip, test_height); + test(Some(status), should_skip, test_height); } } + + // One more test case: a height that does not have a corresponding finalized summary block. + // The certifier should skip for this height, since the subnet splitting status is unknown. + let height_too_ahead = advance_to_splitting_interval(pool, not_scheduled_splitting()).get() + + TEST_DKG_INTERVAL + + 1; + for test_height in height_too_ahead..=height_too_ahead + TEST_DKG_INTERVAL { + let test_height = Height::from(test_height); + + test(None, /*should_skip=*/ true, test_height); + } } /// Signing should be skipped for heights covered by a `Scheduled` or `Done` with different From a5144b8e3ec00fd251b49f02a355405f0f9d925c Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Tue, 11 Aug 2026 08:44:31 +0000 Subject: [PATCH 8/8] chore: SubnetSplittingStatus required --- rs/consensus/certification/src/certifier.rs | 3 +-- rs/consensus/src/consensus/status.rs | 12 +++++------ rs/types/types/src/consensus/dkg.rs | 22 +++++++++------------ 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 69886d36567c..3c47e72215d7 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -705,7 +705,6 @@ mod tests { 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, @@ -1675,7 +1674,7 @@ mod tests { 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)); + payload.dkg.subnet_splitting_status = status; block.payload = Payload::new( ic_types::crypto::crypto_hash, BlockPayload::Summary(payload), diff --git a/rs/consensus/src/consensus/status.rs b/rs/consensus/src/consensus/status.rs index 08f47ebb0cc3..08bf501a174c 100644 --- a/rs/consensus/src/consensus/status.rs +++ b/rs/consensus/src/consensus/status.rs @@ -170,7 +170,6 @@ mod tests { use ic_test_utilities_types::ids::node_test_id; use ic_types::{ ReplicaVersion, - backwards_compatibility::BackwardsCompatible, consensus::{BlockPayload, Payload, dkg::SplittingArgs}, crypto::crypto_hash, }; @@ -326,11 +325,12 @@ mod tests { ); let mut last_summary_block = PoolReader::new(&pool).get_highest_finalized_summary_block(); - let mut payload = last_summary_block.payload.as_ref().as_summary().clone(); - payload.dkg.subnet_splitting_status = - BackwardsCompatible::new_for_test_only(test_case.subnet_splitting_status); - last_summary_block.payload = - Payload::new(crypto_hash, BlockPayload::Summary(payload)); + if let Some(subnet_splitting_status) = test_case.subnet_splitting_status { + let mut payload = last_summary_block.payload.as_ref().as_summary().clone(); + payload.dkg.subnet_splitting_status = subnet_splitting_status; + last_summary_block.payload = + Payload::new(crypto_hash, BlockPayload::Summary(payload)); + } let status = get_status( test_case.current_height, diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index be5154df33ee..9d9b614f9709 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -4,7 +4,6 @@ use super::*; use crate::{ ReplicaVersion, artifact::PbArtifact, - backwards_compatibility::BackwardsCompatible, crypto::threshold_sig::ni_dkg::{ NiDkgDealing, NiDkgId, NiDkgTag, NiDkgTargetId, NiDkgTranscript, config::NiDkgConfig, @@ -279,7 +278,7 @@ pub struct DkgSummary { /// The number of intervals a DKG for the given remote target was attempted. pub remote_dkg_attempts: BTreeMap, /// Status of the subnet splitting. - pub subnet_splitting_status: BackwardsCompatible, + pub subnet_splitting_status: SubnetSplittingStatus, } impl DkgSummary { @@ -307,7 +306,7 @@ impl DkgSummary { next_interval_length, height, remote_dkg_attempts, - subnet_splitting_status: BackwardsCompatible::new(SubnetSplittingStatus::NotScheduled), + subnet_splitting_status: SubnetSplittingStatus::NotScheduled, } } @@ -393,9 +392,6 @@ impl DkgSummary { pub fn subnet_splitting_status(&self) -> SubnetSplittingStatus { self.subnet_splitting_status - .as_ref() - .copied() - .unwrap_or_default() } } @@ -467,10 +463,9 @@ impl From<&DkgSummary> for pb::Summary { summary.transcripts_for_remote_subnets.as_slice(), ), remote_dkg_attempts: build_remote_dkg_attempts_vec(&summary.remote_dkg_attempts), - subnet_splitting_status: summary - .subnet_splitting_status - .as_ref() - .map(pb::summary::SubnetSplittingStatus::from), + subnet_splitting_status: Some(pb::summary::SubnetSplittingStatus::from( + summary.subnet_splitting_status, + )), } } } @@ -551,8 +546,8 @@ fn build_transcript_result( } } -impl From<&SubnetSplittingStatus> for pb::summary::SubnetSplittingStatus { - fn from(status: &SubnetSplittingStatus) -> Self { +impl From for pb::summary::SubnetSplittingStatus { + fn from(status: SubnetSplittingStatus) -> Self { match status { SubnetSplittingStatus::NotScheduled => { pb::summary::SubnetSplittingStatus::NotScheduled(()) @@ -629,8 +624,9 @@ impl TryFrom for DkgSummary { ) .map_err(ProxyDecodeError::Other)?, remote_dkg_attempts: build_remote_dkg_attempts_map(&summary.remote_dkg_attempts), - subnet_splitting_status: BackwardsCompatible::try_from_proto( + subnet_splitting_status: try_from_option_field( summary.subnet_splitting_status, + "Summary::subnet_splitting_status", )?, }) }