diff --git a/Cargo.lock b/Cargo.lock index 51037aa9824c..9573fb282cb7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7941,6 +7941,7 @@ dependencies = [ "ic-consensus-dkg", "ic-consensus-idkg", "ic-consensus-mocks", + "ic-consensus-upgrade", "ic-consensus-utils", "ic-crypto-prng", "ic-crypto-temp-crypto", @@ -8240,6 +8241,15 @@ dependencies = [ "slog", ] +[[package]] +name = "ic-consensus-upgrade" +version = "0.9.0" +dependencies = [ + "ic-interfaces", + "ic-types", + "ic-types-test-utils", +] + [[package]] name = "ic-consensus-utils" version = "0.9.0" @@ -13647,6 +13657,7 @@ dependencies = [ "ic-consensus-features", "ic-consensus-idkg", "ic-consensus-manager", + "ic-consensus-upgrade", "ic-consensus-utils", "ic-crypto-interfaces-sig-verification", "ic-crypto-tls-interfaces", @@ -14710,6 +14721,7 @@ dependencies = [ "ic-config", "ic-consensus", "ic-consensus-cup-utils", + "ic-consensus-upgrade", "ic-consensus-utils", "ic-crypto-iccsa", "ic-crypto-test-utils-crypto-returning-ok", diff --git a/Cargo.toml b/Cargo.toml index f2d7ceddf3be..8fd8d87868f3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,6 +67,7 @@ members = [ "rs/consensus/mocks", "rs/consensus/utils", "rs/consensus/chain_key", + "rs/consensus/upgrade", "rs/criterion_time", "rs/cross-chain/blob_store", "rs/cross-chain/proposal-cli", diff --git a/rs/consensus/BUILD.bazel b/rs/consensus/BUILD.bazel index 8ac9e2de124b..ad57a67f55b4 100644 --- a/rs/consensus/BUILD.bazel +++ b/rs/consensus/BUILD.bazel @@ -66,6 +66,7 @@ rust_library( "//rs/consensus/cup_utils", "//rs/consensus/dkg", "//rs/consensus/idkg:malicious_idkg", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/prng", "//rs/crypto/test_utils/canister_threshold_sigs", @@ -115,6 +116,7 @@ rust_test( "//rs/consensus/dkg", "//rs/consensus/idkg:malicious_idkg", "//rs/consensus/mocks", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/prng", "//rs/crypto/temp_crypto", @@ -190,6 +192,7 @@ rust_test( "//rs/consensus/chain_key", "//rs/consensus/dkg", "//rs/consensus/idkg:malicious_idkg", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/prng", "//rs/crypto/temp_crypto", @@ -263,6 +266,7 @@ rust_test( "//rs/consensus/chain_key", "//rs/consensus/dkg", "//rs/consensus/idkg:malicious_idkg", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/prng", "//rs/crypto/temp_crypto", @@ -335,6 +339,7 @@ rust_test( "//rs/consensus/chain_key", "//rs/consensus/dkg", "//rs/consensus/idkg:malicious_idkg", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/prng", "//rs/crypto/temp_crypto", @@ -462,6 +467,7 @@ rust_ic_bench( ":consensus", "//rs/artifact_pool", "//rs/config", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/temp_crypto", "//rs/crypto/tree_hash", diff --git a/rs/consensus/Cargo.toml b/rs/consensus/Cargo.toml index c88199d5846f..0187b3c9875a 100644 --- a/rs/consensus/Cargo.toml +++ b/rs/consensus/Cargo.toml @@ -46,6 +46,7 @@ ic-btc-replica-types = { path = "../bitcoin/replica_types" } ic-config = { path = "../config" } ic-consensus = { path = ".", features = ["malicious_code"] } ic-consensus-mocks = { path = "./mocks" } +ic-consensus-upgrade = { path = "./upgrade" } ic-crypto-temp-crypto = { path = "../crypto/temp_crypto" } ic-crypto-test-utils-crypto-returning-ok = { path = "../crypto/test_utils/crypto_returning_ok" } ic-crypto-test-utils-ni-dkg = { path = "../crypto/test_utils/ni-dkg" } diff --git a/rs/consensus/benches/validate_payload.rs b/rs/consensus/benches/validate_payload.rs index 643983ab112d..bce9a4bf989c 100644 --- a/rs/consensus/benches/validate_payload.rs +++ b/rs/consensus/benches/validate_payload.rs @@ -176,6 +176,7 @@ where Arc::new(FakeCanisterHttpPayloadBuilder::new()), Arc::new(MockBatchPayloadBuilder::new().expect_noop()), Arc::new(MockBatchPayloadBuilder::new().expect_noop()), + Arc::new(MockBatchPayloadBuilder::new().expect_noop()), metrics_registry, no_op_logger(), )); diff --git a/rs/consensus/src/consensus.rs b/rs/consensus/src/consensus.rs index da124fc884c2..0183d9070bd1 100644 --- a/rs/consensus/src/consensus.rs +++ b/rs/consensus/src/consensus.rs @@ -136,6 +136,7 @@ impl ConsensusImpl { canister_http_payload_builder: Arc, query_stats_payload_builder: Arc, chain_key_payload_builder: Arc, + upgrade_payload_builder: Arc, dkg_pool: Arc>, idkg_pool: Arc>, dkg_key_manager: Arc>, @@ -166,6 +167,7 @@ impl ConsensusImpl { canister_http_payload_builder, query_stats_payload_builder, chain_key_payload_builder, + upgrade_payload_builder, metrics_registry.clone(), logger.clone(), )); @@ -669,6 +671,7 @@ mod tests { Arc::new(FakeCanisterHttpPayloadBuilder::new()), Arc::new(MockBatchPayloadBuilder::new().expect_noop()), Arc::new(MockBatchPayloadBuilder::new().expect_noop()), + Arc::new(MockBatchPayloadBuilder::new().expect_noop()), dkg_pool, idkg_pool, Arc::new(Mutex::new(DkgKeyManager::new( diff --git a/rs/consensus/src/consensus/payload.rs b/rs/consensus/src/consensus/payload.rs index cada9a1b3c00..f693e053870e 100644 --- a/rs/consensus/src/consensus/payload.rs +++ b/rs/consensus/src/consensus/payload.rs @@ -44,6 +44,7 @@ pub(crate) enum BatchPayloadSectionBuilder { CanisterHttp(Arc), QueryStats(Arc), ChainKey(Arc), + Upgrade(Arc), } impl BatchPayloadSectionBuilder { @@ -94,6 +95,7 @@ impl BatchPayloadSectionBuilder { Self::CanisterHttp(_) => "canister_http", Self::QueryStats(_) => "query_stats", Self::ChainKey(_) => "chain_key", + Self::Upgrade(_) => "upgrade", } } @@ -363,6 +365,44 @@ impl BatchPayloadSectionBuilder { } } } + Self::Upgrade(builder) => { + let past_payloads: Vec = + filter_past_payloads(past_payloads, |_, _, payload| { + if payload.is_summary() { + None + } else { + Some(&payload.as_ref().as_data().batch.upgrade) + } + }); + + let upgrade = builder.build_payload( + height, + max_size, + &past_payloads, + proposal_context.validation_context, + ); + let size = NumBytes::new(upgrade.len() as u64); + + // Check validation as safety measure + match builder.validate_payload(height, proposal_context, &upgrade, &past_payloads) { + Ok(()) => { + payload.upgrade = upgrade; + size + } + Err(err) => { + error!( + logger, + "upgrade payload did not pass validation, this is a bug, {:?} @{}", + err, + CRITICAL_ERROR_VALIDATION_NOT_PASSED + ); + + metrics.critical_error_validation_not_passed.inc(); + payload.upgrade = vec![]; + NumBytes::new(0) + } + } + } } } @@ -472,6 +512,25 @@ impl BatchPayloadSectionBuilder { Ok(NumBytes::new(payload.chain_key.len() as u64)) } + Self::Upgrade(builder) => { + let past_payloads: Vec = + filter_past_payloads(past_payloads, |_, _, payload| { + if payload.is_summary() { + None + } else { + Some(&payload.as_ref().as_data().batch.upgrade) + } + }); + + builder.validate_payload( + height, + proposal_context, + &payload.upgrade, + &past_payloads, + )?; + + Ok(NumBytes::new(payload.upgrade.len() as u64)) + } } } } diff --git a/rs/consensus/src/consensus/payload_builder.rs b/rs/consensus/src/consensus/payload_builder.rs index 22ccfe686f34..7d6471cfca62 100644 --- a/rs/consensus/src/consensus/payload_builder.rs +++ b/rs/consensus/src/consensus/payload_builder.rs @@ -50,6 +50,7 @@ impl PayloadBuilderImpl { canister_http_payload_builder: Arc, query_stats_payload_builder: Arc, chain_key_payload_builder: Arc, + upgrade_payload_builder: Arc, metrics: MetricsRegistry, logger: ReplicaLogger, ) -> Self { @@ -60,6 +61,7 @@ impl PayloadBuilderImpl { BatchPayloadSectionBuilder::CanisterHttp(canister_http_payload_builder), BatchPayloadSectionBuilder::QueryStats(query_stats_payload_builder), BatchPayloadSectionBuilder::ChainKey(chain_key_payload_builder), + BatchPayloadSectionBuilder::Upgrade(upgrade_payload_builder), ]; Self { @@ -290,6 +292,7 @@ pub(crate) mod test { FakeCanisterHttpPayloadBuilder::new().with_responses(canister_http_responses); let query_stats_payload_builder = MockBatchPayloadBuilder::new().expect_noop(); let chain_key_payload_builder = MockBatchPayloadBuilder::new().expect_noop(); + let upgrade_payload_builder = MockBatchPayloadBuilder::new().expect_noop(); PayloadBuilderImpl::new( subnet_test_id(0), @@ -301,6 +304,7 @@ pub(crate) mod test { Arc::new(canister_http_payload_builder), Arc::new(query_stats_payload_builder), Arc::new(chain_key_payload_builder), + Arc::new(upgrade_payload_builder), MetricsRegistry::new(), no_op_logger(), ) @@ -420,12 +424,13 @@ pub(crate) mod test { #[test] // NOTE: this test is sensitive to the order in which the individual payload builders are executed. // At the time of the writing the test the order for a block at height 1 is: - // 1. chain_key + // 1. upgrade // 2. ingress // 3. bitcoin - // 3. xnet - // 4. canister hhtp - // 5. query_stats + // 4. xnet + // 5. canister http + // 6. query_stats + // 7. chain_key fn test_get_payload_respect_limits() { ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| { let Dependencies { registry, .. } = DependenciesBuilder::new(pool_config, 1).build(); @@ -437,43 +442,39 @@ pub(crate) mod test { const CHAIN_KEY_PAYLOAD_SIZE: NumBytes = NumBytes::new(512 * KB); const QUERY_STATS_PAYLOAD_SIZE: NumBytes = NumBytes::new(MB); const INGRESS_PAYLOAD_SIZE: NumBytes = NumBytes::new(2 * MB); + const UPGRADE_PAYLOAD_SIZE: NumBytes = NumBytes::new(32 * KB); + + // The expected budgets follow the height-1 build order. Each + // section gets what remains after the earlier ones produced their + // payloads. + let upgrade_budget = MAX_BLOCK_SIZE; + let ingress_budget = upgrade_budget - UPGRADE_PAYLOAD_SIZE; + let bitcoin_budget = ingress_budget - INGRESS_PAYLOAD_SIZE; + let xnet_budget = bitcoin_budget - BITCOIN_PAYLOAD_SIZE; + let http_budget = xnet_budget - XNET_PAYLOAD_SIZE; + let query_stats_budget = http_budget - CANISTER_HTTP_PAYLOAD_SIZE; + let chain_key_budget = query_stats_budget - QUERY_STATS_PAYLOAD_SIZE; let payload_builder = set_up_payload_builder( registry, MocksSettings { - chain_key_payload_to_return: vec![0; CHAIN_KEY_PAYLOAD_SIZE.get() as usize], - expected_chain_key_payload_size_limit: MAX_BLOCK_SIZE, + upgrade_payload_to_return: vec![0; UPGRADE_PAYLOAD_SIZE.get() as usize], + expected_upgrade_payload_size_limit: upgrade_budget, ingress_payload_size_to_return: INGRESS_PAYLOAD_SIZE, - expected_ingress_payload_size_limit: MAX_BLOCK_SIZE - CHAIN_KEY_PAYLOAD_SIZE, + expected_ingress_payload_size_limit: ingress_budget, bitcoin_payload_size_to_return: BITCOIN_PAYLOAD_SIZE, - expected_bitcoin_payload_size_limit: MAX_BLOCK_SIZE - - CHAIN_KEY_PAYLOAD_SIZE - - INGRESS_PAYLOAD_SIZE, + expected_bitcoin_payload_size_limit: bitcoin_budget, xnet_payload_size_to_return: XNET_PAYLOAD_SIZE, - expected_xnet_payload_size_limit: NumBytes::new( - 95 * (MAX_BLOCK_SIZE - - CHAIN_KEY_PAYLOAD_SIZE - - INGRESS_PAYLOAD_SIZE - - BITCOIN_PAYLOAD_SIZE) - .get() - / 100, - ), + expected_xnet_payload_size_limit: NumBytes::new(95 * xnet_budget.get() / 100), http_outcalls_payload_to_return: vec![ 0; CANISTER_HTTP_PAYLOAD_SIZE.get() as usize ], - expected_http_outcalls_size_limit: MAX_BLOCK_SIZE - - CHAIN_KEY_PAYLOAD_SIZE - - INGRESS_PAYLOAD_SIZE - - BITCOIN_PAYLOAD_SIZE - - XNET_PAYLOAD_SIZE, + expected_http_outcalls_size_limit: http_budget, query_stats_payload_to_return: vec![0; QUERY_STATS_PAYLOAD_SIZE.get() as usize], - expected_query_stats_size_limit: MAX_BLOCK_SIZE - - CHAIN_KEY_PAYLOAD_SIZE - - INGRESS_PAYLOAD_SIZE - - BITCOIN_PAYLOAD_SIZE - - XNET_PAYLOAD_SIZE - - CANISTER_HTTP_PAYLOAD_SIZE, + expected_query_stats_size_limit: query_stats_budget, + chain_key_payload_to_return: vec![0; CHAIN_KEY_PAYLOAD_SIZE.get() as usize], + expected_chain_key_payload_size_limit: chain_key_budget, }, ); @@ -515,10 +516,12 @@ pub(crate) mod test { query_stats_payload_to_return: vec![0; MB as usize], chain_key_payload_to_return: vec![0; 512 * KB as usize], http_outcalls_payload_to_return: vec![0; 256 * KB as usize], + upgrade_payload_to_return: vec![0; 32 * KB as usize], bitcoin_payload_size_to_return: NumBytes::new(128 * KB), xnet_payload_size_to_return: NumBytes::new(64 * KB), // The fields below are irrelevant for the test expected_chain_key_payload_size_limit: ZERO_BYTES, + expected_upgrade_payload_size_limit: ZERO_BYTES, expected_ingress_payload_size_limit: ZERO_BYTES, expected_bitcoin_payload_size_limit: ZERO_BYTES, expected_xnet_payload_size_limit: ZERO_BYTES, @@ -547,6 +550,7 @@ pub(crate) mod test { canister_http: settings.http_outcalls_payload_to_return, query_stats: settings.query_stats_payload_to_return, chain_key: settings.chain_key_payload_to_return, + upgrade: settings.upgrade_payload_to_return, }, dkg: DkgDataPayload::new_empty(Height::from(0)), idkg: None, @@ -574,6 +578,8 @@ pub(crate) mod test { expected_ingress_payload_size_limit: NumBytes, chain_key_payload_to_return: Vec, expected_chain_key_payload_size_limit: NumBytes, + upgrade_payload_to_return: Vec, + expected_upgrade_payload_size_limit: NumBytes, bitcoin_payload_size_to_return: NumBytes, expected_bitcoin_payload_size_limit: NumBytes, xnet_payload_size_to_return: NumBytes, @@ -659,6 +665,11 @@ pub(crate) mod test { settings.expected_query_stats_size_limit, ); + let upgrade_payload_builder = MockBatchPayloadBuilder::new().with_response_and_max_size( + settings.upgrade_payload_to_return, + settings.expected_upgrade_payload_size_limit, + ); + PayloadBuilderImpl::new( subnet_test_id(0), node_test_id(0), @@ -669,6 +680,7 @@ pub(crate) mod test { Arc::new(canister_http_payload_builder), Arc::new(query_stats_payload_builder), Arc::new(chain_key_payload_builder), + Arc::new(upgrade_payload_builder), MetricsRegistry::new(), no_op_logger(), ) diff --git a/rs/consensus/tests/framework/runner.rs b/rs/consensus/tests/framework/runner.rs index d5b3a1b33562..951f3ba2b43c 100644 --- a/rs/consensus/tests/framework/runner.rs +++ b/rs/consensus/tests/framework/runner.rs @@ -155,6 +155,7 @@ impl<'a> ConsensusRunner<'a> { deps.canister_http_payload_builder.clone(), deps.query_stats_payload_builder.clone(), deps.chain_key_payload_builder.clone(), + deps.upgrade_payload_builder.clone(), deps.dkg_pool.clone(), deps.idkg_pool.clone(), dkg_key_manager.clone(), diff --git a/rs/consensus/tests/framework/types.rs b/rs/consensus/tests/framework/types.rs index 92f02a99d1a3..59921d3beb05 100644 --- a/rs/consensus/tests/framework/types.rs +++ b/rs/consensus/tests/framework/types.rs @@ -6,6 +6,7 @@ use ic_artifact_pool::{ use ic_config::artifact_pool::ArtifactPoolConfig; use ic_consensus::consensus::{ConsensusBouncer, ConsensusImpl}; use ic_consensus_idkg::IDkgImpl; +use ic_consensus_upgrade::payload_builder::UpgradePayloadBuilderImpl; use ic_consensus_utils::{MAX_CONSENSUS_THREADS, build_thread_pool}; use ic_https_outcalls_consensus::test_utils::FakeCanisterHttpPayloadBuilder; use ic_interfaces::{ @@ -176,6 +177,7 @@ pub struct ConsensusDependencies { pub(crate) canister_http_payload_builder: Arc, pub(crate) query_stats_payload_builder: Arc, pub(crate) chain_key_payload_builder: Arc, + pub(crate) upgrade_payload_builder: Arc, pub consensus_pool: Arc>, pub dkg_pool: Arc>, pub idkg_pool: Arc>, @@ -239,6 +241,7 @@ impl ConsensusDependencies { canister_http_payload_builder: Arc::new(FakeCanisterHttpPayloadBuilder::new()), query_stats_payload_builder: Arc::new(MockBatchPayloadBuilder::new().expect_noop()), chain_key_payload_builder: Arc::new(MockBatchPayloadBuilder::new().expect_noop()), + upgrade_payload_builder: Arc::new(MockBatchPayloadBuilder::new().expect_noop()), state_manager, thread_pool: build_thread_pool(MAX_CONSENSUS_THREADS), metrics_registry, diff --git a/rs/consensus/tests/payload.rs b/rs/consensus/tests/payload.rs index 2821df6e899a..cbe32797d6b5 100644 --- a/rs/consensus/tests/payload.rs +++ b/rs/consensus/tests/payload.rs @@ -66,6 +66,9 @@ fn consensus_produces_expected_batches() { let chain_key_payload_builder = MockBatchPayloadBuilder::new().expect_noop(); let chain_key_payload_builder = Arc::new(chain_key_payload_builder); + let upgrade_payload_builder = MockBatchPayloadBuilder::new().expect_noop(); + let upgrade_payload_builder = Arc::new(upgrade_payload_builder); + let mut state_manager = MockStateManager::new(); state_manager.expect_remove_states_below().return_const(()); state_manager @@ -183,6 +186,7 @@ fn consensus_produces_expected_batches() { Arc::clone(&canister_http_payload_builder) as Arc<_>, query_stats_payload_builder, chain_key_payload_builder, + upgrade_payload_builder, Arc::clone(&dkg_pool) as Arc<_>, Arc::clone(&idkg_pool) as Arc<_>, dkg_key_manager.clone(), diff --git a/rs/consensus/upgrade/BUILD.bazel b/rs/consensus/upgrade/BUILD.bazel new file mode 100644 index 000000000000..399c605e815a --- /dev/null +++ b/rs/consensus/upgrade/BUILD.bazel @@ -0,0 +1,35 @@ +load("@rules_rust//rust:defs.bzl", "rust_doc", "rust_library", "rust_test") + +package(default_visibility = ["//visibility:public"]) + +rust_library( + name = "upgrade", + srcs = glob(["src/**/*.rs"]), + crate_features = select({ + "//conditions:default": [], + }), + crate_name = "ic_consensus_upgrade", + proc_macro_deps = [ + # Keep sorted. + ], + version = "0.9.0", + deps = [ + # Keep sorted. + "//rs/interfaces", + "//rs/types/types", + ], +) + +rust_doc( + name = "consensus_upgrade_doc", + crate = ":upgrade", +) + +rust_test( + name = "upgrade_test", + crate = ":upgrade", + deps = [ + # Keep sorted. + "//rs/types/types_test_utils", + ], +) diff --git a/rs/consensus/upgrade/Cargo.toml b/rs/consensus/upgrade/Cargo.toml new file mode 100644 index 000000000000..67cd13974d6f --- /dev/null +++ b/rs/consensus/upgrade/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "ic-consensus-upgrade" +version.workspace = true +authors.workspace = true +edition.workspace = true +description.workspace = true +documentation.workspace = true + +[dependencies] +ic-interfaces = { path = "../../interfaces" } +ic-types = { path = "../../types/types" } + +[dev-dependencies] +ic-types-test-utils = { path = "../../types/types_test_utils" } diff --git a/rs/consensus/upgrade/src/lib.rs b/rs/consensus/upgrade/src/lib.rs new file mode 100644 index 000000000000..49523ebce73e --- /dev/null +++ b/rs/consensus/upgrade/src/lib.rs @@ -0,0 +1,3 @@ +//! The upgrade permit protocol for the Phase-2 rolling GuestOS reboots. + +pub mod payload_builder; diff --git a/rs/consensus/upgrade/src/payload_builder.rs b/rs/consensus/upgrade/src/payload_builder.rs new file mode 100644 index 000000000000..53216c918061 --- /dev/null +++ b/rs/consensus/upgrade/src/payload_builder.rs @@ -0,0 +1,98 @@ +use ic_interfaces::batch_payload::{BatchPayloadBuilder, PastPayload, ProposalContext}; +use ic_interfaces::consensus::{InvalidPayloadReason, PayloadValidationError}; +use ic_interfaces::upgrade::InvalidUpgradePayloadReason; +use ic_interfaces::validation::{ValidationError, ValidationResult}; +use ic_types::batch::ValidationContext; +use ic_types::{Height, NumBytes}; + +pub struct UpgradePayloadBuilderImpl; + +impl BatchPayloadBuilder for UpgradePayloadBuilderImpl { + fn build_payload( + &self, + _height: Height, + _max_size: NumBytes, + _past_payloads: &[PastPayload], + _context: &ValidationContext, + ) -> Vec { + // TODO: implement payload building + vec![] + } + + fn validate_payload( + &self, + _height: Height, + _proposal_context: &ProposalContext, + payload: &[u8], + _past_payloads: &[PastPayload], + ) -> ValidationResult { + // TODO: implement proper validation + if payload.is_empty() { + Ok(()) + } else { + Err(ValidationError::InvalidArtifact( + InvalidPayloadReason::InvalidUpgradePayload(InvalidUpgradePayloadReason::NonEmpty), + )) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use ic_types::RegistryVersion; + use ic_types::time::UNIX_EPOCH; + use ic_types_test_utils::ids::node_test_id; + + fn validation_context() -> ValidationContext { + ValidationContext { + registry_version: RegistryVersion::from(1), + certified_height: Height::from(0), + time: UNIX_EPOCH, + } + } + + #[test] + fn test_build_payload_is_empty() { + let context = validation_context(); + assert!( + UpgradePayloadBuilderImpl + .build_payload(Height::from(1), NumBytes::new(u64::MAX), &[], &context) + .is_empty() + ); + } + + #[test] + fn test_validate_payload_rejects_non_empty_payload() { + let context = validation_context(); + let proposal_context = ProposalContext { + proposer: node_test_id(1), + validation_context: &context, + }; + assert!(matches!( + UpgradePayloadBuilderImpl.validate_payload( + Height::from(1), + &proposal_context, + &[0xFF, 0xFF], + &[] + ), + Err(ValidationError::InvalidArtifact( + InvalidPayloadReason::InvalidUpgradePayload(InvalidUpgradePayloadReason::NonEmpty) + )) + )); + } + + #[test] + fn test_validate_payload_accepts_empty_payload() { + let context = validation_context(); + let proposal_context = ProposalContext { + proposer: node_test_id(1), + validation_context: &context, + }; + assert!( + UpgradePayloadBuilderImpl + .validate_payload(Height::from(1), &proposal_context, &[], &[]) + .is_ok() + ); + } +} diff --git a/rs/interfaces/src/consensus.rs b/rs/interfaces/src/consensus.rs index 03348807090e..f4d1e46bdef6 100644 --- a/rs/interfaces/src/consensus.rs +++ b/rs/interfaces/src/consensus.rs @@ -15,6 +15,7 @@ use crate::{ InvalidSelfValidatingPayloadReason, SelfValidatingPayloadValidationError, SelfValidatingPayloadValidationFailure, }, + upgrade::InvalidUpgradePayloadReason, validation::{ValidationError, ValidationResult}, }; use ic_base_types::{NumBytes, SubnetId}; @@ -75,6 +76,7 @@ pub enum InvalidPayloadReason { InvalidCanisterHttpPayload(InvalidCanisterHttpPayloadReason), InvalidQueryStatsPayload(InvalidQueryStatsPayloadReason), InvalidChainKeyPayload(InvalidChainKeyPayloadReason), + InvalidUpgradePayload(InvalidUpgradePayloadReason), /// The overall block size is too large, even though the individual payloads are valid PayloadTooBig { expected: NumBytes, diff --git a/rs/interfaces/src/lib.rs b/rs/interfaces/src/lib.rs index be56ff4b6e90..01682b3c21d8 100644 --- a/rs/interfaces/src/lib.rs +++ b/rs/interfaces/src/lib.rs @@ -19,6 +19,7 @@ pub mod p2p; pub mod query_stats; pub mod self_validating_payload; pub mod time_source; +pub mod upgrade; pub mod validation; // Note [Associated Types in Interfaces] diff --git a/rs/interfaces/src/upgrade.rs b/rs/interfaces/src/upgrade.rs new file mode 100644 index 000000000000..748acd4db90d --- /dev/null +++ b/rs/interfaces/src/upgrade.rs @@ -0,0 +1,5 @@ +/// The reason why an upgrade payload was determined to be invalid. +#[derive(Debug, Eq, PartialEq)] +pub enum InvalidUpgradePayloadReason { + NonEmpty, +} diff --git a/rs/protobuf/def/types/v1/consensus.proto b/rs/protobuf/def/types/v1/consensus.proto index 8b25ad0d351a..255fda0d31e8 100644 --- a/rs/protobuf/def/types/v1/consensus.proto +++ b/rs/protobuf/def/types/v1/consensus.proto @@ -69,6 +69,7 @@ message Block { bytes canister_http_payload_bytes = 15; bytes query_stats_payload_bytes = 16; bytes chain_key_payload_bytes = 17; + bytes upgrade_payload_bytes = 18; bytes payload_hash = 11; } diff --git a/rs/protobuf/src/gen/types/types.v1.rs b/rs/protobuf/src/gen/types/types.v1.rs index 57bf74120799..fa3ebd9bb34f 100644 --- a/rs/protobuf/src/gen/types/types.v1.rs +++ b/rs/protobuf/src/gen/types/types.v1.rs @@ -1497,6 +1497,8 @@ pub struct Block { pub query_stats_payload_bytes: ::prost::alloc::vec::Vec, #[prost(bytes = "vec", tag = "17")] pub chain_key_payload_bytes: ::prost::alloc::vec::Vec, + #[prost(bytes = "vec", tag = "18")] + pub upgrade_payload_bytes: ::prost::alloc::vec::Vec, #[prost(bytes = "vec", tag = "11")] pub payload_hash: ::prost::alloc::vec::Vec, } diff --git a/rs/replica/setup_ic_network/BUILD.bazel b/rs/replica/setup_ic_network/BUILD.bazel index 8bc67750be53..a0eeee6c7140 100644 --- a/rs/replica/setup_ic_network/BUILD.bazel +++ b/rs/replica/setup_ic_network/BUILD.bazel @@ -17,6 +17,7 @@ rust_library( "//rs/consensus/dkg", "//rs/consensus/features", "//rs/consensus/idkg", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/interfaces/sig_verification", "//rs/crypto/tls_interfaces", @@ -62,6 +63,7 @@ rust_library( "//rs/consensus/dkg", "//rs/consensus/features", "//rs/consensus/idkg:malicious_idkg", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/interfaces/sig_verification", "//rs/crypto/tls_interfaces", diff --git a/rs/replica/setup_ic_network/Cargo.toml b/rs/replica/setup_ic_network/Cargo.toml index f887c1efd15d..80aa032a1bbe 100644 --- a/rs/replica/setup_ic_network/Cargo.toml +++ b/rs/replica/setup_ic_network/Cargo.toml @@ -17,6 +17,7 @@ ic-consensus-dkg = { path = "../../consensus/dkg" } ic-consensus-features= { path = "../../consensus/features" } ic-consensus-idkg = { path = "../../consensus/idkg" } ic-consensus-manager = { path = "../../p2p/consensus_manager" } +ic-consensus-upgrade = { path = "../../consensus/upgrade" } ic-consensus-utils = { path = "../../consensus/utils" } ic-consensus-chain-key = { path = "../../consensus/chain_key" } ic-crypto-interfaces-sig-verification = { path = "../../crypto/interfaces/sig_verification" } diff --git a/rs/replica/setup_ic_network/src/lib.rs b/rs/replica/setup_ic_network/src/lib.rs index 146fa9666a2b..694d5d536ae5 100644 --- a/rs/replica/setup_ic_network/src/lib.rs +++ b/rs/replica/setup_ic_network/src/lib.rs @@ -16,6 +16,7 @@ use ic_consensus_chain_key::ChainKeyPayloadBuilderImpl; use ic_consensus_dkg::DkgBouncer; use ic_consensus_idkg::{IDkgBouncer, IDkgStatsImpl}; use ic_consensus_manager::{AbortableBroadcastChannel, AbortableBroadcastChannelBuilder}; +use ic_consensus_upgrade::payload_builder::UpgradePayloadBuilderImpl; use ic_consensus_utils::{ MAX_CONSENSUS_THREADS, build_thread_pool, crypto::ConsensusCrypto, pool_reader::PoolReader, }; @@ -543,6 +544,8 @@ fn start_consensus( metrics_registry, log.clone(), )); + + let upgrade_payload_builder = Arc::new(UpgradePayloadBuilderImpl); // ------------------------------------------------------------------------ let replica_config = ReplicaConfig { @@ -572,6 +575,7 @@ fn start_consensus( https_outcalls_payload_builder, Arc::from(query_stats_payload_builder), chain_key_payload_builder, + upgrade_payload_builder, Arc::clone(&artifact_pools.dkg_pool) as Arc<_>, Arc::clone(&artifact_pools.idkg_pool) as Arc<_>, Arc::clone(&dkg_key_manager) as Arc<_>, diff --git a/rs/state_machine_tests/BUILD.bazel b/rs/state_machine_tests/BUILD.bazel index b3ca14ca4c9d..7e207cf2cbb2 100644 --- a/rs/state_machine_tests/BUILD.bazel +++ b/rs/state_machine_tests/BUILD.bazel @@ -22,6 +22,7 @@ rust_library( "//rs/config", "//rs/consensus", "//rs/consensus/cup_utils", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/test_utils/crypto_returning_ok", "//rs/crypto/test_utils/ni-dkg", @@ -133,6 +134,7 @@ rust_ic_test( "//rs/config", "//rs/consensus", "//rs/consensus/cup_utils", + "//rs/consensus/upgrade", "//rs/consensus/utils", "//rs/crypto/test_utils/crypto_returning_ok", "//rs/crypto/test_utils/ni-dkg", diff --git a/rs/state_machine_tests/Cargo.toml b/rs/state_machine_tests/Cargo.toml index b46d96434622..0e65f9ede0e8 100644 --- a/rs/state_machine_tests/Cargo.toml +++ b/rs/state_machine_tests/Cargo.toml @@ -20,6 +20,7 @@ ic-btc-consensus = { path = "../bitcoin/consensus" } ic-config = { path = "../config" } ic-consensus = { path = "../consensus" } ic-consensus-cup-utils = { path = "../consensus/cup_utils" } +ic-consensus-upgrade = { path = "../consensus/upgrade" } ic-consensus-utils = { path = "../consensus/utils" } ic-crypto-iccsa = { path = "../crypto/iccsa" } ic-crypto-test-utils-crypto-returning-ok = { path = "../crypto/test_utils/crypto_returning_ok" } diff --git a/rs/state_machine_tests/src/lib.rs b/rs/state_machine_tests/src/lib.rs index fc187b569733..3e72ce9d2b55 100644 --- a/rs/state_machine_tests/src/lib.rs +++ b/rs/state_machine_tests/src/lib.rs @@ -13,6 +13,7 @@ use ic_config::{ }; use ic_consensus::consensus::payload_builder::PayloadBuilderImpl; use ic_consensus_cup_utils::make_registry_cup; +use ic_consensus_upgrade::payload_builder::UpgradePayloadBuilderImpl; use ic_consensus_utils::{MAX_CONSENSUS_THREADS, build_thread_pool, crypto::SignVerify}; use ic_crypto_test_utils_crypto_returning_ok::CryptoReturningOk; use ic_crypto_test_utils_ni_dkg::{ @@ -1285,6 +1286,7 @@ pub struct StateMachine { query_stats_payload_builder: Arc, local_query_execution_stats: Arc, chain_key_payload_builder: Arc, + upgrade_payload_builder: Arc, remove_old_states: bool, cycles_account_manager: Arc, } @@ -1861,6 +1863,7 @@ impl StateMachineBuilder { sm.canister_http_payload_builder.clone(), sm.query_stats_payload_builder.clone(), sm.chain_key_payload_builder.clone(), + sm.upgrade_payload_builder.clone(), sm.metrics_registry.clone(), sm.replica_logger.clone(), )); @@ -2215,6 +2218,7 @@ impl StateMachine { )); let chain_key_payload_builder = Arc::new(MockBatchPayloadBuilder::new().expect_noop()); + let upgrade_payload_builder = Arc::new(UpgradePayloadBuilderImpl); let cancellation_token = tokio_util::sync::CancellationToken::new(); let cancellation_token_clone = cancellation_token.clone(); @@ -2481,6 +2485,7 @@ impl StateMachine { query_stats_payload_builder: pocket_query_stats_payload_builder, local_query_execution_stats: execution_services.local_query_execution_stats, chain_key_payload_builder, + upgrade_payload_builder, remove_old_states, cycles_account_manager: execution_services.cycles_account_manager, } diff --git a/rs/test_utilities/types/src/batch/payload.rs b/rs/test_utilities/types/src/batch/payload.rs index aaa5e3f9a9df..d39aefd0e96a 100644 --- a/rs/test_utilities/types/src/batch/payload.rs +++ b/rs/test_utilities/types/src/batch/payload.rs @@ -15,6 +15,7 @@ impl Default for PayloadBuilder { canister_http: vec![], query_stats: vec![], chain_key: vec![], + upgrade: vec![], }, } } diff --git a/rs/types/types/src/batch.rs b/rs/types/types/src/batch.rs index cc03fdba7b2d..6fe423e31ebe 100644 --- a/rs/types/types/src/batch.rs +++ b/rs/types/types/src/batch.rs @@ -197,6 +197,7 @@ pub struct BatchPayload { pub canister_http: Vec, pub query_stats: Vec, pub chain_key: Vec, + pub upgrade: Vec, } /// Batch properties collected form the last DKG summary block. @@ -257,6 +258,7 @@ impl BatchPayload { canister_http, query_stats, chain_key, + upgrade, } = &self; ingress.is_empty() @@ -265,6 +267,7 @@ impl BatchPayload { && canister_http.is_empty() && query_stats.is_empty() && chain_key.is_empty() + && upgrade.is_empty() } } @@ -425,6 +428,7 @@ mod tests { canister_http, query_stats, chain_key, + upgrade, } = BatchPayload::default(); assert_eq!(ingress.total_ids_size_estimate(), NumBytes::new(0)); @@ -433,6 +437,7 @@ mod tests { assert_eq!(canister_http.len(), 0); assert_eq!(query_stats.len(), 0); assert_eq!(chain_key.len(), 0); + assert_eq!(upgrade.len(), 0); } /// This is a quick test to check the invariant, that the [`Default`] implementation @@ -449,6 +454,7 @@ mod tests { canister_http, query_stats, chain_key, + upgrade, } = &payload; assert!(ingress.is_empty()); @@ -457,6 +463,7 @@ mod tests { assert!(canister_http.is_empty()); assert!(query_stats.is_empty()); assert!(chain_key.is_empty()); + assert!(upgrade.is_empty()); } #[test] diff --git a/rs/types/types/src/consensus.rs b/rs/types/types/src/consensus.rs index 447f05546637..a0fa1ccef9a1 100644 --- a/rs/types/types/src/consensus.rs +++ b/rs/types/types/src/consensus.rs @@ -1301,6 +1301,7 @@ impl From<&Block> for pb::Block { canister_http_payload_bytes, query_stats_payload_bytes, chain_key_payload_bytes, + upgrade_payload_bytes, idkg_payload, ) = if payload.is_summary() { ( @@ -1311,6 +1312,7 @@ impl From<&Block> for pb::Block { vec![], vec![], vec![], + vec![], payload.as_summary().idkg.as_ref().map(|idkg| idkg.into()), ) } else { @@ -1323,6 +1325,7 @@ impl From<&Block> for pb::Block { batch.canister_http.clone(), batch.query_stats.clone(), batch.chain_key.clone(), + batch.upgrade.clone(), payload.as_data().idkg.as_ref().map(|idkg| idkg.into()), ) }; @@ -1341,6 +1344,7 @@ impl From<&Block> for pb::Block { canister_http_payload_bytes, query_stats_payload_bytes, chain_key_payload_bytes, + upgrade_payload_bytes, idkg_payload, payload_hash: block.payload.get_hash().clone().get().0, } @@ -1372,6 +1376,7 @@ impl TryFrom for Block { canister_http: block.canister_http_payload_bytes, query_stats: block.query_stats_payload_bytes, chain_key: block.chain_key_payload_bytes, + upgrade: block.upgrade_payload_bytes, }; let payload = match dkg_payload { diff --git a/rs/types/types/src/crypto/hash/tests.rs b/rs/types/types/src/crypto/hash/tests.rs index 20afc1c8b401..dc1a377a548d 100644 --- a/rs/types/types/src/crypto/hash/tests.rs +++ b/rs/types/types/src/crypto/hash/tests.rs @@ -78,12 +78,15 @@ mod crypto_hash_stability { CatchUpPackage, CatchUpPackageShare, CatchUpShareContent, ConsensusMessage, DataPayload, EquivocationProof, Finalization, FinalizationContent, FinalizationShare, HashedBlock, HashedRandomBeacon, Notarization, NotarizationContent, NotarizationShare, Payload, - RandomBeacon, RandomBeaconContent, RandomTapeContent, Rank, UpgradeAuthorizationShare, - UpgradePermitRequest, + RandomBeacon, RandomBeaconContent, RandomTapeContent, Rank, SummaryPayload, + UpgradeAuthorizationShare, UpgradePermitRequest, certification::{ Certification, CertificationContent, CertificationMessage, CertificationShare, }, - dkg::{DealingContent, DkgDataPayload, Message as DkgMessage}, + dkg::{ + DealingContent, DkgDataPayload, DkgSummary, Message as DkgMessage, + SubnetSplittingStatus, + }, hashed::Hashed, idkg::{ EcdsaSigShare, IDkgComplaintContent, IDkgMessage, IDkgOpeningContent, RequestId, @@ -581,7 +584,7 @@ mod crypto_hash_stability { /// Test stability of CatchUpContent hash output #[test] fn catch_up_content_stability() { - let block = test_block(); + let block = test_summary_block(); let hashed_block: HashedBlock = Hashed::new(crypto_hash, block); let beacon = test_random_beacon(); let hashed_beacon: HashedRandomBeacon = Hashed::new(crypto_hash, beacon); @@ -590,7 +593,7 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "764535296841f3db421a928cfadff3460be406d0182da64034eee623a9a97e99", + "29390083388965b468a0b4dcf653be560bf4ef0a58150acbf826dcac46890d13", "Hash of CatchUpContent changed" ); } @@ -598,7 +601,7 @@ mod crypto_hash_stability { /// Test stability of CatchUpShareContent hash output #[test] fn catch_up_share_content_stability() { - let block = test_block(); + let block = test_summary_block(); let hashed_block: HashedBlock = Hashed::new(crypto_hash, block); let beacon = test_random_beacon(); let hashed_beacon: HashedRandomBeacon = Hashed::new(crypto_hash, beacon); @@ -608,7 +611,7 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "7f183aaeb495159567a340b5bf61233cf3226141268febaee47de3e4c69cbc4b", + "8c535dda6ce448077a4983e2153ffd299c3a0caa1652379989c89c4964720aa2", "Hash of CatchUpShareContent changed" ); } @@ -640,7 +643,7 @@ mod crypto_hash_stability { /// Test stability of CatchUpPackage hash output #[test] fn catch_up_package_stability() { - let block = test_block(); + let block = test_summary_block(); let hashed_block: HashedBlock = Hashed::new(crypto_hash, block); let beacon = test_random_beacon(); let hashed_beacon: HashedRandomBeacon = Hashed::new(crypto_hash, beacon); @@ -656,7 +659,7 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "31f744bc26627fadbf1d73c66cb54603319a87966a488b6f41c4f0cfc1a30c89", + "90321b317ee0849ebbfd07e3ca0a3bc1595600debf4e84f8a427414b487dd883", "Hash of CatchUpPackage changed" ); } @@ -664,7 +667,7 @@ mod crypto_hash_stability { /// Test stability of CatchUpPackageShare hash output #[test] fn catch_up_package_share_stability() { - let block = test_block(); + let block = test_summary_block(); let hashed_block: HashedBlock = Hashed::new(crypto_hash, block); let beacon = test_random_beacon(); let hashed_beacon: HashedRandomBeacon = Hashed::new(crypto_hash, beacon); @@ -686,7 +689,7 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "bff423705e4cb96b7a391c4cccba8ed1ce441dabf2693ed5b9545a2b57d946bd", + "fd257cf9d018ff22f539008e785ed2a40e055eea5d78b4fa32ffbce14405aee8", "Hash of CatchUpPackageShare changed" ); } @@ -1029,20 +1032,29 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "b040378bc7d9d2b7c2e9067215eae6380a65316922369a1bc6d8376f31fe5d0a", + "5b8ca671118db0ed4f57939788881d95810b36f8d13a9954ecf2c57067e2b8d9", "Hash of Block changed" ); } /// Helper to create a test block for use in other tests - fn test_block() -> Block { + fn test_summary_block() -> Block { Block::new( test_crypto_hash_of(0x42), Payload::new( crypto_hash, - BlockPayload::Data(DataPayload { - batch: BatchPayload::default(), - dkg: DkgDataPayload::new_empty(Height::from(0)), + BlockPayload::Summary(SummaryPayload { + dkg: DkgSummary::new( + /*configs=*/ Vec::default(), + /*current_transcripts=*/ BTreeMap::default(), + /*next_transcripts=*/ BTreeMap::default(), + /*registry_version=*/ RegistryVersion::from(1), + /*interval_length=*/ Height::new(59), + /*next_interval_length=*/ Height::new(59), + /*height=*/ Height::new(0), + /*remote_dkg_attempts=*/ BTreeMap::default(), + /*subnet_splitting_status=*/ SubnetSplittingStatus::default(), + ), idkg: None, }), ), @@ -1060,7 +1072,7 @@ mod crypto_hash_stability { /// Test stability of BlockProposal hash output #[test] fn block_proposal_stability() { - let block = test_block(); + let block = test_summary_block(); let hashed_block: HashedBlock = Hashed::new(crypto_hash, block); let data: BlockProposal = Signed { content: hashed_block, @@ -1072,7 +1084,7 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "d591d695f67c644ddcc5315d96c25f00dede77c725859408ab7f113a18a0bf9a", + "7d7d85b7e8a25a005c6cfe9dd5ca8d2c9eb94193adf20cd46fe028f5696a0fde", "Hash of BlockProposal changed" ); } @@ -1109,7 +1121,7 @@ mod crypto_hash_stability { let hash = crypto_hash(&data); assert_eq!( hex::encode(hash.get_ref().0.as_slice()), - "c94d927dd7300814fef610a7560ba5a7775a859bb3511796cf23cfb59c038a4f", + "f289b64bb469c9aab1710c44b0b2fc778de9e5a552858eb10a566b8bc803d930", "Hash of BlockPayload changed" ); }