Staking-Async: Chill stakers should not have a score #9926
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kianenigma
added
the
A4-backport-unstable2507
sigurpol
approved these changes
Oct 8, 2025
substrate/frame/staking-async/src/tests/election_data_provider.rs
Outdated
Show resolved
Hide resolved
substrate/frame/staking-async/src/tests/election_data_provider.rs
Outdated
Show resolved
Hide resolved
substrate/frame/staking-async/src/tests/election_data_provider.rs
Outdated
Show resolved
Hide resolved
substrate/frame/staking-async/src/tests/election_data_provider.rs
Outdated
Show resolved
Hide resolved
substrate/frame/staking-async/src/tests/election_data_provider.rs
Outdated
Show resolved
Hide resolved
Contributor
|
@kianenigma changes look good. Assuming we want this fix to be part of KAH RU 1.9.2, which branch should we target? Same for next WAH RU... we used to backport to 2509, what about now that stable2509 is ready to go? |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
kianenigma
changed the title
kianenigma
added
the
A4-backport-stable2509
Co-authored-by: Paolo La Camera <paolo@parity.io>
Co-authored-by: Paolo La Camera <paolo@parity.io>
Co-authored-by: Paolo La Camera <paolo@parity.io>
Co-authored-by: Paolo La Camera <paolo@parity.io>
Co-authored-by: Paolo La Camera <paolo@parity.io>
sigurpol
approved these changes
Oct 8, 2025
Contributor
Author
|
/cmd prdoc --bump patch |
andreitrand
reviewed
Oct 8, 2025
| fn score(who: &T::AccountId) -> Option<Self::Score> { | ||
| Self::ledger(Stash(who.clone())) | ||
| .map(|l| l.active) | ||
| .ok() |
Contributor
There was a problem hiding this comment.
Just one question here: before this change, the second map (i.e. .map(|a| ...) would receive a struct with the type of l.active whereas after this change it will receive an option struct with the internal type of l.active. Why does this not affect the T::CurrencyToVote::to_vote(...) call?
Contributor
Author
There was a problem hiding this comment.
This because we want to use and_then here, which works better if you already pass it an Option, and it mutates it either into Some(_) or None, contrary to map which can only mutate the Option into a different Some(_) value.
This is a text-book example of when you use and_then vs. map.
Why does this not affect the T::CurrencyToVote::to_vote(...) call?
The final map, both in the previous code and now, receives a l.active, not an Option<_>.
Compare the type of F in
https://doc.rust-lang.org/std/option/enum.Option.html#method.and_then
and
https://doc.rust-lang.org/std/option/enum.Option.html#method.map
Final note: this type of error that you are trying to prevent me from is impossible in Rust, the compiler would never allow me to pass Option<X> to a function instead of X :)
andreitrand
approved these changes
Oct 8, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
paritytech-release-backport-bot bot
pushed a commit
that referenced
this pull request
Oct 8, 2025
That can re-instate them in the bags-list pallet Identified by https://github.com/paritytech-secops/srlabs_findings/issues/559 While no severe consequence, this bug could cause non-validator and non-nominator stakers to retain a spot in the bags-list pallet, preventing other legit nominators/validators from taking their place. Note that previously, this was not a possibility, because `staking` would always issue a `T::VoterList::on_remove` when someone `chill`s, ensuring they are removed from the list. Moreover, an older version of `pallet_bags_list::Pallet::rebag` didn't allow new nodes to be added, only the score of existing nodes to be adjusted. But, in recent versions of `bags-list`, we added a `Lock` ability that would block any changes to the bags list (during the election snapshot phase). This also had us update the `rebag` transaction to add or remove nodes from the list, which opened the door to this issue. --------- Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 92118ec)
|
Successfully created backport PR for |
paritytech-release-backport-bot bot
pushed a commit
that referenced
this pull request
Oct 8, 2025
That can re-instate them in the bags-list pallet Identified by https://github.com/paritytech-secops/srlabs_findings/issues/559 While no severe consequence, this bug could cause non-validator and non-nominator stakers to retain a spot in the bags-list pallet, preventing other legit nominators/validators from taking their place. Note that previously, this was not a possibility, because `staking` would always issue a `T::VoterList::on_remove` when someone `chill`s, ensuring they are removed from the list. Moreover, an older version of `pallet_bags_list::Pallet::rebag` didn't allow new nodes to be added, only the score of existing nodes to be adjusted. But, in recent versions of `bags-list`, we added a `Lock` ability that would block any changes to the bags list (during the election snapshot phase). This also had us update the `rebag` transaction to add or remove nodes from the list, which opened the door to this issue. --------- Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 92118ec)
|
Successfully created backport PR for |
kianenigma
added a commit
that referenced
this pull request
Oct 9, 2025
Backport #9926 into `stable2509` from kianenigma. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
kianenigma
added a commit
that referenced
this pull request
Oct 10, 2025
Backport #9926 into `unstable2507` from kianenigma. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Oct 10, 2025
fellowship-merge-bot bot
pushed a commit
to polkadot-fellows/runtimes
that referenced
this pull request
Oct 14, 2025
Forwarding paritytech/polkadot-sdk#9926 to here.
alvicsam
pushed a commit
that referenced
this pull request
Oct 17, 2025
That can re-instate them in the bags-list pallet Identified by https://github.com/paritytech-secops/srlabs_findings/issues/559 While no severe consequence, this bug could cause non-validator and non-nominator stakers to retain a spot in the bags-list pallet, preventing other legit nominators/validators from taking their place. Note that previously, this was not a possibility, because `staking` would always issue a `T::VoterList::on_remove` when someone `chill`s, ensuring they are removed from the list. Moreover, an older version of `pallet_bags_list::Pallet::rebag` didn't allow new nodes to be added, only the score of existing nodes to be adjusted. But, in recent versions of `bags-list`, we added a `Lock` ability that would block any changes to the bags list (during the election snapshot phase). This also had us update the `rebag` transaction to add or remove nodes from the list, which opened the door to this issue. --------- Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
That can re-instate them in the bags-list pallet
Identified by https://github.com/paritytech-secops/srlabs_findings/issues/559
While no severe consequence, this bug could cause non-validator and non-nominator stakers to retain a spot in the bags-list pallet, preventing other legit nominators/validators from taking their place.
Note that previously, this was not a possibility, because
stakingwould always issue aT::VoterList::on_removewhen someonechills, ensuring they are removed from the list. Moreover, an older version ofpallet_bags_list::Pallet::rebagdidn't allow new nodes to be added, only the score of existing nodes to be adjusted.But, in recent versions of
bags-list, we added aLockability that would block any changes to the bags list (during the election snapshot phase). This also had us update therebagtransaction to add or remove nodes from the list, which opened the door to this issue.