Add benchmark for new_session hook
#1016
Conversation
There was a problem hiding this comment.
Requesting changes because we really need to think about the impact of this.
I haven't been able to find any examples of implementations of SessionManager which deal with any weights. I only found one for NoteHistoricalRoot which doesn't deal with weights.
Do you have any other counter examples here? Otherwise I would just leave this out tbh
pallets/staking/src/lib.rs
Outdated
| frame_system::Pallet::<T>::register_extra_weight_unchecked( | ||
| result_weight.expect("Error unwraping non error value"), | ||
| DispatchClass::Mandatory, | ||
| ); |
There was a problem hiding this comment.
This seems really risky to use. From the docs:
Even more dangerous is to note that this function does NOT take any action if the new sum of block weight is more than the block weight limit. This is what the unchecked.
This means we could stall the chain at a session boundary
ya, used it before in statemine, in the collator selection pallet, if we don't do weights here on session change we would have larger blocks we can add an if statment for the max weight of a block and be like hey don't add the weight if it is greater then a max block weight and have a larger block instead of a chain stall |
HCastano
changed the title
new_session hook
There was a problem hiding this comment.
Im leaving this as a comment so that @HCastano gets the say on this. I would need you to sit on a call and talk me through this to really get it.
But the fact that the same thing has been done in another pallet which has presumably been audited makes me think its probably fine.
* Format benchmarking code * Update `*_less_then_signers` bench to scale signers in storage Since we end up reading from this storage vec we want to make sure it is populated with up to `MAX_SIGNERS` for the purposes of benchmarking. * Change benchmarking for session handler to only use two benches We add an extra storage read each time, but we can simplify the benchmarking a bit * Always update session weight If the signer length was more than the total signers we wouldn't update the session weight, even though we would still do a few storage operations. * Remove `expect` by using `match` statement * Remove unused `new_session_not_adding_new_signer` bench * Update the name for one of the benches * Update benchmark results * Add missing parameter to base bench
Closes #1009