freeverseio / laos

GNU General Public License v3.0
16 stars 6 forks source link

Investigate Extrinsic Signing Issue for LAOS Metadata Version 15 #710

Closed asiniscalchi closed 5 days ago

asiniscalchi commented 1 month ago

As a developer working on the LAOS project,
I want to investigate the discrepancy in the extrinsic signing process related to metadata version 15,
so that we can determine if the current behavior is intentional or if adjustments are needed to ensure compatibility with various wallets.

Description

Talisman has reported an issue where, when signing on LAOS with metadata version 15, Polkadot.js adds a 'type' prefix byte to the signature by default because the ExtrinsicSignature type on LAOS is an Enum. This behavior differs from other chains such as Moonbeam. The relevant code can be found here: ExtrinsicPayload.ts.

Talisman has to maintain a flag in their chaindata repository to specify withType: false for LAOS to handle this issue. This workaround is acceptable if necessary, but it would be preferable to avoid it if the current implementation is unintentional.

Acceptance Criteria

Additional Information

References

asiniscalchi commented 1 month ago

Dani has a Telegram room with Telegram

chidg commented 1 month ago

Hi @asiniscalchi, Talisman engineering team here - thanks for following up on this. Please keep us up to date.

magecnion commented 2 weeks ago

Hi @chidg I’ve been investigating this issue, and I’ve found that it is not intentional for the ExtrinsicType in LAOS to be an Enum. This setup is typically used for chains that allow multiple types of signatures, which is not the case for us. As you can see, we only support ECDSA signatures, just like Moonbeam does. So, I don’t understand why Polkadot.js adds a type prefix if we’re using the same signature primitive.

Actually, I have a question: when you mention signing on LAOS with metadata V15, what exactly do you mean? Our metadata is V14, or am I mistaken?

alecdwm commented 2 weeks ago

Hey @magecnion! I recommend giving this post a quick read for an overview on the v15 format: https://forum.polkadot.network/t/stablising-v15-metadata/2819

The Metadata_metadata runtime call continues to serve v14 to maintain compatibility with existing dapps and tools, while the new Metadata_metadataAtVersion and Metadata_metadataVersions calls provide the v15 format. Laos serves v15 via the new runtime call

It seems that the v15 format introduces some extrinsic type information that for v14 would have been hardcoded into the client library: https://github.com/paritytech/substrate/issues/12929

I guess for Laos there may be something inconsistent between the extrinsic signature type as encoded into the metadata, and the code which verifies submitted signatures?

And perhaps this inconsistency wouldn’t have been noticed before when for v14 the client had to assume some details for the signature type.

alecdwm commented 2 weeks ago

The Laos signature type fp_account::EthereumSignature does implement From<MultiSignature>, the latter of which the v15 metadata is telling us the chain uses https://github.com/polkadot-evm/frontier/blob/2e219e17a526125da003e64ef22ec037917083fa/primitives/account/src/lib.rs#L214

alecdwm commented 2 weeks ago

Whereas it seems Moonbeam have their own EthereumSignature type and unlike the frontier default theirs implements From<ecdsa::Signature> https://github.com/moonbeam-foundation/moonbeam/blob/1682686be72f78f0bd79532695e0eed7fe32bfde/primitives/account/src/lib.rs#L123

magecnion commented 2 weeks ago

The Laos signature type fp_account::EthereumSignature does implement From<MultiSignature>, the latter of which the v15 metadata is telling us the chain uses https://github.com/polkadot-evm/frontier/blob/2e219e17a526125da003e64ef22ec037917083fa/primitives/account/src/lib.rs#L214

Good observations @alecdwm. We use a previous version of Frontier that doesn't actually implement From<MultiSignature>. I have checked our metadata V15, particularly extrinsic types, and they are quite similar to the Moonbeam ones. However, as you mentioned, Moonbeam does implement fp_account::EthereumSignature, and I agree with you that we are missing something here.

We recently updated our dependencies to a newer version, in which Frontier does include the previously mentioned implementation From<MultiSignature>. Could you check if the new metadata works without the workaround you had to apply? You can find it here

alecdwm commented 2 weeks ago

Oh whoops - 66 bytes is the prefix + signature. 65 bytes is what we're looking for! I'll test out the prod Laos to see if it works without our workaround.

Misinformation:
Hey @magecnion! Here's the extrinsic signature type in the v15 metadata for Laos in prod: ```json [ { "id": 406, "type": { "path": ["fp_account", "EthereumSignature"], "params": [], "def": { "composite": { "fields": [{ "name": null, "type": 407, "typeName": "ecdsa::Signature", "docs": [] }] } }, "docs": [] } }, { "id": 407, "type": { "path": ["sp_core", "ecdsa", "Signature"], "params": [], "def": { "composite": { "fields": [{ "name": null, "type": 408, "typeName": "[u8; 65]", "docs": [] }] } }, "docs": [] } }, { "id": 408, "type": { "path": [], "params": [], "def": { "array": { "len": 65, "type": 2 } }, "docs": [] } } ] ``` Versus the extrinsic signature type in that new metadata: ```json [ { "id": 442, "type": { "path": ["fp_account", "EthereumSignature"], "def": { "composite": { "fields": [{ "type": 443, "typeName": "ecdsa::Signature" }] } } } }, { "id": 443, "type": { "def": { "array": { "len": 65, "type": 2 } } } } ] ``` ~~Unfortunately, despite their differences, it seems that in both cases the `ecdsa::Signature` type is an array of size 65.~~ ~~From what I understand, the signature itself is 64 bytes, and then the final byte is a prefix to indicate the signature type (i.e. a `MultiSignature` enum).~~ ~~With the live version of Laos our workaround is to trim that first byte off the signature before submitting the extrinsic to the chain. As such - unless something has changed in the signature verification for the chain - I expect the bug is still there.~~ Happy to test it out though - is the new runtime deployed anywhere e.g. on a testnet?
alecdwm commented 2 weeks ago

No dice! Still getting the same error in prod: createType(ExtrinsicSignature):: Expected input with 65 bytes (520 bits), found 66 bytes

Although I'll admit that I'm more confused than ever as to why registry.createTypeUnsafe('ExtrinsicSignature', []) instanceof Enum is true, given the type of fp_account::EthereumSignature on Laos.

Here's Moonbeam's v15 metadata for the extrinsic signature type, for comparison:

[
  {
    "id": 187,
    "type": {
      "path": ["account", "EthereumSignature"],
      "params": [],
      "def": { "composite": { "fields": [{ "name": null, "type": 188, "typeName": "ecdsa::Signature", "docs": [] }] } },
      "docs": []
    }
  },
  {
    "id": 188,
    "type": {
      "path": ["sp_core", "ecdsa", "Signature"],
      "params": [],
      "def": {
        "composite": {
          "fields": [{ "name": null, "type": 189, "typeName": "[u8; SIGNATURE_SERIALIZED_SIZE]", "docs": [] }]
        }
      },
      "docs": []
    }
  },
  { "id": 189, "type": { "path": [], "params": [], "def": { "array": { "len": 65, "type": 2 } }, "docs": [] } }
]

It looks the same as Laos to me 🤔

magecnion commented 2 weeks ago

@alecdwm, could you test this out on the testnet that has the new runtime with the upgraded dependencies? This is the endpoint: wss://rpc.laosmercury.gorengine.com. I think that using the new metadata with the prod runtime, which uses the Frontier version that doesn't implement From<MultiSignature>, won't work, and that's why it is still failing.

alecdwm commented 2 weeks ago

@magecnion Same error on wss://rpc.laosmercury.gorengine.com sadly - createType(ExtrinsicSignature):: Expected input with 65 bytes (520 bits), found 66 bytes.

magecnion commented 2 weeks ago

@alecdwm, I'll look into it further and let you know. Is there a script I can use to test the same scenario as yours?

alecdwm commented 2 weeks ago

I'll set one up and get back to you asap!

Edit: I have a small reproducible example here, but surprisingly enough it's creating the correct signatures (65 bytes).
I'll dig a bit more into the differences between this and the talisman wallet source and see if I can figure out what's going on!

magecnion commented 2 weeks ago

I took a look at the example you provided, @alecdwm, and in this line, the signature type is specified. AFAIU, Talisman doesn't do the same. Would it be possible to provide an example using the same pattern that Talisman uses to get the keypair in order to reproduce the same scenario locally?

alecdwm commented 1 week ago

Hey @magecnion, I'm pretty sure we also set the signature type in Talisman:

That being said, there's still some differences between this how example codebase and Talisman are creating the extrinsic. Given that a fresh @polkadot/api setup (the example codebase) is creating the correct signature, I'm starting to suspect that the bug might in fact be in Talisman's tx signing code. Still looking into it!

magecnion commented 5 days ago

@alecdwm any updates? I'm happy to help if needed; otherwise, I will close this issue. Feel free to reopen it when you find something that could clarify the problem.

alecdwm commented 5 days ago

Hey! I think it's cool to close it for now. Our workaround is enough to let users sign on Laos for the time being - meanwhile we're about to make some substantial upgrades to the code which handles tx signing in the wallet. After we finish that upgrade I'm hoping we'll either have a better grasp on why this bug happens - or even better we might simply find a fix for it on the wallet side!