Improves benchmarks for pallet parachain-staking #2317

nbaztec · 2023-05-29T17:37:39Z

What does it do?

fixes staking benchmarks to account for worst case weight and possbily refund the excess.

⚠️ Breaking Change ⚠️

❗ Block Underutilization

The PR changes certain staking benchmark which has a direct effect on how many of these transaction types can fit in a block.
Note that this causes block underutilization since we overestimate the weight for these extrinsics.

❗ `MAX_CANDIDATES` benchmark limit

The staking benchmarks utilize a theoretical limit of 200 max candidates, which at the moment IS NOT enforced in the code. However, this is scheduled to be included soon in a subsequent PR. This is purely to compute the

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

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

What value does it bring to the blockchain users?

nbaztec · 2023-05-30T14:20:22Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `27979`
+		//  Estimated: `193037`
+		// Minimum execution time: 131_740_000 picoseconds.
+		Weight::from_parts(134_429_600, 193037)


significant 2x ref time, 6x pov_size

Did ref time go up because the benchmark is SCALE decoding more items? Or is there more to it than that?

nbaztec · 2023-05-30T14:21:05Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `37308`
+		//  Estimated: `330557`
+		// Minimum execution time: 168_083_000 picoseconds.
+		Weight::from_parts(168_083_000, 330557)


significant 2x ref time, 10x pov_size

nbaztec · 2023-05-30T14:21:25Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `29930`
+		//  Estimated: `238138`
+		// Minimum execution time: 137_954_000 picoseconds.
+		Weight::from_parts(137_954_000, 238138)


significant 4x ref time, 4x pov_size

nbaztec · 2023-05-30T14:23:21Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `48167`
+		//  Estimated: `426257`
+		// Minimum execution time: 275_279_000 picoseconds.
+		Weight::from_parts(275_279_000, 426257)


significant for delegate weight. May not fit in a block.

As per ts-tests we are now able to fit at most 9 delegate calls (was ~50 before).

I think this is true only for the worstcase scenario, which test ?

I added https://github.com/PureStake/moonbeam/pull/2317/files#diff-007387d2d904cb5a8973816c45ce9b11bbfc8c095d0075798fd625ef0a260141R15 which showed the max txs fit within the block.

Since we're overestimating, the block utilization is affected negatively and the test helps us visualize how much.
Results: #2317 (comment) & #2317 (comment)

nbaztec · 2023-05-30T14:25:01Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `15515`
+		//  Estimated: `37960`
+		// Minimum execution time: 55_538_000 picoseconds.
+		Weight::from_parts(58_365_791, 37960)


significant 2x ref time, 5x pov_size

nbaztec · 2023-05-30T14:25:18Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `27979`
+		//  Estimated: `193037`
+		// Minimum execution time: 131_740_000 picoseconds.
+		Weight::from_parts(134_429_600, 193037)


significant 2x ref time, 6x pov_size

nbaztec · 2023-05-30T14:25:30Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `15515`
+		//  Estimated: `37960`
+		// Minimum execution time: 56_573_000 picoseconds.
+		Weight::from_parts(58_214_753, 37960)


significant 2x ref time, 5x pov_size

nbaztec · 2023-05-30T14:26:08Z

pallets/parachain-staking/src/weights.rs

+		//  Measured:  `29930`
+		//  Estimated: `238138`
+		// Minimum execution time: 137_954_000 picoseconds.
+		Weight::from_parts(137_954_000, 238138)


significant 4x ref time, 4x pov_size

notlesh · 2023-05-30T19:08:59Z

pallets/parachain-staking/src/lib.rs

@@ -1133,64 +1075,32 @@ pub mod pallet {

 		/// Temporarily leave the set of collator candidates without unbonding
 		#[pallet::call_index(11)]
-		#[pallet::weight(<T as Config>::WeightInfo::go_offline())]
+		#[pallet::weight(<T as Config>::WeightInfo::go_offline(1_000))]


What's the long term solution for this limit? We could define yet-another runtime config for the max, then it could be safely enforced and used as an upper bound here.

You could do the same but without a runtime constant (just define a complile-time one). I would suggest using a constant here instead of a magic number anyway.

The question is: do we want to enforce it when joining candidates? It could be used as a DoS vector...

Yeah that would be my choice as well.

I would enforce it when joining, but we have to clearly state that limit in the breaking changes

I added https://github.com/PureStake/moonbeam/pull/2317/files#diff-007387d2d904cb5a8973816c45ce9b11bbfc8c095d0075798fd625ef0a260141R15 which showed the max txs fit within the block.

Since we're overestimating, the block utilization is affected negatively and the test helps us visualize how much.
Results: #2317 (comment) & #2317 (comment)

notlesh · 2023-05-30T19:33:45Z

pallets/parachain-staking/src/weights.rs

+			.saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(x.into())))
+			.saturating_add(T::DbWeight::get().writes(1_u64))
+			.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(x.into())))
+			.saturating_add(Weight::from_parts(0, 31269).saturating_mul(x.into()))


I think 31269.saturating_mul(x) is going to be problematic here because x = 100 in our runtimes and 31_269 * 100 is way too close to our block limit.

One thing to note is that, we have deprecated these methods in favor of batch schedule revoke, so perhaps it's alright for it to have a high enough weight to incentivize people to move away from it.

Additionally now that I notice it this functionality has already passed 6 months of time and should be marked for removal #1760

github-actions · 2023-05-31T14:19:03Z

Coverage generated "Mon Jun 5 11:34:18 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2317/html/index.html

Master coverage: 70.87%
Pull coverage: 70.91%

* Properly use scaling parameter in benchmarks * Include updated weights * Undo execute_leave_delegators_worst changes

nbaztec · 2023-06-01T14:50:10Z

@notlesh I added the benchmark test (conditional). Let me know if there's any other extrinsics you'd wanna see up there

notlesh · 2023-06-01T20:45:41Z

@notlesh I added the benchmark test (conditional). Let me know if there's any other extrinsics you'd wanna see up there

Interesting, I got slightly different results -- maybe it is highly dependent on hardware (which wouldn't surprise me):

notlesh · 2023-06-01T21:05:54Z

tests/tests/test-staking/test-weights.ts

+  cb: (context: DevTestContext) => Promise<void>
+) {
+  describeDevMoonbeam(`${title} - ${method}`, (context) => {
+    it("should fit minimum 2", async function () {


We could parameterize this (2) and make these a permanent test. It might add a lot of time to the tests, though.

There's a conditional env variable STAKING_BENCHMARK=1 that is checked whether or not to run this test, but I'm all up for parameterizing if it adds value.

notlesh · 2023-06-01T21:54:52Z

f4a91db adds weight and proof normal class utilization metrics to the output. It's pretty telling:

Notice that weight is very underutilized, and also that scheduleLeaveDelegators doesn't seem to use either metric fully.

…onbeam into nish-staking-new-benchmarks

nbaztec · 2023-06-02T09:53:28Z

f4a91db adds weight and proof normal class utilization metrics to the output. It's pretty telling:

Notice that weight is very underutilized, and also that scheduleLeaveDelegators doesn't seem to use either metric fully.

Yeah this is a common theme here, certain "old" extrinsics do not have a weigh hint parameter and were historically underestimating the weight. Currently we assume in the upfront weight that a delegator is having MaxDelegationsPerDelegator which we refund afterwards, and also that all of these delegations are towards collators that have their scheduled requests (Top + Bottom) full upto the brim.

crystalin

Looks good.
Can you answer some of the questions left please?
Also please open a ticket for each follow-up tasks that are mentioned in the discussion or the code.

Awesome work !

tests/tests/test-staking/test-rewards-auto-compound.ts

crystalin · 2023-06-05T08:04:39Z

tests/tests/test-staking/test-staking-consts.ts

@@ -59,7 +59,14 @@ describeDevMoonbeam("Staking - Consts - MaxDelegationsPerDelegator", (context) =
        context.createBlock(
          randomCandidatesChunk.map((randomCandidate) =>
            context.polkadotApi.tx.parachainStaking
-              .delegate(randomCandidate.address, MIN_GLMR_DELEGATOR, 1, maxDelegationsPerDelegator)
+              .delegateWithAutoCompound(


Any reason this was changed to autocompound and not the other delegates (like the previous ) ?

Probably missed it changing it back, initially the chunks were failing with Transaction Ancient error since the txs were generated before the createBlock and we could only fit at most 8, so I was instead using delegateWithAutoCompound since the delegate method is set for removal now (I didn't wanna remove it in this PR). I can change it back and generate tx within the chunk but it's pretty inconsequential since the next RT should effectively swap out all delegate calls with delegateWithAutoCompound

nbaztec · 2023-06-05T09:20:18Z

Created

https://opslayer.atlassian.net/browse/MOON-2433 for enforcing MaxCandidates limit
https://opslayer.atlassian.net/browse/MOON-2434 for removing leave delegator extrinsic
https://opslayer.atlassian.net/browse/MOON-2435 for removing delegate in 6 months.

/cc @notlesh think I was wrong we never marked delegate as deprecated so we unfortunately cannot remove it soon, do you see any issues related to removing it in December?

notlesh · 2023-06-05T14:47:11Z

do you see any issues related to removing it in December?

Seems fine to me

nbaztec added 2 commits May 26, 2023 12:35

try a way

749ad66

use new benchmarks

74f3650

nbaztec added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited breaking Needs to be mentioned in breaking changes labels May 29, 2023

nbaztec added 4 commits May 29, 2023 21:11

remove unused benchmark

7086a6c

fmt and remove invalid benchmark

446e216

fmt

39d0584

use generated weights

ffd40ab

librelois mentioned this pull request May 30, 2023

runtime-2400 release #2319

Closed

24 tasks

fix tests

77cad9a

nbaztec commented May 30, 2023

View reviewed changes

notlesh reviewed May 30, 2023

View reviewed changes

nbaztec added 3 commits May 31, 2023 12:45

use delegateWithAutoCompound to fix tests

6d2c145

fix ts tests

0ebac16

fix rust tests

0f49e85

notlesh mentioned this pull request May 31, 2023

Fix delegation benchmarks #2323

Merged

notlesh and others added 2 commits May 31, 2023 20:49

Fix delegation benchmarks (#2323)

26cd185

* Properly use scaling parameter in benchmarks * Include updated weights * Undo execute_leave_delegators_worst changes

add staking weight benchmark conditional test

d0c058a

make DelegationScheduledRequests BoundedVec

db89ed8

notlesh reviewed Jun 1, 2023

View reviewed changes

Print weight and proof utilization

f4a91db

notlesh and others added 3 commits June 1, 2023 21:56

prettier

f198520

fix tests

ed7c5b4

Merge branch 'nish-staking-new-benchmarks' of github.com:PureStake/mo…

3860a90

…onbeam into nish-staking-new-benchmarks

crystalin approved these changes Jun 5, 2023

View reviewed changes

deprecate delegate

b999ed0

move comment

572f754

notlesh approved these changes Jun 5, 2023

View reviewed changes

crystalin merged commit 3e0c95c into master Jun 6, 2023

crystalin deleted the nish-staking-new-benchmarks branch June 6, 2023 09:53

crystalin changed the title ~~staking new benchmarks~~ Improves benchmarks for pallet parachain-staking Jun 15, 2023

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 Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves benchmarks for pallet parachain-staking #2317

Improves benchmarks for pallet parachain-staking #2317

nbaztec commented May 29, 2023 •

edited

Loading

nbaztec May 30, 2023

notlesh May 30, 2023

nbaztec May 30, 2023

nbaztec May 30, 2023

nbaztec May 30, 2023

nbaztec May 31, 2023

crystalin Jun 5, 2023

nbaztec Jun 5, 2023

nbaztec May 30, 2023

nbaztec May 30, 2023

nbaztec May 30, 2023

nbaztec May 30, 2023

notlesh May 30, 2023

notlesh May 30, 2023

nbaztec May 31, 2023

crystalin Jun 5, 2023

nbaztec Jun 5, 2023

notlesh May 30, 2023

nbaztec May 31, 2023

github-actions bot commented May 31, 2023 •

edited

Loading

nbaztec commented Jun 1, 2023

notlesh commented Jun 1, 2023

notlesh Jun 1, 2023

nbaztec Jun 2, 2023

notlesh commented Jun 1, 2023 •

edited

Loading

nbaztec commented Jun 2, 2023

crystalin left a comment

crystalin Jun 5, 2023

nbaztec Jun 5, 2023

nbaztec commented Jun 5, 2023

notlesh commented Jun 5, 2023

Improves benchmarks for pallet parachain-staking #2317

Improves benchmarks for pallet parachain-staking #2317

Conversation

nbaztec commented May 29, 2023 • edited Loading

What does it do?

⚠️ Breaking Change ⚠️

❗ Block Underutilization

❗ MAX_CANDIDATES benchmark limit

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

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

What value does it bring to the blockchain users?

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2023 • edited Loading

nbaztec commented Jun 1, 2023

notlesh commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notlesh commented Jun 1, 2023 • edited Loading

nbaztec commented Jun 2, 2023

crystalin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbaztec commented Jun 5, 2023

notlesh commented Jun 5, 2023

nbaztec commented May 29, 2023 •

edited

Loading

❗ `MAX_CANDIDATES` benchmark limit

github-actions bot commented May 31, 2023 •

edited

Loading

notlesh commented Jun 1, 2023 •

edited

Loading