CheckWeight: account for extrinsic len as proof size #4765

sandreim · 2024-06-11T17:49:34Z

Fix #4743 which allows us to remove the defensive limit on pov size in Cumulus after relay chain gets upgraded with these changes. Also add unit test to ensure CheckWeight - StorageWeightReclaim integration works.

TODO:

PRDoc
Add a len to all the other tests in storage weight reclaim and call CheckWeight::pre_dispatch

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

cumulus/client/consensus/aura/src/collators/basic.rs

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

eskimor

Looks sensible.

cumulus/primitives/storage-weight-reclaim/src/lib.rs

eskimor · 2024-06-12T07:49:47Z

substrate/frame/system/src/extensions/check_weight.rs

+	let extrinsic_weight = info
+		.weight
+		.saturating_add(maximum_weight.get(info.class).base_extrinsic)
+		.saturating_add(Weight::from_parts(0, len as u64));


Stupid question: Is this also used for inherents? (Does this have any influence on weights on the relaychain as relevant to parachain consensus?)

Specifically: We already add lengths to the weight in paras inherent for checking within the paras inherent to not exceed the weight e.g. here. I hope we are not counting it twice now?

I don't understand what is the reasoning is for this. Why are we setting the proof size there? On relay chains the maximum proof size is U64::MAX AFAIK, effectively disabling any proof size related checks.

Good point @skunert . I think we should remove and clean the code in parachain inherent. Proof size is indeed only relevant for parachain runtimes.

Do we not need this excape hatch for the Mandatory inherents to avoid what happend on the rococo parachains?

What happened on Rococo parachains ?

It got stuck because an inherent ran a lazy migration that loaded a big Key pair and overflowed the PoV limit.

@ggwpez Sorry for the late reply, I thought about the rococo situation before approval here.

So we don't have an explicit mechanism for mandatory extrinsics here because we will go into these two branches dring the weight checks:

This one because for mandatory max_total is None:

polkadot-sdk/substrate/frame/system/src/extensions/check_weight.rs

Line 206 in 0bb931b

_ => {},

This one because reserved is also None for mandatory extrinsics:

polkadot-sdk/substrate/frame/system/src/extensions/check_weight.rs

Lines 222 to 224 in 0bb931b

// There is either no limit in reserved pool (`None`),

// or we are below the limit.

_ => {},

Of course what I described above is not guaranteed. Parachain teams can define in their runtime whatever they want. This is what the typical weight definition looks like (note no specifics defined for mandatory):

polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

Lines 138 to 155 in 0bb931b

pub RuntimeBlockWeights: BlockWeights = BlockWeights::builder()

.base_block(BlockExecutionWeight::get())

.for_class(DispatchClass::all(), |weights| {

weights.base_extrinsic = ExtrinsicBaseWeight::get();

})

.for_class(DispatchClass::Normal, |weights| {

weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT);

})

.for_class(DispatchClass::Operational, |weights| {

weights.max_total = Some(MAXIMUM_BLOCK_WEIGHT);

// Operational transactions have some extra reserved space, so that they

// are included even if block reached `MAXIMUM_BLOCK_WEIGHT`.

weights.reserved = Some(

MAXIMUM_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT

);

})

.avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO)

.build_or_panic();

The builder has None set by default, which makes sense:

polkadot-sdk/substrate/frame/system/src/limits.rs

Lines 347 to 354 in 0bb931b

let initial =

if class == DispatchClass::Mandatory { None } else { Some(Weight::zero()) };

WeightsPerClass {

base_extrinsic: constants::ExtrinsicBaseWeight::get(),

max_extrinsic: None,

max_total: initial,

reserved: initial,

}

So with this PR the weight checking falls in line with the rest of the extrinsics. If someone has actually set a limit on mandatory weight class, they are living dangerous without the changes of this PR already. If some pallet returns max weight like the session pallet in the rococo case, we would already go over the limit together with on_initialize.

Okay i see. Thanks for the elaborate explanation!

cumulus/client/consensus/aura/src/collators/basic.rs

cumulus/primitives/storage-weight-reclaim/src/lib.rs

This reverts commit 2211d15. Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

paritytech-cicd-pr · 2024-06-12T11:44:49Z

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

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

skunert

Looks good, thank you!

Glutton currently is useful mostly for stress testing relay chain validators. It is unusable for testing the collator networking and block announcement and import scenarios. This PR resolves that by improving glutton pallet to also buff up the blocks, up to the runtime configured `BlockLength`. ### How it works Includes an additional inherent in each parachain block. The `garbage` argument passed to the inherent is filled with trash data. It's size is computed by applying the newly introduced `block_length` percentage to the maximum block length for mandatory dispatch class. After #4765 is merged, the length of inherent extrinsic will be added to the total block proof size. The remaining weight is burnt in `on_idle` as configured by the `storage` percentage parameter. TODO: - [x] PRDoc - [x] Readme update - [x] Add tests --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Fix paritytech#4743 which allows us to remove the defensive limit on pov size in Cumulus after relay chain gets upgraded with these changes. Also add unit test to ensure `CheckWeight` - `StorageWeightReclaim` integration works. TODO: - [x] PRDoc - [x] Add a len to all the other tests in storage weight reclaim and call `CheckWeight::pre_dispatch` --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Glutton currently is useful mostly for stress testing relay chain validators. It is unusable for testing the collator networking and block announcement and import scenarios. This PR resolves that by improving glutton pallet to also buff up the blocks, up to the runtime configured `BlockLength`. ### How it works Includes an additional inherent in each parachain block. The `garbage` argument passed to the inherent is filled with trash data. It's size is computed by applying the newly introduced `block_length` percentage to the maximum block length for mandatory dispatch class. After paritytech#4765 is merged, the length of inherent extrinsic will be added to the total block proof size. The remaining weight is burnt in `on_idle` as configured by the `storage` percentage parameter. TODO: - [x] PRDoc - [x] Readme update - [x] Add tests --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim added 2 commits June 11, 2024 19:10

Account for block lentht as storage proof weight

49f799a

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

collators: use 100% block limits

2211d15

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jun 11, 2024

sandreim requested a review from a team as a code owner June 11, 2024 17:49

sandreim requested review from skunert and bkchr June 11, 2024 17:56

sandreim added 2 commits June 11, 2024 20:57

fmt

e1f9786

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

add prdoc

8b9eda7

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim commented Jun 11, 2024

View reviewed changes

cumulus/client/consensus/aura/src/collators/basic.rs Outdated Show resolved Hide resolved

sandreim added 2 commits June 11, 2024 21:09

fix prdoc

18e481c

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

damage

ef2c858

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

eskimor approved these changes Jun 12, 2024

View reviewed changes

skunert reviewed Jun 12, 2024

View reviewed changes

cumulus/client/consensus/aura/src/collators/basic.rs Outdated Show resolved Hide resolved

cumulus/primitives/storage-weight-reclaim/src/lib.rs Outdated Show resolved Hide resolved

cumulus/primitives/storage-weight-reclaim/src/lib.rs Outdated Show resolved Hide resolved

sandreim added 2 commits June 12, 2024 12:27

Revert "collators: use 100% block limits"

010c33f

This reverts commit 2211d15. Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

adjust PRDoc

c249767

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim changed the title ~~Cumulus: collate with max pov size (remove 50% limit)~~ CheckWeight: account for extrinsic len as proof size Jun 12, 2024

sandreim added 4 commits June 12, 2024 12:55

get_storage_weight in tests

25d7404

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

call CheckWeight:pre_dispatch in all tests

cf9b8d6

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

address comments

4e10b62

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

I once again fixed the PRDoc

89565b0

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim added 2 commits June 12, 2024 17:18

fix executive tests

cf873ef

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

just tests, no bumps

1b985aa

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim mentioned this pull request Jun 12, 2024

glutton: also increase parachain block length #4728

Merged

3 tasks

skunert approved these changes Jun 12, 2024

View reviewed changes

ggwpez approved these changes Jun 14, 2024

View reviewed changes

ggwpez self-requested a review June 14, 2024 11:56

sandreim added this pull request to the merge queue Jun 14, 2024

Merged via the queue into master with commit ae0b3bf Jun 14, 2024
162 checks passed

sandreim deleted the sandreim/block_len_weight branch June 14, 2024 13:15

sandreim mentioned this pull request Jun 17, 2024

Test how many fully loaded cores we can support with multiple collators #4696

Open

3 tasks

librelois mentioned this pull request Aug 12, 2024

Record the storage proof_size with hostfunction polkadot-evm/frontier#1490

Draft

This was referenced Aug 21, 2024

Update polkadot-sdk from v1.11.0 to stable2407 moondance-labs/tanssi#659

Open

Update polkadot-sdk from v1.11.0 to stable2407 moonbeam-foundation/moonbeam#2912

Closed

TDemeco mentioned this pull request Oct 7, 2024

feat: ⏫ upgrade to Polkadot SDK v1.14.0 Moonsong-Labs/storage-hub#218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CheckWeight: account for extrinsic len as proof size #4765

CheckWeight: account for extrinsic len as proof size #4765

sandreim commented Jun 11, 2024 •

edited

Loading

eskimor left a comment

eskimor Jun 12, 2024

eskimor Jun 12, 2024

skunert Jun 12, 2024

sandreim Jun 12, 2024

ggwpez Jun 14, 2024

sandreim Jun 14, 2024

ggwpez Jun 17, 2024

skunert Jun 20, 2024

ggwpez Jun 20, 2024

paritytech-cicd-pr commented Jun 12, 2024

skunert left a comment

	// There is either no limit in reserved pool (`None`),
	// or we are below the limit.
	_ => {},

	pub RuntimeBlockWeights: BlockWeights = BlockWeights::builder()
	.base_block(BlockExecutionWeight::get())
	.for_class(DispatchClass::all(), \|weights\| {
	weights.base_extrinsic = ExtrinsicBaseWeight::get();
	})
	.for_class(DispatchClass::Normal, \|weights\| {
	weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT);
	})
	.for_class(DispatchClass::Operational, \|weights\| {
	weights.max_total = Some(MAXIMUM_BLOCK_WEIGHT);
	// Operational transactions have some extra reserved space, so that they
	// are included even if block reached `MAXIMUM_BLOCK_WEIGHT`.
	weights.reserved = Some(
	MAXIMUM_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT
	);
	})
	.avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO)
	.build_or_panic();

	let initial =
	if class == DispatchClass::Mandatory { None } else { Some(Weight::zero()) };
	WeightsPerClass {
	base_extrinsic: constants::ExtrinsicBaseWeight::get(),
	max_extrinsic: None,
	max_total: initial,
	reserved: initial,
	}

CheckWeight: account for extrinsic len as proof size #4765

CheckWeight: account for extrinsic len as proof size #4765

Conversation

sandreim commented Jun 11, 2024 • edited Loading

eskimor 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

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

paritytech-cicd-pr commented Jun 12, 2024

skunert left a comment

Choose a reason for hiding this comment

sandreim commented Jun 11, 2024 •

edited

Loading