diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 1da670b389192..c523ccc86fc7a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -674,6 +674,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type SignedRewardBase = SignedRewardBase; type SignedDepositBase = SignedDepositBase; type SignedDepositByte = SignedDepositByte; + type SignedMaxRefunds = ConstU32<3>; type SignedDepositWeight = (); type SignedMaxWeight = MinerMaxWeight; type SlashHandler = (); // burn slashes diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 923e9e2d984cc..6dfb2ac794810 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -225,14 +225,24 @@ frame_benchmarking::benchmarks! { compute: Default::default() }; let deposit: BalanceOf = 10u32.into(); - let reward: BalanceOf = 20u32.into(); + + let reward: BalanceOf = T::SignedRewardBase::get(); + let call_fee: BalanceOf = 30u32.into(); assert_ok!(T::Currency::reserve(&receiver, deposit)); assert_eq!(T::Currency::free_balance(&receiver), initial_balance - 10u32.into()); }: { - >::finalize_signed_phase_accept_solution(ready, &receiver, deposit, reward) + >::finalize_signed_phase_accept_solution( + ready, + &receiver, + deposit, + call_fee + ) } verify { - assert_eq!(T::Currency::free_balance(&receiver), initial_balance + 20u32.into()); + assert_eq!( + T::Currency::free_balance(&receiver), + initial_balance + reward + call_fee + ); assert_eq!(T::Currency::reserved_balance(&receiver), 0u32.into()); } @@ -333,7 +343,7 @@ frame_benchmarking::benchmarks! { raw_solution, who: account("submitters", i, SEED), deposit: Default::default(), - reward: Default::default(), + call_fee: Default::default(), }; signed_submissions.insert(signed_submission); } diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 5dc44d38fc365..a9e341bad600f 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -242,7 +242,7 @@ use frame_support::{ use frame_system::{ensure_none, offchain::SendTransactionTypes}; use scale_info::TypeInfo; use sp_arithmetic::{ - traits::{Bounded, CheckedAdd, Saturating, Zero}, + traits::{Bounded, CheckedAdd, Zero}, UpperOf, }; use sp_npos_elections::{ @@ -628,6 +628,10 @@ pub mod pallet { #[pallet::constant] type SignedMaxWeight: Get; + /// The maximum amount of unchecked solutions to refund the call fee for. + #[pallet::constant] + type SignedMaxRefunds: Get; + /// Base reward for a signed solution #[pallet::constant] type SignedRewardBase: Get>; @@ -848,6 +852,11 @@ pub mod pallet { ::MaxVotesPerVoter::get(), as NposSolution>::LIMIT as u32, ); + + // While it won't cause any failures, setting `SignedMaxRefunds` gt + // `SignedMaxSubmissions` is a red flag that the developer does not understand how to + // configure this pallet. + assert!(T::SignedMaxSubmissions::get() >= T::SignedMaxRefunds::get()); } } @@ -988,14 +997,17 @@ pub mod pallet { // create the submission let deposit = Self::deposit_for(&raw_solution, size); - let reward = { + let call_fee = { let call = Call::submit { raw_solution: raw_solution.clone() }; - let call_fee = T::EstimateCallFee::estimate_call_fee(&call, None.into()); - T::SignedRewardBase::get().saturating_add(call_fee) + T::EstimateCallFee::estimate_call_fee(&call, None.into()) }; - let submission = - SignedSubmission { who: who.clone(), deposit, raw_solution: *raw_solution, reward }; + let submission = SignedSubmission { + who: who.clone(), + deposit, + raw_solution: *raw_solution, + call_fee, + }; // insert the submission if the queue has space or it's better than the weakest // eject the weakest if the queue was full diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 2c9d7bb34dba5..38d9c8dfc1b7e 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -257,6 +257,7 @@ parameter_types! { pub static SignedPhase: BlockNumber = 10; pub static UnsignedPhase: BlockNumber = 5; pub static SignedMaxSubmissions: u32 = 5; + pub static SignedMaxRefunds: u32 = 1; pub static SignedDepositBase: Balance = 5; pub static SignedDepositByte: Balance = 0; pub static SignedDepositWeight: Balance = 0; @@ -427,6 +428,7 @@ impl crate::Config for Runtime { type SignedDepositWeight = (); type SignedMaxWeight = SignedMaxWeight; type SignedMaxSubmissions = SignedMaxSubmissions; + type SignedMaxRefunds = SignedMaxRefunds; type SlashHandler = (); type RewardHandler = (); type DataProvider = StakingMock; diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 465528068e71a..5f61eb7575da4 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -38,6 +38,7 @@ use sp_std::{ cmp::Ordering, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, ops::Deref, + vec::Vec, }; /// A raw, unchecked signed submission. @@ -51,8 +52,8 @@ pub struct SignedSubmission { pub deposit: Balance, /// The raw solution itself. pub raw_solution: RawSolution, - /// The reward that should potentially be paid for this solution, if accepted. - pub reward: Balance, + // The estimated fee `who` paid to submit the solution. + pub call_fee: Balance, } impl Ord for SignedSubmission @@ -235,20 +236,33 @@ impl SignedSubmissions { } /// Empty the set of signed submissions, returning an iterator of signed submissions in - /// arbitrary order. + /// order of submission. /// /// Note that if the iterator is dropped without consuming all elements, not all may be removed /// from the underlying `SignedSubmissionsMap`, putting the storages into an invalid state. /// /// Note that, like `put`, this function consumes `Self` and modifies storage. - fn drain(mut self) -> impl Iterator> { + fn drain_submitted_order(mut self) -> impl Iterator> { + let mut keys = SignedSubmissionsMap::::iter_keys() + .filter(|k| { + if self.deletion_overlay.contains(k) { + // Remove submissions that should be deleted. + SignedSubmissionsMap::::remove(k); + false + } else { + true + } + }) + .chain(self.insertion_overlay.keys().copied()) + .collect::>(); + keys.sort(); + SignedSubmissionIndices::::kill(); 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)) + + keys.into_iter().filter_map(move |index| { + SignedSubmissionsMap::::take(index).or_else(|| self.insertion_overlay.remove(&index)) + }) } /// Decode the length of the signed submissions without actually reading the entire struct into @@ -362,7 +376,7 @@ impl Pallet { Self::snapshot_metadata().unwrap_or_default(); while let Some(best) = all_submissions.pop_last() { - let SignedSubmission { raw_solution, who, deposit, reward } = best; + let SignedSubmission { raw_solution, who, deposit, call_fee } = best; let active_voters = raw_solution.solution.voter_count() as u32; let feasibility_weight = { // defensive only: at the end of signed phase, snapshot will exits. @@ -377,7 +391,7 @@ impl Pallet { ready_solution, &who, deposit, - reward, + call_fee, ); found_solution = true; @@ -396,10 +410,23 @@ 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 SignedSubmission { who, deposit, .. } in all_submissions.drain() { + let mut refund_count = 0; + let max_refunds = T::SignedMaxRefunds::get(); + + for SignedSubmission { who, deposit, call_fee, .. } in + all_submissions.drain_submitted_order() + { + if refund_count < max_refunds { + // Refund fee + let positive_imbalance = T::Currency::deposit_creating(&who, call_fee); + T::RewardHandler::on_unbalanced(positive_imbalance); + refund_count += 1; + } + + // Unreserve deposit let _remaining = T::Currency::unreserve(&who, deposit); - weight = weight.saturating_add(T::DbWeight::get().writes(1)); debug_assert!(_remaining.is_zero()); + weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 2)); } debug_assert!(!SignedSubmissionIndices::::exists()); @@ -424,20 +451,22 @@ impl Pallet { ready_solution: ReadySolution, who: &T::AccountId, deposit: BalanceOf, - reward: BalanceOf, + call_fee: BalanceOf, ) { // write this ready solution. >::put(ready_solution); + let reward = T::SignedRewardBase::get(); // emit reward event Self::deposit_event(crate::Event::Rewarded { account: who.clone(), value: reward }); - // unreserve deposit. + // Unreserve deposit. let _remaining = T::Currency::unreserve(who, deposit); debug_assert!(_remaining.is_zero()); - // Reward. - let positive_imbalance = T::Currency::deposit_creating(who, reward); + // Reward and refund the call fee. + let positive_imbalance = + T::Currency::deposit_creating(who, reward.saturating_add(call_fee)); T::RewardHandler::on_unbalanced(positive_imbalance); } @@ -496,8 +525,8 @@ mod tests { use super::*; use crate::{ mock::{ - balances, raw_solution, roll_to, ExtBuilder, MultiPhase, Origin, Runtime, - SignedMaxSubmissions, SignedMaxWeight, + balances, raw_solution, roll_to, Balances, ExtBuilder, MultiPhase, Origin, Runtime, + SignedMaxRefunds, SignedMaxSubmissions, SignedMaxWeight, }, Error, Perbill, Phase, }; @@ -599,8 +628,8 @@ mod tests { // 99 is rewarded. assert_eq!(balances(&99), (100 + 7 + 8, 0)); - // 999 gets everything back. - assert_eq!(balances(&999), (100, 0)); + // 999 gets everything back, including the call fee. + assert_eq!(balances(&999), (100 + 8, 0)); }) } @@ -632,6 +661,44 @@ mod tests { }) } + #[test] + fn call_fee_refund_is_limited_by_signed_max_refunds() { + ExtBuilder::default().build_and_execute(|| { + roll_to(15); + assert!(MultiPhase::current_phase().is_signed()); + assert_eq!(SignedMaxRefunds::get(), 1); + assert!(SignedMaxSubmissions::get() > 2); + + for s in 0..SignedMaxSubmissions::get() { + let account = 99 + s as u64; + Balances::make_free_balance_be(&account, 100); + // score is always decreasing + let mut solution = raw_solution(); + solution.score.minimal_stake -= s as u128; + + assert_ok!(MultiPhase::submit(Origin::signed(account), Box::new(solution))); + assert_eq!(balances(&account), (95, 5)); + } + + assert!(MultiPhase::finalize_signed_phase()); + + for s in 0..SignedMaxSubmissions::get() { + let account = 99 + s as u64; + // lower accounts have higher scores + if s == 0 { + // winning solution always gets call fee + reward + assert_eq!(balances(&account), (100 + 8 + 7, 0)) + } else if s == 1 { + // 1 runner up gets their call fee refunded + assert_eq!(balances(&account), (100 + 8, 0)) + } else { + // all other solutions don't get a call fee refund + assert_eq!(balances(&account), (100, 0)); + } + } + }); + } + #[test] fn cannot_submit_worse_with_full_queue_depends_on_threshold() { ExtBuilder::default() @@ -687,12 +754,15 @@ mod tests { assert!(MultiPhase::current_phase().is_signed()); for s in 0..SignedMaxSubmissions::get() { + let account = 99 + s as u64; + Balances::make_free_balance_be(&account, 100); // score is always getting better let solution = RawSolution { score: ElectionScore { minimal_stake: (5 + s).into(), ..Default::default() }, ..Default::default() }; - assert_ok!(MultiPhase::submit(Origin::signed(99), Box::new(solution))); + assert_ok!(MultiPhase::submit(Origin::signed(account), Box::new(solution))); + assert_eq!(balances(&account), (95, 5)); } assert_eq!( @@ -708,7 +778,7 @@ mod tests { score: ElectionScore { minimal_stake: 20, ..Default::default() }, ..Default::default() }; - assert_ok!(MultiPhase::submit(Origin::signed(99), Box::new(solution))); + assert_ok!(MultiPhase::submit(Origin::signed(999), Box::new(solution))); // the one with score 5 was rejected, the new one inserted. assert_eq!( @@ -718,6 +788,9 @@ mod tests { .collect::>(), vec![6, 7, 8, 9, 20] ); + + // the submitter of the ejected solution does *not* get a call fee refund + assert_eq!(balances(&(99 + 0)), (100, 0)); }) } @@ -873,8 +946,8 @@ mod tests { assert_eq!(balances(&99), (100 + 7 + 8, 0)); // 999 is slashed. assert_eq!(balances(&999), (95, 0)); - // 9999 gets everything back. - assert_eq!(balances(&9999), (100, 0)); + // 9999 gets everything back, including the call fee. + assert_eq!(balances(&9999), (100 + 8, 0)); }) }