-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[NPoS] Implements dynamic number of nominators #12970
Changes from 43 commits
8cb3404
c9b8a17
fcc7657
3ae3e29
98f6e82
5434b47
0a7b714
1c347f9
9bf66c3
cdaeb7c
4e38839
11bd646
3fe1076
a1f77ad
905b8e0
775fe25
eb2456b
b0b21a4
82ac2db
8218e17
af0c52b
5b5231a
8e7ecbb
bfbb91b
2c44857
1bf29f7
125ff54
7d9fdfc
8fddf41
805e3f7
3317bd7
1c20979
8ba9e1e
761d7ae
3136966
8d78bee
397ce89
1349e6a
92b1231
42eeb97
5a29c9c
f51083b
d067a1c
422393c
fb418a9
b786d33
af3b8ba
7d8ffda
1246c73
325c91f
1f8f502
67c4734
70d8579
a68a37d
849a2e0
306668e
7ec6305
e8576b7
e12f287
c7acde4
61a8ca5
5c8a034
187ce07
9023997
ee5c863
c3b4375
9f0319d
bece9d5
8bd5ce9
7970ade
208ee20
6c5b3b1
6619b5d
02d8e78
4805516
d18b175
31ed378
4769857
a5cd416
742f07d
fe61184
87945f6
75be862
b3aab85
bc0e8bd
aab74d6
fe2dfe3
491ddfc
8bfd731
747aa52
0bda7a4
157e3a7
3c3fc5a
86ab41c
e4f7f9f
07c4a7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,8 +231,9 @@ | |
|
|
||
| use codec::{Decode, Encode}; | ||
| use frame_election_provider_support::{ | ||
| BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ElectionProviderBase, | ||
| InstantElectionProvider, NposSolution, | ||
| BoundedSupportsOf, DataProviderBounds, ElectionBounds, ElectionBoundsBuilder, | ||
| ElectionDataProvider, ElectionProvider, ElectionProviderBase, InstantElectionProvider, | ||
| NposSolution, | ||
| }; | ||
| use frame_support::{ | ||
| dispatch::DispatchClass, | ||
|
|
@@ -654,23 +655,19 @@ pub mod pallet { | |
| #[pallet::constant] | ||
| type SignedDepositWeight: Get<BalanceOf<Self>>; | ||
|
|
||
| /// The maximum number of electing voters to put in the snapshot. At the moment, snapshots | ||
| /// are only over a single block, but once multi-block elections are introduced they will | ||
| /// take place over multiple blocks. | ||
| #[pallet::constant] | ||
| type MaxElectingVoters: Get<SolutionVoterIndexOf<Self::MinerConfig>>; | ||
|
|
||
| /// The maximum number of electable targets to put in the snapshot. | ||
| #[pallet::constant] | ||
| type MaxElectableTargets: Get<SolutionTargetIndexOf<Self::MinerConfig>>; | ||
|
|
||
| /// The maximum number of winners that can be elected by this `ElectionProvider` | ||
| /// implementation. | ||
| /// | ||
| /// Note: This must always be greater or equal to `T::DataProvider::desired_targets()`. | ||
| #[pallet::constant] | ||
| type MaxWinners: Get<u32>; | ||
|
|
||
| /// The maximum number of electing voters and electable targets to put in the snapshot. | ||
| /// At the moment, snapshots | ||
| /// are only over a single block, but once multi-block elections are introduced they will | ||
| /// take place over multiple blocks. | ||
| type ElectionBounds: Get<ElectionBounds>; | ||
|
|
||
| /// Handler for the slashed deposits. | ||
| type SlashHandler: OnUnbalanced<NegativeImbalanceOf<Self>>; | ||
|
|
||
|
|
@@ -1081,14 +1078,18 @@ pub mod pallet { | |
| ) -> DispatchResult { | ||
| T::ForceOrigin::ensure_origin(origin)?; | ||
| ensure!(Self::current_phase().is_emergency(), <Error<T>>::CallNotAllowed); | ||
|
|
||
| let supports = | ||
| T::GovernanceFallback::instant_elect(maybe_max_voters, maybe_max_targets).map_err( | ||
| |e| { | ||
| log!(error, "GovernanceFallback failed: {:?}", e); | ||
| Error::<T>::FallbackFailed | ||
| }, | ||
| )?; | ||
| let election_bounds = ElectionBoundsBuilder::new() | ||
| .voters_count(maybe_max_voters.unwrap_or(0)) | ||
| .targets_count(maybe_max_targets.unwrap_or(0)) | ||
| .build(); | ||
|
gpestana marked this conversation as resolved.
|
||
| let supports = T::GovernanceFallback::instant_elect( | ||
| election_bounds.voters, | ||
| election_bounds.targets, | ||
| ) | ||
| .map_err(|e| { | ||
| log!(error, "GovernanceFallback failed: {:?}", e); | ||
| Error::<T>::FallbackFailed | ||
| })?; | ||
|
|
||
| // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into | ||
| // `BoundedVec<_, T::MaxWinners>` | ||
|
|
@@ -1404,13 +1405,16 @@ impl<T: Config> Pallet<T> { | |
| /// Extracted for easier weight calculation. | ||
| fn create_snapshot_external( | ||
| ) -> Result<(Vec<T::AccountId>, Vec<VoterOf<T>>, u32), ElectionError<T>> { | ||
| let target_limit = T::MaxElectableTargets::get().saturated_into::<usize>(); | ||
| let voter_limit = T::MaxElectingVoters::get().saturated_into::<usize>(); | ||
| let election_bounds = T::ElectionBounds::get(); | ||
| let target_limit = | ||
| election_bounds.targets.count.unwrap_or(u32::MAX).saturated_into::<usize>(); | ||
|
rossbulat marked this conversation as resolved.
Outdated
|
||
| let voter_limit = | ||
| election_bounds.voters.count.unwrap_or(u32::MAX).saturated_into::<usize>(); | ||
|
|
||
| let targets = T::DataProvider::electable_targets(Some(target_limit)) | ||
| let targets = T::DataProvider::electable_targets(election_bounds.targets) | ||
| .map_err(ElectionError::DataProvider)?; | ||
|
|
||
| let voters = T::DataProvider::electing_voters(Some(voter_limit)) | ||
| let voters = T::DataProvider::electing_voters(election_bounds.voters) | ||
| .map_err(ElectionError::DataProvider)?; | ||
|
|
||
| if targets.len() > target_limit || voters.len() > voter_limit { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can just get rid of this. It is double checking some bounds that are already "guaranteed" to be met. If we are to check, we should also check the byte size (if possible to do cheaply).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The goal here was to double check that the data coming from the election provider meet the expected bounds, since we have no guarantee that the data provider is properly implemented. fwiw, there are a couple of old test checking for this. Regarding checking the size in bytes of the voter list, this pallet doesn't have the concept of size in bytes of a voter list, since that is a concept only on the data provider side (the size tracker and bounds are implemented on the data provider side). Given the point made above that EPM should not blindly trust the data provider, perhaps we could also add the size in bytes of the voter list checks in EPM (needs a new bound config for this pallet), or we could just assume the data provider always behaves as expected and remove the checks altogether. wdyt?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added voter size checks here too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your point about this pallet not dealing with byte size is correct, but in extension it would also not have to deal with In principle, this pallet only knows that there is an If you were to do this pedantically, you would make the fields of I would formulate this then as: and then inside
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great input. I have refactored this code and it ended up much simpler, thanks! |
||
|
|
@@ -1592,15 +1596,18 @@ impl<T: Config> Pallet<T> { | |
| <QueuedSolution<T>>::take() | ||
| .ok_or(ElectionError::<T>::NothingQueued) | ||
| .or_else(|_| { | ||
| T::Fallback::instant_elect(None, None) | ||
| .map_err(|fe| ElectionError::Fallback(fe)) | ||
| .and_then(|supports| { | ||
| Ok(ReadySolution { | ||
| supports, | ||
| score: Default::default(), | ||
| compute: ElectionCompute::Fallback, | ||
| }) | ||
| T::Fallback::instant_elect( | ||
| DataProviderBounds::new_unbounded(), | ||
|
gpestana marked this conversation as resolved.
Outdated
|
||
| DataProviderBounds::new_unbounded(), | ||
| ) | ||
| .map_err(|fe| ElectionError::Fallback(fe)) | ||
| .and_then(|supports| { | ||
| Ok(ReadySolution { | ||
| supports, | ||
| score: Default::default(), | ||
| compute: ElectionCompute::Fallback, | ||
| }) | ||
| }) | ||
| }) | ||
| .map(|ReadySolution { compute, score, supports }| { | ||
| Self::deposit_event(Event::ElectionFinalized { compute, score }); | ||
|
|
@@ -1876,7 +1883,7 @@ mod tests { | |
| mock::{ | ||
| multi_phase_events, raw_solution, roll_to, roll_to_signed, roll_to_unsigned, AccountId, | ||
| ExtBuilder, MockWeightInfo, MockedWeightInfo, MultiPhase, Runtime, RuntimeOrigin, | ||
| SignedMaxSubmissions, System, TargetIndex, Targets, | ||
| SignedMaxSubmissions, System, | ||
| }, | ||
| Phase, | ||
| }; | ||
|
|
@@ -2479,7 +2486,11 @@ mod tests { | |
| fn snapshot_too_big_failure_onchain_fallback() { | ||
| // the `MockStaking` is designed such that if it has too many targets, it simply fails. | ||
| ExtBuilder::default().build_and_execute(|| { | ||
| Targets::set((0..(TargetIndex::max_value() as AccountId) + 1).collect::<Vec<_>>()); | ||
| // sets bounds on number of targets. | ||
| let new_bounds = ElectionBoundsBuilder::new().targets_count(1_000).build(); | ||
| crate::mock::ElectionsBounds::set(new_bounds); | ||
|
|
||
| crate::mock::Targets::set((0..(1_000 as AccountId) + 1).collect::<Vec<_>>()); | ||
|
|
||
| // Signed phase failed to open. | ||
| roll_to(15); | ||
|
|
@@ -2514,9 +2525,11 @@ mod tests { | |
| fn snapshot_too_big_failure_no_fallback() { | ||
| // and if the backup mode is nothing, we go into the emergency mode.. | ||
| ExtBuilder::default().onchain_fallback(false).build_and_execute(|| { | ||
| crate::mock::Targets::set( | ||
| (0..(TargetIndex::max_value() as AccountId) + 1).collect::<Vec<_>>(), | ||
| ); | ||
| // sets bounds on number of targets. | ||
| let new_bounds = ElectionBoundsBuilder::new().targets_count(1_000).build(); | ||
| crate::mock::ElectionsBounds::set(new_bounds); | ||
|
|
||
| crate::mock::Targets::set((0..(1_000 as AccountId) + 1).collect::<Vec<_>>()); | ||
|
|
||
| // Signed phase failed to open. | ||
| roll_to(15); | ||
|
|
@@ -2548,7 +2561,8 @@ mod tests { | |
| // we have 8 voters in total. | ||
| assert_eq!(crate::mock::Voters::get().len(), 8); | ||
| // but we want to take 2. | ||
| crate::mock::MaxElectingVoters::set(2); | ||
| let new_bounds = ElectionBoundsBuilder::new().voters_count(2).build(); | ||
| crate::mock::ElectionsBounds::set(new_bounds); | ||
|
|
||
| // Signed phase opens just fine. | ||
| roll_to_signed(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.