Repository: chainx-org/ChainX (language: rust)

Issue:
Staking locked balances can be more than the actual free balances
https://github.com/chainx-org/ChainX/blob/8d78cde1e11d80a906cacf81ea1fb10c1d6dc133/xpallets/mining/staking/src/lib.rs#L351

This line will cause there are **more Staking votes than the actual balances**, which also means we might lock more funds than a user has. For instance, `5UW4LiYMc1r1X3yQVLCVi281wi5GCyRCwJZ3d9jHpbBSGW5Y` has negative usable balances due to the over-staking-lock.

```bash
~/.../chainx-org/chainx-cli ❯❯❯ ./target/release/chainx-cli --url ws://116.62.46.8:8087 xstaking storage locks 5UW4LiYMc1r1X3yQVLCVi281wi5GCyRCwJZ3d9jHpbBSGW5Y --block-number 107671
d719d3036ac37da50627651e2353873f5eb9cdbce547ee5684b0618171bf7d0a (5GvjpjTY...): {
    Bonded: 2073339270000,
    BondedWithdrawal: 1264702800000,
}
# The usable is -5339.75070541PCX
~/.../chainx-org/chainx-cli ❯❯❯ ./target/release/chainx-cli --url ws://116.62.46.8:8087 system account-info 5UW4LiYMc1r1X3yQVLCVi281wi5GCyRCwJZ3d9jHpbBSGW5Y --block-number 107671
AccountInfo of d719d3036ac37da50627651e2353873f5eb9cdbce547ee5684b0618171bf7d0a (5GvjpjTY...): AccountInfo {
    nonce: 37,
    refcount: 2,
    data: AccountData {
        free: 2804066999459,
        reserved: 1000000,
        misc_frozen: 3338042070000,
        fee_frozen: 3338042070000,
    },
}
```

```
{
	"id":1,
	"jsonrpc":"2.0",
	"method":"xstaking_getNominationByAccount",
	"params":["5UW4LiYMc1r1X3yQVLCVi281wi5GCyRCwJZ3d9jHpbBSGW5Y", "0x923161c814c5d0d01c3926d36acee704a97c18cdb59ade0cbd72b3da667066c3"]
}
```

We expect 1 PCX 1 vote in theory, but there are more Staking votes now, the consequence is that some stakers are earning more rewards than they should. If someone's frozen balances are more than his free balance, then he has more unexpected gains due to this bug, at the same time some other people will have fewer rewards as the reward pot per validator is correct. This bug only relates to the distribution of all nominators behind a validator, some earn more and some others earn less.

Should be fixed by #408.
https://github.com/chainx-org/ChainX/pull/408/files#diff-acfc4fead7dba36919225985639fe230fc951b8b6630dc3e48d15fb5a3366076R351-R354 will ensure 1 PCX 1 vote again.


The stakers having any windfall, especially these have negative usable balance now, will have to wait for the fix using `XStaking::force_reset_staking_lock()`.

Update: This bug is not the root cause of the staking reward drop as there only are 10 accounts with negative usable balance, the total involved money is less than 6000PCX, which is too small compared to the total stakes(6M+). The main reason is when we migrated to 2.0, we settled all the latest vote weight of 1.0. And the vote weight is measured by blocks, which means we have to spend 3 times the time to obtain the same vote weight in 2.0, so the users with legacy vote weight earns more, but once they make their first claim on 2.0, they will have no advantages.


Discussion / hints:

Fix patch:
diff --git a/runtime/chainx/src/lib.rs b/runtime/chainx/src/lib.rs
index d6eff4173..f1fd4d5e7 100644
--- a/runtime/chainx/src/lib.rs
+++ b/runtime/chainx/src/lib.rs
@@ -104,7 +104,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_name: create_runtime_str!("chainx"),
     impl_name: create_runtime_str!("chainx-net"),
     authoring_version: 1,
-    spec_version: 4,
+    spec_version: 5,
     impl_version: 1,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 1,
diff --git a/xpallets/mining/staking/src/lib.rs b/xpallets/mining/staking/src/lib.rs
index c9a232ed1..51bba506f 100644
--- a/xpallets/mining/staking/src/lib.rs
+++ b/xpallets/mining/staking/src/lib.rs
@@ -348,7 +348,10 @@ decl_module! {
 
             ensure!(!value.is_zero(), Error::<T>::ZeroBalance);
             ensure!(Self::is_validator(&target), Error::<T>::NotValidator);
-            ensure!(value <= Self::free_balance(&sender), Error::<T>::InsufficientBalance);
+            ensure!(
+                value + Self::total_locked_of(&sender) <= Self::free_balance(&sender),
+                Error::<T>::InsufficientBalance
+            );
             if !Self::is_validator_bonding_itself(&sender, &target) {
                 Self::check_validator_acceptable_votes_limit(&target, value)?;
             }
@@ -540,12 +543,19 @@ decl_module! {
             Locks::<T>::mutate(&who, |locks| {
                 locks.remove(&LockedType::BondedWithdrawal);
             });
-            for (target, _) in Nominations::<T>::iter_prefix(&who) {
-                Nominations::<T>::mutate(&who, &target, |nominator| {
-                    nominator.unbonded_chunks.clear();
+            Self::purge_unlockings(&who);
+            Self::deposit_event(Event::<T>::ForceAllWithdrawn(who));
+        }
+
+        #[weight = 10_000_000]
+        fn force_reset_staking_lock(origin, accounts: Vec<T::AccountId>) {
+            for who in accounts.iter() {
+                Locks::<T>::mutate(who, |locks| {
+                    locks.remove(&LockedType::BondedWithdrawal);
+                    Self::purge_unlockings(who);
+                    Self::set_lock(who, *locks.entry(LockedType::Bonded).or_default());
                 });
             }
-            Self::deposit_event(Event::<T>::ForceAllWithdrawn(who));
         }
     }
 }
@@ -614,7 +624,7 @@ impl<T: Trait> Module<T> {
                     Self::free_balance(who) >= *self_bonded,
                     "Validator does not have enough balance to bond."
                 );
-                Self::bond_reserve(who, *self_bonded)?;
+                Self::bond_reserve(who, *self_bonded);
                 Nominations::<T>::mutate(who, who, |nominator| {
                     nominator.nomination = *self_bonded;
                 });
@@ -638,7 +648,7 @@ impl<T: Trait> Module<T> {
         value: BalanceOf<T>,
     ) -> DispatchResult {
         if !value.is_zero() {
-            Self::bond_reserve(sender, value)?;
+            Self::bond_reserve(sender, value);
             Nominations::<T>::mutate(sender, target, |nominator| {
                 nominator.nomination = value;
             });
@@ -777,6 +787,14 @@ impl<T: Trait> Module<T> {
         T::Currency::set_lock(STAKING_ID, who, new_locked, WithdrawReasons::all());
     }
 
+    fn purge_unlockings(who: &T::AccountId) {
+        for (target, _) in Nominations::<T>::iter_prefix(who) {
+            Nominations::<T>::mutate(&who, &target, |nominator| {
+                nominator.unbonded_chunks.clear();
+            });
+        }
+    }
+
     /// Returns an iterator of tuple (active_validator, total_votes_of_this_validator).
     ///
     /// Only these active validators are able to be rewarded on each new session,
@@ -882,24 +900,15 @@ impl<T: Trait> Module<T> {
     }
 
     /// Set a lock on `value` of free balance of an account.
-    pub(crate) fn bond_reserve(who: &T::AccountId, value: BalanceOf<T>) -> DispatchResult {
-        let mut new_locks = Self::locks(who);
-        let old_bonded = *new_locks.entry(LockedType::Bonded).or_default();
-        let new_bonded = old_bonded + value;
-
-        ensure!(
-            Self::free_balance(who) >= new_bonded,
-            Error::<T>::InsufficientBalance
-        );
-
-        new_locks.insert(LockedType::Bonded, new_bonded);
-        let staking_locked = new_locks
-            .values()
-            .fold(Zero::zero(), |acc: BalanceOf<T>, x| acc + *x);
-        Self::set_lock(who, staking_locked);
-        Locks::<T>::insert(who, new_locks);
+    pub(crate) fn bond_reserve(who: &T::AccountId, value: BalanceOf<T>) {
+        Locks::<T>::mutate(who, |locks| {
+            *locks.entry(LockedType::Bonded).or_default() += value;
 
-        Ok(())
+            let staking_locked = locks
+                .values()
+                .fold(Zero::zero(), |acc: BalanceOf<T>, x| acc + *x);
+            Self::set_lock(who, staking_locked);
+        });
     }
 
     fn can_unbond(
@@ -981,7 +990,7 @@ impl<T: Trait> Module<T> {
         nominee: &T::AccountId,
         value: BalanceOf<T>,
     ) -> DispatchResult {
-        Self::bond_reserve(nominator, value)?;
+        Self::bond_reserve(nominator, value);
         Self::update_vote_weight(nominator, nominee, Delta::Add(value));
         Self::deposit_event(Event::<T>::Bonded(
             nominator.clone(),


Test patch:
diff --git a/xpallets/mining/staking/src/tests.rs b/xpallets/mining/staking/src/tests.rs
index 2a140ac5e..9ef28b199 100644
--- a/xpallets/mining/staking/src/tests.rs
+++ b/xpallets/mining/staking/src/tests.rs
@@ -167,6 +167,7 @@ fn bond_should_work() {
         assert_ok!(t_bond(1, 3, 1));
 
         assert_bonded_locks(1, 13);
+        // https://github.com/chainx-org/ChainX/issues/402
         assert_eq!(
             frame_system::Account::<Test>::get(&1).data,
             pallet_balances::AccountData {
@@ -179,6 +180,66 @@ fn bond_should_work() {
     });
 }
 
+#[test]
+fn total_staking_locked_no_more_than_free_balance_should_work() {
+    ExtBuilder::default().build_and_execute(|| {
+        assert_ok!(t_bond(1, 2, 80));
+        assert_eq!(
+            Locks::<Test>::get(&1),
+            vec![(LockedType::Bonded, 90)].into_iter().collect()
+        );
+        assert_eq!(
+            frame_system::Account::<Test>::get(&1).data,
+            pallet_balances::AccountData {
+                free: 100,
+                reserved: 0,
+                misc_frozen: 90,
+                fee_frozen: 90,
+            }
+        );
+        assert_eq!(Balances::usable_balance(&1), 10);
+
+        assert_ok!(t_unbond(1, 2, 80));
+        assert_eq!(
+            Locks::<Test>::get(&1),
+            vec![(LockedType::Bonded, 10), (LockedType::BondedWithdrawal, 80)]
+                .into_iter()
+                .collect()
+        );
+        assert_eq!(
+            frame_system::Account::<Test>::get(&1).data,
+            pallet_balances::AccountData {
+                free: 100,
+                reserved: 0,
+                misc_frozen: 90,
+                fee_frozen: 90,
+            }
+        );
+        assert_eq!(Balances::usable_balance(&1), 10);
+
+        // Total locked balance in Staking can not be more than current _usable_ balance.
+        assert_err!(t_bond(1, 2, 80), Error::<Test>::InsufficientBalance);
+        assert_err!(t_bond(1, 2, 11), Error::<Test>::InsufficientBalance);
+        assert_ok!(t_bond(1, 2, 10));
+        assert_eq!(
+            Locks::<Test>::get(&1),
+            vec![(LockedType::Bonded, 20), (LockedType::BondedWithdrawal, 80)]
+                .into_iter()
+                .collect()
+        );
+        assert_eq!(
+            frame_system::Account::<Test>::get(&1).data,
+            pallet_balances::AccountData {
+                free: 100,
+                reserved: 0,
+                misc_frozen: 90 + 10,
+                fee_frozen: 90 + 10,
+            }
+        );
+        assert_eq!(Balances::usable_balance(&1), 0);
+    });
+}
+
 #[test]
 fn unbond_should_work() {
     ExtBuilder::default().build_and_execute(|| {
@@ -650,11 +711,12 @@ fn balances_reserve_should_work() {
         assert_eq!(Balances::free_balance(&who), 10);
 
         // Bond 6
-        assert_ok!(XStaking::bond_reserve(&who, 6));
+        XStaking::bond_reserve(&who, 6);
         assert_eq!(Balances::usable_balance(&who), 4);
-        let mut locks = BTreeMap::new();
-        locks.insert(LockedType::Bonded, 6);
-        assert_eq!(XStaking::locks(&who), locks);
+        assert_eq!(
+            XStaking::locks(&who),
+            vec![(LockedType::Bonded, 6)].into_iter().collect()
+        );
         assert_eq!(
             frame_system::Account::<Test>::get(&who).data,
             pallet_balances::AccountData {
@@ -670,9 +732,11 @@ fn balances_reserve_should_work() {
         );
 
         // Bond 2 extra
-        assert_ok!(XStaking::bond_reserve(&who, 2));
-        let mut locks = BTreeMap::new();
-        locks.insert(LockedType::Bonded, 8);
+        XStaking::bond_reserve(&who, 2);
+        assert_eq!(
+            XStaking::locks(&who),
+            vec![(LockedType::Bonded, 6 + 2)].into_iter().collect()
+        );
         assert_eq!(
             frame_system::Account::<Test>::get(&who).data,
             pallet_balances::AccountData {
@@ -682,56 +746,60 @@ fn balances_reserve_should_work() {
                 fee_frozen: 8
             }
         );
-        assert_err!(
-            XStaking::bond_reserve(&who, 3),
-            <Error<Test>>::InsufficientBalance
-        );
+
+        // Bond 3 extra
+        XStaking::bond_reserve(&who, 3);
 
         // Unbond 5 now, the frozen balances stay the same,
         // only internal Staking locked state changes.
         assert_ok!(XStaking::unbond_reserve(&who, 5));
-        let mut locks = BTreeMap::new();
-        locks.insert(LockedType::Bonded, 3);
-        locks.insert(LockedType::BondedWithdrawal, 5);
-        assert_eq!(XStaking::locks(&who), locks);
+        assert_eq!(
+            XStaking::locks(&who),
+            vec![(LockedType::Bonded, 6), (LockedType::BondedWithdrawal, 5)]
+                .into_iter()
+                .collect()
+        );
         assert_eq!(
             frame_system::Account::<Test>::get(&who).data,
             pallet_balances::AccountData {
                 free: 10,
                 reserved: 0,
-                misc_frozen: 8,
-                fee_frozen: 8
+                misc_frozen: 11,
+                fee_frozen: 11
             }
         );
 
         // Unlock unbonded withdrawal 4.
         XStaking::apply_unlock_unbonded_withdrawal(&who, 4);
-        let mut locks = BTreeMap::new();
-        locks.insert(LockedType::Bonded, 3);
-        locks.insert(LockedType::BondedWithdrawal, 1);
-        assert_eq!(XStaking::locks(&who), locks);
+        assert_eq!(
+            XStaking::locks(&who),
+            vec![(LockedType::Bonded, 6), (LockedType::BondedWithdrawal, 1)]
+                .into_iter()
+                .collect()
+        );
         assert_eq!(
             frame_system::Account::<Test>::get(&who).data,
             pallet_balances::AccountData {
                 free: 10,
                 reserved: 0,
-                misc_frozen: 4,
-                fee_frozen: 4
+                misc_frozen: 11 - 4,
+                fee_frozen: 11 - 4
             }
         );
 
         // Unlock unbonded withdrawal 1.
         XStaking::apply_unlock_unbonded_withdrawal(&who, 1);
-        let mut locks = BTreeMap::new();
-        locks.insert(LockedType::Bonded, 3);
-        assert_eq!(XStaking::locks(&who), locks);
+        assert_eq!(
+            XStaking::locks(&who),
+            vec![(LockedType::Bonded, 6)].into_iter().collect()
+        );
         assert_eq!(
             frame_system::Account::<Test>::get(&who).data,
             pallet_balances::AccountData {
                 free: 10,
                 reserved: 0,
-                misc_frozen: 3,
-                fee_frozen: 3
+                misc_frozen: 11 - 4 - 1,
+                fee_frozen: 11 - 4 - 1
             }
         );
     });