Multi-block election part 0: Paginated election data provider #11105
Conversation
Co-authored-by: Zeke Mostov <[email protected]>
…ate into kiz-validators-in-bags
kianenigma
added
A0-please_review
|
This PR is now ready for audit and final review. |
| @@ -318,8 +352,28 @@ pub trait ElectionDataProvider { | |||
| /// This is only useful for stateful election providers. | |||
| fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber; | |||
|
|
|||
| /// Shorthand for "most significant page". | |||
| /// | |||
| /// The least significant, aka "first" page that this election provider is expected to receive. | |||
There was a problem hiding this comment.
| /// The least significant, aka "first" page that this election provider is expected to receive. | |
| /// The most significant, aka "last" page that this election provider is expected to receive. |
| /// The least significant, aka "first" page that this election provider is expected to receive. | |
| /// The least significant, aka "first" page that this election provider is expected to receive. |
| @@ -318,8 +352,28 @@ pub trait ElectionDataProvider { | |||
| /// This is only useful for stateful election providers. | |||
| fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber; | |||
|
|
|||
| /// Shorthand for "most significant page". | |||
| /// | |||
| /// The least significant, aka "first" page that this election provider is expected to receive. | |||
There was a problem hiding this comment.
| /// The least significant, aka "first" page that this election provider is expected to receive. | |
| /// The most significant, aka "last" page that this election provider is expected to receive. |
| /// The least significant, aka "first" page that this election provider is expected to receive. | |
| /// The least significant, aka "first" page that this election provider is expected to receive. |
| } | ||
|
|
||
| /// The range of `take` pages, from most significant to least significant. | ||
| fn range(take: PageIndex) -> Box<dyn Iterator<Item = PageIndex>> { |
There was a problem hiding this comment.
Why use trait object? Can we just return the concrete type and save the v table look up?
| } | ||
|
|
||
| /// The range of `take` pages, from most significant to least significant. | ||
| fn range(take: PageIndex) -> Box<dyn Iterator<Item = PageIndex>> { |
There was a problem hiding this comment.
Why use trait object? Can we just return the concrete type and save the v table look up?
| let weight_of = Self::weight_of_fn(); | ||
| let slashing_spans = <SlashingSpans<T>>::iter().collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
We remove this from here but instead pass it in. This would make sense if the slashing spans where getting re-used in the scope where this function is called, however it looks like we don't re-use in that scope; so it seems like we make the function signature slightly more complicated for no benefit?
| let weight_of = Self::weight_of_fn(); | ||
| let slashing_spans = <SlashingSpans<T>>::iter().collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
We remove this from here but instead pass it in. This would make sense if the slashing spans where getting re-used in the scope where this function is called, however it looks like we don't re-use in that scope; so it seems like we make the function signature slightly more complicated for no benefit?
| let is_valid_state = if T::Pages::get() > 1 { | ||
| // in the first page, the `LastIteratedNominator` should not exist, in the rest of the | ||
| // pages it should. | ||
| (remaining == Self::msp()) ^ maybe_last.is_some() |
There was a problem hiding this comment.
Interesting, so this bitwise XOR basically is also a logical XOR? I assume its equivalent to (!A && B) || (A && !B)
There was a problem hiding this comment.
Also assuming there are tests that exercise all cases :)
| let is_valid_state = if T::Pages::get() > 1 { | ||
| // in the first page, the `LastIteratedNominator` should not exist, in the rest of the | ||
| // pages it should. | ||
| (remaining == Self::msp()) ^ maybe_last.is_some() |
There was a problem hiding this comment.
Interesting, so this bitwise XOR basically is also a logical XOR? I assume its equivalent to (!A && B) || (A && !B)
| maybe_last = None; | ||
| } | ||
|
|
||
| let slashing_spans = <SlashingSpans<T>>::iter().collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
| maybe_last = None; | ||
| } | ||
|
|
||
| let slashing_spans = <SlashingSpans<T>>::iter().collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
| Error::<Test>::TemporarilyNotAllowed | ||
| ); | ||
| // this needs to reverted, despite being transactional scope, because it is | ||
| // thread-local, not storage. |
There was a problem hiding this comment.
Interesting - I thought with the thread local stuff it should have no side effects in other tests
There was a problem hiding this comment.
Its also not in the transactional scope I think - just the outermost scope of the closure given to build_and_execute
| Error::<Test>::TemporarilyNotAllowed | ||
| ); | ||
| // this needs to reverted, despite being transactional scope, because it is | ||
| // thread-local, not storage. |
There was a problem hiding this comment.
Interesting - I thought with the thread local stuff it should have no side effects in other tests
|
Closing and putting into my archive group to revisit someday. |
louismerlin
removed
the
D9-needsaudit 👮
This is the first step of paritytech/polkadot-sdk#461 that is being backported to the master branch for review.
Small part of this is extracted as #11104.
polkadot companion: paritytech/polkadot#5348
Context
pallet-staking, as it stands now, relies onpallet-bags-listto implement itself asElectionDataProvider, and servicepallet-election-provider-multi-phasewith the election data, aka snapshot.This PR makes
ElectionDataProviderbecome paginated, and allows staking pallet to be able to return multiple pages of snapshot data, namely for voters.For the time being, we assume that target snapshots need not be paginated.
Impact on chains
All live chains that we know of use a single page snapshot. In other words,
type Pageswill be1. In that case, other than for unforeseen errors in my coding, this PR should be absolutely no-logical change.Impact on users
In case multi-page election snapshot is being created,
pallet-stakingtracks the last item that it fetched formT::VoterList, and continues iteration in the next call. Whoever this poor voter is, either a validator or a nominator, they cannot chill themselves in any possible way for a period of 1 block when they are the head of the snapshot. This is a very small annoyance for the end users.