Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix offchain election to respect the weight #7215

Merged
25 commits merged into from
Oct 2, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
051d71e
Mockup
kianenigma Sep 24, 2020
46686d6
Fix offchain election to respect the weight
kianenigma Sep 25, 2020
649a6d6
Fix builds a bit
kianenigma Sep 25, 2020
ad19e02
Update frame/staking/src/offchain_election.rs
kianenigma Sep 28, 2020
08aec98
Update frame/staking/src/offchain_election.rs
kianenigma Sep 28, 2020
a1feaf8
Make it build, binary search
kianenigma Sep 28, 2020
b7138f9
Merge branch 'kiz-staking-OCW-check-size' of github.com:paritytech/su…
kianenigma Sep 28, 2020
2274196
Fix a number of grumbles
kianenigma Sep 28, 2020
2572e4b
one more fix.
kianenigma Sep 28, 2020
3f53149
remove unwrap.
kianenigma Sep 28, 2020
cc89113
better alg.
kianenigma Sep 28, 2020
a72069f
Better alg again.
kianenigma Sep 29, 2020
8048e4f
Merge branch 'master' of github.com:paritytech/substrate into kiz-sta…
kianenigma Sep 29, 2020
0e3c91f
Final fixes
kianenigma Sep 29, 2020
47d69f7
Fix
kianenigma Sep 30, 2020
86d24a3
Rollback to normal
kianenigma Sep 30, 2020
0b618d4
Merge branch 'master' of github.com:paritytech/substrate into kiz-sta…
kianenigma Oct 2, 2020
6b6ef3e
Final touches.
kianenigma Oct 2, 2020
addd535
Better tests.
kianenigma Oct 2, 2020
e11ab9d
Update frame/staking/src/lib.rs
kianenigma Oct 2, 2020
f1d9f2b
Proper maxExtWeight
kianenigma Oct 2, 2020
4573d36
Merge branch 'kiz-staking-OCW-check-size' of github.com:paritytech/su…
kianenigma Oct 2, 2020
a1ef1b4
Merge branch 'master' of github.com:paritytech/substrate into kiz-sta…
kianenigma Oct 2, 2020
43b149c
Final fix
kianenigma Oct 2, 2020
3ff8c0a
Final fix for the find_voter
kianenigma Oct 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ rls*.log
**/hfuzz_target/
**/hfuzz_workspace/
.cargo/
.cargo-remote.toml
12 changes: 10 additions & 2 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -441,6 +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 mandatory, and this is the absolute maximum weight that we allow for
// it.
pub OffchainSolutionWeightLimit: Weight =
MaximumBlockWeight::get()
.saturating_sub(BlockExecutionWeight::get())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to be the "largest" extrinsic possible, but doesnt take into account weight introduced by on_initialize. Is that an issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you may be better off querying the current block weight as registered in System and using that to determine how much space you can fill?

Copy link
Contributor Author

@kianenigma kianenigma Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed the intention is to be the big boy here, generate the largest possible extrinsic. Because this is now operational, it is fine even if we have some on_initialize weight in the block, as long as average_on_initialize < operational_size. See operational_works_on_full_block in check_weight.rs

Yes, the on_initialize is an issue.

It seems you may be better off querying the current block weight as registered in System and using that to determine how much space you can fill?

maybe... But there are flaws to this as well:

  1. Configurability of the current system is better. A chain might want to allow simply 10% of block weight for npos solution, not always all.
  2. The transaction is almost likely produced with the data of block N and validated later for authoring against N + 1 or N + 2 or.. so this is a good hint, but not accurate.

.saturating_sub(ExtrinsicBaseWeight::get())
.saturating_sub(AVERAGE_ON_INITIALIZE_WEIGHT * MaximumBlockWeight::get());
}

impl pallet_staking::Trait for Runtime {
Expand Down Expand Up @@ -469,6 +476,7 @@ impl pallet_staking::Trait for Runtime {
type MinSolutionScoreBump = MinSolutionScoreBump;
type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
type UnsignedPriority = StakingUnsignedPriority;
type OffchainSolutionWeightLimit = OffchainSolutionWeightLimit;
type WeightInfo = weights::pallet_staking::WeightInfo<Runtime>;
}

Expand Down
1 change: 1 addition & 0 deletions frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ impl pallet_staking::Trait for Test {
type UnsignedPriority = StakingUnsignedPriority;
type MaxIterations = ();
type MinSolutionScoreBump = ();
type OffchainSolutionWeightLimit = ();
type WeightInfo = ();
}

Expand Down
1 change: 1 addition & 0 deletions frame/grandpa/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ impl pallet_staking::Trait for Test {
type UnsignedPriority = StakingUnsignedPriority;
type MaxIterations = ();
type MinSolutionScoreBump = ();
type OffchainSolutionWeightLimit = ();
type WeightInfo = ();
}

Expand Down
1 change: 1 addition & 0 deletions frame/offences/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ impl pallet_staking::Trait for Test {
type UnsignedPriority = ();
type MaxIterations = ();
type MinSolutionScoreBump = ();
type OffchainSolutionWeightLimit = ();
type WeightInfo = ();
}

Expand Down
1 change: 1 addition & 0 deletions frame/session/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ impl pallet_staking::Trait for Test {
type UnsignedPriority = UnsignedPriority;
type MaxIterations = ();
type MinSolutionScoreBump = ();
type OffchainSolutionWeightLimit = ();
type WeightInfo = ();
}

Expand Down
1 change: 1 addition & 0 deletions frame/staking/fuzzer/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,5 +195,6 @@ impl pallet_staking::Trait for Test {
type MinSolutionScoreBump = ();
type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
type UnsignedPriority = ();
type OffchainSolutionWeightLimit = ();
type WeightInfo = ();
}
4 changes: 2 additions & 2 deletions frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ benchmarks! {
compact,
score,
size
) = offchain_election::prepare_submission::<T>(assignments, winners, false).unwrap();
) = offchain_election::prepare_submission::<T>(assignments, winners, false, T::MaximumBlockWeight::get()).unwrap();
tomusdrw marked this conversation as resolved.
Show resolved Hide resolved

assert_eq!(
winners.len(), compact.unique_targets().len(),
Expand Down Expand Up @@ -562,7 +562,7 @@ benchmarks! {
compact,
score,
size
) = offchain_election::prepare_submission::<T>(assignments, winners, false).unwrap();
) = offchain_election::prepare_submission::<T>(assignments, winners, false, T::MaximumBlockWeight::get()).unwrap();

assert_eq!(
winners.len(), compact.unique_targets().len(),
Expand Down
63 changes: 50 additions & 13 deletions frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ pub enum ElectionStatus<BlockNumber> {
/// 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)]
Expand Down Expand Up @@ -883,6 +883,13 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes<Call<Self>> {
/// multiple pallets send unsigned transactions.
type UnsignedPriority: Get<TransactionPriority>;

/// 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>;

/// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;
}
Expand Down Expand Up @@ -1232,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.
Expand Down Expand Up @@ -1339,12 +1348,12 @@ decl_module! {
if Self::era_election_status().is_open_at(now) {
let offchain_status = set_check_offchain_execution_status::<T>(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::<T>() {
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.");
}
}
}
Expand Down Expand Up @@ -2141,14 +2150,15 @@ decl_module! {
/// transaction in the block.
///
/// # <weight>
/// See `crate::weight` module.
/// The weight of this call is not enforced by the system module and is enforced in this
/// module directly.
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
/// # </weight>
#[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(
tomusdrw marked this conversation as resolved.
Show resolved Hide resolved
origin,
winners: Vec<ValidatorIndex>,
Expand All @@ -2158,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::<T>::OffchainElectionBogusElectionWeight.with_weight(0),
);

let adjustments = Self::check_and_replace_solution(
winners,
compact,
Expand All @@ -2171,6 +2188,7 @@ decl_module! {
effectively depriving the validators from their authoring reward. Hence, this panic
is expected."
);

Ok(adjustments)
}
}
Expand All @@ -2190,6 +2208,21 @@ impl<T: Trait> Module<T> {
)
}

/// 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<ValidatorIndex>,
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()
}

kianenigma marked this conversation as resolved.
Show resolved Hide resolved
/// 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
Expand Down Expand Up @@ -3082,7 +3115,6 @@ impl<T: Trait> Module<T> {
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)`
Expand Down Expand Up @@ -3346,11 +3378,11 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
type Call = Call<T>;
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;

Expand All @@ -3363,17 +3395,22 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
}
}

// discard solution if it is too big.
if !Self::check_unsigned_solution_weight(winners, compact, size) {
return Err(InvalidTransaction::ExhaustsResources.into())
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
}

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.
Expand Down
38 changes: 23 additions & 15 deletions frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,29 @@

//! 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},
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;

Expand Down Expand Up @@ -194,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;
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
pub const MaximumBlockLength: u32 = 2 * 1024;
pub const AvailableBlockRatio: Perbill = Perbill::one();
pub const MaxLocks: u32 = 1024;
Expand Down Expand Up @@ -293,6 +298,7 @@ parameter_types! {
pub const MaxNominatorRewardedPerValidator: u32 = 64;
pub const UnsignedPriority: u64 = 1 << 20;
pub const MinSolutionScoreBump: Perbill = Perbill::zero();
pub const OffchainSolutionWeightLimit: Weight = MaximumBlockWeight::get();
}

thread_local! {
Expand Down Expand Up @@ -331,10 +337,12 @@ impl Trait for Test {
type MinSolutionScoreBump = MinSolutionScoreBump;
type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
type UnsignedPriority = UnsignedPriority;
type OffchainSolutionWeightLimit = OffchainSolutionWeightLimit;
type WeightInfo = ();
}

impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Test where
impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Test
where
Call: From<LocalCall>,
{
type OverarchingCall = Call;
Expand Down
Loading