diff --git a/prdoc/pr_9511.prdoc b/prdoc/pr_9511.prdoc new file mode 100644 index 0000000000000..35aa5f493e047 --- /dev/null +++ b/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 diff --git a/substrate/frame/election-provider-multi-block/src/signed/mod.rs b/substrate/frame/election-provider-multi-block/src/signed/mod.rs index 9784cf75a5765..1aa0dadb2d452 100644 --- a/substrate/frame/election-provider-multi-block/src/signed/mod.rs +++ b/substrate/frame/election-provider-multi-block/src/signed/mod.rs @@ -452,13 +452,13 @@ pub mod pallet { ) -> Result { let mut sorted_scores = SortedScores::::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::::Duplicate.into()); } else { // must be new. debug_assert!(!SubmissionMetadataStorage::::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::::is_invulnerable(x)) + .ok_or(Error::::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::::take(round, &discarded).defensive(); // Note: safe to remove unbounded, as at most `Pages` pages are stored. @@ -486,24 +499,27 @@ pub mod pallet { Pallet::::settle_deposit( &discarded, metadata.deposit, - if Pallet::::is_invulnerable(&discarded) { - One::one() - } else { - T::EjectGraceRatio::get() - }, + T::EjectGraceRatio::get(), ); } Pallet::::deposit_event(Event::::Ejected(round, discarded)); true - }, - Err(_) => return Err(Error::::QueueFull.into()), + } else { + // we don't have a better solution + return Err(Error::::QueueFull.into()) + } + } else { + sorted_scores + .try_insert(insert_idx, record) + .expect("length checked above; qed"); + false } }; SortedScores::::insert(round, sorted_scores); SubmissionMetadataStorage::::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 Pallet { ); debug_assert_eq!(_res, Ok(slash)); Self::deposit_event(Event::::Slashed(current_round, loser.clone(), slash)); + Invulnerables::::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) = diff --git a/substrate/frame/election-provider-multi-block/src/signed/tests.rs b/substrate/frame/election-provider-multi-block/src/signed/tests.rs index cbd64adf84c7d..085cec58935c4 100644 --- a/substrate/frame/election-provider-multi-block/src/signed/tests.rs +++ b/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::::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!( - ::EjectGraceRatio::get(), - Perbill::from_percent(20) + Submissions::::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::::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::::QueueFull, + ); + // inserting someone who would eject 98 will work. + assert_ok!(SignedPallet::register(RuntimeOrigin::signed(96), score_from(155))); + assert_eq!( + Submissions::::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::::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::::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::::QueueFull, + ); + + // better than our first item + hypothetically!({ + assert_ok!(SignedPallet::register(RuntimeOrigin::signed(96), score_from(125))); + assert_eq!( + Submissions::::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::::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::::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::::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::::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::::get().contains(&99)); + }); + } } mod defensive_tests { diff --git a/substrate/frame/staking-async/runtimes/parachain/src/staking.rs b/substrate/frame/staking-async/runtimes/parachain/src/staking.rs index ab5c8f7fe4104..aa9da4dc520f4 100644 --- a/substrate/frame/staking-async/runtimes/parachain/src/staking.rs +++ b/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; diff --git a/substrate/frame/staking-async/src/pallet/impls.rs b/substrate/frame/staking-async/src/pallet/impls.rs index 13bc247bcd6a0..8219aa1293f42 100644 --- a/substrate/frame/staking-async/src/pallet/impls.rs +++ b/substrate/frame/staking-async/src/pallet/impls.rs @@ -737,11 +737,6 @@ impl Pallet { .defensive_unwrap_or_default(); } Nominators::::insert(who, nominations); - - debug_assert_eq!( - Nominators::::count() + Validators::::count(), - T::VoterList::count() - ); } /// This function will remove a nominator from the `Nominators` storage map, @@ -761,11 +756,6 @@ impl Pallet { false }; - debug_assert_eq!( - Nominators::::count() + Validators::::count(), - T::VoterList::count() - ); - outcome } @@ -782,11 +772,6 @@ impl Pallet { let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who)); } Validators::::insert(who, prefs); - - debug_assert_eq!( - Nominators::::count() + Validators::::count(), - T::VoterList::count() - ); } /// This function will remove a validator from the `Validators` storage map. @@ -805,11 +790,6 @@ impl Pallet { false }; - debug_assert_eq!( - Nominators::::count() + Validators::::count(), - T::VoterList::count() - ); - outcome }