Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

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

Merged
merged 14 commits into from
Sep 27, 2022

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Sep 24, 2022

Fixes #12227
Original PR #12289 and contributor: @MrishoLukamba

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Sep 24, 2022
@Ank4n Ank4n added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 24, 2022
@Ank4n
Copy link
Contributor Author

Ank4n commented Sep 24, 2022

bot rebase

@paritytech-processbot
Copy link

Rebased

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in itself. But there's a follow-up.

First, look into chill_other and what it does. You will find this wacky code in it:

if Nominators::<T>::contains_key(&stash) && Nominators::<T>::get(&stash).is_none() {
	Self::chill_stash(&stash);
	return Ok(())
}

This is basically checking: if a nominator is in a state where it is not decodable, then chill it.

I think we can take this a step further. Look into reap_stash. We can also add a clause to this function, that if a staker (regardless of the role), is in a state where it is not decodable (e.g. Ledger::contains_key(staler) && Ledger::get(staker).is_none()) then they can be reaped as well.

@Ank4n
Copy link
Contributor Author

Ank4n commented Sep 26, 2022

Looks good in itself. But there's a follow-up.

First, look into chill_other and what it does. You will find this wacky code in it:

if Nominators::<T>::contains_key(&stash) && Nominators::<T>::get(&stash).is_none() {
	Self::chill_stash(&stash);
	return Ok(())
}

This is basically checking: if a nominator is in a state where it is not decodable, then chill it.

I think we can take this a step further. Look into reap_stash. We can also add a clause to this function, that if a staker (regardless of the role), is in a state where it is not decodable (e.g. Ledger::contains_key(staler) && Ledger::get(staker).is_none()) then they can be reaped as well.

There is an issue here since ledger itself is undecodable, the stash can't be read to chill it.

let ledger = Self::ledger(&controller);

if Ledger::<T>::contains_key(&controller) && ledger.is_none() {
				Self::chill_stash(ledger.stash); // can't read ledger stash here
}

@kianenigma
Copy link
Contributor

Looks good in itself. But there's a follow-up.
First, look into chill_other and what it does. You will find this wacky code in it:

if Nominators::<T>::contains_key(&stash) && Nominators::<T>::get(&stash).is_none() {
	Self::chill_stash(&stash);
	return Ok(())
}

This is basically checking: if a nominator is in a state where it is not decodable, then chill it.
I think we can take this a step further. Look into reap_stash. We can also add a clause to this function, that if a staker (regardless of the role), is in a state where it is not decodable (e.g. Ledger::contains_key(staler) && Ledger::get(staker).is_none()) then they can be reaped as well.

There is an issue here since ledger itself is undecodable, the stash can't be read to chill it.

let ledger = Self::ledger(&controller);

if Ledger::<T>::contains_key(&controller) && ledger.is_none() {
				Self::chill_stash(ledger.stash); // can't read ledger stash here
}

Assuming that we know the slash is only un-decodable because of the latter unlocking, we can still decode the first field of the struct, namely the stash.

Let's discuss this in a dedicated issue though.

@Ank4n
Copy link
Contributor Author

Ank4n commented Sep 27, 2022

bot rebase

@paritytech-processbot
Copy link

Rebased

@Ank4n
Copy link
Contributor Author

Ank4n commented Sep 27, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 74daaf1 into master Sep 27, 2022
@paritytech-processbot paritytech-processbot bot deleted the ankan/mrisho-MaxUnlockingChunks branch September 27, 2022 15:44
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…figuration (paritytech#12343)

* used maxunlockingchunks from config

* mhl MaxUnlockingChunks

* no migration needed

* changes as per requested

* fmt

* fix tests

* fix benchmark

* warning in the doc for abrupt changes in the config

* less unnecessary details in the test

* fix tests

Co-authored-by: mrisholukamba <[email protected]>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

MaxUnlockingChunks should be configurable via Runtime
4 participants