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

Recursive ElectionProvider #8573

Closed
wants to merge 2 commits into from
Closed

Recursive ElectionProvider #8573

wants to merge 2 commits into from

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Apr 8, 2021

Related to https://github.com/paritytech/srlabs_findings/issues/79 and https://github.com/paritytech/srlabs_findings/issues/65.

The fundamental change here is that instead of assuming a hardcoded:

enum Fallback {
  Nothing,
  OnChain,
} 

We leave this open: The type Fallback is merely another ElectionProvider that might be anything.

Also, we ensure that a fallback method uses the snapshot that is created in the multi-phase pallet. In the past, the DataProvider of Fallback was Staking, meaning that we would need to re-query all validators and nominators from chain-state one by one to trigger the on-chain election. Now, we implement ElectionDataProvider for multi_phase::Pallet<T>, meaning that multi_phase can itself be the data provider of the fallback.

This is beneficial if: The snapshot is created in multi_phase, but election fails. In this case, we use the snapshot and this is much more optimized in terms of storage operations.
This is tricky if: Everything gets fails in multi_phase, and no snapshot exists there. An example of this is if the snapshot creation fails. In these cases, multi_phase has no snapshot to give to the fallback, and the fallback cannot do anything.

We could code impl DataProvider for multi_phase::Pallet in such a way that first tries to return the data from its own snapshot, and if failure, fallback to its own T::DataProvider (staking).

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders. labels Apr 8, 2021
@kianenigma kianenigma requested review from coriolinus and gui1117 April 8, 2021 14:19
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks good, pending CI.

We could code impl DataProvider for multi_phase::Pallet in such a way that first tries to return the data from its own snapshot, and if failure, fallback to its own T::DataProvider (staking).

Future PR?

Comment on lines +1080 to +1086
fn maybe_trim<T>(items: Vec<T>, maybe_size: Option<usize>) -> Vec<T> {
if let Some(size) = maybe_size {
items.into_iter().take(size).collect::<Vec<T>>()
} else {
items
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the size name a bit confusing. The implementation can also avoid allocating a new vector:

Suggested change
fn maybe_trim<T>(items: Vec<T>, maybe_size: Option<usize>) -> Vec<T> {
if let Some(size) = maybe_size {
items.into_iter().take(size).collect::<Vec<T>>()
} else {
items
}
}
fn maybe_trim<T>(mut items: Vec<T>, maybe_qty: Option<usize>) -> Vec<T> {
items.truncate(maybe_qty.unwrap_or_else(|| items.len()));
items
}

@kianenigma kianenigma added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 13, 2021
@kianenigma
Copy link
Contributor Author

I won't work on this PR immediately now, so putting it inprogress temporarily.

@gnunicorn gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label May 19, 2021
@stale stale bot closed this Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants