Adds multi-block election types and refactors current pallets to support new interfaces and types #6034
Conversation
…into gpestana/epm-mb
| } | ||
|
|
||
| // election failed, clear election prep metadata. | ||
| Self::clear_election_metadata(); |
There was a problem hiding this comment.
clear_election_metadata is called once here and once in trigger_new_era.
There was a problem hiding this comment.
But these are disjointed paths, since we return at the end of this if block and do not call trygger_new_era. I will try to simplify the code.
|
Once this is merged (quite ready, I hope next week I will approve), I would suggest having a set of e2e tests, with the real polkadot data that suggests at least the following code paths:
As in, we can build a new polkadot runtime with this new code, and ensure that the outcome is sensible. @rockbmb is your test setup fit to write something like this? I hope it can done swiftly, and modified as we add more staking changes. |
|
@kianenigma sadly, I'm not sufficiently informed atm to give a definitive answer here - I will be chatting with @gpestana offline about this. |
Ensures max backers per winner bounds are met in Staking miner. This PR replaces #6482 (already reverted in the base branch) and moves the trimming logic when max backer per winner exceed configured bounds to the miner.
| /// Deduplicates stashes in place and returns an error if the bounds are exceeded. In case of | ||
| /// error, it returns the stashes that were not added to the storage. | ||
| pub(crate) fn add_electables( | ||
| mut stashes_iter: impl Iterator<Item = T::AccountId>, |
There was a problem hiding this comment.
There is a lot of cloning around this code. Would be a good exercise to try and minimize them.
| while let Some(stash) = stashes_iter.next() { | ||
| if let Err(_) = (*electable).try_insert(stash.clone()) { | ||
| let mut not_included = stashes_iter.collect::<Vec<_>>(); | ||
| not_included.push(stash); | ||
|
|
||
| return Err(not_included); | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
| while let Some(stash) = stashes_iter.next() { | |
| if let Err(_) = (*electable).try_insert(stash.clone()) { | |
| let mut not_included = stashes_iter.collect::<Vec<_>>(); | |
| not_included.push(stash); | |
| return Err(not_included); | |
| } | |
| } | |
| Ok(()) | |
| for stash in stashes_iter { | |
| if let Err(_) = (*electable).try_insert(stash.clone()) { | |
| let mut not_included = stashes_iter.collect::<Vec<_>>(); | |
| not_included.push(stash); | |
| return Err(not_included); | |
| } | |
| } | |
| Ok(()) |
There was a problem hiding this comment.
The error return type seems incorrect and not tested -- the input is not being .drain()ed as you iterate over it.
Question: is the Err(_) inner value really needed? will this error ever happen? maybe it is best to fail
Suggestion: mutate the input as you iterate for stash in stashes_iter.iter().drain(), and then the input after execution will be the leftovers. Plus, less cloning.
There was a problem hiding this comment.
a bit improved now but I think it can be further optimised. wip;
There was a problem hiding this comment.
OK I think the current code is better now. My hunch is that the current stash.clone() clones the accountid when the try_insert fails (confirming this now).
The drawback of the current code is that in case of failure, we iterate over all the stashes that are returned in the election and keep trying to insert in the btree. But that's about the same cost as the happy path so I'm happy with it. wdyt?
There was a problem hiding this comment.
Question: is the Err(_) inner value really needed? will this error ever happen? maybe it is best to fail
My approach here is to make this potential case -- which happens only in case of misconfig between staking and EPM -- to be handled gracefully. In case of error, we raise a defensive and keep proceeding with the election with as many electable stashes returned from the EPM as the staking config can accomodate.
Returning the error with the vec of non-included accounts is useful for the error message and the tests. Perhaps we could drop the error with the vec.
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
1 similar comment
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
This PR refactors the types and structs required to run a mulit-block election and updates the EPM, staking-pallet and all dependent pallets to use the multi-block types. The Westend runtime is configured to run a 1 paged election, which is a noop refactor compared to the current single-block election.
Notable changes since last reviews:
Tasks based on feedback that can be closed after merging this PR: Umbrella ticket for multi-block election tasks to improve after PR#6034.
The multi-block election provider pallet is wip and it's added in a separate PR (#6213).
To-do before merging:
on_intializein pallet staking