From 1ae1c7b2ca55b52f2e281c306c886c47e6938ef4 Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Mon, 30 Mar 2026 06:07:53 +0000 Subject: [PATCH 01/84] err --- Cargo.lock | 4 + rs/consensus/cup_utils/BUILD.bazel | 1 + rs/consensus/cup_utils/Cargo.toml | 1 + rs/consensus/cup_utils/src/lib.rs | 153 +++++++------- rs/consensus/utils/BUILD.bazel | 3 + rs/consensus/utils/Cargo.toml | 3 + rs/consensus/utils/src/lib.rs | 1 + rs/consensus/utils/src/subnet_splitting.rs | 191 ++++++++++++++++++ .../src/catch_up_package_provider.rs | 6 +- rs/orchestrator/src/error.rs | 30 ++- rs/orchestrator/src/registry_helper.rs | 5 +- rs/replay/src/lib.rs | 3 +- rs/replica/src/setup_ic_stack.rs | 3 +- rs/state_machine_tests/src/lib.rs | 2 +- rs/types/types/src/consensus/catchup.rs | 133 ++++++++---- 15 files changed, 406 insertions(+), 133 deletions(-) create mode 100644 rs/consensus/utils/src/subnet_splitting.rs diff --git a/Cargo.lock b/Cargo.lock index 5fdc8e22e3f5..f03dc726f6bb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7975,6 +7975,7 @@ dependencies = [ "phantom_newtype", "prost 0.13.5", "slog", + "thiserror 2.0.18", ] [[package]] @@ -8136,6 +8137,7 @@ dependencies = [ "ic-metrics", "ic-protobuf", "ic-registry-client-helpers", + "ic-registry-keys", "ic-replicated-state", "ic-test-utilities", "ic-test-utilities-consensus", @@ -8146,7 +8148,9 @@ dependencies = [ "ic-types", "prometheus", "rand 0.8.5", + "rstest", "slog", + "thiserror 2.0.18", ] [[package]] diff --git a/rs/consensus/cup_utils/BUILD.bazel b/rs/consensus/cup_utils/BUILD.bazel index 30503a9687b0..7e947e4a3286 100644 --- a/rs/consensus/cup_utils/BUILD.bazel +++ b/rs/consensus/cup_utils/BUILD.bazel @@ -17,6 +17,7 @@ rust_library( "//rs/registry/helpers", "//rs/types/types", "@crate_index//:slog", + "@crate_index//:thiserror", ], ) diff --git a/rs/consensus/cup_utils/Cargo.toml b/rs/consensus/cup_utils/Cargo.toml index bae8e34e72dc..c2037720ca79 100644 --- a/rs/consensus/cup_utils/Cargo.toml +++ b/rs/consensus/cup_utils/Cargo.toml @@ -16,6 +16,7 @@ ic-registry-client-helpers = { path = "../../registry/helpers" } ic-types = { path = "../../types/types" } phantom_newtype = { path = "../../phantom_newtype" } slog = { workspace = true } +thiserror = { workspace = true } [dev-dependencies] ic-crypto-test-utils-ni-dkg = { path = "../../crypto/test_utils/ni-dkg" } diff --git a/rs/consensus/cup_utils/src/lib.rs b/rs/consensus/cup_utils/src/lib.rs index 8bf354154c3d..f6a144659be5 100644 --- a/rs/consensus/cup_utils/src/lib.rs +++ b/rs/consensus/cup_utils/src/lib.rs @@ -6,7 +6,7 @@ use ic_consensus_idkg::{ utils::{get_idkg_chain_key_config_if_enabled, inspect_idkg_chain_key_initializations}, }; use ic_interfaces_registry::RegistryClient; -use ic_logger::{ReplicaLogger, warn}; +use ic_logger::ReplicaLogger; use ic_protobuf::registry::subnet::v1::CatchUpPackageContents; use ic_registry_client_helpers::subnet::SubnetRegistry; use ic_types::{ @@ -14,15 +14,35 @@ use ic_types::{ batch::ValidationContext, consensus::{ Block, BlockPayload, CatchUpContent, CatchUpPackage, HashedBlock, HashedRandomBeacon, - Payload, RandomBeaconContent, Rank, SummaryPayload, idkg, + Payload, RandomBeaconContent, Rank, RegistryCUP, RegistryCupType, SummaryPayload, idkg, }, crypto::{ CombinedThresholdSig, CombinedThresholdSigOf, CryptoHash, Signed, crypto_hash, threshold_sig::ni_dkg::NiDkgTag, }, + registry::RegistryClientError, signature::ThresholdSignature, }; use phantom_newtype::Id; +use thiserror::Error; + +#[derive(Error, Debug)] +pub enum RegistryCupCreationError { + #[error("Failed to retrieve subnet replica version at registry version {0}: {1:?}")] + ReplicaVersionError(RegistryVersion, RegistryClientError), + #[error("Missing subnet replica version at registry version {0}")] + ReplicaVersionMissing(RegistryVersion), + #[error("Failed constructing NiDKG summary block from CUP contents: {0}")] + DkgSummaryCreationError(String), + #[error("Failed constructing IDKG summary block from CUP contents: {0}")] + IDkgSummaryCreationError(String), + #[error("No current threshold transcript with tag {0:?} in registry CUP contents")] + ThresholdTranscriptMissing(NiDkgTag), + #[error("Missing registry CUP contents at version {0}")] + CupContentsMissing(RegistryVersion), + #[error("Failed to retrieve versioned record from the registry at version {0}: {1:?}")] + FailedToGetCupContents(RegistryVersion, RegistryClientError), +} /// Constructs a genesis/recovery CUP from the CUP contents associated with the /// given subnet from the provided CUP contents @@ -32,73 +52,44 @@ pub fn make_registry_cup_from_cup_contents( cup_contents: CatchUpPackageContents, registry_version: RegistryVersion, logger: &ReplicaLogger, -) -> Option { - let replica_version = match registry.get_replica_version(subnet_id, registry_version) { - Ok(Some(replica_version)) => replica_version, - err => { - warn!( - logger, - "Failed to retrieve subnet replica version at registry version {:?}: {:?}", - registry_version, - err - ); - return None; - } - }; - let dkg_summary = match get_dkg_summary_from_cup_contents( +) -> Result { + let replica_version = registry + .get_replica_version(subnet_id, registry_version) + .map_err(|err| RegistryCupCreationError::ReplicaVersionError(registry_version, err))? + .ok_or(RegistryCupCreationError::ReplicaVersionMissing( + registry_version, + ))?; + + let dkg_summary = get_dkg_summary_from_cup_contents( cup_contents.clone(), subnet_id, registry, registry_version, - ) { - Ok(summary) => summary, - Err(err) => { - warn!( - logger, - "Failed constructing NiDKG summary block from CUP contents: {}.", err - ); - - return None; - } - }; + ) + .map_err(RegistryCupCreationError::DkgSummaryCreationError)?; let cup_height = Height::new(cup_contents.height); - let idkg_summary = match bootstrap_idkg_summary( + let idkg_summary = bootstrap_idkg_summary( cup_contents.clone(), subnet_id, registry_version, registry, logger, - ) { - Ok(summary) => summary, - Err(err) => { - warn!( - logger, - "Failed constructing IDKG summary block from CUP contents: {}.", err - ); - - return None; - } - }; + ) + .map_err(RegistryCupCreationError::IDkgSummaryCreationError)?; - let Some(low_threshold_transcript) = dkg_summary.current_transcript(&NiDkgTag::LowThreshold) - else { - warn!( - logger, - "No current low threshold transcript in registry CUP contents" - ); - return None; - }; + let low_threshold_transcript = dkg_summary + .current_transcript(&NiDkgTag::LowThreshold) + .ok_or(RegistryCupCreationError::ThresholdTranscriptMissing( + NiDkgTag::LowThreshold, + ))?; let low_dkg_id = low_threshold_transcript.dkg_id.clone(); - let Some(high_threshold_transcript) = dkg_summary.current_transcript(&NiDkgTag::HighThreshold) - else { - warn!( - logger, - "No current high threshold transcript in registry CUP contents" - ); - return None; - }; + let high_threshold_transcript = dkg_summary + .current_transcript(&NiDkgTag::HighThreshold) + .ok_or(RegistryCupCreationError::ThresholdTranscriptMissing( + NiDkgTag::HighThreshold, + ))?; let high_dkg_id = high_threshold_transcript.dkg_id.clone(); // In a NNS subnet recovery case the block validation context needs to reference a registry @@ -138,8 +129,9 @@ pub fn make_registry_cup_from_cup_contents( signature: CombinedThresholdSigOf::new(CombinedThresholdSig(vec![])), }, }; + let cup_type = RegistryCupType::from(&cup_contents); - Some(CatchUpPackage { + let cup = CatchUpPackage { content: CatchUpContent::new( HashedBlock::new(crypto_hash, block), HashedRandomBeacon::new(crypto_hash, random_beacon), @@ -150,7 +142,9 @@ pub fn make_registry_cup_from_cup_contents( signer: high_dkg_id, signature: CombinedThresholdSigOf::new(CombinedThresholdSig(vec![])), }, - }) + }; + + Ok(RegistryCUP { cup, cup_type }) } /// Constructs a genesis/recovery CUP from the CUP contents associated with the @@ -159,26 +153,20 @@ pub fn make_registry_cup( registry: &dyn RegistryClient, subnet_id: SubnetId, logger: &ReplicaLogger, -) -> Option { - let versioned_record = match registry.get_cup_contents(subnet_id, registry.get_latest_version()) - { - Ok(versioned_record) => versioned_record, - Err(e) => { - warn!( - logger, - "Failed to retrieve versioned record from the registry {:?}", e, - ); - return None; - } - }; +) -> Result { + let latest_registry_version = registry.get_latest_version(); + let versioned_record = registry + .get_cup_contents(subnet_id, latest_registry_version) + .map_err(|err| { + RegistryCupCreationError::FailedToGetCupContents(latest_registry_version, err) + })?; - let Some(cup_contents) = versioned_record.value else { - warn!( - logger, - "Missing registry CUP contents at version {}", versioned_record.version - ); - return None; - }; + let cup_contents = + versioned_record + .value + .ok_or(RegistryCupCreationError::CupContentsMissing( + versioned_record.version, + ))?; make_registry_cup_from_cup_contents( registry, @@ -313,23 +301,26 @@ mod tests { let result = make_registry_cup(®istry_client, subnet_test_id(0), &no_op_logger()).unwrap(); + assert_eq!(result.cup_type, RegistryCupType::Recovery); + let cup = result.cup; + assert_eq!( - result.content.state_hash.get_ref(), + cup.content.state_hash.get_ref(), &CryptoHash(vec![1, 2, 3, 4, 5]) ); assert_eq!( - result.content.block.get_value().context.registry_version, + cup.content.block.get_value().context.registry_version, RegistryVersion::from(12345) ); assert_eq!( - result.content.block.get_value().context.certified_height, + cup.content.block.get_value().context.certified_height, Height::from(54321) ); assert_eq!( - result.content.version(), + cup.content.version(), &ReplicaVersion::try_from("TestID").unwrap() ); - assert_eq!(result.signature.signer.dealer_subnet, subnet_test_id(0)); + assert_eq!(cup.signature.signer.dealer_subnet, subnet_test_id(0)); } /// `RegistryClient` implementation that allows to provide a custom function diff --git a/rs/consensus/utils/BUILD.bazel b/rs/consensus/utils/BUILD.bazel index 56f02312c344..10967023f779 100644 --- a/rs/consensus/utils/BUILD.bazel +++ b/rs/consensus/utils/BUILD.bazel @@ -17,11 +17,13 @@ DEPENDENCIES = [ "@crate_index//:rand", "@crate_index//:rayon", "@crate_index//:slog", + "@crate_index//:thiserror", ] DEV_DEPENDENCIES = [ # Keep sorted. "//rs/consensus/mocks", + "//rs/registry/keys", "//rs/test_utilities", "//rs/test_utilities/consensus", "//rs/test_utilities/registry", @@ -30,6 +32,7 @@ DEV_DEPENDENCIES = [ "//rs/test_utilities/types", "//rs/types/management_canister_types", "@crate_index//:assert_matches", + "@crate_index//:rstest", ] rust_library( diff --git a/rs/consensus/utils/Cargo.toml b/rs/consensus/utils/Cargo.toml index 0043a08d62d6..aad09cef7b7f 100644 --- a/rs/consensus/utils/Cargo.toml +++ b/rs/consensus/utils/Cargo.toml @@ -19,14 +19,17 @@ ic-types = { path = "../../types/types" } prometheus = { workspace = true } rand = { workspace = true } slog = { workspace = true } +thiserror = { workspace = true } [dev-dependencies] assert_matches = { workspace = true } ic-consensus-mocks = { path = "../mocks" } ic-management-canister-types-private = { path = "../../types/management_canister_types" } +ic-registry-keys = { path = "../../registry/keys" } ic-test-utilities = { path = "../../test_utilities" } ic-test-utilities-consensus = { path = "../../test_utilities/consensus" } ic-test-utilities-registry = { path = "../../test_utilities/registry" } ic-test-utilities-state = { path = "../../test_utilities/state" } ic-test-utilities-time = { path = "../../test_utilities/time" } ic-test-utilities-types = { path = "../../test_utilities/types" } +rstest= { workspace = true } diff --git a/rs/consensus/utils/src/lib.rs b/rs/consensus/utils/src/lib.rs index 55a2b19b1c88..9eb1d85b7f1a 100644 --- a/rs/consensus/utils/src/lib.rs +++ b/rs/consensus/utils/src/lib.rs @@ -25,6 +25,7 @@ pub mod chain_key; pub mod crypto; pub mod membership; pub mod pool_reader; +pub mod subnet_splitting; /// When purging consensus or certification artifacts, we always keep a /// minimum chain length below the catch-up height. diff --git a/rs/consensus/utils/src/subnet_splitting.rs b/rs/consensus/utils/src/subnet_splitting.rs new file mode 100644 index 000000000000..86a1080106e3 --- /dev/null +++ b/rs/consensus/utils/src/subnet_splitting.rs @@ -0,0 +1,191 @@ +use ic_interfaces_registry::RegistryClient; +use ic_protobuf::{ + proxy::ProxyDecodeError, registry::subnet::v1::catch_up_package_contents::CupType, +}; +use ic_registry_client_helpers::subnet::SubnetRegistry; +use ic_types::{ + RegistryVersion, SubnetId, consensus::SubnetSplittingArgs, registry::RegistryClientError, +}; +use thiserror::Error; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Status { + Scheduled { destination_subnet_id: SubnetId }, + AlreadyDone, + NotScheduled, +} + +#[derive(Debug, Error)] +pub enum StatusError { + #[error("Error while getting CatchUpContents at registry version {0}: {1:?}")] + FailedToGetCatchUpContents(RegistryVersion, RegistryClientError), + #[error("CatchUpContents not found at registry version: {0}")] + CatchUpContentsMissingInRegistry(RegistryVersion), + #[error("Failed to deserialize CatchUpContents: {0}")] + CatchUpContentsDeserializationError(ProxyDecodeError), +} + +pub struct Context { + last_summary_block_registry_version: RegistryVersion, + current_registry_version: RegistryVersion, +} + +pub fn get_status( + registry_client: &dyn RegistryClient, + subnet_id: SubnetId, + Context { + last_summary_block_registry_version, + current_registry_version, + }: Context, +) -> Result { + let versioned_record = registry_client + .get_cup_contents(subnet_id, current_registry_version) + .map_err(|err| StatusError::FailedToGetCatchUpContents(current_registry_version, err))?; + + let Some(contents) = versioned_record.value else { + return Err(StatusError::CatchUpContentsMissingInRegistry( + current_registry_version, + )); + }; + + let Some(CupType::SubnetSplitting(subnet_splitting_args_proto)) = contents.cup_type else { + return Ok(Status::NotScheduled); + }; + + if versioned_record.version <= last_summary_block_registry_version { + return Ok(Status::AlreadyDone); + } + + let subnet_splitting_args: SubnetSplittingArgs = subnet_splitting_args_proto + .try_into() + .map_err(StatusError::CatchUpContentsDeserializationError)?; + + Ok(Status::Scheduled { + destination_subnet_id: subnet_splitting_args.destination_subnet_id, + }) +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use ic_protobuf::registry::subnet::v1::CatchUpPackageContents; + use ic_protobuf::registry::subnet::v1::{GenesisArgs, RecoveryArgs}; + use ic_registry_keys::make_catch_up_package_contents_key; + use ic_test_utilities_registry::{SubnetRecordBuilder, setup_registry_non_final}; + use ic_test_utilities_types::ids::{NODE_1, SUBNET_1, SUBNET_2}; + use ic_types::subnet_id_into_protobuf; + use rstest::rstest; + + const SOURCE_SUBNET_ID: SubnetId = SUBNET_1; + const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2; + + use super::*; + + #[rstest] + fn should_return_not_scheduled_test( + #[values( + None, + Some(CupType::Genesis(GenesisArgs { height: 0 })), + Some(CupType::Recovery(RecoveryArgs { + height: 1_000, + time: 1, + state_hash: vec![], + })), + )] + cup_type: Option, + ) { + let registry = set_up_registry(RegistryVersion::new(1), cup_type); + + let status = get_status( + registry.as_ref(), + SUBNET_1, + Context { + last_summary_block_registry_version: RegistryVersion::new(1), + current_registry_version: RegistryVersion::new(2), + }, + ) + .expect("Should succeed given correct inputs"); + + assert_eq!(status, Status::NotScheduled); + } + + #[test] + fn should_return_scheduled_test() { + let registry = set_up_registry( + RegistryVersion::new(1), + Some(CupType::SubnetSplitting( + ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { + destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), + }, + )), + ); + + let status = get_status( + registry.as_ref(), + SOURCE_SUBNET_ID, + Context { + last_summary_block_registry_version: RegistryVersion::new(1), + current_registry_version: RegistryVersion::new(2), + }, + ) + .expect("Should succeed given correct inputs"); + + assert_eq!( + status, + Status::Scheduled { + destination_subnet_id: DESTINATION_SUBNET_ID + } + ); + } + + #[test] + fn should_return_already_done_test() { + let registry = set_up_registry( + RegistryVersion::new(1), + Some(CupType::SubnetSplitting( + ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { + destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), + }, + )), + ); + + let status = get_status( + registry.as_ref(), + SOURCE_SUBNET_ID, + Context { + last_summary_block_registry_version: RegistryVersion::new(2), + current_registry_version: RegistryVersion::new(2), + }, + ) + .expect("Should succeed given correct inputs"); + + assert_eq!(status, Status::AlreadyDone); + } + + fn set_up_registry( + cup_registry_version: RegistryVersion, + cup_type: Option, + ) -> Arc { + let (registry_data_provider, registry) = setup_registry_non_final( + SOURCE_SUBNET_ID, + vec![( + 1, + SubnetRecordBuilder::new().with_committee(&[NODE_1]).build(), + )], + ); + registry_data_provider + .add( + &make_catch_up_package_contents_key(SOURCE_SUBNET_ID), + cup_registry_version, + Some(CatchUpPackageContents { + cup_type, + ..Default::default() + }), + ) + .unwrap(); + registry.update_to_latest_version(); + + registry + } +} diff --git a/rs/orchestrator/src/catch_up_package_provider.rs b/rs/orchestrator/src/catch_up_package_provider.rs index 4441dd56da7d..ab06a169ec73 100644 --- a/rs/orchestrator/src/catch_up_package_provider.rs +++ b/rs/orchestrator/src/catch_up_package_provider.rs @@ -464,6 +464,7 @@ impl CatchUpPackageProvider { let registry_cup = self .registry .get_registry_cup(registry_version, subnet_id) + .inspect_err(|err| warn!(self.logger, "Failed to create a registry cup: {err}")) .map(pb::CatchUpPackage::from) .ok(); @@ -474,10 +475,7 @@ impl CatchUpPackageProvider { .into_iter() .flatten() .max_by_key(get_cup_proto_height) - .ok_or(OrchestratorError::MakeRegistryCupError( - subnet_id, - registry_version, - ))?; + .ok_or(OrchestratorError::CupMissing(subnet_id, registry_version))?; let latest_cup = CatchUpPackage::try_from(&latest_cup_proto).map_err(|err| { OrchestratorError::deserialize_cup_error(get_cup_proto_height(&latest_cup_proto), err) })?; diff --git a/rs/orchestrator/src/error.rs b/rs/orchestrator/src/error.rs index 8359d2d18c57..1e374f19451e 100644 --- a/rs/orchestrator/src/error.rs +++ b/rs/orchestrator/src/error.rs @@ -1,3 +1,4 @@ +use ic_consensus_cup_utils::RegistryCupCreationError; use ic_http_utils::file_downloader::FileDownloadError; use ic_image_upgrader::error::UpgradeError; use ic_types::{ @@ -36,7 +37,10 @@ pub(crate) enum OrchestratorError { RegistryClientError(RegistryClientError), /// The genesis or recovery CUP failed to be constructed - MakeRegistryCupError(SubnetId, RegistryVersion), + MakeRegistryCupError(SubnetId, RegistryVersion, RegistryCupCreationError), + + /// No cup found at the registry version + CupMissing(SubnetId, RegistryVersion), /// The CUP at the given height failed to be deserialized DeserializeCupError(Option, String), @@ -112,7 +116,8 @@ impl fmt::Display for OrchestratorError { OrchestratorError::ReplicaVersionMissingError(replica_version, registry_version) => { write!( f, - "Replica version {replica_version} was not found in the Registry at registry version {registry_version}" + "Replica version {replica_version} was not found in the \ + Registry at registry version {registry_version}" ) } OrchestratorError::IoError(msg, e) => { @@ -137,15 +142,18 @@ impl fmt::Display for OrchestratorError { } OrchestratorError::SubnetMissingError(subnet_id, registry_version) => write!( f, - "Subnet ID {subnet_id} does not exist in the Registry at registry version {registry_version}" + "Subnet ID {subnet_id} does not exist in the \ + Registry at registry version {registry_version}" ), OrchestratorError::ApiBoundaryNodeMissingError(node_id, registry_version) => write!( f, - "Api Boundary Node ID {node_id} does not exist in the Registry at registry version {registry_version}" + "Api Boundary Node ID {node_id} does not exist in the \ + Registry at registry version {registry_version}" ), OrchestratorError::NodeRecordMissingError(node_id, registry_version) => write!( f, - "Node ID {node_id} does not exist in the Registry at registry version {registry_version}" + "Node ID {node_id} does not exist in the \ + Registry at registry version {registry_version}" ), OrchestratorError::ReplicaVersionParseError(e) => { write!(f, "Failed to parse replica version: {e}") @@ -153,9 +161,14 @@ impl fmt::Display for OrchestratorError { OrchestratorError::SerializeCryptoConfigError(e) => { write!(f, "Failed to serialize crypto-config: {e}") } - OrchestratorError::MakeRegistryCupError(subnet_id, registry_version) => write!( + OrchestratorError::MakeRegistryCupError(subnet_id, registry_version, err) => write!( + f, + "Failed to construct the genesis/recovery CUP, \ + subnet_id: {subnet_id}, registry_version: {registry_version}. Error: {err}", + ), + OrchestratorError::CupMissing(subnet_id, registry_version) => write!( f, - "Failed to construct the genesis/recovery CUP, subnet_id: {subnet_id}, registry_version: {registry_version}", + "CUP is missing subnet_id: {subnet_id}, registry_version: {registry_version}.", ), OrchestratorError::DeserializeCupError(height, error) => write!( f, @@ -168,7 +181,8 @@ impl fmt::Display for OrchestratorError { OrchestratorError::RoleError(msg, registry_version) => { write!( f, - "Failed to get the role of the node at the registry version {registry_version}: {msg}" + "Failed to get the role of the node at the \ + registry version {registry_version}: {msg}" ) } OrchestratorError::DomainNameMissingError(node_id) => { diff --git a/rs/orchestrator/src/registry_helper.rs b/rs/orchestrator/src/registry_helper.rs index 848c4fac4362..8199725d567a 100644 --- a/rs/orchestrator/src/registry_helper.rs +++ b/rs/orchestrator/src/registry_helper.rs @@ -157,14 +157,15 @@ impl RegistryHelper { )) } - /// Return the genesis cup at the given registry version for this node + /// Return the registry cup at the given registry version for this node pub(crate) fn get_registry_cup( &self, version: RegistryVersion, subnet_id: SubnetId, ) -> OrchestratorResult { make_registry_cup(&*self.registry_client, subnet_id, &self.logger) - .ok_or(OrchestratorError::MakeRegistryCupError(subnet_id, version)) + .map(|registry_cup| registry_cup.cup) + .map_err(|err| OrchestratorError::MakeRegistryCupError(subnet_id, version, err)) } pub(crate) fn get_firewall_rules( diff --git a/rs/replay/src/lib.rs b/rs/replay/src/lib.rs index a5af0f5e0ce4..c2d545cdb213 100644 --- a/rs/replay/src/lib.rs +++ b/rs/replay/src/lib.rs @@ -258,7 +258,8 @@ fn cmd_get_recovery_cup( registry_version, &player.log, ) - .ok_or_else(|| "couldn't create a registry CUP".to_string())?; + .map_err(|err| format!("couldn't create a registry CUP: {err}"))? + .cup; println!( "height: {}, time: {}, state_hash: {:?}", diff --git a/rs/replica/src/setup_ic_stack.rs b/rs/replica/src/setup_ic_stack.rs index bd1bc1b3b1c5..1571d5064665 100644 --- a/rs/replica/src/setup_ic_stack.rs +++ b/rs/replica/src/setup_ic_stack.rs @@ -108,7 +108,8 @@ pub fn construct_ic_stack( None => { let registry_cup = ic_consensus_cup_utils::make_registry_cup(&*registry, subnet_id, log) - .expect("Couldn't create a registry CUP"); + .expect("Couldn't create a registry CUP") + .cup; info!( log, diff --git a/rs/state_machine_tests/src/lib.rs b/rs/state_machine_tests/src/lib.rs index 624602f247f5..19a3dee496ac 100644 --- a/rs/state_machine_tests/src/lib.rs +++ b/rs/state_machine_tests/src/lib.rs @@ -550,7 +550,7 @@ fn make_fresh_registry_cup( replica_logger, ) .unwrap(); - cup.into() + cup.cup.into() } /// Convert an object into CBOR binary. diff --git a/rs/types/types/src/consensus/catchup.rs b/rs/types/types/src/consensus/catchup.rs index 6d812bb54a54..1ff864a7d3d8 100644 --- a/rs/types/types/src/consensus/catchup.rs +++ b/rs/types/types/src/consensus/catchup.rs @@ -9,8 +9,10 @@ use crate::{ crypto::*, node_id_into_protobuf, node_id_try_from_option, }; +use ic_base_types::{SubnetId, subnet_id_try_from_option}; use ic_protobuf::{ proxy::{ProxyDecodeError, try_from_option_field}, + registry::subnet::v1 as subnet_pb, types::v1 as pb, }; use prost::Message; @@ -384,39 +386,100 @@ impl SignedBytesWithoutDomainSeparator for CatchUpContentProtobufBytes { } } -#[test] -fn test_catch_up_package_param_partial_ord() { - let c1 = CatchUpPackageParam { - height: Height::from(1), - registry_version: RegistryVersion::from(1), - }; - let c2 = CatchUpPackageParam { - height: Height::from(2), - registry_version: RegistryVersion::from(1), - }; - let c3 = CatchUpPackageParam { - height: Height::from(2), - registry_version: RegistryVersion::from(2), - }; - let c4 = CatchUpPackageParam { - height: Height::from(1), - registry_version: RegistryVersion::from(2), - }; - let c5 = CatchUpPackageParam { - height: Height::from(0), - registry_version: RegistryVersion::from(2), - }; - // c2 > c1 - assert_eq!(c2.partial_cmp(&c1), Some(Ordering::Greater)); - // c3 > c1 - assert_eq!(c3.partial_cmp(&c1), Some(Ordering::Greater)); - // c3 > c2. This can happen when we want to recover a stuck subnet - // with a new CatchUpPackage. - assert_eq!(c3.partial_cmp(&c2), Some(Ordering::Greater)); - // c3 == c3 - assert_eq!(c3.partial_cmp(&c3), Some(Ordering::Equal)); - // c4 > c1 - assert_eq!(c4.partial_cmp(&c1), Some(Ordering::Greater)); - // c5 does not compare to c1 - assert_eq!(c5.partial_cmp(&c1), None); +#[derive(Debug, Eq, PartialEq)] +pub enum RegistryCupType { + Genesis, + Recovery, + SubnetSplitting, +} + +impl From<&subnet_pb::CatchUpPackageContents> for RegistryCupType { + fn from(value: &subnet_pb::CatchUpPackageContents) -> Self { + match value.cup_type { + Some(subnet_pb::catch_up_package_contents::CupType::Genesis(..)) => { + RegistryCupType::Genesis + } + Some(subnet_pb::catch_up_package_contents::CupType::Recovery(..)) => { + RegistryCupType::Recovery + } + Some(subnet_pb::catch_up_package_contents::CupType::SubnetSplitting(..)) => { + RegistryCupType::SubnetSplitting + } + None => { + if value.state_hash.is_empty() { + RegistryCupType::Genesis + } else { + RegistryCupType::Recovery + } + } + } + } +} + +pub struct RegistryCUP { + pub cup: CatchUpPackage, + pub cup_type: RegistryCupType, +} + +pub struct SubnetSplittingArgs { + pub destination_subnet_id: SubnetId, +} + +impl TryFrom for SubnetSplittingArgs { + type Error = ProxyDecodeError; + + fn try_from( + subnet_pb::SubnetSplittingArgs { + destination_subnet_id, + }: subnet_pb::SubnetSplittingArgs, + ) -> Result { + let destination_subnet_id = + subnet_id_try_from_option(destination_subnet_id, "destination_subnet_id")?; + + Ok(SubnetSplittingArgs { + destination_subnet_id, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_catch_up_package_param_partial_ord() { + let c1 = CatchUpPackageParam { + height: Height::from(1), + registry_version: RegistryVersion::from(1), + }; + let c2 = CatchUpPackageParam { + height: Height::from(2), + registry_version: RegistryVersion::from(1), + }; + let c3 = CatchUpPackageParam { + height: Height::from(2), + registry_version: RegistryVersion::from(2), + }; + let c4 = CatchUpPackageParam { + height: Height::from(1), + registry_version: RegistryVersion::from(2), + }; + let c5 = CatchUpPackageParam { + height: Height::from(0), + registry_version: RegistryVersion::from(2), + }; + // c2 > c1 + assert_eq!(c2.partial_cmp(&c1), Some(Ordering::Greater)); + // c3 > c1 + assert_eq!(c3.partial_cmp(&c1), Some(Ordering::Greater)); + // c3 > c2. This can happen when we want to recover a stuck subnet + // with a new CatchUpPackage. + assert_eq!(c3.partial_cmp(&c2), Some(Ordering::Greater)); + // c3 == c3 + assert_eq!(c3.partial_cmp(&c3), Some(Ordering::Equal)); + // c4 > c1 + assert_eq!(c4.partial_cmp(&c1), Some(Ordering::Greater)); + // c5 does not compare to c1 + assert_eq!(c5.partial_cmp(&c1), None); + } } From 3a0bd1a8a17db0dfe9183c243d4e1f0fc7344aac Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Mon, 30 Mar 2026 14:23:39 +0000 Subject: [PATCH 02/84] . --- rs/consensus/dkg/src/payload_builder.rs | 7 +- rs/consensus/dkg/src/payload_validator.rs | 27 +- rs/consensus/idkg/src/payload_builder.rs | 10 +- .../idkg/src/payload_builder/errors.rs | 3 +- rs/consensus/idkg/src/payload_verifier.rs | 16 +- rs/consensus/src/consensus/block_maker.rs | 239 +++++++++++++++++- .../src/consensus/malicious_consensus.rs | 16 +- rs/consensus/src/consensus/validator.rs | 44 +++- rs/consensus/utils/src/subnet_splitting.rs | 63 +++-- .../artifact_pool/src/consensus_pool.rs | 5 +- rs/types/types/src/consensus/dkg.rs | 1 - 11 files changed, 348 insertions(+), 83 deletions(-) diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs index b8b977b12e19..2f665cc232a4 100644 --- a/rs/consensus/dkg/src/payload_builder.rs +++ b/rs/consensus/dkg/src/payload_builder.rs @@ -47,16 +47,13 @@ pub fn create_payload( pool_reader: &PoolReader<'_>, dkg_pool: Arc>, parent: &Block, + last_summary_block: &Block, state_manager: &dyn StateManager, validation_context: &ValidationContext, logger: ReplicaLogger, max_dealings_per_block: usize, ) -> 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 { @@ -82,7 +79,7 @@ pub fn create_payload( dkg_pool, parent, max_dealings_per_block, - &last_summary_block, + last_summary_block, last_dkg_summary, ) .map(DkgPayload::Data) diff --git a/rs/consensus/dkg/src/payload_validator.rs b/rs/consensus/dkg/src/payload_validator.rs index b29f9b833ca4..464e2108b503 100644 --- a/rs/consensus/dkg/src/payload_validator.rs +++ b/rs/consensus/dkg/src/payload_validator.rs @@ -30,6 +30,7 @@ pub fn validate_payload( pool_reader: &PoolReader<'_>, dkg_pool: &dyn DkgPool, parent: Block, + last_summary_block: &Block, payload: &BlockPayload, state_manager: &dyn StateManager, validation_context: &ValidationContext, @@ -40,12 +41,6 @@ pub fn validate_payload( let registry_version = pool_reader .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; @@ -279,7 +274,9 @@ mod tests { // This will be a regular block, since we are not at dkg_interval_length height 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), @@ -288,6 +285,7 @@ mod tests { &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), parent_block, + &last_summary_block, block_payload, state_manager.as_ref(), &context, @@ -303,6 +301,9 @@ mod tests { // This will be a summary block, since we are at dkg_interval_length height 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( @@ -312,6 +313,7 @@ mod tests { &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), parent_block, + &last_summary_block, summary, state_manager.as_ref(), &context, @@ -524,6 +526,8 @@ mod tests { idkg: idkg::Payload::default(), }); + let last_summary_block = PoolReader::new(&pool).dkg_summary_block(&parent).unwrap(); + assert_eq!( validate_payload( SUBNET_1, @@ -532,6 +536,7 @@ mod tests { &PoolReader::new(&pool), dkg_pool.read().unwrap().deref(), parent, + &last_summary_block, &block_payload, state_manager.as_ref(), &context, @@ -598,13 +603,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, @@ -771,6 +778,8 @@ 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(), @@ -778,6 +787,7 @@ mod tests { &PoolReader::new(&pool), &dkg_pool, parent.clone(), + &last_summary_block, &block_payload, state_manager.as_ref(), &context, @@ -798,6 +808,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 bacef9bfb1ba..484cb7f67aff 100644 --- a/rs/consensus/idkg/src/payload_builder.rs +++ b/rs/consensus/idkg/src/payload_builder.rs @@ -166,6 +166,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 { @@ -177,9 +178,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). @@ -485,6 +483,7 @@ pub fn create_data_payload( state_manager: &dyn StateManager, context: &ValidationContext, parent_block: &Block, + summary_block: &Block, idkg_payload_metrics: &IDkgPayloadMetrics, log: &ReplicaLogger, ) -> Result { @@ -497,9 +496,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- @@ -531,7 +527,7 @@ pub fn create_data_payload( subnet_id, context, parent_block, - &summary_block, + summary_block, &block_reader, &transcript_builder, state_manager, 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 1798cee005eb..afb1d9d0568e 100644 --- a/rs/consensus/idkg/src/payload_verifier.rs +++ b/rs/consensus/idkg/src/payload_verifier.rs @@ -230,6 +230,7 @@ pub fn validate_payload( state_manager: &dyn StateManager, context: &ValidationContext, parent_block: &Block, + last_summary_block: &Block, payload: &BlockPayload, metrics: HistogramVec, ) -> ValidationResult { @@ -243,6 +244,7 @@ pub fn validate_payload( pool_reader, context, parent_block, + last_summary_block, payload.as_summary().idkg.as_ref(), ) }, @@ -261,6 +263,7 @@ pub fn validate_payload( state_manager, context, parent_block, + last_summary_block, payload.as_data().idkg.as_ref(), &metrics, ) @@ -279,6 +282,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(); @@ -301,6 +305,7 @@ fn validate_summary_payload( pool_reader, context, parent_block, + last_summary_block, None, &ic_logger::replica_logger::no_op_logger(), ) { @@ -332,6 +337,7 @@ fn validate_data_payload( state_manager: &dyn StateManager, context: &ValidationContext, parent_block: &Block, + summary_block: &Block, data_payload: Option<&idkg::IDkgPayload>, metrics: &HistogramVec, ) -> ValidationResult { @@ -378,14 +384,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. @@ -432,7 +430,7 @@ fn validate_data_payload( subnet_id, context, parent_block, - &summary_block, + summary_block, &block_reader, &builder, state_manager, diff --git a/rs/consensus/src/consensus/block_maker.rs b/rs/consensus/src/consensus/block_maker.rs index 5c7f61761140..8b32c31a046c 100755 --- a/rs/consensus/src/consensus/block_maker.rs +++ b/rs/consensus/src/consensus/block_maker.rs @@ -11,13 +11,14 @@ use ic_consensus_utils::{ get_subnet_record, membership::Membership, pool_reader::{PoolReader, UnexpectedChainLength}, + subnet_splitting, }; use ic_interfaces::{ consensus::PayloadBuilder, dkg::DkgPool, idkg::IDkgPool, time_source::TimeSource, }; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::StateManager; -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::{ @@ -186,6 +187,11 @@ impl BlockMaker { let height = parent.height().increment(); let certified_height = self.state_manager.latest_certified_height(); + let Some(last_summary_block) = pool.dkg_summary_block(parent.get_value()) else { + warn!(self.log, "Couldn't find the summary block"); + return None; + }; + // Note that we will skip blockmaking if registry versions or replica_versions // are missing or temporarily not retrievable. // @@ -203,7 +209,16 @@ impl BlockMaker { // The stable registry version to be agreed on in this block. If this is a summary // block, this version will be the new membership version of the next dkg interval. - let stable_registry_version = self.get_stable_registry_version(parent.as_ref())?; + let stable_registry_version = self.get_stable_registry_version( + parent.as_ref(), + last_summary_block.context.registry_version, + last_summary_block + .payload + .as_ref() + .as_summary() + .dkg + .get_next_start_height(), + )?; // Get the subnet records that are relevant to making a block let subnet_records = subnet_records_for_registry_version(self, registry_version, stable_registry_version)?; @@ -272,6 +287,7 @@ impl BlockMaker { pool, context, parent, + &last_summary_block, height, rank, registry_version, @@ -287,6 +303,7 @@ impl BlockMaker { pool: &PoolReader<'_>, context: ValidationContext, parent: HashedBlock, + last_summary_block: &Block, height: Height, rank: Rank, registry_version: RegistryVersion, @@ -302,6 +319,7 @@ impl BlockMaker { pool, Arc::clone(&self.dkg_pool), parent.as_ref(), + last_summary_block, &*self.state_manager, &context, self.log.clone(), @@ -322,6 +340,7 @@ impl BlockMaker { pool, &context, parent.as_ref(), + last_summary_block, Some(&self.idkg_payload_metrics), &self.log, ) @@ -369,6 +388,7 @@ impl BlockMaker { &*self.state_manager, &context, parent.as_ref(), + last_summary_block, &self.idkg_payload_metrics, &self.log, ) @@ -482,20 +502,64 @@ impl BlockMaker { } } - // Returns the registry version received from the NNS some specified amount of - // time ago. If the parent's context references higher version which is already - // available locally, we use that version. - pub(crate) fn get_stable_registry_version(&self, parent: &Block) -> Option { + /// Returns the registry version received from the NNS some specified amount of + /// time ago. If the parent's context references higher version which is already + /// available locally, we use that version. + pub(crate) fn get_stable_registry_version( + &self, + parent: &Block, + last_summary_block_registry_version: RegistryVersion, + next_summary_block_height: Height, + ) -> Option { let parents_version = parent.context.registry_version; + let parents_height = parent.height(); let latest_version = self.registry_client.get_latest_version(); // Check if there is a stable version that we can bump up to. for v in (parents_version.get()..=latest_version.get()).rev() { let version = RegistryVersion::from(v); + + // Don't consider a registry version if it's too fresh. let version_timestamp = self.registry_client.get_version_timestamp(version)?; - if version_timestamp + self.stable_registry_version_age <= current_time() { - return Some(version); + if version_timestamp + self.stable_registry_version_age > current_time() { + continue; } + + let subnet_splitting_status = subnet_splitting::get_status( + self.registry_client.as_ref(), + self.replica_config.subnet_id, + subnet_splitting::Context { + last_summary_block_registry_version, + current_registry_version: version, + }, + ) + .inspect_err(|err| { + warn!( + self.log, + "Failed to get subnet splitting status at registry version {version}: {err}" + ) + }) + .ok()?; + + match subnet_splitting_status { + subnet_splitting::Status::Scheduled { scheduled_at, .. } => { + info!( + every_n_seconds => 30, + self.log, + "Subnet splitting schedulled. Freezing registry version." + ); + + if parents_height.increment() == next_summary_block_height { + return Some(scheduled_at); + } + + continue; + } + subnet_splitting::Status::AlreadyDone | subnet_splitting::Status::NotScheduled => {} + } + + return Some(version); } + // If parent's version is locally available, return that. if parents_version <= latest_version { return Some(parents_version); @@ -630,6 +694,10 @@ mod tests { use ic_interfaces::consensus_pool::ConsensusPool; use ic_logger::replica_logger::no_op_logger; use ic_metrics::MetricsRegistry; + use ic_protobuf::registry::subnet::v1::{ + CatchUpPackageContents, SubnetSplittingArgs, catch_up_package_contents::CupType, + }; + use ic_registry_keys::make_catch_up_package_contents_key; use ic_test_utilities_consensus::{IDkgStatsNoOp, fake::FromParent}; use ic_test_utilities_registry::{SubnetRecordBuilder, add_subnet_record}; use ic_test_utilities_types::ids::{node_test_id, subnet_test_id}; @@ -641,6 +709,8 @@ mod tests { signature::ThresholdSignature, *, }; + use ic_types_test_utils::ids::NODE_1; + use ic_types_test_utils::ids::{SUBNET_0, SUBNET_1}; use rstest::rstest; use std::sync::{Arc, RwLock}; @@ -1201,25 +1271,33 @@ mod tests { block_maker.stable_registry_version_age = current_time().saturating_duration_since(v3_timestamp); assert_eq!( - block_maker.get_stable_registry_version(&parent).unwrap(), + block_maker + .get_stable_registry_version(&parent, RegistryVersion::new(1), Height::new(100)) + .unwrap(), RegistryVersion::from(3) ); block_maker.stable_registry_version_age = current_time().saturating_duration_since(v2_timestamp); assert_eq!( - block_maker.get_stable_registry_version(&parent).unwrap(), + block_maker + .get_stable_registry_version(&parent, RegistryVersion::new(1), Height::new(100)) + .unwrap(), RegistryVersion::from(2) ); block_maker.stable_registry_version_age = current_time().saturating_duration_since(v1_timestamp); assert_eq!( - block_maker.get_stable_registry_version(&parent).unwrap(), + block_maker + .get_stable_registry_version(&parent, RegistryVersion::new(1), Height::new(100)) + .unwrap(), RegistryVersion::from(1) ); // Now let's test if parent's version is used parent.context.registry_version = RegistryVersion::from(2); assert_eq!( - block_maker.get_stable_registry_version(&parent).unwrap(), + block_maker + .get_stable_registry_version(&parent, RegistryVersion::new(1), Height::new(100)) + .unwrap(), RegistryVersion::from(2) ); }) @@ -1337,4 +1415,141 @@ mod tests { ) }) } + + mod subnet_splitting { + use super::*; + + const MAX_REGISTRY_VERSION: u64 = 4; + + #[derive(Debug)] + struct TestCase { + splitting_registry_version: Option, + last_summary_block_registry_version: RegistryVersion, + next_summary_block_height: Height, + parent_height: Height, + expected_stable_registry_version: RegistryVersion, + } + + #[rstest] + #[case::no_splitting(TestCase { + splitting_registry_version: None, + last_summary_block_registry_version: RegistryVersion::new(1), + next_summary_block_height: Height::new(4), + parent_height: Height::new(1), + expected_stable_registry_version: RegistryVersion::new(MAX_REGISTRY_VERSION), + })] + #[case::version_frozen_before_splitting(TestCase { + splitting_registry_version: Some(RegistryVersion::new(MAX_REGISTRY_VERSION - 1)), + last_summary_block_registry_version: RegistryVersion::new(1), + next_summary_block_height: Height::new(4), + parent_height: Height::new(1), + expected_stable_registry_version: RegistryVersion::new(MAX_REGISTRY_VERSION - 2), + })] + #[case::version_frozen_before_splitting(TestCase { + splitting_registry_version: Some(RegistryVersion::new(MAX_REGISTRY_VERSION - 2)), + last_summary_block_registry_version: RegistryVersion::new(1), + next_summary_block_height: Height::new(4), + parent_height: Height::new(1), + expected_stable_registry_version: RegistryVersion::new(MAX_REGISTRY_VERSION - 3), + })] + #[case::exact_version_during_splitting(TestCase { + splitting_registry_version: Some(RegistryVersion::new(MAX_REGISTRY_VERSION - 1)), + last_summary_block_registry_version: RegistryVersion::new(1), + next_summary_block_height: Height::new(4), + parent_height: Height::new(3), + expected_stable_registry_version: RegistryVersion::new(MAX_REGISTRY_VERSION - 1), + })] + fn test_stable_registry_version_with_subnet_splitting(#[case] test_case: TestCase) { + const SOURCE_SUBNET_ID: SubnetId = SUBNET_0; + const DESTINATION_SUBNET_ID: SubnetId = SUBNET_1; + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + let record = SubnetRecordBuilder::from(&[NODE_1]) + .with_dkg_interval_length(4) + .build(); + let Dependencies { + registry, + crypto, + pool, + time_source, + replica_config, + state_manager, + registry_data_provider, + dkg_pool, + idkg_pool, + .. + } = dependencies_with_subnet_params( + pool_config, + SOURCE_SUBNET_ID, + vec![(1, record.clone())], + ); + + let mut payload_builder = MockPayloadBuilder::new(); + payload_builder + .expect_get_payload() + .return_const(BatchPayload::default()); + let membership = Arc::new(Membership::new( + pool.get_cache(), + registry.clone(), + replica_config.subnet_id, + )); + + let block_maker = BlockMaker::new( + Arc::clone(&time_source) as Arc<_>, + replica_config, + Arc::clone(®istry) as Arc, + membership, + crypto, + Arc::new(payload_builder), + dkg_pool, + idkg_pool, + state_manager, + Duration::from_millis(0), + MetricsRegistry::new(), + no_op_logger(), + ); + + for version in 2..=MAX_REGISTRY_VERSION { + add_subnet_record( + ®istry_data_provider, + version, + SOURCE_SUBNET_ID, + record.clone(), + ); + } + if let Some(splitting_registry_version) = test_case.splitting_registry_version { + registry_data_provider + .add( + &make_catch_up_package_contents_key(SOURCE_SUBNET_ID), + splitting_registry_version, + Some(CatchUpPackageContents { + cup_type: Some(CupType::SubnetSplitting(SubnetSplittingArgs { + destination_subnet_id: Some(subnet_id_into_protobuf( + DESTINATION_SUBNET_ID, + )), + })), + ..Default::default() + }), + ) + .unwrap(); + } + + registry.update_to_latest_version(); + let mut parent = pool.get_cache().finalized_block(); + parent.height = test_case.parent_height; + parent.context.registry_version = RegistryVersion::from(1); + + std::thread::sleep(Duration::from_secs(1)); + assert_eq!( + block_maker + .get_stable_registry_version( + &parent, + test_case.last_summary_block_registry_version, + test_case.next_summary_block_height, + ) + .unwrap(), + test_case.expected_stable_registry_version, + ); + }) + } + } } diff --git a/rs/consensus/src/consensus/malicious_consensus.rs b/rs/consensus/src/consensus/malicious_consensus.rs index 2becac747cd4..56017c164a1d 100644 --- a/rs/consensus/src/consensus/malicious_consensus.rs +++ b/rs/consensus/src/consensus/malicious_consensus.rs @@ -143,11 +143,18 @@ 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.get_value())?; // Get the subnet records that are relevant to making a block - let stable_registry_version = self - .block_maker - .get_stable_registry_version(parent.as_ref())?; + let stable_registry_version = self.block_maker.get_stable_registry_version( + parent.as_ref(), + last_summary_block.context.registry_version, + last_summary_block + .payload + .as_ref() + .as_summary() + .dkg + .get_next_start_height(), + )?; let subnet_records = block_maker::subnet_records_for_registry_version( &self.block_maker, registry_version, @@ -158,6 +165,7 @@ impl ConsensusImpl { pool, context, parent, + &last_summary_block, height, rank, registry_version, diff --git a/rs/consensus/src/consensus/validator.rs b/rs/consensus/src/consensus/validator.rs index 87b03b09c1b6..f935a39f3b10 100644 --- a/rs/consensus/src/consensus/validator.rs +++ b/rs/consensus/src/consensus/validator.rs @@ -15,6 +15,7 @@ use ic_consensus_utils::{ get_oldest_idkg_state_registry_version, membership::{Membership, MembershipError}, pool_reader::{PoolReader, UnexpectedChainLength}, + subnet_splitting, }; use ic_interfaces::{ batch_payload::ProposalContext, @@ -86,9 +87,11 @@ enum ValidationFailure { BlockNotFound(CryptoHashOf, Height), FinalizedBlockNotFound(Height), FailedToGetRegistryVersion, + FailedToGetConsensusStatus, ValidationContextNotReached(ValidationContext, ValidationContext), CatchUpHeightNegligible, MissingPastPayloads, + SubnetSplittingStatusError(subnet_splitting::StatusError), } /// Possible reasons for invalid artifacts. @@ -118,6 +121,9 @@ enum InvalidArtifactReason { RepeatedSigner, ReplicaVersionMismatch, NotABlockmaker, + RegistryVersionNotFrozenDuringSubnetSplitting { + context_registry_version: RegistryVersion, + }, } impl From for ValidationFailure { @@ -1200,6 +1206,11 @@ 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(), self.registry_client.as_ref(), @@ -1207,7 +1218,7 @@ impl Validator { pool_reader, &self.log, ) else { - return Err(ValidationFailure::FailedToGetRegistryVersion.into()); + return Err(ValidationFailure::FailedToGetConsensusStatus.into()); }; // If the replica is halted, block payload should be empty. @@ -1219,7 +1230,6 @@ impl Validator { } 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. @@ -1274,6 +1284,34 @@ impl Validator { .into()); } + // if it's not a summary block sure, make sure that the registry version is 'frozen' during + // subnet splitting + if !proposal.payload.is_summary() { + match subnet_splitting::get_status( + self.registry_client.as_ref(), + self.replica_config.subnet_id, + subnet_splitting::Context { + last_summary_block_registry_version: last_summary_block + .context + .registry_version, + current_registry_version: proposal.context.registry_version, + }, + ) + .map_err(ValidationFailure::SubnetSplittingStatusError)? + { + subnet_splitting::Status::Scheduled { .. } => { + return Err( + InvalidArtifactReason::RegistryVersionNotFrozenDuringSubnetSplitting { + context_registry_version: proposal.context.registry_version, + } + .into(), + ); + } + subnet_splitting::Status::AlreadyDone => {} + subnet_splitting::Status::NotScheduled => {} + } + } + // If the replica is halted, the block payload is empty so we can skip the rest of the // validation. if status == Status::Halting || status == Status::Halted { @@ -1328,6 +1366,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 +1390,7 @@ impl Validator { pool_reader, dkg_pool, parent, + &last_summary_block, proposal.payload.as_ref(), self.state_manager.as_ref(), &proposal.context, diff --git a/rs/consensus/utils/src/subnet_splitting.rs b/rs/consensus/utils/src/subnet_splitting.rs index 86a1080106e3..9c44392a6399 100644 --- a/rs/consensus/utils/src/subnet_splitting.rs +++ b/rs/consensus/utils/src/subnet_splitting.rs @@ -10,7 +10,11 @@ use thiserror::Error; #[derive(Debug, Clone, PartialEq, Eq)] pub enum Status { - Scheduled { destination_subnet_id: SubnetId }, + Scheduled { + destination_subnet_id: SubnetId, + /// The registry version at which the subnet was scheduled to be split + scheduled_at: RegistryVersion, + }, AlreadyDone, NotScheduled, } @@ -25,9 +29,10 @@ pub enum StatusError { CatchUpContentsDeserializationError(ProxyDecodeError), } +#[derive(Debug)] pub struct Context { - last_summary_block_registry_version: RegistryVersion, - current_registry_version: RegistryVersion, + pub last_summary_block_registry_version: RegistryVersion, + pub current_registry_version: RegistryVersion, } pub fn get_status( @@ -62,6 +67,7 @@ pub fn get_status( Ok(Status::Scheduled { destination_subnet_id: subnet_splitting_args.destination_subnet_id, + scheduled_at: versioned_record.version, }) } @@ -79,6 +85,7 @@ mod tests { const SOURCE_SUBNET_ID: SubnetId = SUBNET_1; const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2; + const REGISTRY_CUP_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(2); use super::*; @@ -95,14 +102,14 @@ mod tests { )] cup_type: Option, ) { - let registry = set_up_registry(RegistryVersion::new(1), cup_type); + let registry = set_up_registry(cup_type); let status = get_status( registry.as_ref(), SUBNET_1, Context { - last_summary_block_registry_version: RegistryVersion::new(1), - current_registry_version: RegistryVersion::new(2), + last_summary_block_registry_version: REGISTRY_CUP_REGISTRY_VERSION.decrement(), + current_registry_version: REGISTRY_CUP_REGISTRY_VERSION, }, ) .expect("Should succeed given correct inputs"); @@ -112,21 +119,18 @@ mod tests { #[test] fn should_return_scheduled_test() { - let registry = set_up_registry( - RegistryVersion::new(1), - Some(CupType::SubnetSplitting( - ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { - destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), - }, - )), - ); + let registry = set_up_registry(Some(CupType::SubnetSplitting( + ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { + destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), + }, + ))); let status = get_status( registry.as_ref(), SOURCE_SUBNET_ID, Context { - last_summary_block_registry_version: RegistryVersion::new(1), - current_registry_version: RegistryVersion::new(2), + last_summary_block_registry_version: REGISTRY_CUP_REGISTRY_VERSION.decrement(), + current_registry_version: REGISTRY_CUP_REGISTRY_VERSION, }, ) .expect("Should succeed given correct inputs"); @@ -134,28 +138,26 @@ mod tests { assert_eq!( status, Status::Scheduled { - destination_subnet_id: DESTINATION_SUBNET_ID + destination_subnet_id: DESTINATION_SUBNET_ID, + scheduled_at: REGISTRY_CUP_REGISTRY_VERSION, } ); } #[test] fn should_return_already_done_test() { - let registry = set_up_registry( - RegistryVersion::new(1), - Some(CupType::SubnetSplitting( - ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { - destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), - }, - )), - ); + let registry = set_up_registry(Some(CupType::SubnetSplitting( + ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { + destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), + }, + ))); let status = get_status( registry.as_ref(), SOURCE_SUBNET_ID, Context { - last_summary_block_registry_version: RegistryVersion::new(2), - current_registry_version: RegistryVersion::new(2), + last_summary_block_registry_version: REGISTRY_CUP_REGISTRY_VERSION, + current_registry_version: REGISTRY_CUP_REGISTRY_VERSION, }, ) .expect("Should succeed given correct inputs"); @@ -163,10 +165,7 @@ mod tests { assert_eq!(status, Status::AlreadyDone); } - fn set_up_registry( - cup_registry_version: RegistryVersion, - cup_type: Option, - ) -> Arc { + fn set_up_registry(cup_type: Option) -> Arc { let (registry_data_provider, registry) = setup_registry_non_final( SOURCE_SUBNET_ID, vec![( @@ -177,7 +176,7 @@ mod tests { registry_data_provider .add( &make_catch_up_package_contents_key(SOURCE_SUBNET_ID), - cup_registry_version, + REGISTRY_CUP_REGISTRY_VERSION, Some(CatchUpPackageContents { cup_type, ..Default::default() diff --git a/rs/test_utilities/artifact_pool/src/consensus_pool.rs b/rs/test_utilities/artifact_pool/src/consensus_pool.rs index feb338f088d1..5068af546eb6 100644 --- a/rs/test_utilities/artifact_pool/src/consensus_pool.rs +++ b/rs/test_utilities/artifact_pool/src/consensus_pool.rs @@ -150,13 +150,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 47243a8d8550..e6b840dec8a5 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -596,7 +596,6 @@ pub enum DkgPayloadCreationError { FailedToGetDkgIntervalSettingFromRegistry(RegistryClientError), FailedToGetSubnetMemberListFromRegistry(RegistryClientError), FailedToGetVetKdKeyList(RegistryClientError), - MissingDkgStartBlock, } /// Reasons for why a dkg payload might be invalid. From a7534d438e7cc4c064c6b32edf8e47b619252eb6 Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Wed, 1 Apr 2026 12:38:22 +0000 Subject: [PATCH 03/84] status --- rs/consensus/dkg/src/payload_builder.rs | 36 +- rs/consensus/idkg/src/payload_builder.rs | 1 + rs/consensus/src/consensus/batch_delivery.rs | 59 ++-- rs/consensus/src/consensus/block_maker.rs | 39 ++- rs/consensus/src/consensus/notary.rs | 1 + rs/consensus/src/consensus/status.rs | 310 +++++++++++------- rs/consensus/src/consensus/validator.rs | 2 + rs/nns/governance/src/lib.rs | 2 +- rs/protobuf/def/types/v1/dkg.proto | 12 + rs/protobuf/src/gen/types/types.v1.rs | 22 ++ rs/registry/canister/src/flags.rs | 2 +- .../canister/src/mutations/do_split_subnet.rs | 5 +- rs/test_utilities/consensus/src/fake.rs | 1 + .../consensus/subnet_splitting_v2_test.rs | 2 +- rs/types/types/src/consensus.rs | 62 ++++ rs/types/types/src/consensus/dkg.rs | 116 ++++++- rs/types/types/src/exhaustive.rs | 16 + 17 files changed, 513 insertions(+), 175 deletions(-) diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs index 2f665cc232a4..88d8f164a301 100644 --- a/rs/consensus/dkg/src/payload_builder.rs +++ b/rs/consensus/dkg/src/payload_builder.rs @@ -2,7 +2,7 @@ use crate::{ MAX_REMOTE_DKG_ATTEMPTS, MAX_REMOTE_DKGS_PER_INTERVAL, REMOTE_DKG_REPEATED_FAILURE_ERROR, utils::{self, tags_iter, vetkd_key_ids_for_subnet}, }; -use ic_consensus_utils::{crypto::ConsensusCrypto, pool_reader::PoolReader}; +use ic_consensus_utils::{crypto::ConsensusCrypto, pool_reader::PoolReader, subnet_splitting}; use ic_interfaces::{crypto::ErrorReproducibility, dkg::DkgPool}; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::StateManager; @@ -19,7 +19,9 @@ use ic_types::{ batch::ValidationContext, consensus::{ Block, - dkg::{DkgDataPayload, DkgPayload, DkgPayloadCreationError, DkgSummary}, + dkg::{ + DkgDataPayload, DkgPayload, DkgPayloadCreationError, DkgSummary, SubnetSplittingStatus, + }, get_faults_tolerated, }, crypto::threshold_sig::ni_dkg::{ @@ -35,6 +37,8 @@ use std::{ sync::{Arc, RwLock}, }; +const SUBNET_SPLITTING_ENABLED: bool = true; + /// Creates the DKG payload for a new block proposal with the given parent. If /// the new height corresponds to a new DKG start interval, creates a summary, /// otherwise it creates a payload containing new dealings for the current @@ -252,6 +256,32 @@ pub(super) fn create_summary_payload( subnet_id, )?; + let subnet_splitting_status = if SUBNET_SPLITTING_ENABLED { + let status = subnet_splitting::get_status( + registry_client, + subnet_id, + subnet_splitting::Context { + last_summary_block_registry_version: registry_version, + current_registry_version: validation_context.registry_version, + }, + ) + .map_err(|err| DkgPayloadCreationError::SubnetSplittingStatusError(err.to_string()))?; + + match status { + subnet_splitting::Status::Scheduled { + destination_subnet_id, + scheduled_at: _, + } => Some(SubnetSplittingStatus::Scheduled { + destination_subnet_id, + source_subnet_id: subnet_id, + }), + subnet_splitting::Status::AlreadyDone => Some(SubnetSplittingStatus::NotScheduled), + subnet_splitting::Status::NotScheduled => Some(SubnetSplittingStatus::NotScheduled), + } + } else { + None + }; + // New configs are created using the new stable registry version proposed by this // block, which determines receivers of the dealings. configs.append(&mut get_configs_for_local_transcripts( @@ -277,6 +307,7 @@ pub(super) fn create_summary_payload( next_interval_length, height, initial_dkg_attempts, + subnet_splitting_status, )) } @@ -552,6 +583,7 @@ pub fn get_dkg_summary_from_cup_contents( next_interval_length, height, BTreeMap::new(), // initial_dkg_attempts + None, )) } diff --git a/rs/consensus/idkg/src/payload_builder.rs b/rs/consensus/idkg/src/payload_builder.rs index 484cb7f67aff..c621a4320ff8 100644 --- a/rs/consensus/idkg/src/payload_builder.rs +++ b/rs/consensus/idkg/src/payload_builder.rs @@ -775,6 +775,7 @@ mod tests { Height::from(100), height, BTreeMap::new(), + None, ), idkg: Some(idkg_summary), }) diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 8f37e592bb6a..e308b7ecd831 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -32,14 +32,13 @@ use ic_types::{ Batch, BatchContent, BatchMessages, BatchSummary, BlockmakerMetrics, ChainKeyData, ConsensusResponse, }, - consensus::{ - Block, BlockPayload, HasVersion, - idkg::{self}, - }, - crypto::randomness_from_crypto_hashable, - crypto::threshold_sig::{ - ThresholdSigPublicKey, - ni_dkg::{NiDkgId, NiDkgTag, NiDkgTranscript}, + consensus::{Block, BlockPayload, HasVersion, idkg}, + crypto::{ + randomness_from_crypto_hashable, + threshold_sig::{ + ThresholdSigPublicKey, + ni_dkg::{NiDkgId, NiDkgTag, NiDkgTranscript}, + }, }, messages::{CallbackId, Payload, RejectContext}, }; @@ -137,6 +136,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, @@ -145,13 +158,19 @@ 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, + info!( + every_n_seconds => 30, log, - "Batch of height {} is not delivered because replica is halted", - height, + "Batch of height {height} is not delivered because replica is halted" ); return Ok(last_delivered_batch_height); } @@ -168,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 8b32c31a046c..19209bb258f8 100755 --- a/rs/consensus/src/consensus/block_maker.rs +++ b/rs/consensus/src/consensus/block_maker.rs @@ -28,7 +28,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, @@ -348,6 +348,13 @@ impl BlockMaker { .ok() .flatten(); + if matches!( + summary.subnet_splitting_status.as_ref(), + Some(&SubnetSplittingStatus::Scheduled { .. }) + ) { + info!(self.log, "Proposing a Splitting block at height {height}."); + } + BlockPayload::Summary(SummaryPayload { dkg: summary, idkg: idkg_summary, @@ -356,21 +363,34 @@ 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, &self.log, )? { - // Don't propose any block if the replica is halted. Status::Halted => { + info!( + every_n_seconds => 30, + self.log, + "Not proposing any block at height {height} \ + because the replica is halted" + ); return None; } - // Use empty payload and empty DKG dealings if the replica is halting. - Status::Halting => ( - BatchPayload::default(), - DkgDataPayload::new_empty(dkg.start_height), - /*idkg_data=*/ None, - ), + Status::Halting => { + info!( + every_n_seconds => 30, + self.log, + "Proposing an empty block at height {height} \ + because the replica is halting" + ); + ( + BatchPayload::default(), + DkgDataPayload::new_empty(dkg.start_height), + /*idkg_data=*/ None, + ) + } Status::Running => { let batch_payload = self.build_batch_payload( pool, @@ -545,7 +565,8 @@ impl BlockMaker { info!( every_n_seconds => 30, self.log, - "Subnet splitting schedulled. Freezing registry version." + "Subnet splitting schedulled at registry version {scheduled_at} \ + and height {next_summary_block_height}. Freezing registry version." ); if parents_height.increment() == next_summary_block_height { diff --git a/rs/consensus/src/consensus/notary.rs b/rs/consensus/src/consensus/notary.rs index 8767510af5ca..f7136f0b8969 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..37eb6fa6977d 100644 --- a/rs/consensus/src/consensus/status.rs +++ b/rs/consensus/src/consensus/status.rs @@ -4,7 +4,10 @@ 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::SubnetSplittingStatus}, +}; #[derive(Copy, Clone, Eq, PartialEq, Debug)] pub(crate) enum Status { @@ -29,18 +32,33 @@ pub(crate) enum Status { /// 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 +72,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,25 +83,63 @@ 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 + .as_ref() + { + Some(&SubnetSplittingStatus::NotScheduled) => false, + // After the split, don't produce any blocks until we are on the right subnet. + Some(&SubnetSplittingStatus::Done { new_subnet_id }) => subnet_id != new_subnet_id, + Some(&SubnetSplittingStatus::Scheduled { .. }) => height >= summary_block.height, + None => false, + } + }) + .unwrap_or_default(); let should_halt_by_subnet_record = registry_client .get_halt_at_cup_height(subnet_id, registry_version) + .inspect_err(|err| { + warn!( + logger, + "Failed querying the registry at version {registry_version}: {err}" + ) + }) .ok() .flatten() .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_by_subnet_record, + Some(should_halt_due_to_subnet_splitting), + ]) +} + +/// Returns `true` if any of the provided values is known to be `true`. +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 +169,28 @@ 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, + consensus::{BlockPayload, Payload}, + 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 +200,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 +214,138 @@ mod tests { ], ); - pool.advance_round_normal_operation_n(10); + pool.advance_round_normal_operation_no_cup_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 { destination_subnet_id: SUBNET_0, source_subnet_id: SUBNET_1 }), + 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 { destination_subnet_id: SUBNET_0, source_subnet_id: SUBNET_1 }), + subnet_id: SUBNET_0, + expected_status: Some(Status::Halting), + })] + #[case::post_subnet_splitting_old_subnet_id(TestCase{ + certified_height: CUP_HEIGHT.decrement(), + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: Some(SubnetSplittingStatus::Done { new_subnet_id: SUBNET_0 }), + subnet_id: SUBNET_1, + expected_status: Some(Status::Halted), + })] + #[case::post_subnet_splitting_new_subnet_id(TestCase{ + certified_height: CUP_HEIGHT.decrement(), + current_height: CUP_HEIGHT, + replica_version: ReplicaVersion::default(), + halt_at_cup_height: false, + subnet_splitting_status: Some(SubnetSplittingStatus::Done { new_subnet_id: SUBNET_0 }), + subnet_id: SUBNET_0, + 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( + use ic_types::consensus::BackwardsCompatibleOption; + + 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 = + BackwardsCompatibleOption(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 f935a39f3b10..0359defe7111 100644 --- a/rs/consensus/src/consensus/validator.rs +++ b/rs/consensus/src/consensus/validator.rs @@ -99,6 +99,7 @@ enum ValidationFailure { // The fields are only read by the `Debug` implementation. // The `dead_code` lint ignores `Debug` impls, see: https://github.com/rust-lang/rust/issues/88900. #[allow(dead_code)] +#[allow(clippy::large_enum_variant)] enum InvalidArtifactReason { CryptoError(CryptoError), MismatchedRank(Rank, Option), @@ -1213,6 +1214,7 @@ impl Validator { let Some(status) = status::get_status( proposal.height(), + &last_summary_block, self.registry_client.as_ref(), self.replica_config.subnet_id, pool_reader, diff --git a/rs/nns/governance/src/lib.rs b/rs/nns/governance/src/lib.rs index 2a3e01c237b6..d89691ef588e 100644 --- a/rs/nns/governance/src/lib.rs +++ b/rs/nns/governance/src/lib.rs @@ -218,7 +218,7 @@ thread_local! { = const { Cell::new(true) }; static ENABLE_SUBNET_SPLITTING_PROPOSALS: Cell - = const { Cell::new(false) }; + = const { Cell::new(true) }; } thread_local! { diff --git a/rs/protobuf/def/types/v1/dkg.proto b/rs/protobuf/def/types/v1/dkg.proto index c86bb33c2d5f..534f3beb2074 100644 --- a/rs/protobuf/def/types/v1/dkg.proto +++ b/rs/protobuf/def/types/v1/dkg.proto @@ -2,6 +2,7 @@ syntax = "proto3"; package types.v1; +import "google/protobuf/empty.proto"; import "types/v1/types.proto"; message DkgMessage { @@ -28,6 +29,12 @@ message DkgDataPayload { repeated CallbackIdedNiDkgTranscript transcripts_for_remote_subnets = 3; } +message SplittingArgs { + SubnetId destination_subnet_id = 1; + SubnetId source_subnet_id = 2; +} + +// next id: 16 message Summary { reserved 5, 6, 8; reserved "transcripts_for_new_subnets"; @@ -40,6 +47,11 @@ message Summary { repeated CallbackIdedNiDkgTranscript transcripts_for_remote_subnets = 10; repeated NiDkgTranscript current_transcripts = 11; repeated NiDkgTranscript next_transcripts = 12; + oneof subnet_splitting_status { + google.protobuf.Empty not_scheduled = 13; + SplittingArgs scheduled = 14; + SubnetId done = 15; + } } message CallbackIdedNiDkgTranscript { diff --git a/rs/protobuf/src/gen/types/types.v1.rs b/rs/protobuf/src/gen/types/types.v1.rs index b1ace3c9a0b3..f4454456b96f 100644 --- a/rs/protobuf/src/gen/types/types.v1.rs +++ b/rs/protobuf/src/gen/types/types.v1.rs @@ -370,6 +370,14 @@ pub struct DkgDataPayload { pub transcripts_for_remote_subnets: ::prost::alloc::vec::Vec, } #[derive(Clone, PartialEq, ::prost::Message)] +pub struct SplittingArgs { + #[prost(message, optional, tag = "1")] + pub destination_subnet_id: ::core::option::Option, + #[prost(message, optional, tag = "2")] + pub source_subnet_id: ::core::option::Option, +} +/// next id: 16 +#[derive(Clone, PartialEq, ::prost::Message)] pub struct Summary { #[prost(uint64, tag = "1")] pub registry_version: u64, @@ -389,6 +397,20 @@ pub struct Summary { pub current_transcripts: ::prost::alloc::vec::Vec, #[prost(message, repeated, tag = "12")] pub next_transcripts: ::prost::alloc::vec::Vec, + #[prost(oneof = "summary::SubnetSplittingStatus", tags = "13, 14, 15")] + pub subnet_splitting_status: ::core::option::Option, +} +/// Nested message and enum types in `Summary`. +pub mod summary { + #[derive(Clone, PartialEq, ::prost::Oneof)] + pub enum SubnetSplittingStatus { + #[prost(message, tag = "13")] + NotScheduled(()), + #[prost(message, tag = "14")] + Scheduled(super::SplittingArgs), + #[prost(message, tag = "15")] + Done(super::SubnetId), + } } #[derive(Clone, PartialEq, ::prost::Message)] pub struct CallbackIdedNiDkgTranscript { diff --git a/rs/registry/canister/src/flags.rs b/rs/registry/canister/src/flags.rs index f999a0b6abcb..f86aa90875b0 100644 --- a/rs/registry/canister/src/flags.rs +++ b/rs/registry/canister/src/flags.rs @@ -6,7 +6,7 @@ use ic_nervous_system_temporary::Temporary; use ic_types::{PrincipalId, SubnetId}; thread_local! { - static IS_SUBNET_SPLITTING_ENABLED: Cell = const { Cell::new(false) }; + static IS_SUBNET_SPLITTING_ENABLED: Cell = const { Cell::new(true) }; static IS_CHUNKIFYING_LARGE_VALUES_ENABLED: Cell = const { Cell::new(true) }; static IS_NODE_SWAPPING_ENABLED: Cell = const { Cell::new(true) }; diff --git a/rs/registry/canister/src/mutations/do_split_subnet.rs b/rs/registry/canister/src/mutations/do_split_subnet.rs index 6ba96890fa15..8b7134e5a210 100644 --- a/rs/registry/canister/src/mutations/do_split_subnet.rs +++ b/rs/registry/canister/src/mutations/do_split_subnet.rs @@ -388,12 +388,9 @@ impl Registry { &self, record_key: &str, version: Version, - ) -> Version { + ) -> Option { self.get(record_key.as_bytes(), version) .map(|record| record.version) - .unwrap_or_else(|| { - panic!("Record for {record_key} not found in registry"); - }) } } diff --git a/rs/test_utilities/consensus/src/fake.rs b/rs/test_utilities/consensus/src/fake.rs index 3d53ce787e09..ef2fc392784c 100644 --- a/rs/test_utilities/consensus/src/fake.rs +++ b/rs/test_utilities/consensus/src/fake.rs @@ -67,6 +67,7 @@ impl Fake for DkgSummary { /*next_interval_length=*/ Height::new(59), /*height=*/ Height::new(0), /*initial_dkg_attempts=*/ BTreeMap::default(), + /*subnet_splitting_status=*/ None, ) } } diff --git a/rs/tests/consensus/subnet_splitting_v2_test.rs b/rs/tests/consensus/subnet_splitting_v2_test.rs index 87679832ade1..0e0f0d9b554c 100644 --- a/rs/tests/consensus/subnet_splitting_v2_test.rs +++ b/rs/tests/consensus/subnet_splitting_v2_test.rs @@ -68,7 +68,7 @@ const CHATTING_CANISTERS_ON_THIRD_SUBNET_COUNT: usize = 3; const FIRST_CHATTING_CANISTER_ID_TO_MIGRATE_OFFSET: usize = 3; const LAST_CHATTING_CANISTER_ID_TO_MIGRATE_OFFSET: usize = 8; -const TEST_ENABLED: bool = false; +const TEST_ENABLED: bool = true; fn main() -> Result<()> { SystemTestGroup::new() diff --git a/rs/types/types/src/consensus.rs b/rs/types/types/src/consensus.rs index bd208b6c68ea..fb405402f667 100644 --- a/rs/types/types/src/consensus.rs +++ b/rs/types/types/src/consensus.rs @@ -1816,3 +1816,65 @@ impl ConsensusMessageHashable for ConsensusMessage { } } } + +#[derive(Clone, Eq, PartialEq, Debug, Deserialize, Serialize)] +/// A helper struct used for introducing new fields to structs which need to be backwards compatible. +/// The main difference w.r.t. [`Option`] is that its [`Hash`] implementation ignores the `None` +/// variant, which means that the hash of the struct without the field would be the same as the hash +/// of the struct with the field present but set to `None`. +/// +/// Lifecycle of adding a new field to a struct in a backwards compatible way: +/// 1. Add a new field to the struct with type `BackwardsCompatibleOption`. At this point +/// the field is *NOT* allowed to have any value other than `None`. +/// 2. When the change is deployed to all replicas, we can switch the type to +/// `BackwardsCompatibleOption` and the field can begin to be populated. +/// 3. When the change is deployed to all replicas, we can replace the type with `T`. +pub struct BackwardsCompatibleOption(pub Option); + +impl Default for BackwardsCompatibleOption { + fn default() -> Self { + Self(None) + } +} + +impl Default for BackwardsCompatibleOption { + fn default() -> Self { + Self(Some(T::default())) + } +} + +impl Hash for BackwardsCompatibleOption { + fn hash(&self, state: &mut H) { + if let Some(value) = &self.0 { + value.hash(state); + } + } +} + +impl From> for BackwardsCompatibleOption { + fn from(value: Option) -> Self { + Self(value) + } +} + +impl From for BackwardsCompatibleOption { + fn from(value: T) -> Self { + Self(Some(value)) + } +} + +impl BackwardsCompatibleOption { + pub const fn as_ref(&self) -> Option<&T> { + self.0.as_ref() + } + + pub fn try_from_proto>( + proto: Option, + ) -> Result { + let Some(value) = proto else { + return Ok(Self(None)); + }; + + Ok(Self(Some(value.try_into()?))) + } +} diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index e6b840dec8a5..c86efe96bb98 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -150,6 +150,23 @@ impl HasVersion for DealingContent { } } +#[derive(Copy, Clone, Serialize, Deserialize, Eq, PartialEq, Hash, Debug, Default)] +#[cfg_attr(test, derive(ExhaustiveSet))] +/// Represents the status of subnet splitting at the given summary height. +pub enum SubnetSplittingStatus { + /// The subnet hasn't been requested to be split. + #[default] + NotScheduled, + /// The subnet is requested to be split at the height of the summary block. + /// Contains all the information necessary to determine the new subnet of the replica + Scheduled { + destination_subnet_id: SubnetId, + source_subnet_id: SubnetId, + }, + /// The subnet was split at the previous summary block. + Done { new_subnet_id: SubnetId }, +} + /// The DKG summary will be present as the DKG payload at every block, /// corresponding to the start of a new DKG interval. #[serde_as] @@ -182,6 +199,8 @@ pub struct DkgSummary { pub height: Height, /// The number of intervals a DKG for the given remote target was attempted. pub initial_dkg_attempts: BTreeMap, + /// Status of the subnet splitting. + pub subnet_splitting_status: BackwardsCompatibleOption, } impl DkgSummary { @@ -197,6 +216,7 @@ impl DkgSummary { next_interval_length: Height, height: Height, initial_dkg_attempts: BTreeMap, + subnet_splitting_status: Option, ) -> Self { Self { configs: configs @@ -211,6 +231,7 @@ impl DkgSummary { next_interval_length, height, initial_dkg_attempts, + subnet_splitting_status: BackwardsCompatibleOption(subnet_splitting_status), } } @@ -342,23 +363,35 @@ fn build_initial_dkg_attempts_vec( } impl From<&DkgSummary> for pb::Summary { - fn from(summary: &DkgSummary) -> Self { + fn from( + DkgSummary { + registry_version, + configs, + current_transcripts, + next_transcripts, + transcripts_for_remote_subnets, + interval_length, + next_interval_length, + height, + initial_dkg_attempts, + subnet_splitting_status, + }: &DkgSummary, + ) -> Self { Self { - registry_version: summary.registry_version.get(), - configs: summary - .configs - .values() - .map(pb::NiDkgConfig::from) - .collect(), - current_transcripts: build_transcripts_vec(&summary.current_transcripts), - next_transcripts: build_transcripts_vec(&summary.next_transcripts), - interval_length: summary.interval_length.get(), - next_interval_length: summary.next_interval_length.get(), - height: summary.height.get(), + registry_version: registry_version.get(), + configs: configs.values().map(pb::NiDkgConfig::from).collect(), + current_transcripts: build_transcripts_vec(current_transcripts), + next_transcripts: build_transcripts_vec(next_transcripts), + interval_length: interval_length.get(), + next_interval_length: next_interval_length.get(), + height: height.get(), transcripts_for_remote_subnets: build_callback_ided_transcripts_vec( - summary.transcripts_for_remote_subnets.as_slice(), + transcripts_for_remote_subnets, ), - initial_dkg_attempts: build_initial_dkg_attempts_vec(&summary.initial_dkg_attempts), + initial_dkg_attempts: build_initial_dkg_attempts_vec(initial_dkg_attempts), + subnet_splitting_status: subnet_splitting_status + .as_ref() + .map(pb::summary::SubnetSplittingStatus::from), } } } @@ -433,6 +466,56 @@ fn build_transcript_result( } } +impl From<&SubnetSplittingStatus> for pb::summary::SubnetSplittingStatus { + fn from(status: &SubnetSplittingStatus) -> Self { + match status { + SubnetSplittingStatus::NotScheduled => { + pb::summary::SubnetSplittingStatus::NotScheduled(()) + } + SubnetSplittingStatus::Scheduled { + destination_subnet_id, + source_subnet_id, + } => pb::summary::SubnetSplittingStatus::Scheduled(pb::SplittingArgs { + destination_subnet_id: Some(subnet_id_into_protobuf(*destination_subnet_id)), + source_subnet_id: Some(subnet_id_into_protobuf(*source_subnet_id)), + }), + SubnetSplittingStatus::Done { new_subnet_id } => { + pb::summary::SubnetSplittingStatus::Done(subnet_id_into_protobuf(*new_subnet_id)) + } + } + } +} + +impl TryFrom for SubnetSplittingStatus { + type Error = ProxyDecodeError; + + fn try_from(status: pb::summary::SubnetSplittingStatus) -> Result { + match status { + pb::summary::SubnetSplittingStatus::NotScheduled(()) => { + Ok(SubnetSplittingStatus::NotScheduled) + } + pb::summary::SubnetSplittingStatus::Scheduled(pb::SplittingArgs { + destination_subnet_id, + source_subnet_id, + }) => Ok(SubnetSplittingStatus::Scheduled { + destination_subnet_id: subnet_id_try_from_option( + destination_subnet_id, + "SubnetSplittingStatus::destination_subnet_id", + )?, + source_subnet_id: subnet_id_try_from_option( + source_subnet_id, + "SubnetSplittingStatus::source_subnet_id", + )?, + }), + pb::summary::SubnetSplittingStatus::Done(subnet_id) => { + Ok(SubnetSplittingStatus::Done { + new_subnet_id: subnet_id_try_from_protobuf(subnet_id)?, + }) + } + } + } +} + impl TryFrom for DkgSummary { type Error = ProxyDecodeError; @@ -454,6 +537,9 @@ impl TryFrom for DkgSummary { ) .map_err(ProxyDecodeError::Other)?, initial_dkg_attempts: build_initial_dkg_attempts_map(&summary.initial_dkg_attempts), + subnet_splitting_status: BackwardsCompatibleOption::try_from_proto( + summary.subnet_splitting_status, + )?, }) } } @@ -596,9 +682,11 @@ pub enum DkgPayloadCreationError { FailedToGetDkgIntervalSettingFromRegistry(RegistryClientError), FailedToGetSubnetMemberListFromRegistry(RegistryClientError), FailedToGetVetKdKeyList(RegistryClientError), + SubnetSplittingStatusError(String), } /// Reasons for why a dkg payload might be invalid. +#[allow(clippy::large_enum_variant)] #[derive(PartialEq, Debug)] pub enum InvalidDkgPayloadReason { CryptoError(CryptoError), diff --git a/rs/types/types/src/exhaustive.rs b/rs/types/types/src/exhaustive.rs index b789c7b009a8..d5717e086681 100644 --- a/rs/types/types/src/exhaustive.rs +++ b/rs/types/types/src/exhaustive.rs @@ -2,6 +2,7 @@ use crate::artifact::IngressMessageId; use crate::batch::ChainKeyAgreement; +use crate::consensus::BackwardsCompatibleOption; use crate::consensus::hashed::Hashed; use crate::consensus::idkg::IDkgMasterPublicKeyId; use crate::consensus::idkg::common::{PreSignatureInCreation, PreSignatureRef}; @@ -233,6 +234,21 @@ impl ExhaustiveSet for String { } } +impl ExhaustiveSet for BackwardsCompatibleOption { + fn exhaustive_set(_rng: &mut R) -> Vec { + vec![Self::default()] + } +} + +impl ExhaustiveSet for BackwardsCompatibleOption { + fn exhaustive_set(rng: &mut R) -> Vec { + Option::::exhaustive_set(rng) + .into_iter() + .map(From::from) + .collect() + } +} + macro_rules! impl_for_integer { ($t: ty) => { impl ExhaustiveSet for $t { From 5b464584a47eee4177f9fef461a94da2ce92abfd Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Thu, 2 Apr 2026 09:16:40 +0000 Subject: [PATCH 04/84] batch delivery --- rs/consensus/src/consensus/batch_delivery.rs | 63 +++++++++-- rs/consensus/src/consensus/block_maker.rs | 4 +- rs/consensus/src/consensus/finalizer.rs | 1 + rs/consensus/src/consensus/status.rs | 14 ++- rs/types/types/src/consensus.rs | 104 +++++++++++-------- rs/types/types/src/consensus/dkg.rs | 12 ++- rs/types/types/src/exhaustive.rs | 2 +- 7 files changed, 136 insertions(+), 64 deletions(-) diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index e308b7ecd831..36c84f4992fb 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -26,13 +26,14 @@ use ic_protobuf::{ log::consensus_log_entry::v1::ConsensusLogEntry, registry::{crypto::v1::PublicKey as PublicKeyProto, subnet::v1::InitialNiDkgTranscriptRecord}, }; +use ic_registry_client_helpers::node::NodeRegistry; use ic_types::{ - Height, PrincipalId, SubnetId, + Height, NodeId, PrincipalId, SubnetId, batch::{ Batch, BatchContent, BatchMessages, BatchSummary, BlockmakerMetrics, ChainKeyData, ConsensusResponse, }, - consensus::{Block, BlockPayload, HasVersion, idkg}, + consensus::{Block, BlockPayload, HasVersion, dkg::SubnetSplittingStatus, idkg}, crypto::{ randomness_from_crypto_hashable, threshold_sig::{ @@ -65,6 +66,7 @@ pub fn deliver_batches( pool, registry_client, subnet_id, + /*maybe_node_id=*/ None, log, max_batch_height_to_deliver, /*result_processor=*/ None, @@ -81,6 +83,7 @@ pub(crate) fn deliver_batches_with_result_processor( pool: &PoolReader<'_>, registry_client: &dyn RegistryClient, subnet_id: SubnetId, + maybe_node_id: Option, log: &ReplicaLogger, // This argument should only be used by the ic-replay tool. If it is set to `None`, we will // deliver all batches until the finalized height. If it is set to `Some(h)`, we will @@ -223,12 +226,56 @@ pub(crate) fn deliver_batches_with_result_processor( let persist_batch = Some(height) == max_batch_height_to_deliver; let requires_full_state_hash = block.payload.is_summary() || persist_batch; let batch_content = match block.payload.as_ref() { - BlockPayload::Summary(_summary_payload) => BatchContent::Data { - batch_messages: BatchMessages::default(), - chain_key_data, - consensus_responses, - requires_full_state_hash, - }, + BlockPayload::Summary(summary_payload) => { + match summary_payload.dkg.subnet_splitting_status() { + SubnetSplittingStatus::Scheduled { + destination_subnet_id, + source_subnet_id, + } => { + let Ok(Some(subnet_id)) = registry_client + .get_subnet_id_from_node_id( + maybe_node_id + .expect("Subnet splitting not yet enabled in ic-replay"), + block.context.registry_version, + ) + .inspect_err(|err| { + error!( + every_n_seconds => 30, + log, + "Failed to determine the new subnet assignment: {err:?}" + ) + }) + else { + break; + }; + + let (new_subnet_id, other_subnet_id) = if subnet_id == destination_subnet_id + { + (destination_subnet_id, source_subnet_id) + } else { + (source_subnet_id, destination_subnet_id) + }; + + info!( + log, + "Deliverying splitting block. New subnet assignment: {new_subnet_id}" + ); + + BatchContent::Splitting { + new_subnet_id, + other_subnet_id, + } + } + SubnetSplittingStatus::Done { .. } | SubnetSplittingStatus::NotScheduled => { + BatchContent::Data { + batch_messages: BatchMessages::default(), + chain_key_data, + consensus_responses, + requires_full_state_hash, + } + } + } + } BlockPayload::Data(data_payload) => { batch_stats.add_from_payload(&data_payload.batch); BatchContent::Data { diff --git a/rs/consensus/src/consensus/block_maker.rs b/rs/consensus/src/consensus/block_maker.rs index 19209bb258f8..666dd533a7e6 100755 --- a/rs/consensus/src/consensus/block_maker.rs +++ b/rs/consensus/src/consensus/block_maker.rs @@ -349,8 +349,8 @@ impl BlockMaker { .flatten(); if matches!( - summary.subnet_splitting_status.as_ref(), - Some(&SubnetSplittingStatus::Scheduled { .. }) + summary.subnet_splitting_status(), + SubnetSplittingStatus::Scheduled { .. } ) { info!(self.log, "Proposing a Splitting block at height {height}."); } diff --git a/rs/consensus/src/consensus/finalizer.rs b/rs/consensus/src/consensus/finalizer.rs index f69cc144b7e3..4f6e3d0e701a 100644 --- a/rs/consensus/src/consensus/finalizer.rs +++ b/rs/consensus/src/consensus/finalizer.rs @@ -101,6 +101,7 @@ impl Finalizer { pool, &*self.registry_client, self.replica_config.subnet_id, + Some(self.replica_config.node_id), &self.log, None, Some(&|result, block_stats, batch_stats| { diff --git a/rs/consensus/src/consensus/status.rs b/rs/consensus/src/consensus/status.rs index 37eb6fa6977d..6dbd94cb271e 100644 --- a/rs/consensus/src/consensus/status.rs +++ b/rs/consensus/src/consensus/status.rs @@ -95,14 +95,12 @@ pub(crate) fn should_halt( .as_ref() .as_summary() .dkg - .subnet_splitting_status - .as_ref() + .subnet_splitting_status() { - Some(&SubnetSplittingStatus::NotScheduled) => false, + SubnetSplittingStatus::NotScheduled => false, // After the split, don't produce any blocks until we are on the right subnet. - Some(&SubnetSplittingStatus::Done { new_subnet_id }) => subnet_id != new_subnet_id, - Some(&SubnetSplittingStatus::Scheduled { .. }) => height >= summary_block.height, - None => false, + SubnetSplittingStatus::Done { new_subnet_id } => subnet_id != new_subnet_id, + SubnetSplittingStatus::Scheduled { .. } => height >= summary_block.height, } }) .unwrap_or_default(); @@ -318,7 +316,7 @@ mod tests { fn status_test(#[case] test_case: TestCase) { with_test_replica_logger(|logger| { ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { - use ic_types::consensus::BackwardsCompatibleOption; + use ic_types::consensus::backwards_compatibility::BackwardsCompatibleOption; let (pool, registry_client) = set_up( pool_config, @@ -331,7 +329,7 @@ mod tests { 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 = - BackwardsCompatibleOption(test_case.subnet_splitting_status); + BackwardsCompatibleOption::new_for_test_only(test_case.subnet_splitting_status); last_summary_block.payload = Payload::new(crypto_hash, BlockPayload::Summary(payload)); diff --git a/rs/types/types/src/consensus.rs b/rs/types/types/src/consensus.rs index fb405402f667..167bcd152550 100644 --- a/rs/types/types/src/consensus.rs +++ b/rs/types/types/src/consensus.rs @@ -1817,64 +1817,80 @@ impl ConsensusMessageHashable for ConsensusMessage { } } -#[derive(Clone, Eq, PartialEq, Debug, Deserialize, Serialize)] -/// A helper struct used for introducing new fields to structs which need to be backwards compatible. -/// The main difference w.r.t. [`Option`] is that its [`Hash`] implementation ignores the `None` -/// variant, which means that the hash of the struct without the field would be the same as the hash -/// of the struct with the field present but set to `None`. -/// -/// Lifecycle of adding a new field to a struct in a backwards compatible way: -/// 1. Add a new field to the struct with type `BackwardsCompatibleOption`. At this point -/// the field is *NOT* allowed to have any value other than `None`. -/// 2. When the change is deployed to all replicas, we can switch the type to -/// `BackwardsCompatibleOption` and the field can begin to be populated. -/// 3. When the change is deployed to all replicas, we can replace the type with `T`. -pub struct BackwardsCompatibleOption(pub Option); +pub mod backwards_compatibility { + use super::*; + + #[derive(Clone, Eq, PartialEq, Debug, Deserialize, Serialize)] + /// A helper struct used for introducing new fields to structs which need to be backwards compatible. + /// The main difference w.r.t. [`Option`] is that its [`Hash`] implementation ignores the `None` + /// variant, which means that the hash of the struct without the field would be the same as the hash + /// of the struct with the field present but set to `None`. + /// + /// Lifecycle of adding a new field to a struct in a backwards compatible way: + /// 1. Add a new field to the struct with type `BackwardsCompatibleOption`. At this point + /// the field is *NOT* allowed to have any value other than `None`. + /// 2. When the change is deployed to all replicas, we can switch the type to + /// `BackwardsCompatibleOption` and the field can begin to be populated. + /// 3. When the change is deployed to all replicas, we can replace the type with `T`. + pub struct BackwardsCompatibleOption(Option); + + impl Default for BackwardsCompatibleOption { + fn default() -> Self { + Self(None) + } + } -impl Default for BackwardsCompatibleOption { - fn default() -> Self { - Self(None) + impl Default for BackwardsCompatibleOption { + fn default() -> Self { + Self(Some(T::default())) + } } -} -impl Default for BackwardsCompatibleOption { - fn default() -> Self { - Self(Some(T::default())) + impl Hash for BackwardsCompatibleOption { + fn hash(&self, state: &mut H) { + if let Some(value) = &self.0 { + value.hash(state); + } + } } -} -impl Hash for BackwardsCompatibleOption { - fn hash(&self, state: &mut H) { - if let Some(value) = &self.0 { - value.hash(state); + impl From> for BackwardsCompatibleOption { + fn from(value: Option) -> Self { + Self(value) } } -} -impl From> for BackwardsCompatibleOption { - fn from(value: Option) -> Self { - Self(value) + impl From for BackwardsCompatibleOption { + fn from(value: T) -> Self { + Self(Some(value)) + } } -} -impl From for BackwardsCompatibleOption { - fn from(value: T) -> Self { - Self(Some(value)) + impl BackwardsCompatibleOption { + pub const fn new_for_test_only(value: Option) -> Self { + Self(value) + } } -} -impl BackwardsCompatibleOption { - pub const fn as_ref(&self) -> Option<&T> { - self.0.as_ref() + impl BackwardsCompatibleOption { + pub const fn new(value: Option) -> Self { + Self(value) + } } - pub fn try_from_proto>( - proto: Option, - ) -> Result { - let Some(value) = proto else { - return Ok(Self(None)); - }; + impl BackwardsCompatibleOption { + pub const fn as_ref(&self) -> Option<&T> { + self.0.as_ref() + } - Ok(Self(Some(value.try_into()?))) + pub fn try_from_proto>( + proto: Option, + ) -> Result { + let Some(value) = proto else { + return Ok(Self(None)); + }; + + Ok(Self(Some(value.try_into()?))) + } } } diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index c86efe96bb98..ced40c053efa 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -4,6 +4,7 @@ use super::*; use crate::{ ReplicaVersion, artifact::PbArtifact, + consensus::backwards_compatibility::BackwardsCompatibleOption, crypto::threshold_sig::ni_dkg::{ NiDkgDealing, NiDkgId, NiDkgTag, NiDkgTargetId, NiDkgTranscript, config::NiDkgConfig, @@ -231,7 +232,9 @@ impl DkgSummary { next_interval_length, height, initial_dkg_attempts, - subnet_splitting_status: BackwardsCompatibleOption(subnet_splitting_status), + subnet_splitting_status: BackwardsCompatibleOption::new_for_test_only( + subnet_splitting_status, + ), } } @@ -314,6 +317,13 @@ impl DkgSummary { .min() .expect("No current transcripts available") } + + pub fn subnet_splitting_status(&self) -> SubnetSplittingStatus { + self.subnet_splitting_status + .as_ref() + .copied() + .unwrap_or_default() + } } fn build_transcripts_vec( diff --git a/rs/types/types/src/exhaustive.rs b/rs/types/types/src/exhaustive.rs index d5717e086681..522655aa1a4d 100644 --- a/rs/types/types/src/exhaustive.rs +++ b/rs/types/types/src/exhaustive.rs @@ -2,7 +2,7 @@ use crate::artifact::IngressMessageId; use crate::batch::ChainKeyAgreement; -use crate::consensus::BackwardsCompatibleOption; +use crate::consensus::backwards_compatibility::BackwardsCompatibleOption; use crate::consensus::hashed::Hashed; use crate::consensus::idkg::IDkgMasterPublicKeyId; use crate::consensus::idkg::common::{PreSignatureInCreation, PreSignatureRef}; From 1d710218e8c3df36c990e9587597d8120d3ed914 Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Thu, 2 Apr 2026 11:24:54 +0000 Subject: [PATCH 05/84] Update pool_reader.rs --- Cargo.lock | 1 + rs/consensus/BUILD.bazel | 1 + rs/consensus/Cargo.toml | 1 + rs/consensus/dkg/src/dkg_key_manager.rs | 44 +- rs/consensus/dkg/src/lib.rs | 79 ++- rs/consensus/dkg/src/payload_builder.rs | 52 +- rs/consensus/dkg/src/payload_validator.rs | 3 + rs/consensus/mocks/src/lib.rs | 295 +++++--- rs/consensus/src/consensus.rs | 9 +- rs/consensus/src/consensus/batch_delivery.rs | 48 +- .../src/consensus/catchup_package_maker.rs | 659 +++++++++++++++--- rs/consensus/src/consensus/priority.rs | 11 +- .../src/consensus/share_aggregator.rs | 355 +++++++++- rs/consensus/src/consensus/validator.rs | 337 ++++++++- rs/consensus/tests/framework/runner.rs | 2 + rs/consensus/tests/payload.rs | 2 + rs/consensus/utils/src/pool_reader.rs | 9 + rs/consensus/utils/src/subnet_splitting.rs | 70 +- rs/replay/src/validator.rs | 2 + rs/replica/setup_ic_network/src/lib.rs | 2 + 20 files changed, 1681 insertions(+), 301 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1b5b2a17ea47..e6cb1db153e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7819,6 +7819,7 @@ version = "0.9.0" dependencies = [ "assert_matches", "criterion", + "hex", "ic-artifact-pool", "ic-btc-replica-types", "ic-config", diff --git a/rs/consensus/BUILD.bazel b/rs/consensus/BUILD.bazel index bb2430c8b3c5..97605eeb9c4f 100644 --- a/rs/consensus/BUILD.bazel +++ b/rs/consensus/BUILD.bazel @@ -67,6 +67,7 @@ DEV_DEPENDENCIES = [ "//rs/types/types_test_utils", "@crate_index//:assert_matches", "@crate_index//:criterion", + "@crate_index//:hex", "@crate_index//:mockall", "@crate_index//:proptest", "@crate_index//:prost", diff --git a/rs/consensus/Cargo.toml b/rs/consensus/Cargo.toml index 2a7ebb8535be..cee148b191e1 100644 --- a/rs/consensus/Cargo.toml +++ b/rs/consensus/Cargo.toml @@ -40,6 +40,7 @@ strum_macros = { workspace = true } [dev-dependencies] assert_matches = { workspace = true } criterion = { workspace = true } +hex = { workspace = true } ic-artifact-pool = { path = "../artifact_pool" } ic-btc-replica-types = { path = "../bitcoin/replica_types" } ic-config = { path = "../config" } diff --git a/rs/consensus/dkg/src/dkg_key_manager.rs b/rs/consensus/dkg/src/dkg_key_manager.rs index 3ef3b75f6c69..e662f1f17f7c 100644 --- a/rs/consensus/dkg/src/dkg_key_manager.rs +++ b/rs/consensus/dkg/src/dkg_key_manager.rs @@ -3,8 +3,13 @@ //! there is something to do. On high-level, it's responsible of spawning //! threads triggering long-running CSP operation and book-keeping of //! thread-handles. -use ic_consensus_utils::{crypto::ConsensusCrypto, pool_reader::PoolReader}; +use ic_consensus_utils::{ + crypto::ConsensusCrypto, + pool_reader::PoolReader, + subnet_splitting::{self, PostSplitAssignment}, +}; use ic_interfaces::crypto::{ErrorReproducibility, LoadTranscriptResult, NiDkgAlgorithm}; +use ic_interfaces_registry::RegistryClient; use ic_logger::{ReplicaLogger, error, info, warn}; use ic_metrics::{MetricsRegistry, buckets::decimal_buckets}; use ic_types::{ @@ -14,6 +19,7 @@ use ic_types::{ NiDkgId, NiDkgTag, NiDkgTargetSubnet, NiDkgTranscript, errors::load_transcript_error::DkgLoadTranscriptError, }, + replica_config::ReplicaConfig, }; use prometheus::{HistogramVec, IntCounterVec, IntGauge, IntGaugeVec}; use std::{ @@ -25,6 +31,8 @@ use std::{ time::Instant, }; +use crate::payload_builder::get_post_split_dkg_summary; + struct Metrics { pub dkg_ops_duration: HistogramVec, pub dkg_instance_id: IntGaugeVec, @@ -94,6 +102,8 @@ pub struct DkgKeyManager { >, // This is a thread handle used to keep track of asynchronous key removals. pending_key_removal: Option>, + registry: Arc, + replica_config: ReplicaConfig, } impl DkgKeyManager { @@ -103,6 +113,8 @@ impl DkgKeyManager { crypto: Arc, logger: ReplicaLogger, pool_reader: &PoolReader<'_>, + registry: Arc, + replica_config: ReplicaConfig, ) -> Self { let mut manager = Self { crypto, @@ -112,6 +124,8 @@ impl DkgKeyManager { last_cup_height: Default::default(), pending_transcript_loads: Default::default(), pending_key_removal: Default::default(), + registry, + replica_config, }; // By calling on state change during initialization, we make sure, that the key store is @@ -232,6 +246,25 @@ impl DkgKeyManager { // next transcript key irrelevant and remove it). self.delete_inactive_keys(pool_reader); self.load_transcripts_from_summary(&summary.dkg); + + if let Ok(PostSplitAssignment { + new_subnet_id, + other_subnet_id: _, + }) = subnet_splitting::get_post_split_subnet_assignment( + self.replica_config.node_id, + &summary_block, + self.registry.as_ref(), + ) { + let next_summary = get_post_split_dkg_summary( + new_subnet_id, + self.registry.as_ref(), + &summary_block, + ) + .expect("FIXME"); + info!(self.logger, "Adding post split dkg transcripts"); + self.load_transcripts_from_summary(&next_summary); + } + self.last_dkg_summary_height = Some(summary_block.height); } } @@ -570,7 +603,12 @@ mod tests { with_test_replica_logger(|logger| { let nodes: Vec<_> = (0..1).map(node_test_id).collect(); let dkg_interval_len = 3; - let Dependencies { mut pool, .. } = dependencies_with_subnet_params( + let Dependencies { + mut pool, + registry, + replica_config, + .. + } = dependencies_with_subnet_params( pool_config, subnet_test_id(222), vec![( @@ -586,6 +624,8 @@ mod tests { csp.clone(), logger, &PoolReader::new(&pool), + registry, + replica_config, ); // Emulate the first invocation of the dkg key manager and make sure all diff --git a/rs/consensus/dkg/src/lib.rs b/rs/consensus/dkg/src/lib.rs index 0188068a62f9..94a1b85e75ad 100644 --- a/rs/consensus/dkg/src/lib.rs +++ b/rs/consensus/dkg/src/lib.rs @@ -423,6 +423,7 @@ mod tests { crypto::threshold_sig::ni_dkg::{ NiDkgId, NiDkgMasterPublicKeyId, NiDkgTargetId, NiDkgTargetSubnet, }, + replica_config::ReplicaConfig, time::UNIX_EPOCH, }; use std::{collections::BTreeSet, convert::TryFrom}; @@ -442,6 +443,8 @@ mod tests { crypto, mut pool, dkg_pool, + replica_config, + registry, .. } = dependencies_with_subnet_params( pool_config, @@ -457,8 +460,13 @@ mod tests { // Now we instantiate the DKG component for node Id = 1, who is a dealer. let replica_1 = node_test_id(1); - let dkg_key_manager = - new_dkg_key_manager(crypto.clone(), logger.clone(), &PoolReader::new(&pool)); + let dkg_key_manager = new_dkg_key_manager( + crypto.clone(), + logger.clone(), + &PoolReader::new(&pool), + registry.clone(), + replica_config.clone(), + ); let dkg = DkgImpl::new( replica_1, crypto.clone(), @@ -524,8 +532,13 @@ mod tests { // Create another dealer and add his dealings into the unvalidated pool of // replica 1. let replica_2 = node_test_id(2); - let dkg_key_manager_2 = - new_dkg_key_manager(crypto.clone(), logger.clone(), &PoolReader::new(&pool)); + let dkg_key_manager_2 = new_dkg_key_manager( + crypto.clone(), + logger.clone(), + &PoolReader::new(&pool), + registry.clone(), + replica_config.clone(), + ); let dkg_2 = DkgImpl::new( replica_2, crypto, @@ -591,12 +604,21 @@ mod tests { ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { with_test_replica_logger(|logger| { let Dependencies { - mut pool, crypto, .. + mut pool, + crypto, + registry, + replica_config, + .. } = dependencies(pool_config.clone(), 2); let mut dkg_pool = DkgPoolImpl::new(MetricsRegistry::new(), logger.clone()); // Let's check that replica 3, who's not a dealer, does not produce dealings. - let dkg_key_manager = - new_dkg_key_manager(crypto.clone(), logger.clone(), &PoolReader::new(&pool)); + let dkg_key_manager = new_dkg_key_manager( + crypto.clone(), + logger.clone(), + &PoolReader::new(&pool), + registry.clone(), + replica_config.clone(), + ); let dkg = DkgImpl::new( node_test_id(3), crypto.clone(), @@ -608,8 +630,13 @@ mod tests { assert!(dkg.on_state_change(&dkg_pool).is_empty()); // Now we instantiate the DKG component for node Id = 1, who is a dealer. - let dkg_key_manager = - new_dkg_key_manager(crypto.clone(), logger.clone(), &PoolReader::new(&pool)); + let dkg_key_manager = new_dkg_key_manager( + crypto.clone(), + logger.clone(), + &PoolReader::new(&pool), + registry, + replica_config, + ); let dkg = DkgImpl::new( node_test_id(1), crypto, @@ -679,6 +706,7 @@ mod tests { mut pool, crypto, registry, + replica_config, state_manager, .. } = dependencies_with_subnet_records_with_raw_state_manager( @@ -702,8 +730,13 @@ mod tests { ); // Now we instantiate the DKG component for node Id = 1, who is a dealer. - let dkg_key_manager = - new_dkg_key_manager(crypto.clone(), logger.clone(), &PoolReader::new(&pool)); + let dkg_key_manager = new_dkg_key_manager( + crypto.clone(), + logger.clone(), + &PoolReader::new(&pool), + registry.clone(), + replica_config, + ); let dkg = DkgImpl::new( node_test_id(1), crypto, @@ -866,8 +899,10 @@ mod tests { let node_id_1 = node_test_id(1); // This is not a dealer! let node_id_2 = node_test_id(0); - let consensus_pool_1 = dependencies(pool_config_1, 2).pool; - let consensus_pool_2 = dependencies(pool_config_2, 2).pool; + let dependencies_1 = dependencies(pool_config_1, 2); + let dependencies_2 = dependencies(pool_config_2, 2); + let consensus_pool_1 = dependencies_1.pool; + let consensus_pool_2 = dependencies_2.pool; with_test_replica_logger(|logger| { let dkg_pool_1 = DkgPoolImpl::new(MetricsRegistry::new(), logger.clone()); @@ -878,6 +913,8 @@ mod tests { crypto.clone(), logger.clone(), &PoolReader::new(&consensus_pool_1), + dependencies_1.registry, + dependencies_1.replica_config, ); let dkg_1 = DkgImpl::new( node_id_1, @@ -892,6 +929,8 @@ mod tests { crypto.clone(), logger.clone(), &PoolReader::new(&consensus_pool_2), + dependencies_2.registry, + dependencies_2.replica_config, ); let dkg_2 = DkgImpl::new( node_id_2, @@ -1379,6 +1418,8 @@ mod tests { crypto_1.clone(), logger.clone(), &PoolReader::new(&pool_1), + dependencies_1.registry.clone(), + dependencies_1.replica_config.clone(), ); let dkg_1 = DkgImpl::new( node_test_id(1), @@ -1393,7 +1434,13 @@ mod tests { node_test_id(2), crypto_2.clone(), pool_2.get_cache(), - new_dkg_key_manager(crypto_2, logger.clone(), &PoolReader::new(&pool_2)), + new_dkg_key_manager( + crypto_2, + logger.clone(), + &PoolReader::new(&pool_2), + dependencies_2.registry.clone(), + dependencies_2.replica_config.clone(), + ), MetricsRegistry::new(), logger.clone(), ); @@ -2092,12 +2139,16 @@ mod tests { crypto: Arc, logger: ReplicaLogger, pool_reader: &PoolReader<'_>, + registry: Arc, + replica_config: ReplicaConfig, ) -> Arc> { Arc::new(Mutex::new(DkgKeyManager::new( MetricsRegistry::new(), crypto, logger, pool_reader, + registry, + replica_config, ))) } diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs index 88d8f164a301..7fce89b003bb 100644 --- a/rs/consensus/dkg/src/payload_builder.rs +++ b/rs/consensus/dkg/src/payload_builder.rs @@ -481,6 +481,22 @@ pub fn get_dkg_summary_from_cup_contents( subnet_id: SubnetId, registry: &dyn RegistryClient, registry_version: RegistryVersion, +) -> Result { + get_dkg_summary_from_cup_contents_with_subnet_splitting( + cup_contents, + subnet_id, + registry, + registry_version, + /*subnet_splitting_status=*/ None, + ) +} + +fn get_dkg_summary_from_cup_contents_with_subnet_splitting( + cup_contents: CatchUpPackageContents, + subnet_id: SubnetId, + registry: &dyn RegistryClient, + registry_version: RegistryVersion, + subnet_splitting_status: Option, ) -> Result { // If we're in a NNS subnet recovery case with failover nodes, we extract the registry of the // NNS we're recovering. @@ -583,7 +599,7 @@ pub fn get_dkg_summary_from_cup_contents( next_interval_length, height, BTreeMap::new(), // initial_dkg_attempts - None, + subnet_splitting_status, )) } @@ -1014,6 +1030,40 @@ fn create_remote_dkg_config( }) } +/// Creates a DKG summary for the summary block right after the subnet has been split. +pub fn get_post_split_dkg_summary( + new_subnet_id: SubnetId, + registry: &dyn RegistryClient, + last_summary_block: &Block, +) -> Result { + let last_summary = &last_summary_block.payload.as_ref().as_summary().dkg; + debug_assert!(matches!( + last_summary.subnet_splitting_status(), + SubnetSplittingStatus::Scheduled { .. } + )); + let registry_version = last_summary_block.context.registry_version; + + let mut cup_contents = registry + .get_cup_contents(new_subnet_id, registry_version) + .map_err(|err| { + format!("Failed to get the cup contents at registry version {registry_version}: {err}") + })? + .value + .ok_or_else(|| format!("Empty cup contents at registry version {registry_version}"))?; + + // Skip one dkg interval + cup_contents.height = last_summary.get_next_start_height().get(); + + get_dkg_summary_from_cup_contents_with_subnet_splitting( + cup_contents, + new_subnet_id, + registry, + registry_version, + Some(SubnetSplittingStatus::Done { new_subnet_id }), + ) + .map_err(|err| format!("Failed to create post-split dkg summary from contents: {err}")) +} + #[cfg(test)] mod tests { use crate::tests::test_vet_key_config; diff --git a/rs/consensus/dkg/src/payload_validator.rs b/rs/consensus/dkg/src/payload_validator.rs index 464e2108b503..ad30cbbe0ee5 100644 --- a/rs/consensus/dkg/src/payload_validator.rs +++ b/rs/consensus/dkg/src/payload_validator.rs @@ -228,6 +228,7 @@ mod tests { }, crypto::threshold_sig::ni_dkg::{NiDkgId, NiDkgTag, NiDkgTargetSubnet}, messages::CallbackId, + replica_config::ReplicaConfig, time::UNIX_EPOCH, }; use std::{ @@ -730,6 +731,8 @@ mod tests { crypto.clone(), no_op_logger(), &PoolReader::new(&pool), + registry.clone(), + ReplicaConfig { node_id, subnet_id }, ); let key_manager = Arc::new(Mutex::new(key_manager)); let dkg_impl = DkgImpl::new( diff --git a/rs/consensus/mocks/src/lib.rs b/rs/consensus/mocks/src/lib.rs index fedc734930af..f386409eb3df 100644 --- a/rs/consensus/mocks/src/lib.rs +++ b/rs/consensus/mocks/src/lib.rs @@ -17,7 +17,10 @@ use ic_registry_proto_data_provider::ProtoRegistryDataProvider; use ic_test_artifact_pool::consensus_pool::TestConsensusPool; use ic_test_utilities::state_manager::RefMockStateManager; use ic_test_utilities_consensus::IDkgStatsNoOp; -use ic_test_utilities_registry::{SubnetRecordBuilder, setup_registry_non_final}; +use ic_test_utilities_registry::{ + SubnetRecordBuilder, add_single_subnet_record, add_subnet_list_record, + insert_initial_dkg_transcript, +}; use ic_test_utilities_time::FastForwardTimeSource; use ic_test_utilities_types::ids::{node_test_id, subnet_test_id}; use ic_types::{ @@ -28,7 +31,10 @@ use ic_types::{ }; use mockall::predicate::*; use mockall::*; -use std::sync::{Arc, RwLock}; +use std::{ + collections::BTreeSet, + sync::{Arc, RwLock}, +}; mock! { pub PayloadBuilder {} @@ -105,6 +111,168 @@ pub struct Dependencies { pub canister_http_pool: Arc>, } +pub struct DependenciesBuilder { + pool_config: ArtifactPoolConfig, + records: Vec<(u64, SubnetId, SubnetRecord)>, + replica_config: ReplicaConfig, + mocked_state_manager: bool, + #[allow(clippy::type_complexity)] + additional_registry_mutations: Vec)>>, +} + +impl DependenciesBuilder { + pub fn new( + pool_config: ArtifactPoolConfig, + records: Vec<(u64, SubnetId, SubnetRecord)>, + ) -> Self { + Self { + pool_config, + replica_config: ReplicaConfig { + node_id: node_test_id(0), + subnet_id: records[0].1, + }, + records, + mocked_state_manager: false, + additional_registry_mutations: Vec::new(), + } + } + + pub fn with_replica_config(mut self, replica_config: ReplicaConfig) -> Self { + self.replica_config = replica_config; + + self + } + + pub fn with_mocked_state_manager(mut self) -> Self { + self.mocked_state_manager = true; + + self + } + + pub fn add_additional_registry_mutation( + mut self, + mutation: impl Fn(&Arc) + 'static, + ) -> Self { + self.additional_registry_mutations.push(Box::new(mutation)); + + self + } + + pub fn build(self) -> Dependencies { + let time_source = FastForwardTimeSource::new(); + let initial_registry_version = RegistryVersion::from(self.records[0].clone().0); + let registry_data_provider = Arc::new(ProtoRegistryDataProvider::new()); + assert!( + !self.records.is_empty(), + "Cannot setup a registry without records." + ); + let mut subnet_ids: BTreeSet = BTreeSet::default(); + let mut last_version = None; + + for (version, subnet_id, record) in self.records { + if let Some(last_version) = last_version + && last_version != version + { + add_subnet_list_record( + ®istry_data_provider, + last_version, + Vec::from_iter(subnet_ids.clone()), + ); + } + + if subnet_ids.insert(subnet_id) { + insert_initial_dkg_transcript(version, subnet_id, &record, ®istry_data_provider); + } + + add_single_subnet_record(®istry_data_provider, version, subnet_id, record); + + last_version = Some(version); + } + + if let Some(last_version) = last_version { + add_subnet_list_record( + ®istry_data_provider, + last_version, + Vec::from_iter(subnet_ids), + ); + } + + for registry_mutation in self.additional_registry_mutations { + registry_mutation(®istry_data_provider); + } + + let registry = Arc::new(FakeRegistryClient::new( + Arc::clone(®istry_data_provider) as Arc<_> + )); + + registry_data_provider + .add( + ROOT_SUBNET_ID_KEY, + initial_registry_version, + Some(ic_types::subnet_id_into_protobuf(subnet_test_id(0))), + ) + .unwrap(); + registry.update_to_latest_version(); + let crypto = Arc::new(CryptoReturningOk::default()); + let state_manager = Arc::new(RefMockStateManager::default()); + let log = ic_logger::replica_logger::no_op_logger(); + let dkg_pool = Arc::new(RwLock::new(DkgPoolImpl::new( + ic_metrics::MetricsRegistry::new(), + log.clone(), + ))); + let idkg_pool = Arc::new(RwLock::new(IDkgPoolImpl::new( + self.replica_config.node_id, + self.pool_config.clone(), + log.clone(), + ic_metrics::MetricsRegistry::new(), + Box::new(IDkgStatsNoOp {}), + ))); + let canister_http_pool = Arc::new(RwLock::new(CanisterHttpPoolImpl::new( + ic_metrics::MetricsRegistry::new(), + log, + ))); + let pool = TestConsensusPool::new( + self.replica_config.node_id, + self.replica_config.subnet_id, + self.pool_config, + time_source.clone(), + registry.clone(), + crypto.clone(), + state_manager.clone(), + Some(dkg_pool.clone()), + ); + let membership = Arc::new(Membership::new( + pool.get_cache(), + registry.clone(), + self.replica_config.subnet_id, + )); + + if self.mocked_state_manager { + state_manager + .get_mut() + .expect_get_state_at() + .return_const(Ok(ic_interfaces_state_manager::Labeled::new( + Height::new(0), + Arc::new(ic_test_utilities_state::get_initial_state(0, 0)), + ))); + } + + Dependencies { + crypto, + registry, + registry_data_provider, + membership, + time_source, + pool, + replica_config: self.replica_config, + state_manager, + dkg_pool, + idkg_pool, + canister_http_pool, + } + } +} + /// Creates most common consensus components used for testing. All components /// share the same mocked registry with the provided records, so they refer to /// the identical registry content at any time. The MockStateManager instance @@ -114,67 +282,14 @@ pub fn dependencies_with_subnet_records_with_raw_state_manager( subnet_id: SubnetId, records: Vec<(u64, SubnetRecord)>, ) -> Dependencies { - let time_source = FastForwardTimeSource::new(); - let registry_version = RegistryVersion::from(records[0].clone().0); - let (registry_data_provider, registry) = setup_registry_non_final(subnet_id, records); - registry_data_provider - .add( - ROOT_SUBNET_ID_KEY, - registry_version, - Some(ic_types::subnet_id_into_protobuf(subnet_test_id(0))), - ) - .unwrap(); - registry.update_to_latest_version(); - let replica_config = ReplicaConfig { - subnet_id, - node_id: node_test_id(0), - }; - let crypto = Arc::new(CryptoReturningOk::default()); - let state_manager = Arc::new(RefMockStateManager::default()); - let log = ic_logger::replica_logger::no_op_logger(); - let dkg_pool = Arc::new(RwLock::new(DkgPoolImpl::new( - ic_metrics::MetricsRegistry::new(), - log.clone(), - ))); - let idkg_pool = Arc::new(RwLock::new(IDkgPoolImpl::new( - replica_config.node_id, - pool_config.clone(), - log.clone(), - ic_metrics::MetricsRegistry::new(), - Box::new(IDkgStatsNoOp {}), - ))); - let canister_http_pool = Arc::new(RwLock::new(CanisterHttpPoolImpl::new( - ic_metrics::MetricsRegistry::new(), - log, - ))); - let pool = TestConsensusPool::new( - replica_config.node_id, - subnet_id, + DependenciesBuilder::new( pool_config, - time_source.clone(), - registry.clone(), - crypto.clone(), - state_manager.clone(), - Some(dkg_pool.clone()), - ); - let membership = Arc::new(Membership::new( - pool.get_cache(), - registry.clone(), - subnet_id, - )); - Dependencies { - crypto, - registry, - registry_data_provider, - membership, - time_source, - pool, - replica_config, - state_manager, - dkg_pool, - idkg_pool, - canister_http_pool, - } + records + .into_iter() + .map(|(version, record)| (version, subnet_id, record)) + .collect(), + ) + .build() } /// Creates most common consensus components used for testing. All components @@ -186,42 +301,15 @@ pub fn dependencies_with_subnet_params( subnet_id: SubnetId, records: Vec<(u64, SubnetRecord)>, ) -> Dependencies { - let Dependencies { - time_source, - registry_data_provider, - registry, - membership, - crypto, - pool, - replica_config, - state_manager, - dkg_pool, - idkg_pool, - canister_http_pool, - .. - } = dependencies_with_subnet_records_with_raw_state_manager(pool_config, subnet_id, records); - - state_manager - .get_mut() - .expect_get_state_at() - .return_const(Ok(ic_interfaces_state_manager::Labeled::new( - Height::new(0), - Arc::new(ic_test_utilities_state::get_initial_state(0, 0)), - ))); - - Dependencies { - crypto, - registry, - registry_data_provider, - membership, - time_source, - pool, - replica_config, - state_manager, - dkg_pool, - idkg_pool, - canister_http_pool, - } + DependenciesBuilder::new( + pool_config, + records + .into_iter() + .map(|(version, record)| (version, subnet_id, record)) + .collect(), + ) + .with_mocked_state_manager() + .build() } /// Creates most common consensus components used for testing. All components @@ -230,9 +318,14 @@ pub fn dependencies_with_subnet_params( /// their default values. pub fn dependencies(pool_config: ArtifactPoolConfig, nodes: u64) -> Dependencies { let committee = (0..nodes).map(node_test_id).collect::>(); - dependencies_with_subnet_params( + DependenciesBuilder::new( pool_config, - subnet_test_id(0), - vec![(1, SubnetRecordBuilder::from(&committee).build())], + vec![( + 1, + subnet_test_id(0), + SubnetRecordBuilder::from(&committee).build(), + )], ) + .with_mocked_state_manager() + .build() } diff --git a/rs/consensus/src/consensus.rs b/rs/consensus/src/consensus.rs index 240690e92abb..aca27f56ae3e 100644 --- a/rs/consensus/src/consensus.rs +++ b/rs/consensus/src/consensus.rs @@ -251,6 +251,7 @@ impl ConsensusImpl { crypto.clone(), state_manager.clone(), message_routing.clone(), + Arc::clone(®istry_client), logger.clone(), ), block_maker: BlockMaker::new( @@ -285,6 +286,8 @@ impl ConsensusImpl { membership, message_routing.clone(), crypto.clone(), + registry_client.clone(), + replica_config.clone(), logger.clone(), ), purger: Purger::new( @@ -694,8 +697,8 @@ mod tests { let metrics_registry = MetricsRegistry::new(); let consensus_impl = ConsensusImpl::new( - replica_config, - registry, + replica_config.clone(), + registry.clone(), pool.get_cache(), crypto.clone(), Arc::new(FakeIngressSelector::new()), @@ -711,6 +714,8 @@ mod tests { crypto, no_op_logger(), &PoolReader::new(&pool), + registry, + replica_config, ))), Arc::new(FakeMessageRouting::new()), state_manager, diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 36c84f4992fb..ff473c9420c7 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -12,7 +12,11 @@ use ic_consensus_idkg::utils::{ generate_responses_to_signature_request_contexts, get_idkg_subnet_public_keys_and_pre_signatures, }; -use ic_consensus_utils::{membership::Membership, pool_reader::PoolReader}; +use ic_consensus_utils::{ + membership::Membership, + pool_reader::PoolReader, + subnet_splitting::{self, PostSplitAssignment, PostSplitAssignmentError}, +}; use ic_error_types::RejectCode; use ic_https_outcalls_consensus::payload_builder::CanisterHttpPayloadBuilderImpl; use ic_interfaces::{ @@ -26,7 +30,6 @@ use ic_protobuf::{ log::consensus_log_entry::v1::ConsensusLogEntry, registry::{crypto::v1::PublicKey as PublicKeyProto, subnet::v1::InitialNiDkgTranscriptRecord}, }; -use ic_registry_client_helpers::node::NodeRegistry; use ic_types::{ Height, NodeId, PrincipalId, SubnetId, batch::{ @@ -228,32 +231,25 @@ pub(crate) fn deliver_batches_with_result_processor( let batch_content = match block.payload.as_ref() { BlockPayload::Summary(summary_payload) => { match summary_payload.dkg.subnet_splitting_status() { - SubnetSplittingStatus::Scheduled { - destination_subnet_id, - source_subnet_id, - } => { - let Ok(Some(subnet_id)) = registry_client - .get_subnet_id_from_node_id( - maybe_node_id - .expect("Subnet splitting not yet enabled in ic-replay"), - block.context.registry_version, - ) - .inspect_err(|err| { - error!( + SubnetSplittingStatus::Scheduled { .. } => { + let PostSplitAssignment { + new_subnet_id, + other_subnet_id, + } = match subnet_splitting::get_post_split_subnet_assignment( + maybe_node_id.expect("Subnet splitting not yet enabled in ic-replay"), + &block, + registry_client, + ) { + Ok(assignment) => assignment, + Err(PostSplitAssignmentError::NotSplitting) => unreachable!(), + Err(err) => { + warn!( every_n_seconds => 30, log, - "Failed to determine the new subnet assignment: {err:?}" - ) - }) - else { - break; - }; - - let (new_subnet_id, other_subnet_id) = if subnet_id == destination_subnet_id - { - (destination_subnet_id, source_subnet_id) - } else { - (source_subnet_id, destination_subnet_id) + "Error getting new subnet assignment: {err}" + ); + break; + } }; info!( diff --git a/rs/consensus/src/consensus/catchup_package_maker.rs b/rs/consensus/src/consensus/catchup_package_maker.rs index 4c5260e12d89..743c1fd1769a 100644 --- a/rs/consensus/src/consensus/catchup_package_maker.rs +++ b/rs/consensus/src/consensus/catchup_package_maker.rs @@ -13,35 +13,59 @@ //! At the moment, we will start to make a CatchUpPackage once a DKG summary //! block is considered finalized. +use ic_consensus_dkg::payload_builder::get_post_split_dkg_summary; use ic_consensus_utils::{ active_high_threshold_nidkg_id, crypto::ConsensusCrypto, get_oldest_idkg_state_registry_version, membership::Membership, pool_reader::PoolReader, }; use ic_interfaces::messaging::MessageRouting; +use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::{ PermanentStateHashError::*, StateHashError, StateManager, TransientStateHashError::*, }; -use ic_logger::{ReplicaLogger, debug, error, trace}; +use ic_logger::{ReplicaLogger, debug, error, info, trace, warn}; +use ic_registry_client_helpers::node::NodeRegistry; use ic_replicated_state::ReplicatedState; use ic_types::{ + Height, NodeId, SubnetId, + batch::ValidationContext, consensus::{ - Block, CatchUpContent, CatchUpPackage, CatchUpPackageShare, CatchUpShareContent, - HasCommittee, HasHeight, HashedBlock, HashedRandomBeacon, + Block, BlockPayload, CatchUpContent, CatchUpPackage, CatchUpPackageShare, + CatchUpShareContent, HasCommittee, HasHeight, HashedBlock, HashedRandomBeacon, Payload, + RandomBeacon, RandomBeaconContent, Rank, SummaryPayload, dkg::SubnetSplittingStatus, + }, + crypto::{ + CombinedThresholdSig, CombinedThresholdSigOf, CryptoHash, CryptoHashOf, Signed, + crypto_hash, + threshold_sig::ni_dkg::{NiDkgId, NiDkgTag, NiDkgTranscript}, }, replica_config::ReplicaConfig, + signature::ThresholdSignature, }; use std::sync::Arc; -/// CatchUpPackage maker is responsible for creating beacon shares +/// [`CatchUpPackage`] maker is responsible for creating beacon shares pub(crate) struct CatchUpPackageMaker { replica_config: ReplicaConfig, membership: Arc, crypto: Arc, state_manager: Arc>, message_routing: Arc, + registry: Arc, log: ReplicaLogger, } +/// Type of [`CatchUpPackage`]. +#[derive(Copy, Clone, Eq, PartialEq, Debug)] +pub(crate) enum CatchUpPackageType { + Normal, + /// After deliverying a splitting block to the DSM, we immediately create a CUP at the start of + /// the next dkg interval and we create a new summary block and a dummy random beacon on the fly. + PostSplit { + new_subnet_id: SubnetId, + }, +} + impl CatchUpPackageMaker { /// Instantiate a new CatchUpPackage maker and save a copy of the config. pub fn new( @@ -50,6 +74,7 @@ impl CatchUpPackageMaker { crypto: Arc, state_manager: Arc>, message_routing: Arc, + registry: Arc, log: ReplicaLogger, ) -> Self { Self { @@ -58,6 +83,7 @@ impl CatchUpPackageMaker { crypto, state_manager, message_routing, + registry, log, } } @@ -143,132 +169,399 @@ impl CatchUpPackageMaker { } /// Consider the provided block for the creation of a catch up package. - fn consider_block( + pub(crate) fn consider_block( &self, pool: &PoolReader<'_>, start_block: Block, ) -> Option { - let height = start_block.height(); - - // Skip if this node is not in the committee to make CUP shares - let my_node_id = self.replica_config.node_id; - if self.membership.node_belongs_to_threshold_committee( - my_node_id, - height, - CatchUpPackage::committee(), - ) != Ok(true) - { - return None; - } + let summary_height = start_block.height(); + let cup_type = get_catch_up_package_type( + self.registry.as_ref(), + self.replica_config.node_id, + &start_block, + ) + .inspect_err(|err| warn!(self.log, "Failed to get the catch up package type: {err}")) + .ok()?; - // Skip if this node has already made a share - if pool - .get_catch_up_package_shares(height) - .any(|share| share.signature.signer == my_node_id) - { - return None; - } - - // Skip if random beacon does not exist for the height - let random_beacon = pool.get_random_beacon(height)?; - - // Skip if the state referenced by finalization tip has not caught up to - // this height. This is to increase the chance that states are available to - // validate payloads at the chain tip. - if pool.get_finalized_tip().context.certified_height < height { - return None; + match cup_type { + CatchUpPackageType::Normal => { + // Skip if the state referenced by finalization tip has not caught up to + // this height. This is to increase the chance that states are available to + // validate payloads at the chain tip. + if pool.get_finalized_tip().context.certified_height < summary_height { + return None; + } + } + CatchUpPackageType::PostSplit { .. } => { + // During subnet splitting we don't need to wait for the state at the height to be + // certified + } } - match self.state_manager.get_state_hash_at(height) { + let state_hash = match self.state_manager.get_state_hash_at(summary_height) { + Ok(state_hash) => state_hash, Err(StateHashError::Transient(StateNotCommittedYet(_))) => { // TODO: Setup a delay before retry debug!( self.log, - "Cannot make CUP at height {} because state is not committed yet. Will retry", - height + "Cannot make CUP at height {} because \ + state is not committed yet. Will retry", + summary_height ); - None + return None; } Err(StateHashError::Transient(HashNotComputedYet(_))) => { debug!( self.log, - "Cannot make CUP at height {} because state hash is not computed yet. Will retry", - height + "Cannot make CUP at height {} because \ + state hash is not computed yet. Will retry", + summary_height ); - None + return None; } Err(StateHashError::Permanent(StateRemoved(_))) => { // This should never happen as we don't want to remove the state // for CUP before the hash is fetched. panic!( - "State at height {height} had disappeared before we had a chance to make a CUP. This should not happen.", + "State at height {summary_height} had disappeared before \ + we had a chance to make a CUP. \ + This should not happen.", ); } Err(StateHashError::Permanent(StateNotFullyCertified(_))) => { - panic!("Height {height} is not a fully certified height. This should not happen.",); + panic!( + "Height {summary_height} is not a fully certified height. \ + This should not happen.", + ); } - Ok(state_hash) => { - let summary = start_block.payload.as_ref().as_summary(); - let registry_version = if summary.idkg.is_some() { - // Should succeed as we already got the hash above - let state = self - .state_manager - .get_state_at(height) - .map_err(|err| { - error!( - self.log, - "Cannot make IDKG CUP at height {}: `get_state_hash_at` \ - succeeded but `get_state_at` failed with {}. Will retry", - height, - err, - ) - }) - .ok()?; - get_oldest_idkg_state_registry_version(state.get_ref()) - } else { - None - }; - let content = CatchUpContent::new( - HashedBlock::new(ic_types::crypto::crypto_hash, start_block), - HashedRandomBeacon::new(ic_types::crypto::crypto_hash, random_beacon), - state_hash, - registry_version, + }; + + let summary = start_block.payload.as_ref().as_summary(); + + let oldest_registry_version_in_use_by_replicated_state = if summary.idkg.is_some() { + // Should succeed as we already got the hash above + let state = self + .state_manager + .get_state_at(summary_height) + .inspect_err(|err| { + error!( + self.log, + "Cannot make IDKG CUP at height {summary_height}: `get_state_hash_at` \ + succeeded but `get_state_at` failed with {err}. Will retry", + ) + }) + .ok()?; + get_oldest_idkg_state_registry_version(state.get_ref()) + } else { + None + }; + + // Skip if this node has already made a share + if pool + .get_catch_up_package_shares(self.get_cup_height(&start_block, cup_type)) + .any(|share| share.signature.signer == self.replica_config.node_id) + { + return None; + } + + let cup_block = self + .get_cup_block(start_block.clone(), cup_type) + .inspect_err(|err| warn!(self.log, "Can't get a block for a CUP: {err}")) + .ok()?; + + let random_beacon = self + .get_cup_random_beacon(pool, &cup_block, cup_type) + .inspect_err(|err| warn!(self.log, "Can't get a random beacon for a CUP: {err}")) + .ok()?; + + let high_dkg_id = self + .get_high_dkg_id(pool, &cup_block, cup_type) + .inspect_err(|err| warn!(self.log, "Can't get a high dkg id for a CUP: {err}")) + .ok()?; + + if !self + .node_belongs_to_threshold_committee(&cup_block, cup_type) + .inspect_err(|err| warn!(self.log, "Can't check if node belongs to committee: {err}")) + .unwrap_or_default() + { + return None; + } + + let content = CatchUpContent::new( + HashedBlock::new(ic_types::crypto::crypto_hash, cup_block), + HashedRandomBeacon::new(ic_types::crypto::crypto_hash, random_beacon), + state_hash, + oldest_registry_version_in_use_by_replicated_state, + ); + + let share_content = CatchUpShareContent::from(&content); + let share_height = share_content.height(); + match self + .crypto + .sign(&content, self.replica_config.node_id, high_dkg_id) + { + Ok(signature) => { + info!( + self.log, + "Proposing a CatchUpPackageShare (type: {cup_type:?}) at height {share_height}" ); - let share_content = CatchUpShareContent::from(&content); - if let Some(dkg_id) = active_high_threshold_nidkg_id(pool.as_cache(), height) { - match self.crypto.sign(&content, my_node_id, dkg_id) { - Ok(signature) => { - // Caution: The log string below is checked in replica_determinism_test. - // Changing the string might break the test. - debug!( - self.log, - "Proposing a CatchUpPackageShare at height {}", height - ); - Some(CatchUpPackageShare { - content: share_content, - signature, - }) - } - Err(err) => { - error!(self.log, "Couldn't create a signature: {:?}", err); - None - } - } - } else { - error!(self.log, "Couldn't find transcript at height {}", height); - None + Some(CatchUpPackageShare { + content: share_content, + signature, + }) + } + Err(err) => { + error!( + self.log, + "Couldn't create a signature at height {share_height}: {err}" + ); + None + } + } + } + + fn get_cup_height(&self, summary_block: &Block, cup_type: CatchUpPackageType) -> Height { + match cup_type { + CatchUpPackageType::Normal => summary_block.height, + // During subnet splitting we skip one dkg interval + CatchUpPackageType::PostSplit { .. } => summary_block + .payload + .as_ref() + .as_summary() + .dkg + .get_next_start_height(), + } + } + + fn get_cup_block( + &self, + summary_block: Block, + cup_type: CatchUpPackageType, + ) -> Result { + match cup_type { + CatchUpPackageType::Normal => Ok(summary_block), + CatchUpPackageType::PostSplit { new_subnet_id } => create_post_split_summary_block( + &summary_block, + new_subnet_id, + self.registry.as_ref(), + ) + .map_err(|err| format!("Failed to create a post split block: {err}")), + } + } + + fn get_cup_random_beacon( + &self, + pool: &PoolReader<'_>, + cup_block: &Block, + cup_type: CatchUpPackageType, + ) -> Result { + match cup_type { + CatchUpPackageType::Normal => pool + .get_random_beacon(cup_block.height()) + .ok_or_else(|| format!("No random beacon found at height {}", cup_block.height())), + // During subnet splitting we create a dummy, unsigned random beacon, because at the + // height at which we are building a CUP, we won't have a random beacon. + CatchUpPackageType::PostSplit { .. } => create_post_split_random_beacon(cup_block), + } + } + + fn get_high_dkg_id( + &self, + pool: &PoolReader<'_>, + cup_block: &Block, + cup_type: CatchUpPackageType, + ) -> Result { + // TODO: can we always take the transcript from the block? + match cup_type { + CatchUpPackageType::Normal => { + active_high_threshold_nidkg_id(pool.as_cache(), cup_block.height).ok_or_else(|| { + format!("Couldn't find transcript at height {}", cup_block.height) + }) + } + CatchUpPackageType::PostSplit { .. } => { + match get_current_transcript_from_summary_block(cup_block, &NiDkgTag::HighThreshold) + { + Some(transcript) => Ok(transcript.dkg_id.clone()), + None => Err(format!( + "Couldn't find post-split transcript at height {}", + cup_block.height + )), + } + } + } + } + + fn node_belongs_to_threshold_committee( + &self, + cup_block: &Block, + cup_type: CatchUpPackageType, + ) -> Result { + // TODO: can we always take the transcript from the block? + match cup_type { + CatchUpPackageType::Normal => self + .membership + .node_belongs_to_threshold_committee( + self.replica_config.node_id, + cup_block.height, + CatchUpPackage::committee(), + ) + .map_err(|err| { + format!("Failed to check if node belongs to threshold committee {err:?}") + }), + CatchUpPackageType::PostSplit { .. } => { + match get_current_transcript_from_summary_block(cup_block, &NiDkgTag::HighThreshold) + { + Some(transcript) => Ok(transcript + .committee + .position(self.replica_config.node_id) + .is_some()), + None => Err(format!( + "Couldn't find post-split transcript at height {}", + cup_block.height + )), } } } } } +pub(crate) fn get_catch_up_package_type( + registry: &dyn RegistryClient, + node_id: NodeId, + summary_block: &Block, +) -> Result { + match summary_block + .payload + .as_ref() + .as_summary() + .dkg + .subnet_splitting_status() + { + SubnetSplittingStatus::Scheduled { + destination_subnet_id, + source_subnet_id, + } => { + let new_subnet_id = get_new_subnet_id( + registry, + summary_block, + node_id, + source_subnet_id, + destination_subnet_id, + ) + .map_err(|err| format!("Failed to get the new subnet assignment: {err}"))?; + + Ok(CatchUpPackageType::PostSplit { new_subnet_id }) + } + _ => Ok(CatchUpPackageType::Normal), + } +} + +/// Note: this panics if the given block is not a summary block. +fn get_current_transcript_from_summary_block<'a>( + summary_block: &'a Block, + tag: &NiDkgTag, +) -> Option<&'a NiDkgTranscript> { + summary_block + .payload + .as_ref() + .as_summary() + .dkg + .current_transcript(tag) +} + +pub(crate) fn create_post_split_summary_block( + splitting_summary_block: &Block, + subnet_id: SubnetId, + registry: &dyn RegistryClient, +) -> Result { + let post_split_dkg_summary = + get_post_split_dkg_summary(subnet_id, registry, splitting_summary_block) + .map_err(|err| format!("Failed to get post-split DKG summary: {err}"))?; + + let height = post_split_dkg_summary.height; + Ok(Block { + version: splitting_summary_block.version.clone(), + // Fake parent + parent: CryptoHashOf::from(CryptoHash(Vec::new())), + payload: Payload::new( + crypto_hash, + BlockPayload::Summary(SummaryPayload { + dkg: post_split_dkg_summary, + idkg: None, + }), + ), + height, + rank: Rank(0), + context: ValidationContext { + registry_version: splitting_summary_block.context.registry_version, + certified_height: height, + // time needs to be strictly increasing + time: splitting_summary_block.context.time + std::time::Duration::from_millis(1), + }, + }) +} + +// During subnet splitting we create a dummy, unsigned random beacon, because at the +// height at which we are building a CUP, we won't have a random beacon. +pub(crate) fn create_post_split_random_beacon(cup_block: &Block) -> Result { + match get_current_transcript_from_summary_block(cup_block, &NiDkgTag::LowThreshold) { + Some(transcript) => Ok(Signed { + content: RandomBeaconContent { + version: cup_block.version.clone(), + height: cup_block.height(), + parent: CryptoHashOf::from(CryptoHash(Vec::new())), + }, + signature: ThresholdSignature { + signer: transcript.dkg_id.clone(), + signature: CombinedThresholdSigOf::new(CombinedThresholdSig(vec![])), + }, + }), + None => Err(format!( + "Couldn't find post-split transcript at height {}", + cup_block.height(), + )), + } +} + +fn get_new_subnet_id( + registry: &dyn RegistryClient, + summary_block: &Block, + node_id: NodeId, + source_subnet_id: SubnetId, + destination_subnet_id: SubnetId, +) -> Result { + let registry_version = summary_block.context.registry_version; + let new_subnet_id = registry + .get_subnet_id_from_node_id(node_id, registry_version) + .map_err(|err| { + format!( + "Failed to get the new subnet id at \ + registry version {registry_version}: {err}" + ) + })? + .ok_or_else(|| { + format!( + "Node is not assigned to any subnet at \ + registry version {registry_version}" + ) + })?; + + if ![source_subnet_id, destination_subnet_id].contains(&new_subnet_id) { + return Err(format!( + "According to the registry version {registry_version} \ + the node belongs to neither source subnet nor the destination subnet" + )); + } + + Ok(new_subnet_id) +} + #[cfg(test)] mod tests { //! CatchUpPackageMaker unit tests use super::*; use ic_consensus_mocks::{ - Dependencies, dependencies_with_subnet_params, + Dependencies, DependenciesBuilder, dependencies_with_subnet_params, dependencies_with_subnet_records_with_raw_state_manager, }; use ic_logger::replica_logger::no_op_logger; @@ -277,14 +570,21 @@ mod tests { empty_idkg_payload, fake_ecdsa_idkg_master_public_key_id, fake_signature_request_context_with_registry_version, fake_state_with_signature_requests, }; - use ic_test_utilities_registry::SubnetRecordBuilder; + use ic_test_utilities_logger::with_test_replica_logger; + use ic_test_utilities_registry::{SubnetRecordBuilder, insert_initial_dkg_transcript}; use ic_test_utilities_types::ids::{node_test_id, subnet_test_id}; use ic_types::{ - CryptoHashOfState, Height, RegistryVersion, - consensus::{BlockPayload, Payload, SummaryPayload, idkg::PreSigId}, + CryptoHashOfState, Height, NodeId, RegistryVersion, + consensus::{ + BlockPayload, ConsensusMessageHashable, HasVersion, Payload, SummaryPayload, + idkg::PreSigId, + }, crypto::CryptoHash, messages::CallbackId, }; + use ic_types_test_utils::ids::{NODE_1, NODE_2, NODE_3, NODE_4}; + use ic_types_test_utils::ids::{SUBNET_1, SUBNET_2}; + use rstest::rstest; use std::sync::{Arc, RwLock}; #[test] @@ -296,6 +596,7 @@ mod tests { mut pool, membership, replica_config, + registry, crypto, state_manager, .. @@ -325,6 +626,7 @@ mod tests { crypto, state_manager.clone(), message_routing, + registry, no_op_logger(), ); @@ -375,6 +677,7 @@ mod tests { membership, replica_config, crypto, + registry, state_manager, .. } = dependencies_with_subnet_records_with_raw_state_manager( @@ -446,6 +749,7 @@ mod tests { crypto, state_manager.clone(), message_routing, + registry, no_op_logger(), ); @@ -501,6 +805,7 @@ mod tests { mut pool, membership, replica_config, + registry, crypto, state_manager, .. @@ -542,6 +847,7 @@ mod tests { crypto, state_manager, message_routing, + registry, no_op_logger(), ); @@ -562,6 +868,7 @@ mod tests { replica_config, crypto, state_manager, + registry, .. } = dependencies_with_subnet_params( pool_config, @@ -586,6 +893,7 @@ mod tests { crypto, state_manager.clone(), message_routing, + registry, no_op_logger(), ); @@ -631,4 +939,165 @@ mod tests { cup_maker.on_state_change(&PoolReader::new(&pool)); }) } + + #[rstest] + #[case::source_subnet_node( + NODE_1, + "8aa92d736af3d4b815de5f16f257e75f9a3977633db48dbc6f904406ea506650" + )] + #[case::source_subnet_node( + NODE_2, + "8aa92d736af3d4b815de5f16f257e75f9a3977633db48dbc6f904406ea506650" + )] + #[case::destination_subnet_node( + NODE_3, + "6ffa0e9003fa9585fa6d2fcb1081d6980cbc64c26a3d2e0177dc4740a41c099e" + )] + #[case::destination_subnet_node( + NODE_4, + "6ffa0e9003fa9585fa6d2fcb1081d6980cbc64c26a3d2e0177dc4740a41c099e" + )] + #[trace] + fn create_post_split_cup_share_test( + #[case] node_id: NodeId, + // We don't necessarily care what the hash is, but we want to ensure that different + // nodes produce different blocks (and hence different hashes), depending on which subnet + // they are going to land on + #[case] expected_block_hash_in_cup: &str, + #[values(Height::new(0), Height::new(1000))] context_certified_height: Height, + ) { + with_test_replica_logger(|log| { + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + use ic_types::consensus::backwards_compatibility::BackwardsCompatibleOption; + + const SOURCE_SUBNET_ID: SubnetId = SUBNET_1; + const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2; + const INITIAL_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(1); + const SPLITTING_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(2); + const INTERVAL_LENGTH: Height = Height::new(9); + let fake_state_hash = CryptoHashOfState::from(CryptoHash(vec![1, 2, 3])); + + let Dependencies { + mut pool, + membership, + registry, + crypto, + state_manager, + .. + } = DependenciesBuilder::new( + pool_config, + vec![ + ( + INITIAL_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_1, NODE_2, NODE_3, NODE_4]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ( + SPLITTING_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_1, NODE_2]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ( + SPLITTING_REGISTRY_VERSION.get(), + DESTINATION_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_3, NODE_4]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ], + ) + .add_additional_registry_mutation(|registry_data_provider| { + insert_initial_dkg_transcript( + SPLITTING_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + &SubnetRecordBuilder::from(&[NODE_1, NODE_2]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + registry_data_provider, + ) + }) + .with_replica_config(ReplicaConfig { + node_id, + subnet_id: SOURCE_SUBNET_ID, + }) + .with_mocked_state_manager() + .build(); + + state_manager + .get_mut() + .expect_get_state_hash_at() + .return_const(Ok(fake_state_hash.clone())); + + let message_routing = FakeMessageRouting::new(); + *message_routing.next_batch_height.write().unwrap() = Height::from(2); + let message_routing = Arc::new(message_routing); + + let cup_maker = CatchUpPackageMaker::new( + ReplicaConfig { + node_id, + subnet_id: SOURCE_SUBNET_ID, + }, + membership, + crypto, + state_manager, + message_routing, + registry, + log, + ); + + pool.advance_round_normal_operation_n(INTERVAL_LENGTH.get()); + + let subnet_splitting_status = SubnetSplittingStatus::Scheduled { + source_subnet_id: SOURCE_SUBNET_ID, + destination_subnet_id: DESTINATION_SUBNET_ID, + }; + let mut proposal = pool.make_next_block(); + let block = proposal.content.as_mut(); + block.context.certified_height = context_certified_height; + block.context.registry_version = SPLITTING_REGISTRY_VERSION; + let mut payload = block.payload.as_ref().as_summary().clone(); + payload.dkg.subnet_splitting_status = + BackwardsCompatibleOption::new_for_test_only(Some(subnet_splitting_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.insert_validated(proposal.clone()); + pool.notarize(&proposal); + pool.finalize(&proposal); + + let share = cup_maker + .consider_block(&PoolReader::new(&pool), proposal.content.as_ref().clone()) + .expect("Should succeed with valid inputs"); + + assert!(share.check_integrity()); + assert_eq!(share.content.version, *proposal.content.version()); + assert_eq!( + hex::encode(&share.content.block.get().0), + expected_block_hash_in_cup + ); + assert_eq!( + share.content.random_beacon.get_value().content.height, + proposal.content.height() + INTERVAL_LENGTH + Height::new(1), + ); + assert_eq!( + share.content.random_beacon.get_value().content.version, + *proposal.content.version(), + ); + assert_eq!(share.content.state_hash, fake_state_hash); + assert_eq!( + share + .content + .oldest_registry_version_in_use_by_replicated_state, + None + ); + assert_eq!(share.signature.signer, node_id); + }) + }) + } } diff --git a/rs/consensus/src/consensus/priority.rs b/rs/consensus/src/consensus/priority.rs index c467ecd31afa..1f15e0b7f7f9 100644 --- a/rs/consensus/src/consensus/priority.rs +++ b/rs/consensus/src/consensus/priority.rs @@ -18,6 +18,7 @@ pub fn new_bouncer( let finalized_height = pool_reader.get_finalized_height(); let notarized_height = pool_reader.get_notarized_height(); let beacon_height = pool_reader.get_random_beacon_height(); + let next_summary_height = pool_reader.get_next_summary_height(); Box::new(move |id: &'_ ConsensusMessageId| { compute_bouncer( @@ -27,6 +28,7 @@ pub fn new_bouncer( finalized_height, notarized_height, beacon_height, + next_summary_height, id, ) }) @@ -44,6 +46,7 @@ fn compute_bouncer( finalized_height: Height, notarized_height: Height, beacon_height: Height, + next_summary_height: Height, id: &ConsensusMessageId, ) -> BouncerValue { let height = id.height; @@ -53,8 +56,10 @@ fn compute_bouncer( } // Stash non-CUP artifacts, as long as they're too far ahead of the next pending CUP height. // This prevents nodes that have fallen behind from exceeding their validated pool bounds. - if !matches!(id.hash, ConsensusMessageHash::CatchUpPackage(_)) - && height > next_cup_height + Height::new(ACCEPTABLE_NOTARIZATION_CUP_GAP) + if !matches!( + id.hash, + ConsensusMessageHash::CatchUpPackage(_) | ConsensusMessageHash::CatchUpPackageShare(_) + ) && height > next_cup_height + Height::new(ACCEPTABLE_NOTARIZATION_CUP_GAP) { return MaybeWantsLater; } @@ -107,7 +112,7 @@ fn compute_bouncer( ConsensusMessageHash::CatchUpPackageShare(_) => { if height <= cup_height { Unwanted - } else if height <= finalized_height { + } else if height <= next_summary_height { Wants } else { MaybeWantsLater diff --git a/rs/consensus/src/consensus/share_aggregator.rs b/rs/consensus/src/consensus/share_aggregator.rs index 3c26940ed4a5..6696e84d4ce0 100644 --- a/rs/consensus/src/consensus/share_aggregator.rs +++ b/rs/consensus/src/consensus/share_aggregator.rs @@ -2,30 +2,40 @@ //! of shares into full objects. That is, it constructs Random Beacon objects //! from random beacon shares, Notarizations from notarization shares and //! Finalizations from finalization shares. -use crate::consensus::random_tape_maker::RANDOM_TAPE_CHECK_MAX_HEIGHT_RANGE; +use crate::consensus::{ + catchup_package_maker::CatchUpPackageType, + random_tape_maker::RANDOM_TAPE_CHECK_MAX_HEIGHT_RANGE, +}; use ic_consensus_utils::{ active_high_threshold_nidkg_id, active_low_threshold_nidkg_id, aggregate, crypto::ConsensusCrypto, membership::Membership, pool_reader::PoolReader, registry_version_at_height, }; use ic_interfaces::messaging::MessageRouting; -use ic_logger::ReplicaLogger; +use ic_interfaces_registry::RegistryClient; +use ic_logger::{ReplicaLogger, debug, info, warn}; use ic_types::{ Height, consensus::{ - CatchUpContent, ConsensusMessage, ConsensusMessageHashable, FinalizationContent, HasHeight, - RandomTapeContent, + Block, CatchUpContent, CatchUpPackage, ConsensusMessage, ConsensusMessageHashable, + FinalizationContent, HasCommittee, HasHeight, RandomTapeContent, + dkg::SubnetSplittingStatus, }, - crypto::Signed, + crypto::threshold_sig::ni_dkg::NiDkgTag, + replica_config::ReplicaConfig, }; use std::{cmp::min, sync::Arc}; +use super::catchup_package_maker; + /// The ShareAggregator is responsible for aggregating shares of random beacons, /// notarizations, and finalizations into full objects pub(crate) struct ShareAggregator { membership: Arc, crypto: Arc, message_routing: Arc, + registry: Arc, + replica_config: ReplicaConfig, log: ReplicaLogger, } @@ -34,12 +44,16 @@ impl ShareAggregator { membership: Arc, message_routing: Arc, crypto: Arc, + registry: Arc, + replica_config: ReplicaConfig, log: ReplicaLogger, ) -> ShareAggregator { ShareAggregator { membership, crypto, message_routing, + registry, + replica_config, log, } } @@ -53,6 +67,7 @@ impl ShareAggregator { messages.append(&mut self.aggregate_notarization_shares(pool)); messages.append(&mut self.aggregate_finalization_shares(pool)); messages.append(&mut self.aggregate_catch_up_package_shares(pool)); + messages } @@ -135,27 +150,24 @@ impl ShareAggregator { let current_cup_height = pool.get_catch_up_height(); while start_block.height() > current_cup_height { - let height = start_block.height(); - let shares = pool.get_catch_up_package_shares(height).map(|share| { - let block = pool - .get_block(&share.content.block, height) - .unwrap_or_else(|| panic!("Block not found for {share:?}")); - Signed { - content: CatchUpContent::from_share_content(share.content, block.into_inner()), - signature: share.signature, + match self.aggregate_catch_up_package_shares_for_summary_block(pool, &start_block) { + Ok(Some(cup)) => { + return vec![ConsensusMessage::CatchUpPackage(cup)]; + } + Ok(None) => { + debug!( + self.log, + "Not enough shares to be able to create a full CUP at height{}", + start_block.height() + ); + } + Err(err) => { + warn!( + self.log, + "Encountered an error while aggregating CUP shares at height {}: {err}", + start_block.height() + ); } - }); - let state_reader = pool.as_cache(); - let dkg_id = active_high_threshold_nidkg_id(state_reader, height); - let result = aggregate( - &self.log, - self.membership.as_ref(), - self.crypto.as_aggregate(), - Box::new(|_| dkg_id.clone()), - shares, - ); - if !result.is_empty() { - return to_messages(result); } let Some(block_from_last_interval) = @@ -177,6 +189,97 @@ impl ShareAggregator { } Vec::new() } + + fn aggregate_catch_up_package_shares_for_summary_block( + &self, + pool: &PoolReader<'_>, + summary_block: &Block, + ) -> Result, String> { + let (threshold, dkg_id, block) = match catchup_package_maker::get_catch_up_package_type( + self.registry.as_ref(), + self.replica_config.node_id, + summary_block, + ) + .map_err(|err| format!("Failed to determine the cup type: {err}"))? + { + CatchUpPackageType::Normal => { + let threshold = self + .membership + .get_committee_threshold(summary_block.height(), CatchUpPackage::committee()) + .map_err(|err| format!("Failed to get the committee threshold: {err:?}"))?; + + let dkg_id = + active_high_threshold_nidkg_id(pool.as_cache(), summary_block.height()) + .ok_or_else(|| String::from("Couldn't get the high dkg id"))?; + + (threshold, dkg_id, summary_block.clone()) + } + CatchUpPackageType::PostSplit { new_subnet_id } => { + let post_split_summary_block = + catchup_package_maker::create_post_split_summary_block( + summary_block, + new_subnet_id, + self.registry.as_ref(), + ) + .map_err(|err| format!("Failed to create a post-split summary block: {err}"))?; + + let transcript = post_split_summary_block + .payload + .as_ref() + .as_summary() + .dkg + .current_transcript(&NiDkgTag::HighThreshold) + .ok_or_else(|| { + String::from("Couldn't find the transcript in the post-split summary block") + })?; + + let threshold = transcript.threshold.get().get() as usize; + let dkg_id = transcript.dkg_id.clone(); + + (threshold, dkg_id, post_split_summary_block) + } + }; + + let shares = pool + .get_catch_up_package_shares(block.height()) + .collect::>(); + + if shares.len() < threshold { + return Ok(None); + } + + let cup_content = + CatchUpContent::from_share_content(shares[0].content.clone(), block.clone()); + let signatures = shares.iter().map(|share| &share.signature).collect(); + + let cup = self + .crypto + .aggregate(signatures, dkg_id) + .map_err(|err| format!("Failed to aggregate shares: {err}")) + .map(|signature| CatchUpPackage { + content: cup_content, + signature, + })?; + + if let SubnetSplittingStatus::Done { new_subnet_id } = cup + .content + .block + .get_value() + .payload + .as_ref() + .as_summary() + .dkg + .subnet_splitting_status() + { + info!( + self.log, + "Aggregated a Post-Split CUP for subnet {new_subnet_id} at height {}", + cup.height() + ); + } + + Ok(Some(cup)) + } } fn to_messages(artifacts: Vec) -> Vec { @@ -185,23 +288,31 @@ fn to_messages(artifacts: Vec) -> Vec panic!("Expecting CatchUpPackageShare but got {x:?}\n"), }; + assert!(cup.check_integrity()); assert_eq!(CatchUpShareContent::from(&cup.content), share0.content); cup }) } + + #[rstest] + #[trace] + #[case::no_shares(&[], false)] + #[case::not_enough_shares(&[NODE_1], false)] + #[case::not_enough_shares(&[NODE_1, NODE_2], false)] + #[case::enough_shares(&[NODE_1, NODE_2, NODE_3], true)] + #[case::enough_shares(&[NODE_1, NODE_2, NODE_3, NODE_4], true)] + fn aggregate_post_split_cup_shares_test( + #[case] signers: &[NodeId], + #[case] expected_cup: bool, + ) { + with_test_replica_logger(|log| { + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + use ic_types::consensus::{ + backwards_compatibility::BackwardsCompatibleOption, dkg::SubnetSplittingStatus, + }; + + const SOURCE_SUBNET_ID: SubnetId = SUBNET_1; + const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2; + const INITIAL_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(1); + const SPLITTING_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(2); + const INTERVAL_LENGTH: Height = Height::new(9); + let fake_state_hash = CryptoHashOfState::from(CryptoHash(vec![1, 2, 3])); + + let Dependencies { + mut pool, + membership, + registry, + crypto, + state_manager, + replica_config, + .. + } = DependenciesBuilder::new( + pool_config, + vec![ + ( + INITIAL_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_1, NODE_2, NODE_3, NODE_4, NODE_5]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ( + SPLITTING_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_1, NODE_2, NODE_3, NODE_4]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ( + SPLITTING_REGISTRY_VERSION.get(), + DESTINATION_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_5]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ], + ) + .add_additional_registry_mutation(|registry_data_provider| { + insert_initial_dkg_transcript( + SPLITTING_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + &SubnetRecordBuilder::from(&[NODE_1, NODE_2, NODE_3, NODE_4]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + registry_data_provider, + ) + }) + .with_replica_config(ReplicaConfig { + node_id: NODE_1, + subnet_id: SOURCE_SUBNET_ID, + }) + .with_mocked_state_manager() + .build(); + + state_manager + .get_mut() + .expect_get_state_hash_at() + .return_const(Ok(fake_state_hash.clone())); + + let message_routing = FakeMessageRouting::new(); + *message_routing.next_batch_height.write().unwrap() = Height::from(2); + let message_routing = Arc::new(message_routing); + + pool.advance_round_normal_operation_n(INTERVAL_LENGTH.get()); + + let subnet_splitting_status = SubnetSplittingStatus::Scheduled { + source_subnet_id: SOURCE_SUBNET_ID, + destination_subnet_id: DESTINATION_SUBNET_ID, + }; + let mut proposal = pool.make_next_block(); + let block = proposal.content.as_mut(); + block.context.certified_height = block.height; + block.context.registry_version = SPLITTING_REGISTRY_VERSION; + let mut payload = block.payload.as_ref().as_summary().clone(); + payload.dkg.subnet_splitting_status = + BackwardsCompatibleOption::new_for_test_only(Some(subnet_splitting_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.insert_validated(proposal.clone()); + pool.notarize(&proposal); + pool.finalize(&proposal); + + let mut insert_cup_share = |node_id: NodeId| { + let cup_maker = CatchUpPackageMaker::new( + ReplicaConfig { + node_id, + subnet_id: SOURCE_SUBNET_ID, + }, + membership.clone(), + crypto.clone(), + state_manager.clone(), + message_routing.clone(), + registry.clone(), + log.clone(), + ); + + let share = cup_maker + .consider_block(&PoolReader::new(&pool), proposal.content.as_ref().clone()) + .expect("Should succeed with valid inputs"); + pool.insert_validated(share.clone()); + share + }; + + let shares = signers + .iter() + .map(|node_id| insert_cup_share(*node_id)) + .collect::>(); + + let aggregator = ShareAggregator::new( + membership, + message_routing, + crypto, + registry, + replica_config, + log, + ); + + let messages = aggregator.on_state_change(&PoolReader::new(&pool)); + + if expected_cup { + let [ConsensusMessage::CatchUpPackage(cup)] = messages.as_slice() else { + panic!("Should have aggregated a single CUP: {messages:?}"); + }; + + assert!(cup.check_integrity()); + for share in shares { + assert_eq!(CatchUpShareContent::from(&cup.content), share.content); + } + } else { + assert_eq!(messages, vec![], "Shouldn't have aggregated any artifacts"); + } + }) + }) + } } diff --git a/rs/consensus/src/consensus/validator.rs b/rs/consensus/src/consensus/validator.rs index 0359defe7111..7a209f283c8f 100644 --- a/rs/consensus/src/consensus/validator.rs +++ b/rs/consensus/src/consensus/validator.rs @@ -2,15 +2,16 @@ //! artifacts. #![allow(clippy::result_large_err)] use crate::consensus::{ - ConsensusMessageId, check_protocol_version, + ConsensusMessageId, + catchup_package_maker::{self, CatchUpPackageType}, + check_protocol_version, metrics::ValidatorMetrics, status::{self, Status}, }; use ic_consensus_dkg as dkg; use ic_consensus_idkg::{self as idkg}; use ic_consensus_utils::{ - MINIMUM_CHAIN_LENGTH, RoundRobin, active_high_threshold_nidkg_id, - active_low_threshold_nidkg_id, + MINIMUM_CHAIN_LENGTH, RoundRobin, active_low_threshold_nidkg_id, crypto::ConsensusCrypto, get_oldest_idkg_state_registry_version, membership::{Membership, MembershipError}, @@ -28,7 +29,7 @@ use ic_interfaces::{ }; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::{StateHashError, StateManager}; -use ic_logger::{ReplicaLogger, trace, warn}; +use ic_logger::{ReplicaLogger, info, trace, warn}; use ic_metrics::MetricsRegistry; use ic_replicated_state::ReplicatedState; use ic_types::{ @@ -42,7 +43,10 @@ use ic_types::{ RandomTape, RandomTapeShare, Rank, dkg::{DkgPayloadValidationFailure, InvalidDkgPayloadReason}, }, - crypto::{CryptoError, CryptoHashOf, Signed, threshold_sig::ni_dkg::NiDkgId}, + crypto::{ + CryptoError, CryptoHashOf, Signed, + threshold_sig::ni_dkg::{NiDkgId, NiDkgTag}, + }, registry::RegistryClientError, replica_config::ReplicaConfig, signature::{BasicSigned, MultiSignature, MultiSignatureShare, ThresholdSignatureShare}, @@ -92,6 +96,8 @@ enum ValidationFailure { CatchUpHeightNegligible, MissingPastPayloads, SubnetSplittingStatusError(subnet_splitting::StatusError), + CatchUpPackageTypeError(String), + SubnetSplittingError(String), } /// Possible reasons for invalid artifacts. @@ -122,6 +128,7 @@ enum InvalidArtifactReason { RepeatedSigner, ReplicaVersionMismatch, NotABlockmaker, + InvalidHeightInSplittingCatchUpPackageShare, RegistryVersionNotFrozenDuringSubnetSplitting { context_registry_version: RegistryVersion, }, @@ -290,12 +297,24 @@ impl SignatureVerify for Signed, + _pool: &PoolReader<'_>, _cfg: &ReplicaConfig, ) -> ValidationResult { let height = self.height(); - let dkg_id = active_high_threshold_nidkg_id(pool.as_cache(), height) - .ok_or_else(|| ValidationFailure::DkgSummaryNotFound(self.height()))?; + let dkg_id = self + .content + .block + .as_ref() + .payload + .as_ref() + .as_summary() + .dkg + .current_transcript(&NiDkgTag::HighThreshold) + .ok_or_else(|| ValidationFailure::DkgSummaryNotFound(self.height()))? + .dkg_id + .clone(); + //let dkg_id = active_high_threshold_nidkg_id(pool.as_cache(), height) + //.ok_or_else(|| ValidationFailure::DkgSummaryNotFound(self.height()))?; verify_threshold_committee( membership, self.signature.signer, @@ -1286,7 +1305,7 @@ impl Validator { .into()); } - // if it's not a summary block sure, make sure that the registry version is 'frozen' during + // if it's not a summary block, make sure that the registry version is 'frozen' during // subnet splitting if !proposal.payload.is_summary() { match subnet_splitting::get_status( @@ -1718,10 +1737,70 @@ impl Validator { pool_reader: &PoolReader<'_>, share_content: &CatchUpShareContent, ) -> Result { - let height = share_content.height(); - let block = pool_reader - .get_finalized_block(height) - .ok_or(ValidationFailure::FinalizedBlockNotFound(height))?; + let share_height = share_content.height(); + + let dkg_summary_block = pool_reader.get_highest_finalized_summary_block(); + let dkg_summary = &dkg_summary_block.payload.as_ref().as_summary().dkg; + + let (block, beacon, hash) = match catchup_package_maker::get_catch_up_package_type( + self.registry_client.as_ref(), + self.replica_config.node_id, + &dkg_summary_block, + ) + .map_err(|err| { + ValidationFailure::CatchUpPackageTypeError(format!( + "Failed to determine the cup type: {err}" + )) + })? { + CatchUpPackageType::PostSplit { new_subnet_id } + if dkg_summary.get_next_start_height() == share_height => + { + info!( + self.log, + "Validating post-split cup share at height {share_height}" + ); + let post_split_block = catchup_package_maker::create_post_split_summary_block( + &dkg_summary_block, + new_subnet_id, + self.registry_client.as_ref(), + ) + .map_err(ValidationFailure::SubnetSplittingError)?; + + let post_split_random_beacon = + catchup_package_maker::create_post_split_random_beacon(&post_split_block) + .map_err(ValidationFailure::SubnetSplittingError)?; + + let hash = self + .state_manager + .get_state_hash_at(dkg_summary_block.height()) + .map_err(ValidationFailure::StateHashError)?; + + (post_split_block, post_split_random_beacon, hash) + } + // We don't produce CUPs for the height at which a subnet splitting is happening. + CatchUpPackageType::PostSplit { .. } if dkg_summary.height == share_height => { + return Err( + InvalidArtifactReason::InvalidHeightInSplittingCatchUpPackageShare.into(), + ); + } + CatchUpPackageType::PostSplit { .. } | CatchUpPackageType::Normal => { + let block = pool_reader + .get_finalized_block(share_height) + .ok_or(ValidationFailure::FinalizedBlockNotFound(share_height))?; + + let beacon = pool_reader + .get_random_beacon(share_height) + .ok_or(ValidationFailure::RandomBeaconNotFound(share_height))?; + + let hash = self + .state_manager + .get_state_hash_at(share_height) + .map_err(ValidationFailure::StateHashError)?; + + (block, beacon, hash) + } + }; + if ic_types::crypto::crypto_hash(&block) != share_content.block { return Err(InvalidArtifactReason::MismatchedBlockInCatchUpPackageShare.into()); } @@ -1733,17 +1812,10 @@ impl Validator { } }; - let beacon = pool_reader - .get_random_beacon(height) - .ok_or(ValidationFailure::RandomBeaconNotFound(height))?; if &beacon != share_content.random_beacon.get_value() { return Err(InvalidArtifactReason::MismatchedRandomBeaconInCatchUpPackageShare.into()); } - let hash = self - .state_manager - .get_state_hash_at(height) - .map_err(ValidationFailure::StateHashError)?; if hash != share_content.state_hash { return Err(InvalidArtifactReason::MismatchedStateHashInCatchUpPackageShare.into()); } @@ -1752,7 +1824,7 @@ impl Validator { // Should succeed as we already got the hash above let state = self .state_manager - .get_state_at(height) + .get_state_at(block.height()) .map_err(ValidationFailure::StateManagerError)?; get_oldest_idkg_state_registry_version(state.get_ref()) } else { @@ -4537,4 +4609,227 @@ pub mod test { }) }); } + + mod subnet_splitting { + use super::*; + + use crate::consensus::catchup_package_maker::CatchUpPackageMaker; + use ic_consensus_mocks::DependenciesBuilder; + use ic_test_utilities::message_routing::FakeMessageRouting; + use ic_test_utilities_logger::with_test_replica_logger; + use ic_test_utilities_registry::insert_initial_dkg_transcript; + use ic_types::consensus::{ + backwards_compatibility::BackwardsCompatibleOption, dkg::SubnetSplittingStatus, + }; + use ic_types_test_utils::ids::{NODE_3, NODE_4, SUBNET_1, SUBNET_2}; + use std::str::FromStr; + + enum MalformShare { + StateHash, + RandomBeacon, + RegistryVersion, + Height, + } + #[rstest] + #[case(NODE_1, None, Ok(()))] + #[case(NODE_2, None, Ok(()))] + // after the split, nodes NODE_3 and NODE_4 will be on a different subnet than the validator + // (NODE_1) + #[case::wrong_subnet(NODE_3, None, Err("MismatchedBlockInCatchUpPackageShare"))] + #[case::wrong_subnet(NODE_4, None, Err("MismatchedBlockInCatchUpPackageShare"))] + #[case::wrong_state_hash( + NODE_1, + Some(MalformShare::StateHash), + Err("MismatchedStateHashInCatchUpPackageShare") + )] + #[case::wrong_random_beacon( + NODE_1, + Some(MalformShare::RandomBeacon), + Err("MismatchedRandomBeaconInCatchUpPackageShare") + )] + #[case::wrong_registry_version( + NODE_1, + Some(MalformShare::RegistryVersion), + Err("MismatchedOldestRegistryVersionInCatchUpPackageShare") + )] + #[case::wrong_height( + NODE_1, + Some(MalformShare::Height), + Err("InvalidHeightInSplittingCatchUpPackageShare") + )] + fn validate_post_split_cup_share_test( + #[case] cup_share_node_id: NodeId, + #[case] malform_share: Option, + #[case] expected_validation_result: Result<(), &str>, + ) { + with_test_replica_logger(|log| { + ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { + const SOURCE_SUBNET_ID: SubnetId = SUBNET_1; + const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2; + const INITIAL_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(1); + const SPLITTING_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(2); + const INTERVAL_LENGTH: Height = Height::new(9); + let fake_state_hash = CryptoHashOfState::from(CryptoHash(vec![1, 2, 3])); + + let ValidatorAndDependencies { + mut pool, + membership, + registry_client: registry, + crypto, + validator, + state_manager, + .. + } = ValidatorAndDependencies::new( + DependenciesBuilder::new( + pool_config, + vec![ + ( + INITIAL_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_1, NODE_2, NODE_3, NODE_4]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ( + SPLITTING_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_1, NODE_2]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ( + SPLITTING_REGISTRY_VERSION.get(), + DESTINATION_SUBNET_ID, + SubnetRecordBuilder::from(&[NODE_3, NODE_4]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + ), + ], + ) + .add_additional_registry_mutation(|registry_data_provider| { + insert_initial_dkg_transcript( + SPLITTING_REGISTRY_VERSION.get(), + SOURCE_SUBNET_ID, + &SubnetRecordBuilder::from(&[NODE_1, NODE_2]) + .with_dkg_interval_length(INTERVAL_LENGTH.get()) + .build(), + registry_data_provider, + ) + }) + .with_replica_config(ReplicaConfig { + node_id: NODE_1, + subnet_id: SOURCE_SUBNET_ID, + }) + .with_mocked_state_manager() + .build(), + ); + + state_manager + .get_mut() + .expect_get_state_hash_at() + .return_const(Ok(fake_state_hash.clone())); + + let message_routing = FakeMessageRouting::new(); + *message_routing.next_batch_height.write().unwrap() = Height::from(2); + let message_routing = Arc::new(message_routing); + + let cup_maker = CatchUpPackageMaker::new( + ReplicaConfig { + node_id: cup_share_node_id, + subnet_id: SOURCE_SUBNET_ID, + }, + membership, + crypto, + state_manager, + message_routing, + registry, + log, + ); + + pool.advance_round_normal_operation_n(INTERVAL_LENGTH.get()); + + let subnet_splitting_status = SubnetSplittingStatus::Scheduled { + source_subnet_id: SOURCE_SUBNET_ID, + destination_subnet_id: DESTINATION_SUBNET_ID, + }; + let mut proposal = pool.make_next_block(); + let block = proposal.content.as_mut(); + block.context.certified_height = block.height; + block.context.registry_version = SPLITTING_REGISTRY_VERSION; + let mut payload = block.payload.as_ref().as_summary().clone(); + payload.dkg.subnet_splitting_status = + BackwardsCompatibleOption::new_for_test_only(Some(subnet_splitting_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.insert_validated(proposal.clone()); + pool.notarize(&proposal); + pool.finalize(&proposal); + + let mut share = cup_maker + .consider_block(&PoolReader::new(&pool), proposal.content.as_ref().clone()) + .expect("Should succeed with valid inputs"); + + match malform_share { + Some(MalformShare::StateHash) => { + share.content.state_hash = + CryptoHashOfState::from(CryptoHash(vec![3, 1, 4])); + } + Some(MalformShare::RandomBeacon) => { + let mut invalid_beacon = share.content.random_beacon.into_inner(); + invalid_beacon.content.version = + ReplicaVersion::from_str("invalid_replica_version").unwrap(); + + share.content.random_beacon = HashedRandomBeacon::new( + ic_types::crypto::crypto_hash, + invalid_beacon, + ); + } + Some(MalformShare::RegistryVersion) => { + share + .content + .oldest_registry_version_in_use_by_replicated_state = + Some(INITIAL_REGISTRY_VERSION); + } + Some(MalformShare::Height) => { + let mut beacon = share.content.random_beacon.into_inner(); + beacon.content.height = proposal.height(); + + share.content.random_beacon = + HashedRandomBeacon::new(ic_types::crypto::crypto_hash, beacon); + } + None => {} + } + + pool.insert_unvalidated(share.clone()); + + let pool_reader = PoolReader::new(&pool); + let change_set = validator.validate_catch_up_package_shares(&pool_reader); + + match expected_validation_result { + Ok(()) => { + assert_eq!( + change_set, + vec![ChangeAction::MoveToValidated( + ConsensusMessage::CatchUpPackageShare(share) + )] + ); + } + Err(err) => { + assert_eq!( + change_set, + vec![ChangeAction::HandleInvalid( + ConsensusMessage::CatchUpPackageShare(share), + String::from(err), + )] + ); + } + } + }) + }) + } + } } diff --git a/rs/consensus/tests/framework/runner.rs b/rs/consensus/tests/framework/runner.rs index 7caddd443062..6aa930ae2fe5 100644 --- a/rs/consensus/tests/framework/runner.rs +++ b/rs/consensus/tests/framework/runner.rs @@ -141,6 +141,8 @@ impl<'a> ConsensusRunner<'a> { consensus_crypto.clone(), replica_logger.clone(), pool_reader, + deps.registry_client.clone(), + deps.replica_config.clone(), ))); let malicious_flags = MaliciousFlags::default(); let consensus = ic_consensus::consensus::ConsensusImpl::new( diff --git a/rs/consensus/tests/payload.rs b/rs/consensus/tests/payload.rs index 21d7729a4d8f..e6ff3778bb29 100644 --- a/rs/consensus/tests/payload.rs +++ b/rs/consensus/tests/payload.rs @@ -153,6 +153,8 @@ fn consensus_produces_expected_batches() { Arc::clone(&fake_crypto) as Arc<_>, no_op_logger(), &PoolReader::new(&*consensus_pool.read().unwrap()), + registry_client.clone(), + replica_config.clone(), ))); let (dummy_watcher, _) = watch::channel(Height::from(0)); diff --git a/rs/consensus/utils/src/pool_reader.rs b/rs/consensus/utils/src/pool_reader.rs index cc53af781a50..82c2964ba953 100644 --- a/rs/consensus/utils/src/pool_reader.rs +++ b/rs/consensus/utils/src/pool_reader.rs @@ -558,6 +558,15 @@ impl<'a> PoolReader<'a> { .dkg .get_next_start_height() } + + pub fn get_next_summary_height(&self) -> Height { + self.get_highest_finalized_summary_block() + .payload + .as_ref() + .as_summary() + .dkg + .get_next_start_height() + } } /// Take a slice returned by [`PoolReader::get_payloads_from_height`] diff --git a/rs/consensus/utils/src/subnet_splitting.rs b/rs/consensus/utils/src/subnet_splitting.rs index 9c44392a6399..b79caf07e279 100644 --- a/rs/consensus/utils/src/subnet_splitting.rs +++ b/rs/consensus/utils/src/subnet_splitting.rs @@ -2,9 +2,11 @@ use ic_interfaces_registry::RegistryClient; use ic_protobuf::{ proxy::ProxyDecodeError, registry::subnet::v1::catch_up_package_contents::CupType, }; -use ic_registry_client_helpers::subnet::SubnetRegistry; +use ic_registry_client_helpers::{node::NodeRegistry, subnet::SubnetRegistry}; use ic_types::{ - RegistryVersion, SubnetId, consensus::SubnetSplittingArgs, registry::RegistryClientError, + NodeId, RegistryVersion, SubnetId, + consensus::{Block, SubnetSplittingArgs, dkg::SubnetSplittingStatus}, + registry::RegistryClientError, }; use thiserror::Error; @@ -71,6 +73,70 @@ pub fn get_status( }) } +pub struct PostSplitAssignment { + pub new_subnet_id: SubnetId, + // for debugging purposes + pub other_subnet_id: SubnetId, +} + +#[derive(Debug, Error)] +pub enum PostSplitAssignmentError { + #[error("Error while getting the subnet id from the registry at version {0}: {1}")] + FailedToGetSubnetIdFromTheRegistry(RegistryVersion, RegistryClientError), + #[error("The node is unassigned to any subnet at registry version {0}")] + Unassigned(RegistryVersion), + #[error("The subnet is not being split according to the summary block")] + NotSplitting, + #[error("The node changed subnets during subnet splitting")] + DisallowedMembershipChange(SubnetId), +} + +pub fn get_post_split_subnet_assignment( + node_id: NodeId, + summary_block: &Block, + registry_client: &dyn RegistryClient, +) -> Result { + let SubnetSplittingStatus::Scheduled { + destination_subnet_id, + source_subnet_id, + } = summary_block + .payload + .as_ref() + .as_summary() + .dkg + .subnet_splitting_status() + else { + return Err(PostSplitAssignmentError::NotSplitting); + }; + + let new_subnet_id = registry_client + .get_subnet_id_from_node_id(node_id, summary_block.context.registry_version) + .map_err(|err| { + PostSplitAssignmentError::FailedToGetSubnetIdFromTheRegistry( + summary_block.context.registry_version, + err, + ) + })? + .ok_or(PostSplitAssignmentError::Unassigned( + summary_block.context.registry_version, + ))?; + + let other_subnet_id = if new_subnet_id == destination_subnet_id { + source_subnet_id + } else if new_subnet_id == source_subnet_id { + destination_subnet_id + } else { + return Err(PostSplitAssignmentError::DisallowedMembershipChange( + new_subnet_id, + )); + }; + + Ok(PostSplitAssignment { + new_subnet_id, + other_subnet_id, + }) +} + #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/rs/replay/src/validator.rs b/rs/replay/src/validator.rs index eed1d4b02245..5ea8c00554e0 100644 --- a/rs/replay/src/validator.rs +++ b/rs/replay/src/validator.rs @@ -170,6 +170,8 @@ impl ReplayValidator { self.consensus_crypto.clone(), self.log.clone(), pool_reader, + self.registry.clone(), + self.replica_cfg.clone(), ) } diff --git a/rs/replica/setup_ic_network/src/lib.rs b/rs/replica/setup_ic_network/src/lib.rs index 42e112d73aaf..40b175bc6162 100644 --- a/rs/replica/setup_ic_network/src/lib.rs +++ b/rs/replica/setup_ic_network/src/lib.rs @@ -548,6 +548,8 @@ fn start_consensus( Arc::clone(&consensus_crypto), log.clone(), &PoolReader::new(&*consensus_pool.read().unwrap()), + registry_client.clone(), + replica_config.clone(), ))); let mut join_handles = vec![]; From bc49b0ab203c506300f779ca32578c5427b93f0c Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Tue, 7 Apr 2026 12:57:29 +0000 Subject: [PATCH 06/84] revert --- rs/nns/governance/src/lib.rs | 2 +- rs/registry/canister/src/flags.rs | 2 +- rs/registry/canister/src/mutations/do_split_subnet.rs | 5 ++++- rs/tests/consensus/subnet_splitting_v2_test.rs | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/rs/nns/governance/src/lib.rs b/rs/nns/governance/src/lib.rs index fcdcb2eff825..09d6d566a1cf 100644 --- a/rs/nns/governance/src/lib.rs +++ b/rs/nns/governance/src/lib.rs @@ -215,7 +215,7 @@ thread_local! { = const { Cell::new(true) }; static ENABLE_SUBNET_SPLITTING_PROPOSALS: Cell - = const { Cell::new(true) }; + = const { Cell::new(false) }; // The planned effects of enabling this flag include // 1. Reduce max dissolve delay from 8 years to 2 years. This includes capping existing neurons via data migration. diff --git a/rs/registry/canister/src/flags.rs b/rs/registry/canister/src/flags.rs index f86aa90875b0..f999a0b6abcb 100644 --- a/rs/registry/canister/src/flags.rs +++ b/rs/registry/canister/src/flags.rs @@ -6,7 +6,7 @@ use ic_nervous_system_temporary::Temporary; use ic_types::{PrincipalId, SubnetId}; thread_local! { - static IS_SUBNET_SPLITTING_ENABLED: Cell = const { Cell::new(true) }; + static IS_SUBNET_SPLITTING_ENABLED: Cell = const { Cell::new(false) }; static IS_CHUNKIFYING_LARGE_VALUES_ENABLED: Cell = const { Cell::new(true) }; static IS_NODE_SWAPPING_ENABLED: Cell = const { Cell::new(true) }; diff --git a/rs/registry/canister/src/mutations/do_split_subnet.rs b/rs/registry/canister/src/mutations/do_split_subnet.rs index 6e6d4516de39..649ef92f787f 100644 --- a/rs/registry/canister/src/mutations/do_split_subnet.rs +++ b/rs/registry/canister/src/mutations/do_split_subnet.rs @@ -388,9 +388,12 @@ impl Registry { &self, record_key: &str, version: Version, - ) -> Option { + ) -> Version { self.get(record_key.as_bytes(), version) .map(|record| record.version) + .unwrap_or_else(|| { + panic!("Record for {record_key} not found in registry"); + }) } } diff --git a/rs/tests/consensus/subnet_splitting_v2_test.rs b/rs/tests/consensus/subnet_splitting_v2_test.rs index 0e0f0d9b554c..87679832ade1 100644 --- a/rs/tests/consensus/subnet_splitting_v2_test.rs +++ b/rs/tests/consensus/subnet_splitting_v2_test.rs @@ -68,7 +68,7 @@ const CHATTING_CANISTERS_ON_THIRD_SUBNET_COUNT: usize = 3; const FIRST_CHATTING_CANISTER_ID_TO_MIGRATE_OFFSET: usize = 3; const LAST_CHATTING_CANISTER_ID_TO_MIGRATE_OFFSET: usize = 8; -const TEST_ENABLED: bool = true; +const TEST_ENABLED: bool = false; fn main() -> Result<()> { SystemTestGroup::new() From e2280ad91f7615be178e0c8151dcadbc8bb57bdc Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Tue, 7 Apr 2026 13:50:54 +0000 Subject: [PATCH 07/84] revert --- rs/consensus/dkg/src/payload_builder.rs | 55 +++++++++++------------- rs/consensus/idkg/src/payload_builder.rs | 1 - rs/test_utilities/consensus/src/fake.rs | 1 - rs/types/types/src/consensus/dkg.rs | 5 +-- 4 files changed, 25 insertions(+), 37 deletions(-) diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs index 7fce89b003bb..f5b36dc0cf02 100644 --- a/rs/consensus/dkg/src/payload_builder.rs +++ b/rs/consensus/dkg/src/payload_builder.rs @@ -2,7 +2,7 @@ use crate::{ MAX_REMOTE_DKG_ATTEMPTS, MAX_REMOTE_DKGS_PER_INTERVAL, REMOTE_DKG_REPEATED_FAILURE_ERROR, utils::{self, tags_iter, vetkd_key_ids_for_subnet}, }; -use ic_consensus_utils::{crypto::ConsensusCrypto, pool_reader::PoolReader, subnet_splitting}; +use ic_consensus_utils::{crypto::ConsensusCrypto, pool_reader::PoolReader}; use ic_interfaces::{crypto::ErrorReproducibility, dkg::DkgPool}; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::StateManager; @@ -37,8 +37,6 @@ use std::{ sync::{Arc, RwLock}, }; -const SUBNET_SPLITTING_ENABLED: bool = true; - /// Creates the DKG payload for a new block proposal with the given parent. If /// the new height corresponds to a new DKG start interval, creates a summary, /// otherwise it creates a payload containing new dealings for the current @@ -256,31 +254,27 @@ pub(super) fn create_summary_payload( subnet_id, )?; - let subnet_splitting_status = if SUBNET_SPLITTING_ENABLED { - let status = subnet_splitting::get_status( - registry_client, - subnet_id, - subnet_splitting::Context { - last_summary_block_registry_version: registry_version, - current_registry_version: validation_context.registry_version, - }, - ) - .map_err(|err| DkgPayloadCreationError::SubnetSplittingStatusError(err.to_string()))?; - - match status { - subnet_splitting::Status::Scheduled { - destination_subnet_id, - scheduled_at: _, - } => Some(SubnetSplittingStatus::Scheduled { - destination_subnet_id, - source_subnet_id: subnet_id, - }), - subnet_splitting::Status::AlreadyDone => Some(SubnetSplittingStatus::NotScheduled), - subnet_splitting::Status::NotScheduled => Some(SubnetSplittingStatus::NotScheduled), - } - } else { - None - }; + // TODO(CON-1565): uncomment this when ready + // let subnet_splitting_status = match subnet_splitting::get_status( + // registry_client, + // subnet_id, + // subnet_splitting::Context { + // last_summary_block_registry_version: registry_version, + // current_registry_version: validation_context.registry_version, + // }, + // ) + // .map_err(|err| DkgPayloadCreationError::SubnetSplittingStatusError(err.to_string()))? + // { + // subnet_splitting::Status::Scheduled { + // destination_subnet_id, + // scheduled_at: _, + // } => Some(SubnetSplittingStatus::Scheduled { + // destination_subnet_id, + // source_subnet_id: subnet_id, + // }), + // subnet_splitting::Status::AlreadyDone => Some(SubnetSplittingStatus::NotScheduled), + // subnet_splitting::Status::NotScheduled => Some(SubnetSplittingStatus::NotScheduled), + // }; // New configs are created using the new stable registry version proposed by this // block, which determines receivers of the dealings. @@ -307,7 +301,6 @@ pub(super) fn create_summary_payload( next_interval_length, height, initial_dkg_attempts, - subnet_splitting_status, )) } @@ -496,7 +489,7 @@ fn get_dkg_summary_from_cup_contents_with_subnet_splitting( subnet_id: SubnetId, registry: &dyn RegistryClient, registry_version: RegistryVersion, - subnet_splitting_status: Option, + _subnet_splitting_status: Option, ) -> Result { // If we're in a NNS subnet recovery case with failover nodes, we extract the registry of the // NNS we're recovering. @@ -587,6 +580,7 @@ fn get_dkg_summary_from_cup_contents_with_subnet_splitting( format!("Could not retrieve the interval length for the genesis summary: {err:?}") })?; let next_interval_length = interval_length; + // TODO(CON-1565): pass `subnet_splitting_status` Ok(DkgSummary::new( configs, transcripts, @@ -599,7 +593,6 @@ fn get_dkg_summary_from_cup_contents_with_subnet_splitting( next_interval_length, height, BTreeMap::new(), // initial_dkg_attempts - subnet_splitting_status, )) } diff --git a/rs/consensus/idkg/src/payload_builder.rs b/rs/consensus/idkg/src/payload_builder.rs index c621a4320ff8..484cb7f67aff 100644 --- a/rs/consensus/idkg/src/payload_builder.rs +++ b/rs/consensus/idkg/src/payload_builder.rs @@ -775,7 +775,6 @@ mod tests { Height::from(100), height, BTreeMap::new(), - None, ), idkg: Some(idkg_summary), }) diff --git a/rs/test_utilities/consensus/src/fake.rs b/rs/test_utilities/consensus/src/fake.rs index ef2fc392784c..3d53ce787e09 100644 --- a/rs/test_utilities/consensus/src/fake.rs +++ b/rs/test_utilities/consensus/src/fake.rs @@ -67,7 +67,6 @@ impl Fake for DkgSummary { /*next_interval_length=*/ Height::new(59), /*height=*/ Height::new(0), /*initial_dkg_attempts=*/ BTreeMap::default(), - /*subnet_splitting_status=*/ None, ) } } diff --git a/rs/types/types/src/consensus/dkg.rs b/rs/types/types/src/consensus/dkg.rs index ced40c053efa..de7882290a2b 100644 --- a/rs/types/types/src/consensus/dkg.rs +++ b/rs/types/types/src/consensus/dkg.rs @@ -217,7 +217,6 @@ impl DkgSummary { next_interval_length: Height, height: Height, initial_dkg_attempts: BTreeMap, - subnet_splitting_status: Option, ) -> Self { Self { configs: configs @@ -232,9 +231,7 @@ impl DkgSummary { next_interval_length, height, initial_dkg_attempts, - subnet_splitting_status: BackwardsCompatibleOption::new_for_test_only( - subnet_splitting_status, - ), + subnet_splitting_status: BackwardsCompatibleOption::default(), } } From b9aef147a210a0dec1d4b380fe7c96c8e7a5fb6a Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Wed, 8 Apr 2026 13:27:36 +0000 Subject: [PATCH 08/84] More --- Cargo.lock | 1 + rs/consensus/certification/BUILD.bazel | 1 + rs/consensus/certification/Cargo.toml | 1 + rs/consensus/certification/src/certifier.rs | 78 ++++++++++++++++++++- rs/consensus/utils/src/lib.rs | 26 ++++++- rs/orchestrator/src/upgrade.rs | 9 ++- 6 files changed, 113 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c523368d2da0..ac6ba3c80684 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7910,6 +7910,7 @@ dependencies = [ "ic-interfaces-state-manager", "ic-logger", "ic-metrics", + "ic-registry-client-helpers", "ic-registry-subnet-type", "ic-replicated-state", "ic-test-utilities", diff --git a/rs/consensus/certification/BUILD.bazel b/rs/consensus/certification/BUILD.bazel index e4d113670f49..22463437475a 100644 --- a/rs/consensus/certification/BUILD.bazel +++ b/rs/consensus/certification/BUILD.bazel @@ -13,6 +13,7 @@ DEPENDENCIES = [ "//rs/interfaces/state_manager", "//rs/monitoring/logger", "//rs/monitoring/metrics", + "//rs/registry/helpers", "//rs/replicated_state", "//rs/types/types", "@crate_index//:prometheus", diff --git a/rs/consensus/certification/Cargo.toml b/rs/consensus/certification/Cargo.toml index f08aec6a4436..f64474cb9bec 100644 --- a/rs/consensus/certification/Cargo.toml +++ b/rs/consensus/certification/Cargo.toml @@ -16,6 +16,7 @@ ic-interfaces-registry = { path = "../../interfaces/registry" } ic-interfaces-state-manager = { path = "../../interfaces/state_manager" } ic-logger = { path = "../../monitoring/logger" } ic-metrics = { path = "../../monitoring/metrics" } +ic-registry-client-helpers = { path = "../../registry/helpers" } ic-replicated-state = { path = "../../replicated_state" } ic-types = { path = "../../types/types" } prometheus = { workspace = true } diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 3971a8bfbcd7..1fc3fe8f00a7 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::SubnetSplittingStatus, }, crypto::{CryptoHash, Signed}, replica_config::ReplicaConfig, @@ -354,6 +356,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); @@ -493,6 +507,28 @@ impl CertifierImpl { let registry_version = registry_version_at_height(self.consensus_pool_cache.as_ref(), certification.height)?; + 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( @@ -531,6 +567,28 @@ impl CertifierImpl { let msg = CertificationMessage::CertificationShare(share.clone()); let content = &share.signed.content; + 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( @@ -596,6 +654,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::Done { new_subnet_id }) => { + Ok(new_subnet_id != self.replica_config.subnet_id) + } + } + } } fn validate_height_witness( diff --git a/rs/consensus/utils/src/lib.rs b/rs/consensus/utils/src/lib.rs index 9eb1d85b7f1a..9a586efb4b44 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}, @@ -326,6 +329,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, @@ -355,6 +366,19 @@ fn get_active_data_at( .or_else(|| getter(&reader.summary_block(), height)) } +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 + } +} + fn get_registry_version_at_given_summary( summary_block: &Block, height: Height, diff --git a/rs/orchestrator/src/upgrade.rs b/rs/orchestrator/src/upgrade.rs index 308c2e06659b..2736f6e3a4f4 100644 --- a/rs/orchestrator/src/upgrade.rs +++ b/rs/orchestrator/src/upgrade.rs @@ -24,7 +24,7 @@ use ic_registry_local_store::{LocalStore, LocalStoreImpl}; use ic_registry_replicator::RegistryReplicator; use ic_types::{ Height, NodeId, RegistryVersion, ReplicaVersion, SubnetId, - consensus::{CatchUpPackage, HasHeight}, + consensus::{CatchUpPackage, HasHeight, dkg::SubnetSplittingStatus}, crypto::{ canister_threshold_sig::MasterPublicKey, threshold_sig::ni_dkg::{NiDkgId, NiDkgTargetSubnet}, @@ -764,6 +764,13 @@ fn get_subnet_id(registry: &dyn RegistryClient, cup: &CatchUpPackage) -> Result< .as_ref() .as_summary() .dkg; + + // If this is the first CUP created right after the subnet was split, infer the subnet id from + // the subnet splitting status in the dkg summary. + if let SubnetSplittingStatus::Done { new_subnet_id } = dkg_summary.subnet_splitting_status() { + return Ok(new_subnet_id); + } + // Note that although sometimes CUPs have no signatures (e.g. genesis and // recovery CUPs) they always have the signer id (the DKG id), which is taken // from the high-threshold transcript when we build a genesis/recovery CUP. From 226b1ec5df6b0fd2abb4f3b1cba4eb24c960a851 Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Thu, 9 Apr 2026 09:31:15 +0000 Subject: [PATCH 09/84] comment --- rs/consensus/src/consensus/catchup_package_maker.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rs/consensus/src/consensus/catchup_package_maker.rs b/rs/consensus/src/consensus/catchup_package_maker.rs index 743c1fd1769a..eb83543468f3 100644 --- a/rs/consensus/src/consensus/catchup_package_maker.rs +++ b/rs/consensus/src/consensus/catchup_package_maker.rs @@ -940,22 +940,24 @@ mod tests { }) } + // In this test the subnet initially has 4 nodes, and after the split `NODE_1, NODE_2` will stay + // in the original subnet, and `NODE_3, NODE_4` will be moved to a new one. #[rstest] #[case::source_subnet_node( NODE_1, - "8aa92d736af3d4b815de5f16f257e75f9a3977633db48dbc6f904406ea506650" + "d5a517cd0906e1d36b43edf4103ef9b0dfb0e6892a87712ce5ed6602bfa5c97e" )] #[case::source_subnet_node( NODE_2, - "8aa92d736af3d4b815de5f16f257e75f9a3977633db48dbc6f904406ea506650" + "d5a517cd0906e1d36b43edf4103ef9b0dfb0e6892a87712ce5ed6602bfa5c97e" )] #[case::destination_subnet_node( NODE_3, - "6ffa0e9003fa9585fa6d2fcb1081d6980cbc64c26a3d2e0177dc4740a41c099e" + "e8614bf48bba176a546186f90e7cfc02ec573e4b87296e9d73a70547ca168416" )] #[case::destination_subnet_node( NODE_4, - "6ffa0e9003fa9585fa6d2fcb1081d6980cbc64c26a3d2e0177dc4740a41c099e" + "e8614bf48bba176a546186f90e7cfc02ec573e4b87296e9d73a70547ca168416" )] #[trace] fn create_post_split_cup_share_test( From 0dd289dbcbaf3550e888a261cc4edd2fdf71cd67 Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Tue, 14 Apr 2026 12:51:38 +0000 Subject: [PATCH 10/84] . --- rs/registry/canister/src/mutations/do_split_subnet.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rs/registry/canister/src/mutations/do_split_subnet.rs b/rs/registry/canister/src/mutations/do_split_subnet.rs index 649ef92f787f..6e6d4516de39 100644 --- a/rs/registry/canister/src/mutations/do_split_subnet.rs +++ b/rs/registry/canister/src/mutations/do_split_subnet.rs @@ -388,12 +388,9 @@ impl Registry { &self, record_key: &str, version: Version, - ) -> Version { + ) -> Option { self.get(record_key.as_bytes(), version) .map(|record| record.version) - .unwrap_or_else(|| { - panic!("Record for {record_key} not found in registry"); - }) } } From 374122c3cccda770a2466c826cb30fa64797641b Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Tue, 14 Apr 2026 12:59:35 +0000 Subject: [PATCH 11/84] changelog --- rs/registry/canister/unreleased_changelog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rs/registry/canister/unreleased_changelog.md b/rs/registry/canister/unreleased_changelog.md index 94126a0ff421..bf32cdf4ccd6 100644 --- a/rs/registry/canister/unreleased_changelog.md +++ b/rs/registry/canister/unreleased_changelog.md @@ -17,4 +17,7 @@ on the process that this file is part of, see ## Fixed +* `do_split_subnet` - don't assume that all the registry entries exist when checking whether the + entries changed across await point + ## Security From 2b862ee4179b98c0222afc6f0f38e3ed25f23ddb Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Mon, 27 Apr 2026 08:18:31 +0000 Subject: [PATCH 12/84] . --- rs/consensus/src/consensus/batch_delivery.rs | 36 +++++++------------- 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 5ec7b20f8a27..931dc8f437fe 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -108,11 +108,9 @@ pub(crate) fn deliver_batches_with_result_processor( warn!( every_n_seconds => 30, log, - "Do not deliver height {} because no finalized block was found. \ + "Do not deliver height {height} because no finalized block was found. \ This should indicate we are waiting for state sync. \ - Finalized height: {}", - height, - finalized_height + Finalized height: {finalized_height}" ); break; }; @@ -121,8 +119,7 @@ pub(crate) fn deliver_batches_with_result_processor( warn!( every_n_seconds => 30, log, - "Do not deliver height {} because RandomTape is not ready. Will re-try later", - height + "Do not deliver height {height} because RandomTape is not ready. Will re-try later" ); break; }; @@ -144,20 +141,15 @@ pub(crate) fn deliver_batches_with_result_processor( warn!( every_n_seconds => 30, log, - "Do not deliver height {} because no summary block was found. \ - Finalized height: {}", - height, - finalized_height + "Do not deliver height {height} because no summary block was found. \ + Finalized height: {finalized_height}" ); break; }; let dkg_summary = &summary_block.payload.as_ref().as_summary().dkg; if block.payload.is_summary() { - info!( - log, - "Delivering finalized batch at CUP height of {}", height - ); + info!(log, "Delivering finalized batch at CUP height of {height}"); } // When we are not delivering CUP block, we must check if the subnet is halted. else { @@ -209,7 +201,7 @@ pub(crate) fn deliver_batches_with_result_processor( if !chain_key_subnet_public_keys.is_empty() && block.payload.is_summary() { info!( log, - "Subnet {} contains chain keys: {:?}", subnet_id, chain_key_subnet_public_keys + "Subnet {subnet_id} contains chain keys: {chain_key_subnet_public_keys:?}" ); } @@ -276,9 +268,8 @@ pub(crate) fn deliver_batches_with_result_processor( .batch .clone() .into_messages() - .map_err(|err| { - error!(log, "batch payload deserialization failed: {:?}", err); - err + .inspect_err(|err| { + error!(log, "batch payload deserialization failed: {err:?}"); }) .unwrap_or_default(), chain_key_data, @@ -292,8 +283,7 @@ pub(crate) fn deliver_batches_with_result_processor( warn!( every_n_seconds => 5, log, - "No batch delivery at height {}: no random beacon found.", - height + "No batch delivery at height {height}: no random beacon found." ); return Ok(last_delivered_batch_height); }; @@ -307,9 +297,7 @@ pub(crate) fn deliver_batches_with_result_processor( warn!( every_n_seconds => 5, log, - "No batch delivery at height {}: membership error: {:?}", - height, - e + "No batch delivery at height {height}: membership error: {e:?}" ); return Ok(last_delivered_batch_height); } @@ -341,7 +329,7 @@ pub(crate) fn deliver_batches_with_result_processor( f(&result, block_stats, batch_stats); } if let Err(err) = result { - warn!(every_n_seconds => 5, log, "Batch delivery failed: {:?}", err); + warn!(every_n_seconds => 5, log, "Batch delivery failed: {err:?}"); return Err(err); } last_delivered_batch_height = height; From 32d411c30960e9a42a0194c7670ab7844dba4cd5 Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Tue, 28 Apr 2026 09:07:44 +0000 Subject: [PATCH 13/84] . --- rs/state_machine_tests/src/lib.rs | 2 +- rs/types/types/src/consensus/catchup.rs | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/rs/state_machine_tests/src/lib.rs b/rs/state_machine_tests/src/lib.rs index 10b4b393e9eb..46528e23520c 100644 --- a/rs/state_machine_tests/src/lib.rs +++ b/rs/state_machine_tests/src/lib.rs @@ -607,7 +607,7 @@ fn make_fresh_registry_cup( replica_logger, ) .unwrap(); - cup.cup.into() + cup.into() } /// Convert an object into CBOR binary. diff --git a/rs/types/types/src/consensus/catchup.rs b/rs/types/types/src/consensus/catchup.rs index 1ff864a7d3d8..a95b7cc467a0 100644 --- a/rs/types/types/src/consensus/catchup.rs +++ b/rs/types/types/src/consensus/catchup.rs @@ -421,6 +421,12 @@ pub struct RegistryCUP { pub cup_type: RegistryCupType, } +impl From for pb::CatchUpPackage { + fn from(RegistryCUP { cup, cup_type: _ }: RegistryCUP) -> Self { + cup.into() + } +} + pub struct SubnetSplittingArgs { pub destination_subnet_id: SubnetId, } From a38c9e2a51a812e6b5c63049f56aa0248df3c75b Mon Sep 17 00:00:00 2001 From: Kamil Popielarz Date: Tue, 28 Apr 2026 12:20:10 +0000 Subject: [PATCH 14/84] fix --- rs/consensus/dkg/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/rs/consensus/dkg/src/lib.rs b/rs/consensus/dkg/src/lib.rs index 456a1878c777..d0bd4cbae4e5 100644 --- a/rs/consensus/dkg/src/lib.rs +++ b/rs/consensus/dkg/src/lib.rs @@ -1761,7 +1761,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, @@ -1864,6 +1865,7 @@ mod tests { } _ => panic!("expected data block"), }; + assert!( validate_payload( subnet_test_id(0), @@ -1871,7 +1873,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_early_remote, deps.state_manager.as_ref(), &block.context, @@ -2130,6 +2133,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, From b1485e47b8d59d4d5c8425461072a71c40e69da7 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 20 May 2026 16:21:14 +0000 Subject: [PATCH 15/84] docs: capitalize CUP --- rs/orchestrator/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/orchestrator/src/error.rs b/rs/orchestrator/src/error.rs index 1e374f19451e..955aa83783a8 100644 --- a/rs/orchestrator/src/error.rs +++ b/rs/orchestrator/src/error.rs @@ -39,7 +39,7 @@ pub(crate) enum OrchestratorError { /// The genesis or recovery CUP failed to be constructed MakeRegistryCupError(SubnetId, RegistryVersion, RegistryCupCreationError), - /// No cup found at the registry version + /// No CUP found at the registry version CupMissing(SubnetId, RegistryVersion), /// The CUP at the given height failed to be deserialized From 4b7586ddb376d65a77d4f8a4b99ab1eae9ba7fa7 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 20 May 2026 16:26:28 +0000 Subject: [PATCH 16/84] refactor: refactor peer selection for fetching CUPs --- .../src/catch_up_package_provider.rs | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/rs/orchestrator/src/catch_up_package_provider.rs b/rs/orchestrator/src/catch_up_package_provider.rs index c1443adab661..1e3682eb17e7 100644 --- a/rs/orchestrator/src/catch_up_package_provider.rs +++ b/rs/orchestrator/src/catch_up_package_provider.rs @@ -178,25 +178,39 @@ impl CatchUpPackageProvider { // Randomize the order of peer_urls nodes.shuffle(&mut rand::thread_rng()); - let current_node_index = nodes.iter().position(|t| t.0 == self.node_id); - - let max_num_peers_to_try = match (current_node_index, current_cup) { - // If we don't have a local CUP, we try not to fall back to the registry CUP. - // Therefore, we select all nodes. - (_, None) => nodes.len(), - (Some(index), _) => { - // If we are still a member of the subnet, move our own data to the front, so that we - // first try to fetch the CUP from our own replica. This improves the upgrade behaviour - // of a healthy subnet, as we decrease the probability of hitting peers who already - // started the upgrade process and will not serve a CUP until they're online again. - nodes.swap(0, index); - 2 - } - // Try only one peer at-a-time if there is already a local CUP, - (None, _) => 1, - }; + if current_cup.is_none() { + // If we don't have a local CUP, we try not to fall back to the registry CUP. Therefore, + // we select all nodes. + return nodes; + } - nodes.into_iter().take(max_num_peers_to_try).collect() + let mut selected_peers = vec![]; + // Otherwise, move our own data to the front, so that we first try to fetch the CUP from our + // own replica. This improves the upgrade behaviour of a healthy subnet, as we decrease the + // probability of hitting peers who already started the upgrade process and will not serve a + // CUP until they're online again. + // Note that our own replica might not be in the subnet record at that moment in time, + // either because we are leaving the subnet, or there is an ongoing subnet split. Though, we + // still have a local CUP, meaning that our replica is still running. + if let Some(current_node) = self + .registry + .get_node_record(self.node_id, registry_version) + .ok() + .flatten() + .map(|record| (self.node_id, record)) + { + selected_peers.push(current_node); + } + + if let Some(random_other_node) = nodes + .into_iter() + .filter(|(node_id, _)| *node_id != self.node_id) + .next() + { + selected_peers.push(random_other_node); + } + + selected_peers } /// Randomly selects a peer from the subnet and pulls its CUP. If this CUP is @@ -214,20 +228,6 @@ impl CatchUpPackageProvider { registry_version: RegistryVersion, current_cup: Option<&pb::CatchUpPackage>, ) -> Option { - let subnet_id = self - .registry - .get_subnet_id_from_node_id(self.node_id, registry_version) - .unwrap_or_default() - .inspect(|new_subnet_id| { - if *new_subnet_id != subnet_id { - info!( - self.logger, - "Subnet assignment changed from {subnet_id} to {new_subnet_id}" - ) - } - }) - .unwrap_or(subnet_id); - let peers = self.select_peers(subnet_id, registry_version, current_cup); if peers.is_empty() { From 34435a3c1c463e3a6c0280bc1922377e0fdf1c90 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 20 May 2026 16:47:21 +0000 Subject: [PATCH 17/84] refactor: remove redundant `Status::AlreadyDone` variant --- rs/consensus/dkg/src/payload_builder.rs | 3 +-- rs/consensus/src/consensus/block_maker.rs | 2 +- rs/consensus/src/consensus/validator.rs | 3 +-- rs/consensus/utils/src/subnet_splitting.rs | 12 ++++++------ 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/rs/consensus/dkg/src/payload_builder.rs b/rs/consensus/dkg/src/payload_builder.rs index aec546ecb88d..301e563b40c8 100644 --- a/rs/consensus/dkg/src/payload_builder.rs +++ b/rs/consensus/dkg/src/payload_builder.rs @@ -508,6 +508,7 @@ pub(super) fn create_summary_payload( // ) // .map_err(|err| DkgPayloadCreationError::SubnetSplittingStatusError(err.to_string()))? // { + // subnet_splitting::Status::NotScheduled => Some(SubnetSplittingStatus::NotScheduled), // subnet_splitting::Status::Scheduled { // destination_subnet_id, // scheduled_at: _, @@ -515,8 +516,6 @@ pub(super) fn create_summary_payload( // destination_subnet_id, // source_subnet_id: subnet_id, // }), - // subnet_splitting::Status::AlreadyDone => Some(SubnetSplittingStatus::NotScheduled), - // subnet_splitting::Status::NotScheduled => Some(SubnetSplittingStatus::NotScheduled), // }; // New configs are created using the new stable registry version proposed by this diff --git a/rs/consensus/src/consensus/block_maker.rs b/rs/consensus/src/consensus/block_maker.rs index 739e14fe4470..d0bab59ce887 100755 --- a/rs/consensus/src/consensus/block_maker.rs +++ b/rs/consensus/src/consensus/block_maker.rs @@ -561,6 +561,7 @@ impl BlockMaker { .ok()?; match subnet_splitting_status { + subnet_splitting::Status::NotScheduled => {} subnet_splitting::Status::Scheduled { scheduled_at, .. } => { info!( every_n_seconds => 30, @@ -575,7 +576,6 @@ impl BlockMaker { continue; } - subnet_splitting::Status::AlreadyDone | subnet_splitting::Status::NotScheduled => {} } return Some(version); diff --git a/rs/consensus/src/consensus/validator.rs b/rs/consensus/src/consensus/validator.rs index 43ae0b3c3884..acfa7122a609 100644 --- a/rs/consensus/src/consensus/validator.rs +++ b/rs/consensus/src/consensus/validator.rs @@ -1332,6 +1332,7 @@ impl Validator { ) .map_err(ValidationFailure::SubnetSplittingStatusError)? { + subnet_splitting::Status::NotScheduled => {} subnet_splitting::Status::Scheduled { .. } => { return Err( InvalidArtifactReason::RegistryVersionNotFrozenDuringSubnetSplitting { @@ -1340,8 +1341,6 @@ impl Validator { .into(), ); } - subnet_splitting::Status::AlreadyDone => {} - subnet_splitting::Status::NotScheduled => {} } } diff --git a/rs/consensus/utils/src/subnet_splitting.rs b/rs/consensus/utils/src/subnet_splitting.rs index b79caf07e279..815a3a78a5c3 100644 --- a/rs/consensus/utils/src/subnet_splitting.rs +++ b/rs/consensus/utils/src/subnet_splitting.rs @@ -12,13 +12,12 @@ use thiserror::Error; #[derive(Debug, Clone, PartialEq, Eq)] pub enum Status { + NotScheduled, Scheduled { destination_subnet_id: SubnetId, /// The registry version at which the subnet was scheduled to be split scheduled_at: RegistryVersion, }, - AlreadyDone, - NotScheduled, } #[derive(Debug, Error)] @@ -60,7 +59,8 @@ pub fn get_status( }; if versioned_record.version <= last_summary_block_registry_version { - return Ok(Status::AlreadyDone); + // This record corresponds to a past subnet split + return Ok(Status::NotScheduled); } let subnet_splitting_args: SubnetSplittingArgs = subnet_splitting_args_proto @@ -156,7 +156,7 @@ mod tests { use super::*; #[rstest] - fn should_return_not_scheduled_test( + fn should_return_not_scheduled_when_latest_cup_is_not_subnet_splitting_test( #[values( None, Some(CupType::Genesis(GenesisArgs { height: 0 })), @@ -211,7 +211,7 @@ mod tests { } #[test] - fn should_return_already_done_test() { + fn should_return_not_scheduled_when_subnet_splitting_was_already_done_test() { let registry = set_up_registry(Some(CupType::SubnetSplitting( ic_protobuf::registry::subnet::v1::SubnetSplittingArgs { destination_subnet_id: Some(subnet_id_into_protobuf(DESTINATION_SUBNET_ID)), @@ -228,7 +228,7 @@ mod tests { ) .expect("Should succeed given correct inputs"); - assert_eq!(status, Status::AlreadyDone); + assert_eq!(status, Status::NotScheduled); } fn set_up_registry(cup_type: Option) -> Arc { From ebc47d56f3d46319858bc865592b98178fa293e7 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 20 May 2026 16:47:51 +0000 Subject: [PATCH 18/84] refactor: clearer control flow --- rs/consensus/src/consensus/block_maker.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/rs/consensus/src/consensus/block_maker.rs b/rs/consensus/src/consensus/block_maker.rs index d0bab59ce887..aabcd92bf60a 100755 --- a/rs/consensus/src/consensus/block_maker.rs +++ b/rs/consensus/src/consensus/block_maker.rs @@ -561,7 +561,7 @@ impl BlockMaker { .ok()?; match subnet_splitting_status { - subnet_splitting::Status::NotScheduled => {} + subnet_splitting::Status::NotScheduled => return Some(version), subnet_splitting::Status::Scheduled { scheduled_at, .. } => { info!( every_n_seconds => 30, @@ -573,12 +573,8 @@ impl BlockMaker { if parents_height.increment() == next_summary_block_height { return Some(scheduled_at); } - - continue; } } - - return Some(version); } // If parent's version is locally available, return that. From ad3f9e75434527cde761aa3622fd7b8c90b88393 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Wed, 20 May 2026 16:48:56 +0000 Subject: [PATCH 19/84] style --- rs/consensus/utils/src/subnet_splitting.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/consensus/utils/src/subnet_splitting.rs b/rs/consensus/utils/src/subnet_splitting.rs index 815a3a78a5c3..8a0c8e881be1 100644 --- a/rs/consensus/utils/src/subnet_splitting.rs +++ b/rs/consensus/utils/src/subnet_splitting.rs @@ -83,7 +83,7 @@ pub struct PostSplitAssignment { pub enum PostSplitAssignmentError { #[error("Error while getting the subnet id from the registry at version {0}: {1}")] FailedToGetSubnetIdFromTheRegistry(RegistryVersion, RegistryClientError), - #[error("The node is unassigned to any subnet at registry version {0}")] + #[error("The node is unassigned at registry version {0}")] Unassigned(RegistryVersion), #[error("The subnet is not being split according to the summary block")] NotSplitting, From 312340caa7051820ca0eccf858ec064bd77e6509 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 08:00:07 +0000 Subject: [PATCH 20/84] typo --- rs/consensus/src/consensus/batch_delivery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 931dc8f437fe..033e58003751 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -243,7 +243,7 @@ pub(crate) fn deliver_batches_with_result_processor( info!( log, - "Deliverying splitting block. New subnet assignment: {new_subnet_id}" + "Delivering splitting block. New subnet assignment: {new_subnet_id}" ); BatchContent::Splitting { From d3e5dc7237e8b42765416cf1a3ab3de6ca6f233c Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 12:47:19 +0000 Subject: [PATCH 21/84] feat: invalidate certification shares during subnet split instead of ignoring them --- rs/consensus/certification/src/certifier.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 1fc3fe8f00a7..046898d1467d 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -569,14 +569,16 @@ impl CertifierImpl { match self.should_skip_due_to_subnet_splitting(share.height) { Ok(true) => { - info!( - every_n_seconds => 30, + warn!( self.log, - "Skipping the validation of a certification share at height {} because a + "Invalidating certification share at height {} because a \ subnet splitting is taking place", share.height ); - return None; + return Some(ChangeAction::HandleInvalid( + msg, + "Subnet splitting in progress".to_string(), + )); } Ok(false) => {} Err(err) => { From 00d30322796fb9cb503fd1571efbc6200c499b63 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 12:47:48 +0000 Subject: [PATCH 22/84] feat: trust certification's subnet signature even if during a subnet split --- rs/consensus/certification/src/certifier.rs | 50 +++++++++++---------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/rs/consensus/certification/src/certifier.rs b/rs/consensus/certification/src/certifier.rs index 046898d1467d..6fc8766d637b 100644 --- a/rs/consensus/certification/src/certifier.rs +++ b/rs/consensus/certification/src/certifier.rs @@ -15,7 +15,7 @@ use ic_interfaces::{ }; use ic_interfaces_registry::RegistryClient; use ic_interfaces_state_manager::{StateHashMetadata, StateManager}; -use ic_logger::{ReplicaLogger, debug, error, info, trace, warn}; +use ic_logger::{ReplicaLogger, debug, error, trace, warn}; use ic_metrics::{MetricsRegistry, buckets::decimal_buckets}; use ic_replicated_state::ReplicatedState; use ic_types::{ @@ -507,28 +507,6 @@ impl CertifierImpl { let registry_version = registry_version_at_height(self.consensus_pool_cache.as_ref(), certification.height)?; - 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( @@ -545,7 +523,31 @@ impl CertifierImpl { certification, registry_version, ) { - Ok(()) => Some(ChangeAction::MoveToValidated(msg)), + Ok(()) => { + match self.should_skip_due_to_subnet_splitting(certification.height) { + Ok(true) => { + error!( + self.log, + "Certification at height {} should not be valid \ + because a subnet splitting is taking place. Still \ + trusting the subnet signature and validating it. \ + This should not happen.", + certification.height + ); + } + Ok(false) => {} + Err(err) => { + warn!( + self.log, + "Failed to check the subnet splitting status: {err}. \ + Still trusting the subnet signature and validating the \ + certification." + ); + } + } + + Some(ChangeAction::MoveToValidated(msg)) + } Err(ValidationError::InvalidArtifact(err)) => { Some(ChangeAction::HandleInvalid(msg, format!("{err:?}"))) } From 306713e9f7b1a91c6eff7fbb98498472d22cf2b8 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 13:05:27 +0000 Subject: [PATCH 23/84] feat: remove panic by always retrying to load post-split DKG transcripts --- rs/consensus/dkg/src/dkg_key_manager.rs | 46 +++++++++++++++---------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/rs/consensus/dkg/src/dkg_key_manager.rs b/rs/consensus/dkg/src/dkg_key_manager.rs index e662f1f17f7c..29209e5f565c 100644 --- a/rs/consensus/dkg/src/dkg_key_manager.rs +++ b/rs/consensus/dkg/src/dkg_key_manager.rs @@ -246,26 +246,36 @@ impl DkgKeyManager { // next transcript key irrelevant and remove it). self.delete_inactive_keys(pool_reader); self.load_transcripts_from_summary(&summary.dkg); + self.last_dkg_summary_height = Some(summary_block.height); + } - if let Ok(PostSplitAssignment { - new_subnet_id, - other_subnet_id: _, - }) = subnet_splitting::get_post_split_subnet_assignment( - self.replica_config.node_id, - &summary_block, - self.registry.as_ref(), - ) { - let next_summary = get_post_split_dkg_summary( - new_subnet_id, - self.registry.as_ref(), - &summary_block, - ) - .expect("FIXME"); - info!(self.logger, "Adding post split dkg transcripts"); - self.load_transcripts_from_summary(&next_summary); + // Always try to create the summary following a potential subnet split and load its + // transcripts. This is needed as a special case, to let the replica sign and verify CUP + // shares corresponding to that post-split summary using the new DKG transcripts. + if let Ok(PostSplitAssignment { + new_subnet_id, + other_subnet_id: _, + }) = subnet_splitting::get_post_split_subnet_assignment( + self.replica_config.node_id, + &summary_block, + self.registry.as_ref(), + ) { + match get_post_split_dkg_summary(new_subnet_id, self.registry.as_ref(), &summary_block) + { + Ok(next_summary) => { + info!(self.logger, "Adding post-split DKG transcripts"); + + self.load_transcripts_from_summary(&next_summary); + self.last_dkg_summary_height = Some(next_summary.height); + } + Err(err) => { + error!( + self.logger, + "Couldn't get the next DKG summary for the new subnet {new_subnet_id:?} \ + after the split: {err:?}" + ); + } } - - self.last_dkg_summary_height = Some(summary_block.height); } } From f2b939a6590cd514e0c6cfc3f4a506b98bcb96b1 Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 13:05:37 +0000 Subject: [PATCH 24/84] style --- rs/consensus/dkg/src/payload_validator.rs | 1 + rs/consensus/src/consensus/batch_delivery.rs | 4 +++- rs/consensus/src/consensus/catchup_package_maker.rs | 4 ++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/rs/consensus/dkg/src/payload_validator.rs b/rs/consensus/dkg/src/payload_validator.rs index c4b4a57f3d89..257eda3872a8 100644 --- a/rs/consensus/dkg/src/payload_validator.rs +++ b/rs/consensus/dkg/src/payload_validator.rs @@ -636,6 +636,7 @@ mod tests { }); let last_summary_block = PoolReader::new(&pool).dkg_summary_block(&parent).unwrap(); + validate_payload( subnet_id, registry.as_ref(), diff --git a/rs/consensus/src/consensus/batch_delivery.rs b/rs/consensus/src/consensus/batch_delivery.rs index 033e58003751..00b0ce5eb8aa 100644 --- a/rs/consensus/src/consensus/batch_delivery.rs +++ b/rs/consensus/src/consensus/batch_delivery.rs @@ -230,7 +230,9 @@ pub(crate) fn deliver_batches_with_result_processor( registry_client, ) { Ok(assignment) => assignment, - Err(PostSplitAssignmentError::NotSplitting) => unreachable!(), + Err(PostSplitAssignmentError::NotSplitting) => { + panic!("Expected subnet splitting to be scheduled, but it is not") + } Err(err) => { warn!( every_n_seconds => 30, diff --git a/rs/consensus/src/consensus/catchup_package_maker.rs b/rs/consensus/src/consensus/catchup_package_maker.rs index 142fa64c5740..3a68445836fe 100644 --- a/rs/consensus/src/consensus/catchup_package_maker.rs +++ b/rs/consensus/src/consensus/catchup_package_maker.rs @@ -413,8 +413,8 @@ impl CatchUpPackageMaker { { Some(transcript) => Ok(transcript .committee - .position(self.replica_config.node_id) - .is_some()), + .get() + .contains(&self.replica_config.node_id)), None => Err(format!( "Couldn't find post-split transcript at height {}", cup_block.height From 4881192077d658d2b78d440cb5e6b7b8c9dd81aa Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 13:17:12 +0000 Subject: [PATCH 25/84] perf: avoid clones --- .../src/consensus/catchup_package_maker.rs | 2 +- .../src/consensus/share_aggregator.rs | 47 ++++++++++--------- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/rs/consensus/src/consensus/catchup_package_maker.rs b/rs/consensus/src/consensus/catchup_package_maker.rs index 3a68445836fe..782d5fdfb010 100644 --- a/rs/consensus/src/consensus/catchup_package_maker.rs +++ b/rs/consensus/src/consensus/catchup_package_maker.rs @@ -265,7 +265,7 @@ impl CatchUpPackageMaker { } let cup_block = self - .get_cup_block(start_block.clone(), cup_type) + .get_cup_block(start_block, cup_type) .inspect_err(|err| warn!(self.log, "Can't get a block for a CUP: {err}")) .ok()?; diff --git a/rs/consensus/src/consensus/share_aggregator.rs b/rs/consensus/src/consensus/share_aggregator.rs index 6696e84d4ce0..433853ab8c05 100644 --- a/rs/consensus/src/consensus/share_aggregator.rs +++ b/rs/consensus/src/consensus/share_aggregator.rs @@ -149,8 +149,12 @@ impl ShareAggregator { let mut start_block = pool.get_highest_finalized_summary_block(); let current_cup_height = pool.get_catch_up_height(); - while start_block.height() > current_cup_height { - match self.aggregate_catch_up_package_shares_for_summary_block(pool, &start_block) { + loop { + let start_block_height = start_block.height(); + if start_block_height <= current_cup_height { + break; + } + match self.aggregate_catch_up_package_shares_for_summary_block(pool, start_block) { Ok(Some(cup)) => { return vec![ConsensusMessage::CatchUpPackage(cup)]; } @@ -158,20 +162,20 @@ impl ShareAggregator { debug!( self.log, "Not enough shares to be able to create a full CUP at height{}", - start_block.height() + start_block_height ); } Err(err) => { warn!( self.log, "Encountered an error while aggregating CUP shares at height {}: {err}", - start_block.height() + start_block_height ); } } let Some(block_from_last_interval) = - pool.get_finalized_block(start_block.height.decrement()) + pool.get_finalized_block(start_block_height.decrement()) else { break; }; @@ -193,12 +197,12 @@ impl ShareAggregator { fn aggregate_catch_up_package_shares_for_summary_block( &self, pool: &PoolReader<'_>, - summary_block: &Block, + summary_block: Block, ) -> Result, String> { let (threshold, dkg_id, block) = match catchup_package_maker::get_catch_up_package_type( self.registry.as_ref(), self.replica_config.node_id, - summary_block, + &summary_block, ) .map_err(|err| format!("Failed to determine the cup type: {err}"))? { @@ -212,12 +216,12 @@ impl ShareAggregator { active_high_threshold_nidkg_id(pool.as_cache(), summary_block.height()) .ok_or_else(|| String::from("Couldn't get the high dkg id"))?; - (threshold, dkg_id, summary_block.clone()) + (threshold, dkg_id, summary_block) } CatchUpPackageType::PostSplit { new_subnet_id } => { let post_split_summary_block = catchup_package_maker::create_post_split_summary_block( - summary_block, + &summary_block, new_subnet_id, self.registry.as_ref(), ) @@ -240,16 +244,24 @@ impl ShareAggregator { } }; - let shares = pool + let mut shares = pool .get_catch_up_package_shares(block.height()) .collect::>(); + // The validation logic of CUP shares ensures that all of them have the same content for a + // given height, and it matches the content of the summary block. if shares.len() < threshold { return Ok(None); } + let share_content = shares.pop().unwrap().content; - let cup_content = - CatchUpContent::from_share_content(shares[0].content.clone(), block.clone()); + let subnet_splitting_status = block + .payload + .as_ref() + .as_summary() + .dkg + .subnet_splitting_status(); + let cup_content = CatchUpContent::from_share_content(share_content, block); let signatures = shares.iter().map(|share| &share.signature).collect(); let cup = self @@ -261,16 +273,7 @@ impl ShareAggregator { signature, })?; - if let SubnetSplittingStatus::Done { new_subnet_id } = cup - .content - .block - .get_value() - .payload - .as_ref() - .as_summary() - .dkg - .subnet_splitting_status() - { + if let SubnetSplittingStatus::Done { new_subnet_id } = subnet_splitting_status { info!( self.log, "Aggregated a Post-Split CUP for subnet {new_subnet_id} at height {}", From 32a40dba42c5618f1bdbbc3cf0f33efedf713c2a Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 13:28:35 +0000 Subject: [PATCH 26/84] feat: do not know if should halt if not given last summary block --- rs/consensus/src/consensus/status.rs | 32 +++++++++++++--------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/rs/consensus/src/consensus/status.rs b/rs/consensus/src/consensus/status.rs index 6dbd94cb271e..73c207416919 100644 --- a/rs/consensus/src/consensus/status.rs +++ b/rs/consensus/src/consensus/status.rs @@ -88,22 +88,20 @@ pub(crate) fn should_halt( .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, - // After the split, don't produce any blocks until we are on the right subnet. - SubnetSplittingStatus::Done { new_subnet_id } => subnet_id != new_subnet_id, - SubnetSplittingStatus::Scheduled { .. } => height >= summary_block.height, - } - }) - .unwrap_or_default(); + 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, + // After the split, don't produce any blocks until we are on the right subnet. + SubnetSplittingStatus::Done { new_subnet_id } => subnet_id != new_subnet_id, + SubnetSplittingStatus::Scheduled { .. } => height >= summary_block.height, + } + }); let should_halt_by_subnet_record = registry_client .get_halt_at_cup_height(subnet_id, registry_version) @@ -125,8 +123,8 @@ pub(crate) fn should_halt( any(&[ should_halt_due_to_upgrading, + should_halt_due_to_subnet_splitting, should_halt_by_subnet_record, - Some(should_halt_due_to_subnet_splitting), ]) } From 9c820b8226dc111ae3559cf68b97990804e0586a Mon Sep 17 00:00:00 2001 From: Pierugo Pace Date: Thu, 21 May 2026 13:37:25 +0000 Subject: [PATCH 27/84] test: add unit test for `get_post_split_subnet_assignment` --- rs/consensus/utils/src/subnet_splitting.rs | 188 ++++++++++++++++++++- 1 file changed, 186 insertions(+), 2 deletions(-) diff --git a/rs/consensus/utils/src/subnet_splitting.rs b/rs/consensus/utils/src/subnet_splitting.rs index 8a0c8e881be1..97b133e579e9 100644 --- a/rs/consensus/utils/src/subnet_splitting.rs +++ b/rs/consensus/utils/src/subnet_splitting.rs @@ -73,6 +73,7 @@ pub fn get_status( }) } +#[derive(Debug)] pub struct PostSplitAssignment { pub new_subnet_id: SubnetId, // for debugging purposes @@ -139,18 +140,37 @@ pub fn get_post_split_subnet_assignment( #[cfg(test)] mod tests { + use assert_matches::assert_matches; use std::sync::Arc; + use ic_interfaces_registry::RegistryClientVersionedResult; use ic_protobuf::registry::subnet::v1::CatchUpPackageContents; use ic_protobuf::registry::subnet::v1::{GenesisArgs, RecoveryArgs}; use ic_registry_keys::make_catch_up_package_contents_key; - use ic_test_utilities_registry::{SubnetRecordBuilder, setup_registry_non_final}; - use ic_test_utilities_types::ids::{NODE_1, SUBNET_1, SUBNET_2}; + use ic_test_utilities_consensus::fake::Fake; + use ic_test_utilities_registry::{ + SubnetRecordBuilder, add_single_subnet_record, add_subnet_list_record, + setup_registry_non_final, + }; + use ic_test_utilities_types::ids::{ + NODE_1, NODE_2, NODE_3, NODE_4, SUBNET_1, SUBNET_2, SUBNET_3, + }; use ic_types::subnet_id_into_protobuf; + use ic_types::{ + Height, ReplicaVersion, Time, + batch::ValidationContext, + consensus::{ + BlockPayload, Payload, Rank, SummaryPayload, + backwards_compatibility::BackwardsCompatibleOption, + }, + crypto::{CryptoHash, CryptoHashOf}, + time::UNIX_EPOCH, + }; use rstest::rstest; const SOURCE_SUBNET_ID: SubnetId = SUBNET_1; const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2; + const OTHER_SUBNET_ID: SubnetId = SUBNET_3; const REGISTRY_CUP_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(2); use super::*; @@ -253,4 +273,168 @@ mod tests { registry } + + fn make_summary_block_with_status(subnet_splitting_status: SubnetSplittingStatus) -> Block { + let mut summary = SummaryPayload::fake(); + summary.dkg.subnet_splitting_status = + BackwardsCompatibleOption::new_for_test_only(Some(subnet_splitting_status)); + Block { + version: ReplicaVersion::default(), + parent: CryptoHashOf::from(CryptoHash(vec![])), + payload: Payload::new( + ic_types::crypto::crypto_hash, + BlockPayload::Summary(summary), + ), + height: Height::new(0), + rank: Rank(0), + context: ValidationContext { + certified_height: Height::new(0), + registry_version: REGISTRY_CUP_REGISTRY_VERSION, + time: UNIX_EPOCH, + }, + } + } + + fn make_scheduled_summary_block() -> Block { + make_summary_block_with_status(SubnetSplittingStatus::Scheduled { + source_subnet_id: SOURCE_SUBNET_ID, + destination_subnet_id: DESTINATION_SUBNET_ID, + }) + } + + fn set_up_post_split_registry( + source_committee: &[NodeId], + destination_committee: &[NodeId], + other_committee: &[NodeId], + ) -> Arc { + let (registry_data_provider, registry) = setup_registry_non_final( + SOURCE_SUBNET_ID, + vec![( + 1, + SubnetRecordBuilder::new() + .with_committee(source_committee) + .build(), + )], + ); + add_single_subnet_record( + ®istry_data_provider, + REGISTRY_CUP_REGISTRY_VERSION.get(), + DESTINATION_SUBNET_ID, + SubnetRecordBuilder::new() + .with_committee(destination_committee) + .build(), + ); + add_single_subnet_record( + ®istry_data_provider, + REGISTRY_CUP_REGISTRY_VERSION.get(), + OTHER_SUBNET_ID, + SubnetRecordBuilder::new() + .with_committee(other_committee) + .build(), + ); + add_subnet_list_record( + ®istry_data_provider, + REGISTRY_CUP_REGISTRY_VERSION.get(), + vec![SOURCE_SUBNET_ID, DESTINATION_SUBNET_ID, OTHER_SUBNET_ID], + ); + registry.update_to_latest_version(); + registry + } + + struct ErrorRegistryClient; + + impl RegistryClient for ErrorRegistryClient { + fn get_versioned_value( + &self, + _key: &str, + version: RegistryVersion, + ) -> RegistryClientVersionedResult> { + Err(RegistryClientError::VersionNotAvailable { version }) + } + + fn get_key_family( + &self, + _key_prefix: &str, + version: RegistryVersion, + ) -> Result, RegistryClientError> { + Err(RegistryClientError::VersionNotAvailable { version }) + } + + fn get_latest_version(&self) -> RegistryVersion { + RegistryVersion::from(0) + } + + fn get_version_timestamp(&self, _registry_version: RegistryVersion) -> Option