[Staking] Split candidate state for PoV optimization #1117

4meta5 · 2021-12-23T03:00:13Z

What does it do?

Split candidate state such that we do not read any unnecessary storage when we do the delegations snapshot for each selected collator at round transitions and also for any delegation changes in general.

This does NOT change any extrinsics.

update migration to revoke all old bottom delegations that will not fit into new bounded delegations
configure runtimes and update integration tests
add more unit tests (migration, new metadata fields whenever they are supposed to change)
update benchmarking -> weights

Associated Constant Changes

MaxDelegatorsPerCandidate -> MaxTopDelegationsPerCandidate = 300 in all runtimes
_unbounded_bottom -> MaxBottomDelegationsPerCandidate = 50 in all runtimes

Storage and Type Changes

CandidateState(key: AccountId, value: CollatorCandidate) is split into 3 maps:

CandidateInfo(key: AccountId, value: CandidateMetadata)
TopDelegations(key: AccountId, value: Delegations)
BottomDelegations(key: AccountId, value: Delegations)

What important points reviewers should know?

Enforced First Come First Serve For Delegations of Same Amount

This was not enforced in the code prior to this PR even though we thought it was.

CandidateInfo designed to limit getting Top || Bottom delegations

When Can A Delegation Be Kicked

The lowest bottom delegation can be kicked (force revoked) in 2 scenarios. Both scenarios only occur when the top and bottom delegations are full.

new top delegation inserted which pushes lowest top to the bottom => kicks lowest bottom
new bottom delegation greater than lowest bottom => kicks lowest bottom

Is there something left for follow-up PRs?

Similar optimizations should be made to DelegatorState. We should store the delegations themselves separate from the metadata (ie delegation_count, total_amount, etc). This will be done in the same PR that removes the dependency on OrderedSet altogether and fixes the PartialEq impl for Bond.

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

#1105 was a hotfix patch but the design in this PR was preferred and the vuln was determined to not be significant enough to prefer the hotfix solution

What value does it bring to the blockchain users?

Transaction fees decreased for join_candidates, schedule_leave_candidaets, cancel_leave_candidates, go_online, go_offline, candidate_bond_more, schedule_candidate_bond_less, execute_candidate_bond_less, cancel_candidate_bond_less, delegate, schedule_leave_delegators, cancel_leave_delegators, delegator_bond_more.

Transaction fees (weights) increased for execute_leave_delegators, execute_leave_candidates.

The weight hint CandidateDelegationCount was also added to execute_leave_candidates to make the cost more accurately be proportional to the number of delegations for the candidate.

…ntaining Total storage item and im unsure we need it

…increase and decrease delegations

pallets/parachain-staking/src/lib.rs

notlesh · 2022-01-03T19:58:39Z

pallets/parachain-staking/src/lib.rs

+		/// Candidate
+		CandidateWentOffline(T::AccountId),
+		/// Candidate
+		CandidateBackOnline(T::AccountId),


Why remove RoundIndex?

It is an unnecessary GET, we will always know the round in which the event is emitted because we know the block it happened.

As a client receiving this event, wouldn't I either need to track which round I'm in or make an extra query to ask for it?

As @librelois pointed out, this change reduces blockspace slightly. I no longer have a strong opinion about this...

I think that in the absolute, the blocks must contain the strictly necessary and sufficient data, the content of the blocks does not have for role to simplify the life of the users, it's the role of the indexers like subsquid, subscan, etc.

notlesh · 2022-01-03T20:26:13Z

pallets/parachain-staking/src/lib.rs

@@ -223,6 +224,857 @@ pub mod pallet {
 		pub state: CollatorStatus,
 	}

+	#[derive(Clone, Default, Encode, Decode, RuntimeDebug, TypeInfo)]
+	/// Type for top and bottom delegation storage item
+	pub struct Delegations<AccountId, Balance> {


I find this struct (or its impl) a bit confusing. It seems to be either a top list or a bottom list depending on how it's called. Maybe making separate types for the two would help.

Why create separate types if they require the same functionality? Then I have to define the same methods on each struct or write functions that work on either type.

They are already different storage items with different names TopDelegations and BottomDelegations. That should alleviate your concern.

Initially I didn't realize that we are getting rid of the reverse-sorted bottom delegations (right?) which makes sense, I like that change. Given that, having different subclasses seems unnecessary.

My main concern with the design is that, as a "layer of abstraction," this partially bleeds the details of the pallet itself.

Practically speaking, this struct could be used inconsistently by its callers as both a bottom and top. insert_sorted_greatest_to_least makes no assumptions about capacity (it's up to the caller to do that in that case), yet 'top_capacity' and 'bottom_capacity' both make assumptions about capacity. This is especially tricky because both of the capacity functions test equality ('==' rather than '>='). So if we ever accidentally go past our bounds, the struct will continually tell us that we are CapacityStatus::Partial, which would probably trigger the caller to keep inserting more delegations.

In terms of making this struct "difficult to use incorrectly," I think we could simply add the capacity itself to the struct. This would have some advantages:

top_capacity and bottom_capacity collapse into one fn

this capacity fn can convert to >= (that could happen regardless, I suppose)

insert_sorted_greatest_to_least can do proper bounds checking

unit testing becomes trivial and is fully decoupled from the pallet itself

a resize fn could be added (again, that could probably happen regardless).

Potentially, the capacity could be generic, although that would have implications on changing things at runtime.

Initially I didn't realize that we are getting rid of the reverse-sorted bottom delegations (right?) which makes sense, I like that change. Given that, having different subclasses seems unnecessary.

Yes, exactly

Practically speaking, this struct could be used inconsistently by its callers as both a bottom and top. insert_sorted_greatest_to_least makes no assumptions about capacity (it's up to the caller to do that in that case), yet 'top_capacity' and 'bottom_capacity' both make assumptions about capacity.

Yes, but this is always enforced by the caller in the code prior to the call. Moreover, there is a capacity check in each of the caller functions that checks capacity.

In terms of making this struct "difficult to use incorrectly," I think we could simply add the capacity itself to the struct.

There is no need to do this because the Config::MaxTopDelegationsPerCandidate and Config::MaxTopDelegationsPerCandidate are constants so there is no cost to getting them. Inside the methods on CandidateMetadata that edit the delegations, we get these constants and check capacity to ensure correctness. Please audit those methods on CandidateMetadata and the use of the constants inside of them to check capacity.

pallets/parachain-staking/src/migrations.rs

4meta5 · 2022-01-07T15:56:26Z

pallets/parachain-staking/src/lib.rs

-			<CandidateState<T>>::insert(&collator, state);
-			Self::deposit_event(Event::CandidateBackOnline(
-				<Round<T>>::get().current,
-				collator,
-			));
+			<CandidateInfo<T>>::insert(&collator, state);
+			Self::deposit_event(Event::CandidateBackOnline(collator));


This is a small optimization that was unrelated to this PR but included anyway. There is no need to emit round number in event.

notlesh · 2022-01-07T16:13:08Z

pallets/parachain-staking/src/benchmarks.rs

@@ -387,13 +389,13 @@ benchmarks! {
 		)?;
 	} verify {
 		assert!(
-			Pallet::<T>::candidate_state(&caller).unwrap().request.is_none()
+			Pallet::<T>::candidate_info(&caller).unwrap().request.is_none()
 		);
 	}

 	delegate {


Per our conversation this morning:

delegate() is now either cheap (doesn't touch bottom delegations) or expensive (it does). We should do some measurement of this, but assuming it's a drastic difference, we have a few options:

Make two extrinsics (probably a bad option)

Add a hint like we have elsewhere. I'm not quite sure that this will work well with benchmarking, however. We may at least need to create multiple benchmarks for this to work.

Assume worst-case for weight charging and refund. This is unprecedented in our codebase, so it might be a worthwhile experiment in any case.

delegate() is now either cheap (doesn't touch bottom delegations) or expensive (it does).

No, it is cheap when it only touches TOP XOR BOTTOM. There are cases when it touches the bottom and not top which are just as cheap as if it only touched the top (assuming they have the same size which is an assumption which could be challenged). So the weight hint would need to represent this.

From Elois, we should add a bool weight hint that represents whether or not it touches the top && bottom. I think it needs to represent whether it touches the top && bottom as well as the total delegations it searched before insertion i.e. if it just inserts into top, then weight hint also uses length of top (or at least the max)

The hard part is that if it pushes the lowest bottom to the top, then it touches the bottom as well. So the implementation would need to cover that edge case.

I'll think about it, but it may be better suited as a follow up.

…gations with same amount

…he configured max bottom delegations per candidate

…-optimization

4meta5 · 2022-01-16T20:11:35Z

I've tested enough to feel confident marking this as ready for review. I'm still writing more tests though and then updating the benchmarking -> weights.

TODO:

test all CandidateMetadata fields are updated correctly whenever they ought to change
test migration correctly revokes all bottom delegations that don't fit into new bottom bounded delegations
update benchmarking -> weights

…-optimization

notlesh · 2022-01-25T20:09:53Z

pallets/parachain-staking/src/weights.rs

+	fn execute_leave_candidates(x: u32) -> Weight {
+		(0 as Weight) // Standard Error: 8_000
+			.saturating_add((27_557_000 as Weight).saturating_mul(x as Weight))
+			.saturating_add(T::DbWeight::get().reads(6 as Weight))
+			.saturating_add(T::DbWeight::get().reads((2 as Weight).saturating_mul(x as Weight)))
+			.saturating_add(T::DbWeight::get().writes(3 as Weight))
+			.saturating_add(T::DbWeight::get().writes((2 as Weight).saturating_mul(x as Weight)))


This change makes it quite expensive to leave if you have a lot of delegators (something you don't directly control), but that seems perfectly reasonable; you're affecting a lot of other people.

One thing we should keep in mind is that there is some point where this is so "heavy" that it can't be executed in one block.

This was actually not yet changed in this PR. It was in #1207 , so I do need to rerun this and a few other benchmarks.

It became even more expensive

pallets/parachain-staking/src/lib.rs

notlesh

I left a lot of comments, but most were about unsafe math and there were a few minor questions / suggestions. I had at least one concerning question though.

pallets/parachain-staking/src/lib.rs

notlesh · 2022-01-25T21:33:48Z

pallets/parachain-staking/src/lib.rs

+						.delegations
+						.clone()
+						.into_iter()
+						.filter_map(|d| {


This pattern is repeated a lot, it could abstracted

notlesh · 2022-01-25T21:36:34Z

pallets/parachain-staking/src/lib.rs

+					let highest_bottom_delegation = bottom_delegations.delegations.remove(0);
+					bottom_delegations.total -= highest_bottom_delegation.amount;
+					// insert highest bottom into top
+					top_delegations.insert_sorted_greatest_to_least(highest_bottom_delegation);


Here you could probably have taken note of the index from which you removed earlier to avoid a sorted insert

it is not guaranteed to be in the same position as the one removed

notlesh · 2022-01-25T21:37:31Z

pallets/parachain-staking/src/lib.rs

+					// insert highest bottom into top
+					top_delegations.insert_sorted_greatest_to_least(highest_bottom_delegation);
+					// insert previous top into bottom
+					bottom_delegations.insert_sorted_greatest_to_least(delegation);


This should always be going into the top, right?

In fact, even if there is a tie for the top, you want to make sure it goes at the beginning of the identical bonds -- not the end (otherwise we fail to preserve insertion order fairness) -- right?

The highest bottom is going into the top and the changed delegation is going into the bottom.

The condition for this branch to execute is bond_after_less_than_highest_bottom and it is a strict less than so we definitely want to insert the decreased top delegation into the bottom and pop the highest bottom into the top.

pallets/parachain-staking/src/lib.rs

…ave candidates

pallets/parachain-staking/src/benchmarks.rs

pallets/parachain-staking/src/lib.rs

girazoki

My main concerns have been solved so its an approval for me.

4meta5 added 10 commits December 22, 2021 21:57

join candidates leave candidates go online offline

7b94de4

wip

430abbf

still wip on delegation changes

cbdca3f

in progress much more complicated now because of the necessity of mai…

ee5d423

…ntaining Total storage item and im unsure we need it

add highest bottom delegation amount to candidate metadata and start …

f385e91

…increase and decrease delegations

edge cases

c69bcd4

complete impl still needs tests

dd96432

impl more efficient candidate snapshot

1469d2f

passing unit pallet units but need to update migration unit test

b08d567

fix some benchmark and save start migration

8437343

notlesh reviewed Jan 3, 2022

View reviewed changes

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

notlesh reviewed Jan 3, 2022

View reviewed changes

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

notlesh reviewed Jan 3, 2022

View reviewed changes

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

migration impl with minimal pre post checks

ec1a569

4meta5 commented Jan 7, 2022

View reviewed changes

notlesh reviewed Jan 7, 2022

View reviewed changes

4meta5 added 10 commits January 10, 2022 11:13

more tests and make all bound checks more strict

57f1c18

patch sorted insertion bug to enforce first come first serve for dele…

238b46a

…gations with same amount

patch patch

59815cf

update configs and need to update the migration

8171b19

fix migration to work if old bottom delegations len is greater than t…

a101d9b

…he configured max bottom delegations per candidate

save

ac7bfee

update parachain staking precompile

8091d59

master.into

25fea1d

fmt

3aac63e

Merge branch 'master' into amar-staking-split-candidate-state-for-pov…

6f389eb

…-optimization

4meta5 mentioned this pull request Jan 14, 2022

[Staking] Patch candidate bond more to update CandidatePool + hotfix extrinsic to fix incorrect state #1162

Merged

4meta5 added 2 commits January 20, 2022 17:45

Merge branch 'master' into amar-staking-split-candidate-state-for-pov…

591f99b

…-optimization

fix TS test

f175b20

4meta5 mentioned this pull request Jan 24, 2022

release runtime-1200 #1179

Closed

32 tasks

4meta5 added 2 commits January 25, 2022 12:32

replace unstable sorts with stable sorts

424df86

into master

b24cee0

notlesh reviewed Jan 25, 2022

View reviewed changes

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

set max bottom delegations per candidate to 50

74949d2

notlesh reviewed Jan 25, 2022

View reviewed changes

4meta5 added 4 commits January 25, 2022 17:24

insert highest bottom into top when removing a top delegation

0c03694

address most comments

843bdaa

update benchmarking code and add weight hint unit test for execute le…

22286b9

…ave candidates

update only benchmarks that changed a lot relative to existing weights

6ff4664

girazoki requested changes Jan 26, 2022

View reviewed changes

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

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

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

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

nanocryk reviewed Jan 26, 2022

View reviewed changes

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

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

girazoki reviewed Jan 26, 2022

View reviewed changes

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

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

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

4meta5 added 2 commits January 26, 2022 09:38

address most review comments

29bc4d2

address edge case and fix comments

3f25262

4meta5 requested a review from girazoki January 26, 2022 16:35

girazoki approved these changes Jan 26, 2022

View reviewed changes

4meta5 added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Jan 26, 2022

4meta5 merged commit 5e62387 into master Jan 26, 2022

4meta5 deleted the amar-staking-split-candidate-state-for-pov-optimization branch January 26, 2022 18:50

This was referenced Jan 31, 2022

Include SplitCandidateState migration in pallet-migrations #1241

Merged

[Staking] Update delegate new error message #1258

Merged

notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Feb 9, 2022

4meta5 mentioned this pull request Mar 1, 2022

[Staking] Fix benchmarks (and update weights) for pay_one_collator_reward and round_transition_on_initialize #1333

Merged

sea212 mentioned this pull request Mar 31, 2022

Add missing parachain-staking migration zeitgeistpm/zeitgeist#517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Staking] Split candidate state for PoV optimization #1117

[Staking] Split candidate state for PoV optimization #1117

4meta5 commented Dec 23, 2021 •

edited

Loading

notlesh Jan 3, 2022

4meta5 Jan 4, 2022 •

edited

Loading

notlesh Jan 6, 2022

notlesh Jan 7, 2022

librelois Jan 7, 2022 •

edited

Loading

notlesh Jan 3, 2022

4meta5 Jan 4, 2022 •

edited

Loading

4meta5 Jan 4, 2022

notlesh Jan 6, 2022

4meta5 Jan 10, 2022 •

edited

Loading

4meta5 Jan 7, 2022

notlesh Jan 7, 2022

4meta5 Jan 10, 2022 •

edited

Loading

4meta5 commented Jan 16, 2022 •

edited

Loading

notlesh Jan 25, 2022

4meta5 Jan 25, 2022

4meta5 Jan 26, 2022

notlesh left a comment

notlesh Jan 25, 2022

notlesh Jan 25, 2022

4meta5 Jan 25, 2022

notlesh Jan 25, 2022

4meta5 Jan 25, 2022 •

edited

Loading

girazoki left a comment

[Staking] Split candidate state for PoV optimization #1117

[Staking] Split candidate state for PoV optimization #1117

Conversation

4meta5 commented Dec 23, 2021 • edited Loading

What does it do?

Associated Constant Changes

Storage and Type Changes

What important points reviewers should know?

Enforced First Come First Serve For Delegations of Same Amount

CandidateInfo designed to limit getting Top || Bottom delegations

When Can A Delegation Be Kicked

Is there something left for follow-up PRs?

What alternative implementations were considered?

What value does it bring to the blockchain users?

Choose a reason for hiding this comment

4meta5 Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

librelois Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4meta5 Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4meta5 Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4meta5 Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

4meta5 commented Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notlesh 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

Choose a reason for hiding this comment

4meta5 Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

girazoki left a comment

Choose a reason for hiding this comment

4meta5 commented Dec 23, 2021 •

edited

Loading

4meta5 Jan 4, 2022 •

edited

Loading

librelois Jan 7, 2022 •

edited

Loading

4meta5 Jan 4, 2022 •

edited

Loading

4meta5 Jan 10, 2022 •

edited

Loading

4meta5 Jan 10, 2022 •

edited

Loading

4meta5 commented Jan 16, 2022 •

edited

Loading

4meta5 Jan 25, 2022 •

edited

Loading