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.

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

Staking ledger bonding fixes #3639

Merged
merged 23 commits into from
Mar 14, 2024

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Mar 11, 2024

Currently, the staking logic does not prevent a controller from becoming a stash of another ledger (introduced by removing this check). 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).

  1. "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);
  • 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

@gpestana gpestana self-assigned this Mar 11, 2024
@gpestana gpestana marked this pull request as draft March 11, 2024 10:58
@gpestana gpestana added T8-polkadot This PR/Issue is related to/affects the Polkadot network. T10-tests This PR/Issue is related to tests. labels Mar 11, 2024
@gpestana gpestana requested review from rossbulat and Ank4n March 11, 2024 13:29
@gpestana gpestana requested a review from kianenigma March 11, 2024 15:28
Copy link
Contributor

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty important fix 🙏 .

The whole stash/controller logic is not so great. Intuitively I would have not expected Ledger to be keyed by controller which seems the core issue behind this bug. Another observation is, it is not very clear why some extrinsics in staking pallet can be called via stash and others via controller.

substrate/frame/staking/src/ledger.rs Outdated Show resolved Hide resolved
substrate/frame/staking/src/ledger.rs Outdated Show resolved Hide resolved
substrate/frame/staking/src/pallet/impls.rs Show resolved Hide resolved
@Ank4n Ank4n requested a review from a team March 11, 2024 15:37
substrate/frame/staking/src/ledger.rs Outdated Show resolved Hide resolved
gpestana and others added 2 commits March 13, 2024 12:15
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@gpestana gpestana marked this pull request as ready for review March 13, 2024 11:26
…es to leger are made through the staking ledger API
@gpestana gpestana changed the title Fixes in staking set_controller and more try-runtime checks Staking ledger bonding fixes Mar 13, 2024
@EgorPopelyaev
Copy link
Contributor

@gpestana just for info: prdoc file is missing here, which is needed to pass the CI check and also if you want this pr to be mentioned in the release notes

gpestana added a commit that referenced this pull request Mar 27, 2024
Backport for 1.7: #3639

Relevant Issues:
- #3245
gpestana added a commit that referenced this pull request Mar 27, 2024
Backport for 1.7:
- #3639
- #3706

Relevant Issues:
- #3245
bkchr pushed a commit that referenced this pull request Mar 28, 2024
Backports for 1.7: 
- #3639
- #3706

Relevant Issues:
- #3245
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/parity-tech-update-for-march/7226/1

gpestana added a commit that referenced this pull request Apr 11, 2024
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>
gpestana added a commit that referenced this pull request Apr 11, 2024
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>
gpestana added a commit that referenced this pull request Apr 11, 2024
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>
gpestana added a commit that referenced this pull request Apr 11, 2024
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>
gpestana added a commit that referenced this pull request Apr 18, 2024
This backport PR should bump the `pallet-staking` from 30.0.1 to 30.0.2.

Backports for 1.8: 
- #3639

Relevant Issues:
- #3245

---------

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Morganamilo pushed a commit that referenced this pull request Apr 22, 2024
This backport PR should bump the `pallet-staking` from 28.0.0 to 28.0.1.

Backports for 1.6: 
- #3639

Relevant Issues:
- #3245

---------

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Morganamilo pushed a commit that referenced this pull request Apr 22, 2024
This backport PR should bump the `pallet-staking` from 27.0.0 to 27.0.1

Backports for 1.5: 
- #3639

Relevant Issues:
- #3245

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Morganamilo pushed a commit that referenced this pull request Apr 22, 2024
This backport PR should bump the `pallet-staking` from 26.0.1 to 26.0.2.

Backports for 1.4: 
- #3639

Relevant Issues:
- #3245

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/recover-corrupted-staking-ledgers-in-polkadot-and-kusama/9796/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network. T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants