[Feature] P2 Staking Tracker #12744

ruseinov · 2022-11-20T14:16:16Z

This is the second of a series of PRs aimed at reviving #11013
Work on: paritytech/polkadot-sdk#443

ruseinov · 2022-11-28T15:09:26Z

bot rebase

…take-tracker

paritytech-processbot · 2022-11-28T15:09:33Z

Rebased

ruseinov · 2022-11-28T19:22:36Z

bot rebase

…take-tracker

paritytech-processbot · 2022-11-28T19:22:42Z

Rebased

Cargo.toml

kianenigma · 2022-11-28T20:07:40Z

bin/node/runtime/src/lib.rs

@@ -757,6 +765,31 @@ impl pallet_bags_list::Config<VoterBagsListInstance> for Runtime {
 	type WeightInfo = pallet_bags_list::weights::SubstrateWeight<Runtime>;
 }

+/// A special VoterList instance used to test the StakeTracker.


impl blocks should not be documented. Trait definitions should be. If you want to put some doc here, it should start with //.

This is just a rough draft so far, I basically put some random docs here in there to define the intention. The documentation will be refined! Thanks for looking into this.

kianenigma · 2022-11-28T20:08:37Z

bin/node/runtime/src/lib.rs

+	type VoterList = VoterListTracker;
+	type TargetList = TargetList;


the names in two lines are not consistent -- either both somethingTracker or neither.

The idea here is that the TargetList is not used in Staking yet, so we can start tracking it in staking-tracker and then passing it to Staking without any migrations.
Since VoterList already exists - I had to add a suffix.

frame/staking/src/pallet/mod.rs

kianenigma · 2022-12-22T10:36:10Z

frame/staking/src/pallet/mod.rs

+		/// Once this becomes the source of truth - we should make sure `TargetList` and `VoterList`
+		/// are passed to Staking as `ReadOnlySortedListProvider`.
+		///
+		/// NOTE: Once we have more implementors of EventListener - this could be done similarly to


Good comment educational-wise, but toward the end it should go away IMO. The fact that OnStakingUpdate can be a tuple is abstracted and not something that staking needs to know about. All staking knows is that OnStakingUpdate is something that will notify anyone who's interested in these updates.

I guess this can stay until we switch staking to the new lists.

kianenigma · 2022-12-22T10:46:06Z

Here's my brain dump on the migration:

Either do it in one block, but for that you need to provide some stats of how many storage RW you are doing, how much weight, so we have an idea of how much of a slow block it is going to be. You also have to dry-run the migration in WASM to make sure it is fine memory-wise.
Do it over multiple blocks, during which we should freeze any transactions going to staking. Otherwise we need to deal with: what if someone updates their nomination while the initial migration is happening? lots of edge cases here. Perhaps solvable, but I think now allowing staking operations for e.g. 10 blocks is probably fine.

I am inclined to vouch for doing option 1, but as of recently, we lifted the Polkadot max nomination intention limit, so someone can attack this migration. We should, unfortunately, but maturely, opt for option 2.

Perhaps the concurrency issues are not too hard and we can do it in a easy-going manner on-idle.

ruseinov · 2023-01-04T11:22:28Z

bot rebase

paritytech-processbot · 2023-01-04T11:22:33Z

Error: Command 'Command { std: "git" "merge" "origin/master" "--no-ff" "--no-edit", kill_on_drop: false }' failed with status Some(1); output: no output

frame/stake-tracker/src/lib.rs

kianenigma · 2023-01-04T13:11:33Z

frame/stake-tracker/src/lib.rs

+
+		// This will panic if the validator is not in the map, but this should never happen!
+		ApprovalStake::<T>::mutate(who, |x: &mut Option<BalanceOf<T>>| {
+			x.map(|b| b.saturating_sub(Self::slashable_balance_of(who)))


mutate expects you to mutate x. what you are doing here is returning some value from the closure, which will not update x at all. If you had at least one test, this would have been caught by now ;)

ah yeah, it should assign to *x.

kianenigma · 2023-01-04T13:11:45Z

frame/stake-tracker/src/lib.rs

+		let _ = T::VoterList::on_remove(who).defensive();
+		for validator in nominations {
+			ApprovalStake::<T>::mutate(&validator, |x: &mut Option<BalanceOf<T>>| {
+				x.map(|b| b.saturating_sub(score))


kianenigma

I think this PR is moving slower than I expected, and I anticipate it could still be due to its large size.

If you want, we can split it even further:

Part 1

Add EventListeners to staking, and have stake-tracker only track changes to VotersList to EventListeners instead.
By that, I mean we would replace a few calls to T::VotersList::blah to T::EventListeners::on_blah
This should be really straightforward and easy to verify. It won't need a migration either.
As the outcome we would see:

impl pallet_staking::Config for Runtime {

    type VoterList = <Runtime as pallet_stake_tracker::Config>::VoterList;
    type TargetList = ValidatorsMap

We can also move some tests around, and make sure the StakingEvents is well tested.

Part 2

Then, importantly, without needing to make any changes to staking, we make pallet-stake-tracker to also track targets. This will need a single migration to initiate the approval stakes.

Staking would still use type TargetList = ValidatorsMap.

We can, again, do a lot of extensive unit tests here to make sure things are right. We can also test this new TargetList instance that is not used anywhere in an offchain bot and make sure all the approval stakes are correct in all blocks.

Part 3

Finally, once enough time has passed and we are sure, we flip to

impl pallet_staking::Config for Runtime {

    type VoterList = <Runtime as pallet_stake_tracker::Config>::VoterList;
    type TargetList = <Runtime as pallet_stake_tracker::Config>::TargetList;

Ofc you would be working on these in parallel, but I think splitting it further will help us get to some tangible outcome faster, and it will reviews easier.

Co-authored-by: Kian Paimani <[email protected]>

paritytech-cicd-pr · 2023-01-04T13:33:42Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2222619

kianenigma · 2023-01-08T21:55:57Z

closing as it will be replaced by smaller pieces to keep the projects and PR lists tidy.

[Enhancement] P2 Staking Tracker

91dd322

ruseinov requested a review from kianenigma November 20, 2022 14:16

github-actions bot added the A0-please_review Pull request needs code review. label Nov 20, 2022

ruseinov mentioned this pull request Nov 20, 2022

[Staking] Approval Stake tracking paritytech/polkadot-sdk#443

Open

ruseinov added 2 commits November 20, 2022 15:42

Staking interface changes

e8127c7

introducing stake-tracker pallet

c809c57

ruseinov mentioned this pull request Nov 20, 2022

[Feature] Part 2: Enable TargetList for validator ranking and approval stake tracking. #12645

Closed

ruseinov changed the title ~~[Enhancement] P2 Staking Tracker~~ [Feature] P2 Staking Tracker Nov 20, 2022

ruseinov added 9 commits November 21, 2022 21:31

some trait re-shuffling

99bd9d7

more trait fixes

0d4a022

more code

c5bb19c

more tracking logic

5549a78

add reaped handling

1666bdc

implement ReadOnlySortedListProvider

e7cfb35

more import fixes

54b2e33

more import fixes

da8f780

integrate EventListener into Staking

c89d8ec

ruseinov added C7-high ❗️ and removed C7-high ❗️ labels Nov 28, 2022

modify the EventListener interface for better Staking compat

7fcafe0

Merge remote-tracking branch 'origin/master' into ru/feature/pallet-s…

6689460

…take-tracker

Merge remote-tracking branch 'origin/master' into ru/feature/pallet-s…

9f3e436

…take-tracker

generate bags, fix node-runtime

bed5ad4

kianenigma reviewed Nov 28, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

kianenigma reviewed Nov 28, 2022

View reviewed changes

kianenigma reviewed Dec 22, 2022

View reviewed changes

frame/staking/src/pallet/mod.rs Show resolved Hide resolved

kianenigma reviewed Dec 22, 2022

View reviewed changes

ruseinov added 7 commits December 23, 2022 13:02

nl

1cd145a

revert a change

3297703

fix add_era_stakers

c618967

review comment fixes

6b33afc

add some comments, remove redundant debug_asserts in favour of try_state

bd18275

OnStakingUpdate void

9c70db7

some reshuffling

9b3fdf9

ruseinov added 2 commits January 4, 2023 12:23

Merge branch 'master' into ru/feature/pallet-stake-tracker

4a126cc

stake-tracker-migrations -> initializer

70fceb5

kianenigma reviewed Jan 4, 2023

View reviewed changes

frame/stake-tracker/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 4, 2023

View reviewed changes

frame/stake-tracker/src/lib.rs Outdated Show resolved Hide resolved

remove VoterList manipulations from Staking

9a90e4b

kianenigma reviewed Jan 4, 2023

View reviewed changes

ruseinov added 2 commits January 4, 2023 14:13

fix old migration

85529a3

fix migration

08118a4

kianenigma suggested changes Jan 4, 2023

View reviewed changes

ruseinov and others added 2 commits January 4, 2023 14:30

Update frame/stake-tracker/src/lib.rs

ace54bb

Co-authored-by: Kian Paimani <[email protected]>

Update frame/stake-tracker/src/lib.rs

ea35228

Co-authored-by: Kian Paimani <[email protected]>

ruseinov mentioned this pull request Jan 5, 2023

[Feature] StakeTracker P1 - VotersList #13079

Closed

kianenigma closed this Jan 8, 2023

ruseinov mentioned this pull request Jan 22, 2023

[Feature] StakeTracker P2 - TargetList #13195

Closed

ruseinov mentioned this pull request May 3, 2023

[Feature] StakeTracker P1 - VotersList #14073

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] P2 Staking Tracker #12744

[Feature] P2 Staking Tracker #12744

ruseinov commented Nov 20, 2022 •

edited

Loading

ruseinov commented Nov 28, 2022

paritytech-processbot bot commented Nov 28, 2022

ruseinov commented Nov 28, 2022

paritytech-processbot bot commented Nov 28, 2022

kianenigma Nov 28, 2022

ruseinov Nov 28, 2022

kianenigma Nov 28, 2022

ruseinov Nov 28, 2022

kianenigma Dec 22, 2022

ruseinov Dec 23, 2022

kianenigma commented Dec 22, 2022

ruseinov commented Jan 4, 2023

paritytech-processbot bot commented Jan 4, 2023

kianenigma Jan 4, 2023

ruseinov Jan 4, 2023

kianenigma Jan 4, 2023

kianenigma left a comment

paritytech-cicd-pr commented Jan 4, 2023

kianenigma commented Jan 8, 2023

		type VoterList = VoterListTracker;
		type TargetList = TargetList;

[Feature] P2 Staking Tracker #12744

[Feature] P2 Staking Tracker #12744

Conversation

ruseinov commented Nov 20, 2022 • edited Loading

ruseinov commented Nov 28, 2022

paritytech-processbot bot commented Nov 28, 2022

ruseinov commented Nov 28, 2022

paritytech-processbot bot commented Nov 28, 2022

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

kianenigma commented Dec 22, 2022

ruseinov commented Jan 4, 2023

paritytech-processbot bot commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

Part 1

Part 2

Part 3

paritytech-cicd-pr commented Jan 4, 2023

kianenigma commented Jan 8, 2023

ruseinov commented Nov 20, 2022 •

edited

Loading