From 8699ba1988f46782ca52f61905445b217ea0a762 Mon Sep 17 00:00:00 2001 From: Mokhtar Naamani Date: Tue, 30 Aug 2022 21:31:38 +0400 Subject: [PATCH 1/6] Add BondingRestriction config trait for staking pallet --- bin/node/runtime/src/lib.rs | 1 + frame/staking/src/lib.rs | 16 ++++++++++++++++ frame/staking/src/mock.rs | 10 ++++++++++ frame/staking/src/pallet/mod.rs | 17 +++++++++++++---- frame/staking/src/tests.rs | 26 +++++++++++++++++++++++++- 5 files changed, 65 insertions(+), 5 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d4a72ebb9c45a..ce9dc53043b47 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -566,6 +566,7 @@ impl pallet_staking::Config for Runtime { type OnStakerSlash = NominationPools; type WeightInfo = pallet_staking::weights::SubstrateWeight; type BenchmarkingConfig = StakingBenchmarkingConfig; + type BondingRestriction = (); } parameter_types! { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 331095774b741..7ea16c1580e82 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -917,3 +917,19 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { type MaxValidators = frame_support::traits::ConstU32<100>; type MaxNominators = frame_support::traits::ConstU32<100>; } + +/// Means for checking if there is any external restriction on bonding with a specific accounts +/// +/// Allows for parts of the runtime that might implement other forms fund locking to prevent +/// incompatible locking on accounts which could lead to unsafe state. +pub trait BondingRestriction { + /// Determine if bonding is allowed with stash and controller combination + fn can_bond(stash: &AccountId, controller: &AccountId) -> bool; +} + +// Default implementation when no external restrictions ex +impl BondingRestriction for () { + fn can_bond(_stash: &AccountId, _controller: &AccountId) -> bool { + true + } +} diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index bd2d8cdc32ce9..a3de732edbb24 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -41,6 +41,7 @@ use std::cell::RefCell; pub const INIT_TIMESTAMP: u64 = 30_000; pub const BLOCK_TIME: u64 = 1000; +pub const RESTRICTED_ACCOUNT: AccountId = 55500; /// The AccountId alias in this test module. pub(crate) type AccountId = u64; @@ -302,6 +303,7 @@ impl crate::pallet::pallet::Config for Test { type OnStakerSlash = OnStakerSlashMock; type BenchmarkingConfig = TestBenchmarkingConfig; type WeightInfo = (); + type BondingRestriction = AccountRestricted555; } impl frame_system::offchain::SendTransactionTypes for Test @@ -887,3 +889,11 @@ pub(crate) fn staking_events() -> Vec> { pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) { (Balances::free_balance(who), Balances::reserved_balance(who)) } + +pub struct AccountRestricted555; + +impl BondingRestriction for AccountRestricted555 { + fn can_bond(stash: &AccountId, controller: &AccountId) -> bool { + !(*stash == RESTRICTED_ACCOUNT || *controller == RESTRICTED_ACCOUNT) + } +} diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index e53464195de23..df96f4f85ea2b 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -40,10 +40,10 @@ mod impls; pub use impls::*; use crate::{ - slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, EraRewardPoints, Exposure, - Forcing, MaxUnlockingChunks, NegativeImbalanceOf, Nominations, PositiveImbalanceOf, Releases, - RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk, - ValidatorPrefs, + slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, BondingRestriction, EraPayout, + EraRewardPoints, Exposure, Forcing, MaxUnlockingChunks, NegativeImbalanceOf, Nominations, + PositiveImbalanceOf, Releases, RewardDestination, SessionInterface, StakingLedger, + UnappliedSlash, UnlockChunk, ValidatorPrefs, }; const STAKING_ID: LockIdentifier = *b"staking "; @@ -200,6 +200,9 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + + /// Interface for doing bonding restriction check + type BondingRestriction: BondingRestriction; } #[pallet::type_value] @@ -705,6 +708,8 @@ pub mod pallet { TooManyValidators, /// Commission is too low. Must be at least `MinCommission`. CommissionTooLow, + /// External restriction prevents bonding with given accounts + BondingRestricted, } #[pallet::hooks] @@ -788,6 +793,10 @@ pub mod pallet { return Err(Error::::AlreadyPaired.into()) } + if !T::BondingRestriction::can_bond(&stash, &controller) { + return Err(Error::::BondingRestricted.into()) + } + // Reject a bond which is considered to be _dust_. if value < T::Currency::minimum_balance() { return Err(Error::::InsufficientBond.into()) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index ccd9558c5c21d..752a78987a3e4 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -20,7 +20,7 @@ use super::{ConfigOp, Event, MaxUnlockingChunks, *}; use frame_election_provider_support::{ElectionProvider, SortedListProvider, Support}; use frame_support::{ - assert_noop, assert_ok, assert_storage_noop, bounded_vec, + assert_err, assert_noop, assert_ok, assert_storage_noop, bounded_vec, dispatch::WithPostDispatchInfo, pallet_prelude::*, traits::{Currency, Get, ReservableCurrency}, @@ -5141,3 +5141,27 @@ fn proportional_ledger_slash_works() { BTreeMap::from([(4, 0), (5, value_slashed), (6, 0), (7, 0)]) ); } + +#[test] +fn bond_restriction_works() { + ExtBuilder::default().build_and_execute(|| { + assert_err!( + Staking::bond( + Origin::signed(RESTRICTED_ACCOUNT), + 1, + 1000, + RewardDestination::Controller + ), + Error::::BondingRestricted + ); + assert_err!( + Staking::bond( + Origin::signed(1), + RESTRICTED_ACCOUNT, + 1000, + RewardDestination::Controller + ), + Error::::BondingRestricted + ); + }); +} From 70b0509bbd7a5aa60576585a9e778af92cd7be91 Mon Sep 17 00:00:00 2001 From: Mokhtar Naamani Date: Wed, 31 Aug 2022 09:21:18 +0400 Subject: [PATCH 2/6] additional test for no bonding restriction --- frame/staking/src/lib.rs | 2 +- frame/staking/src/mock.rs | 1 + frame/staking/src/tests.rs | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 7ea16c1580e82..abadf6757e331 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -927,7 +927,7 @@ pub trait BondingRestriction { fn can_bond(stash: &AccountId, controller: &AccountId) -> bool; } -// Default implementation when no external restrictions ex +// No restrictions impl BondingRestriction for () { fn can_bond(_stash: &AccountId, _controller: &AccountId) -> bool { true diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index a3de732edbb24..63813f49ee774 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -892,6 +892,7 @@ pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) { pub struct AccountRestricted555; +// Restrict both stash and controller accounts impl BondingRestriction for AccountRestricted555 { fn can_bond(stash: &AccountId, controller: &AccountId) -> bool { !(*stash == RESTRICTED_ACCOUNT || *controller == RESTRICTED_ACCOUNT) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 752a78987a3e4..613b628d29872 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5145,6 +5145,7 @@ fn proportional_ledger_slash_works() { #[test] fn bond_restriction_works() { ExtBuilder::default().build_and_execute(|| { + start_session(2); assert_err!( Staking::bond( Origin::signed(RESTRICTED_ACCOUNT), @@ -5163,5 +5164,18 @@ fn bond_restriction_works() { ), Error::::BondingRestricted ); + + // put some money in account that we'll use. + for i in RESTRICTED_ACCOUNT + 1..RESTRICTED_ACCOUNT + 2 { + let _ = Balances::make_free_balance_be(&i, 2000); + } + + // Using unrestricted accounts should not fail + assert_ok!(Staking::bond( + Origin::signed(RESTRICTED_ACCOUNT + 1), + RESTRICTED_ACCOUNT + 2, + 1000, + RewardDestination::Controller + )); }); } From 6ea7747f4ed87edf936b6d683082fc3f503ef0f9 Mon Sep 17 00:00:00 2001 From: Mokhtar Naamani Date: Wed, 31 Aug 2022 19:44:19 +0400 Subject: [PATCH 3/6] Update frame/staking/src/lib.rs Co-authored-by: Leszek Wiesner --- frame/staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index abadf6757e331..1ed3d78fb2c33 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -918,7 +918,7 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { type MaxNominators = frame_support::traits::ConstU32<100>; } -/// Means for checking if there is any external restriction on bonding with a specific accounts +/// Means for checking if there is any external restriction on bonding with a specific account /// /// Allows for parts of the runtime that might implement other forms fund locking to prevent /// incompatible locking on accounts which could lead to unsafe state. From 16219fdec797cae2da203e9e1964c0ef3f8b2ee3 Mon Sep 17 00:00:00 2001 From: Mokhtar Naamani Date: Wed, 31 Aug 2022 19:44:32 +0400 Subject: [PATCH 4/6] Update frame/staking/src/lib.rs Co-authored-by: Leszek Wiesner --- frame/staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 1ed3d78fb2c33..2531683e35898 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -920,7 +920,7 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { /// Means for checking if there is any external restriction on bonding with a specific account /// -/// Allows for parts of the runtime that might implement other forms fund locking to prevent +/// Allows for parts of the runtime that might implement other forms of fund locking to prevent /// incompatible locking on accounts which could lead to unsafe state. pub trait BondingRestriction { /// Determine if bonding is allowed with stash and controller combination From ae512518d349878d010145c5609576889a1a9925 Mon Sep 17 00:00:00 2001 From: Mokhtar Naamani Date: Wed, 31 Aug 2022 20:00:40 +0400 Subject: [PATCH 5/6] Staking BondingRestriction - only check stash account --- frame/staking/src/lib.rs | 6 +++--- frame/staking/src/mock.rs | 6 +++--- frame/staking/src/pallet/mod.rs | 4 ++-- frame/staking/src/tests.rs | 17 +++-------------- 4 files changed, 11 insertions(+), 22 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 2531683e35898..611805b796613 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -921,15 +921,15 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { /// Means for checking if there is any external restriction on bonding with a specific account /// /// Allows for parts of the runtime that might implement other forms of fund locking to prevent -/// incompatible locking on accounts which could lead to unsafe state. +/// incompatible locking on a stash account which could lead to unsafe state. pub trait BondingRestriction { /// Determine if bonding is allowed with stash and controller combination - fn can_bond(stash: &AccountId, controller: &AccountId) -> bool; + fn can_bond(stash: &AccountId) -> bool; } // No restrictions impl BondingRestriction for () { - fn can_bond(_stash: &AccountId, _controller: &AccountId) -> bool { + fn can_bond(_stash: &AccountId) -> bool { true } } diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 63813f49ee774..4c2cdde61c12a 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -892,9 +892,9 @@ pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) { pub struct AccountRestricted555; -// Restrict both stash and controller accounts +// Restrict stash account impl BondingRestriction for AccountRestricted555 { - fn can_bond(stash: &AccountId, controller: &AccountId) -> bool { - !(*stash == RESTRICTED_ACCOUNT || *controller == RESTRICTED_ACCOUNT) + fn can_bond(stash: &AccountId) -> bool { + *stash != RESTRICTED_ACCOUNT } } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index df96f4f85ea2b..9656d26bad34c 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -708,7 +708,7 @@ pub mod pallet { TooManyValidators, /// Commission is too low. Must be at least `MinCommission`. CommissionTooLow, - /// External restriction prevents bonding with given accounts + /// External restriction prevents bonding with given account BondingRestricted, } @@ -793,7 +793,7 @@ pub mod pallet { return Err(Error::::AlreadyPaired.into()) } - if !T::BondingRestriction::can_bond(&stash, &controller) { + if !T::BondingRestriction::can_bond(&stash) { return Err(Error::::BondingRestricted.into()) } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 613b628d29872..b16afb9416da5 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5155,25 +5155,14 @@ fn bond_restriction_works() { ), Error::::BondingRestricted ); - assert_err!( - Staking::bond( - Origin::signed(1), - RESTRICTED_ACCOUNT, - 1000, - RewardDestination::Controller - ), - Error::::BondingRestricted - ); // put some money in account that we'll use. - for i in RESTRICTED_ACCOUNT + 1..RESTRICTED_ACCOUNT + 2 { - let _ = Balances::make_free_balance_be(&i, 2000); - } + let _ = Balances::make_free_balance_be(&(RESTRICTED_ACCOUNT + 1), 2000); - // Using unrestricted accounts should not fail + // Using unrestricted account should not fail assert_ok!(Staking::bond( Origin::signed(RESTRICTED_ACCOUNT + 1), - RESTRICTED_ACCOUNT + 2, + 1, 1000, RewardDestination::Controller )); From 867507cab3a5ef2d2e5e56093f9880d722a81e29 Mon Sep 17 00:00:00 2001 From: Mokhtar Naamani Date: Wed, 31 Aug 2022 20:28:09 +0400 Subject: [PATCH 6/6] Update frame mocks - add missing BondingRestriction type on pallet_staking::Config impls --- frame/babe/src/mock.rs | 1 + frame/grandpa/src/mock.rs | 1 + frame/nomination-pools/benchmarking/src/mock.rs | 1 + frame/offences/benchmarking/src/mock.rs | 1 + frame/session/benchmarking/src/mock.rs | 1 + 5 files changed, 5 insertions(+) diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 5677eb7e28e49..61730c478cbc7 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -206,6 +206,7 @@ impl pallet_staking::Config for Test { type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); + type BondingRestriction = (); } impl pallet_offences::Config for Test { diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 5e6c955c441c5..54d968ae32bdd 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -210,6 +210,7 @@ impl pallet_staking::Config for Test { type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); + type BondingRestriction = (); } impl pallet_offences::Config for Test { diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index eb884869f6d32..a441a7eb6f5bf 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -113,6 +113,7 @@ impl pallet_staking::Config for Runtime { type OnStakerSlash = Pools; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); + type BondingRestriction = (); } parameter_types! { diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index d51a81b1212c0..a6239799f0900 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -182,6 +182,7 @@ impl pallet_staking::Config for Test { type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); + type BondingRestriction = (); } impl pallet_im_online::Config for Test { diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index c777f2c56de3a..5b25b31bd14b2 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -188,6 +188,7 @@ impl pallet_staking::Config for Test { type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); + type BondingRestriction = (); } impl crate::Config for Test {}