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

Decouple Staking and Election - Part 3: Signed Phase #7910

Merged
103 commits merged into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
103 commits
Select commit Hold shift + click to select a range
d998b12
Base features and traits.
kianenigma Jan 15, 2021
5aea9cc
pallet and unsigned phase
kianenigma Jan 15, 2021
8607d3d
add signed phase.
kianenigma Jan 15, 2021
6523f5b
remove comments
kianenigma Jan 15, 2021
632e107
Undo bad formattings.
kianenigma Jan 15, 2021
c52e65d
Master.into()
kianenigma Jan 18, 2021
b4fc5e1
some formatting cleanup.
kianenigma Jan 18, 2021
cc26881
Small self-cleanup.
kianenigma Jan 18, 2021
15cdb28
Add todo
kianenigma Jan 19, 2021
67a9fae
Master.into()
kianenigma Jan 27, 2021
75eca3f
Make it all build
kianenigma Jan 27, 2021
8daec3a
self-review
kianenigma Jan 27, 2021
49613ed
Some doc tests.
kianenigma Jan 27, 2021
eae69b0
Upstream.into()
kianenigma Jan 27, 2021
35f1faf
Some changes from other PR
kianenigma Jan 27, 2021
6c82d9f
Upstream.into()
kianenigma Jan 27, 2021
e6ffc4d
Fix session test
kianenigma Jan 27, 2021
483018c
Master.into()
kianenigma Mar 3, 2021
54460eb
Update bin/node/runtime/src/lib.rs
kianenigma Mar 3, 2021
33b8e69
Fix name.
kianenigma Mar 3, 2021
be89dc5
Merge branch 'kiz-election-provider-3-signed-phase' of github.com:par…
kianenigma Mar 3, 2021
62f8ce4
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Mar 3, 2021
dff422a
typos and verbiage
coriolinus Mar 30, 2021
5f1637e
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
coriolinus Mar 30, 2021
c58b2f9
no glob imports in signed.rs
coriolinus Mar 30, 2021
76c3387
meaningful generic type parameters for SignedSubmission
coriolinus Mar 30, 2021
872a3c8
dedup feasibility check weight calculation
coriolinus Mar 30, 2021
f5f4605
simplify/optimize fn insert_submission
coriolinus Mar 30, 2021
859f594
tests: remove glob, cause to build without error
coriolinus Mar 30, 2021
0fc7a58
use sp_std::vec::Vec
coriolinus Mar 31, 2021
1137698
maintain invariant within fn insert_submission
coriolinus Mar 31, 2021
8c26900
fix accidentally ordering the list backward
coriolinus Mar 31, 2021
a882ff5
intentionally order the list in reverse
coriolinus Mar 31, 2021
d5fe671
get rid of unused import
coriolinus Mar 31, 2021
75ddba9
ensure signed submissions are cleared in early elect
coriolinus Mar 31, 2021
abae602
finalize the signed phase when appropriate
coriolinus Mar 31, 2021
f4aea1d
resolve dispatch error todo
coriolinus Mar 31, 2021
ebface4
update assumptions in submit benchmark
coriolinus Mar 31, 2021
151e880
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
Mar 31, 2021
189a870
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
Apr 12, 2021
8d95722
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Apr 12, 2021
edfb376
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
coriolinus May 25, 2021
a3574d5
line length
coriolinus May 25, 2021
5ecd6b5
Master.into()
kianenigma May 26, 2021
ee66c84
make a few more things pub
kianenigma May 26, 2021
17faa86
restore missing import
coriolinus May 26, 2021
94314bf
update ui test output
coriolinus May 26, 2021
9e6b559
update tests from master branch
coriolinus May 26, 2021
10cfdee
Merge branch 'master' of https://github.com/paritytech/substrate into…
May 27, 2021
6f9cf10
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
May 27, 2021
134ab4f
remove duplicate definitions
coriolinus May 27, 2021
61b7680
remove signed reward factor due to its attack potential
coriolinus May 28, 2021
b06f3a8
Update frame/election-provider-multi-phase/src/signed.rs
coriolinus May 28, 2021
ca97603
remove SignedRewardMax; no longer necessary
coriolinus May 28, 2021
7ce5afb
compute the encoded size without actually encoding
coriolinus May 28, 2021
bdf45ba
remove unused PostInfo
coriolinus May 28, 2021
87b7ff1
pub use some stuff
coriolinus May 28, 2021
308b522
ensure `pub use` things are in fact `pub`
coriolinus May 28, 2021
0d892f4
add event information: was another solution ejected to make room
coriolinus May 28, 2021
ea2b1ab
unconditionally run the unsigned phase even if signed was successful
coriolinus May 28, 2021
22248e1
remove dead test code
coriolinus May 28, 2021
c651962
meaningful witness data name
coriolinus May 31, 2021
29d9242
use errors instead of defensive `unwrap_or_default`
coriolinus May 31, 2021
fd38356
get rid of a log message redundant with an event
coriolinus May 31, 2021
c503fa7
saturating math
coriolinus May 31, 2021
252f310
import Saturating
coriolinus May 31, 2021
725cadf
mv `fn submit` to end of call
coriolinus May 31, 2021
88f856a
add log line
kianenigma Jun 1, 2021
ce0974f
Use a better data structure for SignedSubmissions instead of Vec (#8933)
coriolinus Jun 18, 2021
c1a4f1e
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
coriolinus Jun 18, 2021
75e6637
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jun 20, 2021
2f26f16
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jun 20, 2021
eb07d0b
remove duplicate weight definitions injected by benchmark bot
coriolinus Jun 21, 2021
18f4aaa
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
coriolinus Jun 21, 2021
a6b3770
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
coriolinus Jun 21, 2021
fb187fc
check deletion overlay before getting
coriolinus Jun 21, 2021
2ed81bc
clarify non-conflict between delete, insert overlays
coriolinus Jun 21, 2021
8f0c16e
drain can be used wrong so is private
coriolinus Jun 21, 2021
0b0abb8
update take_submission docs
coriolinus Jun 21, 2021
73071aa
more drain improvements
coriolinus Jun 22, 2021
955f5bc
more take_submission docs
coriolinus Jun 22, 2021
65b6926
debug assertion helps prove expectation is valid
coriolinus Jun 22, 2021
7b673d1
doc on changing SignedMaxSubmissions
coriolinus Jun 22, 2021
300c5f6
take_submission inner doc on system properties
coriolinus Jun 22, 2021
f84093b
Apply suggestions from code review
coriolinus Jun 22, 2021
2355a78
get SolutionOrSnapshotSize out of the loop
coriolinus Jun 22, 2021
e4e9eda
doc which items comprise `SignedSubmissions`
coriolinus Jun 22, 2021
65d7af6
add doc about index as unique identifier
coriolinus Jun 22, 2021
2c61152
Add debug assertions to prove drain worked properly
coriolinus Jun 22, 2021
368e97b
replace take_submission with swap_out_submission
coriolinus Jun 23, 2021
2491916
use a match to demonstrate all cases from signed_submissions.insert
coriolinus Jun 23, 2021
e12e542
refactor signed_submissions.insert return type
coriolinus Jun 23, 2021
a195ed8
prettify test assertion
coriolinus Jun 23, 2021
b0a57da
improve docs
coriolinus Jun 23, 2021
3325691
add tests that finalize_signed_phase is idempotent
coriolinus Jun 23, 2021
e0b3a52
add some debug assertions to guard against misuse of storage
coriolinus Jun 23, 2021
ad9d095
log internal logic errors instead of panicing
coriolinus Jun 23, 2021
3a46890
Merge remote-tracking branch 'origin/master' into kiz-election-provid…
coriolinus Jun 23, 2021
0ddfeda
don't store the reward with each signed submission
coriolinus Jun 28, 2021
bf4572c
emit Rewarded, Slashed events as appropriate
coriolinus Jun 28, 2021
dfd0bf1
update docs
coriolinus Jun 28, 2021
786898d
use a custom enum to be explicit about the outcome of insertion
coriolinus Jun 28, 2021
9e6e75f
remove outdated docs
coriolinus Jun 28, 2021
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
21 changes: 18 additions & 3 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ use pallet_session::{historical as pallet_session_historical};
use sp_inherents::{InherentData, CheckInherentsResult};
use static_assertions::const_assert;
use pallet_contracts::weights::WeightInfo;
use pallet_election_provider_multi_phase::FallbackStrategy;

#[cfg(any(feature = "std", test))]
pub use sp_runtime::BuildStorage;
Expand Down Expand Up @@ -516,9 +517,14 @@ parameter_types! {
pub const SignedPhase: u32 = EPOCH_DURATION_IN_BLOCKS / 4;
pub const UnsignedPhase: u32 = EPOCH_DURATION_IN_BLOCKS / 4;

// fallback: no need to do on-chain phragmen initially.
pub const Fallback: pallet_election_provider_multi_phase::FallbackStrategy =
pallet_election_provider_multi_phase::FallbackStrategy::Nothing;
// signed config
pub const SignedMaxSubmissions: u32 = 10;
pub const SignedRewardBase: Balance = 1 * DOLLARS;
pub const SignedDepositBase: Balance = 1 * DOLLARS;
pub const SignedDepositByte: Balance = 1 * CENTS;

// fallback: no on-chain fallback.
pub const Fallback: FallbackStrategy = FallbackStrategy::Nothing;
kianenigma marked this conversation as resolved.
Show resolved Hide resolved

pub SolutionImprovementThreshold: Perbill = Perbill::from_rational(1u32, 10_000);

Expand Down Expand Up @@ -559,6 +565,14 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
type MinerMaxWeight = MinerMaxWeight;
type MinerMaxLength = MinerMaxLength;
type MinerTxPriority = MultiPhaseUnsignedPriority;
type SignedMaxSubmissions = SignedMaxSubmissions;
type SignedRewardBase = SignedRewardBase;
type SignedDepositBase = SignedDepositBase;
type SignedDepositByte = SignedDepositByte;
type SignedDepositWeight = ();
Copy link
Member

Choose a reason for hiding this comment

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

note

type SignedMaxWeight = MinerMaxWeight;
type SlashHandler = (); // burn slashes
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
type RewardHandler = (); // nothing to do upon rewards
coriolinus marked this conversation as resolved.
Show resolved Hide resolved
emostov marked this conversation as resolved.
Show resolved Hide resolved
type DataProvider = Staking;
type OnChainAccuracy = Perbill;
type CompactSolution = NposCompactSolution16;
Expand Down Expand Up @@ -1556,6 +1570,7 @@ impl_runtime_apis! {
add_benchmark!(params, batches, pallet_uniques, Uniques);
add_benchmark!(params, batches, pallet_utility, Utility);
add_benchmark!(params, batches, pallet_vesting, Vesting);
add_benchmark!(params, batches, pallet_election_provider_multi_phase, ElectionProviderMultiPhase);

if batches.is_empty() { return Err("Benchmark not found for this pallet.".into()) }
Ok(batches)
Expand Down
105 changes: 93 additions & 12 deletions frame/election-provider-multi-phase/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

use super::*;
use crate::{Pallet as MultiPhase, unsigned::IndexAssignmentOf};
use frame_benchmarking::impl_benchmark_test_suite;
use frame_benchmarking::{account, impl_benchmark_test_suite};
use frame_support::{assert_ok, traits::OnInitialize};
use frame_system::RawOrigin;
use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng};
Expand Down Expand Up @@ -57,7 +57,7 @@ fn solution_with_size<T: Config>(
let targets: Vec<T::AccountId> =
(0..size.targets).map(|i| frame_benchmarking::account("Targets", i, SEED)).collect();

let mut rng = SmallRng::seed_from_u64(SEED as u64);
let mut rng = SmallRng::seed_from_u64(SEED.into());

// decide who are the winners.
let winners = targets
Expand Down Expand Up @@ -176,6 +176,39 @@ frame_benchmarking::benchmarks! {
assert!(<MultiPhase<T>>::current_phase().is_unsigned());
}

finalize_signed_phase_accept_solution {
let receiver = account("receiver", 0, SEED);
let initial_balance = T::Currency::minimum_balance() * 10u32.into();
T::Currency::make_free_balance_be(&receiver, initial_balance);
let ready: ReadySolution<T::AccountId> = Default::default();
let deposit: BalanceOf<T> = 10u32.into();
let reward: BalanceOf<T> = 20u32.into();

assert_ok!(T::Currency::reserve(&receiver, deposit));
assert_eq!(T::Currency::free_balance(&receiver), initial_balance - 10u32.into());
}: {
<MultiPhase<T>>::finalize_signed_phase_accept_solution(ready, &receiver, deposit, reward)
} verify {
assert_eq!(T::Currency::free_balance(&receiver), initial_balance + 20u32.into());
assert_eq!(T::Currency::reserved_balance(&receiver), 0u32.into());
}

finalize_signed_phase_reject_solution {
let receiver = account("receiver", 0, SEED);
let initial_balance = T::Currency::minimum_balance().max(One::one()) * 10u32.into();
let deposit: BalanceOf<T> = 10u32.into();
T::Currency::make_free_balance_be(&receiver, initial_balance);
assert_ok!(T::Currency::reserve(&receiver, deposit));

assert_eq!(T::Currency::free_balance(&receiver), initial_balance - 10u32.into());
assert_eq!(T::Currency::reserved_balance(&receiver), 10u32.into());
}: {
<MultiPhase<T>>::finalize_signed_phase_reject_solution(&receiver, deposit)
} verify {
assert_eq!(T::Currency::free_balance(&receiver), initial_balance - 10u32.into());
assert_eq!(T::Currency::reserved_balance(&receiver), 0u32.into());
}

on_initialize_open_unsigned_without_snapshot {
// need to assume signed phase was open before
<MultiPhase<T>>::on_initialize_open_signed().unwrap();
Expand Down Expand Up @@ -227,16 +260,51 @@ frame_benchmarking::benchmarks! {
assert!(<MultiPhase<T>>::snapshot().is_some());
}

submit {
let c in 1 .. (T::SignedMaxSubmissions::get() - 1);

// the solution will be worse than all of them meaning the score need to be checked against
// ~ log2(c)
let solution = RawSolution {
score: [(10_000_000u128 - 1).into(), 0, 0],
..Default::default()
};

MultiPhase::<T>::on_initialize_open_signed().expect("should be ok to start signed phase");
<Round<T>>::put(1);

let mut signed_submissions = SignedSubmissions::<T>::get();
for i in 0..c {
let solution = RawSolution {
score: [(10_000_000 + i).into(), 0, 0],
..Default::default()
};
let signed_submission = SignedSubmission { solution, ..Default::default() };
signed_submissions.insert(signed_submission);
}
signed_submissions.put();

let caller = frame_benchmarking::whitelisted_caller();
T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance() * 10u32.into());

}: _(RawOrigin::Signed(caller), solution, c)
verify {
assert!(<MultiPhase<T>>::signed_submissions().len() as u32 == c + 1);
}

submit_unsigned {
// number of votes in snapshot.
let v in (T::BenchmarkingConfig::VOTERS[0]) .. T::BenchmarkingConfig::VOTERS[1];
// number of targets in snapshot.
let t in (T::BenchmarkingConfig::TARGETS[0]) .. T::BenchmarkingConfig::TARGETS[1];
// number of assignments, i.e. compact.len(). This means the active nominators, thus must be
// a subset of `v` component.
let a in (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
let a in
(T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1];
let d in
(T::BenchmarkingConfig::DESIRED_TARGETS[0]) ..
T::BenchmarkingConfig::DESIRED_TARGETS[1];

let witness = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(witness, a, d);
Expand All @@ -249,7 +317,8 @@ frame_benchmarking::benchmarks! {
let encoded_call = <Call<T>>::submit_unsigned(raw_solution.clone(), witness).encode();
}: {
assert_ok!(<MultiPhase<T>>::submit_unsigned(RawOrigin::None.into(), raw_solution, witness));
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot).unwrap();
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot)
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of decoding here?

Copy link
Member

Choose a reason for hiding this comment

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

I believe they are trying to take into account decode time to the benchmarking. Especially if it is a very large and potentially expensive storage item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I think I see: is it that feasibility_check, which is in submit_unsigned's code path, does not return a weight and the most weight intensive part of it is decoding the snapshot?

If this is the case though, shouldn't decoding be taken into account because we have a scenario that will read in the snapshot?

let _decoded_call = <Call<T> as Decode>::decode(&mut &*encoded_call).unwrap();
} verify {
assert!(<MultiPhase<T>>::queued_solution().is_some());
Expand All @@ -263,13 +332,17 @@ frame_benchmarking::benchmarks! {
let t in (T::BenchmarkingConfig::TARGETS[0]) .. T::BenchmarkingConfig::TARGETS[1];
// number of assignments, i.e. compact.len(). This means the active nominators, thus must be
// a subset of `v` component.
let a in (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
let a in
(T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1];
let d in
(T::BenchmarkingConfig::DESIRED_TARGETS[0]) ..
T::BenchmarkingConfig::DESIRED_TARGETS[1];
// Subtract this percentage from the actual encoded size
let f in 0 .. 95;

// Compute a random solution, then work backwards to get the lists of voters, targets, and assignments
// Compute a random solution, then work backwards to get the lists of voters, targets, and
// assignments
let witness = SolutionOrSnapshotSize { voters: v, targets: t };
let RawSolution { compact, .. } = solution_with_size::<T>(witness, a, d);
let RoundSnapshot { voters, targets } = MultiPhase::<T>::snapshot().unwrap();
Expand Down Expand Up @@ -313,7 +386,11 @@ frame_benchmarking::benchmarks! {
} verify {
let compact = CompactOf::<T>::try_from(index_assignments.as_slice()).unwrap();
let encoding = compact.encode();
log!(trace, "encoded size prediction = {}", encoded_size_of(index_assignments.as_slice()).unwrap());
log!(
trace,
"encoded size prediction = {}",
encoded_size_of(index_assignments.as_slice()).unwrap(),
);
log!(trace, "actual encoded size = {}", encoding.len());
assert!(encoding.len() <= desired_size);
}
Expand All @@ -326,9 +403,12 @@ frame_benchmarking::benchmarks! {
let t in (T::BenchmarkingConfig::TARGETS[0]) .. T::BenchmarkingConfig::TARGETS[1];
// number of assignments, i.e. compact.len(). This means the active nominators, thus must be
// a subset of `v` component.
let a in (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
let a in
(T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1];
let d in
(T::BenchmarkingConfig::DESIRED_TARGETS[0]) ..
T::BenchmarkingConfig::DESIRED_TARGETS[1];

let size = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(size, a, d);
Expand All @@ -340,7 +420,8 @@ frame_benchmarking::benchmarks! {
let encoded_snapshot = <MultiPhase<T>>::snapshot().unwrap().encode();
}: {
assert_ok!(<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Unsigned));
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot).unwrap();
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot)
.unwrap();
}
}

Expand Down
8 changes: 4 additions & 4 deletions frame/election-provider-multi-phase/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ pub fn generate_voter_cache<T: Config>(
cache
}

/// Create a function the returns the index a voter in the snapshot.
/// Create a function that returns the index of a voter in the snapshot.
///
/// The returning index type is the same as the one defined in `T::CompactSolution::Voter`.
///
/// ## Warning
///
/// The snapshot must be the same is the one used to create `cache`.
/// Note that this will represent the snapshot data from which the `cache` is generated.
pub fn voter_index_fn<T: Config>(
cache: &BTreeMap<T::AccountId, usize>,
) -> impl Fn(&T::AccountId) -> Option<CompactVoterIndexOf<T>> + '_ {
Expand All @@ -78,7 +78,7 @@ pub fn voter_index_fn_owned<T: Config>(
///
/// ## Warning
///
/// The snapshot must be the same is the one used to create `cache`.
/// Note that this will represent the snapshot data from which the `cache` is generated.
pub fn voter_index_fn_usize<T: Config>(
cache: &BTreeMap<T::AccountId, usize>,
) -> impl Fn(&T::AccountId) -> Option<usize> + '_ {
Expand All @@ -103,7 +103,7 @@ pub fn voter_index_fn_linear<T: Config>(
}
}

/// Create a function the returns the index to a target in the snapshot.
/// Create a function that returns the index of a target in the snapshot.
///
/// The returned index type is the same as the one defined in `T::CompactSolution::Target`.
///
Expand Down
Loading