Fix DryRunApi client-facing XCM versions #7438

mrshiposha · 2025-02-03T15:30:40Z

Description

Integration

This PR updates the DryRunApi. The signature of the dry_run_call is changed, and the XCM version of the return values of dry_run_xcm now follows the version of the input XCM program.

Review Notes

The DryRunApi is modified
Added the Router::clear_messages to dry_run_xcm common implementation
Fixed the xcmp-queue's Router's clear_messages: channels details' first_index and last_index are reset when clearing
The MIN_XCM_VERSION is added
The common implementation in the pallet-xcm is modified accordingly
The DryRunApi tests are modified to account for testing old XCM versions
The implementation from the pallet-xcm is used where it was not used (including the DryRunApi tests)
All the runtime implementations are modified according to the Runtime API change

polkadot/xcm/pallet-xcm/src/lib.rs

polkadot/xcm/xcm-runtime-apis/tests/fee_estimation.rs

bkchr · 2025-02-07T21:46:20Z

This PR makes breaking changes to the DryRunApi. The signature of the dry_run_call is changed, and the XCM version of the return values of dry_run_xcm now follows the version of the input XCM program.

Can you not just determine the XCM version of input xcm program and use this for the output? Otherwise you should at least bump the version of the runtime api.

mrshiposha · 2025-02-07T21:52:34Z

@bkchr

Can you not just determine the XCM version of input xcm program and use this for the output?

I did this for the dry_run_xcm. It was possible there. As for the dry_run_call, there is no input XCM program, just an arbitrary RuntimeCall.

Otherwise you should at least bump the version of the runtime API.

Sure, will do.

bkchr · 2025-02-07T22:06:18Z

As for the dry_run_call, there is no input XCM program, just an arbitrary RuntimeCall.

Somewhere in this RuntimeCall there will be a XCM? If not, you could just assume the minimum XCM version?

mrshiposha · 2025-02-07T22:20:25Z

We could examine the call and see if it is, say, transfer_assets from pallet-xcm. It seems to be a good enough default for common implementation.

I only worried about alternative XCM frontends like ORML pallet-xtokens. If the common implementation only considers pallet-xcm, the pallet-xtokens users will always receive the minimum XCM version.

CC @xlc

acatangiu · 2025-02-10T07:52:33Z

Assuming minimum is brittle, some new instructions do not have equivalents in older versions and conversion will fail. You could start with minimum and go through them until one successfully converts, but feels like overkill.

I'm personally happy with the current PR state where an explicit version is requested by the caller.

acatangiu · 2025-02-10T07:56:20Z

polkadot/xcm/xcm-runtime-apis/src/dry_run.rs

@@ -64,7 +64,7 @@ sp_api::decl_runtime_apis! {
 		OriginCaller: Encode
 	{
 		/// Dry run call.
-		fn dry_run_call(origin: OriginCaller, call: Call) -> Result<CallDryRunEffects<Event>, Error>;
+		fn dry_run_call(origin: OriginCaller, xcms_version: XcmVersion, call: Call) -> Result<CallDryRunEffects<Event>, Error>;


Having this param between origin and call makes one think it is somehow related to the call.

I would move it at the back, and also rename it to something more explicit like result_xcm_version/output_xcm_version/desired_xcm_version.

Suggested change

fn dry_run_call(origin: OriginCaller, xcms_version: XcmVersion, call: Call) -> Result<CallDryRunEffects<Event>, Error>;

fn dry_run_call(origin: OriginCaller, call: Call, result_xcms_version: XcmVersion) -> Result<CallDryRunEffects<Event>, Error>;

polkadot/xcm/xcm-runtime-apis/tests/fee_estimation.rs

acatangiu · 2025-02-10T08:04:13Z

polkadot/xcm/pallet-xcm/src/lib.rs

-		frame_system::Pallet::<Runtime>::reset_events(); // To make sure we only record events from current call.
+
+		// To make sure we only record events from current call.
+		Router::clear_messages();


nice! I see this was done for dry_run_call but not for dry_run_xcm 🤦‍♂️

Yet, this fix breaks penpal runtime somehow...

It seems the xcmp-queue's clear_messages doesn't fix the page indices in the OutboundXcmpStatus and this causes penpal to break when using the common DryRunApi::dry_run_xcm implementation from pallet-xcm (see CI, broken tests::xcm_fee_estimation::multi_hop_works and tests::xcm_fee_estimation::multi_hop_pay_fees_works).

Does it influence prod, though? I have a feeling that these storages should be empty either way when running Runtime API properly.

Maybe the issue is with the test itself?

Does it influence prod, though? I have a feeling that these storages should be empty either way when running Runtime API properly.

Why should it be empty? Calling clear_messages here is correct, as otherwise the function will return invalid messages.

(That got send when building block the runtime api function is called on)

Yes, calling clear_messages() here is correct. Can you look into the tests and find out why exactly they are failing? Is it penpal runtime config issue or test issue?

Why should it be empty?

I just thought that maybe Runtime API runs with the state of the last finalized block. And I remembered that ParachainSystem's on_finalize clears the XCMP storages. However, it seems it does it partially anyway. So yeah, manual clearing before dry-running is needed anyway.

I will look into that.

Turned out it wasn't the Penpal. The bug appeared in the Westend/Rococo AssetHub runtimes.

See the fix: 98dea1d

As to why it appeared:

The xcmp-queue's OutboundXcmpMessages and OutboundXcmpStatus are supposed to be in sync. It isn't enough to clear OutboundXcmpMessages without syncing the indices from the OutboundXcmpStatus. If the storages are out-of-sync, it is possible (and that is what happened in the tests) that the have_active flag is erroneously set and the execution goes on the wrong branch. See here.

How they got out-of-sync: The integration tests use an emulated environment and call runtime APIs directly as Rust functions without things like with_transaction. This way, the dry run turns into an actual run, and all the networks process the resulting messages. The AssetHub is the reserve chain in the test scenario and has its storages modified after the PenpalA chain "dry-runs" a transfer call. When we try to "dry-run" a forwarded XCM program, we run into the issue of the storage being out-of-sync if we use the clear_messages implementation without the provided fix. You can verify this reasoning by looking at this code

acatangiu

nice!

polkadot/xcm/xcm-runtime-apis/Cargo.toml

prdoc/pr_7438.prdoc

Co-authored-by: Adrian Catangiu <[email protected]>

mrshiposha added 4 commits February 3, 2025 16:17

fix: DryRunApi honors client's XCM version

047fb16

fix(test): DryRunApi tests old XCM versions

d66e18e

fix: DryRunApi runtime implementations

6804c15

fix: fmt

b2f2f06

mrshiposha requested a review from a team as a code owner February 3, 2025 15:30

mrshiposha added 5 commits February 3, 2025 17:57

fix: clippy

4bda5a7

fix: clippy

709a39f

fix: clippy

ba1b1bf

fix: integration tests

9ba3c46

fix: integration tests

84a29b9

acatangiu requested review from karolk91, x3c41a and raymondkfcheung February 4, 2025 09:24

x3c41a reviewed Feb 4, 2025

View reviewed changes

polkadot/xcm/pallet-xcm/src/lib.rs Show resolved Hide resolved

x3c41a reviewed Feb 4, 2025

View reviewed changes

polkadot/xcm/xcm-runtime-apis/tests/fee_estimation.rs Outdated Show resolved Hide resolved

x3c41a approved these changes Feb 4, 2025

View reviewed changes

mrshiposha added 2 commits February 5, 2025 13:53

refactor: runtime api version tests

f21fc2b

fix: clear messages before xcm dry run

1d77746

x3c41a approved these changes Feb 5, 2025

View reviewed changes

acatangiu approved these changes Feb 10, 2025

View reviewed changes

acatangiu added T6-XCM This PR/Issue is related to XCM. T4-runtime_API This PR/Issue is related to runtime APIs. labels Feb 10, 2025

raymondkfcheung approved these changes Feb 10, 2025

View reviewed changes

mrshiposha added 2 commits February 20, 2025 16:35

fix: xcmp-queue router clear_messages

98dea1d

fix: make result_xcms_version param the last one

e05ecf6

mrshiposha added 5 commits February 21, 2025 16:13

fix: mark DryRunApi with api_version(2)

e053881

feat: add MIN_XCM_VERSION

eb6559a

chore: add prdoc

beac005

fix: xcm-runtime-apis version

75f2697

fix: Cargo.lock

9ada758

acatangiu approved these changes Feb 21, 2025

View reviewed changes

polkadot/xcm/xcm-runtime-apis/Cargo.toml Outdated Show resolved Hide resolved

prdoc/pr_7438.prdoc Outdated Show resolved Hide resolved

mrshiposha and others added 7 commits February 21, 2025 17:43

Update polkadot/xcm/xcm-runtime-apis/Cargo.toml

b54bbd6

Co-authored-by: Adrian Catangiu <[email protected]>

Update prdoc/pr_7438.prdoc

ac4cc14

Co-authored-by: Adrian Catangiu <[email protected]>

fix: fmt

e09b9b1

fix: prdoc

195cc5e

fix: Cargo.lock

b41d6c9

Merge branch 'master' into fix-dryrunapi-xcm-versions

7a20dcc

fix: Cargo.lock

c030b46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DryRunApi client-facing XCM versions #7438

Fix DryRunApi client-facing XCM versions #7438

mrshiposha commented Feb 3, 2025 •

edited

Loading

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

acatangiu commented Feb 10, 2025

acatangiu Feb 10, 2025

mrshiposha Feb 21, 2025

acatangiu Feb 10, 2025

mrshiposha Feb 10, 2025

mrshiposha Feb 10, 2025

mrshiposha Feb 10, 2025

bkchr Feb 10, 2025

bkchr Feb 10, 2025

acatangiu Feb 11, 2025

mrshiposha Feb 11, 2025

mrshiposha Feb 21, 2025

acatangiu Feb 21, 2025

acatangiu left a comment

	fn dry_run_call(origin: OriginCaller, xcms_version: XcmVersion, call: Call) -> Result<CallDryRunEffects<Event>, Error>;
	fn dry_run_call(origin: OriginCaller, call: Call, result_xcms_version: XcmVersion) -> Result<CallDryRunEffects<Event>, Error>;

Fix DryRunApi client-facing XCM versions #7438

Are you sure you want to change the base?

Fix DryRunApi client-facing XCM versions #7438

Conversation

mrshiposha commented Feb 3, 2025 • edited Loading

Description

Integration

Review Notes

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

bkchr commented Feb 7, 2025

mrshiposha commented Feb 7, 2025

acatangiu commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

mrshiposha commented Feb 3, 2025 •

edited

Loading