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

accounts: switch Ledger derivation path to canonical one #19438

Merged
merged 2 commits into from Apr 25, 2019

Conversation

Projects
None yet
3 participants
@karalabe
Copy link
Member

commented Apr 10, 2019

The crux of this PR is adding support for the new Ledger derivation path (standardised), opposed to the old one they used.

This could have been done trivially (#17387), at the expense of dropping support for the old paths. That's not really an option, because it would result in people losing default access to those accounts and freaking out (even though they could be recovered manually).

An alternative approach proposed was in #19358, which introduced a CLI flag to switch between the old and new paths. That doesn't fly either because the use all of a sudden needs to be aware of deterministic wallet internals just to use a HW wallet.

This PR takes a more complex approach that keeps the dirty details hidden. It extends self derivation to support multiple base paths. This ensures that HW wallets can discover non-zero accounts from both the legacy derivation path as well as the standardized one. One special case added is that the auto-derived last empty account is only done for the last derivation path (i.e. the standard one). This ensures that in time, people will switch over to new paths even if the old ones are currently used.


Beside the above feature, this PR does 2 changes:

  • It reverts a faulty feature that was added to support deriving multiple empty accounts.
    • The original goal of that feature request was for Clef to support deriving N accounts, but we already have a public Derive method that Clef already uses, so the feature it not necessary.
    • The implementation was broken, because self derivation only monitors the last empty account, so it won't derive new ones if a non-last account is funded.
    • Another bug is that funding the last empty account would derive yet again N new empty accounts.
    • This functionality was added to a private method, so was really not even callable from the outside.
  • It fixes a bug in Clef whereby it modified the global DefaultBaseDerivationPath instead of creating a copy when deriving accounts.

karalabe added some commits Apr 10, 2019

@gballet

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

  * Another bug is that funding the last empty account would derive yet again N new empty accounts.

That is incorrect, it only derives N accounts because emptyCount is set only once.

@karalabe

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

@gballet I've already given you an example yesterday on chat that your code was wrong:

INFO [04-10|13:38:36.698] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0x9d9D86aCa9fbCaCE6CF215f0b00BF7747D5CdC43 path=m/44'/60'/0'/0/0 balance=0 nonce=0
INFO [04-10|13:38:38.070] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0x5Ac01c64BDD2E9a51cCCE2D96aFf27f4cC3a119f path=m/44'/60'/0'/0/1 balance=0 nonce=0
INFO [04-10|13:38:39.427] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0xF9139D9Fc8e4Fdaf70a03b005e68e0791D8835e5 path=m/44'/60'/0'/0/2 balance=0 nonce=0
INFO [04-10|13:38:40.795] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0xaEC1fda21276BA47c7685ed4ca66A216Aa9D7517 path=m/44'/60'/0'/0/3 balance=0 nonce=0

> eth.sendTransaction({from: eth.accounts[0], to: "0xaEC1fda21276BA47c7685ed4ca66A216Aa9D7517", value: 1})
INFO [04-10|13:39:13.315] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0xc6a8333613FD2C4E8635EF42f1ECAB1a11D8f5E9 path=m/44'/60'/0'/0/4 balance=0 nonce=0
INFO [04-10|13:39:14.914] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0x86683FB37ccC2D5eF468c39456E6729bf8a493C7 path=m/44'/60'/0'/0/5 balance=0 nonce=0
INFO [04-10|13:39:16.505] Smartcard wallet discovered new account  url=pcsc://4433d156                          address=0x71C9a77881a5446c10FaeE31d8D5f0f031Ce04D5 path=m/44'/60'/0'/0/6 balance=0 nonce=0
@karalabe

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Self derivation runs periodically when certain events hit. It's not a one-shot method. You will derive new empty accounts every time it's run an the last one is not empty.

@gballet gballet self-assigned this Apr 18, 2019

@gballet
Copy link
Member

left a comment

Checked that the derivation still works with the smartcard, LGTM.

@karalabe karalabe merged commit 7c91038 into ethereum:master Apr 25, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.