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 https://github.com/paritytech/polkadot-sdk/issues/3245

---------

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
This commit is contained in:
Gonçalo Pestana
2024-03-14 10:49:46 +01:00
committed by GitHub
parent d55d4f64b5
commit 606664e1bd
6 changed files with 378 additions and 24 deletions
+15 -13
View File
@@ -791,6 +791,8 @@ pub mod pallet {
SnapshotTargetsSizeExceeded { size: u32 },
/// A new force era mode was set.
ForceEra { mode: Forcing },
/// Report of a controller batch deprecation.
ControllerBatchDeprecated { failures: u32 },
}
#[pallet::error]
@@ -935,6 +937,11 @@ pub mod pallet {
return Err(Error::<T>::AlreadyBonded.into())
}
// An existing controller cannot become a stash.
if StakingLedger::<T>::is_bonded(StakingAccount::Controller(stash.clone())) {
return Err(Error::<T>::AlreadyPaired.into())
}
// Reject a bond which is considered to be _dust_.
if value < T::Currency::minimum_balance() {
return Err(Error::<T>::InsufficientBond.into())
@@ -975,7 +982,6 @@ pub mod pallet {
#[pallet::compact] max_additional: BalanceOf<T>,
) -> DispatchResult {
let stash = ensure_signed(origin)?;
let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?;
let stash_balance = T::Currency::free_balance(&stash);
@@ -1332,8 +1338,6 @@ pub mod pallet {
pub fn set_controller(origin: OriginFor<T>) -> DispatchResult {
let stash = ensure_signed(origin)?;
// The bonded map and ledger are mutated directly as this extrinsic is related to a
// (temporary) passive migration.
Self::ledger(StakingAccount::Stash(stash.clone())).map(|ledger| {
let controller = ledger.controller()
.defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.")
@@ -1343,9 +1347,8 @@ pub mod pallet {
// Stash is already its own controller.
return Err(Error::<T>::AlreadyPaired.into())
}
<Ledger<T>>::remove(controller);
<Bonded<T>>::insert(&stash, &stash);
<Ledger<T>>::insert(&stash, ledger);
let _ = ledger.set_controller_to_stash()?;
Ok(())
})?
}
@@ -1960,7 +1963,7 @@ pub mod pallet {
};
if ledger.stash != *controller && !payee_deprecated {
Some((controller.clone(), ledger))
Some(ledger)
} else {
None
}
@@ -1969,13 +1972,12 @@ pub mod pallet {
.collect();
// Update unique pairs.
for (controller, ledger) in filtered_batch_with_ledger {
let stash = ledger.stash.clone();
<Bonded<T>>::insert(&stash, &stash);
<Ledger<T>>::remove(controller);
<Ledger<T>>::insert(stash, ledger);
let mut failures = 0;
for ledger in filtered_batch_with_ledger {
let _ = ledger.clone().set_controller_to_stash().map_err(|_| failures += 1);
}
Self::deposit_event(Event::<T>::ControllerBatchDeprecated { failures });
Ok(Some(T::WeightInfo::deprecate_controller_batch(controllers.len() as u32)).into())
}
}