Patch staking client author prediction bug #828
Conversation
4meta5
added
A0-pleasereview
| candidates | ||
| let mut collators = candidates | ||
| .into_iter() | ||
| .rev() | ||
| .take(top_n) | ||
| .filter(|x| x.amount >= T::MinCollatorStk::get()) | ||
| .map(|x| x.owner) | ||
| .collect::<Vec<T::AccountId>>() | ||
| .collect::<Vec<T::AccountId>>(); | ||
| collators.sort(); | ||
| collators |
There was a problem hiding this comment.
So this is the core fix right? The sort we added sorts by collator id? That seems like a weird thing to do since they will all be in the set.
There was a problem hiding this comment.
Yes, the core change is to sort the AccountIds here before returning from compute_top_candidates instead of sorting later in select_top_candidates
That seems like a weird thing to do since they will all be in the set.
is_selected_candidate uses binary search so that's why they're stored sorted by AccountId https://github.com/PureStake/moonbeam/blob/master/pallets/parachain-staking/src/lib.rs#L1658-L1660
There was a problem hiding this comment.
We should add a comment about why sort() exists here
There was a problem hiding this comment.
Oh, I see (from your other comment) -- we sort because we use binary_search, not because this needs to match some other (possibly incorrect) behavior.
There was a problem hiding this comment.
match some other (possibly incorrect) behavior
That was my first thought. But I think the binary search point makes sense and addresses it.
| @@ -1937,7 +1938,6 @@ pub mod pallet { | |||
| <AtStake<T>>::insert(next, account, exposure); | |||
| Self::deposit_event(Event::CollatorChosen(next, account.clone(), amount)); | |||
| } | |||
| collators.sort(); | |||
There was a problem hiding this comment.
Or is this the core change? Is it enough to not sort by collator id here? I don't see any reason we need this list sorted by collator id.
There was a problem hiding this comment.
I don't see any reason we need this list sorted by collator id.
is_selected_candidate uses binary search so that's why they're stored sorted by AccountId https://github.com/PureStake/moonbeam/blob/master/pallets/parachain-staking/src/lib.rs#L1658-L1660
The core change is to sort the AccountIds before returning in compute_top_candidates instead of here.
4meta5
changed the title
|
Do we need to rebase off of the |
If we merge into master, the commit can be cherrypicked on top of runtime-700 and we can tag it 701 |
crystalin
added
A8-mergeoncegreen
Patches author prediction code that would incorrectly compute
SelectedCandidatesat the start of every round.Same change as #830 but to the
compute_top_candidatesfunction instead.The original bug was that the
Vec<AccountId>returned fromcompute_top_candidateswas used directly in author prediction because of changes made in #744 . Using it before sorting it resulted in a difference between the returnedVec<AccountId>fromcompute_top_candidatesand the actual computedSelectedCandidatesin storage. This disparity led to an incorrect eligibility calculation which stalled Stagenet.What else does it change?
The logic within parachain-staking stays the same. One sort is being done in a different place than before, but it is stored sorted in both cases.
The order of snapshotting the CandidateState changes but this does not effect much in practice because they are all snapshotted at once. The order of
CollatorChosenevent emission changed so unit tests had to be updated for that.