From 051d71eb890559f34c5b60570438e800b1385756 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 24 Sep 2020 21:29:28 +0200 Subject: [PATCH 01/20] Mockup --- frame/staking/src/offchain_election.rs | 9 +++ primitives/npos-elections/compact/src/lib.rs | 34 +++++++++++ primitives/npos-elections/src/tests.rs | 62 ++++++++++++++++++++ 3 files changed, 105 insertions(+) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 9e797d0b1d270..e0a110444d889 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -140,6 +140,15 @@ pub(crate) fn compute_offchain_election() -> Result<(), OffchainElecti .map_err(|_| OffchainElectionError::PoolSubmissionFailed) } +/// A greedy effort to reduce the size if a solution. +// pub fn trim_solution_to_fit( +// winners: Vec, +// compact: CompactAssignments, +// size: ElectionSize, +// ) -> (Vec, CompactAssignments, ElectionSize) { + +// } + /// Get a random number of iterations to run the balancing. /// /// Uses the offchain seed to generate a random number. diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index 134f3f59ff177..5355b6530e97e 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -152,6 +152,7 @@ fn struct_def( let len_impl = len_impl(count); let edge_count_impl = edge_count_impl(count); let unique_targets_impl = unique_targets_impl(count); + let remove_voter_impl = remove_voter_impl(count); let derives_and_maybe_compact_encoding = if compact_encoding { // custom compact encoding. @@ -220,10 +221,43 @@ fn struct_def( pub fn average_edge_count(&self) -> usize { self.edge_count().checked_div(self.len()).unwrap_or(0) } + + /// Remove a certain voter. + /// + /// This is currently O(n), meaning that it searches the entire struct. + pub fn remove_voter(&mut self, to_remove: #voter_type) { + #remove_voter_impl + } } )) } +fn remove_voter_impl(count: usize) -> TokenStream2 { + // 1s + let filed_name = field_name_for(1); + let single = quote! { + self.#filed_name.retain(|(x, _)| *x != to_remove); + }; + + let filed_name = field_name_for(2); + let double = quote! { + self.#filed_name.retain(|(x, _, _)| *x != to_remove); + }; + + let rest = (3..=count).map(|c| { + let filed_name = field_name_for(c); + quote! { + self.#filed_name.retain(|(x, _, _)| *x != to_remove); + } + }).collect::(); + + quote! { + #single + #double + #rest + } +} + fn len_impl(count: usize) -> TokenStream2 { (1..=count).map(|c| { let field_name = field_name_for(c); diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index 44a82eaf4ef99..f4fa82a053af0 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -1155,6 +1155,68 @@ mod solution_type { assert_eq!(compact.unique_targets(), vec![10, 11, 20, 40, 50, 51]); } + #[test] + fn remove_voter_works() { + let mut compact = TestSolutionCompact { + votes1: vec![(0, 2), (1, 6)], + votes2: vec![ + (2, (0, TestAccuracy::from_percent(80)), 1), + (3, (7, TestAccuracy::from_percent(85)), 8), + ], + votes3: vec![ + ( + 4, + [(3, TestAccuracy::from_percent(50)), (4, TestAccuracy::from_percent(25))], + 5, + ), + ], + ..Default::default() + }; + + compact.remove_voter(2); + assert_eq!( + compact, + TestSolutionCompact { + votes1: vec![(0, 2), (1, 6)], + votes2: vec![ + (3, (7, TestAccuracy::from_percent(85)), 8), + ], + votes3: vec![ + ( + 4, + [(3, TestAccuracy::from_percent(50)), (4, TestAccuracy::from_percent(25))], + 5, + ), + ], + ..Default::default() + }, + ); + + compact.remove_voter(4); + assert_eq!( + compact, + TestSolutionCompact { + votes1: vec![(0, 2), (1, 6)], + votes2: vec![ + (3, (7, TestAccuracy::from_percent(85)), 8), + ], + ..Default::default() + }, + ); + + compact.remove_voter(1); + assert_eq!( + compact, + TestSolutionCompact { + votes1: vec![(0, 2)], + votes2: vec![ + (3, (7, TestAccuracy::from_percent(85)), 8), + ], + ..Default::default() + }, + ); + } + #[test] fn basic_from_and_into_compact_works_assignments() { let voters = vec![ From 46686d62eedf588706b716c877bfa41e1a1e3dc4 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 25 Sep 2020 15:32:59 +0200 Subject: [PATCH 02/20] Fix offchain election to respect the weight --- frame/staking/src/lib.rs | 10 +- frame/staking/src/mock.rs | 39 ++-- frame/staking/src/offchain_election.rs | 203 +++++++++++++++---- primitives/npos-elections/compact/src/lib.rs | 36 +++- primitives/npos-elections/src/tests.rs | 7 +- 5 files changed, 227 insertions(+), 68 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index e5c1a68dfbeae..80a0f9e34b07a 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -695,7 +695,7 @@ pub enum ElectionStatus { /// Note that these values must reflect the __total__ number, not only those that are present in the /// solution. In short, these should be the same size as the size of the values dumped in /// `SnapshotValidators` and `SnapshotNominators`. -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default)] +#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, RuntimeDebug, Default)] pub struct ElectionSize { /// Number of validators in the snapshot of the current election round. #[codec(compact)] @@ -883,6 +883,9 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes> { /// multiple pallets send unsigned transactions. type UnsignedPriority: Get; + /// Maximum weight that the unsigned transaction can have. + type MaximumUnsignedWeight: Get; + /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; } @@ -2143,12 +2146,13 @@ decl_module! { /// # /// See `crate::weight` module. /// # - #[weight = T::WeightInfo::submit_solution_better( + #[weight = ( + T::WeightInfo::submit_solution_better( size.validators.into(), size.nominators.into(), compact.len() as u32, winners.len() as u32, - )] + ), frame_support::weights::DispatchClass::Operational)] pub fn submit_election_solution_unsigned( origin, winners: Vec, diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 4b499d5b4626a..c9ce93719c200 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -17,24 +17,32 @@ //! Test utilities -use std::{collections::HashSet, cell::RefCell}; -use sp_runtime::Perbill; -use sp_runtime::curve::PiecewiseLinear; -use sp_runtime::traits::{IdentityLookup, Convert, SaturatedConversion, Zero}; -use sp_runtime::testing::{Header, UintAuthorityId, TestXt}; -use sp_staking::{SessionIndex, offence::{OffenceDetails, OnOffenceHandler}}; -use sp_core::H256; +use crate::*; use frame_support::{ - assert_ok, impl_outer_origin, parameter_types, impl_outer_dispatch, impl_outer_event, - StorageValue, StorageMap, StorageDoubleMap, IterableStorageMap, - traits::{Currency, Get, FindAuthor, OnFinalize, OnInitialize}, - weights::{Weight, constants::RocksDbWeight}, + assert_ok, impl_outer_dispatch, impl_outer_event, impl_outer_origin, parameter_types, + traits::{Currency, FindAuthor, Get, OnFinalize, OnInitialize}, + weights::{ + constants::{RocksDbWeight, WEIGHT_PER_SECOND}, + Weight, + }, + IterableStorageMap, StorageDoubleMap, StorageMap, StorageValue, }; +use sp_core::H256; use sp_io; use sp_npos_elections::{ - build_support_map, evaluate_support, reduce, ExtendedBalance, StakedAssignment, ElectionScore, + build_support_map, evaluate_support, reduce, ElectionScore, ExtendedBalance, StakedAssignment, }; -use crate::*; +use sp_runtime::{ + curve::PiecewiseLinear, + testing::{Header, TestXt, UintAuthorityId}, + traits::{Convert, IdentityLookup, SaturatedConversion, Zero}, + Perbill, +}; +use sp_staking::{ + offence::{OffenceDetails, OnOffenceHandler}, + SessionIndex, +}; +use std::{cell::RefCell, collections::HashSet}; pub const INIT_TIMESTAMP: u64 = 30_000; @@ -293,6 +301,7 @@ parameter_types! { pub const MaxNominatorRewardedPerValidator: u32 = 64; pub const UnsignedPriority: u64 = 1 << 20; pub const MinSolutionScoreBump: Perbill = Perbill::zero(); + pub const MaximumUnsignedWeight: Weight = MaximumBlockWeight::get(); } thread_local! { @@ -331,10 +340,12 @@ impl Trait for Test { type MinSolutionScoreBump = MinSolutionScoreBump; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = UnsignedPriority; + type MaximumUnsignedWeight = MaximumUnsignedWeight; type WeightInfo = (); } -impl frame_system::offchain::SendTransactionTypes for Test where +impl frame_system::offchain::SendTransactionTypes for Test +where Call: From, { type OverarchingCall = Call; diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index e0a110444d889..4b7fc1f67d79d 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -17,19 +17,20 @@ //! Helpers for offchain worker election. -use codec::Decode; use crate::{ - Call, CompactAssignments, Module, NominatorIndex, OffchainAccuracy, Trait, ValidatorIndex, - ElectionSize, + Call, CompactAssignments, ElectionSize, Module, NominatorIndex, Nominators, OffchainAccuracy, + Trait, ValidatorIndex, WeightInfo, }; +use codec::Decode; +use frame_support::{traits::Get, weights::Weight}; use frame_system::offchain::SubmitTransaction; use sp_npos_elections::{ - build_support_map, evaluate_support, reduce, Assignment, ExtendedBalance, ElectionResult, - ElectionScore, + build_support_map, evaluate_support, reduce, Assignment, ElectionResult, ElectionScore, + ExtendedBalance, +}; +use sp_runtime::{ + offchain::storage::StorageValueRef, traits::TrailingZeroInput, PerThing, RuntimeDebug, }; -use sp_runtime::offchain::storage::StorageValueRef; -use sp_runtime::{PerThing, RuntimeDebug, traits::TrailingZeroInput}; -use frame_support::traits::Get; use sp_std::{convert::TryInto, prelude::*}; /// Error types related to the offchain election machinery. @@ -115,7 +116,8 @@ pub(crate) fn compute_offchain_election() -> Result<(), OffchainElecti .ok_or(OffchainElectionError::ElectionFailed)?; // process and prepare it for submission. - let (winners, compact, score, size) = prepare_submission::(assignments, winners, true)?; + let (winners, compact, score, size) = + prepare_submission::(assignments, winners, true, T::MaximumUnsignedWeight::get())?; crate::log!( info, @@ -164,6 +166,105 @@ pub fn get_balancing_iters() -> usize { } } +/// Find the maximum `len` that a compact can have in order to fit into the block weight. +pub fn maximum_compact_len( + winners_len: u32, + size: ElectionSize, + max_weight: Weight, +) -> u32 { + // TODO: ofc we can do this nicer, if we rely on the internals of the weight function :\ + // TODO: check infinite loop is not possible. + let mut voters = 0; + loop { + let weight = T::WeightInfo::submit_solution_better( + size.validators.into(), + size.nominators.into(), + voters, + winners_len, + ); + if weight < max_weight { + voters += 1; + } else { + break; + } + } + voters +} + +/// Greedily reduce the size of the a solution to fit into the block, wrt weight. +/// +/// The weight of the solution is foremost a function of the number of voters (i.e. +/// `compact.len()`). Aside from this, the other components of the weight are invariant. The number +/// of winners shall not be changed (otherwise the solution is invalid) and the `ElectionSize` is +/// merely a representation of the total number of stakers. +/// +/// Thus, we reside to stripping away some voters. This means only changing the `compact` struct. +/// +/// Note that the solution is already computed, and the winners are elected based on the merit of +/// teh entire stake in the system. Nonetheless, some of the voters will be removed further down the +/// line. +/// +/// Indeed, the score must be computed **after** this step. If this step reduces the score too much, +/// then the solution will be discarded. +pub fn trim_to_weight( + maximum_allowed_voters: u32, + mut compact: CompactAssignments, + nominator_index: FN, +) -> CompactAssignments +where + for<'r> FN: Fn(&'r T::AccountId) -> Option, +{ + use frame_support::IterableStorageMap; + + if let Some(to_remove) = compact.len().checked_sub(maximum_allowed_voters as usize) { + // grab all voters and sort them by least stake. + let mut voters_sorted = >::iter() + .map(|(who, _)| { + ( + who.clone(), + >::slashable_balance_of_vote_weight(&who), + ) + }) + .collect::>(); + voters_sorted.sort_by_key(|(_, y)| *y); + + // start removing from the leas stake. Iterate until we know enough have been removed. + let mut removed = 0; + for (i, _stake) in voters_sorted + .iter() + .map(|(who, stake)| (nominator_index(&who).unwrap(), stake)) + { + if compact.remove_voter(i) { + crate::log!( + debug, + "removed a voter at index {} with stake {:?} from compact to reduce the size", + i, + _stake, + ); + removed += 1 + } + + if removed > to_remove { + break; + } + } + + crate::log!( + warn, + "{} voters had to be removed from compact solution due to size limits.", + removed + ); + compact + } else { + // nada, return as-is + crate::log!( + info, + "Compact solution did not get trimmed due to block weight limits.", + ); + compact + } +} + /// Takes an election result and spits out some data that can be submitted to the chain. /// /// This does a lot of stuff; read the inline comments. @@ -171,12 +272,17 @@ pub fn prepare_submission( assignments: Vec>, winners: Vec<(T::AccountId, ExtendedBalance)>, do_reduce: bool, -) -> Result<( - Vec, - CompactAssignments, - ElectionScore, - ElectionSize, -), OffchainElectionError> where + maximum_weight: Weight, +) -> Result< + ( + Vec, + CompactAssignments, + ElectionScore, + ElectionSize, + ), + OffchainElectionError, +> +where ExtendedBalance: From<::Inner>, { // make sure that the snapshot is available. @@ -185,7 +291,7 @@ pub fn prepare_submission( let snapshot_nominators = >::snapshot_nominators().ok_or(OffchainElectionError::SnapshotUnavailable)?; - // all helper closures + // all helper closures that we'd ever need. let nominator_index = |a: &T::AccountId| -> Option { snapshot_nominators .iter() @@ -198,6 +304,19 @@ pub fn prepare_submission( .position(|x| x == a) .and_then(|i| >::try_into(i).ok()) }; + let nominator_at = |i: NominatorIndex| -> Option { + snapshot_nominators.get(i as usize).cloned() + }; + + let validator_at = |i: ValidatorIndex| -> Option { + snapshot_validators.get(i as usize).cloned() + }; + + // both conversions are safe; snapshots are not created if they exceed. + let size = ElectionSize { + validators: snapshot_validators.len() as ValidatorIndex, + nominators: snapshot_nominators.len() as NominatorIndex, + }; // Clean winners. let winners = sp_npos_elections::to_without_backing(winners); @@ -217,17 +336,38 @@ pub fn prepare_submission( let low_accuracy_assignment = sp_npos_elections::assignment_staked_to_ratio_normalized(staked) .map_err(|e| OffchainElectionError::from(e))?; - // convert back to staked to compute the score in the receiver's accuracy. This can be done - // nicer, for now we do it as such since this code is not time-critical. This ensure that the - // score _predicted_ here is the same as the one computed on chain and you will not get a - // `PhragmenBogusScore` error. This is totally NOT needed if we don't do reduce. This whole - // _accuracy glitch_ happens because reduce breaks that assumption of rounding and **scale**. - // The initial phragmen results are computed in `OffchainAccuracy` and the initial `staked` - // assignment set is also all multiples of this value. After reduce, this no longer holds. Hence - // converting to ratio thereafter is not trivially reversible. + // compact encode the assignment. + let compact = CompactAssignments::from_assignment( + low_accuracy_assignment, + nominator_index, + validator_index, + ) + .map_err(|e| OffchainElectionError::from(e))?; + + // potentially reduce the size of the compact to fit weight. + let maximum_allowed_voters = + maximum_compact_len::(winners.len() as u32, size, maximum_weight); + + crate::log!(info, "Maximum weight = {:?} // current weight = {:?} // maximum voters = {:?} // current votes = {:?}", + maximum_weight, + T::WeightInfo::submit_solution_better( + size.validators.into(), + size.nominators.into(), + compact.len() as u32, + winners.len() as u32, + ), + maximum_allowed_voters, + compact.len(), + ); + + let compact = trim_to_weight::(maximum_allowed_voters, compact, &nominator_index); + + // re-compute the score. We re-create what the chain will do. let score = { + let compact = compact.clone(); + let assignments = compact.into_assignment(nominator_at, validator_at).unwrap(); let staked = sp_npos_elections::assignment_ratio_to_staked( - low_accuracy_assignment.clone(), + assignments, >::slashable_balance_of_vote_weight, ); @@ -236,13 +376,6 @@ pub fn prepare_submission( evaluate_support::(&support_map) }; - // compact encode the assignment. - let compact = CompactAssignments::from_assignment( - low_accuracy_assignment, - nominator_index, - validator_index, - ).map_err(|e| OffchainElectionError::from(e))?; - // winners to index. Use a simple for loop for a more expressive early exit in case of error. let mut winners_indexed: Vec = Vec::with_capacity(winners.len()); for w in winners { @@ -256,11 +389,5 @@ pub fn prepare_submission( } } - // both conversions are safe; snapshots are not created if they exceed. - let size = ElectionSize { - validators: snapshot_validators.len() as ValidatorIndex, - nominators: snapshot_nominators.len() as NominatorIndex, - }; - Ok((winners_indexed, compact, score, size)) } diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index 5355b6530e97e..4c0184e638853 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -224,9 +224,14 @@ fn struct_def( /// Remove a certain voter. /// - /// This is currently O(n), meaning that it searches the entire struct. - pub fn remove_voter(&mut self, to_remove: #voter_type) { + /// This will only search until the first instance of `to_remove`, and return true. If + /// no instance is found (no-op), then it returns false. + /// + /// In other words, if this return true, exactly one element must have been removed from + /// `self.len()`. + pub fn remove_voter(&mut self, to_remove: #voter_type) -> bool { #remove_voter_impl + return false } } )) @@ -236,20 +241,31 @@ fn remove_voter_impl(count: usize) -> TokenStream2 { // 1s let filed_name = field_name_for(1); let single = quote! { - self.#filed_name.retain(|(x, _)| *x != to_remove); + if let Some(idx) = self.#filed_name.iter().position(|(x, _)| *x == to_remove) { + self.#filed_name.remove(idx); + return true + } }; let filed_name = field_name_for(2); let double = quote! { - self.#filed_name.retain(|(x, _, _)| *x != to_remove); + if let Some(idx) = self.#filed_name.iter().position(|(x, _, _)| *x == to_remove) { + self.#filed_name.remove(idx); + return true + } }; - let rest = (3..=count).map(|c| { - let filed_name = field_name_for(c); - quote! { - self.#filed_name.retain(|(x, _, _)| *x != to_remove); - } - }).collect::(); + let rest = (3..=count) + .map(|c| { + let filed_name = field_name_for(c); + quote! { + if let Some(idx) = self.#filed_name.iter().position(|(x, _, _)| *x == to_remove) { + self.#filed_name.remove(idx); + return true + } + } + }) + .collect::(); quote! { #single diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index f4fa82a053af0..dc7a1a5fdfb97 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -1173,7 +1173,8 @@ mod solution_type { ..Default::default() }; - compact.remove_voter(2); + assert!(!compact.remove_voter(11)); + assert!(compact.remove_voter(2)); assert_eq!( compact, TestSolutionCompact { @@ -1192,7 +1193,7 @@ mod solution_type { }, ); - compact.remove_voter(4); + assert!(compact.remove_voter(4)); assert_eq!( compact, TestSolutionCompact { @@ -1204,7 +1205,7 @@ mod solution_type { }, ); - compact.remove_voter(1); + assert!(compact.remove_voter(1)); assert_eq!( compact, TestSolutionCompact { From 649a6d6e7f2bc5c6eb956155ec6fcf92a74e2bee Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 25 Sep 2020 15:47:01 +0200 Subject: [PATCH 03/20] Fix builds a bit --- .gitignore | 1 + bin/node/runtime/src/lib.rs | 2 ++ frame/babe/src/mock.rs | 1 + frame/grandpa/src/mock.rs | 1 + frame/offences/benchmarking/src/mock.rs | 1 + frame/session/benchmarking/src/mock.rs | 1 + frame/staking/fuzzer/src/mock.rs | 1 + 7 files changed, 8 insertions(+) diff --git a/.gitignore b/.gitignore index 353d49df28f34..c8f1ea9567bc2 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,4 @@ rls*.log **/hfuzz_target/ **/hfuzz_workspace/ .cargo/ +.cargo-remote.toml diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 2d5825abc906c..63c3ca97d08ed 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -441,6 +441,7 @@ parameter_types! { pub const MaxIterations: u32 = 10; // 0.05%. The higher the value, the more strict solution acceptance becomes. pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); + pub MaximumUnsignedWeight: Weight = PerbilL::from_percent(90) * MaximumBlockWeight::get(); } impl pallet_staking::Trait for Runtime { @@ -469,6 +470,7 @@ impl pallet_staking::Trait for Runtime { type MinSolutionScoreBump = MinSolutionScoreBump; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = StakingUnsignedPriority; + type MaximumUnsignedWeight = StakingUnsignedPriority; type WeightInfo = weights::pallet_staking::WeightInfo; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 3adad1c4bf794..c36217cb65ce1 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -218,6 +218,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = StakingUnsignedPriority; type MaxIterations = (); type MinSolutionScoreBump = (); + type MaximumUnsignedWeight = (); type WeightInfo = (); } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 215a1f0a35932..3006f18b9a097 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -233,6 +233,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = StakingUnsignedPriority; type MaxIterations = (); type MinSolutionScoreBump = (); + type MaximumUnsignedWeight = (); type WeightInfo = (); } diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index c247e9a3779b4..85a506bde9c3f 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -183,6 +183,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = (); type MaxIterations = (); type MinSolutionScoreBump = (); + type MaximumUnsignedWeight = (); type WeightInfo = (); } diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index 645ac14d88a44..5e865779052c6 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -190,6 +190,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = UnsignedPriority; type MaxIterations = (); type MinSolutionScoreBump = (); + type MaximumUnsignedWeight = (); type WeightInfo = (); } diff --git a/frame/staking/fuzzer/src/mock.rs b/frame/staking/fuzzer/src/mock.rs index aae044d75acd9..1fcbbd116da0c 100644 --- a/frame/staking/fuzzer/src/mock.rs +++ b/frame/staking/fuzzer/src/mock.rs @@ -195,5 +195,6 @@ impl pallet_staking::Trait for Test { type MinSolutionScoreBump = (); type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = (); + type MaximumUnsignedWeight = (); // TODO: this will now break the fuzzer.. this is complicated. type WeightInfo = (); } From ad19e02bdea5b67d29202b5963acf10bf5fb20eb Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 28 Sep 2020 09:05:51 +0200 Subject: [PATCH 04/20] Update frame/staking/src/offchain_election.rs Co-authored-by: Shawn Tabrizi --- frame/staking/src/offchain_election.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 4b7fc1f67d79d..c399480b5efd2 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -228,7 +228,7 @@ where .collect::>(); voters_sorted.sort_by_key(|(_, y)| *y); - // start removing from the leas stake. Iterate until we know enough have been removed. + // start removing from the least stake. Iterate until we know enough have been removed. let mut removed = 0; for (i, _stake) in voters_sorted .iter() From 08aec9867677f1dc76efd1b2da27bd4dfb374338 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 28 Sep 2020 09:05:59 +0200 Subject: [PATCH 05/20] Update frame/staking/src/offchain_election.rs Co-authored-by: Shawn Tabrizi --- frame/staking/src/offchain_election.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index c399480b5efd2..2fc70a51e0d39 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -191,7 +191,7 @@ pub fn maximum_compact_len( voters } -/// Greedily reduce the size of the a solution to fit into the block, wrt weight. +/// Greedily reduce the size of the a solution to fit into the block, w.r.t. weight. /// /// The weight of the solution is foremost a function of the number of voters (i.e. /// `compact.len()`). Aside from this, the other components of the weight are invariant. The number From a1feaf8472948349ac2864966914d47779456807 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 28 Sep 2020 10:10:25 +0200 Subject: [PATCH 06/20] Make it build, binary search --- bin/node/runtime/src/lib.rs | 4 +- frame/staking/src/benchmarking.rs | 4 +- frame/staking/src/mock.rs | 5 +- frame/staking/src/offchain_election.rs | 168 +++++++++++++++++++++---- frame/staking/src/testing_utils.rs | 41 ++++-- 5 files changed, 184 insertions(+), 38 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 63c3ca97d08ed..1d908e39271d5 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -441,7 +441,9 @@ parameter_types! { pub const MaxIterations: u32 = 10; // 0.05%. The higher the value, the more strict solution acceptance becomes. pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); - pub MaximumUnsignedWeight: Weight = PerbilL::from_percent(90) * MaximumBlockWeight::get(); + // The unsigned solution is allowed to take almost the entire block. Note that it is also + // operational. + pub MaximumUnsignedWeight: Weight = Perbill::from_percent(90) * MaximumBlockWeight::get(); } impl pallet_staking::Trait for Runtime { diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index afda58db4672f..92b017b99ed93 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -502,7 +502,7 @@ benchmarks! { compact, score, size - ) = offchain_election::prepare_submission::(assignments, winners, false).unwrap(); + ) = offchain_election::prepare_submission::(assignments, winners, false, T::MaximumBlockWeight::get()).unwrap(); assert_eq!( winners.len(), compact.unique_targets().len(), @@ -570,7 +570,7 @@ benchmarks! { compact, score, size - ) = offchain_election::prepare_submission::(assignments, winners, false).unwrap(); + ) = offchain_election::prepare_submission::(assignments, winners, false, T::MaximumBlockWeight::get()).unwrap(); assert_eq!( winners.len(), compact.unique_targets().len(), diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index c9ce93719c200..d136ff28a095a 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -21,10 +21,7 @@ use crate::*; use frame_support::{ assert_ok, impl_outer_dispatch, impl_outer_event, impl_outer_origin, parameter_types, traits::{Currency, FindAuthor, Get, OnFinalize, OnInitialize}, - weights::{ - constants::{RocksDbWeight, WEIGHT_PER_SECOND}, - Weight, - }, + weights::{constants::RocksDbWeight, Weight}, IterableStorageMap, StorageDoubleMap, StorageMap, StorageValue, }; use sp_core::H256; diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 4b7fc1f67d79d..cfe20ae5be2e3 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -22,7 +22,7 @@ use crate::{ Trait, ValidatorIndex, WeightInfo, }; use codec::Decode; -use frame_support::{traits::Get, weights::Weight}; +use frame_support::{traits::Get, weights::Weight, IterableStorageMap}; use frame_system::offchain::SubmitTransaction; use sp_npos_elections::{ build_support_map, evaluate_support, reduce, Assignment, ElectionResult, ElectionScore, @@ -142,15 +142,6 @@ pub(crate) fn compute_offchain_election() -> Result<(), OffchainElecti .map_err(|_| OffchainElectionError::PoolSubmissionFailed) } -/// A greedy effort to reduce the size if a solution. -// pub fn trim_solution_to_fit( -// winners: Vec, -// compact: CompactAssignments, -// size: ElectionSize, -// ) -> (Vec, CompactAssignments, ElectionSize) { - -// } - /// Get a random number of iterations to run the balancing. /// /// Uses the offchain seed to generate a random number. @@ -167,27 +158,55 @@ pub fn get_balancing_iters() -> usize { } /// Find the maximum `len` that a compact can have in order to fit into the block weight. -pub fn maximum_compact_len( +/// +/// We start by assuming `voters == size.nominators`, i.e. all nominators are active in the +/// solution. Then we do a binary search until we find the ideal value. +/// +/// Thus, this only returns a value between zero and `size.nominators`. +pub fn maximum_compact_len( winners_len: u32, size: ElectionSize, max_weight: Weight, ) -> u32 { - // TODO: ofc we can do this nicer, if we rely on the internals of the weight function :\ - // TODO: check infinite loop is not possible. - let mut voters = 0; - loop { - let weight = T::WeightInfo::submit_solution_better( + use sp_std::cmp::Ordering; + let weight_with = |voters: u32| -> Weight { + W::submit_solution_better( size.validators.into(), size.nominators.into(), voters, winners_len, - ); - if weight < max_weight { - voters += 1; - } else { + ) + }; + + let mut voters = size.nominators.max(2); + let mut step = voters; + loop { + let current = weight_with(voters); + step = step / 2; + if step == 0 { + // We might have reduced less than expected due to rounding error. Reduce one last time. + while weight_with(voters) > max_weight { + voters -= 1; + } + // For the opposite case. + while weight_with(voters + 1) < max_weight { + voters += 1; + } break; } + match current.cmp(&max_weight) { + Ordering::Less => { + voters += step; + } + Ordering::Greater => { + voters -= step; + } + Ordering::Equal => { + break; + } + } } + voters } @@ -214,8 +233,6 @@ pub fn trim_to_weight( where for<'r> FN: Fn(&'r T::AccountId) -> Option, { - use frame_support::IterableStorageMap; - if let Some(to_remove) = compact.len().checked_sub(maximum_allowed_voters as usize) { // grab all voters and sort them by least stake. let mut voters_sorted = >::iter() @@ -346,7 +363,7 @@ where // potentially reduce the size of the compact to fit weight. let maximum_allowed_voters = - maximum_compact_len::(winners.len() as u32, size, maximum_weight); + maximum_compact_len::(winners.len() as u32, size, maximum_weight); crate::log!(info, "Maximum weight = {:?} // current weight = {:?} // maximum voters = {:?} // current votes = {:?}", maximum_weight, @@ -391,3 +408,108 @@ where Ok((winners_indexed, compact, score, size)) } + +#[cfg(test)] +mod test { + #![allow(unused_variables)] + use super::*; + use crate::ElectionSize; + + struct Staking; + + impl crate::WeightInfo for Staking { + fn bond() -> Weight { + unimplemented!() + } + fn bond_extra() -> Weight { + unimplemented!() + } + fn unbond() -> Weight { + unimplemented!() + } + fn withdraw_unbonded_update(s: u32) -> Weight { + unimplemented!() + } + fn withdraw_unbonded_kill(s: u32) -> Weight { + unimplemented!() + } + fn validate() -> Weight { + unimplemented!() + } + fn nominate(n: u32) -> Weight { + unimplemented!() + } + fn chill() -> Weight { + unimplemented!() + } + fn set_payee() -> Weight { + unimplemented!() + } + fn set_controller() -> Weight { + unimplemented!() + } + fn set_validator_count() -> Weight { + unimplemented!() + } + fn force_no_eras() -> Weight { + unimplemented!() + } + fn force_new_era() -> Weight { + unimplemented!() + } + fn force_new_era_always() -> Weight { + unimplemented!() + } + fn set_invulnerables(v: u32) -> Weight { + unimplemented!() + } + fn force_unstake(s: u32) -> Weight { + unimplemented!() + } + fn cancel_deferred_slash(s: u32) -> Weight { + unimplemented!() + } + fn payout_stakers_dead_controller(n: u32) -> Weight { + unimplemented!() + } + fn payout_stakers_alive_staked(n: u32) -> Weight { + unimplemented!() + } + fn rebond(l: u32) -> Weight { + unimplemented!() + } + fn set_history_depth(e: u32) -> Weight { + unimplemented!() + } + fn reap_stash(s: u32) -> Weight { + unimplemented!() + } + fn new_era(v: u32, n: u32) -> Weight { + unimplemented!() + } + fn submit_solution_better(v: u32, n: u32, a: u32, w: u32) -> Weight { + (0 * v + 0 * n + 1000 * a + 0 * w) as Weight + } + } + + #[test] + fn find_max_voter_binary_search_works() { + let size = ElectionSize { + validators: 0, + nominators: 10, + }; + + assert_eq!(maximum_compact_len::(0, size, 999), 0); + assert_eq!(maximum_compact_len::(0, size, 1000), 1); + assert_eq!(maximum_compact_len::(0, size, 1001), 1); + assert_eq!(maximum_compact_len::(0, size, 1999), 1); + assert_eq!(maximum_compact_len::(0, size, 2000), 2); + assert_eq!(maximum_compact_len::(0, size, 3333), 3); + assert_eq!(maximum_compact_len::(0, size, 5500), 5); + assert_eq!(maximum_compact_len::(0, size, 7777), 7); + assert_eq!(maximum_compact_len::(0, size, 9999), 9); + assert_eq!(maximum_compact_len::(0, size, 10_000), 10); + assert_eq!(maximum_compact_len::(0, size, 10_999), 10); + assert_eq!(maximum_compact_len::(0, size, 11_000), 11); + } +} diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 3d3688b6c03a0..a6660de1ebbd6 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -277,7 +277,12 @@ pub fn get_weak_solution( /// worker code. pub fn get_seq_phragmen_solution( do_reduce: bool, -) -> (Vec, CompactAssignments, ElectionScore, ElectionSize) { +) -> ( + Vec, + CompactAssignments, + ElectionScore, + ElectionSize, +) { let iters = offchain_election::get_balancing_iters::(); let sp_npos_elections::ElectionResult { @@ -285,22 +290,42 @@ pub fn get_seq_phragmen_solution( assignments, } = >::do_phragmen::(iters).unwrap(); - offchain_election::prepare_submission::(assignments, winners, do_reduce).unwrap() + offchain_election::prepare_submission::( + assignments, + winners, + do_reduce, + T::MaximumBlockWeight::get(), + ) + .unwrap() } /// Returns a solution in which only one winner is elected with just a self vote. pub fn get_single_winner_solution( - winner: T::AccountId -) -> Result<(Vec, CompactAssignments, ElectionScore, ElectionSize), &'static str> { + winner: T::AccountId, +) -> Result< + ( + Vec, + CompactAssignments, + ElectionScore, + ElectionSize, + ), + &'static str, +> { let snapshot_validators = >::snapshot_validators().unwrap(); let snapshot_nominators = >::snapshot_nominators().unwrap(); - let val_index = snapshot_validators.iter().position(|x| *x == winner).ok_or("not a validator")?; - let nom_index = snapshot_nominators.iter().position(|x| *x == winner).ok_or("not a nominator")?; + let val_index = snapshot_validators + .iter() + .position(|x| *x == winner) + .ok_or("not a validator")?; + let nom_index = snapshot_nominators + .iter() + .position(|x| *x == winner) + .ok_or("not a nominator")?; let stake = >::slashable_balance_of(&winner); - let stake = , VoteWeight>>::convert(stake) - as ExtendedBalance; + let stake = + , VoteWeight>>::convert(stake) as ExtendedBalance; let val_index = val_index as ValidatorIndex; let nom_index = nom_index as NominatorIndex; From 2274196c7755807c9d3d4532e45798b94a3e91c7 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 28 Sep 2020 15:19:46 +0200 Subject: [PATCH 07/20] Fix a number of grumbles --- bin/node/runtime/src/lib.rs | 9 +-- frame/babe/src/mock.rs | 2 +- frame/grandpa/src/mock.rs | 2 +- frame/offences/benchmarking/src/mock.rs | 2 +- frame/session/benchmarking/src/mock.rs | 2 +- frame/staking/fuzzer/src/mock.rs | 2 +- frame/staking/src/lib.rs | 12 +++- frame/staking/src/mock.rs | 6 +- frame/staking/src/offchain_election.rs | 58 +++++++++++++++++--- frame/staking/src/tests.rs | 6 +- frame/system/src/extensions/check_weight.rs | 14 +++-- primitives/npos-elections/compact/src/lib.rs | 19 +++---- 12 files changed, 94 insertions(+), 40 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 1d908e39271d5..c2e4ccb28af4d 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -20,10 +20,10 @@ #![cfg_attr(not(feature = "std"), no_std)] // `construct_runtime!` does a lot of recursion and requires us to increase the limit to 256. -#![recursion_limit="256"] +#![recursion_limit = "256"] -use sp_std::prelude::*; +use sp_std::prelude::*; use frame_support::{ construct_runtime, parameter_types, debug, RuntimeDebug, weights::{ @@ -443,7 +443,8 @@ parameter_types! { pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); // The unsigned solution is allowed to take almost the entire block. Note that it is also // operational. - pub MaximumUnsignedWeight: Weight = Perbill::from_percent(90) * MaximumBlockWeight::get(); + pub OffchainSolutionWeightLimit: Weight = + MaximumBlockWeight::get().saturating_sub(BlockExecutionWeight::get()); } impl pallet_staking::Trait for Runtime { @@ -472,7 +473,7 @@ impl pallet_staking::Trait for Runtime { type MinSolutionScoreBump = MinSolutionScoreBump; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = StakingUnsignedPriority; - type MaximumUnsignedWeight = StakingUnsignedPriority; + type OffchainSolutionWeightLimit = OffchainSolutionWeightLimit; type WeightInfo = weights::pallet_staking::WeightInfo; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index c36217cb65ce1..b820a3108daf1 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -218,7 +218,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = StakingUnsignedPriority; type MaxIterations = (); type MinSolutionScoreBump = (); - type MaximumUnsignedWeight = (); + type OffchainSolutionWeightLimit = (); type WeightInfo = (); } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 3006f18b9a097..1244c97c1c4a6 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -233,7 +233,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = StakingUnsignedPriority; type MaxIterations = (); type MinSolutionScoreBump = (); - type MaximumUnsignedWeight = (); + type OffchainSolutionWeightLimit = (); type WeightInfo = (); } diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 85a506bde9c3f..685aba012d45a 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -183,7 +183,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = (); type MaxIterations = (); type MinSolutionScoreBump = (); - type MaximumUnsignedWeight = (); + type OffchainSolutionWeightLimit = (); type WeightInfo = (); } diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index 5e865779052c6..2ba92c26530a3 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -190,7 +190,7 @@ impl pallet_staking::Trait for Test { type UnsignedPriority = UnsignedPriority; type MaxIterations = (); type MinSolutionScoreBump = (); - type MaximumUnsignedWeight = (); + type OffchainSolutionWeightLimit = (); type WeightInfo = (); } diff --git a/frame/staking/fuzzer/src/mock.rs b/frame/staking/fuzzer/src/mock.rs index 1fcbbd116da0c..ac7c7efe43d46 100644 --- a/frame/staking/fuzzer/src/mock.rs +++ b/frame/staking/fuzzer/src/mock.rs @@ -195,6 +195,6 @@ impl pallet_staking::Trait for Test { type MinSolutionScoreBump = (); type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = (); - type MaximumUnsignedWeight = (); // TODO: this will now break the fuzzer.. this is complicated. + type OffchainSolutionWeightLimit = (); type WeightInfo = (); } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 80a0f9e34b07a..37a0ac81f6a83 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -884,7 +884,7 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes> { type UnsignedPriority: Get; /// Maximum weight that the unsigned transaction can have. - type MaximumUnsignedWeight: Get; + type OffchainSolutionWeightLimit: Get; /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; @@ -1376,6 +1376,16 @@ decl_module! { ) ); + // Offchain solution is operational, so it can consume all the block weight except for + // `System::BlockExecutionWeight`. So setting the limit above this is invalid. + assert!( + T::OffchainSolutionWeightLimit::get() <= + ( + ::MaximumBlockWeight::get() - + ::BlockExecutionWeight::get() + ) + ); + use sp_runtime::UpperOf; // see the documentation of `Assignment::try_normalize`. Now we can ensure that this // will always return `Ok`. diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index d136ff28a095a..4e9da1a3bf730 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -199,7 +199,7 @@ pub struct Test; parameter_types! { pub const BlockHashCount: u64 = 250; - pub const MaximumBlockWeight: Weight = 1024; + pub const MaximumBlockWeight: Weight = frame_support::weights::constants::WEIGHT_PER_SECOND * 2; pub const MaximumBlockLength: u32 = 2 * 1024; pub const AvailableBlockRatio: Perbill = Perbill::one(); pub const MaxLocks: u32 = 1024; @@ -298,7 +298,7 @@ parameter_types! { pub const MaxNominatorRewardedPerValidator: u32 = 64; pub const UnsignedPriority: u64 = 1 << 20; pub const MinSolutionScoreBump: Perbill = Perbill::zero(); - pub const MaximumUnsignedWeight: Weight = MaximumBlockWeight::get(); + pub const OffchainSolutionWeightLimit: Weight = MaximumBlockWeight::get(); } thread_local! { @@ -337,7 +337,7 @@ impl Trait for Test { type MinSolutionScoreBump = MinSolutionScoreBump; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = UnsignedPriority; - type MaximumUnsignedWeight = MaximumUnsignedWeight; + type OffchainSolutionWeightLimit = OffchainSolutionWeightLimit; type WeightInfo = (); } diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index c1e13f5cd56ad..ed24c76220d95 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -116,8 +116,12 @@ pub(crate) fn compute_offchain_election() -> Result<(), OffchainElecti .ok_or(OffchainElectionError::ElectionFailed)?; // process and prepare it for submission. - let (winners, compact, score, size) = - prepare_submission::(assignments, winners, true, T::MaximumUnsignedWeight::get())?; + let (winners, compact, score, size) = prepare_submission::( + assignments, + winners, + true, + T::OffchainSolutionWeightLimit::get(), + )?; crate::log!( info, @@ -169,6 +173,10 @@ pub fn maximum_compact_len( max_weight: Weight, ) -> u32 { use sp_std::cmp::Ordering; + if size.nominators < 1 { + return size.nominators; + } + let weight_with = |voters: u32| -> Weight { W::submit_solution_better( size.validators.into(), @@ -184,9 +192,14 @@ pub fn maximum_compact_len( let current = weight_with(voters); step = step / 2; if step == 0 { - // We might have reduced less than expected due to rounding error. Reduce one last time. + // We might have reduced less than expected due to rounding error. Reduce linearly one + // last time. while weight_with(voters) > max_weight { - voters -= 1; + if let Some(next) = voters.checked_sub(1) { + voters = next + } else { + break; + } } // For the opposite case. while weight_with(voters + 1) < max_weight { @@ -207,7 +220,7 @@ pub fn maximum_compact_len( } } - voters + voters.min(size.nominators) } /// Greedily reduce the size of the a solution to fit into the block, w.r.t. weight. @@ -268,8 +281,9 @@ where crate::log!( warn, - "{} voters had to be removed from compact solution due to size limits.", - removed + "{} voters of {} had to be removed from compact solution due to size limits.", + removed, + compact.len() + removed, ); compact } else { @@ -499,6 +513,7 @@ mod test { nominators: 10, }; + assert_eq!(maximum_compact_len::(0, size, 1), 0); assert_eq!(maximum_compact_len::(0, size, 999), 0); assert_eq!(maximum_compact_len::(0, size, 1000), 1); assert_eq!(maximum_compact_len::(0, size, 1001), 1); @@ -510,6 +525,33 @@ mod test { assert_eq!(maximum_compact_len::(0, size, 9999), 9); assert_eq!(maximum_compact_len::(0, size, 10_000), 10); assert_eq!(maximum_compact_len::(0, size, 10_999), 10); - assert_eq!(maximum_compact_len::(0, size, 11_000), 11); + assert_eq!(maximum_compact_len::(0, size, 11_000), 10); + assert_eq!(maximum_compact_len::(0, size, 22_000), 10); + + let size = ElectionSize { + validators: 0, + nominators: 1, + }; + + assert_eq!(maximum_compact_len::(0, size, 1), 0); + assert_eq!(maximum_compact_len::(0, size, 999), 0); + assert_eq!(maximum_compact_len::(0, size, 1000), 1); + assert_eq!(maximum_compact_len::(0, size, 1001), 1); + assert_eq!(maximum_compact_len::(0, size, 1999), 1); + assert_eq!(maximum_compact_len::(0, size, 2000), 1); + assert_eq!(maximum_compact_len::(0, size, 3333), 1); + + let size = ElectionSize { + validators: 0, + nominators: 2, + }; + + assert_eq!(maximum_compact_len::(0, size, 1), 0); + assert_eq!(maximum_compact_len::(0, size, 999), 0); + assert_eq!(maximum_compact_len::(0, size, 1000), 1); + assert_eq!(maximum_compact_len::(0, size, 1001), 1); + assert_eq!(maximum_compact_len::(0, size, 1999), 1); + assert_eq!(maximum_compact_len::(0, size, 2000), 2); + assert_eq!(maximum_compact_len::(0, size, 3333), 2); } } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index a568214f9a2e8..2a02d87aa2c57 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3100,7 +3100,7 @@ mod offchain_election { } #[test] - #[ignore] // This takes a few mins + #[ignore] fn offchain_wont_work_if_snapshot_fails() { ExtBuilder::default() .offchain_election_ext() @@ -3382,8 +3382,8 @@ mod offchain_election { } #[test] - fn offchain_worker_runs_with_equalise() { - // Offchain worker equalises based on the number provided by randomness. See the difference + fn offchain_worker_runs_with_balancing() { + // Offchain worker balances based on the number provided by randomness. See the difference // in the priority, which comes from the computed score. let mut ext = ExtBuilder::default() .offchain_election_ext() diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 092ac59da97c8..126e65989a520 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -35,8 +35,9 @@ use frame_support::{ #[derive(Encode, Decode, Clone, Eq, PartialEq, Default)] pub struct CheckWeight(sp_std::marker::PhantomData); -impl CheckWeight where - T::Call: Dispatchable +impl CheckWeight +where + T::Call: Dispatchable, { /// Get the quota ratio of each dispatch class type. This indicates that all operational and mandatory /// dispatches can use the full capacity of any resource, while user-triggered ones can consume @@ -59,9 +60,9 @@ impl CheckWeight where DispatchClass::Mandatory => Ok(()), // Normal transactions must not exceed `MaximumExtrinsicWeight`. DispatchClass::Normal => { - let maximum_weight = T::MaximumExtrinsicWeight::get(); + let maximum_extrinsic_weight = T::MaximumExtrinsicWeight::get(); let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); - if extrinsic_weight > maximum_weight { + if extrinsic_weight > maximum_extrinsic_weight { Err(InvalidTransaction::ExhaustsResources.into()) } else { Ok(()) @@ -70,11 +71,12 @@ impl CheckWeight where // For operational transactions we make sure it doesn't exceed // the space alloted for `Operational` class. DispatchClass::Operational => { - let maximum_weight = T::MaximumBlockWeight::get(); + let maximum_block_weight = T::MaximumBlockWeight::get(); let operational_limit = - Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_block_weight; let operational_limit = operational_limit.saturating_sub(T::BlockExecutionWeight::get()); + let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); if extrinsic_weight > operational_limit { Err(InvalidTransaction::ExhaustsResources.into()) diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index 4c0184e638853..b35c407c40cd5 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -238,29 +238,28 @@ fn struct_def( } fn remove_voter_impl(count: usize) -> TokenStream2 { - // 1s - let filed_name = field_name_for(1); + let field_name = field_name_for(1); let single = quote! { - if let Some(idx) = self.#filed_name.iter().position(|(x, _)| *x == to_remove) { - self.#filed_name.remove(idx); + if let Some(idx) = self.#field_name.iter().position(|(x, _)| *x == to_remove) { + self.#field_name.remove(idx); return true } }; - let filed_name = field_name_for(2); + let field_name = field_name_for(2); let double = quote! { - if let Some(idx) = self.#filed_name.iter().position(|(x, _, _)| *x == to_remove) { - self.#filed_name.remove(idx); + if let Some(idx) = self.#field_name.iter().position(|(x, _, _)| *x == to_remove) { + self.#field_name.remove(idx); return true } }; let rest = (3..=count) .map(|c| { - let filed_name = field_name_for(c); + let field_name = field_name_for(c); quote! { - if let Some(idx) = self.#filed_name.iter().position(|(x, _, _)| *x == to_remove) { - self.#filed_name.remove(idx); + if let Some(idx) = self.#field_name.iter().position(|(x, _, _)| *x == to_remove) { + self.#field_name.remove(idx); return true } } From 2572e4be8dc7e84b122085b1b789a4ff0f3f19ea Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 28 Sep 2020 15:21:23 +0200 Subject: [PATCH 08/20] one more fix. --- frame/staking/src/offchain_election.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index ed24c76220d95..87c49d145c606 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -203,6 +203,9 @@ pub fn maximum_compact_len( } // For the opposite case. while weight_with(voters + 1) < max_weight { + if voters + 1 >= size.nominators { + break + } voters += 1; } break; From 3f53149fb9542dd14d969fe5f64eb7c846be3417 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 28 Sep 2020 15:28:03 +0200 Subject: [PATCH 09/20] remove unwrap. --- frame/staking/src/offchain_election.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 87c49d145c606..d12b0dccab320 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -47,6 +47,8 @@ pub enum OffchainElectionError { InternalElectionError(sp_npos_elections::Error), /// One of the computed winners is invalid. InvalidWinner, + /// A nominator is not available in the snapshot. + NominatorSnapshotCorrupt, } impl From for OffchainElectionError { @@ -245,7 +247,7 @@ pub fn trim_to_weight( maximum_allowed_voters: u32, mut compact: CompactAssignments, nominator_index: FN, -) -> CompactAssignments +) -> Result where for<'r> FN: Fn(&'r T::AccountId) -> Option, { @@ -263,15 +265,16 @@ where // start removing from the least stake. Iterate until we know enough have been removed. let mut removed = 0; - for (i, _stake) in voters_sorted + for (maybe_index, _stake) in voters_sorted .iter() - .map(|(who, stake)| (nominator_index(&who).unwrap(), stake)) + .map(|(who, stake)| (nominator_index(&who), stake)) { - if compact.remove_voter(i) { + let index = maybe_index.ok_or(OffchainElectionError::NominatorSnapshotCorrupt)?; + if compact.remove_voter(index) { crate::log!( debug, "removed a voter at index {} with stake {:?} from compact to reduce the size", - i, + index, _stake, ); removed += 1 @@ -288,14 +291,14 @@ where removed, compact.len() + removed, ); - compact + Ok(compact) } else { // nada, return as-is crate::log!( info, "Compact solution did not get trimmed due to block weight limits.", ); - compact + Ok(compact) } } @@ -394,7 +397,7 @@ where compact.len(), ); - let compact = trim_to_weight::(maximum_allowed_voters, compact, &nominator_index); + let compact = trim_to_weight::(maximum_allowed_voters, compact, &nominator_index)?; // re-compute the score. We re-create what the chain will do. let score = { From cc89113f3fb680daf4ae1762e67f791efadd3271 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 28 Sep 2020 16:39:06 +0200 Subject: [PATCH 10/20] better alg. --- bin/node/runtime/src/lib.rs | 9 ++++++--- frame/staking/src/lib.rs | 7 ++++++- frame/staking/src/offchain_election.rs | 14 ++++++++------ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index c2e4ccb28af4d..a04a11e386c60 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -441,10 +441,13 @@ parameter_types! { pub const MaxIterations: u32 = 10; // 0.05%. The higher the value, the more strict solution acceptance becomes. pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); - // The unsigned solution is allowed to take almost the entire block. Note that it is also - // operational. + // The unsigned solution is operational, so as long as the average on_initialize weights are + // less than `MaximumBlockWeight * (1 - T::AvailableRatio)`, it can consume at most most this + // amount. pub OffchainSolutionWeightLimit: Weight = - MaximumBlockWeight::get().saturating_sub(BlockExecutionWeight::get()); + MaximumBlockWeight::get() + .saturating_sub(BlockExecutionWeight::get()) + .saturating_sub(ExtrinsicBaseWeight::get()); } impl pallet_staking::Trait for Runtime { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 37a0ac81f6a83..a159e497ac697 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -884,6 +884,10 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes> { type UnsignedPriority: Get; /// Maximum weight that the unsigned transaction can have. + /// + /// Chose this value with care. On one hand, it should be as high as possible, so the solution + /// can contain as many nominators/validators as possible. On the other hand, it should be small + /// enough to fit in the block. type OffchainSolutionWeightLimit: Get; /// Weight information for extrinsics in this pallet. @@ -1382,7 +1386,8 @@ decl_module! { T::OffchainSolutionWeightLimit::get() <= ( ::MaximumBlockWeight::get() - - ::BlockExecutionWeight::get() + ::BlockExecutionWeight::get() - + ::ExtrinsicBaseWeight::get() ) ); diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index d12b0dccab320..1be2aaa92c28c 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -165,10 +165,7 @@ pub fn get_balancing_iters() -> usize { /// Find the maximum `len` that a compact can have in order to fit into the block weight. /// -/// We start by assuming `voters == size.nominators`, i.e. all nominators are active in the -/// solution. Then we do a binary search until we find the ideal value. -/// -/// Thus, this only returns a value between zero and `size.nominators`. +/// This only returns a value between zero and `size.nominators`. pub fn maximum_compact_len( winners_len: u32, size: ElectionSize, @@ -190,10 +187,11 @@ pub fn maximum_compact_len( let mut voters = size.nominators.max(2); let mut step = voters; - loop { + while voters <= size.nominators.max(2) { let current = weight_with(voters); step = step / 2; if step == 0 { + // Time to finish. // We might have reduced less than expected due to rounding error. Reduce linearly one // last time. while weight_with(voters) > max_weight { @@ -205,7 +203,7 @@ pub fn maximum_compact_len( } // For the opposite case. while weight_with(voters + 1) < max_weight { - if voters + 1 >= size.nominators { + if voters + 1 > size.nominators { break } voters += 1; @@ -215,6 +213,9 @@ pub fn maximum_compact_len( match current.cmp(&max_weight) { Ordering::Less => { voters += step; + if voters > size.nominators { + break; + } } Ordering::Greater => { voters -= step; @@ -225,6 +226,7 @@ pub fn maximum_compact_len( } } + // defensive only. We will never exceed `size.nominators`. voters.min(size.nominators) } From a72069f6f3beda037265d46d0e0147d55e801287 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 29 Sep 2020 18:32:26 +0200 Subject: [PATCH 11/20] Better alg again. --- frame/staking/src/offchain_election.rs | 106 ++++++++++++++++--------- 1 file changed, 69 insertions(+), 37 deletions(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 1be2aaa92c28c..945d7051238cb 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -172,10 +172,15 @@ pub fn maximum_compact_len( max_weight: Weight, ) -> u32 { use sp_std::cmp::Ordering; + if size.nominators < 1 { return size.nominators; } + let max_voters = size.nominators.max(1); + let mut voters = max_voters; + + // helper closures. let weight_with = |voters: u32| -> Weight { W::submit_solution_better( size.validators.into(), @@ -185,48 +190,70 @@ pub fn maximum_compact_len( ) }; - let mut voters = size.nominators.max(2); - let mut step = voters; - while voters <= size.nominators.max(2) { - let current = weight_with(voters); - step = step / 2; - if step == 0 { - // Time to finish. - // We might have reduced less than expected due to rounding error. Reduce linearly one - // last time. - while weight_with(voters) > max_weight { - if let Some(next) = voters.checked_sub(1) { - voters = next - } else { - break; - } - } - // For the opposite case. - while weight_with(voters + 1) < max_weight { - if voters + 1 > size.nominators { - break - } - voters += 1; - } - break; - } - match current.cmp(&max_weight) { + let next_voters = |current_weight: Weight, voters: u32, step: u32| -> Result { + match current_weight.cmp(&max_weight) { Ordering::Less => { - voters += step; - if voters > size.nominators { - break; + let next_voters = voters.checked_add(step); + match next_voters { + Some(voters) if voters < max_voters => Ok(voters), + _ => Err(()), } - } - Ordering::Greater => { - voters -= step; - } - Ordering::Equal => { + }, + Ordering::Greater => voters.checked_sub(step).ok_or(()), + Ordering::Equal => Ok(voters), + } + }; + + // First binary-search the right amount of voters + let mut step = voters / 2; + let mut current_weight = weight_with(voters); + while step > 0 { + match next_voters(current_weight, voters, step) { + // proceed with the binary search + Ok(next) if next != voters => { + voters = next; + }, + // we are out of bounds, break out of the loop. + Err(()) => { break; - } + }, + // we found the right value - early exit the function. + Ok(next) => return next } + step = step / 2; + current_weight = weight_with(voters); + } + + + // Time to finish. + // We might have reduced less than expected due to rounding error. Reduce linearly one last + // time. + let mut initialized = false; + let mut prev_voters = voters; + loop { + let next = next_voters(current_weight, voters, 1).unwrap_or(voters); + // no change? break. + if next == voters { + break + } + + // then we are stuck in a loop; + if prev_voters == next { + break; + } + + // skip 1 loop, then update these 3 variables in a sequence: + // prev_voters --> voter --> next + if !initialized { + initialized = true + } else { + prev_voters = voters + } + + voters = next; + current_weight = weight_with(voters); } - // defensive only. We will never exceed `size.nominators`. voters.min(size.nominators) } @@ -401,7 +428,9 @@ where let compact = trim_to_weight::(maximum_allowed_voters, compact, &nominator_index)?; - // re-compute the score. We re-create what the chain will do. + // re-compute the score. We re-create what the chain will do. This is a bit verbose and wastes + // CPU time, but it is necessary to ensure that the score that we claim is the same as the one + // calculated by the chain. let score = { let compact = compact.clone(); let assignments = compact.into_assignment(nominator_at, validator_at).unwrap(); @@ -521,6 +550,7 @@ mod test { nominators: 10, }; + assert_eq!(maximum_compact_len::(0, size, 0), 0); assert_eq!(maximum_compact_len::(0, size, 1), 0); assert_eq!(maximum_compact_len::(0, size, 999), 0); assert_eq!(maximum_compact_len::(0, size, 1000), 1); @@ -541,6 +571,7 @@ mod test { nominators: 1, }; + assert_eq!(maximum_compact_len::(0, size, 0), 0); assert_eq!(maximum_compact_len::(0, size, 1), 0); assert_eq!(maximum_compact_len::(0, size, 999), 0); assert_eq!(maximum_compact_len::(0, size, 1000), 1); @@ -554,6 +585,7 @@ mod test { nominators: 2, }; + assert_eq!(maximum_compact_len::(0, size, 0), 0); assert_eq!(maximum_compact_len::(0, size, 1), 0); assert_eq!(maximum_compact_len::(0, size, 999), 0); assert_eq!(maximum_compact_len::(0, size, 1000), 1); From 0e3c91f12fa4dbcfba60d088a516e5fa78f9cbfa Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 29 Sep 2020 21:29:52 +0200 Subject: [PATCH 12/20] Final fixes --- frame/staking/src/offchain_election.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 945d7051238cb..044b0ba183d40 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -254,6 +254,7 @@ pub fn maximum_compact_len( current_weight = weight_with(voters); } + debug_assert!(weight_with(voters) <= max_weight); voters.min(size.nominators) } @@ -309,7 +310,7 @@ where removed += 1 } - if removed > to_remove { + if removed >= to_remove { break; } } From 47d69f763ca6c854e9d12da23f3e330f0fb25e5c Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 30 Sep 2020 15:53:33 +0200 Subject: [PATCH 13/20] Fix --- bin/node/runtime/src/lib.rs | 8 +- frame/staking/src/lib.rs | 66 +++++++++----- frame/staking/src/offchain_election.rs | 97 +++++++++++---------- frame/system/src/extensions/check_weight.rs | 52 +++++++---- 4 files changed, 131 insertions(+), 92 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 03f8d45fc97db..630527cc49aee 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -441,13 +441,13 @@ parameter_types! { pub const MaxIterations: u32 = 10; // 0.05%. The higher the value, the more strict solution acceptance becomes. pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); - // The unsigned solution is operational, so as long as the average on_initialize weights are - // less than `MaximumBlockWeight * (1 - T::AvailableRatio)`, it can consume at most most this - // amount. + // The unsigned solution is mandatory, and this is the absolute maximum weight that we allow for + // it. pub OffchainSolutionWeightLimit: Weight = MaximumBlockWeight::get() .saturating_sub(BlockExecutionWeight::get()) - .saturating_sub(ExtrinsicBaseWeight::get()); + .saturating_sub(ExtrinsicBaseWeight::get()) + .saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT * MaximumBlockWeight::get()); } impl pallet_staking::Trait for Runtime { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index a159e497ac697..45a0a946a0048 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1239,6 +1239,8 @@ decl_error! { OffchainElectionBogusScore, /// The election size is invalid. OffchainElectionBogusElectionSize, + /// The election weight is invalid. + OffchainElectionBogusElectionWeight, /// The call is not allowed at the given time due to restrictions of election period. CallNotAllowed, /// Incorrect previous history depth input provided. @@ -1346,12 +1348,12 @@ decl_module! { if Self::era_election_status().is_open_at(now) { let offchain_status = set_check_offchain_execution_status::(now); if let Err(why) = offchain_status { - log!(debug, "skipping offchain worker in open election window due to [{}]", why); + log!(warn, "💸 skipping offchain worker in open election window due to [{}]", why); } else { if let Err(e) = compute_offchain_election::() { log!(error, "💸 Error in election offchain worker: {:?}", e); } else { - log!(debug, "Executed offchain worker thread without errors."); + log!(debug, "💸 Executed offchain worker thread without errors."); } } } @@ -1380,17 +1382,6 @@ decl_module! { ) ); - // Offchain solution is operational, so it can consume all the block weight except for - // `System::BlockExecutionWeight`. So setting the limit above this is invalid. - assert!( - T::OffchainSolutionWeightLimit::get() <= - ( - ::MaximumBlockWeight::get() - - ::BlockExecutionWeight::get() - - ::ExtrinsicBaseWeight::get() - ) - ); - use sp_runtime::UpperOf; // see the documentation of `Assignment::try_normalize`. Now we can ensure that this // will always return `Ok`. @@ -2159,15 +2150,15 @@ decl_module! { /// transaction in the block. /// /// # - /// See `crate::weight` module. + /// The weight of this call is not enforced by the system module and is enforced in this + /// module directly. /// # - #[weight = ( - T::WeightInfo::submit_solution_better( + #[weight = (T::WeightInfo::submit_solution_better( size.validators.into(), size.nominators.into(), compact.len() as u32, winners.len() as u32, - ), frame_support::weights::DispatchClass::Operational)] + ), frame_support::weights::DispatchClass::Mandatory)] pub fn submit_election_solution_unsigned( origin, winners: Vec, @@ -2177,6 +2168,13 @@ decl_module! { size: ElectionSize, ) -> DispatchResultWithPostInfo { ensure_none(origin)?; + // The signed solution is `Normal` and the system module checks the weight. But for this + // mandatory one, we need to do it. + ensure!( + Self::check_unsigned_solution_weight(&winners, &compact, &size), + Error::::OffchainElectionBogusElectionWeight.with_weight(0), + ); + let adjustments = Self::check_and_replace_solution( winners, compact, @@ -2190,6 +2188,7 @@ decl_module! { effectively depriving the validators from their authoring reward. Hence, this panic is expected." ); + Ok(adjustments) } } @@ -2209,6 +2208,21 @@ impl Module { ) } + /// Check the weight of an unsigned solution. Returns true of the weight is below the designated + /// limit . + pub fn check_unsigned_solution_weight( + winners: &Vec, + compact: &CompactAssignments, + size: &ElectionSize, + ) -> bool { + T::WeightInfo::submit_solution_better( + size.validators.into(), + size.nominators.into(), + compact.len() as u32, + winners.len() as u32, + ) <= T::OffchainSolutionWeightLimit::get() + } + /// Dump the list of validators and nominators into vectors and keep them on-chain. /// /// This data is used to efficiently evaluate election results. returns `true` if the operation @@ -3101,7 +3115,6 @@ impl Module { pub fn set_slash_reward_fraction(fraction: Perbill) { SlashRewardFraction::put(fraction); } - } /// In this implementation `new_session(session)` must be called before `end_session(session-1)` @@ -3365,11 +3378,11 @@ impl frame_support::unsigned::ValidateUnsigned for Module { type Call = Call; fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity { if let Call::submit_election_solution_unsigned( - _, - _, + winners, + compact, score, era, - _, + size, ) = call { use offchain_election::DEFAULT_LONGEVITY; @@ -3382,17 +3395,22 @@ impl frame_support::unsigned::ValidateUnsigned for Module { } } + // discard solution if it is too big. + if !Self::check_unsigned_solution_weight(winners, compact, size) { + return Err(InvalidTransaction::ExhaustsResources.into()) + } + if let Err(error_with_post_info) = Self::pre_dispatch_checks(*score, *era) { let invalid = to_invalid(error_with_post_info); log!( debug, - "validate unsigned pre dispatch checks failed due to error #{:?}.", + "💸 validate unsigned pre dispatch checks failed due to error #{:?}.", invalid, ); - return invalid .into(); + return invalid.into(); } - log!(debug, "validateUnsigned succeeded for a solution at era {}.", era); + log!(debug, "💸 validateUnsigned succeeded for a solution at era {}.", era); ValidTransaction::with_tag_prefix("StakingOffchain") // The higher the score[0], the better a solution is. diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 044b0ba183d40..f4ce05491b6e8 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -127,7 +127,7 @@ pub(crate) fn compute_offchain_election() -> Result<(), OffchainElecti crate::log!( info, - "prepared a seq-phragmen solution with {} balancing iterations and score {:?}", + "💸 prepared a seq-phragmen solution with {} balancing iterations and score {:?}", iters, score, ); @@ -281,54 +281,57 @@ pub fn trim_to_weight( where for<'r> FN: Fn(&'r T::AccountId) -> Option, { - if let Some(to_remove) = compact.len().checked_sub(maximum_allowed_voters as usize) { - // grab all voters and sort them by least stake. - let mut voters_sorted = >::iter() - .map(|(who, _)| { - ( - who.clone(), - >::slashable_balance_of_vote_weight(&who), - ) - }) - .collect::>(); - voters_sorted.sort_by_key(|(_, y)| *y); - - // start removing from the least stake. Iterate until we know enough have been removed. - let mut removed = 0; - for (maybe_index, _stake) in voters_sorted - .iter() - .map(|(who, stake)| (nominator_index(&who), stake)) - { - let index = maybe_index.ok_or(OffchainElectionError::NominatorSnapshotCorrupt)?; - if compact.remove_voter(index) { - crate::log!( - debug, - "removed a voter at index {} with stake {:?} from compact to reduce the size", - index, - _stake, - ); - removed += 1 - } + match compact.len().checked_sub(maximum_allowed_voters as usize) { + Some(to_remove) if to_remove > 0 => { + // grab all voters and sort them by least stake. + let mut voters_sorted = >::iter() + .map(|(who, _)| { + ( + who.clone(), + >::slashable_balance_of_vote_weight(&who), + ) + }) + .collect::>(); + voters_sorted.sort_by_key(|(_, y)| *y); + + // start removing from the least stake. Iterate until we know enough have been removed. + let mut removed = 0; + for (maybe_index, _stake) in voters_sorted + .iter() + .map(|(who, stake)| (nominator_index(&who), stake)) + { + let index = maybe_index.ok_or(OffchainElectionError::NominatorSnapshotCorrupt)?; + if compact.remove_voter(index) { + crate::log!( + trace, + "💸 removed a voter at index {} with stake {:?} from compact to reduce the size", + index, + _stake, + ); + removed += 1 + } - if removed >= to_remove { - break; + if removed >= to_remove { + break; + } } - } - crate::log!( - warn, - "{} voters of {} had to be removed from compact solution due to size limits.", - removed, - compact.len() + removed, - ); - Ok(compact) - } else { - // nada, return as-is - crate::log!( - info, - "Compact solution did not get trimmed due to block weight limits.", - ); - Ok(compact) + crate::log!( + warn, + "💸 {} nominators out of {} had to be removed from compact solution due to size limits.", + removed, + compact.len() + removed, + ); + Ok(compact) + } + _ => { + // nada, return as-is + crate::log!( + info, + "💸 Compact solution did not get trimmed due to block weight limits.", + ); + Ok(compact) + } } } @@ -415,7 +418,7 @@ where let maximum_allowed_voters = maximum_compact_len::(winners.len() as u32, size, maximum_weight); - crate::log!(info, "Maximum weight = {:?} // current weight = {:?} // maximum voters = {:?} // current votes = {:?}", + crate::log!(debug, "💸 Maximum weight = {:?} // current weight = {:?} // maximum voters = {:?} // current votes = {:?}", maximum_weight, T::WeightInfo::submit_solution_better( size.validators.into(), diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 126e65989a520..da47c82e06ab5 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -93,7 +93,7 @@ where fn check_block_weight( info: &DispatchInfoOf, ) -> Result { - let maximum_weight = T::MaximumBlockWeight::get(); + let maximum_block_weight = T::MaximumBlockWeight::get(); let mut all_weight = Module::::block_weight(); match info.class { // If we have a dispatch that must be included in the block, it ignores all the limits. @@ -101,36 +101,46 @@ where let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); all_weight.add(extrinsic_weight, DispatchClass::Mandatory); Ok(all_weight) - }, + } // If we have a normal dispatch, we follow all the normal rules and limits. DispatchClass::Normal => { - let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; - let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) + let normal_limit = + Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_block_weight; + let extrinsic_weight = info + .weight + .checked_add(T::ExtrinsicBaseWeight::get()) .ok_or(InvalidTransaction::ExhaustsResources)?; - all_weight.checked_add(extrinsic_weight, DispatchClass::Normal) + all_weight + .checked_add(extrinsic_weight, DispatchClass::Normal) .map_err(|_| InvalidTransaction::ExhaustsResources)?; if all_weight.get(DispatchClass::Normal) > normal_limit { Err(InvalidTransaction::ExhaustsResources.into()) } else { Ok(all_weight) } - }, + } // If we have an operational dispatch, allow it if we have not used our full // "operational space" (independent of existing fullness). DispatchClass::Operational => { - let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; - let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; + let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) + * maximum_block_weight; + let normal_limit = + Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_block_weight; let operational_space = operational_limit.saturating_sub(normal_limit); - let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) + let extrinsic_weight = info + .weight + .checked_add(T::ExtrinsicBaseWeight::get()) .ok_or(InvalidTransaction::ExhaustsResources)?; - all_weight.checked_add(extrinsic_weight, DispatchClass::Operational) + all_weight + .checked_add(extrinsic_weight, DispatchClass::Operational) .map_err(|_| InvalidTransaction::ExhaustsResources)?; // If it would fit in normally, its okay - if all_weight.total() <= maximum_weight || + if all_weight.total() <= maximum_block_weight || // If we have not used our operational space - all_weight.get(DispatchClass::Operational) <= operational_space { + all_weight.get(DispatchClass::Operational) <= operational_space + { Ok(all_weight) } else { Err(InvalidTransaction::ExhaustsResources.into()) @@ -373,6 +383,7 @@ mod tests { let operational_limit = CheckWeight::::get_dispatch_limit_ratio( DispatchClass::Operational ) * ::MaximumBlockWeight::get(); + let base_weight = ::ExtrinsicBaseWeight::get(); let block_base = ::BlockExecutionWeight::get(); @@ -382,7 +393,7 @@ mod tests { class: DispatchClass::Operational, ..Default::default() }; - let max = DispatchInfo { + let error = DispatchInfo { weight: weight + 1, class: DispatchClass::Operational, ..Default::default() @@ -397,14 +408,14 @@ mod tests { }) ); assert_noop!( - CheckWeight::::do_validate(&max, len), + CheckWeight::::do_validate(&error, len), InvalidTransaction::ExhaustsResources ); }); } #[test] - fn register_extra_weight_unchecked_doesnt_care_about_limits() { + fn register_extra_weight_unchecked_does_not_care_about_limits() { new_test_ext().execute_with(|| { System::register_extra_weight_unchecked(Weight::max_value(), DispatchClass::Normal); assert_eq!(System::block_weight().total(), Weight::max_value()); @@ -481,8 +492,15 @@ mod tests { #[test] fn signed_ext_check_weight_works_operational_tx() { new_test_ext().execute_with(|| { - let normal = DispatchInfo { weight: 100, ..Default::default() }; - let op = DispatchInfo { weight: 100, class: DispatchClass::Operational, pays_fee: Pays::Yes }; + let normal = DispatchInfo { + weight: 100, + ..Default::default() + }; + let op = DispatchInfo { + weight: 100, + class: DispatchClass::Operational, + pays_fee: Pays::Yes, + }; let len = 0_usize; let normal_limit = normal_weight_limit(); From 86d24a3dc613c88e64eb0a0c8991bb8adc5bfd20 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 30 Sep 2020 17:23:55 +0200 Subject: [PATCH 14/20] Rollback to normal --- bin/node/runtime/src/lib.rs | 11 +++-------- frame/staking/src/lib.rs | 28 +++++++--------------------- 2 files changed, 10 insertions(+), 29 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 630527cc49aee..e2dd95e798171 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -441,13 +441,6 @@ parameter_types! { pub const MaxIterations: u32 = 10; // 0.05%. The higher the value, the more strict solution acceptance becomes. pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); - // The unsigned solution is mandatory, and this is the absolute maximum weight that we allow for - // it. - pub OffchainSolutionWeightLimit: Weight = - MaximumBlockWeight::get() - .saturating_sub(BlockExecutionWeight::get()) - .saturating_sub(ExtrinsicBaseWeight::get()) - .saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT * MaximumBlockWeight::get()); } impl pallet_staking::Trait for Runtime { @@ -476,7 +469,9 @@ impl pallet_staking::Trait for Runtime { type MinSolutionScoreBump = MinSolutionScoreBump; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type UnsignedPriority = StakingUnsignedPriority; - type OffchainSolutionWeightLimit = OffchainSolutionWeightLimit; + // The unsigned solution weight targeted by the OCW. We set it to the maximum possible value of + // a single extrinsic. + type OffchainSolutionWeightLimit = MaximumExtrinsicWeight; type WeightInfo = weights::pallet_staking::WeightInfo; } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 45a0a946a0048..532aeab606a8d 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1239,8 +1239,6 @@ decl_error! { OffchainElectionBogusScore, /// The election size is invalid. OffchainElectionBogusElectionSize, - /// The election weight is invalid. - OffchainElectionBogusElectionWeight, /// The call is not allowed at the given time due to restrictions of election period. CallNotAllowed, /// Incorrect previous history depth input provided. @@ -1303,6 +1301,7 @@ decl_module! { consumed_weight += T::DbWeight::get().reads_writes(reads, writes); consumed_weight += weight; }; + if // if we don't have any ongoing offchain compute. Self::era_election_status().is_closed() && @@ -2150,15 +2149,14 @@ decl_module! { /// transaction in the block. /// /// # - /// The weight of this call is not enforced by the system module and is enforced in this - /// module directly. + /// See [`submit_election_solution`]. /// # - #[weight = (T::WeightInfo::submit_solution_better( + #[weight = T::WeightInfo::submit_solution_better( size.validators.into(), size.nominators.into(), compact.len() as u32, winners.len() as u32, - ), frame_support::weights::DispatchClass::Mandatory)] + )] pub fn submit_election_solution_unsigned( origin, winners: Vec, @@ -2168,13 +2166,6 @@ decl_module! { size: ElectionSize, ) -> DispatchResultWithPostInfo { ensure_none(origin)?; - // The signed solution is `Normal` and the system module checks the weight. But for this - // mandatory one, we need to do it. - ensure!( - Self::check_unsigned_solution_weight(&winners, &compact, &size), - Error::::OffchainElectionBogusElectionWeight.with_weight(0), - ); - let adjustments = Self::check_and_replace_solution( winners, compact, @@ -3378,11 +3369,11 @@ impl frame_support::unsigned::ValidateUnsigned for Module { type Call = Call; fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity { if let Call::submit_election_solution_unsigned( - winners, - compact, + _, + _, score, era, - size, + _, ) = call { use offchain_election::DEFAULT_LONGEVITY; @@ -3395,11 +3386,6 @@ impl frame_support::unsigned::ValidateUnsigned for Module { } } - // discard solution if it is too big. - if !Self::check_unsigned_solution_weight(winners, compact, size) { - return Err(InvalidTransaction::ExhaustsResources.into()) - } - if let Err(error_with_post_info) = Self::pre_dispatch_checks(*score, *era) { let invalid = to_invalid(error_with_post_info); log!( From 6b6ef3ee8764023e0ad3957f9b429e52d42fa91c Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 2 Oct 2020 10:28:42 +0200 Subject: [PATCH 15/20] Final touches. --- frame/system/src/extensions/check_weight.rs | 66 +++++++-------------- 1 file changed, 23 insertions(+), 43 deletions(-) diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index da47c82e06ab5..092ac59da97c8 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -35,9 +35,8 @@ use frame_support::{ #[derive(Encode, Decode, Clone, Eq, PartialEq, Default)] pub struct CheckWeight(sp_std::marker::PhantomData); -impl CheckWeight -where - T::Call: Dispatchable, +impl CheckWeight where + T::Call: Dispatchable { /// Get the quota ratio of each dispatch class type. This indicates that all operational and mandatory /// dispatches can use the full capacity of any resource, while user-triggered ones can consume @@ -60,9 +59,9 @@ where DispatchClass::Mandatory => Ok(()), // Normal transactions must not exceed `MaximumExtrinsicWeight`. DispatchClass::Normal => { - let maximum_extrinsic_weight = T::MaximumExtrinsicWeight::get(); + let maximum_weight = T::MaximumExtrinsicWeight::get(); let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); - if extrinsic_weight > maximum_extrinsic_weight { + if extrinsic_weight > maximum_weight { Err(InvalidTransaction::ExhaustsResources.into()) } else { Ok(()) @@ -71,12 +70,11 @@ where // For operational transactions we make sure it doesn't exceed // the space alloted for `Operational` class. DispatchClass::Operational => { - let maximum_block_weight = T::MaximumBlockWeight::get(); + let maximum_weight = T::MaximumBlockWeight::get(); let operational_limit = - Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_block_weight; + Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; let operational_limit = operational_limit.saturating_sub(T::BlockExecutionWeight::get()); - let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); if extrinsic_weight > operational_limit { Err(InvalidTransaction::ExhaustsResources.into()) @@ -93,7 +91,7 @@ where fn check_block_weight( info: &DispatchInfoOf, ) -> Result { - let maximum_block_weight = T::MaximumBlockWeight::get(); + let maximum_weight = T::MaximumBlockWeight::get(); let mut all_weight = Module::::block_weight(); match info.class { // If we have a dispatch that must be included in the block, it ignores all the limits. @@ -101,46 +99,36 @@ where let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); all_weight.add(extrinsic_weight, DispatchClass::Mandatory); Ok(all_weight) - } + }, // If we have a normal dispatch, we follow all the normal rules and limits. DispatchClass::Normal => { - let normal_limit = - Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_block_weight; - let extrinsic_weight = info - .weight - .checked_add(T::ExtrinsicBaseWeight::get()) + let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; + let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) .ok_or(InvalidTransaction::ExhaustsResources)?; - all_weight - .checked_add(extrinsic_weight, DispatchClass::Normal) + all_weight.checked_add(extrinsic_weight, DispatchClass::Normal) .map_err(|_| InvalidTransaction::ExhaustsResources)?; if all_weight.get(DispatchClass::Normal) > normal_limit { Err(InvalidTransaction::ExhaustsResources.into()) } else { Ok(all_weight) } - } + }, // If we have an operational dispatch, allow it if we have not used our full // "operational space" (independent of existing fullness). DispatchClass::Operational => { - let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) - * maximum_block_weight; - let normal_limit = - Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_block_weight; + let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; let operational_space = operational_limit.saturating_sub(normal_limit); - let extrinsic_weight = info - .weight - .checked_add(T::ExtrinsicBaseWeight::get()) + let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) .ok_or(InvalidTransaction::ExhaustsResources)?; - all_weight - .checked_add(extrinsic_weight, DispatchClass::Operational) + all_weight.checked_add(extrinsic_weight, DispatchClass::Operational) .map_err(|_| InvalidTransaction::ExhaustsResources)?; // If it would fit in normally, its okay - if all_weight.total() <= maximum_block_weight || + if all_weight.total() <= maximum_weight || // If we have not used our operational space - all_weight.get(DispatchClass::Operational) <= operational_space - { + all_weight.get(DispatchClass::Operational) <= operational_space { Ok(all_weight) } else { Err(InvalidTransaction::ExhaustsResources.into()) @@ -383,7 +371,6 @@ mod tests { let operational_limit = CheckWeight::::get_dispatch_limit_ratio( DispatchClass::Operational ) * ::MaximumBlockWeight::get(); - let base_weight = ::ExtrinsicBaseWeight::get(); let block_base = ::BlockExecutionWeight::get(); @@ -393,7 +380,7 @@ mod tests { class: DispatchClass::Operational, ..Default::default() }; - let error = DispatchInfo { + let max = DispatchInfo { weight: weight + 1, class: DispatchClass::Operational, ..Default::default() @@ -408,14 +395,14 @@ mod tests { }) ); assert_noop!( - CheckWeight::::do_validate(&error, len), + CheckWeight::::do_validate(&max, len), InvalidTransaction::ExhaustsResources ); }); } #[test] - fn register_extra_weight_unchecked_does_not_care_about_limits() { + fn register_extra_weight_unchecked_doesnt_care_about_limits() { new_test_ext().execute_with(|| { System::register_extra_weight_unchecked(Weight::max_value(), DispatchClass::Normal); assert_eq!(System::block_weight().total(), Weight::max_value()); @@ -492,15 +479,8 @@ mod tests { #[test] fn signed_ext_check_weight_works_operational_tx() { new_test_ext().execute_with(|| { - let normal = DispatchInfo { - weight: 100, - ..Default::default() - }; - let op = DispatchInfo { - weight: 100, - class: DispatchClass::Operational, - pays_fee: Pays::Yes, - }; + let normal = DispatchInfo { weight: 100, ..Default::default() }; + let op = DispatchInfo { weight: 100, class: DispatchClass::Operational, pays_fee: Pays::Yes }; let len = 0_usize; let normal_limit = normal_weight_limit(); From addd5358cfd422d9871e5c44d6d3bfd788685944 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 2 Oct 2020 10:56:49 +0200 Subject: [PATCH 16/20] Better tests. --- frame/staking/src/offchain_election.rs | 49 ++++++++++++-------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index f4ce05491b6e8..b896e7518740f 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -226,35 +226,19 @@ pub fn maximum_compact_len( // Time to finish. - // We might have reduced less than expected due to rounding error. Reduce linearly one last - // time. - let mut initialized = false; - let mut prev_voters = voters; - loop { - let next = next_voters(current_weight, voters, 1).unwrap_or(voters); - // no change? break. - if next == voters { - break - } - - // then we are stuck in a loop; - if prev_voters == next { - break; - } - - // skip 1 loop, then update these 3 variables in a sequence: - // prev_voters --> voter --> next - if !initialized { - initialized = true - } else { - prev_voters = voters - } - - voters = next; - current_weight = weight_with(voters); + // We might have reduced less than expected due to rounding error. Increase one last time if we + // have any room left, the reduce until we are sure we are below limit. + while weight_with(voters + 1) < max_weight { + voters += 1; + } + while voters.checked_sub(1).is_some() && weight_with(voters) > max_weight { + voters -= 1; } - debug_assert!(weight_with(voters) <= max_weight); + debug_assert!( + weight_with(voters.min(size.nominators)) <= max_weight, + "weight_with({}) <= {}", voters.min(size.nominators), max_weight, + ); voters.min(size.nominators) } @@ -559,8 +543,14 @@ mod test { assert_eq!(maximum_compact_len::(0, size, 999), 0); assert_eq!(maximum_compact_len::(0, size, 1000), 1); assert_eq!(maximum_compact_len::(0, size, 1001), 1); + assert_eq!(maximum_compact_len::(0, size, 1990), 1); assert_eq!(maximum_compact_len::(0, size, 1999), 1); assert_eq!(maximum_compact_len::(0, size, 2000), 2); + assert_eq!(maximum_compact_len::(0, size, 2001), 2); + assert_eq!(maximum_compact_len::(0, size, 2010), 2); + assert_eq!(maximum_compact_len::(0, size, 2990), 2); + assert_eq!(maximum_compact_len::(0, size, 2999), 2); + assert_eq!(maximum_compact_len::(0, size, 3000), 3); assert_eq!(maximum_compact_len::(0, size, 3333), 3); assert_eq!(maximum_compact_len::(0, size, 5500), 5); assert_eq!(maximum_compact_len::(0, size, 7777), 7); @@ -580,8 +570,11 @@ mod test { assert_eq!(maximum_compact_len::(0, size, 999), 0); assert_eq!(maximum_compact_len::(0, size, 1000), 1); assert_eq!(maximum_compact_len::(0, size, 1001), 1); + assert_eq!(maximum_compact_len::(0, size, 1990), 1); assert_eq!(maximum_compact_len::(0, size, 1999), 1); assert_eq!(maximum_compact_len::(0, size, 2000), 1); + assert_eq!(maximum_compact_len::(0, size, 2001), 1); + assert_eq!(maximum_compact_len::(0, size, 2010), 1); assert_eq!(maximum_compact_len::(0, size, 3333), 1); let size = ElectionSize { @@ -596,6 +589,8 @@ mod test { assert_eq!(maximum_compact_len::(0, size, 1001), 1); assert_eq!(maximum_compact_len::(0, size, 1999), 1); assert_eq!(maximum_compact_len::(0, size, 2000), 2); + assert_eq!(maximum_compact_len::(0, size, 2001), 2); + assert_eq!(maximum_compact_len::(0, size, 2010), 2); assert_eq!(maximum_compact_len::(0, size, 3333), 2); } } From e11ab9d4208802b7287261205e7b19a530591d6e Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Fri, 2 Oct 2020 13:18:03 +0200 Subject: [PATCH 17/20] Update frame/staking/src/lib.rs Co-authored-by: Guillaume Thiolliere --- frame/staking/src/lib.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 532aeab606a8d..362e71f17d602 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -2199,21 +2199,6 @@ impl Module { ) } - /// Check the weight of an unsigned solution. Returns true of the weight is below the designated - /// limit . - pub fn check_unsigned_solution_weight( - winners: &Vec, - compact: &CompactAssignments, - size: &ElectionSize, - ) -> bool { - T::WeightInfo::submit_solution_better( - size.validators.into(), - size.nominators.into(), - compact.len() as u32, - winners.len() as u32, - ) <= T::OffchainSolutionWeightLimit::get() - } - /// Dump the list of validators and nominators into vectors and keep them on-chain. /// /// This data is used to efficiently evaluate election results. returns `true` if the operation From f1d9f2b0850a590538946f11dfc6cd07742a1639 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 2 Oct 2020 13:31:55 +0200 Subject: [PATCH 18/20] Proper maxExtWeight --- bin/node/runtime/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 797757fdda5c7..a33b5aa92d141 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -150,8 +150,12 @@ parameter_types! { pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75); /// Assume 10% of weight for average on_initialize calls. pub MaximumExtrinsicWeight: Weight = - AvailableBlockRatio::get().saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT) - * MaximumBlockWeight::get(); + ( + AvailableBlockRatio::get().saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT) + * MaximumBlockWeight::get() + ) + .saturating_sub(BlockExecutionWeight::get()) + .saturating_sub(ExtrinsicBaseWeight::get()); pub const MaximumBlockLength: u32 = 5 * 1024 * 1024; pub const Version: RuntimeVersion = VERSION; } From 43b149c5ad48973ce74fa0b0408f82bd8df1ad57 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 2 Oct 2020 14:13:53 +0200 Subject: [PATCH 19/20] Final fix --- bin/node/runtime/src/lib.rs | 14 ++++++-------- frame/system/src/extensions/check_weight.rs | 4 ++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index a33b5aa92d141..f1793d40ee139 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -149,13 +149,8 @@ parameter_types! { pub const MaximumBlockWeight: Weight = 2 * WEIGHT_PER_SECOND; pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75); /// Assume 10% of weight for average on_initialize calls. - pub MaximumExtrinsicWeight: Weight = - ( - AvailableBlockRatio::get().saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT) - * MaximumBlockWeight::get() - ) - .saturating_sub(BlockExecutionWeight::get()) - .saturating_sub(ExtrinsicBaseWeight::get()); + pub MaximumExtrinsicWeight: Weight = AvailableBlockRatio::get().saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT) + * MaximumBlockWeight::get(); pub const MaximumBlockLength: u32 = 5 * 1024 * 1024; pub const Version: RuntimeVersion = VERSION; } @@ -445,6 +440,9 @@ parameter_types! { pub const MaxIterations: u32 = 10; // 0.05%. The higher the value, the more strict solution acceptance becomes. pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000); + pub OffchainSolutionWeightLimit: Weight = MaximumExtrinsicWeight::get() + .saturating_sub(BlockExecutionWeight::get()) + .saturating_sub(ExtrinsicBaseWeight::get()); } impl pallet_staking::Trait for Runtime { @@ -475,7 +473,7 @@ impl pallet_staking::Trait for Runtime { type UnsignedPriority = StakingUnsignedPriority; // The unsigned solution weight targeted by the OCW. We set it to the maximum possible value of // a single extrinsic. - type OffchainSolutionWeightLimit = MaximumExtrinsicWeight; + type OffchainSolutionWeightLimit = OffchainSolutionWeightLimit; type WeightInfo = weights::pallet_staking::WeightInfo; } diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 092ac59da97c8..30052468fe253 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -182,7 +182,7 @@ impl CheckWeight where /// Do the pre-dispatch checks. This can be applied to both signed and unsigned. /// /// It checks and notes the new weight and length. - fn do_pre_dispatch( + pub fn do_pre_dispatch( info: &DispatchInfoOf, len: usize, ) -> Result<(), TransactionValidityError> { @@ -198,7 +198,7 @@ impl CheckWeight where /// Do the validate checks. This can be applied to both signed and unsigned. /// /// It only checks that the block weight and length limit will not exceed. - fn do_validate( + pub fn do_validate( info: &DispatchInfoOf, len: usize, ) -> TransactionValidity { From 3ff8c0ac1c3ab6b85a7d7972746cfdac3bb29a48 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 2 Oct 2020 14:24:53 +0200 Subject: [PATCH 20/20] Final fix for the find_voter --- frame/staking/src/offchain_election.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index b896e7518740f..2f6e6384bf879 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -228,7 +228,7 @@ pub fn maximum_compact_len( // Time to finish. // We might have reduced less than expected due to rounding error. Increase one last time if we // have any room left, the reduce until we are sure we are below limit. - while weight_with(voters + 1) < max_weight { + while voters + 1 <= max_voters && weight_with(voters + 1) < max_weight { voters += 1; } while voters.checked_sub(1).is_some() && weight_with(voters) > max_weight {