-
Notifications
You must be signed in to change notification settings - Fork 742
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Staking ledger bonding fixes (#3639)
Currently, the staking logic does not prevent a controller from becoming a stash of *another* ledger (introduced by [removing this check](https://github.com/paritytech/polkadot-sdk/pull/1484/files#diff-3aa6ceab5aa4e0ab2ed73a7245e0f5b42e0832d8ca5b1ed85d7b2a52fb196524L850)). Given that the remaining of the code expects that never happens, bonding a ledger with a stash that is a controller of another ledger may lead to data inconsistencies and data losses in bonded ledgers. For more detailed explanation of this issue: https://hackmd.io/@gpestana/HJoBm2tqo/%2FTPdi28H7Qc2mNUqLSMn15w In a nutshell, when fetching a ledger with a given controller, we may be end up getting the wrong ledger which can lead to unexpected ledger states. This PR also ensures that `set_controller` does not lead to data inconsistencies in the staking ledger and bonded storage in the case when a controller of a stash is a stash of *another* ledger. and improves the staking `try-runtime` checks to catch potential issues with the storage preemptively. In summary, there are two important cases here: 1. **"Sane" double bonded ledger** When a controller of a ledger is a stash of *another* ledger. In this case, we have: ``` > Bonded(stash, controller) (A, B) // stash A with controller B (B, C) // B is also a stash of another ledger (C, D) > Ledger(controller) Ledger(B) = L_a (stash = A) Ledger(C) = L_b (stash = B) Ledger(D) = L_c (stash = C) ``` In this case, the ledgers can be mutated and all operations are OK. However, we should not allow `set_controller` to be called if it means it results in a "corrupt" double bonded ledger (see below). 3. **"Corrupt" double bonded ledger** ``` > Bonded(stash, controller) (A, B) // stash A with controller B (B, B) (C, D) ``` In this case, B is a stash and controller AND is corrupted, since B is responsible for 2 ledgers which is not correct and will lead to inconsistent states. Thus, in this case, in this PR we are preventing these ledgers from mutating (i.e. operations like bonding extra etc) until the ledger is brought back to a consistent state. --- **Changes**: - Checks if stash is already a controller when calling `Call::bond` (fixes the regression introduced by [removing this check](https://github.com/paritytech/polkadot-sdk/pull/1484/files#diff-3aa6ceab5aa4e0ab2ed73a7245e0f5b42e0832d8ca5b1ed85d7b2a52fb196524L850)); - Ensures that all fetching ledgers from storage are done through the `StakingLedger` API; - Ensures that -- when fetching a ledger from storage using the `StakingLedger` API --, a `Error::BadState` is returned if the ledger bonding is in a bad state. This prevents bad ledgers from mutating (e.g. `bond_extra`, `set_controller`, etc) its state and avoid further data inconsistencies. - Prevents stashes which are controllers or another ledger from calling `set_controller`, since that may lead to a bad state. - Adds further try-state runtime checks that check if there are ledgers in a bad state based on their bonded metadata. Related to #3245 --------- Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: kianenigma <kian@parity.io>
- Loading branch information
1 parent
a493179
commit 6524144
Showing
6 changed files
with
407 additions
and
27 deletions.
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
title: Prevents staking controllers from becoming stashes of different ledgers; Ensures that no ledger in bad state is mutated. | ||
|
||
doc: | ||
- audience: Runtime User | ||
description: | | ||
This PR introduces a fix to the staking logic which prevents an existing controller from bonding as a stash of another ledger, which | ||
lead to staking ledger inconsistencies down the line. In addition, it adds a few (temporary) gates to prevent ledgers that are already | ||
in a bad state from mutating its state. | ||
|
||
In summary: | ||
* Checks if stash is already a controller when calling `Call::bond` and fails if that's the case; | ||
* Ensures that all fetching ledgers from storage are done through the `StakingLedger` API; | ||
* Ensures that a `Error::BadState` is returned if the ledger bonding is in a bad state. This prevents bad ledgers from mutating (e.g. | ||
`bond_extra`, `set_controller`, etc) its state and avoid further data inconsistencies. | ||
* Prevents stashes which are controllers or another ledger from calling `set_controller`, since that may lead to a bad state. | ||
* Adds further try-state runtime checks that check if there are ledgers in a bad state based on their bonded metadata. | ||
|
||
crates: | ||
- name: pallet-staking |
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.