`set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. #7327

gui1117 · 2025-01-24T14:25:23Z

Related #6772

For an extrinsic, in the post dispatch info, the actual weight is only used to reclaim unused weight. If the actual weight is more than the pre dispatch weight, then the extrinsic is using the minimum, e.g., the weight used registered in pre dispatch.

In parachain-system pallet one call is set_validation_data. This call is returning an actual weight, but the pre-dispatch weight is 0.

This PR fix the disregard of actual weight of set_validation_data by registering it manually.

gui1117 · 2025-01-24T14:29:14Z

/cmd prdoc --audience runtime-dev

github-actions · 2025-01-24T14:31:38Z

Command "prdoc --audience runtime-dev" has failed ❌! See logs here

athei

Thank you. I can confirm that the try-runtime errors are gone.

athei · 2025-01-24T14:46:36Z

cumulus/pallets/parachain-system/src/lib.rs

@@ -692,7 +692,12 @@ pub mod pallet {
 				vfp.relay_parent_number,
 			));

-			Ok(PostDispatchInfo { actual_weight: Some(total_weight), pays_fee: Pays::No })
+			frame_system::Pallet::<T>::register_extra_weight_unchecked(
+				total_weight,


I assume we don't know an upper bound for this value pre-dispatch because it is calculated ad-hoc?

Yes but it's once per block and we register the weight, meaning it will be correctly accounted for in the following transactions.

If we go with the worst case weight here before dispatching, we might greatly underestimate the block space we have since those stuctures in ParachainInherentData can grow a lot as far as I'm aware. This model works best IMO.

athei · 2025-01-24T14:47:46Z

cumulus/pallets/parachain-system/src/lib.rs

+				DispatchClass::Mandatory,
+			);
+
+			Ok(Pays::No.into())


Nit: You can change the return type of the dispatchable to just DispatchResult.

done in e988d2b

georgepisaltu · 2025-01-24T15:04:05Z

cumulus/pallets/parachain-system/src/lib.rs

@@ -692,7 +692,12 @@ pub mod pallet {
 				vfp.relay_parent_number,
 			));

-			Ok(PostDispatchInfo { actual_weight: Some(total_weight), pays_fee: Pays::No })
+			frame_system::Pallet::<T>::register_extra_weight_unchecked(
+				total_weight,


Yes but it's once per block and we register the weight, meaning it will be correctly accounted for in the following transactions.

If we go with the worst case weight here before dispatching, we might greatly underestimate the block space we have since those stuctures in ParachainInherentData can grow a lot as far as I'm aware. This model works best IMO.

ggwpez · 2025-01-24T15:25:29Z

cumulus/pallets/parachain-system/src/lib.rs

+				DispatchClass::Mandatory,
+			);
+
+			Ok(Pays::No.into())


Does it make a difference here if we just return this vs Ok(PostDispatchInfo { actual_weight: Some(total_weight), pays_fee: Pays::No })? It would look more correct to me

That is what we were doing before (this PR removes this line) and it was incorrect. This is because you cant (or should not) return a post dispatch weight that is higher than the pre dispatch weight. It will not be charged. Hence the error that was emitted by try runtime.

skunert · 2025-01-24T16:49:57Z

This indeed gets rid of the message for set_validation_data. Another annoyance is that we also see the message on the relay chain:

2025-01-24 17:42:54.008 ERROR ⋮runtime::frame-support: [early-airport-4761] Post dispatch weight is greater than pre dispatch weight. Pre dispatch weight may underestimating the actual weight. Greater post dispatch weight components are ignored.
					Pre dispatch weight: Weight { ref_time: 15254022361, proof_size: 879 },
					Post dispatch weight: Weight { ref_time: 4812758536, proof_size: 37021 }

Here only the proof_size is larger, but relay chain does not care about it and the limit is u64::max. This is new since the print went from the storage weight reclaim extension to here:

polkadot-sdk/substrate/frame/support/src/dispatch.rs

Lines 313 to 320 in e5a9dab

    
           				log::error!( 
        
           					target: crate::LOG_TARGET, 
        
           					"Post dispatch weight is greater than pre dispatch weight. \ 
        
           					Pre dispatch weight may underestimating the actual weight. \ 
        
           					Greater post dispatch weight components are ignored. 
        
           					Pre dispatch weight: {info_total_weight:?}, 
        
           					Post dispatch weight: {actual_weight:?}", 
        
           				);

Maybe we need to move it again so that we can differentiate between para and relaychain.

athei · 2025-01-24T17:59:52Z

Do you know which dispatchable is causing that? We can just give it the same treatment. Or if it is a pallet only used on the relay chain (where PoV doesn't matter) just set pre-dispatch PoV to something super high?

bkchr · 2025-01-24T19:38:05Z

Maybe we need to move it again so that we can differentiate between para and relaychain.

Or do not print it if proof_size is infinite.

gui1117 · 2025-01-25T01:53:44Z

This is new since the print went from the storage weight reclaim extension to here:

I have kept other logs in storage weight reclaim. I just added this log because it was scary to silently ignoring post dispatch excess weight.

Another annoyance is that we also see the message on the relay chain: [..]

If the pallet is only used in relay chain and we don't want to fix the benchmark or weight calculation for the call, we can also set the proof size in the post info equal to the pre-dispatch info.

github-actions · 2025-01-25T02:22:56Z

Command help:

usage: /cmd bench [-h] [--quiet] [--clean] [--image IMAGE]
                  [--runtime [{dev,westend,rococo,asset-hub-westend,asset-hub-rococo,bridge-hub-rococo,bridge-hub-westend,collectives-westend,contracts-rococo,coretime-rococo,coretime-westend,glutton-westend,people-rococo,people-westend} ...]]
                  [--pallet [PALLET ...]] [--fail-fast]

options:
  -h, --help            show this help message and exit
  --quiet               Won't print start/end/failed messages in PR
  --clean               Clean up the previous bot's & author's comments in PR
  --image IMAGE         Override docker image '--image
                        docker.io/paritytech/ci-unified:latest'
  --runtime [{dev,westend,rococo,asset-hub-westend,asset-hub-rococo,bridge-hub-rococo,bridge-hub-westend,collectives-westend,contracts-rococo,coretime-rococo,coretime-westend,glutton-westend,people-rococo,people-westend} ...]
                        Runtime(s) space separated
  --pallet [PALLET ...]
                        Pallet(s) space separated
  --fail-fast           Fail fast on first failed benchmark

**Examples**:
 Runs all benchmarks
 /cmd bench

 Runs benchmarks for pallet_balances and pallet_multisig for all runtimes which have these pallets. **--quiet** makes it to output nothing to PR but reactions
 /cmd bench --pallet pallet_balances pallet_xcm_benchmarks::generic --quiet

 Runs bench for all pallets for westend runtime and fails fast on first failed benchmark
 /cmd bench --runtime westend --fail-fast

 Does not output anything and cleans up the previous bot's & author command triggering comments in PR
 /cmd bench --runtime westend rococo --pallet pallet_balances pallet_multisig --quiet --clean

gui1117 requested a review from skunert January 24, 2025 14:37

gui1117 added the T2-pallets This PR/Issue is related to a particular pallet. label Jan 24, 2025

gui1117 requested a review from athei January 24, 2025 14:40

athei approved these changes Jan 24, 2025

View reviewed changes

georgepisaltu approved these changes Jan 24, 2025

View reviewed changes

comment

e5a9dab

ggwpez reviewed Jan 24, 2025

View reviewed changes

gui1117 added 5 commits January 25, 2025 07:31

do not use refund when not in pre dispatch info

360d7f5

fix

a4d8a01

prdoc

26abf9a

Merge branch 'master' into gui-do-not-use-refund-when-not-benchmarked

46827d4

use dispatch result without post info

e988d2b

remove unused import

15d6c29

gui1117 enabled auto-merge January 25, 2025 01:57

remove unused

a9e8ed8

paritytech deleted a comment from github-actions bot Jan 25, 2025

paritytech deleted a comment from command-bot bot Jan 25, 2025

paritytech deleted a comment from paritytech-workflow-stopper bot Jan 25, 2025

BabakArtist approved these changes Jan 25, 2025

View reviewed changes

gui1117 added this pull request to the merge queue Jan 25, 2025

Merged via the queue into master with commit 682f8cd Jan 25, 2025
196 of 206 checks passed

gui1117 deleted the gui-do-not-use-refund-when-not-benchmarked branch January 25, 2025 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. #7327

`set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. #7327

gui1117 commented Jan 24, 2025

gui1117 commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

athei left a comment •

edited

Loading

athei Jan 24, 2025

georgepisaltu Jan 24, 2025

athei Jan 24, 2025

gui1117 Jan 25, 2025

georgepisaltu Jan 24, 2025

ggwpez Jan 24, 2025

athei Jan 24, 2025 •

edited

Loading

skunert commented Jan 24, 2025

athei commented Jan 24, 2025

bkchr commented Jan 24, 2025

gui1117 commented Jan 25, 2025

github-actions bot commented Jan 25, 2025

set_validation_data register weight manually, do not use refund when the pre dispatch is zero. #7327

set_validation_data register weight manually, do not use refund when the pre dispatch is zero. #7327

Conversation

gui1117 commented Jan 24, 2025

gui1117 commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

athei left a comment • edited Loading

Choose a reason for hiding this comment

athei Jan 24, 2025

Choose a reason for hiding this comment

georgepisaltu Jan 24, 2025

Choose a reason for hiding this comment

athei Jan 24, 2025

Choose a reason for hiding this comment

gui1117 Jan 25, 2025

Choose a reason for hiding this comment

georgepisaltu Jan 24, 2025

Choose a reason for hiding this comment

ggwpez Jan 24, 2025

Choose a reason for hiding this comment

athei Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

skunert commented Jan 24, 2025

athei commented Jan 24, 2025

bkchr commented Jan 24, 2025

gui1117 commented Jan 25, 2025

github-actions bot commented Jan 25, 2025

`set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. #7327

`set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. #7327

athei left a comment •

edited

Loading

athei Jan 24, 2025 •

edited

Loading