Follow up work on `TransactionExtension` - fix weights and clean up `UncheckedExtrinsic` #6418

georgepisaltu · 2024-11-08T15:18:08Z

Follow up to #3685
Partially fixes #6403

The main PR introduced bare support for the new extension version byte as well as extension weights and benchmarking.

This PR:

Removes the redundant extension version byte from the signed v4 extrinsic, previously unused and defaulted to 0.
Adds the extension version byte to the inherited implication passed to General transactions.
Whitelists the pallet_authorship::Author, frame_system::Digest and pallet_transaction_payment::NextFeeMultiplier storage items as they are read multiple times by extensions for each transaction, but are hot in memory and currently overestimate the weight.
Whitelists the benchmark caller for CheckEra and CheckGenesis as the reads are performed for every transaction and overestimate the weight.
Updates the umbrella frame weight template to work with the system extension changes.
Plans on re-running the benchmarks at least for the frame_system extensions.

Signed-off-by: georgepisaltu <[email protected]>

georgepisaltu · 2024-11-08T15:59:20Z

bot bench-all substrate -v PIPELINE_SCRIPTS_REF=george/substrate-ext

command-bot · 2024-11-08T15:59:26Z

@georgepisaltu https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7726182 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --target_dir=substrate. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 67-8250ce9d-3053-4ab9-bb93-914cdeaedc3a to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-11-08T21:44:18Z

@georgepisaltu Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --target_dir=substrate has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7726182 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7726182/artifacts/download.

gui1117

Looks good to me, but one comment

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

Signed-off-by: georgepisaltu <[email protected]>

ggwpez · 2024-11-13T05:37:02Z

substrate/frame/verify-signature/src/weights.rs

 //! WASM-EXECUTION: `Compiled`, CHAIN: `Some("dev")`, DB CACHE: `1024`

 // Executed Command:
-// ./target/debug/substrate-node


Lol these weights were def wrong.

Yes it's a new and currently unused extension introduced in #3685 and those weights were generated on my machine just to make sure the benchmarks work.

ggwpez · 2024-11-13T06:05:58Z

substrate/frame/staking/src/weights.rs

+			.saturating_add(Weight::from_parts(155_577_516, 0).saturating_mul(v.into()))
+			.saturating_add(T::DbWeight::get().reads(9010_u64))
+			.saturating_add(T::DbWeight::get().reads((9_u64).saturating_mul(v.into())))
+			.saturating_add(T::DbWeight::get().writes(7008_u64))


9000 READs and 7000 WRITEs per iteration is a lot. Not sure if this is expected. @Ank4n @gpestana

good find. this is indeed an error: https://github.com/paritytech/polkadot-sdk/pull/6025/files#r1839635371

I didn't review the weight carefully I will do another review.

a PR is opened: #6463

I normally use this tool when comparing weights (have to run it locally when using forks).
It has a CLI and web version. Outputs for proof-size.pdf and ref-time.pdf. Url would be this for the self-hosted version.

substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs

gui1117 · 2024-11-13T07:58:25Z

substrate/frame/broker/src/weights.rs

-		// Minimum execution time: 229_000 picoseconds.
-		Weight::from_parts(268_000, 0)
+		// Minimum execution time: 0_000 picoseconds.
+		Weight::from_parts(0, 0)


enable_auto_renew went from quite a lot to zero.

It was reading into Authorship::Author and System::Digest, which are now both whitelisted, but that shouldn't have anything to do with this result.

Possibly one of the origins is set to NeverEnsure and triggers the Weightless path here:

polkadot-sdk/substrate/frame/broker/src/benchmarking.rs

Line 1032 in f170af6

.map_err(|_| BenchmarkError::Weightless)?;

gui1117 · 2024-11-13T08:01:48Z

substrate/frame/broker/src/weights.rs

-	/// Storage: `Broker::Workplan` (r:0 w:1)
-	/// Proof: `Broker::Workplan` (`max_values`: None, `max_size`: Some(1216), added: 3691, mode: `MaxEncodedLen`)
-	fn enable_auto_renew() -> Weight {
+	fn disable_auto_renew() -> Weight {


disable_auto_renew went from little to zero.

gui1117 · 2024-11-13T08:50:49Z

substrate/frame/nomination-pools/src/weights.rs

+		//  Measured:  `0`
+		//  Estimated: `0`
+		// Minimum execution time: 2_846_000 picoseconds.
+		Weight::from_parts(3_058_000, 0)


a bit surprising that this call do 0 read and writes.

there is a call apply_slash that allows a reporter to apply a pending slash to a member account. It is quite unexpected that the operation doesn't read and write anything considering it seems possible the reporter is unrelated to the slashed account.

gui1117 · 2024-11-13T08:54:50Z

substrate/frame/nomination-pools/src/weights.rs

+		//  Measured:  `0`
+		//  Estimated: `0`
+		// Minimum execution time: 2_858_000 picoseconds.
+		Weight::from_parts(3_010_000, 0)


Also surprising that applying a slash doesn't read any storage.

gui1117 · 2024-11-13T08:56:24Z

substrate/frame/nomination-pools/src/weights.rs

-		// Minimum execution time: 759_000 picoseconds.
-		Weight::from_parts(819_000, 0)
+		// Minimum execution time: 2_719_000 picoseconds.
+		Weight::from_parts(2_828_000, 0)


Also surprising that this operation doesn't read or write anything.
The caller is not related to the operation.

gui1117 · 2024-11-13T08:56:50Z

substrate/frame/nomination-pools/src/weights.rs

+		//  Measured:  `0`
+		//  Estimated: `0`
+		// Minimum execution time: 2_776_000 picoseconds.
+		Weight::from_parts(2_987_000, 0)


Also surprising that this operation doesn't read or write anything.
The caller is not related to the operation.

Signed-off-by: georgepisaltu <[email protected]>

serban300

The bridge modifications look good

Signed-off-by: georgepisaltu <[email protected]>

…UncheckedExtrinsic` (#6418) Follow up to #3685 Partially fixes #6403 The main PR introduced bare support for the new extension version byte as well as extension weights and benchmarking. This PR: - Removes the redundant extension version byte from the signed v4 extrinsic, previously unused and defaulted to 0. - Adds the extension version byte to the inherited implication passed to `General` transactions. - Whitelists the `pallet_authorship::Author`, `frame_system::Digest` and `pallet_transaction_payment::NextFeeMultiplier` storage items as they are read multiple times by extensions for each transaction, but are hot in memory and currently overestimate the weight. - Whitelists the benchmark caller for `CheckEra` and `CheckGenesis` as the reads are performed for every transaction and overestimate the weight. - Updates the umbrella frame weight template to work with the system extension changes. - Plans on re-running the benchmarks at least for the `frame_system` extensions. --------- Signed-off-by: georgepisaltu <[email protected]> Co-authored-by: command-bot <> Co-authored-by: gui <[email protected]>

georgepisaltu added 3 commits November 8, 2024 16:30

Remove stale extenstion version from v4 struct

3eee059

Signed-off-by: georgepisaltu <[email protected]>

Whitelist storage items commonly read every block

c04fffa

Signed-off-by: georgepisaltu <[email protected]>

Update substrate weight template

2e88d75

Signed-off-by: georgepisaltu <[email protected]>

georgepisaltu added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Nov 8, 2024

georgepisaltu self-assigned this Nov 8, 2024

georgepisaltu requested a review from a team as a code owner November 8, 2024 15:18

georgepisaltu mentioned this pull request Nov 8, 2024

Remove stale extension version from UncheckedExtrinsic v4 structure #6404

Closed

".git/.scripts/commands/bench-all/bench-all.sh" --target_dir=substrate

4dfe581

paritytech-ci requested review from acatangiu and cheme as code owners November 8, 2024 21:44

gui1117 approved these changes Nov 10, 2024

View reviewed changes

substrate/frame/system/src/extensions/weights.rs Show resolved Hide resolved

substrate/frame/system/src/extensions/weights.rs Show resolved Hide resolved

georgepisaltu and others added 13 commits November 11, 2024 11:43

Fix weight compilation

c3bf0e8

Signed-off-by: georgepisaltu <[email protected]>

More granular whitelist in sys extensions

d6c999c

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into second-tx-ext-fix

25bcc85

Add prdoc

a54f916

Signed-off-by: georgepisaltu <[email protected]>

Typo in prdoc

6287213

Signed-off-by: georgepisaltu <[email protected]>

Other typos

3dfe258

Signed-off-by: georgepisaltu <[email protected]>

Replace proxy and multisig weights

85cf99f

Signed-off-by: georgepisaltu <[email protected]>

Include ext version into inherited implication

48cfdea

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into second-tx-ext-fix

e927eed

Fix prdoc

eec5d7a

Signed-off-by: georgepisaltu <[email protected]>

fix tests

dcb2b14

Fix authorization example tests

65e741d

Signed-off-by: georgepisaltu <[email protected]>

Fix verify signature tests

103fb7b

Signed-off-by: georgepisaltu <[email protected]>

ggwpez reviewed Nov 13, 2024

View reviewed changes

gui1117 reviewed Nov 13, 2024

View reviewed changes

ggwpez approved these changes Nov 13, 2024

View reviewed changes

georgepisaltu added 4 commits November 13, 2024 11:40

Merge remote-tracking branch 'upstream/master' into second-tx-ext-fix

dbf227a

Revert broker weight update

31aac7d

Signed-off-by: georgepisaltu <[email protected]>

Revert nomination pools weights update

cb0bf2e

Signed-off-by: georgepisaltu <[email protected]>

Revert staking weights update

c1e8e50

Signed-off-by: georgepisaltu <[email protected]>

serban300 approved these changes Nov 13, 2024

View reviewed changes

georgepisaltu added 3 commits November 13, 2024 16:05

Merge remote-tracking branch 'upstream/master' into second-tx-ext-fix

a349e34

Signed-off-by: georgepisaltu <[email protected]>

Fix typo

f925ef8

Signed-off-by: georgepisaltu <[email protected]>

Merge remote-tracking branch 'upstream/master' into second-tx-ext-fix

5cd03c8

pgherveou approved these changes Nov 13, 2024

View reviewed changes

georgepisaltu enabled auto-merge November 13, 2024 15:44

Merge branch 'master' into second-tx-ext-fix

9c4c7e9

davidk-pt approved these changes Nov 14, 2024

View reviewed changes

xermicus approved these changes Nov 14, 2024

View reviewed changes

georgepisaltu added this pull request to the merge queue Nov 14, 2024

Merged via the queue into paritytech:master with commit ae4b68b Nov 14, 2024
191 of 199 checks passed

georgepisaltu deleted the second-tx-ext-fix branch November 14, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up work on `TransactionExtension` - fix weights and clean up `UncheckedExtrinsic` #6418

Follow up work on `TransactionExtension` - fix weights and clean up `UncheckedExtrinsic` #6418

georgepisaltu commented Nov 8, 2024 •

edited

Loading

georgepisaltu commented Nov 8, 2024

command-bot bot commented Nov 8, 2024 •

edited

Loading

command-bot bot commented Nov 8, 2024

gui1117 left a comment

ggwpez Nov 13, 2024

georgepisaltu Nov 13, 2024

ggwpez Nov 13, 2024 •

edited

Loading

gui1117 Nov 13, 2024 •

edited

Loading

ggwpez Nov 13, 2024 •

edited

Loading

gui1117 Nov 13, 2024

georgepisaltu Nov 13, 2024

ggwpez Nov 13, 2024

gui1117 Nov 13, 2024

gui1117 Nov 13, 2024

gui1117 Nov 13, 2024

gui1117 Nov 13, 2024

gui1117 Nov 13, 2024

serban300 left a comment

Follow up work on TransactionExtension - fix weights and clean up UncheckedExtrinsic #6418

Follow up work on TransactionExtension - fix weights and clean up UncheckedExtrinsic #6418

Conversation

georgepisaltu commented Nov 8, 2024 • edited Loading

georgepisaltu commented Nov 8, 2024

command-bot bot commented Nov 8, 2024 • edited Loading

command-bot bot commented Nov 8, 2024

gui1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

gui1117 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

ggwpez Nov 13, 2024 • edited Loading

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

serban300 left a comment

Choose a reason for hiding this comment

Follow up work on `TransactionExtension` - fix weights and clean up `UncheckedExtrinsic` #6418

Follow up work on `TransactionExtension` - fix weights and clean up `UncheckedExtrinsic` #6418

georgepisaltu commented Nov 8, 2024 •

edited

Loading

command-bot bot commented Nov 8, 2024 •

edited

Loading

ggwpez Nov 13, 2024 •

edited

Loading

gui1117 Nov 13, 2024 •

edited

Loading

ggwpez Nov 13, 2024 •

edited

Loading