diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index b2f1c9bb4c8a..7988163e98f6 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -273,19 +273,16 @@ frame_benchmarking::benchmarks! { MultiPhase::::on_initialize_open_signed().expect("should be ok to start signed phase"); >::put(1); - >::mutate(|queue| { - for i in 0..c { - let solution = RawSolution { - score: [(10_000_000 + i).into(), 0, 0], - ..Default::default() - }; - let signed_submission = SignedSubmission { solution, ..Default::default() }; - queue.push(signed_submission); - } - // as of here, the queue is ordered worst-to-best. - // However, we have an invariant that it should be ordered best-to-worst - queue.reverse(); - }); + let mut signed_submissions = SignedSubmissions::::get(); + for i in 0..c { + let solution = RawSolution { + score: [(10_000_000 + i).into(), 0, 0], + ..Default::default() + }; + let signed_submission = SignedSubmission { solution, ..Default::default() }; + signed_submissions.insert(signed_submission); + } + signed_submissions.put(); let caller = frame_benchmarking::whitelisted_caller(); T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance() * 10u32.into()); diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 2d8fee6b1e2e..e599f4cfaa03 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -249,14 +249,16 @@ pub mod helpers; const LOG_TARGET: &'static str = "runtime::election-provider"; -pub mod unsigned; pub mod signed; +pub mod unsigned; pub mod weights; +pub use signed::{ + BalanceOf, NegativeImbalanceOf, PositiveImbalanceOf, SignedSubmission, SignedSubmissionOf, + SignedSubmissions, SubmissionIndicesOf, +}; pub use weights::WeightInfo; -pub use signed::{SignedSubmission, BalanceOf, NegativeImbalanceOf, PositiveImbalanceOf}; - /// The compact solution type used by this crate. pub type CompactOf = ::CompactSolution; @@ -387,7 +389,7 @@ impl Default for ElectionCompute { /// /// Such a solution should never become effective in anyway before being checked by the /// `Pallet::feasibility_check` -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug)] +#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, PartialOrd, Ord)] pub struct RawSolution { /// Compact election edges. pub compact: C, @@ -562,6 +564,7 @@ pub mod pallet { /// Maximum number of signed submissions that can be queued. #[pallet::constant] type SignedMaxSubmissions: Get; + /// Maximum weight of a signed solution. /// /// This should probably be similar to [`Config::MinerMaxWeight`]. @@ -603,6 +606,7 @@ pub mod pallet { + Eq + Clone + sp_std::fmt::Debug + + Ord + CompactSolution; /// Accuracy used for fallback on-chain election. @@ -879,22 +883,37 @@ pub mod pallet { Error::::SignedTooMuchWeight, ); - // ensure solution claims is better. + // create the submission + let reward = T::SignedRewardBase::get(); + let deposit = Self::deposit_for(&solution, size); + let submission = SignedSubmission { who: who.clone(), deposit, reward, solution }; + + // insert the submission if the queue has space or it's better than the weakest + // eject the weakest if the queue was full let mut signed_submissions = Self::signed_submissions(); - let ejected_a_solution = signed_submissions.len() - == T::SignedMaxSubmissions::get().saturated_into::(); - let index = Self::insert_submission(&who, &mut signed_submissions, solution, size) - .ok_or(Error::::SignedQueueFull)?; - - // collect deposit. Thereafter, the function cannot fail. - let deposit = signed_submissions - .get(index) - .map(|s| s.deposit) - .ok_or(Error::::InvalidSubmissionIndex)?; - T::Currency::reserve(&who, deposit).map_err(|_| Error::::SignedCannotPayDeposit)?; - - // store the new signed submission. - >::put(signed_submissions); + let (inserted, maybe_weakest) = signed_submissions.insert(submission); + let ejected_a_solution = maybe_weakest.is_some(); + + // it's an error if we neither inserted nor removed any submissions: this indicates + // the queue was full but our solution had insufficient score to eject any solution + ensure!( + (false, false) != (inserted, ejected_a_solution), + Error::::SignedQueueFull, + ); + + if inserted { + // collect deposit. Thereafter, the function cannot fail. + T::Currency::reserve(&who, deposit) + .map_err(|_| Error::::SignedCannotPayDeposit)?; + } + + // if we had to remove the weakest solution, unreserve its deposit + if let Some(weakest) = maybe_weakest { + let _remainder = T::Currency::unreserve(&weakest.who, weakest.deposit); + debug_assert!(_remainder.is_zero()); + } + + signed_submissions.put(); Self::deposit_event(Event::SolutionStored(ElectionCompute::Signed, ejected_a_solution)); Ok(()) } @@ -1048,14 +1067,34 @@ pub mod pallet { #[pallet::getter(fn snapshot_metadata)] pub type SnapshotMetadata = StorageValue<_, SolutionOrSnapshotSize>; - /// Sorted (worse -> best) list of unchecked, signed solutions. + /// The next index to be assigned to an incoming signed submission. + /// + /// We can't just use `SignedSubmissionIndices.len()`, because that's a bounded set; past its + /// capacity, it will simply saturate. We can't just iterate over `SignedSubmissionsMap`, + /// because iteration is slow. Instead, we store the value here. + #[pallet::storage] + pub(crate) type SignedSubmissionNextIndex = StorageValue<_, u32, ValueQuery>; + + /// A sorted, bounded set of `(score, 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 + /// them one at a time instead of reading and decoding all of them at once. + #[pallet::storage] + pub(crate) type SignedSubmissionIndices = + StorageValue<_, SubmissionIndicesOf, ValueQuery>; + + /// Unchecked, signed solutions. + /// + /// Together with `SubmissionIndices`, this stores a bounded set of `SignedSubmissions` while + /// allowing us to keep only a single one in memory at a time. + /// + /// Twox note: the key of the map is an auto-incrementing index which users cannot inspect or + /// affect; we shouldn't need a cryptographically secure hasher. #[pallet::storage] - #[pallet::getter(fn signed_submissions)] - pub type SignedSubmissions = StorageValue< - _, - Vec, CompactOf>>, - ValueQuery, - >; + pub(crate) type SignedSubmissionsMap = + StorageMap<_, Twox64Concat, u32, SignedSubmissionOf, ValueQuery>; /// The minimum score that each 'untrusted' solution must attain in order to be considered /// feasible. diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index e0f1a82919b9..97633b28967f 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -19,14 +19,26 @@ use crate::{ CompactOf, Config, ElectionCompute, Pallet, RawSolution, ReadySolution, SolutionOrSnapshotSize, - Weight, WeightInfo, QueuedSolution, SignedSubmissions, + Weight, WeightInfo, QueuedSolution, SignedSubmissionsMap, SignedSubmissionIndices, + SignedSubmissionNextIndex, }; use codec::{Encode, Decode, HasCompact}; -use frame_support::traits::{Currency, Get, OnUnbalanced, ReservableCurrency}; +use frame_support::{ + storage::bounded_btree_map::BoundedBTreeMap, + traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, + DebugNoBound, +}; use sp_arithmetic::traits::SaturatedConversion; -use sp_npos_elections::{is_score_better, CompactSolution}; -use sp_runtime::{Perbill, RuntimeDebug, traits::{Saturating, Zero}}; -use sp_std::{cmp::Ordering, vec::Vec}; +use sp_npos_elections::{is_score_better, CompactSolution, ElectionScore}; +use sp_runtime::{ + RuntimeDebug, + traits::{Saturating, Zero}, +}; +use sp_std::{ + cmp::Ordering, + collections::{btree_map::BTreeMap, btree_set::BTreeSet}, + ops::Deref, +}; /// A raw, unchecked signed submission. /// @@ -43,6 +55,38 @@ pub struct SignedSubmission { pub solution: RawSolution, } +impl Ord + for SignedSubmission +where + AccountId: Ord, + Balance: Ord + HasCompact, + CompactSolution: Ord, + RawSolution: Ord, +{ + fn cmp(&self, other: &Self) -> Ordering { + self.solution + .score + .cmp(&other.solution.score) + .then_with(|| self.solution.cmp(&other.solution)) + .then_with(|| self.deposit.cmp(&other.deposit)) + .then_with(|| self.reward.cmp(&other.reward)) + .then_with(|| self.who.cmp(&other.who)) + } +} + +impl PartialOrd + for SignedSubmission +where + AccountId: Ord, + Balance: Ord + HasCompact, + CompactSolution: Ord, + RawSolution: Ord, +{ + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + pub type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; pub type PositiveImbalanceOf = <::Currency as Currency< @@ -51,8 +95,175 @@ pub type PositiveImbalanceOf = <::Currency as Currency< pub type NegativeImbalanceOf = <::Currency as Currency< ::AccountId, >>::NegativeImbalance; +pub type SignedSubmissionOf = + SignedSubmission<::AccountId, BalanceOf, CompactOf>; + +pub type SubmissionIndicesOf = + BoundedBTreeMap::SignedMaxSubmissions>; + +/// Mask type which pretends to be a set of `SignedSubmissionOf`, while in fact delegating to the +/// actual implementations in `SignedSubmissionIndices`, `SignedSubmissionsMap`, and +/// `SignedSubmissionNextIndex`. +#[cfg_attr(feature = "std", derive(DebugNoBound))] +pub struct SignedSubmissions { + indices: SubmissionIndicesOf, + next_idx: u32, + insertion_overlay: BTreeMap>, + deletion_overlay: BTreeSet, +} + +impl SignedSubmissions { + /// Get the signed submissions from storage. + pub fn get() -> Self { + SignedSubmissions { + indices: SignedSubmissionIndices::::get(), + next_idx: SignedSubmissionNextIndex::::get(), + insertion_overlay: BTreeMap::new(), + deletion_overlay: BTreeSet::new(), + } + } + + /// Put the signed submissions back into storage. + pub fn put(self) { + SignedSubmissionIndices::::put(self.indices); + SignedSubmissionNextIndex::::put(self.next_idx); + for key in self.deletion_overlay { + SignedSubmissionsMap::::remove(key); + } + for (key, value) in self.insertion_overlay { + SignedSubmissionsMap::::insert(key, value); + } + } + + /// Get the submission at a particular index. + fn get_submission(&self, idx: u32) -> Option> { + self.insertion_overlay + .get(&idx) + .cloned() + .or_else(|| SignedSubmissionsMap::::try_get(idx).ok()) + } + + /// Take the submission at a particular index. + fn take_submission(&mut self, idx: u32) -> Option> { + self.insertion_overlay.remove(&idx).or_else(|| { + if self.deletion_overlay.contains(&idx) { + None + } else { + self.deletion_overlay.insert(idx); + SignedSubmissionsMap::::try_get(idx).ok() + } + }) + } + + /// Iterate through the set of signed submissions in order of increasing score. + pub fn iter(&self) -> impl '_ + Iterator> { + self.indices.iter().map(move |(_score, idx)| { + self.get_submission(*idx).expect("SignedSubmissions must remain internally consistent") + }) + } + + /// Empty the set of signed submissions, returning an iterator of signed submissions in + /// arbitrary order + pub fn drain(&mut self) -> impl '_ + Iterator> { + self.indices.clear(); + SignedSubmissionNextIndex::::kill(); + let insertion_overlay = sp_std::mem::take(&mut self.insertion_overlay); + SignedSubmissionsMap::::drain() + .filter(move |(k, _v)| !self.deletion_overlay.contains(k)) + .map(|(_k, v)| v) + .chain(insertion_overlay.into_iter().map(|(_k, v)| v)) + } + + /// Decode the length of the signed submissions without actually reading the entire struct into + /// memory. + /// + /// Note that if you hold an instance of `SignedSubmissions`, this function does _not_ + /// track its current length. This only decodes what is currently stored in memory. + pub fn decode_len() -> Option { + SignedSubmissionIndices::::decode_len() + } + + /// Insert a new signed submission into the set. + /// + /// Returns `(inserted, removed)`. `inserted` is true when the submission was inserted. + /// `removed` is the removed weakest submission, if any. + /// + /// In the event that the new submission is not better than the current weakest according + /// to `is_score_better`, we do not change anything. + pub fn insert( + &mut self, + submission: SignedSubmissionOf, + ) -> (bool, Option>) { + let weakest = match self.indices.try_insert(submission.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.solution.score, prev_idx) + .expect("didn't change the map size; qed"); + return (false, None); + } + Ok(None) => { + // successfully inserted into the set; no need to take out weakest member + None + } + Err((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, weakest_idx) = match self.indices.iter().next() { + None => return (false, None), + Some((score, idx)) => (*score, *idx), + }; + let threshold = T::SolutionImprovementThreshold::get(); + + // if we haven't improved on the weakest score, don't change anything. + if !is_score_better(score, weakest_score, threshold) { + return (false, None); + } + + self.indices.remove(&weakest_score); + self.indices + .try_insert(score, insert_idx) + .expect("just removed an item, we must be under capacity; qed"); + + // ensure that SignedSubmissionsMap never grows past capacity by taking out the + // weakest member here. + self.take_submission(weakest_idx) + } + }; + + // we've taken out the weakest, so update the storage map and the next index + self.insertion_overlay.insert(self.next_idx, submission); + self.next_idx += 1; + (true, weakest) + } + + /// Remove the signed submission with the highest score from the set. + pub fn pop_last(&mut self) -> Option> { + let (highest_score, idx) = self.indices.iter().rev().next()?; + let (highest_score, idx) = (*highest_score, *idx); + self.indices.remove(&highest_score); + self.take_submission(idx) + } +} + +impl Deref for SignedSubmissions { + type Target = SubmissionIndicesOf; + + fn deref(&self) -> &Self::Target { + &self.indices + } +} impl Pallet { + /// `Self` accessor for `SignedSubmission`. + pub fn signed_submissions() -> SignedSubmissions { + SignedSubmissions::::get() + } + /// Finish the signed phase. Process the signed submissions from best to worse until a valid one /// is found, rewarding the best one and slashing the invalid ones along the way. /// @@ -61,16 +272,11 @@ impl Pallet { /// This drains the [`SignedSubmissions`], potentially storing the best valid one in /// [`QueuedSolution`]. pub fn finalize_signed_phase() -> (bool, Weight) { - let mut all_submissions: Vec> = >::take(); + let mut all_submissions = Self::signed_submissions(); let mut found_solution = false; let mut weight = T::DbWeight::get().reads(1); - // Reverse the ordering of submissions: previously it was ordered such that high-scoring - // solutions have low indices. Now, the code flows more cleanly if high-scoring solutions - // have high indices. - all_submissions.reverse(); - - while let Some(best) = all_submissions.pop() { + while let Some(best) = all_submissions.pop_last() { let SignedSubmission { solution, who, deposit, reward } = best; let active_voters = solution.compact.voter_count() as u32; let feasibility_weight = { @@ -112,12 +318,13 @@ impl Pallet { // Any unprocessed solution is pointless to even consider. Feasible or malicious, // they didn't end up being used. Unreserve the bonds. let discarded = all_submissions.len(); - for not_processed in all_submissions { - let SignedSubmission { who, deposit, .. } = not_processed; + for SignedSubmission { who, deposit, .. } in all_submissions.drain() { let _remaining = T::Currency::unreserve(&who, deposit); weight = weight.saturating_add(T::DbWeight::get().writes(1)); debug_assert!(_remaining.is_zero()); - }; + } + + all_submissions.put(); log!(debug, "closed signed phase, found solution? {}, discarded {}", found_solution, discarded); (found_solution, weight) @@ -157,72 +364,6 @@ impl Pallet { T::SlashHandler::on_unbalanced(negative_imbalance); } - /// Insert a solution into the queue while maintaining an ordering by solution quality. - /// - /// Solutions are ordered in reverse: strong solutions have low indices. - /// - /// If insertion was successful, `Some(index)` is returned where index is the - /// index of the newly inserted item. - /// - /// Note: this function maintains the invariant that `queue.len() <= T::SignedMaxSubmissions`. - /// In the event that insertion would violate that invariant, the weakest element is dropped. - /// - /// Invariant: The returned index is always a valid index in `queue` and can safely be used to - /// inspect the newly inserted element. - pub fn insert_submission( - who: &T::AccountId, - queue: &mut Vec, CompactOf>>, - solution: RawSolution>, - size: SolutionOrSnapshotSize, - ) -> Option { - // Let's ensure that our input is valid. - let max_submissions = T::SignedMaxSubmissions::get(); - debug_assert!(queue.len() as u32 <= max_submissions); - - let threshold = T::SolutionImprovementThreshold::get(); - // this insertion logic is a bit unusual in that a new solution which beats an existing - // solution by less than the threshold is sorted as "greater" than the existing solution. - // this means that the produced ordering depends on the order of insertion, and that - // attempts to produce a total ordering using this comparitor are highly unstable. - // - // this ordering prioritizes earlier solutions over slightly better later ones. - let insertion_position = queue.binary_search_by(|s| { - if is_score_better::( - solution.score, - s.solution.score, - threshold, - ) { - Ordering::Greater - } else { - Ordering::Less - } - }).expect_err("comparitor function never returns Ordering::Equal; qed"); - - // if this solution is the worst one so far and the queue is full, don't insert - if insertion_position == queue.len() && queue.len() as u32 >= max_submissions { - return None; - } - - // add to the designated spot. If the length is too much, remove one. - let reward = T::SignedRewardBase::get(); - let deposit = Self::deposit_for(&solution, size); - let submission = - SignedSubmission { who: who.clone(), deposit, reward, solution }; - queue.insert(insertion_position, submission); - - // Remove the weakest if queue is overflowing. - // This doesn't adjust insertion_position: in the event that it might have, we'd have short- - // circuited above. - if queue.len() as u32 > max_submissions { - if let Some(SignedSubmission { who, deposit, .. }) = queue.pop() { - let _remainder = T::Currency::unreserve(&who, deposit); - debug_assert!(_remainder.is_zero()); - } - } - debug_assert!(queue.len() as u32 <= max_submissions); - Some(insertion_position) - } - /// The feasibility weight of the given raw solution. pub fn feasibility_weight_of( solution: &RawSolution>, @@ -325,7 +466,7 @@ mod tests { assert_ok!(submit_with_witness(Origin::signed(99), solution)); assert_eq!(balances(&99), (95, 5)); - assert_eq!(MultiPhase::signed_submissions().first().unwrap().deposit, 5); + assert_eq!(MultiPhase::signed_submissions().iter().next().unwrap().deposit, 5); }) } @@ -409,6 +550,8 @@ mod tests { assert_ok!(submit_with_witness(Origin::signed(99), solution)); } + dbg!(MultiPhase::signed_submissions().len(), SignedMaxSubmissions::get()); + // weaker. let solution = RawSolution { score: [4, 0, 0], ..Default::default() }; @@ -433,10 +576,10 @@ mod tests { assert_eq!( MultiPhase::signed_submissions() - .into_iter() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![9, 8, 7, 6, 5] + vec![5, 6, 7, 8, 9] ); // better. @@ -446,10 +589,10 @@ mod tests { // the one with score 5 was rejected, the new one inserted. assert_eq!( MultiPhase::signed_submissions() - .into_iter() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![20, 9, 8, 7, 6] + vec![6, 7, 8, 9, 20] ); }) } @@ -471,10 +614,10 @@ mod tests { assert_eq!( MultiPhase::signed_submissions() - .into_iter() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![9, 8, 7, 6, 4], + vec![4, 6, 7, 8, 9], ); // better. @@ -484,10 +627,10 @@ mod tests { // the one with score 5 was rejected, the new one inserted. assert_eq!( MultiPhase::signed_submissions() - .into_iter() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![9, 8, 7, 6, 5], + vec![5, 6, 7, 8, 9], ); }) } @@ -529,10 +672,10 @@ mod tests { } assert_eq!( MultiPhase::signed_submissions() - .into_iter() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![7, 6, 5] + vec![5, 6, 7] ); // 5 is not accepted. This will only cause processing with no benefit. @@ -544,49 +687,6 @@ mod tests { }) } - #[test] - fn solutions_are_always_sorted() { - ExtBuilder::default().signed_max_submission(3).build_and_execute(|| { - let scores = || { - MultiPhase::signed_submissions() - .into_iter() - .map(|s| s.solution.score[0]) - .collect::>() - }; - - roll_to(15); - assert!(MultiPhase::current_phase().is_signed()); - - let solution = RawSolution { score: [5, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![5]); - - let solution = RawSolution { score: [8, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![8, 5]); - - let solution = RawSolution { score: [3, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![8, 5, 3]); - - let solution = RawSolution { score: [6, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![8, 6, 5]); - - let solution = RawSolution { score: [6, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![8, 6, 6]); - - let solution = RawSolution { score: [10, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![10, 8, 6]); - - let solution = RawSolution { score: [12, 0, 0], ..Default::default() }; - assert_ok!(submit_with_witness(Origin::signed(99), solution)); - assert_eq!(scores(), vec![12, 10, 8]); - }) - } - #[test] fn all_in_one_signed_submission_scenario() { // a combination of: @@ -600,23 +700,25 @@ mod tests { assert_eq!(balances(&99), (100, 0)); assert_eq!(balances(&999), (100, 0)); assert_eq!(balances(&9999), (100, 0)); - let mut solution = raw_solution(); + let solution = raw_solution(); // submit a correct one. assert_ok!(submit_with_witness(Origin::signed(99), solution.clone())); // make the solution invalidly better and submit. This ought to be slashed. - solution.score[0] += 1; - assert_ok!(submit_with_witness(Origin::signed(999), solution.clone())); + let mut solution_999 = solution.clone(); + solution_999.score[0] += 1; + assert_ok!(submit_with_witness(Origin::signed(999), solution_999)); // make the solution invalidly worse and submit. This ought to be suppressed and // returned. - solution.score[0] -= 1; - assert_ok!(submit_with_witness(Origin::signed(9999), solution)); + let mut solution_9999 = solution.clone(); + solution_9999.score[0] -= 1; + assert_ok!(submit_with_witness(Origin::signed(9999), solution_9999)); assert_eq!( MultiPhase::signed_submissions().iter().map(|x| x.who).collect::>(), - vec![999, 99, 9999] + vec![9999, 99, 999] ); // _some_ good solution was stored. @@ -661,4 +763,52 @@ mod tests { ); }) } + + #[test] + fn insufficient_deposit_doesnt_store_submission() { + ExtBuilder::default().build_and_execute(|| { + roll_to(15); + assert!(MultiPhase::current_phase().is_signed()); + + let solution = raw_solution(); + + assert_eq!(balances(&123), (0, 0)); + assert_noop!( + submit_with_witness(Origin::signed(123), solution), + Error::::SignedCannotPayDeposit, + ); + + assert_eq!(balances(&123), (0, 0)); + }) + } + + // given a full queue, and a solution which _should_ be allowed in, but the proposer of this + // new solution has insufficient deposit, we should not modify storage at all + #[test] + fn insufficient_deposit_with_full_queue_works_properly() { + ExtBuilder::default().build_and_execute(|| { + roll_to(15); + assert!(MultiPhase::current_phase().is_signed()); + + for s in 0..SignedMaxSubmissions::get() { + // score is always getting better + let solution = RawSolution { score: [(5 + s).into(), 0, 0], ..Default::default() }; + assert_ok!(submit_with_witness(Origin::signed(99), solution)); + } + + // this solution has a higher score than any in the queue + let solution = RawSolution { + score: [(5 + SignedMaxSubmissions::get()).into(), 0, 0], + ..Default::default() + }; + + assert_eq!(balances(&123), (0, 0)); + assert_noop!( + submit_with_witness(Origin::signed(123), solution), + Error::::SignedCannotPayDeposit, + ); + + assert_eq!(balances(&123), (0, 0)); + }) + } } diff --git a/frame/support/src/storage/bounded_btree_map.rs b/frame/support/src/storage/bounded_btree_map.rs index 8c50557618ee..0c1994d63a35 100644 --- a/frame/support/src/storage/bounded_btree_map.rs +++ b/frame/support/src/storage/bounded_btree_map.rs @@ -39,7 +39,8 @@ pub struct BoundedBTreeMap(BTreeMap, PhantomData); impl Decode for BoundedBTreeMap where - BTreeMap: Decode, + K: Decode + Ord, + V: Decode, S: Get, { fn decode(input: &mut I) -> Result { @@ -115,14 +116,15 @@ where self.0.get_mut(key) } - /// Exactly the same semantics as [`BTreeMap::insert`], but returns an `Err` (and is a noop) if the - /// new length of the map exceeds `S`. - pub fn try_insert(&mut self, key: K, value: V) -> Result<(), ()> { - if self.len() < Self::bound() { - self.0.insert(key, value); - Ok(()) + /// Exactly the same semantics as [`BTreeMap::insert`], but returns an `Err` (and is a noop) if + /// the new length of the map exceeds `S`. + /// + /// In the `Err` case, returns the inserted pair so it can be further used without cloning. + pub fn try_insert(&mut self, key: K, value: V) -> Result, (K, V)> { + if self.len() < Self::bound() || self.0.contains_key(&key) { + Ok(self.0.insert(key, value)) } else { - Err(()) + Err((key, value)) } } @@ -407,4 +409,50 @@ pub mod test { Err("BoundedBTreeMap exceeds its limit".into()), ); } + + #[test] + fn unequal_eq_impl_insert_works() { + // given a struct with a strange notion of equality + #[derive(Debug)] + struct Unequal(u32, bool); + + impl PartialEq for Unequal { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } + } + impl Eq for Unequal {} + + impl Ord for Unequal { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.0.cmp(&other.0) + } + } + + impl PartialOrd for Unequal { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + } + + let mut map = BoundedBTreeMap::::new(); + + // when the set is full + + for i in 0..4 { + map.try_insert(Unequal(i, false), i).unwrap(); + } + + // can't insert a new distinct member + map.try_insert(Unequal(5, false), 5).unwrap_err(); + + // but _can_ insert a distinct member which compares equal, though per the documentation, + // neither the set length nor the actual member are changed, but the value is + map.try_insert(Unequal(0, true), 6).unwrap(); + assert_eq!(map.len(), 4); + let (zero_key, zero_value) = map.get_key_value(&Unequal(0, true)).unwrap(); + assert_eq!(zero_key.0, 0); + assert_eq!(zero_key.1, false); + assert_eq!(*zero_value, 6); + } } diff --git a/frame/support/src/storage/bounded_btree_set.rs b/frame/support/src/storage/bounded_btree_set.rs index f551a3cbfa38..10c2300a08a0 100644 --- a/frame/support/src/storage/bounded_btree_set.rs +++ b/frame/support/src/storage/bounded_btree_set.rs @@ -39,7 +39,7 @@ pub struct BoundedBTreeSet(BTreeSet, PhantomData); impl Decode for BoundedBTreeSet where - BTreeSet: Decode, + T: Decode + Ord, S: Get, { fn decode(input: &mut I) -> Result { @@ -103,14 +103,15 @@ where self.0.clear() } - /// Exactly the same semantics as [`BTreeSet::insert`], but returns an `Err` (and is a noop) if the - /// new length of the set exceeds `S`. - pub fn try_insert(&mut self, item: T) -> Result<(), ()> { - if self.len() < Self::bound() { - self.0.insert(item); - Ok(()) + /// Exactly the same semantics as [`BTreeSet::insert`], but returns an `Err` (and is a noop) if + /// the new length of the set exceeds `S`. + /// + /// In the `Err` case, returns the inserted item so it can be further used without cloning. + pub fn try_insert(&mut self, item: T) -> Result { + if self.len() < Self::bound() || self.0.contains(&item) { + Ok(self.0.insert(item)) } else { - Err(()) + Err(item) } } @@ -393,4 +394,49 @@ pub mod test { Err("BoundedBTreeSet exceeds its limit".into()), ); } + + #[test] + fn unequal_eq_impl_insert_works() { + // given a struct with a strange notion of equality + #[derive(Debug)] + struct Unequal(u32, bool); + + impl PartialEq for Unequal { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } + } + impl Eq for Unequal {} + + impl Ord for Unequal { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.0.cmp(&other.0) + } + } + + impl PartialOrd for Unequal { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + } + + let mut set = BoundedBTreeSet::::new(); + + // when the set is full + + for i in 0..4 { + set.try_insert(Unequal(i, false)).unwrap(); + } + + // can't insert a new distinct member + set.try_insert(Unequal(5, false)).unwrap_err(); + + // but _can_ insert a distinct member which compares equal, though per the documentation, + // neither the set length nor the actual member are changed + set.try_insert(Unequal(0, true)).unwrap(); + assert_eq!(set.len(), 4); + let zero_item = set.get(&Unequal(0, true)).unwrap(); + assert_eq!(zero_item.0, 0); + assert_eq!(zero_item.1, false); + } } diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index e49518cc25cc..14aefecef6da 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -169,7 +169,7 @@ fn struct_def( ); quote!{ #compact_impl - #[derive(Default, PartialEq, Eq, Clone, Debug)] + #[derive(Default, PartialEq, Eq, Clone, Debug, PartialOrd, Ord)] } } else { // automatically derived.