centrifuge / go-substrate-rpc-client

Substrate RPC client for go aka GSRPC
Apache License 2.0
200 stars 167 forks source link

TestAuthor_SubmitExtrinsic Failed with Westend netowrk #386

Open wooyeeyii opened 2 weeks ago

wooyeeyii commented 2 weeks ago

signed raw Tx is 0x41028400d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d01d401d94c6cfeeca5ddae003f90e6512c2eaf8bb82abe32bd2cff907cc910126f625b4d4b41dbb30dc0869b09195bd0e96c890865ca9ddeb4cf7d0a6d8bce818f00b10200040000bb90297ca6d058b0efe8137d3853d5c68eb5e4375dc3dc6231ffc01e5183bc0a0700e8764817

 submit_extrinsic_test.go:76: extrinsic submit failed: Verification Error: Runtime error: Execution failed: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed
        WASM backtrace:
        error while executing at wasm backtrace:
            0: 0x71d10 - <unknown>!rust_begin_unwind
            1: 0xc920 - <unknown>!core::panicking::panic_fmt::h7d22643b0becf577
            2: 0x25f601 - <unknown>!TaggedTransactionQueue_validate_transaction
wooyeeyii commented 2 weeks ago

when I submitExtrinsic with https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fwestend-rpc.polkadot.io#/rpc I got

28: author.submitExtrinsic: Hash
createType(Extrinsic):: createType(ExtrinsicV4):: createType(Call):: Call: failed decoding system.remark:: Struct: failed on args: {"remark":"Bytes"}:: decodeU8aStruct: failed at 0xbb90297ca6d058b0efe8137d3853d5c6… on remark (index 1/1): Bytes:: Compact input is > Number.MAX_SAFE_INTEGER
redmaner commented 2 weeks ago

We are encountering the exact same issue. I expect it might be related to the CheckMetadataHash extension which is new in the runtime, and is flagged as a breaking change. Have been trying to get it working but to no avail as of yet.

wooyeeyii commented 2 weeks ago

By comparing with raw transaction generated by polkadot.js, I suspect it might be related to the AssetId. https://github.com/polkadot-js/api/pull/5839

cdamian commented 2 weeks ago

We are encountering the exact same issue. I expect it might be related to the CheckMetadataHash extension which is new in the runtime, and is flagged as a breaking change. Have been trying to get it working but to no avail as of yet.

Hey, yes, that does seem to be the cause for this. We are currently looking into fixing this.

GCrispino commented 2 weeks ago

@wooyeeyii @cdamian I've been able to successfully craft and send a transaction by forking the library code and modifying it a bit.

My changes are in the fix/checkmetadatahash branch of my forked go-substrate-rpc-client repository.

What was changed was that a UCompact AssetID field was added to the ExtrinsicSignatureV4 type (as @wooyeeyii suggested), and then both a UCompact AssetID field and a byte Mode field were added in the ExtrinsicPayloadV4 type Encode implementation.

In the following repo's main.go file you can find a code example that uses this fork when importing go-substrate-rpc-client and creates a transfer transaction (it needs a phrase.txt file with a seedphrase in it in the same directory to work): https://github.com/GCrispino/test-tx-metadata-hash.

This is rudimentary and not the best way to solve this as I'm probably missing some other details, but perhaps it can be helpful to you.

cdamian commented 2 weeks ago

@wooyeeyii @cdamian I've been able to successfully craft and send a transaction by forking the library code and modifying it a bit.

My changes are in the fix/checkmetadatahash branch of my forked go-substrate-rpc-client repository.

What was changed was that a UCompact AssetID field was added to the ExtrinsicSignatureV4 type (as @wooyeeyii suggested), and then both a UCompact AssetID field and a byte Mode field were added in the ExtrinsicPayloadV4 type Encode implementation.

In the following repo's main.go file you can find a code example that uses this fork when importing go-substrate-rpc-client and creates a transfer transaction (it needs a phrase.txt file with a seedphrase in it in the same directory to work): https://github.com/GCrispino/test-tx-metadata-hash.

This is rudimentary and not the best way to solve this as I'm probably missing some other details, but perhaps it can be helpful to you.

Hey,

Thank you very much for taking the time to look into this!

I am also currently testing a potential solution that doesn't involve the AssetId - if you get a signed extrinsic hex from the polkadot JS app, you can decode that and check that no asset ID is provided for the westend runtime.

Will take a look at your branch, bring everything together, and post an update as soon as I have it.

GCrispino commented 2 weeks ago

@cdamian

Thank you very much for taking the time to look into this!

No problem, I'm in need of a fix for this too 😅

I am also currently testing a potential solution that doesn't involve the AssetId - if you get a signed extrinsic hex from the polkadot JS app, you can decode that and check that no asset ID is provided for the westend runtime.

Interesting... to be honest I don't really know what the AssetID is or does, but I could see it in other SDKs (the official JS one as @wooyeeyii pointed out, and in the Python py-substrate-interface SDK: https://github.com/polkascan/py-substrate-interface/pull/392/files#diff-b36d9abb1f67b35f6023f469f458fbad76cad62fe37ec98abd53b82391d4e302), so I was wondering if it was a new thing added to the transaction format recently and if it is now a needed thing such as the new mode property

Will take a look at your branch, bring everything together, and post an update as soon as I have it.

Awesome! Thanks for letting us know

cdamian commented 2 weeks ago

Please try out - https://github.com/centrifuge/go-substrate-rpc-client/tree/add-check-metadata-support-in-extrinsic

Interesting... to be honest I don't really know what the AssetID is or does, but I could see it in other SDKs (the official JS one as @wooyeeyii pointed out, and in the Python py-substrate-interface SDK: https://github.com/polkascan/py-substrate-interface/pull/392/files#diff-b36d9abb1f67b35f6023f469f458fbad76cad62fe37ec98abd53b82391d4e302), so I was wondering if it was a new thing added to the transaction format recently and if it is now a needed thing such as the new mode property

There are some chains that support passing an AssetId when signing the extrinsic, but I don't think that's common. You can check this for more info.

I have tested this so far using this, however, please note that I only managed to test it successfully using the disabled mode. I tried the enabled mode but the metadata hash that I provided seemed incorrect and I'm not sure how to properly generate that at this point.

GCrispino commented 2 weeks ago

@cdamian

Please try out - https://github.com/centrifuge/go-substrate-rpc-client/tree/add-check-metadata-support-in-extrinsic

I have tested this so far using this, however, please note that I only managed to test it successfully using the disabled mode. I tried the enabled mode but the metadata hash that I provided seemed incorrect and I'm not sure how to properly generate that at this point.

Nice! Did you get to successfully submit a transaction without using the AssetID then?

If you don't get the metadata hash working properly, an option is to have support only for the disabled mode for now. That's what was done in the py-substrate-interface Python package for example: https://github.com/polkascan/py-substrate-interface/pull/392

There are some chains that support passing an AssetId when signing the extrinsic, but I don't think that's common. You can check https://github.com/centrifuge/go-substrate-rpc-client/issues/240 for more info.

Got it, thanks 👍🏼

https://github.com/polkascan/py-substrate-interface/pull/392

redmaner commented 2 weeks ago

Hi all, just something to raise in this discussion. It seems that not all networks have the same signed extensions enabled in the runtime. CheckMetadataHash is for example enabled on Westend, but not (yet) on Polkadot mainnet. Would it make sense to add support for creating an extrinsic signing payload that dynamically encodes the required fields based on Metadata? I see the python library doing something similar here: https://github.com/polkascan/py-substrate-interface/blob/master/substrateinterface/base.py#L1465

cdamian commented 2 weeks ago

Hi all, just something to raise in this discussion. It seems that not all networks have the same signed extensions enabled in the runtime. CheckMetadataHash is for example enabled on Westend, but not (yet) on Polkadot mainnet. Would it make sense to add support for creating an extrinsic signing payload that dynamically encodes the required fields based on Metadata? I see the python library doing something similar here: https://github.com/polkascan/py-substrate-interface/blob/master/substrateinterface/base.py#L1465

@redmaner based on the info I gathered so far, most runtimes will use this extension. We will look into adding a way to dynamically provide the required payload information for extrinsic signing based on the metadata, until then, please let us know if you encounter any issues using the above mentioned solution.

redmaner commented 2 weeks ago

@cdamian your proposed solution in the branch you mentioned should do the job. What I was referring to is that currently Polkadot mainnet doesn't have the CheckMetadataHash extension enforced, so you would still have to use SingingPayloadV4 while other networks require SingingPayloadV5. If we at some point can have a dynamic SigningPayload using the Metadata as an argument to determine what is encoded and in what order that would abstract some complex logic away, but is is nice to have at this stage.

redmaner commented 5 days ago

@cdamian any updates on this issue?

cdamian commented 5 days ago

@redmaner I'm currently testing a solution that can support different signed extension setups, without relying too much on hardcoded types. Will post that as soon as it's ready.

cdamian commented 2 days ago

@redmaner I managed to test this branch successfully on westend and centrifuge-development using the test script provided in this PR. Please check it out and let me know what you think.