Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Statemine XCM: panic on cross-chain transfer two kinds of assets #892

Open
shaunxw opened this issue Jan 10, 2022 · 11 comments
Open

Statemine XCM: panic on cross-chain transfer two kinds of assets #892

shaunxw opened this issue Jan 10, 2022 · 11 comments
Labels
T6-XCM This PR/Issue is related to XCM. T7-system_parachains This PR/Issue is related to System Parachains.

Comments

@shaunxw
Copy link
Contributor

shaunxw commented Jan 10, 2022

I was trying cross-chain transfer from Statemine to Karura, after the HRMP channel opened. Transferring one kind of assets works great, but it panicked every time on transferring two kinds of assets (one for the target asset to send, the other is KSM to pay fees).
Screen Shot 2022-01-11 at 11 16 02 AM

@joepetrowski
Copy link
Contributor

What's the full call? @KiChjang can you help?

@joepetrowski joepetrowski added T7-system_parachains This PR/Issue is related to System Parachains. T6-XCM This PR/Issue is related to XCM. Z1-question labels Jan 17, 2022
@shaunxw
Copy link
Contributor Author

shaunxw commented Jan 17, 2022

Encoded call data: 0x1f0201010100411f010001010038d7db4bec59ba1afe95543de9e1065f2df53e9bb406fc3d8ec6d2e26101bd240108000001050c00a10f0000000002286bee01000000

@KiChjang
Copy link
Contributor

Does the Here asset on Statemine represent KSM? I was thinking that it should be ../Here instead, but I could be wrong...

@xlc
Copy link
Contributor

xlc commented Jan 18, 2022

Even if the XCM is bad, the transaction could fail, but it shouldn't panic the transaction verification logic?

@KiChjang
Copy link
Contributor

Indeed, I'll try and look for the source of panic in the runtime in the meantime.

@KiChjang
Copy link
Contributor

KiChjang commented Feb 2, 2022

Okay, so I recently spun up a local westmint parachain, and tried to submit this transaction as-is, and indeed I have ran into an error, but it's different from the one that you see:

2022-02-02 11:22:16             DRR: 1002: Verification Error: Runtime error: Execution failed: ApiError(FailedToConvertParameter { function: "validate_transaction", parameter: "tx", error: Error { cause: Some(Error { cause: Some(Error { cause: Some(Error { cause: None, desc: "Out of order" }), desc: "Could not decode `VersionedMultiAssets::V1.0`" }), desc: "Could not decode `Call::reserve_transfer_assets::assets`" }), desc: "Could not decode `Call::PolkadotXcm.0`" } }): RuntimeApi, Execution failed: ApiError, FailedToConvertParameter { function: \"validate_transaction\", parameter: \"tx\", error: Error { cause: Some, Error { cause: Some, Error { cause: Some, Error { cause: None, desc: \"Out of order\" }, desc: \"Could … polkadot.02.d1788076.js:1:282870

This is suggesting to me that somehow decoding of the raw data is wrong, and so I tried to change the assets parameter to use V0 instead and resubmit as an unsigned transaction, and indeed I have ran into a different error:

2022-02-02 11:27:48        RPC-CORE: submitAndWatchExtrinsic(extrinsic: Extrinsic): ExtrinsicStatus:: 1011: Unknown Transaction Validity: NoUnsignedValidator polkadot.02.d1788076.js:1:282870

To ensure that this is indeed a decoding problem, I tried changing the call to use teleport_assets, and indeed the same error surfaced:

2022-02-02 11:31:38        RPC-CORE: submitAndWatchExtrinsic(extrinsic: Extrinsic): ExtrinsicStatus:: 1002: Verification Error: Runtime error: Execution failed: ApiError(FailedToConvertParameter { function: "validate_transaction", parameter: "tx", error: Error { cause: Some(Error { cause: Some(Error { cause: Some(Error { cause: None, desc: "Out of order" }), desc: "Could not decode `VersionedMultiAssets::V1.0`" }), desc: "Could not decode `Call::teleport_assets::assets`" }), desc: "Could not decode `Call::PolkadotXcm.0`" } }): RuntimeApi, Execution failed: ApiError, FailedToConvertParameter { function: \"validate_transaction\", parameter: \"tx\", error: Error { cause: Some, Error { cause: Some, Error { cause: Some, Error { cause: None, desc: \"Out of order\" }, desc: \"Could … polkadot.02.d1788076.js:1:282870

My guess of where exactly the decoding error is in decoding the Vec in the V1 assets parameter:

image

XcmVersionedMultiAssets is defined as follows:

pub enum VersionedMultiAssets {
	V0(Vec<v0::MultiAsset>),
	V1(v1::MultiAssets),
}

And v1::MultiAssets is defined as:

pub struct MultiAssets(Vec<MultiAsset>);

As we can see here, there is an additional MultiAssets wrapper struct around the Vec, which seems to be missing from the call data. Can anybody verify that my findings are correct? cc @shawntabrizi @jacogr

@jacogr
Copy link

jacogr commented Feb 2, 2022

There is no difference in the SCALE-encoding when you have a struct/tuple with a single param, vs just the field. So the following SCALE encodings would all be equivalent -

  • pub struct MultiAssets(Vec<MultiAsset>)
  • (Vec<MultiAsset>)
  • Vec<MultiAsset>

So the JS API maps single field structs/tuples to just the inner since it doesn't affect SCALE encoding/decoding at all.

@KiChjang
Copy link
Contributor

KiChjang commented Feb 3, 2022

Okay, I think I'm starting to see where the error is coming from:

https://github.com/paritytech/polkadot/blob/498a7976d588553e1ed27ff0b1b7728e062e334b/xcm/src/v1/multiasset.rs#L294-L297

From the looks of it, it would seem that from_sorted_and_deduplicated always results in an error.

@jacogr
Copy link

jacogr commented Feb 3, 2022

Ahhh... ok, so in submission it needs to be pre-sorted?

So this is actually the one place where the JS API can help, i.e. have a specific encoder for MultiAssets and then sort the inner vec. (There are a couple of types where it has specific additional encoding logic, this could be one of them - it is always horrible having to duplicate logic between Rusts and the APIs, i.e. they could get out of sync, so to be used sparingly)

@KiChjang
Copy link
Contributor

KiChjang commented Feb 3, 2022

I just verified it, and yes, the assets parameter needs to be sorted in order to be decoded the proper way. I switched the order of the GeneralIndex(3) asset and the Here asset, and after that, the extrinsic submission stopped throwing the Out of order error.

I think it is indeed a good idea for the JS API to pre-sort the inner vec, because IIRC the ordering does matter -- fungibles first, then non-fungibles. And for the fungibles, assets with shorter MultiLocations are first. I can't exactly remember the reason why off the top of my head, but I do recall that some algorithm relies on such an ordering.

@KiChjang
Copy link
Contributor

KiChjang commented Feb 3, 2022

Right ok, Gav has reminded me that it is because we need to ensure that there are no duplicate entries within the vec, and so they need to be sorted in order to be able to validate it in O(n) time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T6-XCM This PR/Issue is related to XCM. T7-system_parachains This PR/Issue is related to System Parachains.
Development

No branches or pull requests

6 participants