-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove failing assertion related to VoterList count mismatch #10880
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
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
prdoc/pr_10880.prdoc
Outdated
| doc: | ||
| - audience: Runtime Dev | ||
| description: |- | ||
| Refers to: #10687 |
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.
I know the bot has generated this prdoc starting from the description you put in the PR but usually we prefer not to link explicitly to other GH issues / PRs. Just a very syntethic description of the problem you are trying to fix would be better
| "not all genesis stakers were inserted into sorted list provider, something is wrong." | ||
| ); | ||
| // Due to `PendingRebag`, the bags-list may temporarily lag behind the true | ||
| // voter set in `pallet-staking`, making such an assertion invalid. |
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.
Rather than commenting out, I would evaluate if we might see value in changing the assertion into just logging for debugging purposes (leaving the comment about why this is not a valid invariant).
Pedantic: technically this PR could be backported to a runtime that doesn't have PendingRebag implementation (#9725, currently on 2509 / 2512 but not on 2507). Probably we won't make a new 2507 release backporting this PR and not #9725 but maybe we can rephrase better.
Also it's not because PendingRebag that we lag behind but it's because PendingRebag that eventually we converge and get rid of misalignment
|
/cmd prdoc --audience runtime_dev --bump patch |
|
Command "prdoc --audience runtime_dev --bump patch" has failed ❌! See logs here |
|
/cmd prdoc --audience runtime_dev --bump patch --force true |
…time_dev --bump patch --force true'
|
Good job! I would suggest to add an integration test under ahm-tests that simulates the following scenario reported by @Ank4n in #10687 . The test would need to verify that, in such a case, the PendingRebag mechanism ensures that once the voterlist is unlocked, validators/nominators are successfully inserted into the bags lists. We have unit tests in bags-list pallet such as I believe we might not have a more comprehensive integration test (to be checked!!!) so that would be a good excuse for you to look at integration tests in staking as well 😉 |
| title: Remove failing assertion related to VoterList count mismatch | ||
| doc: | ||
| - audience: Runtime Dev | ||
| description: PR resolves the error that occurs when VoterList count does not align |
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.
small tip or at least my preference: Once you generate the prdoc the first time with /cmd prdoc --audience runtime_dev --bump patch , I usually prefer not to re-enter /cmd prdoc --audience runtime_dev --bump patch --force true if I need to modify prdoc further, I'll just edit manually in my editor and push together with my other changes (pay attention to indentation, there are some guides in the how-to-contribute guide or similar, and some plugin for some popular Microsfot editor I refuse to use ;-) ). This allows you to easily decouple what you have in the PR message that can be more verbose and what you have in the prdoc that must be kept to the minimum. In the PR message for example it would be valuable to mention Close #10687 so that it automatically closes that issue once we merge. And you can provide more details or explanation if you feel they are needed
prdoc/pr_10880.prdoc
Outdated
| doc: | ||
| - audience: Runtime Dev | ||
| description: PR resolves the error that occurs when VoterList count does not align | ||
| with sum of Nominations & Validators in bags-list. This misalignment happens while |
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] this might be improved probably since VoterList is associated to bags-list, not Nominators and Validators. Also Nominations -> Nominators.
Something like this maybe?
The invariant in try-state related to VoterList in bags-list matching the sum of Nominations and Validators no longer holds. The misalignment arises when the bags-list is locked (e.g. during election Snapshot phase) and nominators / validators try to nominate / validate. This discrepancy is eventually reconciled via the PendingRebag mechanism (automatically via on_idle or explicitly via rebag() extrinsic)
| Nominators::<T>::count() + Validators::<T>::count(), | ||
| "not all genesis stakers were inserted into sorted list provider, something is wrong." | ||
| Nominators::<T>::count(), | ||
| Validators::<T>::count() |
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.
The actual failing assertion was in fn check_count() though, right?
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.
yes
[2026-01-21T23:35:44Z ERROR runtime::frame-support] ❌ "Staking" try_state checks failed: Other("wrong external count")
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.
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.
so the current code here should stay as it was, and this needs to be modified
ensure!(
<T as Config>::VoterList::count() ==
Nominators::<T>::count() + Validators::<T>::count(),
"wrong external count"
);updates: - revert back to first assertion in `mod.rs` - log instead of assert in `check_count()` function - update prdoc
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
PR resolves the error that occurs when
VoterListcount does not align with sum ofNominators&Validatorsin bags-list. This misalignment happens while bags-list is locked & nominations/validations are received. Eventually this misalignment evens out duePendingRebagimplementation