From b71334ac3de38338e618aaf8ea6b4a884d2d80f5 Mon Sep 17 00:00:00 2001 From: Kristofer Peterson Date: Sun, 29 Nov 2020 12:43:15 +0000 Subject: [PATCH] accounts, signer: fix Ledger Live account derivation path (clef) (#21757) * signer/core/api: fix derivation of ledger live accounts For ledger hardware wallets, change account iteration as follows: - ledger legacy: m/44'/60'/0'/X; for 0<=X<5 - ledger live: m/44'/60'/0'/0/X; for 0<=X<5 - ledger legacy: m/44'/60'/0'/X; for 0<=X<10 - ledger live: m/44'/60'/X'/0/0; for 0<=X<10 Non-ledger derivation is unchanged and remains as: - non-ledger: m/44'/60'/0'/0/X; for 0<=X<10 * signer/core/api: derive ten default paths for all hardware wallets, plus ten legacy and ten live paths for ledger wallets * signer/core/api: as .../0'/0/0 already included by default paths, do not include it again with ledger live paths * accounts, signer: implement path iterators for hd wallets Co-authored-by: Martin Holst Swende --- accounts/hd.go | 28 +++++++++++++ accounts/hd_test.go | 39 ++++++++++++++++++ signer/core/api.go | 97 +++++++++++++++++++++++---------------------- 3 files changed, 117 insertions(+), 47 deletions(-) diff --git a/accounts/hd.go b/accounts/hd.go index 75c4761106..54acea3b26 100644 --- a/accounts/hd.go +++ b/accounts/hd.go @@ -150,3 +150,31 @@ func (path *DerivationPath) UnmarshalJSON(b []byte) error { *path, err = ParseDerivationPath(dp) return err } + +// DefaultIterator creates a BIP-32 path iterator, which progresses by increasing the last component: +// i.e. m/44'/60'/0'/0/0, m/44'/60'/0'/0/1, m/44'/60'/0'/0/2, ... m/44'/60'/0'/0/N. +func DefaultIterator(base DerivationPath) func() DerivationPath { + path := make(DerivationPath, len(base)) + copy(path[:], base[:]) + // Set it back by one, so the first call gives the first result + path[len(path)-1]-- + return func() DerivationPath { + path[len(path)-1]++ + return path + } +} + +// LedgerLiveIterator creates a bip44 path iterator for Ledger Live. +// Ledger Live increments the third component rather than the fifth component +// i.e. m/44'/60'/0'/0/0, m/44'/60'/1'/0/0, m/44'/60'/2'/0/0, ... m/44'/60'/N'/0/0. +func LedgerLiveIterator(base DerivationPath) func() DerivationPath { + path := make(DerivationPath, len(base)) + copy(path[:], base[:]) + // Set it back by one, so the first call gives the first result + path[2]-- + return func() DerivationPath { + // ledgerLivePathIterator iterates on the third component + path[2]++ + return path + } +} diff --git a/accounts/hd_test.go b/accounts/hd_test.go index 3156a487ee..0743bbe666 100644 --- a/accounts/hd_test.go +++ b/accounts/hd_test.go @@ -17,6 +17,7 @@ package accounts import ( + "fmt" "reflect" "testing" ) @@ -77,3 +78,41 @@ func TestHDPathParsing(t *testing.T) { } } } + +func testDerive(t *testing.T, next func() DerivationPath, expected []string) { + t.Helper() + for i, want := range expected { + if have := next(); fmt.Sprintf("%v", have) != want { + t.Errorf("step %d, have %v, want %v", i, have, want) + } + } +} + +func TestHdPathIteration(t *testing.T) { + testDerive(t, DefaultIterator(DefaultBaseDerivationPath), + []string{ + "m/44'/60'/0'/0/0", "m/44'/60'/0'/0/1", + "m/44'/60'/0'/0/2", "m/44'/60'/0'/0/3", + "m/44'/60'/0'/0/4", "m/44'/60'/0'/0/5", + "m/44'/60'/0'/0/6", "m/44'/60'/0'/0/7", + "m/44'/60'/0'/0/8", "m/44'/60'/0'/0/9", + }) + + testDerive(t, DefaultIterator(LegacyLedgerBaseDerivationPath), + []string{ + "m/44'/60'/0'/0", "m/44'/60'/0'/1", + "m/44'/60'/0'/2", "m/44'/60'/0'/3", + "m/44'/60'/0'/4", "m/44'/60'/0'/5", + "m/44'/60'/0'/6", "m/44'/60'/0'/7", + "m/44'/60'/0'/8", "m/44'/60'/0'/9", + }) + + testDerive(t, LedgerLiveIterator(DefaultBaseDerivationPath), + []string{ + "m/44'/60'/0'/0/0", "m/44'/60'/1'/0/0", + "m/44'/60'/2'/0/0", "m/44'/60'/3'/0/0", + "m/44'/60'/4'/0/0", "m/44'/60'/5'/0/0", + "m/44'/60'/6'/0/0", "m/44'/60'/7'/0/0", + "m/44'/60'/8'/0/0", "m/44'/60'/9'/0/0", + }) +} diff --git a/signer/core/api.go b/signer/core/api.go index 43926a75ff..7595d2d484 100644 --- a/signer/core/api.go +++ b/signer/core/api.go @@ -322,62 +322,65 @@ func (api *SignerAPI) openTrezor(url accounts.URL) { // startUSBListener starts a listener for USB events, for hardware wallet interaction func (api *SignerAPI) startUSBListener() { - events := make(chan accounts.WalletEvent, 16) + eventCh := make(chan accounts.WalletEvent, 16) am := api.am - am.Subscribe(events) - go func() { + am.Subscribe(eventCh) + // Open any wallets already attached + for _, wallet := range am.Wallets() { + if err := wallet.Open(""); err != nil { + log.Warn("Failed to open wallet", "url", wallet.URL(), "err", err) + if err == usbwallet.ErrTrezorPINNeeded { + go api.openTrezor(wallet.URL()) + } + } + } + go api.derivationLoop(eventCh) +} - // Open any wallets already attached - for _, wallet := range am.Wallets() { - if err := wallet.Open(""); err != nil { - log.Warn("Failed to open wallet", "url", wallet.URL(), "err", err) +// derivationLoop listens for wallet events +func (api *SignerAPI) derivationLoop(events chan accounts.WalletEvent) { + // Listen for wallet event till termination + for event := range events { + switch event.Kind { + case accounts.WalletArrived: + if err := event.Wallet.Open(""); err != nil { + log.Warn("New wallet appeared, failed to open", "url", event.Wallet.URL(), "err", err) if err == usbwallet.ErrTrezorPINNeeded { - go api.openTrezor(wallet.URL()) + go api.openTrezor(event.Wallet.URL()) } } - } - // Listen for wallet event till termination - for event := range events { - switch event.Kind { - case accounts.WalletArrived: - if err := event.Wallet.Open(""); err != nil { - log.Warn("New wallet appeared, failed to open", "url", event.Wallet.URL(), "err", err) - if err == usbwallet.ErrTrezorPINNeeded { - go api.openTrezor(event.Wallet.URL()) + case accounts.WalletOpened: + status, _ := event.Wallet.Status() + log.Info("New wallet appeared", "url", event.Wallet.URL(), "status", status) + var derive = func(limit int, next func() accounts.DerivationPath) { + // Derive first N accounts, hardcoded for now + for i := 0; i < limit; i++ { + path := next() + if acc, err := event.Wallet.Derive(path, true); err != nil { + log.Warn("Account derivation failed", "error", err) + } else { + log.Info("Derived account", "address", acc.Address, "path", path) } } - case accounts.WalletOpened: - status, _ := event.Wallet.Status() - log.Info("New wallet appeared", "url", event.Wallet.URL(), "status", status) - var derive = func(numToDerive int, base accounts.DerivationPath) { - // Derive first N accounts, hardcoded for now - var nextPath = make(accounts.DerivationPath, len(base)) - copy(nextPath[:], base[:]) - - for i := 0; i < numToDerive; i++ { - acc, err := event.Wallet.Derive(nextPath, true) - if err != nil { - log.Warn("Account derivation failed", "error", err) - } else { - log.Info("Derived account", "address", acc.Address, "path", nextPath) - } - nextPath[len(nextPath)-1]++ - } - } - if event.Wallet.URL().Scheme == "ledger" { - log.Info("Deriving ledger default paths") - derive(numberOfAccountsToDerive/2, accounts.DefaultBaseDerivationPath) - log.Info("Deriving ledger legacy paths") - derive(numberOfAccountsToDerive/2, accounts.LegacyLedgerBaseDerivationPath) - } else { - derive(numberOfAccountsToDerive, accounts.DefaultBaseDerivationPath) - } - case accounts.WalletDropped: - log.Info("Old wallet dropped", "url", event.Wallet.URL()) - event.Wallet.Close() } + log.Info("Deriving default paths") + derive(numberOfAccountsToDerive, accounts.DefaultIterator(accounts.DefaultBaseDerivationPath)) + if event.Wallet.URL().Scheme == "ledger" { + log.Info("Deriving ledger legacy paths") + derive(numberOfAccountsToDerive, accounts.DefaultIterator(accounts.LegacyLedgerBaseDerivationPath)) + log.Info("Deriving ledger live paths") + // For ledger live, since it's based off the same (DefaultBaseDerivationPath) + // as one we've already used, we need to step it forward one step to avoid + // hitting the same path again + nextFn := accounts.LedgerLiveIterator(accounts.DefaultBaseDerivationPath) + nextFn() + derive(numberOfAccountsToDerive, nextFn) + } + case accounts.WalletDropped: + log.Info("Old wallet dropped", "url", event.Wallet.URL()) + event.Wallet.Close() } - }() + } } // List returns the set of wallet this signer manages. Each wallet can contain