Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate Extrinsic Signing Issue for LAOS Metadata Version 15 #710

Closed
asiniscalchi opened this issue Aug 2, 2024 · 17 comments
Closed
Labels
Grooming Needed Extra attention is needed High Priority Proposed to focus on this when other tasks are present

Comments

@asiniscalchi
Copy link
Member

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

  • Investigate whether the addition of the 'type' prefix byte to the signature for LAOS is intentional.

Additional Information

  • This issue also occurred with Subwallet, indicating that other wallets might face similar issues if the current behavior remains unchanged.
  • Communication with Talisman has been established, and they are waiting for our confirmation on whether the current behavior is intentional.

References

@asiniscalchi
Copy link
Member Author

Dani has a Telegram room with Telegram

@tonimateos tonimateos added Grooming Needed Extra attention is needed High Priority Proposed to focus on this when other tasks are present labels Aug 6, 2024
@chidg
Copy link

chidg commented Aug 6, 2024

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

@magecnion
Copy link
Contributor

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
Copy link

alecdwm commented Aug 22, 2024

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: paritytech/substrate#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
Copy link

alecdwm commented Aug 22, 2024

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
Copy link

alecdwm commented Aug 22, 2024

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
Copy link
Contributor

magecnion commented Aug 23, 2024

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
Copy link

alecdwm commented Aug 23, 2024

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:
[
  {
    "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:

[
  {
    "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
Copy link

alecdwm commented Aug 23, 2024

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
Copy link
Contributor

@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
Copy link

alecdwm commented Aug 23, 2024

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

@magecnion
Copy link
Contributor

@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
Copy link

alecdwm commented Aug 23, 2024

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
Copy link
Contributor

magecnion commented Aug 26, 2024

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
Copy link

alecdwm commented Aug 27, 2024

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

  • We pass the parameter to the keyring in the background script here
  • The frontend passes it to the background script here
  • The frontend originally sets the parameter to sr25519 or ethereum here

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
Copy link
Contributor

@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
Copy link

alecdwm commented Sep 4, 2024

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!

@magecnion magecnion removed their assignment Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Grooming Needed Extra attention is needed High Priority Proposed to focus on this when other tasks are present
Projects
Status: Done
Development

No branches or pull requests

5 participants