used MaxUnlockingChunks from config #12289

MrishoLukamba · 2022-09-18T10:18:45Z

@kianenigma @ggwpez
Fixes #12227
Polkadot address: 133WyzJ4cEEbQ4twhT3nQpoXYeTCzeKn8kWfRfEi4XDF7wKh

MrishoLukamba · 2022-09-18T10:20:50Z

Solved issue #12227

kianenigma · 2022-09-19T09:04:33Z

Please follow this syntax to close the linked issue https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

kianenigma

You should document in the config type parameter that if this value is ever set, and then decreased, it leads to a corrupt state and a migration is needed. We also need a test to demonstrate this.

@Ank4n please note that we have the same story with MaxNominations as well.

MrishoLukamba · 2022-09-19T13:04:07Z

closes #12227

Ank4n · 2022-09-20T08:47:14Z

@MrishoLukamba Can you also add a test that documents what happens in the scenario where MaxUnlockingChunks is reduced without migration?
Here are couple of examples you can refer
https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L4872
https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L5367

MrishoLukamba · 2022-09-20T08:56:00Z

@MrishoLukamba Can you also add a test that documents what happens in the scenario where MaxUnlockingChunks is reduced without migration? Here are couple of examples you can refer https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L4872 https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L5367

Okey let me get into it, if anything I will ask

MrishoLukamba · 2022-09-22T20:26:52Z

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful,
I think is a bug.

Ank4n · 2022-09-22T21:04:13Z

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

Can you elaborate more? May be refer in the code.

I see that when we unbond, a new UnlockChunk is pushed (or UnlockChunk for same era updated).
https://github.com/paritytech/substrate/blob/master/frame/staking/src/pallet/mod.rs#L984

MrishoLukamba · 2022-09-23T08:55:15Z

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

Can you elaborate more? May be refer in the code.

I see that when we unbond, a new UnlockChunk is pushed (or UnlockChunk for same era updated). https://github.com/paritytech/substrate/blob/master/frame/staking/src/pallet/mod.rs#L984

In the code it shows that,a new UnlockChunk is being pushed but in testing it is not pushed it is just updated the value field. So the bound MaxUnlockChunk placed there is not used at all. You can see in the new test i've added

Ank4n · 2022-09-23T11:17:54Z

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

Can you elaborate more? May be refer in the code.
I see that when we unbond, a new UnlockChunk is pushed (or UnlockChunk for same era updated). https://github.com/paritytech/substrate/blob/master/frame/staking/src/pallet/mod.rs#L984

In the code it shows that,a new UnlockChunk is being pushed but in testing it is not pushed it is just updated the value field. So the bound MaxUnlockChunk placed there is not used at all. You can see in the new test i've added

@MrishoLukamba The behaviour looks correct to me. An UnlockChunk is only updated if multiple unbond happen in the same era. If you try to move forward in eras and unbond, you should see multiple chunks being added. If the UnlockChunks are already full, the transaction should return an error NoMoreChunks as the boundedVec is full.

@kianenigma Shouldn't we be able to keep the MaxUnlockChunk equal to BondingDuration. This will imply users can have UnlockChunks without claiming only for up to 28 eras. And in that case all the UnlockChunks are ready to be claimed (after 28 eras) so encouraging the user to just claim existing unlocks before requesting for more unlocks.
If you agree with above, we can bound StakingLedger.unlocking with BondingDuration and not have to introduce a new bound MaxUnlockChunk.

kianenigma · 2022-09-24T08:19:47Z

frame/staking/src/tests.rs

@@ -1346,7 +1346,8 @@ fn too_many_unbond_calls_should_not_work() {
 	ExtBuilder::default().build_and_execute(|| {
 		let mut current_era = 0;
 		// locked at era MaxUnlockingChunks - 1 until 3
-		for i in 0..MaxUnlockingChunks::get() - 1 {
+		//maxunlockingchunks


Suggested change

//maxunlockingchunks

kianenigma · 2022-09-24T08:20:15Z

frame/staking/src/tests.rs

@@ -1346,7 +1346,8 @@ fn too_many_unbond_calls_should_not_work() {
 	ExtBuilder::default().build_and_execute(|| {
 		let mut current_era = 0;
 		// locked at era MaxUnlockingChunks - 1 until 3
-		for i in 0..MaxUnlockingChunks::get() - 1 {
+		//maxunlockingchunks
+		for i in 0..<<Test as Config>::MaxUnlockingChunks as Get::<u32>>::get()- 1 {


Suggested change

for i in 0..<<Test as Config>::MaxUnlockingChunks as Get::<u32>>::get()- 1 {

for i in 0..<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get()-1 {

kianenigma · 2022-09-24T08:20:26Z

frame/staking/src/tests.rs

@@ -1361,7 +1362,8 @@ fn too_many_unbond_calls_should_not_work() {
 		assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
 		assert_eq!(
 			Staking::ledger(&10).unwrap().unlocking.len(),
-			MaxUnlockingChunks::get() as usize
+
+			<<Test as Config>::MaxUnlockingChunks as Get::<u32>>::get() as usize


Suggested change

<<Test as Config>::MaxUnlockingChunks as Get::<u32>>::get() as usize

<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() as usize

kianenigma · 2022-09-24T08:20:39Z

frame/staking/src/tests.rs

+	//Concern is on validators only
+	//By Default 11, 10 are stash and ctrl and 21,20


Suggested change

//Concern is on validators only

//By Default 11, 10 are stash and ctrl and 21,20

// Concern is on validators only

// By Default 11, 10 are stash and ctrl and 21,20

Please fix all comments accordingly.

kianenigma

Logic seems correct, but code needs to be formatted.

MrishoLukamba · 2022-09-24T09:39:58Z

Logic seems correct, but code needs to be formatted.

Done, and should i use cargo fmt?

Ank4n · 2022-09-24T10:13:04Z

@MrishoLukamba I will close this PR and work on top of your code to get it merged. Pls add your Polkadot address in the PR for the tip.

Super thanks for your contribution. Feel free to follow the new PR here: #12343

MrishoLukamba · 2022-09-24T10:52:02Z

@MrishoLukamba I will close this PR and work on top of your code to get it merged. Pls add your Polkadot address in the PR for the tip.

Super thanks for your contribution. Feel free to follow the new PR here: #12343

Thanks, will also continue contributing on other issues.

Ank4n · 2022-09-24T10:54:09Z

/tip small

substrate-tip-bot · 2022-09-24T10:54:11Z

@Ank4n You are not allowed to request a tip. Only shawntabrizi, gavofyork, rphmeier, athei, andresilva, arkpar, bkchr, eskimor, drahnr, dvdplm, robbepop, cmichi, tomaka, pepyakin, tomusdrw, kianenigma, jacogr, rossbulat are allowed.

kianenigma · 2022-09-25T08:09:10Z

/tip small

substrate-tip-bot · 2022-09-25T08:09:13Z

@kianenigma A small tip was successfully submitted for MrishoLukamba (133WyzJ4cEEbQ4twhT3nQpoXYeTCzeKn8kWfRfEi4XDF7wKh on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

MrishoLukamba · 2022-09-25T09:54:54Z

/tip small

Sorry @kianenigma @Ank4n https://polkadot.js.org/apps/#/treasury/tips, the tip didnt go through

used maxunlockingchunks from config

690f8e0

MrishoLukamba requested a review from kianenigma as a code owner September 18, 2022 10:18

Ank4n self-requested a review September 19, 2022 09:07

kianenigma added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 19, 2022

kianenigma reviewed Sep 19, 2022

View reviewed changes

MrishoLukamba added 3 commits September 22, 2022 13:13

mhl MaxUnlockingChunks

c48c2de

no migration needed

355729c

tested for migration

4be8bce

kianenigma reviewed Sep 24, 2022

View reviewed changes

kianenigma suggested changes Sep 24, 2022

View reviewed changes

changes as per requested

fd3961e

Ank4n mentioned this pull request Sep 24, 2022

[fix] Bound staking ledger correctly with MaxUnlockingChunks from configuration #12343

Merged

Ank4n closed this Sep 24, 2022

louismerlin removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 13, 2022

MrishoLukamba mentioned this pull request Oct 18, 2022

Polkadot Fellow application polkadot-fellows/seeding#43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

used MaxUnlockingChunks from config #12289

used MaxUnlockingChunks from config #12289

MrishoLukamba commented Sep 18, 2022 •

edited

Loading

MrishoLukamba commented Sep 18, 2022

kianenigma commented Sep 19, 2022

kianenigma left a comment

MrishoLukamba commented Sep 19, 2022 •

edited

Loading

Ank4n commented Sep 20, 2022

MrishoLukamba commented Sep 20, 2022

MrishoLukamba commented Sep 22, 2022

Ank4n commented Sep 22, 2022

MrishoLukamba commented Sep 23, 2022 •

edited

Loading

Ank4n commented Sep 23, 2022

kianenigma Sep 24, 2022

kianenigma Sep 24, 2022

kianenigma Sep 24, 2022

kianenigma Sep 24, 2022

kianenigma Sep 24, 2022

kianenigma left a comment

MrishoLukamba commented Sep 24, 2022

Ank4n commented Sep 24, 2022

MrishoLukamba commented Sep 24, 2022

Ank4n commented Sep 24, 2022

substrate-tip-bot bot commented Sep 24, 2022

kianenigma commented Sep 25, 2022

substrate-tip-bot bot commented Sep 25, 2022

MrishoLukamba commented Sep 25, 2022

	for i in 0..<<Test as Config>::MaxUnlockingChunks as Get::<u32>>::get()- 1 {
	for i in 0..<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get()-1 {

	<<Test as Config>::MaxUnlockingChunks as Get::<u32>>::get() as usize
	<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() as usize

		//Concern is on validators only
		//By Default 11, 10 are stash and ctrl and 21,20

used MaxUnlockingChunks from config #12289

used MaxUnlockingChunks from config #12289

Conversation

MrishoLukamba commented Sep 18, 2022 • edited Loading

MrishoLukamba commented Sep 18, 2022

kianenigma commented Sep 19, 2022

kianenigma left a comment

Choose a reason for hiding this comment

MrishoLukamba commented Sep 19, 2022 • edited Loading

Ank4n commented Sep 20, 2022

MrishoLukamba commented Sep 20, 2022

MrishoLukamba commented Sep 22, 2022

Ank4n commented Sep 22, 2022

MrishoLukamba commented Sep 23, 2022 • edited Loading

Ank4n commented Sep 23, 2022

kianenigma Sep 24, 2022

Choose a reason for hiding this comment

kianenigma Sep 24, 2022

Choose a reason for hiding this comment

kianenigma Sep 24, 2022

Choose a reason for hiding this comment

kianenigma Sep 24, 2022

Choose a reason for hiding this comment

kianenigma Sep 24, 2022

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

MrishoLukamba commented Sep 24, 2022

Ank4n commented Sep 24, 2022

MrishoLukamba commented Sep 24, 2022

Ank4n commented Sep 24, 2022

substrate-tip-bot bot commented Sep 24, 2022

kianenigma commented Sep 25, 2022

substrate-tip-bot bot commented Sep 25, 2022

MrishoLukamba commented Sep 25, 2022

MrishoLukamba commented Sep 18, 2022 •

edited

Loading

MrishoLukamba commented Sep 19, 2022 •

edited

Loading

MrishoLukamba commented Sep 23, 2022 •

edited

Loading