diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index a8195df7305ff..a0fd0a9a0512f 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -320,21 +320,14 @@ frame_benchmarking::benchmarks! { } submit { - // the solution will be worse than all of them meaning the score need to be checked against - // ~ log2(c) - let solution = RawSolution { - score: ElectionScore { minimal_stake: 10_000_000u128 - 1, ..Default::default() }, - ..Default::default() - }; - + // the queue is full and the solution is only better than the worse. >::create_snapshot().map_err(<&str>::from)?; MultiPhase::::on_initialize_open_signed(); >::put(1); let mut signed_submissions = SignedSubmissions::::get(); - // Insert `max - 1` submissions because the call to `submit` will insert another - // submission and the score is worse then the previous scores. + // Insert `max` submissions for i in 0..(T::SignedMaxSubmissions::get() - 1) { let raw_solution = RawSolution { score: ElectionScore { minimal_stake: 10_000_000u128 + (i as u128), ..Default::default() }, @@ -350,6 +343,12 @@ frame_benchmarking::benchmarks! { } signed_submissions.put(); + // this score will eject the weakest one. + let solution = RawSolution { + score: ElectionScore { minimal_stake: 10_000_000u128 + 1, ..Default::default() }, + ..Default::default() + }; + let caller = frame_benchmarking::whitelisted_caller(); let deposit = MultiPhase::::deposit_for( &solution, diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 3dc6161bb202a..3e5a6d12f575c 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -267,6 +267,7 @@ pub mod helpers; const LOG_TARGET: &str = "runtime::election-provider"; +pub mod migrations; pub mod signed; pub mod unsigned; pub mod weights; @@ -1265,8 +1266,8 @@ pub mod pallet { #[pallet::storage] pub type SignedSubmissionNextIndex = StorageValue<_, u32, ValueQuery>; - /// A sorted, bounded set of `(score, index)`, where each `index` points to a value in - /// `SignedSubmissions`. + /// A sorted, bounded vector of `(score, block_number, index)`, where each `index` points to a + /// value in `SignedSubmissions`. /// /// We never need to process more than a single signed submission at a time. Signed submissions /// can be quite large, so we're willing to pay the cost of multiple database accesses to access @@ -1296,9 +1297,14 @@ pub mod pallet { #[pallet::getter(fn minimum_untrusted_score)] pub type MinimumUntrustedScore = StorageValue<_, ElectionScore>; + /// The current storage version. + /// + /// v1: https://github.com/paritytech/substrate/pull/12237/ + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + #[pallet::pallet] - #[pallet::generate_store(pub(super) trait Store)] #[pallet::without_storage_info] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(PhantomData); } diff --git a/frame/election-provider-multi-phase/src/migrations.rs b/frame/election-provider-multi-phase/src/migrations.rs new file mode 100644 index 0000000000000..77efe0d0c5e92 --- /dev/null +++ b/frame/election-provider-multi-phase/src/migrations.rs @@ -0,0 +1,78 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +pub mod v1 { + use frame_support::{ + storage::unhashed, + traits::{Defensive, GetStorageVersion, OnRuntimeUpgrade}, + BoundedVec, + }; + use sp_std::collections::btree_map::BTreeMap; + + use crate::*; + pub struct MigrateToV1(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::current_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with current storage version {:?} / onchain {:?}", + current, + onchain + ); + + if current == 1 && onchain == 0 { + if SignedSubmissionIndices::::exists() { + // This needs to be tested at a both a block height where this value exists, and + // when it doesn't. + let now = frame_system::Pallet::::block_number(); + let map = unhashed::get::>( + &SignedSubmissionIndices::::hashed_key(), + ) + .defensive_unwrap_or_default(); + let vector = map + .into_iter() + .map(|(score, index)| (score, now, index)) + .collect::>(); + + log!( + debug, + "{:?} SignedSubmissionIndices read from storage (max: {:?})", + vector.len(), + T::SignedMaxSubmissions::get() + ); + + // defensive-only, assuming a constant `SignedMaxSubmissions`. + let bounded = BoundedVec::<_, _>::truncate_from(vector); + SignedSubmissionIndices::::put(bounded); + + log!(info, "SignedSubmissionIndices existed and got migrated"); + } else { + log!(info, "SignedSubmissionIndices did NOT exist."); + } + + current.put::>(); + T::DbWeight::get().reads_writes(2, 1) + } else { + log!(info, "Migration did not execute. This probably should be removed"); + T::DbWeight::get().reads(1) + } + } + } +} diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index e04e0bf474caf..f645886d78899 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -571,6 +571,12 @@ impl ExtBuilder { balances: vec![ // bunch of account for submitting stuff only. (99, 100), + (100, 100), + (101, 100), + (102, 100), + (103, 100), + (104, 100), + (105, 100), (999, 100), (9999, 100), ], diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 175c92757f35e..cda87dd6c839a 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -24,11 +24,11 @@ use crate::{ }; use codec::{Decode, Encode, HasCompact}; use frame_election_provider_support::NposSolution; -use frame_support::{ - storage::bounded_btree_map::BoundedBTreeMap, - traits::{defensive_prelude::*, Currency, Get, OnUnbalanced, ReservableCurrency}, +use frame_support::traits::{ + defensive_prelude::*, Currency, Get, OnUnbalanced, ReservableCurrency, }; use sp_arithmetic::traits::SaturatedConversion; +use sp_core::bounded::BoundedVec; use sp_npos_elections::ElectionScore; use sp_runtime::{ traits::{Saturating, Zero}, @@ -37,7 +37,6 @@ use sp_runtime::{ use sp_std::{ cmp::Ordering, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, - ops::Deref, vec::Vec, }; @@ -99,8 +98,12 @@ pub type SignedSubmissionOf = SignedSubmission< <::MinerConfig as MinerConfig>::Solution, >; -pub type SubmissionIndicesOf = - BoundedBTreeMap::SignedMaxSubmissions>; +/// Always sorted vector of a score, submitted at the given block number, which can be found at the +/// given index (`u32`) of the `SignedSubmissionsMap`. +pub type SubmissionIndicesOf = BoundedVec< + (ElectionScore, ::BlockNumber, u32), + ::SignedMaxSubmissions, +>; /// Outcome of [`SignedSubmissions::insert`]. pub enum InsertResult { @@ -126,6 +129,16 @@ pub struct SignedSubmissions { } impl SignedSubmissions { + /// `true` if the structure is empty. + pub fn is_empty(&self) -> bool { + self.indices.is_empty() + } + + /// Get the length of submitted solutions. + pub fn len(&self) -> usize { + self.indices.len() + } + /// Get the signed submissions from storage. pub fn get() -> Self { let submissions = SignedSubmissions { @@ -134,10 +147,12 @@ impl SignedSubmissions { insertion_overlay: BTreeMap::new(), deletion_overlay: BTreeSet::new(), }; + // validate that the stored state is sane debug_assert!(submissions .indices - .values() + .iter() + .map(|(_, _, index)| index) .copied() .max() .map_or(true, |max_idx| submissions.next_idx > max_idx,)); @@ -155,7 +170,8 @@ impl SignedSubmissions { .map_or(true, |max_idx| self.next_idx > max_idx,)); debug_assert!(self .indices - .values() + .iter() + .map(|(_, _, index)| index) .copied() .max() .map_or(true, |max_idx| self.next_idx > max_idx,)); @@ -174,9 +190,9 @@ impl SignedSubmissions { /// Get the submission at a particular index. fn get_submission(&self, index: u32) -> Option> { if self.deletion_overlay.contains(&index) { - // Note: can't actually remove the item from the insertion overlay (if present) - // because we don't want to use `&mut self` here. There may be some kind of - // `RefCell` optimization possible here in the future. + // Note: can't actually remove the item from the insertion overlay (if present) because + // we don't want to use `&mut self` here. There may be some kind of `RefCell` + // optimization possible here in the future. None } else { self.insertion_overlay @@ -188,27 +204,30 @@ impl SignedSubmissions { /// Perform three operations: /// - /// - Remove a submission (identified by score) - /// - Insert a new submission (identified by score and insertion index) - /// - Return the submission which was removed. - /// - /// Note: in the case that `weakest_score` is not present in `self.indices`, this will return - /// `None` without inserting the new submission and without further notice. + /// - Remove the solution at the given position of `self.indices`. + /// - Insert a new submission (identified by score and insertion index), if provided. + /// - Return the submission which was removed, if any. /// - /// Note: this does not enforce any ordering relation between the submission removed and that - /// inserted. + /// The call site must ensure that `remove_pos` is a valid index. If otherwise, `None` is + /// silently returned. /// /// Note: this doesn't insert into `insertion_overlay`, the optional new insertion must be - /// inserted into `insertion_overlay` to keep the variable `self` in a valid state. + /// inserted into `insertion_overlay` to keep the variable `self` in a valid state. fn swap_out_submission( &mut self, - remove_score: ElectionScore, - insert: Option<(ElectionScore, u32)>, + remove_pos: usize, + insert: Option<(ElectionScore, T::BlockNumber, u32)>, ) -> Option> { - let remove_index = self.indices.remove(&remove_score)?; - if let Some((insert_score, insert_idx)) = insert { + if remove_pos >= self.indices.len() { + return None + } + + // safe: index was just checked in the line above. + let (_, _, remove_index) = self.indices.remove(remove_pos); + + if let Some((insert_score, block_number, insert_idx)) = insert { self.indices - .try_insert(insert_score, insert_idx) + .try_push((insert_score, block_number, insert_idx)) .expect("just removed an item, we must be under capacity; qed"); } @@ -222,20 +241,17 @@ impl SignedSubmissions { }) } + /// Remove the signed submission with the highest score from the set. + pub fn pop_last(&mut self) -> Option> { + let best_index = self.indices.len().checked_sub(1)?; + self.swap_out_submission(best_index, None) + } + /// Iterate through the set of signed submissions in order of increasing score. pub fn iter(&self) -> impl '_ + Iterator> { - self.indices.iter().filter_map(move |(_score, &idx)| { - let maybe_submission = self.get_submission(idx); - if maybe_submission.is_none() { - log!( - error, - "SignedSubmissions internal state is invalid (idx {}); \ - there is a logic error in code handling signed solution submissions", - idx, - ) - } - maybe_submission - }) + self.indices + .iter() + .filter_map(move |(_score, _bn, idx)| self.get_submission(*idx).defensive()) } /// Empty the set of signed submissions, returning an iterator of signed submissions in @@ -283,68 +299,54 @@ impl SignedSubmissions { /// to `is_score_better`, we do not change anything. pub fn insert(&mut self, submission: SignedSubmissionOf) -> InsertResult { // verify the expectation that we never reuse an index - debug_assert!(!self.indices.values().any(|&idx| idx == self.next_idx)); - - let weakest = match self.indices.try_insert(submission.raw_solution.score, self.next_idx) { - Ok(Some(prev_idx)) => { - // a submission of equal score was already present in the set; - // no point editing the actual backing map as we know that the newer solution can't - // be better than the old. However, we do need to put the old value back. - self.indices - .try_insert(submission.raw_solution.score, prev_idx) - .expect("didn't change the map size; qed"); - return InsertResult::NotInserted - }, - Ok(None) => { - // successfully inserted into the set; no need to take out weakest member - None - }, - Err((insert_score, insert_idx)) => { - // could not insert into the set because it is full. - // note that we short-circuit return here in case the iteration produces `None`. - // If there wasn't a weakest entry to remove, then there must be a capacity of 0, - // which means that we can't meaningfully proceed. - let weakest_score = match self.indices.iter().next() { + debug_assert!(!self.indices.iter().map(|(_, _, x)| x).any(|&idx| idx == self.next_idx)); + let block_number = frame_system::Pallet::::block_number(); + + let maybe_weakest = match self.indices.try_push(( + submission.raw_solution.score, + block_number, + self.next_idx, + )) { + Ok(_) => None, + Err(_) => { + // the queue is full -- if this is better, insert it. + let weakest_score = match self.indices.iter().next().defensive() { None => return InsertResult::NotInserted, - Some((score, _)) => *score, + Some((score, _, _)) => *score, }; let threshold = T::BetterSignedThreshold::get(); // if we haven't improved on the weakest score, don't change anything. - if !insert_score.strict_threshold_better(weakest_score, threshold) { + if !submission.raw_solution.score.strict_threshold_better(weakest_score, threshold) + { return InsertResult::NotInserted } - self.swap_out_submission(weakest_score, Some((insert_score, insert_idx))) + self.swap_out_submission( + 0, // swap out the worse one, which is always index 0. + Some((submission.raw_solution.score, block_number, self.next_idx)), + ) }, }; + // this is the ONLY place that we insert, and we sort post insertion. If scores are the + // same, we sort based on reverse of submission block number. + self.indices + .sort_by(|(score1, bn1, _), (score2, bn2, _)| match score1.cmp(score2) { + Ordering::Equal => bn1.cmp(&bn2).reverse(), + x => x, + }); + // we've taken out the weakest, so update the storage map and the next index debug_assert!(!self.insertion_overlay.contains_key(&self.next_idx)); self.insertion_overlay.insert(self.next_idx, submission); debug_assert!(!self.deletion_overlay.contains(&self.next_idx)); self.next_idx += 1; - match weakest { + match maybe_weakest { Some(weakest) => InsertResult::InsertedEjecting(weakest), None => InsertResult::Inserted, } } - - /// Remove the signed submission with the highest score from the set. - pub fn pop_last(&mut self) -> Option> { - let (score, _) = self.indices.iter().rev().next()?; - // deref in advance to prevent mutable-immutable borrow conflict - let score = *score; - self.swap_out_submission(score, None) - } -} - -impl Deref for SignedSubmissions { - type Target = SubmissionIndicesOf; - - fn deref(&self) -> &Self::Target { - &self.indices - } } impl Pallet { @@ -379,6 +381,12 @@ impl Pallet { Self::snapshot_metadata().unwrap_or_default(); while let Some(best) = all_submissions.pop_last() { + log!( + debug, + "finalized_signed: trying to verify from {:?} score {:?}", + best.who, + best.raw_solution.score + ); let SignedSubmission { raw_solution, who, deposit, call_fee } = best; let active_voters = raw_solution.solution.voter_count() as u32; let feasibility_weight = { @@ -386,6 +394,7 @@ impl Pallet { let desired_targets = Self::desired_targets().defensive_unwrap_or_default(); T::WeightInfo::feasibility_check(voters, targets, active_voters, desired_targets) }; + // the feasibility check itself has some weight weight = weight.saturating_add(feasibility_weight); match Self::feasibility_check(raw_solution, ElectionCompute::Signed) { @@ -397,12 +406,14 @@ impl Pallet { call_fee, ); found_solution = true; + log!(debug, "finalized_signed: found a valid solution"); weight = weight .saturating_add(T::WeightInfo::finalize_signed_phase_accept_solution()); break }, Err(_) => { + log!(warn, "finalized_signed: invalid signed submission found, slashing."); Self::finalize_signed_phase_reject_solution(&who, deposit); weight = weight .saturating_add(T::WeightInfo::finalize_signed_phase_reject_solution()); @@ -526,14 +537,7 @@ impl Pallet { #[cfg(test)] mod tests { use super::*; - use crate::{ - mock::{ - balances, multi_phase_events, raw_solution, roll_to, roll_to_signed, Balances, - ExtBuilder, MockedWeightInfo, MultiPhase, Runtime, RuntimeOrigin, SignedMaxRefunds, - SignedMaxSubmissions, SignedMaxWeight, - }, - Error, Event, Perbill, Phase, - }; + use crate::{mock::*, ElectionCompute, Error, Event, Perbill, Phase}; use frame_support::{assert_noop, assert_ok, assert_storage_noop}; #[test] @@ -868,8 +872,8 @@ mod tests { } #[test] - fn replace_weakest_works() { - ExtBuilder::default().build_and_execute(|| { + fn replace_weakest_by_score_works() { + ExtBuilder::default().signed_max_submission(3).build_and_execute(|| { roll_to_signed(); assert!(MultiPhase::current_phase().is_signed()); @@ -893,7 +897,7 @@ mod tests { .iter() .map(|s| s.raw_solution.score.minimal_stake) .collect::>(), - vec![4, 6, 7, 8, 9], + vec![4, 6, 7], ); // better. @@ -909,7 +913,7 @@ mod tests { .iter() .map(|s| s.raw_solution.score.minimal_stake) .collect::>(), - vec![5, 6, 7, 8, 9], + vec![5, 6, 7], ); }) } @@ -946,7 +950,8 @@ mod tests { } #[test] - fn equally_good_solution_is_not_accepted() { + fn equally_good_solution_is_not_accepted_when_queue_full() { + // because in ordering of solutions, an older solution has higher priority and should stay. ExtBuilder::default().signed_max_submission(3).build_and_execute(|| { roll_to_signed(); assert!(MultiPhase::current_phase().is_signed()); @@ -958,6 +963,7 @@ mod tests { }; assert_ok!(MultiPhase::submit(RuntimeOrigin::signed(99), Box::new(solution))); } + assert_eq!( MultiPhase::signed_submissions() .iter() @@ -978,6 +984,99 @@ mod tests { }) } + #[test] + fn equally_good_solution_is_accepted_when_queue_not_full() { + // because in ordering of solutions, an older solution has higher priority and should stay. + ExtBuilder::default().signed_max_submission(3).build_and_execute(|| { + roll_to(15); + assert!(MultiPhase::current_phase().is_signed()); + + let solution = RawSolution { + score: ElectionScore { minimal_stake: 5, ..Default::default() }, + ..Default::default() + }; + assert_ok!(MultiPhase::submit(RuntimeOrigin::signed(99), Box::new(solution))); + + assert_eq!( + MultiPhase::signed_submissions() + .iter() + .map(|s| (s.who, s.raw_solution.score.minimal_stake,)) + .collect::>(), + vec![(99, 5)] + ); + + roll_to(16); + let solution = RawSolution { + score: ElectionScore { minimal_stake: 5, ..Default::default() }, + ..Default::default() + }; + assert_ok!(MultiPhase::submit(RuntimeOrigin::signed(999), Box::new(solution))); + + assert_eq!( + MultiPhase::signed_submissions() + .iter() + .map(|s| (s.who, s.raw_solution.score.minimal_stake,)) + .collect::>(), + vec![(999, 5), (99, 5)] + ); + + let solution = RawSolution { + score: ElectionScore { minimal_stake: 6, ..Default::default() }, + ..Default::default() + }; + assert_ok!(MultiPhase::submit(RuntimeOrigin::signed(9999), Box::new(solution))); + + assert_eq!( + MultiPhase::signed_submissions() + .iter() + .map(|s| (s.who, s.raw_solution.score.minimal_stake,)) + .collect::>(), + vec![(999, 5), (99, 5), (9999, 6)] + ); + }) + } + + #[test] + fn all_equal_score() { + // because in ordering of solutions, an older solution has higher priority and should stay. + ExtBuilder::default().signed_max_submission(3).build_and_execute(|| { + roll_to(15); + assert!(MultiPhase::current_phase().is_signed()); + + for i in 0..SignedMaxSubmissions::get() { + roll_to((15 + i).into()); + let solution = raw_solution(); + assert_ok!(MultiPhase::submit( + RuntimeOrigin::signed(100 + i as AccountId), + Box::new(solution) + )); + } + + assert_eq!( + MultiPhase::signed_submissions() + .iter() + .map(|s| (s.who, s.raw_solution.score.minimal_stake)) + .collect::>(), + vec![(102, 40), (101, 40), (100, 40)] + ); + + roll_to(25); + + // The first one that will actually get verified is the last one. + assert_eq!( + multi_phase_events(), + vec![ + Event::SignedPhaseStarted { round: 1 }, + Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false }, + Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false }, + Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false }, + Event::Rewarded { account: 100, value: 7 }, + Event::UnsignedPhaseStarted { round: 1 } + ] + ); + }) + } + #[test] fn all_in_one_signed_submission_scenario() { // a combination of: @@ -991,6 +1090,7 @@ mod tests { assert_eq!(balances(&99), (100, 0)); assert_eq!(balances(&999), (100, 0)); assert_eq!(balances(&9999), (100, 0)); + let solution = raw_solution(); // submit a correct one.