Add benchmarks to transaction extensions

[georgepisaltu]

Follow up effort on top of #2280

This PR implements benchmarking code for transaction extensions:

• ChargeTransactionPayment
• ChargeAssetTxPayment (asset-tx-payment)
• ChargeAssetTxPayment (asset-conversion-tx-payment)
• CheckWeight
• CheckTxVersion
• CheckSpecVersion
• CheckNonce
• CheckNonZeroSender
• CheckMortality
• CheckGenesis
• CheckOnlySudoAccount
• PrevalidateAttests

Extensions which still don't have benchmarks but could probably continue to use the default impl of Weight::zero() or call some existing weight function in the case of wrappers:

• Test extensions
  • DummyExtension
  • WatchDummy
• Wrappers
  • GenericTransactionExtension
  • TransactionExtension (chain-polkadot-bulletin)
• Bridge extensions (see discussion below)
  • RefundTransactionExtensionAdapter

Each extension's TransactionExtensionBase::weight impl needs to use the resulting weight function and adapt existing runtimes' benchmarking configuration to these changes. So far, this has been done for:

• ChargeTransactionPayment
• ChargeAssetTxPayment (asset-tx-payment)
• ChargeAssetTxPayment (asset-conversion-tx-payment)
• CheckWeight
• CheckTxVersion
• CheckSpecVersion
• CheckNonce
• CheckNonZeroSender
• CheckMortality
• CheckGenesis
• CheckOnlySudoAccount
• PrevalidateAttests
• RefundTransactionExtensionAdapter

[svyatonik]

Re RefundTransactionExtensionAdapter - it is a bit complicated case && we already have some benchmarking there. I haven't checked - where this weight will be used. If it'll be added to weight of all runtime calls (as a base extrinsic weight or something like that), then it won't work for us - extensions does non-trivial things. So couple of details:

1. first thing that bridge extensions do (in pre_dispatch/validate), is that they check whether the call is the call of one of bridge pallets or batch of those calls by matching. So for most calls the weight should be near zero and adding it to the base tx weight would mean that the signer overpays and we waste block space;
2. then it does some checks in pre_dispatch that are then repeated by the calls themselves. They're mostly about reading tdb values and comparing them to arguments. Important thing there is that since those reads + comparisons are then duplicated, we do not account weight spent on this here, in extension;
3. if those checks ^^^ are failed, then we do not include transaction to the block, so no weight is spent;
4. in post_dispatch there are two possible options that could happen and actions on both branches are non-trivial. That's why we add max of those two weights to the weight of our bridge call. So if we'll add it to the base tx weight, it would mean duplication.

So the main question here is whether this weight you got through benchmarks is added to the base tx weight. If so, then please just left all as is (no additional benchmarks required for our extensions) - just return Weight::zero() from where it is required. Otherwise, please ping one of us (@paritytech/bridges-core ) - we'll help solve any issues here. Actually, I've been working on adapting bridge extensions to new scheme here: https://github.com/paritytech/polkadot-sdk/tree/sv-gav-tx-ext, but I think it was lost somewhere

[georgepisaltu]

So the main question here is whether this weight you got through benchmarks is added to the base tx weight. If so, then please just left all as is (no additional benchmarks required for our extensions) - just return Weight::zero() from where it is required. Otherwise, please ping one of us (@paritytech/bridges-core ) - we'll help solve any issues here.

Regarding RefundTransactionExtensionAdapter, I agree the benchmarking there is complicated. I wanted to first come up with a worst case benchmark just to know it can be done, refining it to consume only what it should would have been incremental work. It could probably be done with "choice" arguments to the weight function to signal which code path would execute so we could choose an appropriate weight function. I'll try to come up with a POC for what is described above, but if it proves too difficult we may resort to accounting for this extension's weight in a different way and just return Weight::zero() in this extension's implementation.

Actually, I've been working on adapting bridge extensions to new scheme here: https://github.com/paritytech/polkadot-sdk/tree/sv-gav-tx-ext, but I think it was lost somewhere

This part is already done, check out the PR doing it and merging into the original PR. Feel free to create additional PRs with any fixes/improvements.

[georgepisaltu]

So the main question here is whether this weight you got through benchmarks is added to the base tx weight. If so, then please just left all as is (no additional benchmarks required for our extensions) - just return Weight::zero() from where it is required.

Something I forgot to address in my original comment: in the current implementation I'm just aiming to have a weight associated with running the whole validate - prepare - post_dispatch flow in the trait impl (which is obviously flawed in the current iteration of the RefundTransactionExtensionAdapter benches). The TransactionExtensionBase::weight() function isn't hooked to any fee/weight management yet. My current understanding is that when run on-chain, these will need to consume weight on top of the base TX weight, but nothing stops off-chain workers from running validate while disregarding the weight. What I mean is that the weight function is there to associate a weight with each validate - prepare - post_dispatch flow; it's up to the extension user to do something (or nothing) with it. In our reference runtimes I think we will be charging weight for extensions, so if bridges deals with the weight of these extensions separately, having the weight function return Weight::zero() is probably correct.

[svyatonik]

My current understanding is that when run on-chain, these will need to consume weight on top of the base TX weight

That is what worries me. As I said, for non-bridge transactions, our extensions are just (almost) noops in term of weight. So if we'll have non-zero weights for that tx and it'll be added by default to the tx base weight (like bas_weight = base_weight + SignedExtensions::pre_dispatch_weight + SignedExtensions::validate_weight + SignedExtensions::post_dispatch_weight), that'd be a problem. I'd just leave some big detailed comment (we may do that) on how to plugin this extension to the runtime with bridge pallets and just go with the "weight function return Weight::zero()" approach.

[georgepisaltu]

@svyatonik at least for now, I reverted the benches for the bridge extensions altogether here.

[ggwpez]

Code looks good.

Just about the design: why did you opt to create a new SystemExtensionsWeightInfo instead of just making it part of the WeightInfo of frame-system?
It causes a lot of changes (also breaking) and i dont really see the advantage.

[ggwpez]

Will need to re-generate those with the bench bot.

[georgepisaltu]

There are a bunch of weight files which were just copied, some of them are outdated as well. Everything will need to be benched again before a release.

[ggwpez]

I wonder if we can get around copy&pasting this everywhere.
Maybe you can pull both functions into a shared crate?

[georgepisaltu]

I tried really hard to make it generic but I found the setup is just too specific. One improvement here would be to move this to a BenchmarkHelper kind of thing in the regular config.

[ggwpez]

I understand that a TE itself should be Send + Sync, but i dont get why we need this on the Config as well.

[georgepisaltu]

We need it for extensions holding a PhantomData, but not for the other ones. I removed it where applicable.

[ggwpez]

Looks a bit hacky, see my review comment

[georgepisaltu]

It doesn't feel hacky to me, it's basically the same as the model for the frame_system weights in the other templates. They also get special treatment because of the name, like in substrate/.maintain/frame-weight-template.hbs.

[ggwpez]

Yea but its not neccecary here since you can move the TE benchmarks to a new frame-system-ext-benchmarking crate.

PS: But then it will need to use the weight trait from a different crate in frame-system, that is also not so nice...

[georgepisaltu]

Just about the design: why did you opt to create a new SystemExtensionsWeightInfo instead of just making it part of the WeightInfo of frame-system? It causes a lot of changes (also breaking) and i dont really see the advantage.

I actually think it's cleaner having them separate since the code being benchmarked in extensions does very different things compared to the system pallet, even though they both depend on frame_system::Config. Main advantage is this way you can easily choose to ignore extension weights, which would be backwards compatible with what we have now, and have real weights for the rest of the system stuff. Given how the current frame_system weights are implemented now, with the benches in a different crate, I think it fits fine.