Skip to content

Commit 2664b3b

Browse files
A0-4020: fix for fix accounts consumers underflow (#1643)
# Description `operations.fix_accounts_consumers_underflow` does not take into account controller != stash scenario, in which case `consumers` should not be incremented on stash account, but on the controller account. ## Type of change Please delete options that are not relevant. - Bug fix (non-breaking change which fixes an issue)
1 parent 02b83f6 commit 2664b3b

10 files changed

Lines changed: 184 additions & 57 deletions

File tree

Cargo.lock

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

bin/node/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "aleph-node"
3-
version = "0.13.1"
3+
version = "0.13.2"
44
description = "Aleph node binary"
55
build = "build.rs"
66
license = "GPL-3.0-or-later"

bin/runtime/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "aleph-runtime"
3-
version = "0.13.1"
3+
version = "0.13.2"
44
license = "GPL-3.0-or-later"
55
authors.workspace = true
66
edition.workspace = true

bin/runtime/src/lib.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
9797
spec_name: create_runtime_str!("aleph-node"),
9898
impl_name: create_runtime_str!("aleph-node"),
9999
authoring_version: 1,
100-
spec_version: 70,
100+
spec_version: 71,
101101
impl_version: 1,
102102
apis: RUNTIME_API_VERSIONS,
103103
transaction_version: 18,
@@ -381,6 +381,7 @@ impl pallet_operations::Config for Runtime {
381381
type AccountInfoProvider = System;
382382
type BalancesProvider = Balances;
383383
type NextKeysSessionProvider = Session;
384+
type BondedStashProvider = Staking;
384385
}
385386

386387
impl pallet_committee_management::Config for Runtime {

pallets/operations/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@ frame-support = { workspace = true }
1515
frame-system = { workspace = true }
1616
pallet-session = { workspace = true }
1717
pallet-balances = { workspace = true }
18+
pallet-staking = { workspace = true }
1819
sp-runtime = { workspace = true }
1920
sp-core = { workspace = true }
2021

2122
[dev-dependencies]
2223
sp-io = { workspace = true }
23-
pallet-staking = { workspace = true }
2424
pallet-timestamp = { workspace = true }
2525
frame-election-provider-support = { workspace = true }
2626

pallets/operations/src/impls.rs

Lines changed: 55 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,85 @@
11
#![allow(clippy::nonminimal_bool)]
22

3-
use frame_support::{
4-
dispatch::DispatchResultWithPostInfo, pallet_prelude::Get, traits::LockIdentifier,
5-
WeakBoundedVec,
6-
};
3+
use frame_support::{dispatch::DispatchResult, traits::LockIdentifier, WeakBoundedVec};
74
use pallet_balances::BalanceLock;
85
use parity_scale_codec::Encode;
96
use sp_core::hexdisplay::HexDisplay;
107
use sp_runtime::DispatchError;
118

129
use crate::{
1310
pallet::{Config, Event, Pallet},
14-
traits::{AccountInfoProvider, BalancesProvider, NextKeysSessionProvider},
11+
traits::{AccountInfoProvider, BalancesProvider, BondedStashProvider, NextKeysSessionProvider},
1512
LOG_TARGET, STAKING_ID, VESTING_ID,
1613
};
1714

1815
impl<T: Config> Pallet<T> {
1916
/// Checks if account has an underflow of `consumers` counter. In such case, it increments
2017
/// it by one.
21-
pub fn fix_underflow_consumer_counter(who: T::AccountId) -> DispatchResultWithPostInfo {
22-
let mut weight = T::DbWeight::get().reads(1);
23-
let consumers = T::AccountInfoProvider::get_consumers(&who);
18+
pub fn fix_underflow_consumer_counter(who: T::AccountId) -> DispatchResult {
19+
let current_consumers = T::AccountInfoProvider::get_consumers(&who);
20+
let mut expected_consumers: u32 = 0;
2421

25-
weight += T::DbWeight::get().reads(1);
26-
if Self::no_consumers_some_reserved(&who, consumers) {
27-
Self::increment_consumers(who)?;
28-
weight += T::DbWeight::get().writes(1);
29-
return Ok(Some(weight).into());
22+
if Self::reserved_or_frozen_non_zero(&who) {
23+
expected_consumers += 1;
24+
}
25+
let has_vesting_lock = Self::has_vesting_lock(&who);
26+
let has_staking_lock = Self::has_staking_lock(&who);
27+
if has_staking_lock || has_vesting_lock {
28+
expected_consumers += 1;
29+
if has_staking_lock {
30+
expected_consumers += 1;
31+
}
32+
}
33+
if Self::has_next_session_keys_and_account_is_controller(&who) {
34+
expected_consumers += 1;
3035
}
3136

32-
weight += T::DbWeight::get().reads(2);
33-
if Self::staker_has_consumers_underflow(&who, consumers) {
37+
if current_consumers < expected_consumers {
38+
log::debug!(
39+
target: LOG_TARGET,
40+
"Account {:?} has current consumers {} less than expected consumers {:?}, incrementing ",
41+
HexDisplay::from(&who.encode()), current_consumers, expected_consumers);
3442
Self::increment_consumers(who)?;
35-
weight += T::DbWeight::get().writes(1);
36-
return Ok(Some(weight).into());
43+
} else {
44+
log::debug!(
45+
target: LOG_TARGET,
46+
"Account {:?} does not have consumers underflow, not incrementing",
47+
HexDisplay::from(&who.encode())
48+
);
3749
}
3850

39-
log::debug!(
40-
target: LOG_TARGET,
41-
"Account {:?} has correct consumer counter, not incrementing",
42-
HexDisplay::from(&who.encode())
43-
);
44-
Ok(Some(weight).into())
51+
Ok(())
52+
}
53+
54+
fn reserved_or_frozen_non_zero(who: &T::AccountId) -> bool {
55+
!T::BalancesProvider::is_reserved_zero(who) || !T::BalancesProvider::is_frozen_zero(who)
4556
}
4657

47-
fn staker_has_consumers_underflow(who: &T::AccountId, consumers: u32) -> bool {
58+
fn has_vesting_lock(who: &T::AccountId) -> bool {
4859
let locks = T::BalancesProvider::locks(who);
49-
let has_vesting_lock = Self::has_lock(&locks, VESTING_ID);
50-
let vester_has_consumers_underflow = consumers == 1 && has_vesting_lock;
51-
let has_staking_lock = Self::has_lock(&locks, STAKING_ID);
52-
let nominator_has_consumers_underflow = consumers == 2 && has_staking_lock;
53-
let has_next_session_keys = T::NextKeysSessionProvider::has_next_session_keys(who);
54-
let validator_has_consumers_underflow =
55-
consumers == 3 && has_staking_lock && has_next_session_keys;
56-
vester_has_consumers_underflow
57-
|| nominator_has_consumers_underflow
58-
|| validator_has_consumers_underflow
60+
Self::has_lock(&locks, VESTING_ID)
5961
}
6062

61-
fn no_consumers_some_reserved(who: &T::AccountId, consumers: u32) -> bool {
62-
let is_reserved_not_zero = T::BalancesProvider::is_reserved_not_zero(who);
63+
fn has_staking_lock(who: &T::AccountId) -> bool {
64+
let locks = T::BalancesProvider::locks(who);
65+
Self::has_lock(&locks, STAKING_ID)
66+
}
6367

64-
consumers == 0 && is_reserved_not_zero
68+
fn has_next_session_keys_and_account_is_controller(who: &T::AccountId) -> bool {
69+
let has_next_session_keys = T::NextKeysSessionProvider::has_next_session_keys(who);
70+
let stash_equal_to_controller = match T::BondedStashProvider::get_controller(who) {
71+
Some(controller) => *who == controller,
72+
None => false,
73+
};
74+
if has_next_session_keys && stash_equal_to_controller {
75+
return true;
76+
}
77+
match T::BondedStashProvider::get_stash(who) {
78+
Some(stash) => {
79+
*who != stash && T::NextKeysSessionProvider::has_next_session_keys(&stash)
80+
}
81+
None => false,
82+
}
6583
}
6684

6785
fn has_lock<U, V>(locks: &WeakBoundedVec<BalanceLock<U>, V>, id: LockIdentifier) -> bool {

pallets/operations/src/lib.rs

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ pub mod pallet {
2626
use frame_system::{ensure_signed, pallet_prelude::OriginFor};
2727

2828
use crate::{
29-
traits::{AccountInfoProvider, BalancesProvider, NextKeysSessionProvider},
29+
traits::{
30+
AccountInfoProvider, BalancesProvider, BondedStashProvider, NextKeysSessionProvider,
31+
},
3032
STORAGE_VERSION,
3133
};
3234

@@ -35,8 +37,12 @@ pub mod pallet {
3537
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
3638
/// Something that provides information about an account's consumers counter
3739
type AccountInfoProvider: AccountInfoProvider<AccountId = Self::AccountId, RefCount = u32>;
40+
/// Something that provides information about account's balances
3841
type BalancesProvider: BalancesProvider<AccountId = Self::AccountId>;
42+
/// Something that provides information about an account's next session keys
3943
type NextKeysSessionProvider: NextKeysSessionProvider<AccountId = Self::AccountId>;
44+
/// Something that provides information about an account's controller
45+
type BondedStashProvider: BondedStashProvider<AccountId = Self::AccountId>;
4046
}
4147

4248
#[pallet::pallet]
@@ -56,14 +62,13 @@ pub mod pallet {
5662
/// An account can have an underflow of a `consumers` counter.
5763
/// Account categories that are impacted by this issue depends on a chain runtime,
5864
/// but specifically for AlephNode runtime are as follows:
59-
/// * `consumers` == 0, `reserved` > 0
60-
/// * `consumers` == 1, `balances.Locks` contain an entry with `id` == `vesting`
61-
/// * `consumers` == 2, `balances.Locks` contain an entry with `id` == `staking`
62-
/// * `consumers` == 3, `balances.Locks` contain entries with `id` == `staking`
63-
/// and account id is in `session.nextKeys`
65+
/// +1 consumers if reserved > 0 || frozen > 0
66+
/// +1 consumers if there is at least one lock (staking or vesting)
67+
/// +1 consumers if there's session.nextKeys set, for controller account
68+
/// +1 consumers if account bonded
6469
///
65-
/// `fix_accounts_consumers_underflow` checks if the account falls into one of above
66-
/// categories, and increase its `consumers` counter.
70+
/// `fix_accounts_consumers_underflow` calculates expected consumers counter and comperes
71+
/// it with current consumers counter, incrementing by one in case of an underflow
6772
///
6873
/// - `origin`: Must be `Signed`.
6974
/// - `who`: An account to be fixed
@@ -75,10 +80,10 @@ pub mod pallet {
7580
pub fn fix_accounts_consumers_underflow(
7681
origin: OriginFor<T>,
7782
who: T::AccountId,
78-
) -> DispatchResultWithPostInfo {
83+
) -> DispatchResult {
7984
ensure_signed(origin)?;
8085
Self::fix_underflow_consumer_counter(who)?;
81-
Ok(().into())
86+
Ok(())
8287
}
8388
}
8489
}

pallets/operations/src/tests/setup.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ impl pallet_operations::Config for TestRuntime {
187187
type AccountInfoProvider = System;
188188
type BalancesProvider = Balances;
189189
type NextKeysSessionProvider = Session;
190+
type BondedStashProvider = Staking;
190191
}
191192

192193
pub fn new_test_ext(accounts_and_balances: &[(u64, bool, u128)]) -> sp_io::TestExternalities {

pallets/operations/src/tests/suite.rs

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -416,7 +416,7 @@ fn given_nominator_account_with_staking_lock_when_fixing_consumers_then_consumer
416416
}
417417

418418
#[test]
419-
fn given_validator_account_with_staking_lock_when_fixing_consumers_then_consumers_increase() {
419+
fn given_validator_with_stash_equal_to_consumer_when_fixing_consumers_then_consumers_increases() {
420420
let authority_id = 1_u64;
421421
let non_authority_id = 2_u64;
422422
let total_balance_authority = 1000_u128;
@@ -450,3 +450,60 @@ fn given_validator_account_with_staking_lock_when_fixing_consumers_then_consumer
450450
assert_eq!(consumers(authority_id), 4);
451451
});
452452
}
453+
454+
#[test]
455+
fn given_validator_with_stash_not_equal_to_controller_when_fixing_consumers_then_consumers_increases_on_controller_only(
456+
) {
457+
let authority_id = 1_u64;
458+
let non_authority_id = 2_u64;
459+
let total_balance_authority = 1000_u128;
460+
let total_balance_non_authority = 999_u128;
461+
new_test_ext(&[
462+
(authority_id, true, total_balance_authority),
463+
(non_authority_id, false, total_balance_non_authority),
464+
])
465+
.execute_with(|| {
466+
let bonded = 123_u128;
467+
assert_ok!(pallet_staking::Pallet::<TestRuntime>::bond(
468+
RuntimeOrigin::signed(authority_id),
469+
bonded,
470+
RewardDestination::Controller
471+
));
472+
// direct manipulation on pallet storage is not possible from end user perspective,
473+
// but to mimic that scenario we need to directly set Bonded so stash != controller,
474+
// that is not possible to do via pallet staking API anymore
475+
// below procedure mimic what set_controller did back in 11 version, ie no manipulations
476+
// on consumers counter
477+
pallet_staking::Bonded::<TestRuntime>::set(authority_id, Some(non_authority_id));
478+
let ledger = pallet_staking::Ledger::<TestRuntime>::take(authority_id).unwrap();
479+
pallet_staking::Ledger::<TestRuntime>::set(non_authority_id, Some(ledger));
480+
481+
frame_system::Pallet::<TestRuntime>::dec_consumers(&authority_id);
482+
assert_eq!(consumers(authority_id), 3);
483+
assert_eq!(consumers(non_authority_id), 0);
484+
frame_system::Pallet::<TestRuntime>::reset_events();
485+
assert_eq!(pallet_operations_events().len(), 0);
486+
assert_ok!(
487+
crate::Pallet::<TestRuntime>::fix_accounts_consumers_underflow(
488+
RuntimeOrigin::signed(non_authority_id),
489+
authority_id
490+
)
491+
);
492+
assert_eq!(pallet_operations_events().len(), 0);
493+
assert_eq!(consumers(authority_id), 3);
494+
495+
assert_ok!(
496+
crate::Pallet::<TestRuntime>::fix_accounts_consumers_underflow(
497+
RuntimeOrigin::signed(authority_id),
498+
non_authority_id
499+
)
500+
);
501+
assert_eq!(
502+
pallet_operations_events(),
503+
[crate::Event::ConsumersUnderflowFixed {
504+
who: non_authority_id
505+
}]
506+
);
507+
assert_eq!(consumers(non_authority_id), 1);
508+
});
509+
}

0 commit comments

Comments
 (0)