Full Unbond in Staking #3811

dharjeezy · 2024-03-24T14:56:49Z

closes #414

Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW

test and benchmark

dharjeezy · 2024-03-24T15:22:21Z

@Ank4n according to @kianenigma we want to fully unbond everything after chilling. Therefore, the extrinsic don't need to take any value, also there will be no need to add a check for minimum active bond which you suggested earlier here. #3629 (comment)

Also, if we want to do a chill also when we expect a value, we can definitely do that in the unbond extrinsic. should i go ahead and include that?

Ank4n · 2024-03-27T18:57:02Z

@kianenigma wdyt about call::unbond doing chill implicitly if it is a full unbond (unbond value == ledger.total)?

kianenigma · 2024-03-31T09:41:36Z

@kianenigma wdyt about call::unbond doing chill implicitly if it is a full unbond (unbond value == ledger.total)?

This is also fine with me, no strong opinion. I like your idea better as it improves UX without wallets needing to do much.

dharjeezy · 2024-03-31T13:15:09Z

OK. So, should we still keep the full unbond extrinsic and also implement the implicit one that @Ank4n is suggesting?

# Conflicts: # polkadot/runtime/westend/src/weights/pallet_staking.rs # substrate/frame/staking/src/benchmarking.rs # substrate/frame/staking/src/pallet/impls.rs # substrate/frame/staking/src/pallet/mod.rs # substrate/frame/staking/src/weights.rs

# Conflicts: # Cargo.lock

dharjeezy · 2024-04-10T12:15:36Z

@Ank4n @kianenigma
I have implicitly added the chill for unbonding

please help review again

…re unbonding all amount

kianenigma

Looks good, needs more tests (for unbond fully calling chill) and better prdoc and bench etc.

Ank4n

@dharjeezy Sorry for the delay in reviews. Had been on vacation.

Suggested some nits + there are CI errors. Could you please fix it and ping me. I will try to get this merged promptly.

Ank4n · 2024-08-04T09:54:24Z

substrate/frame/staking/src/benchmarking.rs

+		assert!(new_bonded == BalanceOf::<T>::try_from(0u128).map_err(|_| "balance expected to be a u128")
+			.unwrap());


This should work.

Suggested change

assert!(new_bonded == BalanceOf::<T>::try_from(0u128).map_err(|_| "balance expected to be a u128")

.unwrap());

assert!(new_bonded == Zero::zero());

Ank4n · 2024-08-04T09:54:49Z

substrate/frame/staking/src/ledger.rs

@@ -36,6 +36,7 @@ use frame_support::{
 	traits::{Defensive, LockableCurrency},
 };
 use sp_staking::{StakingAccount, StakingInterface};
+use sp_std::prelude::*;


Do we need this?

Ank4n · 2024-08-04T10:01:09Z

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


-				Self::deposit_event(Event::<T>::Unbonded { stash, amount: value });
+			if value == ledger.total {


Because of this, you may want to check value >= ledger.total instead.

Alternatively, you can move the ceiling logic let mut value = value.min(ledger.active) to this function.

Ank4n · 2024-08-13T11:53:38Z

bot bench polkadot-pallet --pallet=pallet_staking --runtime=westend

command-bot · 2024-08-13T11:53:43Z

@Ank4n https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6988164 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_staking. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 10-0373acde-d692-46d8-a50e-545384f7ad2c to cancel this command or bot cancel to cancel all commands in this pull request.

Ank4n · 2024-08-13T11:56:32Z

substrate/frame/staking/src/benchmarking.rs

@@ -274,16 +274,15 @@ benchmarks! {

 		let stash = scenario.origin_stash1.clone();
 		let controller = scenario.origin_controller1.clone();
-		let amount = origin_weight - scenario.dest_weight;


Would this impact the worst case weight calculation for updating bags list?

wrt https://github.com/paritytech/polkadot-sdk/pull/3811/files?diff=split&w=0#r1717563991, an alternate way would be to keep this existing benchmark and add the chill weight to it for worst case weight of unbond.

…=westend --target_dir=polkadot --pallet=pallet_staking

command-bot · 2024-08-13T13:14:40Z

@Ank4n Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_staking has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6988164 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6988164/artifacts/download.

Ank4n · 2024-08-14T21:17:24Z

polkadot/runtime/westend/src/weights/pallet_staking.rs

 	fn unbond() -> Weight {
 		// Proof Size summary in bytes:
-		//  Measured:  `2128`


Seems the unbond benchmark is not correct. You would expect weight to increase with the extra chill in the worst case but both proof size and execution time decreases.

gpestana · 2024-08-22T14:07:17Z

prdoc/pr_3811.prdoc

+      if the value to be unbonded is the total of what is bonded
+
+crates:
+  - name: pallet-staking


needs the bump field here, I'd suggest minor.

gpestana · 2024-08-22T15:14:41Z

substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs

+		// unbonding total of active stake passes because Chill occurs implicitly when unbonding
+		// full amount


Suggested change

// unbonding total of active stake passes because Chill occurs implicitly when unbonding

// full amount

// unbonding total of active stake passes because chill occurs implicitly when unbonding

// full amount.

gpestana · 2024-08-22T15:16:49Z

prdoc/pr_3811.prdoc

@@ -0,0 +1,10 @@
+title: "Chill and Full Unbond in [pallet_staking]"


Suggested change

title: "Chill and Full Unbond in [pallet_staking]"

title: "Implicit chill when full unbounding in [pallet_staking]"

gpestana · 2024-08-22T15:18:47Z

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


-				Self::deposit_event(Event::<T>::Unbonded { stash, amount: value });
+			if value >= ledger.total {
+				Self::chill_stash(&ledger.stash);


The weight returned by this call should also be used in the PostInfo, i.e. as part of the actual_weight calculation.

There is no weight returned by that call, that function does not return anything.

i understand you now, i have updated this. @gpestana

gpestana · 2024-08-22T15:19:07Z

substrate/frame/staking/src/tests.rs

@@ -4133,6 +4133,49 @@ fn test_multi_page_payout_stakers_by_page() {
 	});
 }

+#[test]
+fn unbond_with_chill_works() {
+	//


Suggested change

//

gpestana · 2024-08-22T15:19:17Z

substrate/frame/staking/src/tests.rs

+#[test]
+fn unbond_with_chill_works() {
+	//
+	// * Should test


Suggested change

// * Should test

// Should test:

gpestana · 2024-08-22T15:20:31Z

substrate/frame/staking/src/tests.rs

@@ -7995,7 +8038,7 @@ mod ledger_recovery {
 			assert_eq!(Balances::balance_locked(crate::STAKING_ID, &333), lock_333_before); // OK
 			assert_eq!(Bonded::<Test>::get(&333), Some(444)); // OK
 			assert!(Payee::<Test>::get(&333).is_some()); // OK
-											 // however, ledger associated with its controller was killed.
+												// however, ledger associated with its controller was killed.


Suggested change

// however, ledger associated with its controller was killed.

// however, ledger associated with its controller was killed.

for some reason the fmt is not working here (noticed that elsewhere). Better to add a break line and it should be fine.

gpestana · 2024-08-22T15:22:45Z

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

@@ -1161,6 +1162,91 @@ impl<T: Config> Pallet<T> {
 	) -> Exposure<T::AccountId, BalanceOf<T>> {
 		EraInfo::<T>::get_full_exposure(era, account)
 	}
+
+	/// Whether `who` is a virtual staker whose funds are managed by another pallet.
+	pub(crate) fn is_virtual_staker(who: &T::AccountId) -> bool {


This fn seems unrelated to the logic to be implemented?

did not touch that actually

Ank4n

Looking good. I am happy to approve once you fix the default pallet::weight for fn unbond. Few other suggestions but those are just nits.

Ank4n · 2024-09-18T12:02:05Z

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

@@ -1094,86 +1094,21 @@ pub mod pallet {
 			#[pallet::compact] value: BalanceOf<T>,
 		) -> DispatchResultWithPostInfo {
 			let controller = ensure_signed(origin)?;
-			let unlocking =


github doesn't allow me to add comment on the non changed line 1090 so adding here.

Add chill weight to the call so in worst case it always adds chill weight.

#[pallet::call_index(2)] #[pallet::weight( T::WeightInfo::withdraw_unbonded_kill(SPECULATIVE_NUM_SPANS).saturating_add(T::WeightInfo::unbond()) .saturating_add(T::WeightInfo::Chill())) ]

Additional info: Before any transaction this fund is reserved, and any difference in the PostInfo is refunded. So this should always contain the worst case weight.

Ank4n · 2024-09-18T12:04:22Z

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

-				Self::deposit_event(Event::<T>::Unbonded { stash, amount: value });
+			if value >= ledger.total {
+				Self::chill_stash(&ledger.stash);
+				total_weight = total_weight.saturating_add(T::WeightInfo::chill());


nit

Suggested change

total_weight = total_weight.saturating_add(T::WeightInfo::chill());

total_weight.saturating_accrue(T::WeightInfo::chill());

Ank4n · 2024-09-18T12:08:23Z

substrate/frame/staking/src/tests.rs

+		assert_eq!(*staking_events().last().unwrap(), Event::Unbonded { stash: 11, amount: 1000 });
+		assert_eq!(
+			*staking_events().get(staking_events().len() - 2).unwrap(),
+			Event::Chilled { stash: 11 }
+		);
+	})


nit: would be lot nicer to assert against all events (see this) or do a match (see this)

dharjeezy added 5 commits March 24, 2024 15:54

force unbond in staking

c6e4c93

check to see if controller wants to fully unbond

12e62a9

test and benchmark

comment

345b2fa

unimplemented force_unbond

0b8ef9e

full unbond instead

df5c186

dharjeezy changed the title ~~Dami/full unbond~~ Full Unbond in Staking Mar 24, 2024

dharjeezy mentioned this pull request Mar 24, 2024

force unbond in staking #3629

Closed

dharjeezy requested review from athei, acatangiu, andresilva, sorpaas, cheme, a team and koute as code owners April 9, 2024 09:13

paritytech-review-bot bot requested a review from a team April 9, 2024 09:14

dharjeezy force-pushed the dami/full_unbond branch from f6032b7 to df5c186 Compare April 9, 2024 09:17

dharjeezy added 4 commits April 9, 2024 16:13

implicit chill when unbonding

66d9b61

fmt

0292c5f

Merge remote-tracking branch 'origin/master' into dami/full_unbond

9db89f3

# Conflicts: # Cargo.lock

no need for outright chill since unbond does chilling implicitly befo…

4d449bf

…re unbonding all amount

kianenigma reviewed Apr 11, 2024

View reviewed changes

Ank4n reviewed Aug 4, 2024

View reviewed changes

dharjeezy added 3 commits August 11, 2024 21:21

Merge remote-tracking branch 'origin/master' into dami/full_unbond

215ae86

corrections based on review

52ad45f

fmt

bf9680a

dharjeezy requested a review from Ank4n August 12, 2024 04:53

Merge branch 'master' into dami/full_unbond

11d1ddf

Ank4n added the T2-pallets This PR/Issue is related to a particular pallet. label Aug 12, 2024

Merge branch 'master' into dami/full_unbond

96667b1

Ank4n reviewed Aug 13, 2024

View reviewed changes

".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…

14212e8

…=westend --target_dir=polkadot --pallet=pallet_staking

Ank4n reviewed Aug 14, 2024

View reviewed changes

gpestana reviewed Aug 22, 2024

View reviewed changes

dharjeezy and others added 9 commits August 25, 2024 08:03

nit

f19177f

Merge branch 'master' into dami/full_unbond

945f69d

add chill weight to unbond

aca4c8f

Merge remote-tracking branch 'origin/master' into dami/full_unbond

36d8564

revert unsolicited changes

38705e6

revert benchmarks according to Ankan

95e61d0

Merge branch 'master' into dami/full_unbond

30015c7

revert

4536363

fmt

06e8563

Ank4n reviewed Sep 18, 2024

View reviewed changes

Ank4n requested a review from gpestana September 18, 2024 12:14

dharjeezy added 2 commits October 3, 2024 18:48

code review changes

2fc715a

fmt

766458c

dharjeezy requested a review from Ank4n October 3, 2024 17:49

Merge branch 'master' into dami/full_unbond

16f6535

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full Unbond in Staking #3811

Full Unbond in Staking #3811

dharjeezy commented Mar 24, 2024 •

edited

Loading

dharjeezy commented Mar 24, 2024

Ank4n commented Mar 27, 2024

kianenigma commented Mar 31, 2024

dharjeezy commented Mar 31, 2024

dharjeezy commented Apr 10, 2024 •

edited

Loading

kianenigma left a comment

Ank4n left a comment

Ank4n Aug 4, 2024

Ank4n Aug 4, 2024

Ank4n Aug 4, 2024

Ank4n commented Aug 13, 2024

command-bot bot commented Aug 13, 2024 •

edited

Loading

Ank4n Aug 13, 2024

Ank4n Aug 14, 2024

command-bot bot commented Aug 13, 2024

Ank4n Aug 14, 2024 •

edited

Loading

gpestana Aug 22, 2024

gpestana Aug 22, 2024

gpestana Aug 22, 2024

gpestana Aug 22, 2024

dharjeezy Aug 25, 2024 •

edited

Loading

dharjeezy Sep 14, 2024 •

edited

Loading

gpestana Aug 22, 2024

gpestana Aug 22, 2024

gpestana Aug 22, 2024

gpestana Aug 22, 2024

gpestana Aug 22, 2024

dharjeezy Aug 25, 2024

Ank4n left a comment

Ank4n Sep 18, 2024

Ank4n Sep 18, 2024

Ank4n Sep 18, 2024

		assert!(new_bonded == BalanceOf::<T>::try_from(0u128).map_err(\|_\| "balance expected to be a u128")
		.unwrap());

	assert!(new_bonded == BalanceOf::<T>::try_from(0u128).map_err(\|_\| "balance expected to be a u128")
	.unwrap());
	assert!(new_bonded == Zero::zero());


		Self::deposit_event(Event::<T>::Unbonded { stash, amount: value });
		if value == ledger.total {

		// unbonding total of active stake passes because Chill occurs implicitly when unbonding
		// full amount

		@@ -0,0 +1,10 @@
		title: "Chill and Full Unbond in [pallet_staking]"

	title: "Chill and Full Unbond in [pallet_staking]"
	title: "Implicit chill when full unbounding in [pallet_staking]"

	// however, ledger associated with its controller was killed.

	// however, ledger associated with its controller was killed.

	total_weight = total_weight.saturating_add(T::WeightInfo::chill());
	total_weight.saturating_accrue(T::WeightInfo::chill());

Full Unbond in Staking #3811

Are you sure you want to change the base?

Full Unbond in Staking #3811

Conversation

dharjeezy commented Mar 24, 2024 • edited Loading

dharjeezy commented Mar 24, 2024

Ank4n commented Mar 27, 2024

kianenigma commented Mar 31, 2024

dharjeezy commented Mar 31, 2024

dharjeezy commented Apr 10, 2024 • edited Loading

kianenigma left a comment

Choose a reason for hiding this comment

Ank4n left a 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

Ank4n commented Aug 13, 2024

command-bot bot commented Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

command-bot bot commented Aug 13, 2024

Ank4n Aug 14, 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

dharjeezy Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

dharjeezy Sep 14, 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

Choose a reason for hiding this comment

Ank4n left a 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

dharjeezy commented Mar 24, 2024 •

edited

Loading

dharjeezy commented Apr 10, 2024 •

edited

Loading

command-bot bot commented Aug 13, 2024 •

edited

Loading

Ank4n Aug 14, 2024 •

edited

Loading

dharjeezy Aug 25, 2024 •

edited

Loading

dharjeezy Sep 14, 2024 •

edited

Loading