ParachainsInherent: apply weight limit only in ProvideInherent (#7530)

* move filtering and panic in enter

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* typo

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* more typo

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* review feedback

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* 🤦🤦🤦 Test only `polkadot-runtime-parachains`

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

---------

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

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::<T>::exists(), Error::<T>::TooManyInclusionInherents);
 			Included::<T>::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<T: Config> Pallet<T> {
 				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<T: Config> Pallet<T> {
 	/// 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
+	/// When called from `create_inherent` the `context` must be set to `ProcessInherentDataContext::ProvideInherent`
-	/// in `enter`. The mutation of `data` is only useful in the `create_inherent` case as it
+	/// so it guarantees the invariant that inherent is not overweight.
-	/// avoids overweight blocks for example.
+	///  
+	/// 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<HeaderFor<T>>,
+		context: ProcessInherentDataContext,
 	) -> sp_std::result::Result<
 		(ParachainsInherentData<HeaderFor<T>>, PostDispatchInfo),
 		DispatchErrorWithPostInfo,
@@ -342,6 +358,7 @@ impl<T: Config> Pallet<T> {
 		let candidates_weight = backed_candidates_weight::<T>(&backed_candidates);
 		let bitfields_weight = signed_bitfields_weight::<T>(bitfields.len());
 		let disputes_weight = multi_dispute_statement_sets_weight::<T, _, _>(&disputes);
+		let max_block_weight = <T as frame_system::Config>::BlockWeights::get().max_block;
 
 		METRICS
 			.on_before_filter((candidates_weight + bitfields_weight + disputes_weight).ref_time());
@@ -349,7 +366,6 @@ impl<T: Config> Pallet<T> {
 		let current_session = <shared::Pallet<T>>::session_index();
 		let expected_bits = <scheduler::Pallet<T>>::availability_cores().len();
 		let validator_public = shared::Pallet::<T>::active_validator_keys();
-		let max_block_weight = <T as frame_system::Config>::BlockWeights::get().max_block;
 
 		let entropy = compute_entropy::<T>(parent_hash);
 		let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into());
@@ -375,22 +391,47 @@ impl<T: Config> Pallet<T> {
 				&mut rng,
 			);
 
-		// Assure the maximum block weight is adhered, by limiting bitfields and backed
+		let full_weight = if context == ProcessInherentDataContext::ProvideInherent {
-		// candidates. Dispute statement sets were already limited before.
+			// Assure the maximum block weight is adhered, by limiting bitfields and backed
-		let non_disputes_weight = apply_weight_limit::<T>(
+			// candidates. Dispute statement sets were already limited before.
-			&mut backed_candidates,
+			let non_disputes_weight = apply_weight_limit::<T>(
-			&mut bitfields,
+				&mut backed_candidates,
-			max_block_weight.saturating_sub(checked_disputes_sets_consumed_weight),
+				&mut bitfields,
-			&mut rng,
+				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) {
+			if full_weight.any_gt(max_block_weight) {
-			log::warn!(target: LOG_TARGET, "Post weight limiting weight is still too large.");
+				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::<T>::InherentOverweight);
+			full_weight
+		};
 
 		// Note that `process_checked_multi_dispute_data` will iterate and import each
 		// dispute; so the input here must be reasonably bounded,

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::<Test>::enter(
+				frame_system::RawOrigin::None.into(),
+				expected_para_inherent_data,
+			)
+			.unwrap_err()
+			.error;
+
+			assert_eq!(dispatch_error, Error::<Test>::InherentOverweight.into());
+		});
+	}
+
+	// TODO: Test process inherent invariant
 }
 
 fn default_header() -> primitives::Header {

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