-
Notifications
You must be signed in to change notification settings - Fork 1k
Staking-Async + EPMB: Migrate operations to poll
#9925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
All GitHub workflows were cancelled due to failure one of the required jobs. |
/cmd bench --help |
Command help:
|
/cmd bench --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_verifier pallet_election_provider_multi_block_unsigned --runtime asset-hub-westend |
Command "bench --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_verifier pallet_election_provider_multi_block_unsigned --runtime asset-hub-westend" has started 🚀 See logs here |
Command "bench --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_verifier pallet_election_provider_multi_block_unsigned --runtime asset-hub-westend" has failed ❌! See logs here |
//! | ||
//! ### Phase Transition | ||
//! | ||
//! Within all 4 pallets only the parent pallet is allowed to move forward the phases. As of now, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/"move forward the phases"/"move the phases forward"/
_ => T::WeightInfo::on_initialize_nothing(), | ||
}; | ||
fn on_poll(_now: BlockNumberFor<T>, weight_meter: &mut WeightMeter) { | ||
// we need current phase to be read in any case -- we can live with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Comment not clear enough, maybe rephrase?
crate::log!(info, "TESTING: Starting election at block {}", _now); | ||
crate::mock::MultiBlock::start().unwrap(); | ||
} | ||
// NOTE: why in here? bc it is more accessible, for example `roll_to_with_ocw`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/bc/because/ (for extra clarity)
/// * Upon last page of `Phase::Signed`, instruct the `Verifier` to start, if any solution | ||
/// exists. | ||
/// | ||
/// What it does not: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "what it does not do"
Phase::Off => Err(()), | ||
|
||
// we're doing sth but not read. | ||
// we're doing sth but not ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/sth/something/
); | ||
|
||
// we have 1 block left in signed verification, but we cannot do anything here. | ||
// we have 2 block left in signed verification, but we cannot do anything here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/block/blocks/
|
||
// we have 1 block left in signed verification, but we cannot do anything here. | ||
// we have 2 block left in signed verification, but we cannot do anything here. | ||
// status is not set, as not enough time to do anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: (...) as there's not enough time (...)
Self::Export(0) => Self::Off, | ||
Self::Export(non_zero_left) => | ||
Self::Export(non_zero_left.defensive_saturating_sub(One::one())), | ||
// Export never moves forward via this function, and is always manually set in `elect` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: (...) set in the elect
code path.
/// * Optionally, it can return an updated weight that is more "accurate", based on the | ||
/// execution. | ||
fn per_block_exec(current_phase: Phase<T>) -> (Weight, Box<dyn Fn() -> Option<Weight>>) { | ||
type ExecuteFn = Box<dyn Fn() -> Option<Weight>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the syntax Box<dyn Fn() -> Option<Weight>>
is reused several times in this PR, wouldn't it make sense (for extra clarity) to define it as the type ExecuteFn
at the start of the file and then reuse that type alias instead?
/// called. `Ok(false)` means we are doing something, but work is still ongoing. `elect` should | ||
/// not be called. `Ok(true)` means we are done and ready for a call to `elect`. | ||
fn status() -> Result<bool, ()>; | ||
/// * `Err(())` should signal that we are not doing anything, and `elect` should def. not be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: given the emphasis on it here, I suggest expanding "def." to the full word "definitely".
This PR moves all operations related to staking elections from a mandatory
on_initialize
with no consideration to weight, to an optionalon_poll
with accurate, pre-execution weight checking.Why
on_initialize
is a mandatory hook. If a single parachain block happens to contain too many of them, this block can never be authored and imported. In solo/relay chains, this is more forgiving, as you would have one slow block, instead of an indefinite stall.on_initialize
in AH (unlikely, but totally possible), and put the chain at risk.poll
hooks:frame-executive
(e.g. during MBMs)WeigthMeter
, allowing the subject to make a decision about whether to proceed or not.Functional Changes
As seen by the minimal diff in existing tests, this change, in the absence of weight scarcity, is almost a noop. The only difference is that the start signal from the signed pallet to the verifier pallet is now sent at the end of the signed phase, not the beginning the signed validation.
Non-Functional Changes
on_poll
aremulti_block
(only the parent, not verifier and signed), andstaking_async
. This makes the code easier to audit.on_initialize
terminology from weight functionson_poll
.Implementation/Review Notes
Overall Design
The overall idea is to move all operations to a model similar to dispatchables, where before executing
f(input) -> Result
, we have access to aw(input) -> Weight
that gives us the pre-execution weight. If the pre-execution weight is good, we proceed with executing. The execution may override the pre-execution weight to a smaller value if it wishes so.Export Weight
Through this PR, I realized that we previously were never registering the weight of the export process. This is because the export is managed by staking pallet, and previously it had no way to know how much the weight of each export step is.
Now, we alter the
ElectionProvider::status
interface such that not only we signal if we are ready or not, but also we signal we are ready, and this is the weight of the nextelect
.Integration
The only breaking change of this PR is:
TODO