Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[pallet-staking] Additional check for virtual stakers #5985

Merged
merged 7 commits into from
Nov 5, 2024
Merged

[pallet-staking] Additional check for virtual stakers #5985

merged 7 commits into from
Nov 5, 2024

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Oct 8, 2024 •
edited
Loading

closes #5791.

This is not strictly necessary but serves as a defensive check.

The staking pallet exposes apis that other runtime pallets (pallet-delegated-staking) can use to create virtual stakers. However, there’s no way for pallet-staking to ensure that the staker is truly keyless. If the caller (this is a trusted caller so this would only happen due to a bug) registers an account with a private key as a virtual_staker, these accounts could later interact directly with pallet-staking dispatchables (such as bond_extra) and bypass any locking mechanism. The check above ensures this scenario can never occur by performing an integrity check.

@Ank4n Ank4n added R0-silent Changes should not be mentioned in any release notes T2-pallets This PR/Issue is related to a particular pallet. labels Oct 8, 2024
bkchr
bkchr approved these changes Oct 10, 2024
View reviewed changes
substrate/frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved
@Ank4n Ank4n requested a review from gpestana October 10, 2024 12:17
@Ank4n @bkchr
Update substrate/frame/staking/src/pallet/impls.rs
e7066c6 
Co-authored-by: Bastian Köcher <git@kchr.de>
@gpestana
Copy link
Contributor

gpestana commented Oct 10, 2024 •
edited
Loading

LGTM but I'd add a new test to make sure a fresh non-keyless account (registered as a virtual staker) cannot call e.g. bond-extra as its first call in the system. I'd expect the nonce to be updated before the check but just in case.

@Ank4n Ank4n enabled auto-merge November 5, 2024 00:35
@Ank4n Ank4n requested a review from koute as a code owner November 5, 2024 00:53
@Ank4n
Copy link
Contributor Author

Ank4n commented Nov 5, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Nov 5, 2024 •
edited
Loading

@Ank4n https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7693629 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 17-e3f24670-89b9-44e6-9f63-e1fe8ef3051a to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Nov 5, 2024

@Ank4n Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7693629 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7693629/artifacts/download.

@Ank4n Ank4n added this pull request to the merge queue Nov 5, 2024
Merged via the queue into master with commit be26d62 Nov 5, 2024
193 of 196 checks passed
@Ank4n Ank4n deleted the ankan/stakng/additional-check-for-virtual-stakers branch November 5, 2024 12:09
ordian added a commit that referenced this pull request Nov 5, 2024
@ordian
Merge branch 'master' into ao-fix-issue-64
9ea2099 
* master: (129 commits)
  pallet-revive: Use `RUSTUP_TOOLCHAIN` if set (#6365)
  [eth-rpc] proxy /health (#6360)
  [Release|CI/CD] adjust release pipelines (#6366)
  Bump the known_good_semver group across 1 directory with 3 updates (#6339)
  Run check semver in MQ (#6287)
  [Deprecation] deprecate treasury `spend_local` call and related items (#6169)
  refactor and harden check_core_index (#6217)
  litep2p: Update litep2p to v0.8.0 (#6353)
  [pallet-staking] Additional check for virtual stakers (#5985)
  migrate pallet-remarks to v2 bench syntax (#6291)
  Remove leftover references of Wococo (#6361)
  snowbridge: allow account conversion for Ethereum accounts (#6221)
  authority-discovery: Populate DHT records with public listen addresses (#6298)
  Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet (#5961)
  Silent annoying log (#6351)
  [pallet-revive] rework balance transfers (#6187)
  `statement-distribution`: RFC103 implementation (#5883)
  Disable flaky tests reported in #6343 / #6345 (#6346)
  migrate pallet-recovery to benchmark V2 syntax (#6299)
  inclusion emulator: correctly handle UMP signals (#6178)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gui1117 gui1117 gui1117 approved these changes

@bkchr bkchr bkchr approved these changes

@gpestana gpestana gpestana approved these changes

@koute koute Awaiting requested review from koute koute is a code owner

Assignees
No one assigned
Labels
R0-silent Changes should not be mentioned in any release notes T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Staking] Extra check for Virtual Stakers
4 participants
@Ank4n @gpestana @bkchr @gui1117