feat: freeze registry version until summary

Author: pierugo-dfinity

Cargo.lock

@@ -13,6 +13,7 @@ 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,
@@ -217,7 +218,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)?;
@@ -521,20 +531,59 @@ 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<RegistryVersion> {
+    /// 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<RegistryVersion> {
         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,
+                last_summary_block_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::NotScheduled => return Some(version),
+                subnet_splitting::Status::Scheduled { scheduled_at, .. } => {
+                    info!(
+                        every_n_seconds => 30,
+                        self.log,
+                        "Subnet splitting scheduled at registry version {scheduled_at} \
+                        and height {next_summary_block_height}. Freezing registry version."
+                    );
+
+                    if parents_height.increment() == next_summary_block_height {
+                        return Some(scheduled_at);
+                    }
+                }
             }
         }
+
         // If parent's version is locally available, return that.
         if parents_version <= latest_version {
             return Some(parents_version);
@@ -669,6 +718,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::fake::FromParent;
     use ic_test_utilities_registry::{SubnetRecordBuilder, add_subnet_record};
     use ic_test_utilities_types::ids::{node_test_id, subnet_test_id};
@@ -680,6 +733,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;
 
@@ -1230,25 +1285,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)
             );
         })
@@ -1366,4 +1429,141 @@ mod tests {
             )
         })
     }
+
+    mod subnet_splitting {
+        use super::*;
+
+        const MAX_REGISTRY_VERSION: u64 = 4;
+
+        #[derive(Debug)]
+        struct TestCase {
+            splitting_registry_version: Option<RegistryVersion>,
+            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(&registry) as Arc<dyn RegistryClient>,
+                    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(
+                        &registry_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,
+                );
+            })
+        }
+    }
 }

rs/consensus/src/consensus/block_maker.rs

@@ -146,9 +146,16 @@ impl ConsensusImpl {
         let last_summary_block = pool.dkg_summary_block(parent.as_ref())?;
 
         // Get the subnet records that are relevant to making a block
-        let stable_registry_version = self
-            .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,

rs/consensus/src/consensus/malicious_consensus.rs

@@ -15,6 +15,7 @@ use ic_consensus_utils::{
     get_oldest_state_registry_version,
     membership::{Membership, MembershipError},
     pool_reader::{PoolReader, UnexpectedChainLength},
+    subnet_splitting,
 };
 use ic_interfaces::{
     batch_payload::ProposalContext,
@@ -90,6 +91,7 @@ enum ValidationFailure {
     ValidationContextNotReached(ValidationContext, ValidationContext),
     CatchUpHeightNegligible,
     MissingPastPayloads,
+    SubnetSplittingStatusError(subnet_splitting::StatusError),
 }
 
 /// Possible reasons for invalid artifacts.
@@ -119,6 +121,9 @@ enum InvalidArtifactReason {
     RepeatedSigner,
     ReplicaVersionMismatch,
     NotABlockmaker,
+    RegistryVersionNotFrozenDuringSubnetSplitting {
+        context_registry_version: RegistryVersion,
+    },
 }
 
 impl From<CryptoError> for ValidationFailure {
@@ -1271,6 +1276,29 @@ impl Validator {
             .into());
         }
 
+        // 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(
+                self.registry_client.as_ref(),
+                self.replica_config.subnet_id,
+                last_summary_block.context.registry_version,
+                proposal.context.registry_version,
+            )
+            .map_err(ValidationFailure::SubnetSplittingStatusError)?
+            {
+                subnet_splitting::Status::NotScheduled => {}
+                subnet_splitting::Status::Scheduled { .. } => {
+                    return Err(
+                        InvalidArtifactReason::RegistryVersionNotFrozenDuringSubnetSplitting {
+                            context_registry_version: proposal.context.registry_version,
+                        }
+                        .into(),
+                    );
+                }
+            }
+        }
+
         // While halting or halted, data blocks must have an empty payload: skip the rest of the
         // validation if they do, and reject them otherwise. Summary blocks always carry a payload,
         // so they are validated normally.

rs/consensus/src/consensus/validator.rs

@@ -29,6 +29,7 @@ rust_library(
         "@crate_index//:rand",
         "@crate_index//:rayon",
         "@crate_index//:slog",
+        "@crate_index//:thiserror",
     ],
 )
 
@@ -64,6 +65,7 @@ rust_test(
         "@crate_index//:prometheus",
         "@crate_index//:rand",
         "@crate_index//:rayon",
+        "@crate_index//:rstest",
         "@crate_index//:slog",
     ],
 )