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

[gui1117]

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]

/cmd prdoc --audience runtime-dev

[github-actions]

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

[athei]

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

[athei]

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

[georgepisaltu]

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]

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

[gui1117]

done in e988d2b

[georgepisaltu]

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]

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

[athei]

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]

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]

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]

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]

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]

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