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 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.

[ggwpez]

I think CloneNoBound etc would also work? There should be NoBound variants for all of these in frame-support.

[gui1117]

if we use CloneNoBound we need to implement Clone unconditionally, that mean we would need to bound Clone to the generic S, the wrapped set of transaction extension.

In the case of Clone it is fine. but maybe it is getting a bit controversial to assume S must always bound Eq or Default.

That is why I used derivative, I don't want to force S to bound Eq, but at the same time I want to derive Eq when S is Eq. But also because of T I can't use the standard Eq derive macro.

Maybe overkill but seems ok to me.

[ggwpez]

Not sure i understand this line. Shouldnt the post_dispatch always be less or equal to the pre because our benchmarking over-estimates?

[gui1117]

In the case of weights it is true but here we are talking about something different:

post_dispatch_proof_size is the proof size measured in the node after the dispatch of the transaction.
pre_dispatch_proof_size is the proof size measured in the node before the dispatch of the transaction.

The proof size is increasing as the executions goes, so we do the substration.

I renamed the varialbe to proof_size_after_dispatch and proof_size_before_dispatch for less confusion with weight info in d70e552

[ggwpez]

I think it could be worth it to put this into a function in frame-system?

[gui1117]

It is a bit equivalent to me. People are also moving away from getter in general.

[gui1117]

CI is passing, some continuous integration tests are failing, but nothing indicates it is because of the PR, and there is an issue with semver test itself I think paritytech/parity-publish#42