A0-4020: fix for fix accounts consumers underflow (#1643)

`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.

Please delete options that are not relevant.

- Bug fix (non-breaking change which fixes an issue)

bin/runtime/src/lib.rs

@@ -101,7 +101,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_name: create_runtime_str!("aleph-node"),
     impl_name: create_runtime_str!("aleph-node"),
     authoring_version: 1,
-    spec_version: 70,
+    spec_version: 71,
     impl_version: 1,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 18,
@@ -397,6 +397,7 @@ impl pallet_operations::Config for Runtime {
     type AccountInfoProvider = System;
     type BalancesProvider = Balances;
     type NextKeysSessionProvider = Session;
+    type BondedStashProvider = Staking;
 }
 
 impl pallet_committee_management::Config for Runtime {

pallets/operations/Cargo.toml

@@ -15,12 +15,12 @@ frame-support = { workspace = true }
 frame-system = { workspace = true }
 pallet-session = { workspace = true }
 pallet-balances = { workspace = true }
+pallet-staking = { workspace = true }
 sp-runtime = { workspace = true }
 sp-core = { workspace = true }
 
 [dev-dependencies]
 sp-io = { workspace = true }
-pallet-staking = { workspace = true }
 pallet-timestamp = { workspace = true }
 frame-election-provider-support = { workspace = true }
 

pallets/operations/src/impls.rs

@@ -1,67 +1,85 @@
 #![allow(clippy::nonminimal_bool)]
 
-use frame_support::{
-    dispatch::DispatchResultWithPostInfo, pallet_prelude::Get, traits::LockIdentifier,
-    WeakBoundedVec,
-};
+use frame_support::{dispatch::DispatchResult, traits::LockIdentifier, WeakBoundedVec};
 use pallet_balances::BalanceLock;
 use parity_scale_codec::Encode;
 use sp_core::hexdisplay::HexDisplay;
 use sp_runtime::DispatchError;
 
 use crate::{
     pallet::{Config, Event, Pallet},
-    traits::{AccountInfoProvider, BalancesProvider, NextKeysSessionProvider},
+    traits::{AccountInfoProvider, BalancesProvider, BondedStashProvider, NextKeysSessionProvider},
     LOG_TARGET, STAKING_ID, VESTING_ID,
 };
 
 impl<T: Config> Pallet<T> {
     /// Checks if account has an underflow of `consumers` counter. In such case, it increments
     /// it by one.
-    pub fn fix_underflow_consumer_counter(who: T::AccountId) -> DispatchResultWithPostInfo {
-        let mut weight = T::DbWeight::get().reads(1);
-        let consumers = T::AccountInfoProvider::get_consumers(&who);
+    pub fn fix_underflow_consumer_counter(who: T::AccountId) -> DispatchResult {
+        let current_consumers = T::AccountInfoProvider::get_consumers(&who);
+        let mut expected_consumers: u32 = 0;
 
-        weight += T::DbWeight::get().reads(1);
-        if Self::no_consumers_some_reserved(&who, consumers) {
-            Self::increment_consumers(who)?;
-            weight += T::DbWeight::get().writes(1);
-            return Ok(Some(weight).into());
+        if Self::reserved_or_frozen_non_zero(&who) {
+            expected_consumers += 1;
+        }
+        let has_vesting_lock = Self::has_vesting_lock(&who);
+        let has_staking_lock = Self::has_staking_lock(&who);
+        if has_staking_lock || has_vesting_lock {
+            expected_consumers += 1;
+            if has_staking_lock {
+                expected_consumers += 1;
+            }
+        }
+        if Self::has_next_session_keys_and_account_is_controller(&who) {
+            expected_consumers += 1;
         }
 
-        weight += T::DbWeight::get().reads(2);
-        if Self::staker_has_consumers_underflow(&who, consumers) {
+        if current_consumers < expected_consumers {
+            log::debug!(
+                target: LOG_TARGET,
+                "Account {:?} has current consumers {} less than expected consumers {:?}, incrementing ",
+                HexDisplay::from(&who.encode()), current_consumers, expected_consumers);
             Self::increment_consumers(who)?;
-            weight += T::DbWeight::get().writes(1);
-            return Ok(Some(weight).into());
+        } else {
+            log::debug!(
+                target: LOG_TARGET,
+                "Account {:?} does not have consumers underflow, not incrementing",
+                HexDisplay::from(&who.encode())
+            );
         }
 
-        log::debug!(
-            target: LOG_TARGET,
-            "Account {:?} has correct consumer counter, not incrementing",
-            HexDisplay::from(&who.encode())
-        );
-        Ok(Some(weight).into())
+        Ok(())
+    }
+
+    fn reserved_or_frozen_non_zero(who: &T::AccountId) -> bool {
+        !T::BalancesProvider::is_reserved_zero(who) || !T::BalancesProvider::is_frozen_zero(who)
     }
 
-    fn staker_has_consumers_underflow(who: &T::AccountId, consumers: u32) -> bool {
+    fn has_vesting_lock(who: &T::AccountId) -> bool {
         let locks = T::BalancesProvider::locks(who);
-        let has_vesting_lock = Self::has_lock(&locks, VESTING_ID);
-        let vester_has_consumers_underflow = consumers == 1 && has_vesting_lock;
-        let has_staking_lock = Self::has_lock(&locks, STAKING_ID);
-        let nominator_has_consumers_underflow = consumers == 2 && has_staking_lock;
-        let has_next_session_keys = T::NextKeysSessionProvider::has_next_session_keys(who);
-        let validator_has_consumers_underflow =
-            consumers == 3 && has_staking_lock && has_next_session_keys;
-        vester_has_consumers_underflow
-            || nominator_has_consumers_underflow
-            || validator_has_consumers_underflow
+        Self::has_lock(&locks, VESTING_ID)
     }
 
-    fn no_consumers_some_reserved(who: &T::AccountId, consumers: u32) -> bool {
-        let is_reserved_not_zero = T::BalancesProvider::is_reserved_not_zero(who);
+    fn has_staking_lock(who: &T::AccountId) -> bool {
+        let locks = T::BalancesProvider::locks(who);
+        Self::has_lock(&locks, STAKING_ID)
+    }
 
-        consumers == 0 && is_reserved_not_zero
+    fn has_next_session_keys_and_account_is_controller(who: &T::AccountId) -> bool {
+        let has_next_session_keys = T::NextKeysSessionProvider::has_next_session_keys(who);
+        let stash_equal_to_controller = match T::BondedStashProvider::get_controller(who) {
+            Some(controller) => *who == controller,
+            None => false,
+        };
+        if has_next_session_keys && stash_equal_to_controller {
+            return true;
+        }
+        match T::BondedStashProvider::get_stash(who) {
+            Some(stash) => {
+                *who != stash && T::NextKeysSessionProvider::has_next_session_keys(&stash)
+            }
+            None => false,
+        }
     }
 
     fn has_lock<U, V>(locks: &WeakBoundedVec<BalanceLock<U>, V>, id: LockIdentifier) -> bool {

pallets/operations/src/lib.rs

@@ -26,7 +26,9 @@ pub mod pallet {
     use frame_system::{ensure_signed, pallet_prelude::OriginFor};
 
     use crate::{
-        traits::{AccountInfoProvider, BalancesProvider, NextKeysSessionProvider},
+        traits::{
+            AccountInfoProvider, BalancesProvider, BondedStashProvider, NextKeysSessionProvider,
+        },
         STORAGE_VERSION,
     };
 
@@ -35,8 +37,12 @@ pub mod pallet {
         type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
         /// Something that provides information about an account's consumers counter
         type AccountInfoProvider: AccountInfoProvider<AccountId = Self::AccountId, RefCount = u32>;
+        /// Something that provides information about account's balances
         type BalancesProvider: BalancesProvider<AccountId = Self::AccountId>;
+        /// Something that provides information about an account's next session keys
         type NextKeysSessionProvider: NextKeysSessionProvider<AccountId = Self::AccountId>;
+        /// Something that provides information about an account's controller
+        type BondedStashProvider: BondedStashProvider<AccountId = Self::AccountId>;
     }
 
     #[pallet::pallet]
@@ -56,14 +62,13 @@ pub mod pallet {
         /// An account can have an underflow of a `consumers` counter.
         /// Account categories that are impacted by this issue depends on a chain runtime,
         /// but specifically for AlephNode runtime are as follows:
-        /// * `consumers`  == 0, `reserved`  > 0
-        /// * `consumers`  == 1, `balances.Locks` contain an entry with `id`  == `vesting`
-        /// * `consumers`  == 2, `balances.Locks` contain an entry with `id`  == `staking`
-        /// * `consumers`  == 3, `balances.Locks` contain entries with `id`  == `staking`
-        ///    and account id is in `session.nextKeys`
+        /// +1 consumers if reserved > 0 || frozen > 0
+        /// +1 consumers if there is at least one lock (staking or vesting)
+        /// +1 consumers if there's session.nextKeys set, for controller account
+        /// +1 consumers if account bonded
         ///
-        ///	`fix_accounts_consumers_underflow` checks if the account falls into one of above
-        /// categories, and increase its `consumers` counter.
+        ///	`fix_accounts_consumers_underflow` calculates expected consumers counter and comperes
+        /// it with current consumers counter, incrementing by one in case of an underflow
         ///
         /// - `origin`: Must be `Signed`.
         /// - `who`: An account to be fixed
@@ -75,10 +80,10 @@ pub mod pallet {
         pub fn fix_accounts_consumers_underflow(
             origin: OriginFor<T>,
             who: T::AccountId,
-        ) -> DispatchResultWithPostInfo {
+        ) -> DispatchResult {
             ensure_signed(origin)?;
             Self::fix_underflow_consumer_counter(who)?;
-            Ok(().into())
+            Ok(())
         }
     }
 }

pallets/operations/src/tests/setup.rs

@@ -188,6 +188,7 @@ impl pallet_operations::Config for TestRuntime {
     type AccountInfoProvider = System;
     type BalancesProvider = Balances;
     type NextKeysSessionProvider = Session;
+    type BondedStashProvider = Staking;
 }
 
 pub fn new_test_ext(accounts_and_balances: &[(u64, bool, u128)]) -> sp_io::TestExternalities {

pallets/operations/src/tests/suite.rs

@@ -416,7 +416,7 @@ fn given_nominator_account_with_staking_lock_when_fixing_consumers_then_consumer
 }
 
 #[test]
-fn given_validator_account_with_staking_lock_when_fixing_consumers_then_consumers_increase() {
+fn given_validator_with_stash_equal_to_consumer_when_fixing_consumers_then_consumers_increases() {
     let authority_id = 1_u64;
     let non_authority_id = 2_u64;
     let total_balance_authority = 1000_u128;
@@ -450,3 +450,60 @@ fn given_validator_account_with_staking_lock_when_fixing_consumers_then_consumer
         assert_eq!(consumers(authority_id), 4);
     });
 }
+
+#[test]
+fn given_validator_with_stash_not_equal_to_controller_when_fixing_consumers_then_consumers_increases_on_controller_only(
+) {
+    let authority_id = 1_u64;
+    let non_authority_id = 2_u64;
+    let total_balance_authority = 1000_u128;
+    let total_balance_non_authority = 999_u128;
+    new_test_ext(&[
+        (authority_id, true, total_balance_authority),
+        (non_authority_id, false, total_balance_non_authority),
+    ])
+    .execute_with(|| {
+        let bonded = 123_u128;
+        assert_ok!(pallet_staking::Pallet::<TestRuntime>::bond(
+            RuntimeOrigin::signed(authority_id),
+            bonded,
+            RewardDestination::Controller
+        ));
+        // direct manipulation on pallet storage is not possible from end user perspective,
+        // but to mimic that scenario we need to directly set Bonded so stash != controller,
+        // that is not possible to do via pallet staking API anymore
+        // below procedure mimic what set_controller did back in 11 version, ie no manipulations
+        // on consumers counter
+        pallet_staking::Bonded::<TestRuntime>::set(authority_id, Some(non_authority_id));
+        let ledger = pallet_staking::Ledger::<TestRuntime>::take(authority_id).unwrap();
+        pallet_staking::Ledger::<TestRuntime>::set(non_authority_id, Some(ledger));
+
+        frame_system::Pallet::<TestRuntime>::dec_consumers(&authority_id);
+        assert_eq!(consumers(authority_id), 3);
+        assert_eq!(consumers(non_authority_id), 0);
+        frame_system::Pallet::<TestRuntime>::reset_events();
+        assert_eq!(pallet_operations_events().len(), 0);
+        assert_ok!(
+            crate::Pallet::<TestRuntime>::fix_accounts_consumers_underflow(
+                RuntimeOrigin::signed(non_authority_id),
+                authority_id
+            )
+        );
+        assert_eq!(pallet_operations_events().len(), 0);
+        assert_eq!(consumers(authority_id), 3);
+
+        assert_ok!(
+            crate::Pallet::<TestRuntime>::fix_accounts_consumers_underflow(
+                RuntimeOrigin::signed(authority_id),
+                non_authority_id
+            )
+        );
+        assert_eq!(
+            pallet_operations_events(),
+            [crate::Event::ConsumersUnderflowFixed {
+                who: non_authority_id
+            }]
+        );
+        assert_eq!(consumers(non_authority_id), 1);
+    });
+}

pallets/operations/src/traits.rs

@@ -3,9 +3,12 @@ use pallet_balances::BalanceLock;
 use sp_runtime::traits::Zero;
 
 pub trait AccountInfoProvider {
+    /// Account id type used by runtime
     type AccountId;
+    /// Reference counter type used by runtime
     type RefCount;
 
+    /// Retrieves account's consumer counter
     fn get_consumers(who: &Self::AccountId) -> Self::RefCount;
 }
 
@@ -22,12 +25,20 @@ where
 }
 
 pub trait BalancesProvider {
+    /// Account id type used by runtime
     type AccountId;
+    /// Balance type used by runtime
     type Balance;
+    /// Max locks constant used by runtime
     type MaxLocks;
 
-    fn is_reserved_not_zero(who: &Self::AccountId) -> bool;
+    /// Returns reserved funds of an account
+    fn is_reserved_zero(who: &Self::AccountId) -> bool;
 
+    /// Returns frozen funds of an account
+    fn is_frozen_zero(who: &Self::AccountId) -> bool;
+
+    /// Retrieves account's balance locks (e.g. staking or vesting)
     fn locks(who: &Self::AccountId) -> WeakBoundedVec<BalanceLock<Self::Balance>, Self::MaxLocks>;
 }
 
@@ -36,8 +47,12 @@ impl<T: pallet_balances::Config<I>, I: 'static> BalancesProvider for pallet_bala
     type Balance = T::Balance;
     type MaxLocks = T::MaxLocks;
 
-    fn is_reserved_not_zero(who: &Self::AccountId) -> bool {
-        !T::AccountStore::get(who).reserved.is_zero()
+    fn is_reserved_zero(who: &Self::AccountId) -> bool {
+        T::AccountStore::get(who).reserved.is_zero()
+    }
+
+    fn is_frozen_zero(who: &Self::AccountId) -> bool {
+        T::AccountStore::get(who).frozen.is_zero()
     }
 
     fn locks(who: &Self::AccountId) -> WeakBoundedVec<BalanceLock<Self::Balance>, Self::MaxLocks> {
@@ -46,8 +61,10 @@ impl<T: pallet_balances::Config<I>, I: 'static> BalancesProvider for pallet_bala
 }
 
 pub trait NextKeysSessionProvider {
+    /// Account id type used by runtime
     type AccountId;
 
+    /// Retrieves information whether given account is in the next session keys
     fn has_next_session_keys(who: &Self::AccountId) -> bool;
 }
 
@@ -61,3 +78,31 @@ where
         pallet_session::NextKeys::<T>::get(who).is_some()
     }
 }
+
+pub trait BondedStashProvider {
+    /// Account id type used by runtime
+    type AccountId;
+
+    /// Retrieves information about controller of given stash account, or None if account
+    /// have not bonded yet
+    fn get_controller(stash: &Self::AccountId) -> Option<Self::AccountId>;
+
+    /// Retrieves information about stash of given controller account, or None if account
+    /// have not bonded yet
+    fn get_stash(stash: &Self::AccountId) -> Option<Self::AccountId>;
+}
+
+impl<T> BondedStashProvider for pallet_staking::Pallet<T>
+where
+    T: frame_system::Config + pallet_staking::Config,
+{
+    type AccountId = T::AccountId;
+
+    fn get_controller(stash: &Self::AccountId) -> Option<Self::AccountId> {
+        pallet_staking::Pallet::<T>::bonded(stash)
+    }
+
+    fn get_stash(stash: &Self::AccountId) -> Option<Self::AccountId> {
+        pallet_staking::Pallet::<T>::ledger(stash).map(|ledger| ledger.stash)
+    }
+}