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

ECDSA keys derived from mnemonics don't match subkey #4467

Closed
JoshOrndorff opened this issue Jan 21, 2021 · 12 comments
Closed

ECDSA keys derived from mnemonics don't match subkey #4467

JoshOrndorff opened this issue Jan 21, 2021 · 12 comments

Comments

@JoshOrndorff
Copy link
Contributor

When the Apps UI derives an ecdsa key from a mnemonic, the resulting keypair is not the same as when using the same mnemonic with subkey or Moonbeam. (Moonbeam and subkey agree).

For example, let's consider Alice. (This output is from subkey at commit 5aae3999 from 20 January)

# Inspect Alice's ECDSA key
$ subkey inspect --scheme ecdsa //Alice
Secret Key URI `//Alice` is account:
  Secret seed:      0xcb6df9de1efca7a3998a8ead4e02159d5fa99c3e0d4fd6432667390bb4726854
  Public key (hex): 0x020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1
  Account ID:       0x01e552298e47454041ea31273b4b630c64c104e4514aa3643490b8aaca9cf8ed
  SS58 Address:     5C7C2Z5sWbytvHpuLTvzKunnnRwQxft1jiqrLD5rhucQ5S9X

# Same result when using the full mnemonic
$ subkey inspect --scheme ecdsa "bottom drive obey lake curtain smoke basket hold race lonely fit walk"//Alice
Secret Key URI `bottom drive obey lake curtain smoke basket hold race lonely fit walk//Alice` is account:
  Secret seed:      0xcb6df9de1efca7a3998a8ead4e02159d5fa99c3e0d4fd6432667390bb4726854
  Public key (hex): 0x020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1
  Account ID:       0x01e552298e47454041ea31273b4b630c64c104e4514aa3643490b8aaca9cf8ed
  SS58 Address:     5C7C2Z5sWbytvHpuLTvzKunnnRwQxft1jiqrLD5rhucQ5S9X

When I derive from the same mnemonic in Apps, I get a different result.
image

We can confirm this by importing the account subkey generated by its private key rather than its mnemonic.
image

To summarize:
image

cc @joelamouche

@jacogr
Copy link
Member

jacogr commented Jan 21, 2021

Good catch, the derivation doesn't use the correct patch.

@jacogr jacogr transferred this issue from polkadot-js/apps Jan 21, 2021
@jacogr
Copy link
Member

jacogr commented Jan 21, 2021

It does match -

image

And with the seed itself -

image

Are you doing this against Moonbeam?

@jacogr
Copy link
Member

jacogr commented Jan 21, 2021

Yes, you are doing it against moonbeam. Ok the addresses in Moonbeam show as "Ethereum addresses", so it will never match as-is to the ss58. And they are not the same as the Moonbeam Ethereum accounts. (Which is why is says "non-ETH compatible), Moonbeam has a specific ethereum-type and your cannot mix/match the 2 and expect the same output.

Substrate edcsa is not the same as Ethereum edcsa.

@jacogr jacogr transferred this issue from polkadot-js/common Jan 21, 2021
@joelamouche
Copy link
Contributor

This is the issue right: polkadot-js/common#773 ?

@jacogr
Copy link
Member

jacogr commented Jan 27, 2021

See this test-case added (previously was daly missing) - polkadot-js/common#850

util-crypto matches subkey

Not sure how the Moonbeam keys are derived. If indeed via an Ethereum-compatible HDKey, yes, it won't match. The keys on the dev keyring are always inserted as sr25519 like in Substrate, so it may be mismatched crypto as well. (dev-keyring is not configurable with the dev-key types, it is just always sr25519)

@jacogr
Copy link
Member

jacogr commented Jan 27, 2021

Anyway, going to close this one - as per the above tests, the derived keys do match Substrate 1-to-1, i.e. there is no mismatch between subkey and the derivation, see https://github.com/polkadot-js/common/pull/850/files#diff-6a57f0d7cdb19e38f51b979d37a92c636e63328129f4776dfdf382a0840b83d1R14-R20.

@jacogr jacogr closed this as completed Jan 27, 2021
@joelamouche
Copy link
Contributor

If indeed via an Ethereum-compatible HDKey, yes, it won't match.

@JoshOrndorff We are indeed using ethereum compatible derivation, right?

So @jacogr if I solve polkadot-js/common#773, the problem will be solved, right?

@jacogr
Copy link
Member

jacogr commented Jan 27, 2021

No, it won't be solved - since the dev keyring always does only sr25519 - https://github.com/polkadot-js/common/blob/master/packages/keyring/src/testing.ts#L75

It doesn't actually even derive them since that adds a (exceptionally noticable) 2-3s delay (cased by the mnemonic -> miniSecret dance), so it knows the exact sr25519 keypairs and just injects them.

Well, it may be solved alongside the above if -

  • the seeds are actually adjusted in the dev keyring (they are not correct for the isDerived path, the line creating from suri needs to add a // at the front - it is a known bug, but actually not a "bug" since the originals were in that form, so it is correct in the past, but not for current purposes)
  • the isDerived flag is passed when constructing the test keyring (which is actually the one used in the apps UI, the dev accounts are just available on non-dev instances)

@joelamouche
Copy link
Contributor

Let me try to clarify:

  • our original problem was that when using the app and adding an address with [dev-memnonic]+'//Alice', we don't get the expected Alice address, which from my understanding would be solved if we just fix the derivation function to handle Ethereum derivation (common#773). Right @JoshOrndorff ?
  • what you are explaining in the previous comment is how to have Alice and Bob included in the dev accounts automatically (I actually didn't know about this feature: can I do that through the UI?), correct @jacogr ?

@jacogr
Copy link
Member

jacogr commented Jan 27, 2021

Connect to a dev node, the Alice/Bob accounts are injected. (And the dev option is available in creating accounts)

@crystalin
Copy link
Contributor

Yes @jacogr is correct, @JoshOrndorff .
You can't use the subkey to retrieve the address of //Alice as it only use the Substrate ECDSA which is not compatible with Ethereum ECDSA that we use.
The problem was also described in another ticket polkadot-js/common#773

@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants