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
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
30b00e3
Add more try runtime checks to staking
gpestana Feb 28, 2024
8b49b43
more changes
gpestana Mar 1, 2024
20a87e2
clean up
gpestana Mar 6, 2024
bb837d2
Merge branch 'master' into gpestana/3245ledger_consistency_runtimecheck
gpestana Mar 6, 2024
31cbd74
fixes set controller and batch deprecation when controller is double …
gpestana Mar 11, 2024
6bc85bc
removes unecessary warns
gpestana Mar 11, 2024
16a692e
prevents controller to become a stash
gpestana Mar 13, 2024
47f9e25
warn only try-runtime checks
gpestana Mar 13, 2024
8ed57de
prevents calling bond_extra in inconsistent ledgers
gpestana Mar 13, 2024
410d784
Update substrate/frame/staking/src/mock.rs
gpestana Mar 13, 2024
86b0ad7
adds bad state checks at the staking ledger level; ensures all access…
gpestana Mar 13, 2024
6e16fa4
nit
gpestana Mar 13, 2024
5986a4e
comments nits
gpestana Mar 13, 2024
578a407
add tests for staking ledger fetch in bad state
gpestana Mar 13, 2024
8dd4818
adds prdoc
gpestana Mar 13, 2024
52c648e
fixes prdoc
gpestana Mar 13, 2024
e3e9672
Update substrate/frame/staking/src/ledger.rs
gpestana Mar 13, 2024
541037f
Update substrate/frame/staking/src/ledger.rs
gpestana Mar 13, 2024
0c1bff6
replaces btreemap with btreeset in try runtime checks
gpestana Mar 13, 2024
371b0cf
simplifies getter check
gpestana Mar 13, 2024
4670659
check if bonded controller is the expected
gpestana Mar 13, 2024
649bf59
Merge branch 'master' into gpestana/3245ledger_consistency_runtimecheck
gpestana Mar 14, 2024
0dde68a
Empty-Commit
kianenigma Mar 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions substrate/frame/staking/src/ledger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
//! state consistency.

use frame_support::{
defensive,
traits::{LockableCurrency, WithdrawReasons},
defensive, ensure,
traits::{Defensive, LockableCurrency, WithdrawReasons},
};
use sp_staking::StakingAccount;
use sp_std::prelude::*;
Expand Down Expand Up @@ -201,6 +201,32 @@ impl<T: Config> StakingLedger<T> {
}
}

/// Sets the ledger controller to its stash.
pub(crate) fn set_controller_to_stash(self) -> Result<(), Error<T>> {
let controller = self.controller.as_ref()
.defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.")
.ok_or(Error::<T>::NotController)?;

ensure!(self.stash != *controller, Error::<T>::AlreadyPaired);

match Ledger::<T>::get(&self.stash) {
gpestana marked this conversation as resolved.
Show resolved Hide resolved
Some(other_ledger) => {
if other_ledger.stash != self.stash {
// stash is a controller of another ledger, fail with `Error::<T>::DoubleBonded`
// to avoid data inconsistencies.
return Err(Error::<T>::DoubleBonded);
gpestana marked this conversation as resolved.
Show resolved Hide resolved
}
gpestana marked this conversation as resolved.
Show resolved Hide resolved
},
None => (), // proceed.
}

<Ledger<T>>::remove(&controller);
<Ledger<T>>::insert(&self.stash, &self);
<Bonded<T>>::insert(&self.stash, &self.stash);

Ok(())
}

/// Clears all data related to a staking ledger and its bond in both [`Ledger`] and [`Bonded`]
/// storage items and updates the stash staking lock.
pub(crate) fn kill(stash: &T::AccountId) -> Result<(), Error<T>> {
Expand Down
52 changes: 52 additions & 0 deletions substrate/frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,58 @@ pub(crate) fn bond_controller_stash(controller: AccountId, stash: AccountId) ->
Ok(())
}

pub(crate) fn setup_double_bonded_ledgers() {
assert_ok!(Staking::bond(RuntimeOrigin::signed(1), 10, RewardDestination::Staked));
assert_ok!(Staking::bond(RuntimeOrigin::signed(2), 20, RewardDestination::Staked));
assert_ok!(Staking::bond(RuntimeOrigin::signed(3), 20, RewardDestination::Staked));
// not relevant to the test case, but ensures try-runtime checks pass.
let _ = [1, 2, 3]
.iter()
.map(|s| Payee::<Test>::insert(s, RewardDestination::Staked))
.collect::<Vec<_>>();
gpestana marked this conversation as resolved.
Show resolved Hide resolved

// we want to test the case where a controller can also be a stash of another ledger.
// for that, we change the controller/stash bonding so that:
// * 2 becomes controller of 1.
// * 3 becomes controller of 2.
// * 4 becomes controller of 3.
let ledger_1 = Ledger::<Test>::get(1).unwrap();
let ledger_2 = Ledger::<Test>::get(2).unwrap();
let ledger_3 = Ledger::<Test>::get(3).unwrap();

// 4 becomes controller of 3.
Bonded::<Test>::mutate(3, |controller| *controller = Some(4));
Ledger::<Test>::insert(4, ledger_3);

// 3 becomes controller of 2.
Bonded::<Test>::mutate(2, |controller| *controller = Some(3));
Ledger::<Test>::insert(3, ledger_2);

// 2 becomes controller of 1
Bonded::<Test>::mutate(1, |controller| *controller = Some(2));
Ledger::<Test>::insert(2, ledger_1);
// 1 is not controller anymore.
Ledger::<Test>::remove(1);

// checks. now we have:
// * 3 ledgers
assert_eq!(Ledger::<Test>::iter().count(), 3);
// * stash 1 has controller 2.
assert_eq!(Bonded::<Test>::get(1), Some(2));
assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(1)), Some(2));
assert_eq!(Ledger::<Test>::get(2).unwrap().stash, 1);

// * stash 2 has controller 3.
assert_eq!(Bonded::<Test>::get(2), Some(3));
assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(2)), Some(3));
assert_eq!(Ledger::<Test>::get(3).unwrap().stash, 2);

// * stash 3 has controller 4.
assert_eq!(Bonded::<Test>::get(3), Some(4));
assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(3)), Some(4));
assert_eq!(Ledger::<Test>::get(4).unwrap().stash, 3);
}

#[macro_export]
macro_rules! assert_session_era {
($session:expr, $era:expr) => {
Expand Down
70 changes: 69 additions & 1 deletion substrate/frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1836,6 +1836,7 @@ impl<T: Config> Pallet<T> {
"VoterList contains non-staker"
);

Self::check_bonded_consistency()?;
Self::check_payees()?;
Self::check_nominators()?;
Self::check_exposures()?;
Expand All @@ -1844,9 +1845,58 @@ impl<T: Config> Pallet<T> {
Self::check_count()
}

/// Invariants:
/// * A bonded (stash, controller) pair should have only one associated ledger. I.e. if the
gpestana marked this conversation as resolved.
Show resolved Hide resolved
/// ledger is bonded by stash, the controller account must not bond a different ledger. (note:
/// only warn, do not fail this check).
/// * A bonded (stash, controller) pair must have an associated ledger.
fn check_bonded_consistency() -> Result<(), TryRuntimeError> {
use sp_std::collections::btree_map::BTreeMap;

let mut count_double = 0;
let mut count_none = 0;
// sanity check to ensure that each controller in Bonded storage is associated with only one
// ledger.
let mut controllers: BTreeMap<T::AccountId, ()> = BTreeMap::new();
gpestana marked this conversation as resolved.
Show resolved Hide resolved

for (stash, controller) in <Bonded<T>>::iter() {
ensure!(
!controllers.contains_key(&controller),
"controller associated with more than 1 ledger"
);
controllers.insert(controller.clone(), ());

match (<Ledger<T>>::get(&stash), <Ledger<T>>::get(&controller)) {
(Some(_), Some(_)) =>
// if stash == controller, it means that the ledger has migrated to
// post-controller. If no migration happened, we expect that the (stash,
// controller) pair has only one associated ledger.
if stash != controller {
count_double += 1;
},
(None, None) => {
count_none += 1;
},
_ => {},
};
}

if count_double == 0 {
log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger");
};

ensure!(
count_none == 0,
"inconsistent bonded state: (stash, controller) pair missing associated ledger",
);

Ok(())
}

/// Invariants:
/// * A bonded ledger should always have an assigned `Payee`.
/// * The number of entries in `Payee` and of bonded staking ledgers *must* match.
/// * The stash account in the ledger must match that of the bonded acount.
fn check_payees() -> Result<(), TryRuntimeError> {
for (stash, _) in Bonded::<T>::iter() {
ensure!(Payee::<T>::get(&stash).is_some(), "bonded ledger does not have payee set");
Expand All @@ -1861,6 +1911,11 @@ impl<T: Config> Pallet<T> {
Ok(())
}

/// Invariants:
/// * Number of voters in `VoterList` match that of the number of Nominators and Validators in
/// the system (validator is both voter and target).
/// * Number of targets in `TargetList` matches the number of validators in the system.
/// * Current validator count is bounded by the election provider's max winners.
fn check_count() -> Result<(), TryRuntimeError> {
ensure!(
<T as Config>::VoterList::count() ==
Expand All @@ -1879,6 +1934,11 @@ impl<T: Config> Pallet<T> {
Ok(())
}

/// Invariants:
/// * `ledger.controller` is not stored in the storage (but populated at retrieval).
/// * Stake consistency: ledger.total == ledger.active + sum(ledger.unlocking).
/// * The controller keyeing the ledger and the ledger stash matches the state of the `Bonded`
/// storage.
fn check_ledgers() -> Result<(), TryRuntimeError> {
Bonded::<T>::iter()
.map(|(stash, ctrl)| {
Expand All @@ -1896,8 +1956,10 @@ impl<T: Config> Pallet<T> {
Ok(())
}

/// Invariants:
/// * For each era exposed validator, check if the exposure total is sane (exposure.total =
/// exposure.own + exposure.own).
fn check_exposures() -> Result<(), TryRuntimeError> {
// a check per validator to ensure the exposure struct is always sane.
let era = Self::active_era().unwrap().index;
ErasStakers::<T>::iter_prefix_values(era)
.map(|expo| {
Expand All @@ -1915,6 +1977,10 @@ impl<T: Config> Pallet<T> {
.collect::<Result<(), TryRuntimeError>>()
}

/// Invariants:
/// * For each paged era exposed validator, check if the exposure total is sane (exposure.total
/// = exposure.own + exposure.own).
/// * Paged exposures metadata (`ErasStakersOverview`) matches the paged exposures state.
fn check_paged_exposures() -> Result<(), TryRuntimeError> {
use sp_staking::PagedExposureMetadata;
use sp_std::collections::btree_map::BTreeMap;
Expand Down Expand Up @@ -1979,6 +2045,8 @@ impl<T: Config> Pallet<T> {
.collect::<Result<(), TryRuntimeError>>()
}

/// Invariants:
/// * Checks that each nominator has its entire stake correctly distributed.
fn check_nominators() -> Result<(), TryRuntimeError> {
// a check per nominator to ensure their entire stake is correctly distributed. Will only
// kick-in if the nomination was submitted before the current era.
Expand Down
25 changes: 13 additions & 12 deletions substrate/frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -853,6 +855,9 @@ pub mod pallet {
BoundNotMet,
/// Used when attempting to use deprecated controller account logic.
ControllerDeprecated,
/// Account is double bonded, i.e. a stash is also a controller for another ledger or a
/// controller is a stash for another ledger.
DoubleBonded,
}

#[pallet::hooks]
Expand Down Expand Up @@ -1332,8 +1337,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.")
Expand All @@ -1343,9 +1346,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(())
})?
}
Expand Down Expand Up @@ -1960,7 +1962,7 @@ pub mod pallet {
};

if ledger.stash != *controller && !payee_deprecated {
Some((controller.clone(), ledger))
Some(ledger)
} else {
None
}
Expand All @@ -1969,13 +1971,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())
}
}
Expand Down
74 changes: 74 additions & 0 deletions substrate/frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7161,4 +7161,78 @@ mod ledger {
assert_eq!(ledger_updated.stash, stash);
})
}

#[test]
fn deprecate_controller_batch_with_double_bonded_ok() {
ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
setup_double_bonded_ledgers();

// now let's deprecate all the controllers for all the existing ledgers.
let bounded_controllers: BoundedVec<
_,
<Test as Config>::MaxControllersInDeprecationBatch,
> = BoundedVec::try_from(vec![1, 2, 3, 4]).unwrap();

assert_ok!(Staking::deprecate_controller_batch(
RuntimeOrigin::root(),
bounded_controllers
));

assert_eq!(
*staking_events().last().unwrap(),
Event::ControllerBatchDeprecated { failures: 0 }
);
})
}

#[test]
fn deprecate_controller_batch_with_double_bonded_failures() {
ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
setup_double_bonded_ledgers();

// now let's deprecate all the controllers for all the existing ledgers.
let bounded_controllers: BoundedVec<
_,
<Test as Config>::MaxControllersInDeprecationBatch,
> = BoundedVec::try_from(vec![4, 3, 2, 1]).unwrap();

assert_ok!(Staking::deprecate_controller_batch(
RuntimeOrigin::root(),
bounded_controllers
));

assert_eq!(
*staking_events().last().unwrap(),
Event::ControllerBatchDeprecated { failures: 2 }
);
})
}

#[test]
fn set_controller_with_double_bonded_ok() {
ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
setup_double_bonded_ledgers();

assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(2)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(3)));
})
}

#[test]
fn set_controller_with_double_bonded_fail() {
ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
setup_double_bonded_ledgers();

assert_noop!(
Staking::set_controller(RuntimeOrigin::signed(3)),
Error::<Test>::DoubleBonded
);
assert_noop!(
Staking::set_controller(RuntimeOrigin::signed(2)),
Error::<Test>::DoubleBonded
);
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1)));
})
}
}
Loading