diff --git a/rs/consensus/dkg/src/lib.rs b/rs/consensus/dkg/src/lib.rs index cac4769d1f84..bdf33dfc803a 100644 --- a/rs/consensus/dkg/src/lib.rs +++ b/rs/consensus/dkg/src/lib.rs @@ -1805,7 +1805,8 @@ mod tests { deps.crypto.as_ref(), &pool_reader, &*deps.dkg_pool.read().unwrap(), - parent, + parent.clone(), + &pool_reader.dkg_summary_block(&parent).unwrap(), block.payload.as_ref(), deps.state_manager.as_ref(), &block.context, @@ -1954,7 +1955,8 @@ mod tests { deps.crypto.as_ref(), &pool_reader, &*deps.dkg_pool.read().unwrap(), - parent, + parent.clone(), + &pool_reader.dkg_summary_block(&parent).unwrap(), &payload_without_remote, deps.state_manager.as_ref(), &block.context, @@ -2061,6 +2063,7 @@ mod tests { &pool_reader, &*deps.dkg_pool.read().unwrap(), parent.clone(), + &pool_reader.dkg_summary_block(&parent).unwrap(), &payload, deps.state_manager.as_ref(), &validation_context, diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs index 941470ddf7a9..cd4f4fa8cc95 100644 --- a/rs/consensus/dkg/src/payload_builder.rs +++ b/rs/consensus/dkg/src/payload_builder.rs @@ -56,6 +56,7 @@ pub fn create_payload( pool_reader: &PoolReader<'_>, dkg_pool: Arc>, parent: &Block, + last_summary_block: &Block, state_reader: &dyn StateReader, validation_context: &ValidationContext, logger: ReplicaLogger, @@ -63,10 +64,6 @@ pub fn create_payload( dkg_payload_metrics: Option<&DkgPayloadMetrics>, ) -> Result { let height = parent.height.increment(); - // Get the last summary from the chain. - let last_summary_block = pool_reader - .dkg_summary_block(parent) - .ok_or(DkgPayloadCreationError::MissingDkgStartBlock)?; let last_dkg_summary = &last_summary_block.payload.as_ref().as_summary().dkg; if last_dkg_summary.get_next_start_height() == height { @@ -96,7 +93,7 @@ pub fn create_payload( dkg_pool, parent, max_dealings_per_block, - &last_summary_block, + last_summary_block, last_dkg_summary, crypto, state_reader, diff --git a/rs/consensus/dkg/src/payload_validator.rs b/rs/consensus/dkg/src/payload_validator.rs index 10e416d12909..aaba630b5695 100644 --- a/rs/consensus/dkg/src/payload_validator.rs +++ b/rs/consensus/dkg/src/payload_validator.rs @@ -34,6 +34,7 @@ pub fn validate_payload( pool_reader: &PoolReader<'_>, dkg_pool: &dyn DkgPool, parent: Block, + last_summary_block: &Block, payload: &BlockPayload, state_reader: &dyn StateReader, validation_context: &ValidationContext, @@ -45,13 +46,7 @@ pub fn validate_payload( .registry_version(current_height) .ok_or(DkgPayloadValidationFailure::FailedToGetRegistryVersion)?; - let last_summary_block = pool_reader - .dkg_summary_block(&parent) - // We expect the parent to be valid, so there will be _always_ a DKG start block on the - // chain. - .expect("No DKG start block found for the parent block."); let last_dkg_summary = &last_summary_block.payload.as_ref().as_summary().dkg; - let is_dkg_start_height = last_dkg_summary.get_next_start_height() == current_height; match payload { @@ -335,6 +330,9 @@ mod tests { let block = Block::from(pool.make_next_block()); let block_payload = block.payload.as_ref(); + let last_summary_block = PoolReader::new(&pool) + .dkg_summary_block(&parent_block) + .unwrap(); assert!( validate_payload( subnet_test_id(0), @@ -343,6 +341,7 @@ mod tests { &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), parent_block, + &last_summary_block, block_payload, state_manager.as_ref(), &context, @@ -359,6 +358,9 @@ mod tests { let block = Block::from(pool.make_next_block()); let summary = block.payload.as_ref(); + let last_summary_block = PoolReader::new(&pool) + .dkg_summary_block(&parent_block) + .unwrap(); assert!( validate_payload( subnet_test_id(0), @@ -367,6 +369,7 @@ mod tests { &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), parent_block, + &last_summary_block, summary, state_manager.as_ref(), &context, @@ -579,6 +582,7 @@ mod tests { idkg: idkg::Payload::default(), }); + let last_summary_block = PoolReader::new(&pool).dkg_summary_block(&parent).unwrap(); assert_eq!( validate_payload( SUBNET_1, @@ -587,6 +591,7 @@ mod tests { &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), parent, + &last_summary_block, &block_payload, state_manager.as_ref(), &context, @@ -653,13 +658,15 @@ mod tests { idkg: idkg::Payload::default(), }); + let last_summary_block = PoolReader::new(&pool).dkg_summary_block(&parent).unwrap(); validate_payload( subnet_id, registry.as_ref(), crypto.as_ref(), &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), - parent.clone(), + parent, + &last_summary_block, &block_payload, state_manager.as_ref(), &context, @@ -838,6 +845,7 @@ mod tests { idkg: idkg::Payload::default(), }); + let last_summary_block = PoolReader::new(&pool).dkg_summary_block(&parent).unwrap(); let result = validate_payload( subnet_id, registry.as_ref(), @@ -845,6 +853,7 @@ mod tests { &PoolReader::new(&pool), &dkg_pool, parent.clone(), + &last_summary_block, &block_payload, state_manager.as_ref(), &context, @@ -865,6 +874,7 @@ mod tests { &PoolReader::new(&pool), &dkg_pool, parent, + &last_summary_block, &block_payload, state_manager.as_ref(), &context, diff --git a/rs/consensus/idkg/src/payload_builder.rs b/rs/consensus/idkg/src/payload_builder.rs index 9cf161296ad4..1194f2a67190 100644 --- a/rs/consensus/idkg/src/payload_builder.rs +++ b/rs/consensus/idkg/src/payload_builder.rs @@ -167,6 +167,7 @@ pub fn create_summary_payload( pool_reader: &PoolReader<'_>, context: &ValidationContext, parent_block: &Block, + prev_summary_block: &Block, idkg_payload_metrics: Option<&IDkgPayloadMetrics>, log: &ReplicaLogger, ) -> Result { @@ -178,9 +179,6 @@ pub fn create_summary_payload( }); let height = parent_block.height().increment(); - let prev_summary_block = pool_reader - .dkg_summary_block(parent_block) - .ok_or_else(|| IDkgPayloadError::ConsensusSummaryBlockNotFound(parent_block.height()))?; // For this interval: context.registry_version from prev summary block // which is the same as calling pool_reader.registry_version(height). @@ -483,6 +481,7 @@ pub fn create_data_payload( state_reader: &dyn StateReader, context: &ValidationContext, parent_block: &Block, + summary_block: &Block, idkg_payload_metrics: &IDkgPayloadMetrics, log: &ReplicaLogger, ) -> Result { @@ -495,9 +494,6 @@ pub fn create_data_payload( if parent_block.payload.as_ref().as_idkg().is_none() { return Ok(None); }; - let summary_block = pool_reader - .dkg_summary_block(parent_block) - .ok_or_else(|| IDkgPayloadError::ConsensusSummaryBlockNotFound(parent_block.height()))?; // In case the certified height is below the summary height, add the heights in // between to the blockchain. This is needed to calculate the total number of pre- @@ -529,7 +525,7 @@ pub fn create_data_payload( subnet_id, context, parent_block, - &summary_block, + summary_block, &block_reader, &transcript_builder, state_reader, diff --git a/rs/consensus/idkg/src/payload_builder/errors.rs b/rs/consensus/idkg/src/payload_builder/errors.rs index 0b3d3a5ea682..ead147a0cf80 100644 --- a/rs/consensus/idkg/src/payload_builder/errors.rs +++ b/rs/consensus/idkg/src/payload_builder/errors.rs @@ -1,6 +1,6 @@ use ic_crypto::MegaKeyFromRegistryError; use ic_types::{ - Height, RegistryVersion, SubnetId, + RegistryVersion, SubnetId, consensus::idkg, crypto::canister_threshold_sig::{ error::{ @@ -19,7 +19,6 @@ use super::InvalidChainCacheError; pub enum IDkgPayloadError { RegistryClientError(RegistryClientError), MegaKeyFromRegistryError(MegaKeyFromRegistryError), - ConsensusSummaryBlockNotFound(Height), StateManagerError(StateManagerError), SubnetWithNoNodes(SubnetId, RegistryVersion), PreSignatureError(EcdsaPresignatureQuadrupleCreationError), diff --git a/rs/consensus/idkg/src/payload_verifier.rs b/rs/consensus/idkg/src/payload_verifier.rs index d7ba03dc8e49..d10b400ea128 100644 --- a/rs/consensus/idkg/src/payload_verifier.rs +++ b/rs/consensus/idkg/src/payload_verifier.rs @@ -181,6 +181,7 @@ pub fn validate_payload( state_reader: &dyn StateReader, context: &ValidationContext, parent_block: &Block, + last_summary_block: &Block, payload: &BlockPayload, metrics: HistogramVec, ) -> ValidationResult { @@ -194,6 +195,7 @@ pub fn validate_payload( pool_reader, context, parent_block, + last_summary_block, payload.as_summary().idkg.as_ref(), ) }, @@ -212,6 +214,7 @@ pub fn validate_payload( state_reader, context, parent_block, + last_summary_block, payload.as_data().idkg.as_ref(), &metrics, ) @@ -230,6 +233,7 @@ fn validate_summary_payload( pool_reader: &PoolReader<'_>, context: &ValidationContext, parent_block: &Block, + last_summary_block: &Block, summary_payload: Option<&idkg::IDkgPayload>, ) -> ValidationResult { let height = parent_block.height().increment(); @@ -252,6 +256,7 @@ fn validate_summary_payload( pool_reader, context, parent_block, + last_summary_block, None, &ic_logger::replica_logger::no_op_logger(), ) { @@ -283,6 +288,7 @@ fn validate_data_payload( state_reader: &dyn StateReader, context: &ValidationContext, parent_block: &Block, + summary_block: &Block, data_payload: Option<&idkg::IDkgPayload>, metrics: &HistogramVec, ) -> ValidationResult { @@ -329,14 +335,6 @@ fn validate_data_payload( } }; - let summary_block = pool_reader - .dkg_summary_block(parent_block) - .unwrap_or_else(|| { - panic!( - "Impossible: fail to the summary block that governs height {}", - parent_block.height() - ) - }); // In case the certified height is below the summary height, add the heights in // between to the blockchain. This is needed to calculate the total number of pre- // signatures in the certified state and every block since then. @@ -383,7 +381,7 @@ fn validate_data_payload( subnet_id, context, parent_block, - &summary_block, + summary_block, &block_reader, &builder, state_reader, diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 187e68baf849..6773ccc1bba6 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -135,6 +135,20 @@ pub(crate) fn deliver_batches_with_result_processor( } ); + // Retrieve the dkg summary block + let Some(summary_block) = pool.dkg_summary_block_for_finalized_height(height) else { + warn!( + every_n_seconds => 30, + log, + "Do not deliver height {} because no summary block was found. \ + Finalized height: {}", + height, + finalized_height + ); + break; + }; + let dkg_summary = &summary_block.payload.as_ref().as_summary().dkg; + if block.payload.is_summary() { info!( log, @@ -143,7 +157,14 @@ pub(crate) fn deliver_batches_with_result_processor( } // When we are not delivering CUP block, we must check if the subnet is halted. else { - match status::get_status(height, registry_client, subnet_id, pool, log) { + match status::get_status( + height, + &summary_block, + registry_client, + subnet_id, + pool, + log, + ) { Some(Status::Halting | Status::Halted) => { debug!( every_n_seconds => 5, @@ -166,20 +187,6 @@ pub(crate) fn deliver_batches_with_result_processor( let randomness = randomness_from_crypto_hashable(&tape); - // Retrieve the dkg summary block - let Some(summary_block) = pool.dkg_summary_block_for_finalized_height(height) else { - warn!( - every_n_seconds => 30, - log, - "Do not deliver height {} because no summary block was found. \ - Finalized height: {}", - height, - finalized_height - ); - break; - }; - let dkg_summary = &summary_block.payload.as_ref().as_summary().dkg; - let mut chain_key_subnet_public_keys = BTreeMap::new(); let (mut idkg_subnet_public_keys, idkg_pre_signatures) = get_idkg_subnet_public_keys_and_pre_signatures( diff --git a/rs/consensus/src/consensus/block_maker.rs b/rs/consensus/src/consensus/block_maker.rs index ad75c90b5c54..9fd93ec4a6b7 100644 --- a/rs/consensus/src/consensus/block_maker.rs +++ b/rs/consensus/src/consensus/block_maker.rs @@ -19,7 +19,7 @@ use ic_interfaces::{ }; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::StateReader; -use ic_logger::{ReplicaLogger, debug, error, trace, warn}; +use ic_logger::{ReplicaLogger, debug, error, info, trace, warn}; use ic_metrics::MetricsRegistry; use ic_replicated_state::ReplicatedState; use ic_types::{ @@ -29,7 +29,7 @@ use ic_types::{ Block, BlockMetadata, BlockPayload, BlockProposal, DataPayload, HasHeight, HasRank, HashedBlock, Payload, RandomBeacon, Rank, SummaryPayload, block_maker::SubnetRecords, - dkg::{DkgDataPayload, DkgPayload}, + dkg::{DkgDataPayload, DkgPayload, SubnetSplittingStatus}, hashed, }, replica_config::ReplicaConfig, @@ -190,6 +190,16 @@ impl BlockMaker { let height = parent.height().increment(); let certified_height = self.state_reader.latest_certified_height(); + let Some(last_summary_block) = pool.dkg_summary_block(parent.as_ref()) else { + warn!( + every_n_seconds => 30, + self.log, + "Couldn't find the DKG summary block for parent height {}", + parent.height() + ); + return None; + }; + // Note that we will skip blockmaking if registry versions or replica_versions // are missing or temporarily not retrievable. // @@ -276,6 +286,7 @@ impl BlockMaker { pool, context, parent, + &last_summary_block, height, rank, registry_version, @@ -291,6 +302,7 @@ impl BlockMaker { pool: &PoolReader<'_>, context: ValidationContext, parent: HashedBlock, + last_summary_block: &Block, height: Height, rank: Rank, registry_version: RegistryVersion, @@ -306,6 +318,7 @@ impl BlockMaker { pool, Arc::clone(&self.dkg_pool), parent.as_ref(), + last_summary_block, &*self.state_reader, &context, self.log.clone(), @@ -331,6 +344,7 @@ impl BlockMaker { pool, &context, parent.as_ref(), + last_summary_block, Some(&self.idkg_payload_metrics), &self.log, ) @@ -341,6 +355,16 @@ impl BlockMaker { }) .ok()?; + if matches!( + summary.subnet_splitting_status(), + SubnetSplittingStatus::Scheduled(..) + ) { + info!( + self.log, + "Proposing a Splitting block at height {}.", height + ); + } + BlockPayload::Summary(SummaryPayload { dkg: summary, idkg: idkg_summary, @@ -349,6 +373,7 @@ impl BlockMaker { DkgPayload::Data(dkg) => { let (batch_payload, dkg, idkg_data) = match status::get_status( height, + last_summary_block, self.registry_client.as_ref(), self.replica_config.subnet_id, pool, @@ -381,6 +406,7 @@ impl BlockMaker { &*self.state_reader, &context, parent.as_ref(), + last_summary_block, &self.idkg_payload_metrics, &self.log, ) diff --git a/rs/consensus/src/consensus/catchup_package_maker.rs b/rs/consensus/src/consensus/catchup_package_maker.rs index b153128642e7..a98e40af549b 100644 --- a/rs/consensus/src/consensus/catchup_package_maker.rs +++ b/rs/consensus/src/consensus/catchup_package_maker.rs @@ -176,6 +176,7 @@ impl CatchUpPackageMaker { let halting = || { status::should_halt( height, + Some(&start_block), self.membership.registry_client.as_ref(), self.membership.subnet_id, pool, diff --git a/rs/consensus/src/consensus/malicious_consensus.rs b/rs/consensus/src/consensus/malicious_consensus.rs index b8be8c3fafce..6326fb4a52d0 100644 --- a/rs/consensus/src/consensus/malicious_consensus.rs +++ b/rs/consensus/src/consensus/malicious_consensus.rs @@ -143,6 +143,7 @@ impl ConsensusImpl { // Note that we will skip blockmaking if registry versions or replica_versions // are missing or temporarily not retrievable. let registry_version = pool.registry_version(height)?; + let last_summary_block = pool.dkg_summary_block(parent.as_ref())?; // Get the subnet records that are relevant to making a block let stable_registry_version = self @@ -158,6 +159,7 @@ impl ConsensusImpl { pool, context, parent, + &last_summary_block, height, rank, registry_version, diff --git a/rs/consensus/src/consensus/notary.rs b/rs/consensus/src/consensus/notary.rs index e9660391d4c3..56101736c910 100644 --- a/rs/consensus/src/consensus/notary.rs +++ b/rs/consensus/src/consensus/notary.rs @@ -349,6 +349,7 @@ fn get_adjusted_notary_delay_from_settings( let halting = || { status::should_halt( notarized_height, + None, membership.registry_client.as_ref(), membership.subnet_id, pool, diff --git a/rs/consensus/src/consensus/status.rs b/rs/consensus/src/consensus/status.rs index 21859c3fccfd..08f47ebb0cc3 100644 --- a/rs/consensus/src/consensus/status.rs +++ b/rs/consensus/src/consensus/status.rs @@ -4,7 +4,13 @@ use ic_consensus_utils::{lookup_replica_version, pool_reader::PoolReader}; use ic_interfaces_registry::RegistryClient; use ic_logger::{ReplicaLogger, warn}; use ic_registry_client_helpers::subnet::SubnetRegistry; -use ic_types::{Height, ReplicaVersion, SubnetId}; +use ic_types::{ + Height, ReplicaVersion, SubnetId, + consensus::{ + Block, + dkg::{PostSplitArgs, SubnetSplittingStatus}, + }, +}; #[derive(Copy, Clone, Eq, PartialEq, Debug)] pub(crate) enum Status { @@ -22,25 +28,41 @@ pub(crate) enum Status { /// Note: If 'height' is smaller than the height of the last CUP, this will return [None]. /// /// Returns -/// * [Status::Halting] when there is a pending upgrade or the registry instructs the subnet to halt -/// * [Status::Halted] when a CUP height has been finalized and either an upgrade is in progress or -/// the registry instructs the subnet to halt; -/// * [Status::Running] when there is no upgrade and the registry doesn't instruct the subnet to -/// halt. +/// * [Status::Halting] when there is a pending upgrade, subnet split or the registry instructs the +/// subnet to halt +/// * [Status::Halted] when the validation context's certified height reached the CUP height and +/// either an upgrade/subnet split is in progress or the registry instructs the subnet to halt; +/// * [Status::Running] when there is no upgrade, no split, and the registry doesn't instruct the +/// subnet to halt. pub(crate) fn get_status( height: Height, + last_summary_block: &Block, registry_client: &(impl RegistryClient + ?Sized), subnet_id: SubnetId, pool: &PoolReader<'_>, logger: &ReplicaLogger, ) -> Option { - if should_halt(height, registry_client, subnet_id, pool, logger) - .warn_if_none(logger, "Failed to check if the subnet is halting!")? + if should_halt( + height, + Some(last_summary_block), + registry_client, + subnet_id, + pool, + logger, + ) + .warn_if_none(logger, "Failed to check if the subnet is halting!")? { let certified_height = pool.get_finalized_tip().context.certified_height; - if should_halt(certified_height, registry_client, subnet_id, pool, logger) - .warn_if_none(logger, "Failed to check if the subnet is halted!") + if should_halt( + certified_height, + Some(last_summary_block), + registry_client, + subnet_id, + pool, + logger, + ) + .warn_if_none(logger, "Failed to check if the subnet is halted!") == Some(true) { return Some(Status::Halted); @@ -54,6 +76,7 @@ pub(crate) fn get_status( pub(crate) fn should_halt( height: Height, + last_summary_block: Option<&Block>, registry_client: &(impl RegistryClient + ?Sized), subnet_id: SubnetId, pool: &PoolReader<'_>, @@ -64,9 +87,27 @@ pub(crate) fn should_halt( format!("Failed to get the registry version at height {height}"), )?; - let upgrading = lookup_replica_version(registry_client, subnet_id, logger, registry_version) - .map(|replica_version| replica_version != ReplicaVersion::default()) - .warn_if_none(logger, "Failed to check if the upgrade is pending!"); + let should_halt_due_to_upgrading = + lookup_replica_version(registry_client, subnet_id, logger, registry_version) + .map(|replica_version| replica_version != ReplicaVersion::default()) + .warn_if_none(logger, "Failed to check if the upgrade is pending!"); + + let should_halt_due_to_subnet_splitting = last_summary_block.map(|summary_block| { + match summary_block + .payload + .as_ref() + .as_summary() + .dkg + .subnet_splitting_status() + { + SubnetSplittingStatus::NotScheduled => false, + SubnetSplittingStatus::Scheduled(..) => height >= summary_block.height, + // After the split, don't produce any blocks until we are on the right subnet. + SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id }) => { + subnet_id != new_subnet_id + } + } + }); let should_halt_by_subnet_record = registry_client .get_halt_at_cup_height(subnet_id, registry_version) @@ -75,14 +116,28 @@ pub(crate) fn should_halt( .warn_if_none( logger, format!( - "Failed to check if the registry version at height {height} instructs the subnet to halt!", + "Failed to check if the registry version at height {height} \ + instructs the subnet to halt!", ), ); - match (upgrading, should_halt_by_subnet_record) { - (Some(true), _) | (_, Some(true)) => Some(true), - (Some(false), Some(false)) => Some(false), - (_, _) => None, + any(&[ + should_halt_due_to_upgrading, + should_halt_due_to_subnet_splitting, + should_halt_by_subnet_record, + ]) +} + +/// Returns `Some(true)` if any of the provided values is known to be `true`. +/// Returns `Some(false)` if all of the provided values are known to be `false`. +/// Returns `None` if at least one of the provided values is otherwise unknown. +fn any(values: &[Option]) -> Option { + if values.contains(&Some(true)) { + Some(true) + } else if values.iter().all(|value| *value == Some(false)) { + Some(false) + } else { + None } } @@ -112,20 +167,29 @@ mod tests { use ic_test_artifact_pool::consensus_pool::{Round, TestConsensusPool}; 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::ReplicaVersion; + use ic_test_utilities_types::ids::node_test_id; + use ic_types::{ + ReplicaVersion, + backwards_compatibility::BackwardsCompatible, + consensus::{BlockPayload, Payload, dkg::SplittingArgs}, + crypto::crypto_hash, + }; + use ic_types_test_utils::ids::{SUBNET_0, SUBNET_1}; + use rstest::rstest; use super::*; + const DKG_LENGTH: u64 = 3; + const CUP_HEIGHT: Height = Height::new(2 * (1 + DKG_LENGTH)); + fn set_up( pool_config: ArtifactPoolConfig, + subnet_id: SubnetId, certified_height: Height, replica_version: ReplicaVersion, halt_at_cup_height: bool, - ) -> (TestConsensusPool, Arc, SubnetId) { - let dkg_interval_length = 3; + ) -> (TestConsensusPool, Arc) { let node_ids = [node_test_id(0)]; - let subnet_id = subnet_test_id(0); let Dependencies { mut pool, registry, .. } = dependencies_with_subnet_params( @@ -135,13 +199,13 @@ mod tests { ( 1, SubnetRecordBuilder::from(&node_ids) - .with_dkg_interval_length(dkg_interval_length) + .with_dkg_interval_length(DKG_LENGTH) .build(), ), ( 10, SubnetRecordBuilder::from(&node_ids) - .with_dkg_interval_length(dkg_interval_length) + .with_dkg_interval_length(DKG_LENGTH) .with_replica_version(replica_version.as_ref()) .with_halt_at_cup_height(halt_at_cup_height) .build(), @@ -149,125 +213,136 @@ mod tests { ], ); - pool.advance_round_normal_operation_n(10); + pool.advance_round_normal_operation_n(CUP_HEIGHT.get()); Round::new(&mut pool) .with_certified_height(certified_height) .advance(); - (pool, registry, subnet_id) + (pool, registry) } - fn run_test_case( + #[derive(Debug)] + struct TestCase { certified_height: Height, current_height: Height, replica_version: ReplicaVersion, halt_at_cup_height: bool, + subnet_splitting_status: Option, + subnet_id: SubnetId, expected_status: Option, - ) { + } + + #[rstest] + #[case::upgrade_finalized(TestCase{ + certified_height: CUP_HEIGHT, + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::try_from("new_replica_version").unwrap(), + halt_at_cup_height: false, + subnet_splitting_status: None, + subnet_id: SUBNET_0, + expected_status: Some(Status::Halted), + })] + #[case::upgrade_pending(TestCase{ + certified_height: CUP_HEIGHT.decrement(), + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::try_from("new_replica_version").unwrap(), + halt_at_cup_height: false, + subnet_splitting_status: None, + subnet_id: SUBNET_0, + expected_status: Some(Status::Halting), + })] + #[case::subnet_splitting_finalized(TestCase{ + certified_height: CUP_HEIGHT, + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: Some(SubnetSplittingStatus::Scheduled(SplittingArgs { destination_subnet_id: SUBNET_1, source_subnet_id: SUBNET_0 })), + subnet_id: SUBNET_0, + expected_status: Some(Status::Halted), + })] + #[case::subnet_splitting_pending(TestCase{ + certified_height: CUP_HEIGHT.decrement(), + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: Some(SubnetSplittingStatus::Scheduled(SplittingArgs { destination_subnet_id: SUBNET_1, source_subnet_id: SUBNET_0 })), + subnet_id: SUBNET_0, + expected_status: Some(Status::Halting), + })] + #[case::post_subnet_splitting_old_subnet_id(TestCase{ + certified_height: CUP_HEIGHT, + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: Some(SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id: SUBNET_1 })), + subnet_id: SUBNET_0, + expected_status: Some(Status::Halted), + })] + #[case::post_subnet_splitting_new_subnet_id(TestCase{ + certified_height: CUP_HEIGHT, + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: Some(SubnetSplittingStatus::PostSplit(PostSplitArgs { new_subnet_id: SUBNET_1 })), + subnet_id: SUBNET_1, + expected_status: Some(Status::Running), + })] + #[case::halt_at_cup_height(TestCase{ + certified_height: CUP_HEIGHT, + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: true, + subnet_splitting_status: None, + subnet_id: SUBNET_0, + expected_status: Some(Status::Halted), + })] + #[case::halting_at_cup_height(TestCase{ + certified_height: CUP_HEIGHT.decrement(), + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: true, + subnet_splitting_status: None, + subnet_id: SUBNET_0, + expected_status: Some(Status::Halting), + })] + #[case::running(TestCase{ + certified_height: CUP_HEIGHT, + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: None, + subnet_id: SUBNET_0, + expected_status: Some(Status::Running), + })] + fn status_test(#[case] test_case: TestCase) { with_test_replica_logger(|logger| { ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { - let (pool, registry_client, subnet_id) = set_up( + let (pool, registry_client) = set_up( pool_config, - certified_height, - replica_version, - halt_at_cup_height, + test_case.subnet_id, + test_case.certified_height, + test_case.replica_version, + test_case.halt_at_cup_height, ); + 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)); let status = get_status( - current_height, + test_case.current_height, + &last_summary_block, registry_client.as_ref(), - subnet_id, + test_case.subnet_id, &PoolReader::new(&pool), &logger, ); - assert_eq!(status, expected_status); + assert_eq!(status, test_case.expected_status); }) }) } - - /// The replica version changes at height = 8 - /// CUP height = 8 - /// Certified height = 8 - /// Current height = 10 - /// - /// Therefore the status should be [Status::Halted] - #[test] - fn upgrade_finalized_test() { - run_test_case( - Height::from(8), - Height::from(10), - ReplicaVersion::try_from("new_replica_version").unwrap(), - /*halt_at_cup_height=*/ false, - Some(Status::Halted), - ); - } - - /// The replica version changes at height = 8 - /// CUP height = 8 - /// Certified height = 7 - /// Current height = 10 - /// - /// Therefore the status should be [Status::Halting] - #[test] - fn upgrade_pending_test() { - run_test_case( - Height::from(7), - Height::from(10), - ReplicaVersion::try_from("new_replica_version").unwrap(), - /*halt_at_cup_height=*/ false, - Some(Status::Halting), - ); - } - - /// The registry version at height >= 8 has halt_at_cup_height = true. - /// CUP height = 8 - /// Certified height = 8 - /// Current height = 10 - /// - /// Therefore the status should be [Status::Halted] - #[test] - fn halt_finalized_test() { - run_test_case( - Height::from(8), - Height::from(10), - ReplicaVersion::default(), - /*halt_at_cup_height=*/ true, - Some(Status::Halted), - ); - } - - /// The registry version at height >= 8 has halt_at_cup_height = true. - /// CUP height = 8 - /// Certified height = 7 - /// Current height = 10 - /// - /// Therefore the status should be [Status::Halting] - #[test] - fn halting_test() { - run_test_case( - Height::from(7), - Height::from(10), - ReplicaVersion::default(), - /*halt_at_cup_height=*/ true, - Some(Status::Halting), - ); - } - - /// The replica version never changes and the registry doesn't instruct the subnet to halt. - /// CUP height = 8 - /// Certified height = 7 - /// Current height = 10 - /// - /// Therefore the status should be [Status::Running] - #[test] - fn running_test() { - run_test_case( - Height::from(7), - Height::from(10), - ReplicaVersion::default(), - /*halt_at_cup_height=*/ false, - Some(Status::Running), - ); - } } diff --git a/rs/consensus/src/consensus/validator.rs b/rs/consensus/src/consensus/validator.rs index 539dd1130898..f372793b7ab0 100644 --- a/rs/consensus/src/consensus/validator.rs +++ b/rs/consensus/src/consensus/validator.rs @@ -86,6 +86,7 @@ enum ValidationFailure { BlockNotFound(CryptoHashOf, Height), FinalizedBlockNotFound(Height), FailedToGetRegistryVersion, + FailedToGetConsensusStatus, ValidationContextNotReached(ValidationContext, ValidationContext), CatchUpHeightNegligible, MissingPastPayloads, @@ -108,7 +109,7 @@ enum InvalidArtifactReason { InvalidIDkgPayload(InvalidIDkgPayloadReason), InsufficientSignatures, CannotVerifyBlockHeightZero, - NonEmptyPayloadPastUpgradePoint, + NonEmptyPayloadWhileHalting, NonStrictlyIncreasingValidationContext, MismatchedBlockInCatchUpPackageShare, DataPayloadBlockInCatchUpPackageShare, @@ -1200,18 +1201,22 @@ impl Validator { return Err(InvalidArtifactReason::CannotVerifyBlockHeightZero.into()); } + let parent = get_notarized_parent(pool_reader, proposal)?; + let last_summary_block = pool_reader + .dkg_summary_block(&parent) + .ok_or(ValidationFailure::DkgSummaryNotFound(parent.height))?; let Some(status) = status::get_status( proposal.height(), + &last_summary_block, self.registry_client.as_ref(), self.replica_config.subnet_id, pool_reader, &self.log, ) else { - return Err(ValidationFailure::FailedToGetRegistryVersion.into()); + return Err(ValidationFailure::FailedToGetConsensusStatus.into()); }; let proposer = proposal.signature.signer; - let parent = get_notarized_parent(pool_reader, proposal)?; // Ensure registry_version, certified_height increase monotonically and that // time increases *strictly* monotonically. @@ -1275,7 +1280,7 @@ impl Validator { return if payload.is_empty() { Ok(()) } else { - Err(InvalidArtifactReason::NonEmptyPayloadPastUpgradePoint.into()) + Err(InvalidArtifactReason::NonEmptyPayloadWhileHalting.into()) }; } } @@ -1328,6 +1333,7 @@ impl Validator { self.state_manager.as_ref(), &proposal.context, &parent, + &last_summary_block, proposal.payload.as_ref(), self.metrics.idkg_validation_duration.clone(), ) @@ -1351,6 +1357,7 @@ impl Validator { pool_reader, dkg_pool, parent, + &last_summary_block, proposal.payload.as_ref(), self.state_manager.as_ref(), &proposal.context, @@ -2857,6 +2864,7 @@ pub mod test { assert_matches!( status::get_status( summary_proposal.height(), + &PoolReader::new(&pool).get_highest_finalized_summary_block(), registry_client.as_ref(), replica_config.subnet_id, &PoolReader::new(&pool), diff --git a/rs/test_utilities/artifact_pool/src/consensus_pool.rs b/rs/test_utilities/artifact_pool/src/consensus_pool.rs index 0e62e1e84349..49058f983a3d 100644 --- a/rs/test_utilities/artifact_pool/src/consensus_pool.rs +++ b/rs/test_utilities/artifact_pool/src/consensus_pool.rs @@ -152,13 +152,16 @@ fn dkg_payload_builder_fn( dkg_pool: Arc>, ) -> Box DkgPayload> { Box::new(move |cons_pool, parent, validation_context| { + let pool = PoolReader::new(cons_pool); + let last_summary_block = pool.dkg_summary_block(&parent).expect("No DKG summary"); ic_consensus_dkg::create_payload( subnet_id, &*registry_client, &*crypto, - &PoolReader::new(cons_pool), + &pool, dkg_pool.clone(), &parent, + &last_summary_block, &*state_manager, validation_context, no_op_logger(), diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index 49f6b3b546e2..c49d6206b324 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -787,7 +787,6 @@ pub enum DkgPayloadCreationError { FailedToGetDkgIntervalSettingFromRegistry(RegistryClientError), FailedToGetSubnetMemberListFromRegistry(RegistryClientError), FailedToGetVetKdKeyList(RegistryClientError), - MissingDkgStartBlock, } /// Reasons for why a dkg payload might be invalid.