[pallet_xcm] Customizable/extendable remote_message estimation

[bkontur]

Cumulus companion: paritytech/cumulus#2811

TL;DR: this PR fixes at least two things:

• adds ability to include additional instructions (SetTopic, UniversalOrigin, DescendOrigin, ...) to remote weight estimation (see more Problem 1. bellow)
• adds ability to configure different weigher for remote estimation for pallet_xcm than local one (now it just uses local weigher (see more Problem 2. bellow)

Note: This PR does not change any existing behavior, local weigher is now used for remote estimation, this PR just adds ability to configure different weigher for remote estimation.

---

Problem 1.:

Issue is that pallet_xcm for (do_teleport_assets/do_reserve_transfer_assets) tries to estimate weight for remote_message, but calculates only with several instruction, and this weight is used as a weight_limit for BuyExecution on destination.

But on the way, we can add possibly to this message other instructions, like SetTopic - feature for tracking message_id, bridging exporters add UniversalOrigin/DescendOrigin and so on. So on destination BuyExecution.weight_limit could be lower than the real calculated weight by XcmExecutor for received message (which includes those other instructions) and possibly Barrier can stop this message.

So with this PR, any runtime, which uses pallet_xcm, can configure additional instructions to this remote message estimation, because Runtime knows about his configuration.

---

Problem 2.:
the correct solution here is to split weights:

• runtime's real regenerate weights (e.g. used for local xcm execution processing)
• weights used for remote weight estimation, e.g. for pallet_xcm::reserve_transfer_assets / teleport_assets

problem could be potential, e.g. that local runtime does not process ReserveAssetDeposited so the real generated weight is should be Weight::MAX, so if we take this local weight for ReserveAssetDeposited (e.g. Weight::MAX) for remote estimation, it will never work,
because e.g. remote destination as AssetHubPolkadot could have its real regenerated value

also this PR will prepare pallet_xcm for "xcm standard weights" (Gav's idea from one discussion), because using local weight for remote estimation is not good

[vstam1]

Overall structure and idea looks good to me. We could allow UniversalWeigherAdapter to take a tuple of AdditionalDestinationInstructions so we can apply simple additions instead of a full function that needs to track which additions to do. So lets say for a certain ML I add the SetTopic and for other ML I add DescendOrigin or I need to add both. This function should now match for all these MLs. If we can add a tuple we can simplify the functions and we might be able to create standards inside the xcm-builder that every runtime can use.

It is a suggestion and might be over engineering for the problem. Let me know what you think.

[bkontur]

bot rebase

[paritytech-processbot]

Rebased

[KiChjang]

I'm not really keen on this approach, because it still utilizes the local weigher to weight the XCMs that will ultimately be weighed by the destination's weigher. I am not sure if the current approach allows for us in the future to easily swap the local weigher with the destination weigher.

[bkontur]

I'm not really keen on this approach, because it still utilizes the local weigher to weight the XCMs that will ultimately be weighed by the destination's weigher.

yes, exactly, I agree, using local weigher is no go - also more here,

lets assume pallet_xcm::reserve_transfer_assets from AssetHubKusama to AssetHubPolkadot, and assume that both using their own local weighers (which have the same values, e.g. 1 weight per 1 instruction), so reserve_transfer_assets does remote weight estimation for instrusctions: ReserveAssetDeposited/ClearOrigin/BuyExecution/DepositAsset one AssetHubKusama, means 4 weight in BuyExecution(weight_limit: 4),

but AssetHubKusama uses WithUniqueTopic which adds SetTopic instruction plus sending BridgeHubKusama adds two more instructions UniversalOrigin/DescendOrigin plus target BridgeHubPolkadot adds one more instruction DescendOrigin,

so AssetHubPolkadot receives XCM with DescendOrigin/UniversalOrigin/DescendOrigin/ReserveAssetDeposited/ClearOrigin/BuyExecution/DepositAsset/SetTopic, means 8 weight measured,
but our original estimation was ``BuyExecution(weight_limit: 4), so this does not pass AllowTopLevelPaidExecutionFrom` and we have a problem

---

so this PR fixes at least two things:

• adds ability to configure different weigher than local one
• adds ability to include additional instructions (SetTopic, UniversalOrigin, DescendOrigin, ...) to remote weight estimation

---

I am not sure if the current approach allows for us in the future to easily swap the local weigher with the destination weigher.
yes, this PR exactly allows us to easily change destination weigher

pallet_xcm uses local weigher now, this PR does not change that, because I didnt know how, but Gav just recently came up with idea of "xcm standard weights", so if we find out how to generate "xcm standard weights", we can use them just for remote weight estimation instead of local weigher

[bkontur]

bot fmt

[command-bot]

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404426 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 41-a3f4aba4-c50f-497e-9ddf-4d77341b52b7 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404426 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404426/artifacts/download.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404440

[KiChjang]

Should be ok, given that you can configure which weigher to use in UniversalWeighterAdapter.

[bkontur]

bot fmt

[command-bot]

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3406501 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 46-aaa7574b-8fce-4b46-bb19-f4ee8d9b447f to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3406501 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3406501/artifacts/download.

[bkontur]

Overall structure and idea looks good to me. We could allow UniversalWeigherAdapter to take a tuple of AdditionalDestinationInstructions so we can apply simple additions instead of a full function that needs to track which additions to do. So lets say for a certain ML I add the SetTopic and for other ML I add DescendOrigin or I need to add both. This function should now match for all these MLs. If we can add a tuple we can simplify the functions and we might be able to create standards inside the xcm-builder that every runtime can use.

It is a suggestion and might be over engineering for the problem. Let me know what you think.

@vstam1
thank you, yes, I did exactly what you suggested, tuple and e.g. impl for WithUniqueTopic, and it is much much nicer/simplier