[runtime] follow up relay chain cleanups (#4657)

* fix miscalculation of remaining weight

* rename a var

* move out enforcing filtering by dropping inherents

* prepare for dispute statement validity check being split off

* refactor

* refactor, only check disputes we actually want to include

* more refactor and documentation

* refactor and minimize inherent checks

* chore: warnings

* fix a few tests

* fix dedup regression

* fix

* more asserts in tests

* remove some asserts

* chore: fmt

* skip signatures checks, some more

* undo unwatend changes

* Update runtime/parachains/src/paras_inherent/mod.rs

Co-authored-by: sandreim <54316454+sandreim@users.noreply.github.com>

* cleanups, checking CheckedDisputeStatments makes no sense

* integrity, if called create_inherent_inner, it shall do the checks, and not rely on enter_inner

* review comments

* use from impl rather than into

* remove outdated comment

* adjust tests accordingly

* assure no weight is lost

* address review comments

* remove unused import

* split error into two and document

* use assurance, O(n)

* Revert "adjust tests accordingly"

This reverts commit 3cc9a3c449f82db38cea22c48f4a21876603374b.

* fix comment

* fix sorting

* comment

Co-authored-by: sandreim <54316454+sandreim@users.noreply.github.com>

polkadot/runtime/parachains/src/inclusion/mod.rs

@@ -21,10 +21,8 @@
 //! to included.
 
 use crate::{
-	configuration, disputes, dmp, hrmp, paras,
-	paras_inherent::{sanitize_bitfields, DisputedBitfield},
-	scheduler::CoreAssignment,
-	shared, ump,
+	configuration, disputes, dmp, hrmp, paras, paras_inherent::DisputedBitfield,
+	scheduler::CoreAssignment, shared, ump,
 };
 use bitvec::{order::Lsb0 as BitOrderLsb0, vec::BitVec};
 use frame_support::pallet_prelude::*;
@@ -58,7 +56,7 @@ pub struct AvailabilityBitfieldRecord<N> {
 
 /// Determines if all checks should be applied or if a subset was already completed
 /// in a code path that will be executed afterwards or was already executed before.
-#[derive(Encode, Decode, PartialEq, Eq, RuntimeDebug, TypeInfo)]
+#[derive(Clone, Copy, Encode, Decode, PartialEq, Eq, RuntimeDebug, TypeInfo)]
 pub(crate) enum FullCheck {
 	/// Yes, do a full check, skip nothing.
 	Yes,
@@ -214,8 +212,18 @@ pub mod pallet {
 
 	#[pallet::error]
 	pub enum Error<T> {
+		/// Validator indices are out of order or contains duplicates.
+		UnsortedOrDuplicateValidatorIndices,
+		/// Dispute statement sets are out of order or contain duplicates.
+		UnsortedOrDuplicateDisputeStatementSet,
+		/// Backed candidates are out of order (core index) or contain duplicates.
+		UnsortedOrDuplicateBackedCandidates,
+		/// A different relay parent was provided compared to the on-chain stored one.
+		UnexpectedRelayParent,
 		/// Availability bitfield has unexpected size.
 		WrongBitfieldSize,
+		/// Bitfield consists of zeros only.
+		BitfieldAllZeros,
 		/// Multiple bitfields submitted by same validator or validators out of order by index.
 		BitfieldDuplicateOrUnordered,
 		/// Validator index out of bounds.
@@ -311,11 +319,12 @@ impl<T: Config> Pallet<T> {
 	/// Extract the freed cores based on cores that became available.
 	///
 	/// Updates storage items `PendingAvailability` and `AvailabilityBitfields`.
-	pub(crate) fn update_pending_availability_and_get_freed_cores<F, const ON_CHAIN_USE: bool>(
+	pub(crate) fn update_pending_availability_and_get_freed_cores<F>(
 		expected_bits: usize,
 		validators: &[ValidatorId],
 		signed_bitfields: UncheckedSignedAvailabilityBitfields,
 		core_lookup: F,
+		enact_candidate: bool,
 	) -> Vec<(CoreIndex, CandidateHash)>
 	where
 		F: Fn(CoreIndex) -> Option<ParaId>,
@@ -387,7 +396,7 @@ impl<T: Config> Pallet<T> {
 					},
 				};
 
-				if ON_CHAIN_USE {
+				if enact_candidate {
 					let receipt = CommittedCandidateReceipt {
 						descriptor: pending_availability.descriptor,
 						commitments,
@@ -420,29 +429,31 @@ impl<T: Config> Pallet<T> {
 		signed_bitfields: UncheckedSignedAvailabilityBitfields,
 		disputed_bitfield: DisputedBitfield,
 		core_lookup: impl Fn(CoreIndex) -> Option<ParaId>,
-	) -> Vec<(CoreIndex, CandidateHash)> {
+		full_check: FullCheck,
+	) -> Result<Vec<(CoreIndex, CandidateHash)>, crate::inclusion::Error<T>> {
 		let validators = shared::Pallet::<T>::active_validator_keys();
 		let session_index = shared::Pallet::<T>::session_index();
 		let parent_hash = frame_system::Pallet::<T>::parent_hash();
 
-		let checked_bitfields = sanitize_bitfields::<T>(
+		let checked_bitfields = crate::paras_inherent::assure_sanity_bitfields::<T>(
 			signed_bitfields,
 			disputed_bitfield,
 			expected_bits,
 			parent_hash,
 			session_index,
 			&validators[..],
-			FullCheck::Yes,
-		);
+			full_check,
+		)?;
 
-		let freed_cores = Self::update_pending_availability_and_get_freed_cores::<_, true>(
+		let freed_cores = Self::update_pending_availability_and_get_freed_cores::<_>(
 			expected_bits,
 			&validators[..],
 			checked_bitfields,
 			core_lookup,
+			true,
 		);
 
-		freed_cores
+		Ok(freed_cores)
 	}
 
 	/// Process candidates that have been backed. Provide the relay storage root, a set of candidates

polkadot/runtime/parachains/src/inclusion/tests.rs

@@ -26,6 +26,7 @@ use crate::{
 	paras_inherent::DisputedBitfield,
 	scheduler::AssignmentKind,
 };
+use assert_matches::assert_matches;
 use frame_support::assert_noop;
 use futures::executor::block_on;
 use keyring::Sr25519Keyring;
@@ -41,7 +42,7 @@ use sc_keystore::LocalKeystore;
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
 use std::sync::Arc;
 use test_helpers::{
-	dummy_candidate_descriptor, dummy_collator, dummy_collator_signature, dummy_hash,
+	dummy_candidate_receipt, dummy_collator, dummy_collator_signature, dummy_hash,
 	dummy_validation_code,
 };
 
@@ -406,17 +407,18 @@ fn bitfield_checks() {
 		// mark all candidates as pending availability
 		let set_pending_av = || {
 			for (p_id, _) in paras {
+				let receipt = dummy_candidate_receipt(dummy_hash());
 				PendingAvailability::<Test>::insert(
 					p_id,
 					CandidatePendingAvailability {
 						availability_votes: default_availability_votes(),
-						core: Default::default(),
-						hash: Default::default(),
-						descriptor: dummy_candidate_descriptor(dummy_hash()),
-						backers: Default::default(),
-						relay_parent_number: Default::default(),
-						backed_in_number: Default::default(),
-						backing_group: Default::default(),
+						core: CoreIndex(0),
+						hash: receipt.hash(),
+						descriptor: receipt.descriptor,
+						backers: BitVec::default(),
+						relay_parent_number: BlockNumber::from(0_u32),
+						backed_in_number: BlockNumber::from(0_u32),
+						backing_group: GroupIndex(0),
 					},
 				)
 			}
@@ -434,14 +436,15 @@ fn bitfield_checks() {
 				&signing_context,
 			));
 
-			assert_eq!(
+			assert_matches!(
 				ParaInclusion::process_bitfields(
 					expected_bits(),
 					vec![signed.into()],
 					DisputedBitfield::zeros(expected_bits()),
 					&core_lookup,
+					FullCheck::Yes,
 				),
-				vec![]
+				Err(Error::<Test>::WrongBitfieldSize)
 			);
 		}
 
@@ -456,14 +459,15 @@ fn bitfield_checks() {
 				&signing_context,
 			));
 
-			assert_eq!(
+			assert_matches!(
 				ParaInclusion::process_bitfields(
 					expected_bits() + 1,
 					vec![signed.into()],
 					DisputedBitfield::zeros(expected_bits()),
 					&core_lookup,
+					FullCheck::Yes,
 				),
-				vec![]
+				Err(Error::<Test>::WrongBitfieldSize)
 			);
 		}
 
@@ -494,20 +498,23 @@ fn bitfield_checks() {
 
 			// the threshold to free a core is 4 availability votes, but we only expect 1 valid
 			// valid bitfield.
-			assert!(ParaInclusion::process_bitfields(
-				expected_bits(),
-				vec![signed.clone(), signed],
-				DisputedBitfield::zeros(expected_bits()),
-				&core_lookup,
-			)
-			.is_empty());
+			assert_matches!(
+				ParaInclusion::process_bitfields(
+					expected_bits(),
+					vec![signed.clone(), signed],
+					DisputedBitfield::zeros(expected_bits()),
+					&core_lookup,
+					FullCheck::Yes,
+				),
+				Err(Error::<Test>::UnsortedOrDuplicateValidatorIndices)
+			);
 
 			assert_eq!(
 				<PendingAvailability<Test>>::get(chain_a)
 					.unwrap()
 					.availability_votes
 					.count_ones(),
-				1
+				0
 			);
 
 			// clean up
@@ -550,20 +557,23 @@ fn bitfield_checks() {
 
 			// the threshold to free a core is 4 availability votes, but we only expect 1 valid
 			// valid bitfield because `signed_0` will get skipped for being out of order.
-			assert!(ParaInclusion::process_bitfields(
-				expected_bits(),
-				vec![signed_1, signed_0],
-				DisputedBitfield::zeros(expected_bits()),
-				&core_lookup,
-			)
-			.is_empty());
+			assert_matches!(
+				ParaInclusion::process_bitfields(
+					expected_bits(),
+					vec![signed_1, signed_0],
+					DisputedBitfield::zeros(expected_bits()),
+					&core_lookup,
+					FullCheck::Yes,
+				),
+				Err(Error::<Test>::UnsortedOrDuplicateValidatorIndices)
+			);
 
 			assert_eq!(
 				<PendingAvailability<Test>>::get(chain_a)
 					.unwrap()
 					.availability_votes
 					.count_ones(),
-				1
+				0
 			);
 
 			PendingAvailability::<Test>::remove_all(None);
@@ -581,13 +591,13 @@ fn bitfield_checks() {
 				&signing_context,
 			));
 
-			assert!(ParaInclusion::process_bitfields(
+			assert_matches!(ParaInclusion::process_bitfields(
 				expected_bits(),
 				vec![signed.into()],
 				DisputedBitfield::zeros(expected_bits()),
 				&core_lookup,
-			)
-			.is_empty());
+				FullCheck::Yes,
+			), Ok(x) => { assert!(x.is_empty())});
 		}
 
 		// empty bitfield signed: always ok, but kind of useless.
@@ -601,13 +611,13 @@ fn bitfield_checks() {
 				&signing_context,
 			));
 
-			assert!(ParaInclusion::process_bitfields(
+			assert_matches!(ParaInclusion::process_bitfields(
 				expected_bits(),
 				vec![signed.into()],
 				DisputedBitfield::zeros(expected_bits()),
 				&core_lookup,
-			)
-			.is_empty());
+				FullCheck::Yes,
+			), Ok(x) => { assert!(x.is_empty())});
 		}
 
 		// bitfield signed with pending bit signed.
@@ -641,13 +651,13 @@ fn bitfield_checks() {
 				&signing_context,
 			));
 
-			assert!(ParaInclusion::process_bitfields(
+			assert_matches!(ParaInclusion::process_bitfields(
 				expected_bits(),
 				vec![signed.into()],
 				DisputedBitfield::zeros(expected_bits()),
 				&core_lookup,
-			)
-			.is_empty());
+				FullCheck::Yes,
+			), Ok(v) => { assert!(v.is_empty())} );
 
 			<PendingAvailability<Test>>::remove(chain_a);
 			PendingAvailabilityCommitments::<Test>::remove(chain_a);
@@ -684,13 +694,13 @@ fn bitfield_checks() {
 			));
 
 			// no core is freed
-			assert!(ParaInclusion::process_bitfields(
+			assert_matches!(ParaInclusion::process_bitfields(
 				expected_bits(),
 				vec![signed.into()],
 				DisputedBitfield::zeros(expected_bits()),
 				&core_lookup,
-			)
-			.is_empty());
+				FullCheck::Yes,
+			), Ok(v) => { assert!(v.is_empty()) });
 		}
 	});
 }
@@ -827,14 +837,17 @@ fn supermajority_bitfields_trigger_availability() {
 			.collect();
 
 		// only chain A's core is freed.
-		assert_eq!(
+		assert_matches!(
 			ParaInclusion::process_bitfields(
 				expected_bits(),
 				signed_bitfields,
 				DisputedBitfield::zeros(expected_bits()),
 				&core_lookup,
+				FullCheck::Yes,
 			),
-			vec![(CoreIndex(0), candidate_a.hash())]
+			Ok(v) => {
+				assert_eq!(vec![(CoreIndex(0), candidate_a.hash())], v);
+			}
 		);
 
 		// chain A had 4 signing off, which is >= threshold.