Fix notify inactive collators failures at the end of a round #3128
Conversation
WASM runtime size check:Compared to target branchMoonbase runtime: 2280 KB (no changes) ✅ Moonbeam runtime: 2260 KB (no changes) ✅ Moonriver runtime: 2260 KB (no changes) ✅ Compared to latest release (runtime-3400)Moonbase runtime: 2280 KB (+252 KB compared to latest release) Moonbeam runtime: 2260 KB (+248 KB compared to latest release) Moonriver runtime: 2260 KB (+248 KB compared to latest release) |
RomarQ
added
the
B7-runtimenoteworthy
| if finalize_parachain_staking { | ||
| ParachainStaking::on_finalize(System::block_number()); | ||
| } |
There was a problem hiding this comment.
We should always call the on_finalize, otherwise we may get false positives/negatives in the tests.
There was a problem hiding this comment.
Addressed in 325137d
Please double check this changes in particular:
325137d#diff-a2fcd9e2ec1e588e9fcfed674d8d0058713d685aaba3ed374ba248d4c279e02fL6778-R6781
Note on the changes introduced in this commit: the function previously used to mimic the distribution of points and to obviate to the missing call to ParachainStaking::on_finalize directly modifies the storage in a similar way to what on_finalize does (although matching logic is not enforced).
// Same storage changes as ParachainStaking::on_finalize
pub(crate) fn set_author(round: BlockNumber, acc: u64, pts: u32) {
<Points<Test>>::mutate(round, |p| *p += pts);
<AwardedPts<Test>>::mutate(round, acc, |p| *p += pts);
}With the call to on_finalize within block rolling, and the concurrent use of this function, rounds may assign a variable amount of points making check on rewards amounts unreliable.
IMO a better approach would be to introduce utility functions as:
/// Sets the block author and advances n blocks.
pub(crate) fn author_blocks(n: BlockNumber, acc: u64) -> BlockNumber {
set_block_author(acc);
roll_blocks(n)
}
/// Sets the block author and advances blocks until the beginning of the specified round.
pub(crate) fn author_to_round_begin(round: BlockNumber, acc: u64) -> BlockNumber {
set_block_author(acc);
roll_to_round_begin(round)
}That would guarantee a higher fidelity of the mocking framework.
|
For optimization purposes (knowing that Storage/PoV is usually the limiting factor), shouldn't we store those who didn't have stake. Instead of |
pallets/parachain-staking/src/lib.rs
Outdated
| @@ -503,6 +503,7 @@ pub mod pallet { | |||
| } | |||
| fn on_finalize(_n: BlockNumberFor<T>) { | |||
| Self::award_points_to_block_author(); | |||
| Self::cleanup_stake_info(); | |||
There was a problem hiding this comment.
We need to account for this extra cost in the weight estimated inside on_initialize.
Coverage Report@@ Coverage Diff @@
## master manuel/notify-inactive-collators-works-only-at-the-beginning-of-a-round +/- ##
===========================================================================================================
+ Coverage 74.40% 74.42% +0.02%
Files 376 376
+ Lines 95432 95559 +127
===========================================================================================================
+ Hits 70998 71119 +121
+ Misses 24434 24440 +6
|
Yes, having a pub type WasInactive<T: Config> =
StorageDoubleMap<_, Twox64Concat, RoundIndex, Twox64Concat, T::AccountId, (), OptionQuery>;When starting a new round, we would check the inactivity like the example below, where moonbeam/pallets/parachain-staking/src/lib.rs Lines 1427 to 1430 in fef3821 We would still clear the storage after |
pallets/parachain-staking/src/lib.rs
Outdated
| let _ = <HadStake<T>>::iter_prefix(now - minimum_rounds_required) | ||
| .drain() | ||
| .next(); | ||
| } |
There was a problem hiding this comment.
If we properly estimate the weights in on_initialize, we could remove all entries from WasInactive for the round here.
…y-at-the-beginning-of-a-round
What does it do?
Addresses issue #3100 for which "notify_inactive_collator" only works at the beginning of a round.
What important points reviewers should know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?