Reap pool member whose balance goes below ED #5769

eagr · 2024-09-19T10:36:22Z

TODO

reap delegator & staked balance
benchmark -> weight function?
tests

New to the pallet code. I'd appreciate it if you could take a look to see if it's on the right direction before I do the bench and tests @Ank4n

PieWol · 2024-09-19T11:01:49Z

substrate/frame/nomination-pools/src/lib.rs

+				}
+			}
+
+			Ok(())


This is basically a service for the greater good of the chain. If the account has been successfully reaped it would make sense to return fees at the end.

PieWol · 2024-09-19T12:49:49Z

substrate/frame/nomination-pools/src/lib.rs

+		/// Remove the member if stake falls below the existential deposit.
+		#[pallet::call_index(26)]
+		// FIXME weight functions
+		#[pallet::weight(T::WeightInfo::reap_member_below_ed())]


you could just put a constant value in here before you create the benchmarks. then the compiler won't complain.

Ank4n · 2024-09-19T18:53:12Z

substrate/frame/nomination-pools/src/lib.rs

+		let pool = Pool::from(bonded_pool.bonded_account());
+
+		// Ensure any dangling delegation is withdrawn.
+		let dangling_withdrawal = match T::StakeAdapter::member_delegation_balance(member.clone()) {


The general approach looks fine to me. Good next step would be to write an integration test (delegate-stake and transfer stake) to ensure the stake is updated and storages are cleaned as expected.

eagr · 2024-10-05T16:02:48Z

What should happen If the active balance of the pool creator falls below ED? To be aligned with the logic of withdrawal, I suppose if it's removed, the pool should be destroyed?

Is it appropriate to make active_balance() public like total_balance()?

Given 3 members with 15/10/5 deposit and a 15 slash, they will be slashed by 7.5/5/2.5 in proportion to their stake, rounded up to 8/5/3 respectively. Sounds right?

/// This might be lower but never higher than the sum of `total_balance` of all [`PoolMembers`]
/// because calling `pool_withdraw_unbonded` might decrease the total stake of the pool's
/// `bonded_account` without adjusting the pallet-internal `UnbondingPool`'s.
#[pallet::storage]
pub type TotalValueLocked<T: Config> = StorageValue<_, BalanceOf<T>, ValueQuery>;

The value of TotalValueLocked after the slash is 15, which is higher than the sum of total_balance of all the members (14). Is this a bug or the doc needs updating? Or am I missing something?

PieWol · 2024-10-05T19:38:51Z

Rounding the remainding value does not sound right at all. Where did you get that from?

eagr · 2024-10-06T04:06:37Z

https://github.com/paritytech/polkadot-sdk/pull/5769/files#diff-4848533a01a6a73b5b5167ea823bb99810372a4883ec8559006d46c98d8fe8b0R985-R990

That's what I deducted from the resulted balances. 👆 Now that I think of it, maybe this is specific to the tests coz they are using ints for simpler comparisons. If that's the case then never mind. :))

PieWol · 2024-10-08T10:47:35Z

That's what I deducted from the resulted balances. 👆 Now that I think of it, maybe this is specific to the tests coz they are using ints for simpler comparisons. If that's the case then never mind. :))

I think that using ints might be the reason, but I would need to check the code to see where the actual differences come from. I don't think the doc needs an update.

Ank4n · 2024-10-13T11:54:59Z

substrate/frame/staking/src/pallet/impls.rs

@@ -1880,6 +1881,28 @@ impl<T: Config> StakingInterface for Pallet<T> {
 		}
 	}

+	fn force_withdraw(stash: &Self::AccountId, amount: Self::Balance) -> DispatchResult {
+		let mut ledger = Self::ledger(Stash(stash.clone()))?;
+		ledger.active = ledger.active.saturating_sub(amount);


May be you should look to withdraw first from ledger.unlocking chunks before trying to withdraw from active?

Wouldn't it affect other members who're unbonding funds? Let's say

ED = 100, MIN_BOND = ED A deposits 500 and unbonds 100 B deposits 100 next era, slash happens (so the 100 unlocking fund isn't affected right?) obviously, B will be reaped assuming no one unbonds after the slash if we withdraw first from unlocking, then when the fund is fully unlocked what's left would be less than what A would expect to withdraw (100 - x) right?

Do you mean we withdraw from unlocking and unbond more to make up for the portion withdrawn from the unlocking? Then wouldn't it length the unlock period?

Ank4n · 2024-10-13T11:57:29Z

substrate/frame/staking/src/pallet/impls.rs

+		T::EventListeners::on_withdraw(stash, amount);
+		Self::deposit_event(Event::<T>::Withdrawn { stash: stash.clone(), amount });
+
+		T::Currency::burn(amount);


seems weird to withdraw as well as burn? Shouldn't it be nomination pool to decide what to do with withdrawn funds?

Shouldn't it be nomination pool to decide what to do with withdrawn funds?

Yeah, that makes more sense :))

Ank4n · 2024-10-13T11:59:40Z

substrate/frame/staking/src/pallet/mod.rs

@@ -863,6 +863,8 @@ pub mod pallet {
 		ForceEra { mode: Forcing },
 		/// Report of a controller batch deprecation.
 		ControllerBatchDeprecated { failures: u32 },
+		/// Certain amount of bonded fund has been burned.
+		Burnt { stash: T::AccountId, amount: BalanceOf<T> },


IMO we should just withdraw here, and not burn. Leave the burn (or something else such as transfer these funds to reward pool) logic with the NominationPools.

Ank4n · 2024-10-13T12:01:18Z

substrate/frame/nomination-pools/src/adapter.rs

@@ -183,6 +183,15 @@ pub trait StakeStrategy {
 		num_slashing_spans: u32,
 	) -> DispatchResult;

+	/// Clean up ledger leftovers before reaping a dusted member.
+	// FIXME eagr better name?
+	fn member_dust(


better name?

force_withdraw?

Ank4n · 2024-10-13T12:04:07Z

substrate/frame/nomination-pools/src/lib.rs

+		// FIXME eagr weight functions
+		// #[pallet::weight(T::WeightInfo::reap_member())]
+		#[pallet::weight(1)]
+		pub fn reap_member(


You raised a great question of whether it needs to be an extrinsic and I was convinced that we can do better.
See here.

Oh, I totally misunderstood your comment there. Then I'll try the other route. But we do need to recalibrate the weights of migrate_delegation() and apply_slash() right?

Ank4n · 2024-10-13T12:06:55Z

substrate/frame/nomination-pools/src/lib.rs

+						num_slashing_spans,
+					)?;
+
+					// FIXME eagr is this needed?


Just one MemberReaped event is ideal.

I see. We don't need to emit a withdrawn event, which makes sense. Do you mean we need an extra MemberReaped event besides MemberRemoved?

Ank4n · 2024-10-13T12:09:21Z

substrate/frame/nomination-pools/src/lib.rs

+				);
+
+				if contribution > Zero::zero() {
+					T::StakeAdapter::member_dust(


We still have to update member points and stuff right?

Ank4n · 2024-10-13T12:11:39Z

substrate/frame/nomination-pools/src/lib.rs

+				Self::deposit_event(Event::<T>::MemberRemoved {
+					pool_id,
+					member: member.clone(),
+					// FIXME eagr there should not be any dangling delegation at this point right?


I believe dangling delegation is independent of reap logic. Dangling delegation can occur due to merging of unbonding pools in different eras.

Ank4n · 2024-10-13T12:16:48Z

substrate/frame/nomination-pools/src/lib.rs

+
+			Ok(())
+
+			// FIXME eagr should return fee info?


Really depends on what you want to do.
For permissionless extrinsic (that anyone can call to cleanup chain if some criteria is fulfilled), we want to not charge any fees. So if the call succeeds, we should refund the fees.

See https://docs.substrate.io/build/tx-weights-fees/#post-dispatch-weight-correction.

Though, I believe we might not need this to be an extrinsic anyway?

Ank4n · 2024-10-13T12:19:39Z

substrate/frame/staking/src/pallet/impls.rs

+	fn force_withdraw(stash: &Self::AccountId, amount: Self::Balance) -> DispatchResult {
+		let mut ledger = Self::ledger(Stash(stash.clone()))?;
+		ledger.active = ledger.active.saturating_sub(amount);
+		// FIXME eagr should we check if active >= ED before updating?


You don't strictly need to since saturating_sub would just make it 0. But probably a good defensive measure to add is to ensure force_withdraw can only be done for 0 < amount < ED.

Oh, I mean do we need to

ledger.active = ledger.active.saturating_sub(amount); // check before updating the ledger ensure!(ledger.active >= T::Currency::minimum_balance()); ledger.update()?;

eagr · 2024-10-20T10:20:48Z

The tests are somewhat broken (unrelated to the changes here). You could run this in /substrate/frame/nomination-pools/test-delegate-stake and watch it crash.

#[test]
fn reap_member_delegate() {
	new_test_ext().execute_with(|| {
		assert_eq!(Balances::minimum_balance(), 5);
		assert_eq!(Staking::current_era(), None);

		assert_ok!(Pools::create(RuntimeOrigin::signed(10), 15, 10, 10, 10));
		assert_eq!(LastPoolId::<Runtime>::get(), 1);

		assert_ok!(Pools::join(RuntimeOrigin::signed(20), 10, 1));
		assert_ok!(Pools::join(RuntimeOrigin::signed(21), 5, 1));

		CurrentEra::<Runtime>::set(Some(1));

		// slash era 1
		pallet_staking::slashing::do_slash::<Runtime>(
			&POOL1_BONDED,
			15,
			&mut Default::default(),
			&mut Default::default(),
			1,
		);

		CurrentEra::<Runtime>::set(Some(5));

		// >ED after slash
		assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().total_balance(), 7);
		// =ED after slash
		assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().total_balance(), 5);
		// <ED after slash, will be reaped
		assert_eq!(PoolMembers::<Runtime>::get(21).unwrap().total_balance(), 2);

		assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(21), 10));
		assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(21), 20));

		// here it crashes
		assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(21), 21));
	})
}

Defensive failure has been triggered!
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14
   2: core::panicking::panic_display
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:264:5
   3: <core::option::Option<T> as frame_support::traits::misc::Defensive<T>>::defensive_unwrap_or_else::panic_cold_display
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panic.rs:101:13
   4: <core::option::Option<T> as frame_support::traits::misc::Defensive<T>>::defensive_unwrap_or_else
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/support/src/traits/misc.rs:74:3
   5: <T as frame_support::traits::misc::DefensiveSaturating>::defensive_saturating_sub
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/support/src/traits/misc.rs:417:3
   6: pallet_delegated_staking::types::AgentLedgerOuter<T>::remove_slash
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/delegated-staking/src/types.rs:248:23
   7: pallet_delegated_staking::<impl pallet_delegated_staking::pallet::Pallet<T>>::do_slash
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/delegated-staking/src/lib.rs:741:3
   8: pallet_delegated_staking::impls::<impl sp_staking::DelegationInterface for pallet_delegated_staking::pallet::Pallet<T>>::delegator_slash
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/delegated-staking/src/impls.rs:99:3
   9: <pallet_nomination_pools::adapter::DelegateStake<T,Staking,Delegation> as pallet_nomination_pools::adapter::StakeStrategy>::member_slash
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/nomination-pools/src/adapter.rs:443:3
  10: <pallet_nomination_pools_test_delegate_stake::mock::MockAdapter as pallet_nomination_pools::adapter::StakeStrategy>::member_slash
             at ./src/mock.rs:229:3
  11: pallet_nomination_pools::<impl pallet_nomination_pools::pallet::Pallet<T>>::do_apply_slash
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/nomination-pools/src/lib.rs:3586:3
  12: pallet_nomination_pools::pallet::Pallet<T>::apply_slash::{{closure}}
             at /home/julian/proj/eagr/rs/polkadot-sdk/substrate/frame/nomination-pools/src/lib.rs:2992:4

which seems to come from

polkadot-sdk/substrate/frame/delegated-staking/src/types.rs

Line 248 in 5e0843e

let pending_slash = self.ledger.pending_slash.defensive_saturating_sub(amount);

I suspect it's also related to ints being used for testing.

reap pool member below ed

7df5cf5

eagr marked this pull request as draft September 19, 2024 10:41

PieWol reviewed Sep 19, 2024

View reviewed changes

Ank4n reviewed Sep 19, 2024

View reviewed changes

failing basline e2e tests

e7f5039

eagr added 2 commits October 8, 2024 11:52

clean up ledgers before reaping member

05b7efa

glitches

d6d1cec

eagr requested review from Ank4n and PieWol October 8, 2024 08:28

Ank4n reviewed Oct 13, 2024

View reviewed changes

fixes

712c15b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reap pool member whose balance goes below ED #5769

Reap pool member whose balance goes below ED #5769

eagr commented Sep 19, 2024

PieWol Sep 19, 2024

PieWol Sep 19, 2024

Ank4n Sep 19, 2024

eagr commented Oct 5, 2024

PieWol commented Oct 5, 2024

eagr commented Oct 6, 2024

PieWol commented Oct 8, 2024

Ank4n Oct 13, 2024

eagr Oct 15, 2024

Ank4n Oct 13, 2024

eagr Oct 15, 2024

Ank4n Oct 13, 2024

Ank4n Oct 13, 2024

Ank4n Oct 13, 2024

eagr Oct 15, 2024 •

edited

Loading

Ank4n Oct 13, 2024

eagr Oct 15, 2024 •

edited

Loading

Ank4n Oct 13, 2024

Ank4n Oct 13, 2024

Ank4n Oct 13, 2024

Ank4n Oct 13, 2024

eagr Oct 16, 2024

eagr commented Oct 20, 2024

+              				}
+              			}
+              			Ok(())

Reap pool member whose balance goes below ED #5769

Are you sure you want to change the base?

Reap pool member whose balance goes below ED #5769

Conversation

eagr commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagr commented Oct 5, 2024

PieWol commented Oct 5, 2024

eagr commented Oct 6, 2024

PieWol commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagr Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagr Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagr commented Oct 20, 2024

eagr Oct 15, 2024 •

edited

Loading

eagr Oct 15, 2024 •

edited

Loading