From 57169b22b93a72787565f5115f06a7c3bc99f3da Mon Sep 17 00:00:00 2001 From: Andrei Sandu <54316454+sandreim@users.noreply.github.com> Date: Tue, 25 Jul 2023 12:00:44 +0300 Subject: [PATCH] ParachainsInherent: apply weight limit only in `ProvideInherent` (#7530) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * move filtering and panic in enter Signed-off-by: Andrei Sandu * typo Signed-off-by: Andrei Sandu * more typo Signed-off-by: Andrei Sandu * review feedback Signed-off-by: Andrei Sandu * 🤦🤦🤦 Test only `polkadot-runtime-parachains` Signed-off-by: Andrei Sandu --------- Signed-off-by: Andrei Sandu --- .../parachains/src/paras_inherent/mod.rs | 79 ++++++++++++++----- .../parachains/src/paras_inherent/tests.rs | 53 +++++++++++++ polkadot/scripts/ci/gitlab/pipeline/test.yml | 3 + 3 files changed, 116 insertions(+), 19 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index ca846949b7..0afab5e58d 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -98,6 +98,15 @@ impl DisputedBitfield { } } +/// The context in which the inherent data is checked or processed. +#[derive(PartialEq)] +pub enum ProcessInherentDataContext { + /// Enables filtering/limits weight of inherent up to maximum block weight. + /// Invariant: InherentWeight <= BlockWeight. + ProvideInherent, + /// Checks the InherentWeight invariant. + Enter, +} pub use pallet::*; #[frame_support::pallet] @@ -269,7 +278,8 @@ pub mod pallet { ensure!(!Included::::exists(), Error::::TooManyInclusionInherents); Included::::set(Some(())); - Self::process_inherent_data(data).map(|(_processed, post_info)| post_info) + Self::process_inherent_data(data, ProcessInherentDataContext::Enter) + .map(|(_processed, post_info)| post_info) } } } @@ -286,7 +296,10 @@ impl Pallet { return None }, }; - match Self::process_inherent_data(parachains_inherent_data) { + match Self::process_inherent_data( + parachains_inherent_data, + ProcessInherentDataContext::ProvideInherent, + ) { Ok((processed, _)) => Some(processed), Err(err) => { log::warn!(target: LOG_TARGET, "Processing inherent data failed: {:?}", err); @@ -300,14 +313,17 @@ impl Pallet { /// The given inherent data is processed and state is altered accordingly. If any data could /// not be applied (inconsitencies, weight limit, ...) it is removed. /// - /// This function can both be called on block creation in `create_inherent` and on block import - /// in `enter`. The mutation of `data` is only useful in the `create_inherent` case as it - /// avoids overweight blocks for example. + /// When called from `create_inherent` the `context` must be set to `ProcessInherentDataContext::ProvideInherent` + /// so it guarantees the invariant that inherent is not overweight. + /// + /// It is **mandatory** that calls from `enter` set `context` to `ProcessInherentDataContext::Enter` to ensure + /// the weight invariant is checked. /// /// Returns: Result containing processed inherent data and weight, the processed inherent would /// consume. fn process_inherent_data( data: ParachainsInherentData>, + context: ProcessInherentDataContext, ) -> sp_std::result::Result< (ParachainsInherentData>, PostDispatchInfo), DispatchErrorWithPostInfo, @@ -342,6 +358,7 @@ impl Pallet { let candidates_weight = backed_candidates_weight::(&backed_candidates); let bitfields_weight = signed_bitfields_weight::(bitfields.len()); let disputes_weight = multi_dispute_statement_sets_weight::(&disputes); + let max_block_weight = ::BlockWeights::get().max_block; METRICS .on_before_filter((candidates_weight + bitfields_weight + disputes_weight).ref_time()); @@ -349,7 +366,6 @@ impl Pallet { let current_session = >::session_index(); let expected_bits = >::availability_cores().len(); let validator_public = shared::Pallet::::active_validator_keys(); - let max_block_weight = ::BlockWeights::get().max_block; let entropy = compute_entropy::(parent_hash); let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); @@ -375,22 +391,47 @@ impl Pallet { &mut rng, ); - // Assure the maximum block weight is adhered, by limiting bitfields and backed - // candidates. Dispute statement sets were already limited before. - let non_disputes_weight = apply_weight_limit::( - &mut backed_candidates, - &mut bitfields, - max_block_weight.saturating_sub(checked_disputes_sets_consumed_weight), - &mut rng, - ); + let full_weight = if context == ProcessInherentDataContext::ProvideInherent { + // Assure the maximum block weight is adhered, by limiting bitfields and backed + // candidates. Dispute statement sets were already limited before. + let non_disputes_weight = apply_weight_limit::( + &mut backed_candidates, + &mut bitfields, + max_block_weight.saturating_sub(checked_disputes_sets_consumed_weight), + &mut rng, + ); - let full_weight = non_disputes_weight.saturating_add(checked_disputes_sets_consumed_weight); + let full_weight = + non_disputes_weight.saturating_add(checked_disputes_sets_consumed_weight); - METRICS.on_after_filter(full_weight.ref_time()); + METRICS.on_after_filter(full_weight.ref_time()); - if full_weight.any_gt(max_block_weight) { - log::warn!(target: LOG_TARGET, "Post weight limiting weight is still too large."); - } + if full_weight.any_gt(max_block_weight) { + log::warn!(target: LOG_TARGET, "Post weight limiting weight is still too large."); + } + + full_weight + } else { + // We compute the weight for the unfiltered disputes for a stronger check, since `create_inherent` + // should already have filtered them out in block authorship. + let full_weight = candidates_weight + .saturating_add(bitfields_weight) + .saturating_add(disputes_weight); + + // This check is performed in the context of block execution. Ensures inherent weight invariants guaranteed + // by `create_inherent_data` for block authorship. + if full_weight.any_gt(max_block_weight) { + log::error!( + "Overweight para inherent data reached the runtime {:?}: {} > {}", + parent_hash, + full_weight, + max_block_weight + ); + } + + ensure!(full_weight.all_lte(max_block_weight), Error::::InherentOverweight); + full_weight + }; // Note that `process_checked_multi_dispute_data` will iterate and import each // dispute; so the input here must be reasonably bounded, diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 9e2e8c8fff..0098814c8a 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -673,6 +673,59 @@ mod enter { ); }); } + + #[test] + // Ensure that overweight parachain inherents are always rejected by the runtime. + // Runtime should panic and return `InherentOverweight` error. + fn inherent_create_weight_invariant() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + // Create an overweight inherent and oversized block + let mut dispute_statements = BTreeMap::new(); + dispute_statements.insert(2, 100); + dispute_statements.insert(3, 200); + dispute_statements.insert(4, 300); + + let mut backed_and_concluding = BTreeMap::new(); + + for i in 0..30 { + backed_and_concluding.insert(i, i); + } + + let scenario = make_inherent_data(TestConfig { + dispute_statements, + dispute_sessions: vec![2, 2, 1], // 3 cores with disputes + backed_and_concluding, + num_validators_per_core: 5, + code_upgrade: None, + }); + + let expected_para_inherent_data = scenario.data.clone(); + assert!(max_block_weight().any_lt(inherent_data_weight(&expected_para_inherent_data))); + + // Check the para inherent data is as expected: + // * 1 bitfield per validator (5 validators per core, 30 backed candidates, 3 disputes => 5*33 = 165) + assert_eq!(expected_para_inherent_data.bitfields.len(), 165); + // * 30 backed candidates + assert_eq!(expected_para_inherent_data.backed_candidates.len(), 30); + // * 3 disputes. + assert_eq!(expected_para_inherent_data.disputes.len(), 3); + let mut inherent_data = InherentData::new(); + inherent_data + .put_data(PARACHAINS_INHERENT_IDENTIFIER, &expected_para_inherent_data) + .unwrap(); + + let dispatch_error = Pallet::::enter( + frame_system::RawOrigin::None.into(), + expected_para_inherent_data, + ) + .unwrap_err() + .error; + + assert_eq!(dispatch_error, Error::::InherentOverweight.into()); + }); + } + + // TODO: Test process inherent invariant } fn default_header() -> primitives::Header { diff --git a/polkadot/scripts/ci/gitlab/pipeline/test.yml b/polkadot/scripts/ci/gitlab/pipeline/test.yml index 222a6d0017..60886dc60c 100644 --- a/polkadot/scripts/ci/gitlab/pipeline/test.yml +++ b/polkadot/scripts/ci/gitlab/pipeline/test.yml @@ -42,6 +42,9 @@ test-linux-stable: RUSTFLAGS: "-Cdebug-assertions=y -Dwarnings" script: - time cargo test --workspace --profile testnet --verbose --locked --features=runtime-benchmarks,runtime-metrics,try-runtime,ci-only-tests + # Run `polkadot-runtime-parachains` tests a second time because `paras_inherent::enter` tests are gated by not having + # the `runtime-benchmarks` feature enabled. + - time cargo test --profile testnet --verbose --locked --features=runtime-metrics,try-runtime -p polkadot-runtime-parachains test-linux-oldkernel-stable: extends: test-linux-stable