Implement cumulus StorageWeightReclaim as wrapping transaction extension + frame system ReclaimWeight

[gui1117]

(rebasing of #5234)

Issues:

• Transaction extensions have weights and refund weight. So the reclaiming of unused weight must happen last in the transaction extension pipeline. Currently it is inside CheckWeight.
• cumulus storage weight reclaim transaction extension misses the proof size of logic happening prior to itself.

Done:

• a new storage ExtrinsicWeightReclaimed in frame-system. Any logic which attempts to do some reclaim must use this storage to avoid double reclaim.

• a new function reclaim_weight in frame-system pallet: info and post info in arguments, read the already reclaimed weight, calculate the new unused weight from info and post info. do the more accurate reclaim if higher.

• CheckWeight is unchanged and still reclaim the weight in post dispatch

• ReclaimWeight is a new transaction extension in frame system. For solo chains it must be used last in the transactino extension pipeline. It does the final most accurate reclaim

• StorageWeightReclaim is moved from cumulus primitives into its own pallet (in order to define benchmark) and is changed into a wrapping transaction extension.
  It does the recording of proof size and does the reclaim using this recording and the info and post info. So parachains don't need to use ReclaimWeight. But also if they use it, there is no bug.

  /// The TransactionExtension to the basic transaction logic.
  pub type TxExtension = cumulus_pallet_weight_reclaim::StorageWeightReclaim<
       Runtime,
       (
               frame_system::CheckNonZeroSender<Runtime>,
               frame_system::CheckSpecVersion<Runtime>,
               frame_system::CheckTxVersion<Runtime>,
               frame_system::CheckGenesis<Runtime>,
               frame_system::CheckEra<Runtime>,
               frame_system::CheckNonce<Runtime>,
               frame_system::CheckWeight<Runtime>,
               pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
               BridgeRejectObsoleteHeadersAndMessages,
               (bridge_to_rococo_config::OnBridgeHubWestendRefundBridgeHubRococoMessages,),
               frame_metadata_hash_extension::CheckMetadataHash<Runtime>,
       ),
  >;

[bkchr]

Anyway this doesn't have any vulnerability, it just wastes resources, but people should put the CheckWeight last in the pipeline.

I would say it depends. When you are required to put CheckWeight as the latest extension, it also means that you are missing a cheap, early return.

[georgepisaltu]

you are missing a cheap, early return

Going through the pipeline should be cheap anyway. It's just extensions which are pretty light and the "wasted" work for overweight transactions should be done off-chain when validators are building their blocks.

Because the weight check isn't hardcoded and users can build whatever extension they like to handle it, we need to have some sort of convention when we introduce other weight related logic. I skimmed through the PR and I like the approach, but I won't formally approve because I didn't review thoroughly.

[gui1117]

Anyway this doesn't have any vulnerability, it just wastes resources, but people should put the CheckWeight last in the pipeline.

I would say it depends. When you are required to put CheckWeight as the latest extension, it also means that you are missing a cheap, early return.

Maybe it is time to split this transaction extension into CheckWeight and RefundWeight.

EDIT: or we can do the RefundWeight in note_applied_extrinsic.

EDIT: or we can use a storage to store the weight refunded by CheckWeight, then StorageWeightReclaim will just take this value instead of trying to guess it incorrectly.

EDIT: I decided to with a new storage ExtrinsicWeight or ExtrinsicWeightRefunded, CheckWeight will register its refund there, then StorageWeightReclaim will undo the CheckWeight operation and do the correct refund.

Later we can introduce another RefundWeight for solo-chains or parachains that doesn't want to use StorageWeightReclaim. RefundWeight can be placed at the end of the pipeline, and CheckWeight is unchanged and not breaking.

[gui1117]

I used /cmd bench --pallet frame_system_extensions to run this benchmark.
Somehow this doesn't give a chain anymore.
Is there some specific settings where giving a chain would change the result of the benchmarks?

[mordamax]

afair it's expected behavior for frame-omni-bencher
right, @ggwpez ?

btw, for now it's still better to use old command bot, as this one we still test/tweak - Here's docs for old one https://command-bot.parity-prod.parity.io/static/docs/latest.html?repo=polkadot-sdk, but maybe you can leave these results if they look fine

Once it's tested we'll notify everyone on forum about new bot

[gui1117]

Great to know, also the new /cmd seems to have missed coretime-rococo, coretime-westend, people-rococo, people-westend when I wrote /cmd bench --pallet frame_system_extensions. (I might mistaken)

[ggwpez]

Hm I think this is since we use the --runtime argument, and it is not extracting the chain spec name. But it should, good find: #6320

[gui1117]

PR should be ready for review, I updated the description

[georgepisaltu]

Great work! I'm going to approve once the questions in the comments are answered.

[georgepisaltu]

I don't think there's any case where we could allow a negative refund, in other words a post dispatch weight greater than what was estimated. If such a case ever exists, we just take the hit and don't refund anything, the code was executed and the damage was done. There are multiple places in the weight checking code that assumes that no weight increases are possible after dispatch and take the route described above.

I'd change the code to something like this:

Suggested change

	current_weight.accrue(already_reclaimed, info.class);
	current_weight.reduce(accurate_reclaim, info.class);
	let to_reclaim = accurate_reclaim.saturating_sub(already_reclaimed);
	current_weight.reduce(to_reclaim, info.class);

[gui1117]

The saturation will happen if accurate_reclaim is less than already_reclaim.
Indeed I considered better to make the ReclaimWeight authoritative and erasing previous reclaim.
But it sounds better saturate.
I will update.

This is equivalent because few lines before we do let accurate_reclaim = already_reclaimed.max(unspent);

So the saturation will never happen.

No logic change refactor: 472d63c

[georgepisaltu]

I'd find it easier to read if this was benchmarked_actual_weight or something, the current name implies it's from before dispatch.

[gui1117]

Yes, I renamed and tried to keep the concept of "actual" 5322297

[georgepisaltu]

Shouldn't we refund the overestimated proof sizes of the inner extensions? I read the function a bunch of times but I don't think we do, the proof size overestimation we'd need to refund would be something like post_info_with_inner.calc_actual_weight(info).proof_size().saturating_sub(measured_proof_size) and we'd need to add this to inner_refund. Am I missing anything?

[gui1117]

Indeed I didn't bother returning accurate unspent amount.

If we include the unspent amount of all the dispatch (call, and inner transaction extension), then the unspent we can return is info.total_weight().saturating_sub(accurate_weight).

I will return this and also ensure it is tested

[gui1117]

done in 597fde4

[gui1117]

I forgot to include https://github.com/paritytech/polkadot-sdk/pull/5281/files

[gui1117]

@georgepisaltu concerns should be addressed now, also I missed one change in the old transaction extension.
To keep the accrue in case node proof size is higher than what is registered in block weight: 1ad6e56

@skunert you might be interested by this PR

[georgepisaltu]

Good to merge from my POV 😉 when the last comment is addressed.

[georgepisaltu]

Need to fix this and also we probably need to test this with a post dispatch info with a Some(...) as post_info.actual_weight as well as a None, but in practice our CheckedExtrinsic::apply + dispatch_transaction implementation would guarantee that the weight is always Some.

[gui1117]

Fixed in 363f7d3

The case where None is given as post_info is also tested.
In such case some unspent is returned. (But ignored later as we can't reduce weight from None.)