Delay nominator revocations and exits #610

4meta5 · 2021-07-17T19:34:30Z

What does it do?

Splits Config::BondDuration into 4 new constants for each delay constant:

delay candidate exits by Config::LeaveCandidatesDelay rounds (was implemented before but constant delay was called Config::BondDuration)
delay reward payments to collators and nominators by Config::RewardPaymentDelay rounds (was implemented before but constant delay was called Config::BondDuration)
delay nomination revocations by Config::RevokeNominationDelay rounds (added in this in PR, before nominator revocations occurred as soon as the request succeeded)
delay nominator exits by Config::LeaveNominatorsDelay rounds (added in this PR, before nominator exits occurred as soon as the request succeeded)

bumped patch version because this does change how things work, even if it doesn't change the interface per se

Storage Migration

The migration code is in fn delay_nomination_exits_migration_execution. It will only run once by design. When it executes, it will emit an event and put a true value in storage so that it does not execute again.

Storage Value `ExitQueue` -> `ExitQueue2`

The ExitQueue went from just handling candidate exits to also handling delayed nominator exits and revocations.

Storage Map `NominatorState` -> `NominatorState2`

When delaying execution of nominator revocation and nominator exits, we need to have local context when the nominator is leaving to prevent it from performing more nominations when it is already scheduled to leave.

From storage map NominatorState -> NominatorState2 such that the keys (AccountId) stay the same, but the value for the map changes from struct Nominator -> struct Nominator2. There is an implementation of From<Nominator> for Nominator2 which is what is used to migrate the storage values from the first map to the second.

Weight Updates

The weights for leave_nominators and revoke_nomination is consistently lower than before. That's because they change from executing exits to scheduling exits. Scheduling execution of exits is cheaper than executing the exit itself.

pallets/parachain-staking/src/lib.rs

…diate and it is now delayed and there is no leave candidates TS test to show how to delay

…ing it out

4meta5 · 2021-07-18T20:54:51Z

Update weights for revoke_nomination and leave_nominators

tests/tests/test-stake.ts

notlesh · 2021-07-19T18:55:46Z

pallets/parachain-staking/src/lib.rs

 	}

 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_runtime_upgrade() -> Weight {


How would you feel about using the migrations pallet for this?

I don't know how to use it, and don't think it's necessary for this migration.

Feel free to make a PR into this branch which uses it.

notlesh · 2021-07-19T18:59:40Z

pallets/parachain-staking/src/lib.rs

+
+	#[derive(Encode, Decode, RuntimeDebug)]
+	/// Nominator state
+	pub struct Nominator2<AccountId, Balance> {


It would be awesome if we could architect this migration such that the pallet itself just changes the Nominator struct (in a non-backwards-compatible way) and we put all the gory details of what the original struct was and how to migrate in a separate file/lib/etc.

This is still a bit vague to me, but I'd be happy to help figure out what this should look like you want to go down this path.

From the PR description:

From storage map NominatorState -> NominatorState2 such that the keys (AccountId) stay the same, but the value for the map changes from struct Nominator -> struct Nominator2. There is an implementation of From<Nominator> for Nominator2 which is what is used to migrate the storage values from the first map to the second.

I commented out this impl From<Nominator<AccountId, Balance>> for Nominator2<AccountId, Balance> below

Ah, I see. From is an elegant way to handle that.

I guess, then, my suggestion would come down to:

Rename Nominator -> OldNominator (etc)

Move it to a different file (for clarity and/or potentially being excluded from future runtimes)

Rename Nominator2 -> Nominator

It looks like pulling a #[pallet::storage] definition out of the main pallet isn't really possible, but these utilities might help:

https://crates.parity.io/frame_support/storage/migration/index.html

I'll play with this to see if I can come up with something clean.

Ah, I see. From is an elegant way to handle that.

I guess, then, my suggestion would come down to:

Rename Nominator -> OldNominator (etc)

Move it to a different file (for clarity and/or potentially being excluded from future runtimes)

Rename Nominator2 -> Nominator

I had the same initial feeling when we did the previous migration which moved from CollatorState to CollatorState2. I had 2 reasons for that:

It is more elegant in the code to not have a number at the end of a variable (not too important)

It changes the way we query for the CollatorState (chain state -> parachainStaking -> CollatorState2)

But after thinking more about it, I'm not sure if it makes more sense to try to keep a same (exposed) variable with a different type. Because in both cases (changing the name of the storage, or changing its type), it is a breaking change that requires the developer to update its tools/sdk.
I'm open to discussion on this

One idea for handling these cases:

The new version of the pallet depends on the old version. The old type definition and storage definition are imported from the old pallet.

4meta5 · 2021-07-19T19:29:13Z

pallets/parachain-staking/src/lib.rs

+	fn delay_nomination_exits_migration_execution<T: Config>() {
+		if !<DelayNominationExitsMigration<T>>::get() {
+			// migrate from Nominator -> Nominator2
+			for (acc, nominator_state) in NominatorState::<T>::drain() {
+				let state: Nominator2<T::AccountId, BalanceOf<T>> = nominator_state.into();
+				<NominatorState2<T>>::insert(acc, state);
+			}
+			// migrate from ExitQueue -> ExitQueue2
+			let just_collators_exit_queue = <ExitQueue<T>>::take();
+			let mut candidates: Vec<T::AccountId> = Vec::new();
+			for (acc, _) in just_collators_exit_queue.clone().into_iter() {
+				candidates.push(acc);
+			}
+			<ExitQueue2<T>>::put(ExitQ {
+				candidates: candidates.into(),
+				nominators_leaving: OrderedSet::new(),
+				candidate_schedule: just_collators_exit_queue,
+				nominator_schedule: Vec::new(),
+			});
+			<DelayNominationExitsMigration<T>>::put(true);
+			Pallet::<T>::deposit_event(Event::DelayNominationExitsMigrationExecuted);
+		}


@notlesh this is the full migration logic

It emits an event when it executes and also replaces a false storage value with true so it doesn't run more than once.

4meta5 · 2021-07-19T19:29:53Z

pallets/parachain-staking/src/lib.rs

+		if !<DelayNominationExitsMigration<T>>::get() {
+			// migrate from Nominator -> Nominator2
+			for (acc, nominator_state) in NominatorState::<T>::drain() {
+				let state: Nominator2<T::AccountId, BalanceOf<T>> = nominator_state.into();


@notlesh This .into() uses the From impl, which contains exactly how we convert each Nominator -> Nominator2

While this is an elegant use of the From, IF this is only used here in the migration, I would (for future times) argue that keeping the mutation logic directly here would benefit the reader and prevent bug where someone would use the wrong structure because it gets converted.

That would imply we keep the old struct in the code as well. I'd prefer to move the old struct and the From impl to docs, something that isn't code. We can link the code at each migration commit so someone could easily write From<Nominator1> for Nominator3 if they want to, but we don't have to write that ourselves.

Sounds good

moonbeam-types-bundle/index.ts

pallets/parachain-staking/src/lib.rs

runtime/moonriver/tests/integration_test.rs

ghost

lgtm, I had a few questions but nothing blocking

crystalin · 2021-07-20T15:55:50Z

@notlesh if you can review this one and once done change the labels (remove the pleasereview -> appropriate Ax-xxxxxx)

4meta5 · 2021-07-21T02:17:22Z

I feel more comfortable with the migration design in this PR than the one in #616 (which uses pallet-migrations from #527). I recognize that if I agree to merge #616 into this branch, then this PR will be approved and merged. Despite this easy path to getting this PR merged, I still don't like moving the migration logic into another pallet for the sake of doing all the migrations in that pallet.

To be fair, I don't fully understand the pallet-migrations, but I do understand the migration logic in this PR and feel it is complete as is.

notlesh · 2021-07-21T02:58:02Z

I feel more comfortable with the migration design in this PR than the one in #616 (which uses pallet-migrations from #527). I recognize that if I agree to merge #616 into this branch, then this PR will be approved and merged. Despite this easy path to getting this PR merged, I still don't like moving the migration logic into another pallet for the sake of doing all the migrations in that pallet.

To be fair, I don't fully understand the pallet-migrations, but I do understand the migration logic in this PR and feel it is complete as is.

Thanks for the feedback. I should spend some time documenting pallet-migrations so that it's clear what its purpose is, how it works, and how to use it.

That said, I wanted to point out that the changes to this particular migration in #616 actually have little to do with pallet-migrations. In fact, you could take #616, simply implement the on_runtime_upgrade() hook and have it invoke delay_nominator_exits_migration() and it would act in every way like a "normal" runtime upgrade.

If you wanted to avoid the pallet-migrations for now, you could probably copy the migrations.rs file and then remove the duplicated (e.g. Nominator vs Nominator2) stuff from lib.rs and be done.

crystalin · 2021-07-21T03:12:38Z

@4meta5 , let me give you a bit of context.
The pallet migrations was started (few weeks ago ?) after a meeting where we realized that keeping the migration in the pallet itself is dangerous (mostly because both the pallet and the runtime can be updated independently).
Parity has also decided to not include their migration in the pallet directly neither for the same reason.

While your migration is safe in case it gets executed multiple times, it is not safe from when we are going to remove it from the pallet. For example, when we will resume Moonshadow, it is likely that the staking pallet won't include this migration anymore, and Moonshadow wouldn't not be upgraded properly.

The idea behind the pallet migration is to keep track and offer an easy solution to perform those runtime upgrade in the runtime directly.

@notlesh, we should probably have you make a presentation of your pallet and also spend more time testing it before using it in production.

4meta5 · 2021-07-21T08:19:56Z

In fact, you could take #616, simply implement the on_runtime_upgrade() hook and have it invoke delay_nominator_exits_migration() and it would act in every way like a "normal" runtime upgrade.

This PR does contain the implementation on_runtime_upgrade which just invokes delay_nominator_exits_migration_execution(). Is this suggestion to put it in a different file?

While your migration is safe in case it gets executed multiple times, it is not safe from when we are going to remove it from the pallet. For example, when we will resume Moonshadow, it is likely that the staking pallet won't include this migration anymore, and Moonshadow wouldn't not be upgraded properly.

How does pallet-migrations fix this problem? Does it store the migration logic in local storage so it can be executed on a chain that hasn't executed the migration before?

We either have to purge chain state or store migrations from every schema to every other schema. There's no other solution as far as I can see.

notlesh · 2021-07-21T14:26:48Z

In fact, you could take #616, simply implement the on_runtime_upgrade() hook and have it invoke delay_nominator_exits_migration() and it would act in every way like a "normal" runtime upgrade.

This PR does contain the implementation on_runtime_upgrade which just invokes delay_nominator_exits_migration_execution(). Is this suggestion to put it in a different file?

Right, I've probably made some confusion here because I did multiple things in that PR. One of the suggestions it implements is to isolate the migration logic so that the main pallet itself remains uncluttered with the migration code (and also avoids needing to rename or duplicate types, storage items, etc.)

While your migration is safe in case it gets executed multiple times, it is not safe from when we are going to remove it from the pallet. For example, when we will resume Moonshadow, it is likely that the staking pallet won't include this migration anymore, and Moonshadow wouldn't not be upgraded properly.

How does pallet-migrations fix this problem? Does it store the migration logic in local storage so it can be executed on a chain that hasn't executed the migration before?

It tracks which migrations (based on their unique name) have been executed, which means that it's safe to leave a migration around as long as desired. In our case, this is helpful because we can leave something like your migration laying around until it has been deployed to all networks we care about, and then it could be removed (or replaced with a no-op, or just left there, etc. But in no case will it be called more or less than once.)

Your implementation handles this manually by adding an extra storage item -- it's not fundamentally different than what pallet-migrations does, so you could think of that as a convenience.

The code isn't stored on chain. pallet-migrations requires a list of migrations to be provided to it. These migrations essentially have a fn pointer and that's how it accesses the migration code. It doesn't care what that code does, though, so it could be removed later.

(all that is stored on chain is basically a map of pallet name -> migration state)

We either have to purge chain state or store migrations from every schema to every other schema. There's no other solution as far as I can see.

Yes, what I'd like to propose is that each migration is fully self sufficient in that it knows the old schema and the new schema (in neither case relying on types defined in the main pallet). This would allow the migration to live on long after a pallet has significantly changed, lending all the flexibility you could want out of that migration.

pallets/parachain-staking/src/lib.rs

4meta5 added 11 commits July 13, 2021 17:25

init

a72fa49

save

3bee0f8

Merge branch 'master' into amar-delay-nominator-exits

2bd14b6

save

80b0fdc

save

64c8975

log::trace -> log::warn, drop errors, and fix compile errors

ed37abc

commit migrationss design will revert for simpler impl

bdef907

minimal migration impl instead

14ddec0

delay leave nominators with all unit tests passing

753c1ff

delayed revoke_nomination with all tests passing

1dc6721

unit tests

828ccfe

4meta5 commented Jul 17, 2021

View reviewed changes

pallets/parachain-staking/src/lib.rs Outdated Show resolved Hide resolved

4meta5 commented Jul 17, 2021

View reviewed changes

pallets/parachain-staking/src/lib.rs Show resolved Hide resolved

4meta5 added 6 commits July 18, 2021 11:24

split BondDuration into 4 constants and update runtimes

3dfaa37

try green

e47cbff

green

5b402f6

rm revoke nomination TS test because it expects revocation to be imme…

7ead86a

…diate and it is now delayed and there is no leave candidates TS test to show how to delay

add types to moonbeam types bundle and update test instead of comment…

b4b1c5f

…ing it out

fix

dc39836

4meta5 added 5 commits July 18, 2021 17:07

fix

48fa23c

try fix TS test

b81d122

try again

c4a70cd

every byte counts

cddd23d

x

b0060fb

4meta5 commented Jul 19, 2021

View reviewed changes

tests/tests/test-stake.ts Outdated Show resolved Hide resolved

notlesh reviewed Jul 19, 2021

View reviewed changes

4meta5 commented Jul 19, 2021

View reviewed changes

4meta5 added 2 commits July 19, 2021 15:58

z

15fb7dd

update weights

fc793dc

4meta5 mentioned this pull request Jul 19, 2021

DO NOT MERGE Try delay nominator migration with pallet-migrations #616

Closed

4meta5 added 2 commits July 19, 2021 17:04

Merge branch 'master' into amar-delay-nominator-exits

d86eb2f

ready for review

f90f0b3

4meta5 marked this pull request as ready for review July 20, 2021 11:00

4meta5 added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Jul 20, 2021

ghost reviewed Jul 20, 2021

View reviewed changes

moonbeam-types-bundle/index.ts Show resolved Hide resolved

ghost reviewed Jul 20, 2021

View reviewed changes

pallets/parachain-staking/src/lib.rs Outdated Show resolved Hide resolved

ghost reviewed Jul 20, 2021

View reviewed changes

runtime/moonriver/tests/integration_test.rs Show resolved Hide resolved

ghost approved these changes Jul 20, 2021

View reviewed changes

rm unused errors

94183d8

4meta5 requested a review from notlesh July 20, 2021 16:54

4meta5 added 2 commits July 20, 2021 14:03

more accurate weight returned in on runtime upgrade

32f0985

fix weight, one read to check if migration has run before

1eed8ab

4meta5 added the D1-runtime-migration PR introduces code that might require downstream chains to run a runtime upgrade. label Jul 21, 2021

Merge branch 'master' into amar-delay-nominator-exits

bf7af99

crystalin added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Jul 22, 2021

4meta5 commented Jul 22, 2021

View reviewed changes

pallets/parachain-staking/src/lib.rs Outdated Show resolved Hide resolved

Update pallets/parachain-staking/src/lib.rs

8855011

4meta5 merged commit 5086f00 into master Jul 22, 2021

4meta5 deleted the amar-delay-nominator-exits branch July 22, 2021 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay nominator revocations and exits #610

Delay nominator revocations and exits #610

4meta5 commented Jul 17, 2021 •

edited

4meta5 commented Jul 18, 2021 •

edited

notlesh Jul 19, 2021

4meta5 Jul 19, 2021

notlesh Jul 19, 2021

4meta5 Jul 19, 2021 •

edited

notlesh Jul 19, 2021

notlesh Jul 19, 2021

crystalin Jul 21, 2021

JoshOrndorff Jul 27, 2021

4meta5 Jul 19, 2021

4meta5 Jul 19, 2021

crystalin Jul 21, 2021

4meta5 Jul 21, 2021 •

edited

crystalin Jul 21, 2021

ghost left a comment

crystalin commented Jul 20, 2021

4meta5 commented Jul 21, 2021

notlesh commented Jul 21, 2021

crystalin commented Jul 21, 2021

4meta5 commented Jul 21, 2021 •

edited

notlesh commented Jul 21, 2021

Delay nominator revocations and exits #610

Delay nominator revocations and exits #610

Conversation

4meta5 commented Jul 17, 2021 • edited

What does it do?

Storage Migration

Storage Value ExitQueue -> ExitQueue2

Storage Map NominatorState -> NominatorState2

Weight Updates

4meta5 commented Jul 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4meta5 Jul 19, 2021 • edited

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

Choose a reason for hiding this comment

4meta5 Jul 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

crystalin commented Jul 20, 2021

4meta5 commented Jul 21, 2021

notlesh commented Jul 21, 2021

crystalin commented Jul 21, 2021

4meta5 commented Jul 21, 2021 • edited

notlesh commented Jul 21, 2021

4meta5 commented Jul 17, 2021 •

edited

Storage Value `ExitQueue` -> `ExitQueue2`

Storage Map `NominatorState` -> `NominatorState2`

4meta5 commented Jul 18, 2021 •

edited

4meta5 Jul 19, 2021 •

edited

4meta5 Jul 21, 2021 •

edited

4meta5 commented Jul 21, 2021 •

edited