EPMB/Signed: Make invulnerables non-eject-able

prdoc/pr_9511.prdoc

@@ -0,0 +1,11 @@
+title: 'EPMB/Signed: Make invulnerables non-eject-able'
+doc:
+- audience: Runtime Dev
+  description: 'Follow-up to https://github.com/paritytech/polkadot-sdk/pull/8877
+    and audits: Make it such that invulnerable accounts cannot be ejected from the
+    election signed queue altogether. '
+crates:
+- name: pallet-election-provider-multi-block
+  bump: patch
+- name: pallet-staking-async
+  bump: patch

substrate/frame/election-provider-multi-block/src/signed/mod.rs

@@ -452,13 +452,13 @@ pub mod pallet {
 		) -> Result<bool, DispatchError> {
 			let mut sorted_scores = SortedScores::<T>::get(round);
 
-			let discarded = if let Some(_) = sorted_scores.iter().position(|(x, _)| x == who) {
+			let did_eject = if let Some(_) = sorted_scores.iter().position(|(x, _)| x == who) {
 				return Err(Error::<T>::Duplicate.into());
 			} else {
 				// must be new.
 				debug_assert!(!SubmissionMetadataStorage::<T>::contains_key(round, who));
 
-				let pos = match sorted_scores
+				let insert_idx = match sorted_scores
 					.binary_search_by_key(&metadata.claimed_score, |(_, y)| *y)
 				{
 					// an equal score exists, unlikely, but could very well happen. We just put them
@@ -468,10 +468,23 @@ pub mod pallet {
 					Err(pos) => pos,
 				};
 
-				let record = (who.clone(), metadata.claimed_score);
-				match sorted_scores.force_insert_keep_right(pos, record) {
-					Ok(None) => false,
-					Ok(Some((discarded, _score))) => {
+				let mut record = (who.clone(), metadata.claimed_score);
+				if sorted_scores.is_full() {
+					let remove_idx = sorted_scores
+						.iter()
+						.position(|(x, _)| !Pallet::<T>::is_invulnerable(x))
+						.ok_or(Error::<T>::QueueFull)?;
+					if insert_idx > remove_idx {
+						// we have a better solution
+						sp_std::mem::swap(&mut sorted_scores[remove_idx], &mut record);
+						// slicing safety note:
+						// - `insert_idx` is at most `sorted_scores.len()`, obtained from
+						//   `binary_search_by_key`, valid for the upper bound of slicing.
+						// - `remove_idx` is a valid index, less then `insert_idx`, obtained from
+						//   `.iter().position()`
+						sorted_scores[remove_idx..insert_idx].rotate_left(1);
+
+						let discarded = record.0;
 						let maybe_metadata =
 							SubmissionMetadataStorage::<T>::take(round, &discarded).defensive();
 						// Note: safe to remove unbounded, as at most `Pages` pages are stored.
@@ -486,24 +499,27 @@ pub mod pallet {
 							Pallet::<T>::settle_deposit(
 								&discarded,
 								metadata.deposit,
-								if Pallet::<T>::is_invulnerable(&discarded) {
-									One::one()
-								} else {
-									T::EjectGraceRatio::get()
-								},
+								T::EjectGraceRatio::get(),
 							);
 						}
 
 						Pallet::<T>::deposit_event(Event::<T>::Ejected(round, discarded));
 						true
-					},
-					Err(_) => return Err(Error::<T>::QueueFull.into()),
+					} else {
+						// we don't have a better solution
+						return Err(Error::<T>::QueueFull.into())
+					}
+				} else {
+					sorted_scores
+						.try_insert(insert_idx, record)
+						.expect("length checked above; qed");
+					false
 				}
 			};
 
 			SortedScores::<T>::insert(round, sorted_scores);
 			SubmissionMetadataStorage::<T>::insert(round, who, metadata);
-			Ok(discarded)
+			Ok(did_eject)
 		}
 
 		/// Submit a page of `solution` to the `page` index of `who`'s submission.
@@ -1020,6 +1036,7 @@ impl<T: Config> Pallet<T> {
 			);
 			debug_assert_eq!(_res, Ok(slash));
 			Self::deposit_event(Event::<T>::Slashed(current_round, loser.clone(), slash));
+			Invulnerables::<T>::mutate(|x| x.retain(|y| y != &loser));
 
 			// Try to start verification again if we still have submissions
 			if let crate::types::Phase::SignedValidation(remaining_blocks) =

substrate/frame/election-provider-multi-block/src/signed/tests.rs

@@ -29,6 +29,10 @@ use sp_npos_elections::ElectionScore;
 
 pub type T = Runtime;
 
+fn score_from(x: u128) -> ElectionScore {
+	ElectionScore { minimal_stake: x, ..Default::default() }
+}
+
 mod calls {
 	use super::*;
 	use sp_runtime::{DispatchError, TokenError::FundsUnavailable};
@@ -213,13 +217,11 @@ mod calls {
 	}
 
 	#[test]
-	fn metadata_submission_sorted_based_on_stake() {
+	fn metadata_submission_sorted_based_on_score() {
 		ExtBuilder::signed().build_and_execute(|| {
 			roll_to_signed_open();
 			assert_full_snapshot();
 
-			let score_from = |x| ElectionScore { minimal_stake: x, ..Default::default() };
-
 			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(91), score_from(100)));
 			assert_eq!(*Submissions::<Runtime>::leaderboard(0), vec![(91, score_from(100))]);
 			assert_eq!(balances(91), (95, 5));
@@ -1259,6 +1261,8 @@ mod e2e {
 }
 
 mod invulnerables {
+	use frame_support::hypothetically;
+
 	use super::*;
 
 	fn make_invulnerable(who: AccountId) {
@@ -1395,73 +1399,102 @@ mod invulnerables {
 	}
 
 	#[test]
-	fn ejected_invulnerable_gets_deposit_back() {
-		ExtBuilder::signed().max_signed_submissions(2).build_and_execute(|| {
+	fn invulnerable_cannot_eject_first() {
+		ExtBuilder::signed().max_signed_submissions(3).build_and_execute(|| {
 			roll_to_signed_open();
 			assert_full_snapshot();
 			make_invulnerable(99);
 
-			// by default, we pay back 20% of the discarded deposit back
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(99), score_from(100)));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(98), score_from(150)));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(97), score_from(200)));
+
 			assert_eq!(
-				<Runtime as crate::signed::Config>::EjectGraceRatio::get(),
-				Perbill::from_percent(20)
+				Submissions::<Runtime>::leaderboard(0),
+				vec![(99, score_from(100)), (98, score_from(150)), (97, score_from(200))]
 			);
 
-			// submit 99 as invulnerable
-			assert_ok!(SignedPallet::register(
-				RuntimeOrigin::signed(99),
-				ElectionScore { minimal_stake: 100, ..Default::default() }
-			));
-			assert!(matches!(
-				Submissions::<Runtime>::metadata_of(0, 99).unwrap(),
-				SubmissionMetadata { deposit: 7, .. }
-			));
+			// inserting someone who would eject 99 won't work.
+			assert_noop!(
+				SignedPallet::register(RuntimeOrigin::signed(96), score_from(100)),
+				Error::<T>::QueueFull,
+			);
+			// inserting someone who would eject 98 will work.
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(96), score_from(155)));
+			assert_eq!(
+				Submissions::<Runtime>::leaderboard(0),
+				vec![(99, score_from(100)), (96, score_from(155)), (97, score_from(200))]
+			);
+		})
+	}
 
-			// submit 98 as normal
-			assert_ok!(SignedPallet::register(
-				RuntimeOrigin::signed(98),
-				ElectionScore { minimal_stake: 101, ..Default::default() }
-			));
-			assert!(matches!(
-				Submissions::<Runtime>::metadata_of(0, 98).unwrap(),
-				SubmissionMetadata { deposit: 5, .. }
-			));
-			let _ = signed_events_since_last_call();
-			assert_eq!(balances(99), (93, 7));
-			assert_eq!(balances(98), (95, 5));
+	#[test]
+	fn invulnerable_cannot_eject_middle() {
+		ExtBuilder::signed().max_signed_submissions(3).build_and_execute(|| {
+			roll_to_signed_open();
+			assert_full_snapshot();
+			make_invulnerable(99);
 
-			// submit 97 and 96 with higher scores, eject both of the previous ones
-			assert_ok!(SignedPallet::register(
-				RuntimeOrigin::signed(97),
-				ElectionScore { minimal_stake: 200, ..Default::default() }
-			));
-			assert_ok!(SignedPallet::register(
-				RuntimeOrigin::signed(96),
-				ElectionScore { minimal_stake: 201, ..Default::default() }
-			));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(99), score_from(150)));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(98), score_from(100)));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(97), score_from(200)));
 
 			assert_eq!(
-				signed_events_since_last_call(),
-				vec![
-					Event::Ejected(0, 99),
-					Event::Registered(
-						0,
-						97,
-						ElectionScore { minimal_stake: 200, sum_stake: 0, sum_stake_squared: 0 }
-					),
-					Event::Ejected(0, 98),
-					Event::Registered(
-						0,
-						96,
-						ElectionScore { minimal_stake: 201, sum_stake: 0, sum_stake_squared: 0 }
-					)
-				]
+				Submissions::<Runtime>::leaderboard(0),
+				vec![(98, score_from(100)), (99, score_from(150)), (97, score_from(200))]
 			);
 
-			// 99 gets everything back
-			assert_eq!(balances(99), (100, 0));
-			// 98 gets 20% x 5 = 1 back
-			assert_eq!(balances(98), (96, 0));
+			// worse than all
+			assert_noop!(
+				SignedPallet::register(RuntimeOrigin::signed(96), score_from(50)),
+				Error::<T>::QueueFull,
+			);
+
+			// better than our first item
+			hypothetically!({
+				assert_ok!(SignedPallet::register(RuntimeOrigin::signed(96), score_from(125)));
+				assert_eq!(
+					Submissions::<Runtime>::leaderboard(0),
+					vec![(96, score_from(125)), (99, score_from(150)), (97, score_from(200))]
+				);
+			});
+
+			// better than inv
+			hypothetically!({
+				assert_ok!(SignedPallet::register(RuntimeOrigin::signed(96), score_from(175)));
+				assert_eq!(
+					Submissions::<Runtime>::leaderboard(0),
+					vec![(99, score_from(150)), (96, score_from(175)), (97, score_from(200))]
+				);
+			});
+
+			// better than all
+			hypothetically!({
+				assert_ok!(SignedPallet::register(RuntimeOrigin::signed(96), score_from(225)));
+				assert_eq!(
+					Submissions::<Runtime>::leaderboard(0),
+					vec![(99, score_from(150)), (97, score_from(200)), (96, score_from(225))]
+				);
+			});
+		})
+	}
+
+	#[test]
+	fn all_invulnerables() {
+		ExtBuilder::signed().max_signed_submissions(3).build_and_execute(|| {
+			roll_to_signed_open();
+			assert_full_snapshot();
+			assert_ok!(SignedPallet::set_invulnerables(RuntimeOrigin::root(), vec![99, 98, 97]));
+
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(99), score_from(150)));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(98), score_from(100)));
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(97), score_from(200)));
+
+			// Nothing can touch our boys now.
+			assert_noop!(
+				SignedPallet::register(RuntimeOrigin::signed(96), score_from(1000)),
+				Error::<T>::QueueFull,
+			);
 		})
 	}
 
@@ -1559,6 +1592,48 @@ mod invulnerables {
 			assert_eq!(new_deposit, 5); // Should not be fixed deposit anymore
 		});
 	}
+
+	#[test]
+	fn slashed_invulnerable_is_expelled() {
+		ExtBuilder::signed().build_and_execute(|| {
+			roll_to_signed_open();
+			assert_full_snapshot();
+			make_invulnerable(99);
+			assert!(Invulnerables::<T>::get().contains(&99));
+
+			let invalid_score = score_from(777);
+			assert_ok!(SignedPallet::register(RuntimeOrigin::signed(99), invalid_score));
+
+			roll_to_signed_validation_open();
+			roll_next(); // one block so signed pallet will send start signal.
+			roll_to_full_verification();
+
+			// Check that rejection events were properly generated
+			assert_eq!(
+				verifier_events_since_last_call(),
+				vec![
+					crate::verifier::Event::Verified(2, 0), // empty page
+					crate::verifier::Event::Verified(1, 0), // empty page
+					crate::verifier::Event::Verified(0, 0), // empty page
+					crate::verifier::Event::VerificationFailed(0, FeasibilityError::InvalidScore),
+				]
+			);
+
+			// Check that expected events were emitted for the rejection
+			assert_eq!(
+				signed_events(),
+				vec![Event::Registered(0, 99, invalid_score), Event::Slashed(0, 99, 7)] /* slash
+				                                                                         * amount
+				                                                                         * is indeed
+				                                                                         * the invulnerable
+				                                                                         * deposit
+				                                                                         * (7) ^^^^ */
+			);
+
+			// Verify invulnerable is expelled
+			assert!(!Invulnerables::<T>::get().contains(&99));
+		});
+	}
 }
 
 mod defensive_tests {

substrate/frame/staking-async/runtimes/parachain/src/staking.rs

@@ -81,8 +81,10 @@ parameter_types! {
 	/// better.
 	pub storage Pages: u32 = 4;
 
-	/// * Polkadot: 8 * 32 (256 blocks, 25.6m). Enough time to verify up to 8 solutions.
-	/// * Kusama: 4 * 16 (64 blocks, 6.4m). Enough time to verify up to 4 solutions.
+	/// * Polkadot: 16 * 32 (512 blocks, 51.2m).
+	/// * Kusama: 8 * 16 (12 blocks, 12.8m).
+	///
+	/// (MaxSubmissions * Pages) for both, enough to verify all solutions.
 	///
 	/// Reasoning: Less security needed in Kusama, to compensate for the shorter session duration.
 	pub storage SignedValidationPhase: u32 = Pages::get() * 2;