From 8cf764da896d77ca2dc2181b24ea52839645185f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Wed, 10 Apr 2019 12:51:22 +0300 Subject: [PATCH 1/2] Revert "Can now specify the number of empty accounts to derive" This reverts commit 5b30aa59d63fcb7ef8111ec89a6f06509b5ce687. --- accounts/scwallet/wallet.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/accounts/scwallet/wallet.go b/accounts/scwallet/wallet.go index 033a46c77..d2b55d0be 100644 --- a/accounts/scwallet/wallet.go +++ b/accounts/scwallet/wallet.go @@ -390,7 +390,7 @@ func (w *Wallet) Open(passphrase string) error { w.deriveReq = make(chan chan struct{}) w.deriveQuit = make(chan chan error) - go w.selfDerive(0) + go w.selfDerive() // Notify anyone listening for wallet events that a new device is accessible go w.Hub.updateFeed.Send(accounts.WalletEvent{Wallet: w, Kind: accounts.WalletOpened}) @@ -426,9 +426,8 @@ func (w *Wallet) Close() error { } // selfDerive is an account derivation loop that upon request attempts to find -// new non-zero accounts. maxEmpty specifies the number of empty accounts that -// should be derived once an initial empty account has been found. -func (w *Wallet) selfDerive(maxEmpty int) { +// new non-zero accounts. +func (w *Wallet) selfDerive() { w.log.Debug("Smart card wallet self-derivation started") defer w.log.Debug("Smart card wallet self-derivation stopped") @@ -466,7 +465,7 @@ func (w *Wallet) selfDerive(maxEmpty int) { context = context.Background() ) - for empty, emptyCount := false, maxEmpty+1; !empty || emptyCount > 0; { + for empty := false; !empty; { // Retrieve the next derived Ethereum account if nextAddr == (common.Address{}) { if nextAcc, err = w.session.derive(nextPath); err != nil { @@ -490,11 +489,9 @@ func (w *Wallet) selfDerive(maxEmpty int) { w.log.Warn("Smartcard wallet nonce retrieval failed", "err", err) break } - // If the next account is empty and no more empty accounts are - // allowed, stop self-derivation. Add the current one nonetheless. + // If the next account is empty, stop self-derivation, but add it nonetheless if balance.Sign() == 0 && nonce == 0 { empty = true - emptyCount-- } // We've just self-derived a new account, start tracking it locally path := make(accounts.DerivationPath, len(nextPath)) @@ -508,7 +505,7 @@ func (w *Wallet) selfDerive(maxEmpty int) { pairing.Accounts[nextAddr] = path // Fetch the next potential account - if !empty || emptyCount > 0 { + if !empty { nextAddr = common.Address{} nextPath[len(nextPath)-1]++ } @@ -592,7 +589,7 @@ func (w *Wallet) Contains(account accounts.Account) bool { // Initialize installs a keypair generated from the provided key into the wallet. func (w *Wallet) Initialize(seed []byte) error { - go w.selfDerive(0) + go w.selfDerive() // DO NOT lock at this stage, as the initialize // function relies on Status() return w.session.initialize(seed) From ae7344d7999723cfef99fd0e01acd12e20cd5a85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Wed, 10 Apr 2019 13:09:08 +0300 Subject: [PATCH 2/2] accounts: switch Ledger derivation path to canonical one --- accounts/accounts.go | 6 +- accounts/external/backend.go | 2 +- accounts/hd.go | 8 +- accounts/keystore/wallet.go | 3 +- accounts/scwallet/wallet.go | 117 ++++++++++++++++-------------- accounts/usbwallet/wallet.go | 137 ++++++++++++++++++++--------------- cmd/geth/main.go | 8 +- signer/core/api.go | 8 +- 8 files changed, 161 insertions(+), 128 deletions(-) diff --git a/accounts/accounts.go b/accounts/accounts.go index a52aa425c..afeb412fe 100644 --- a/accounts/accounts.go +++ b/accounts/accounts.go @@ -92,9 +92,13 @@ type Wallet interface { // opposed to decending into a child path to allow discovering accounts starting // from non zero components. // + // Some hardware wallets switched derivation paths through their evolution, so + // this method supports providing multiple bases to discover old user accounts + // too. Only the last base will be used to derive the next empty account. + // // You can disable automatic account discovery by calling SelfDerive with a nil // chain state reader. - SelfDerive(base DerivationPath, chain ethereum.ChainStateReader) + SelfDerive(bases []DerivationPath, chain ethereum.ChainStateReader) // SignData requests the wallet to sign the hash of the given data // It looks up the account specified either solely via its address contained within, diff --git a/accounts/external/backend.go b/accounts/external/backend.go index 21a313b66..23037f52d 100644 --- a/accounts/external/backend.go +++ b/accounts/external/backend.go @@ -143,7 +143,7 @@ func (api *ExternalSigner) Derive(path accounts.DerivationPath, pin bool) (accou return accounts.Account{}, fmt.Errorf("operation not supported on external signers") } -func (api *ExternalSigner) SelfDerive(base accounts.DerivationPath, chain ethereum.ChainStateReader) { +func (api *ExternalSigner) SelfDerive(bases []accounts.DerivationPath, chain ethereum.ChainStateReader) { log.Error("operation SelfDerive not supported on external signers") } diff --git a/accounts/hd.go b/accounts/hd.go index 24aa777ae..75c476110 100644 --- a/accounts/hd.go +++ b/accounts/hd.go @@ -35,10 +35,10 @@ var DefaultRootDerivationPath = DerivationPath{0x80000000 + 44, 0x80000000 + 60, // at m/44'/60'/0'/0/1, etc. var DefaultBaseDerivationPath = DerivationPath{0x80000000 + 44, 0x80000000 + 60, 0x80000000 + 0, 0, 0} -// DefaultLedgerBaseDerivationPath is the base path from which custom derivation endpoints -// are incremented. As such, the first account will be at m/44'/60'/0'/0, the second -// at m/44'/60'/0'/1, etc. -var DefaultLedgerBaseDerivationPath = DerivationPath{0x80000000 + 44, 0x80000000 + 60, 0x80000000 + 0, 0} +// LegacyLedgerBaseDerivationPath is the legacy base path from which custom derivation +// endpoints are incremented. As such, the first account will be at m/44'/60'/0'/0, the +// second at m/44'/60'/0'/1, etc. +var LegacyLedgerBaseDerivationPath = DerivationPath{0x80000000 + 44, 0x80000000 + 60, 0x80000000 + 0, 0} // DerivationPath represents the computer friendly version of a hierarchical // deterministic wallet account derivaion path. diff --git a/accounts/keystore/wallet.go b/accounts/keystore/wallet.go index 1b36b6dff..498067d49 100644 --- a/accounts/keystore/wallet.go +++ b/accounts/keystore/wallet.go @@ -77,7 +77,8 @@ func (w *keystoreWallet) Derive(path accounts.DerivationPath, pin bool) (account // SelfDerive implements accounts.Wallet, but is a noop for plain wallets since // there is no notion of hierarchical account derivation for plain keystore accounts. -func (w *keystoreWallet) SelfDerive(base accounts.DerivationPath, chain ethereum.ChainStateReader) {} +func (w *keystoreWallet) SelfDerive(bases []accounts.DerivationPath, chain ethereum.ChainStateReader) { +} // signHash attempts to sign the given hash with // the given account. If the wallet does not wrap this particular account, an diff --git a/accounts/scwallet/wallet.go b/accounts/scwallet/wallet.go index d2b55d0be..c7ff3e2d4 100644 --- a/accounts/scwallet/wallet.go +++ b/accounts/scwallet/wallet.go @@ -119,11 +119,11 @@ type Wallet struct { session *Session // The secure communication session with the card log log.Logger // Contextual logger to tag the base with its id - deriveNextPath accounts.DerivationPath // Next derivation path for account auto-discovery - deriveNextAddr common.Address // Next derived account address for auto-discovery - deriveChain ethereum.ChainStateReader // Blockchain state reader to discover used account with - deriveReq chan chan struct{} // Channel to request a self-derivation on - deriveQuit chan chan error // Channel to terminate the self-deriver with + deriveNextPaths []accounts.DerivationPath // Next derivation paths for account auto-discovery (multiple bases supported) + deriveNextAddrs []common.Address // Next derived account addresses for auto-discovery (multiple bases supported) + deriveChain ethereum.ChainStateReader // Blockchain state reader to discover used account with + deriveReq chan chan struct{} // Channel to request a self-derivation on + deriveQuit chan chan error // Channel to terminate the self-deriver with } // NewWallet constructs and returns a new Wallet instance. @@ -460,54 +460,59 @@ func (w *Wallet) selfDerive() { paths []accounts.DerivationPath nextAcc accounts.Account - nextAddr = w.deriveNextAddr - nextPath = w.deriveNextPath + nextPaths = append([]accounts.DerivationPath{}, w.deriveNextPaths...) + nextAddrs = append([]common.Address{}, w.deriveNextAddrs...) context = context.Background() ) - for empty := false; !empty; { - // Retrieve the next derived Ethereum account - if nextAddr == (common.Address{}) { - if nextAcc, err = w.session.derive(nextPath); err != nil { - w.log.Warn("Smartcard wallet account derivation failed", "err", err) + for i := 0; i < len(nextAddrs); i++ { + for empty := false; !empty; { + // Retrieve the next derived Ethereum account + if nextAddrs[i] == (common.Address{}) { + if nextAcc, err = w.session.derive(nextPaths[i]); err != nil { + w.log.Warn("Smartcard wallet account derivation failed", "err", err) + break + } + nextAddrs[i] = nextAcc.Address + } + // Check the account's status against the current chain state + var ( + balance *big.Int + nonce uint64 + ) + balance, err = w.deriveChain.BalanceAt(context, nextAddrs[i], nil) + if err != nil { + w.log.Warn("Smartcard wallet balance retrieval failed", "err", err) break } - nextAddr = nextAcc.Address - } - // Check the account's status against the current chain state - var ( - balance *big.Int - nonce uint64 - ) - balance, err = w.deriveChain.BalanceAt(context, nextAddr, nil) - if err != nil { - w.log.Warn("Smartcard wallet balance retrieval failed", "err", err) - break - } - nonce, err = w.deriveChain.NonceAt(context, nextAddr, nil) - if err != nil { - w.log.Warn("Smartcard wallet nonce retrieval failed", "err", err) - break - } - // If the next account is empty, stop self-derivation, but add it nonetheless - if balance.Sign() == 0 && nonce == 0 { - empty = true - } - // We've just self-derived a new account, start tracking it locally - path := make(accounts.DerivationPath, len(nextPath)) - copy(path[:], nextPath[:]) - paths = append(paths, path) + nonce, err = w.deriveChain.NonceAt(context, nextAddrs[i], nil) + if err != nil { + w.log.Warn("Smartcard wallet nonce retrieval failed", "err", err) + break + } + // If the next account is empty, stop self-derivation, but add for the last base path + if balance.Sign() == 0 && nonce == 0 { + empty = true + if i < len(nextAddrs)-1 { + break + } + } + // We've just self-derived a new account, start tracking it locally + path := make(accounts.DerivationPath, len(nextPaths[i])) + copy(path[:], nextPaths[i][:]) + paths = append(paths, path) - // Display a log message to the user for new (or previously empty accounts) - if _, known := pairing.Accounts[nextAddr]; !known || !empty || nextAddr != w.deriveNextAddr { - w.log.Info("Smartcard wallet discovered new account", "address", nextAddr, "path", path, "balance", balance, "nonce", nonce) - } - pairing.Accounts[nextAddr] = path + // Display a log message to the user for new (or previously empty accounts) + if _, known := pairing.Accounts[nextAddrs[i]]; !known || !empty || nextAddrs[i] != w.deriveNextAddrs[i] { + w.log.Info("Smartcard wallet discovered new account", "address", nextAddrs[i], "path", path, "balance", balance, "nonce", nonce) + } + pairing.Accounts[nextAddrs[i]] = path - // Fetch the next potential account - if !empty { - nextAddr = common.Address{} - nextPath[len(nextPath)-1]++ + // Fetch the next potential account + if !empty { + nextAddrs[i] = common.Address{} + nextPaths[i][len(nextPaths[i])-1]++ + } } } // If there are new accounts, write them out @@ -515,8 +520,8 @@ func (w *Wallet) selfDerive() { err = w.Hub.setPairing(w, pairing) } // Shift the self-derivation forward - w.deriveNextAddr = nextAddr - w.deriveNextPath = nextPath + w.deriveNextAddrs = nextAddrs + w.deriveNextPaths = nextPaths // Self derivation complete, release device lock w.lock.Unlock() @@ -626,16 +631,22 @@ func (w *Wallet) Derive(path accounts.DerivationPath, pin bool) (accounts.Accoun // opposed to decending into a child path to allow discovering accounts starting // from non zero components. // +// Some hardware wallets switched derivation paths through their evolution, so +// this method supports providing multiple bases to discover old user accounts +// too. Only the last base will be used to derive the next empty account. +// // You can disable automatic account discovery by calling SelfDerive with a nil // chain state reader. -func (w *Wallet) SelfDerive(base accounts.DerivationPath, chain ethereum.ChainStateReader) { +func (w *Wallet) SelfDerive(bases []accounts.DerivationPath, chain ethereum.ChainStateReader) { w.lock.Lock() defer w.lock.Unlock() - w.deriveNextPath = make(accounts.DerivationPath, len(base)) - copy(w.deriveNextPath[:], base[:]) - - w.deriveNextAddr = common.Address{} + w.deriveNextPaths = make([]accounts.DerivationPath, len(bases)) + for i, base := range bases { + w.deriveNextPaths[i] = make(accounts.DerivationPath, len(base)) + copy(w.deriveNextPaths[i][:], base[:]) + } + w.deriveNextAddrs = make([]common.Address, len(bases)) w.deriveChain = chain } diff --git a/accounts/usbwallet/wallet.go b/accounts/usbwallet/wallet.go index 2ddfa30a6..7ff9c5372 100644 --- a/accounts/usbwallet/wallet.go +++ b/accounts/usbwallet/wallet.go @@ -83,11 +83,11 @@ type wallet struct { accounts []accounts.Account // List of derive accounts pinned on the hardware wallet paths map[common.Address]accounts.DerivationPath // Known derivation paths for signing operations - deriveNextPath accounts.DerivationPath // Next derivation path for account auto-discovery - deriveNextAddr common.Address // Next derived account address for auto-discovery - deriveChain ethereum.ChainStateReader // Blockchain state reader to discover used account with - deriveReq chan chan struct{} // Channel to request a self-derivation on - deriveQuit chan chan error // Channel to terminate the self-deriver with + deriveNextPaths []accounts.DerivationPath // Next derivation paths for account auto-discovery (multiple bases supported) + deriveNextAddrs []common.Address // Next derived account addresses for auto-discovery (multiple bases supported) + deriveChain ethereum.ChainStateReader // Blockchain state reader to discover used account with + deriveReq chan chan struct{} // Channel to request a self-derivation on + deriveQuit chan chan error // Channel to terminate the self-deriver with healthQuit chan chan error @@ -339,57 +339,62 @@ func (w *wallet) selfDerive() { accs []accounts.Account paths []accounts.DerivationPath - nextAddr = w.deriveNextAddr - nextPath = w.deriveNextPath + nextPaths = append([]accounts.DerivationPath{}, w.deriveNextPaths...) + nextAddrs = append([]common.Address{}, w.deriveNextAddrs...) context = context.Background() ) - for empty := false; !empty; { - // Retrieve the next derived Ethereum account - if nextAddr == (common.Address{}) { - if nextAddr, err = w.driver.Derive(nextPath); err != nil { - w.log.Warn("USB wallet account derivation failed", "err", err) + for i := 0; i < len(nextAddrs); i++ { + for empty := false; !empty; { + // Retrieve the next derived Ethereum account + if nextAddrs[i] == (common.Address{}) { + if nextAddrs[i], err = w.driver.Derive(nextPaths[i]); err != nil { + w.log.Warn("USB wallet account derivation failed", "err", err) + break + } + } + // Check the account's status against the current chain state + var ( + balance *big.Int + nonce uint64 + ) + balance, err = w.deriveChain.BalanceAt(context, nextAddrs[i], nil) + if err != nil { + w.log.Warn("USB wallet balance retrieval failed", "err", err) break } - } - // Check the account's status against the current chain state - var ( - balance *big.Int - nonce uint64 - ) - balance, err = w.deriveChain.BalanceAt(context, nextAddr, nil) - if err != nil { - w.log.Warn("USB wallet balance retrieval failed", "err", err) - break - } - nonce, err = w.deriveChain.NonceAt(context, nextAddr, nil) - if err != nil { - w.log.Warn("USB wallet nonce retrieval failed", "err", err) - break - } - // If the next account is empty, stop self-derivation, but add it nonetheless - if balance.Sign() == 0 && nonce == 0 { - empty = true - } - // We've just self-derived a new account, start tracking it locally - path := make(accounts.DerivationPath, len(nextPath)) - copy(path[:], nextPath[:]) - paths = append(paths, path) + nonce, err = w.deriveChain.NonceAt(context, nextAddrs[i], nil) + if err != nil { + w.log.Warn("USB wallet nonce retrieval failed", "err", err) + break + } + // If the next account is empty, stop self-derivation, but add for the last base path + if balance.Sign() == 0 && nonce == 0 { + empty = true + if i < len(nextAddrs)-1 { + break + } + } + // We've just self-derived a new account, start tracking it locally + path := make(accounts.DerivationPath, len(nextPaths[i])) + copy(path[:], nextPaths[i][:]) + paths = append(paths, path) - account := accounts.Account{ - Address: nextAddr, - URL: accounts.URL{Scheme: w.url.Scheme, Path: fmt.Sprintf("%s/%s", w.url.Path, path)}, - } - accs = append(accs, account) + account := accounts.Account{ + Address: nextAddrs[i], + URL: accounts.URL{Scheme: w.url.Scheme, Path: fmt.Sprintf("%s/%s", w.url.Path, path)}, + } + accs = append(accs, account) - // Display a log message to the user for new (or previously empty accounts) - if _, known := w.paths[nextAddr]; !known || (!empty && nextAddr == w.deriveNextAddr) { - w.log.Info("USB wallet discovered new account", "address", nextAddr, "path", path, "balance", balance, "nonce", nonce) - } - // Fetch the next potential account - if !empty { - nextAddr = common.Address{} - nextPath[len(nextPath)-1]++ + // Display a log message to the user for new (or previously empty accounts) + if _, known := w.paths[nextAddrs[i]]; !known || (!empty && nextAddrs[i] == w.deriveNextAddrs[i]) { + w.log.Info("USB wallet discovered new account", "address", nextAddrs[i], "path", path, "balance", balance, "nonce", nonce) + } + // Fetch the next potential account + if !empty { + nextAddrs[i] = common.Address{} + nextPaths[i][len(nextPaths[i])-1]++ + } } } // Self derivation complete, release device lock @@ -406,8 +411,8 @@ func (w *wallet) selfDerive() { } // Shift the self-derivation forward // TODO(karalabe): don't overwrite changes from wallet.SelfDerive - w.deriveNextAddr = nextAddr - w.deriveNextPath = nextPath + w.deriveNextAddrs = nextAddrs + w.deriveNextPaths = nextPaths w.stateLock.Unlock() // Notify the user of termination and loop after a bit of time (to avoid trashing) @@ -479,18 +484,30 @@ func (w *wallet) Derive(path accounts.DerivationPath, pin bool) (accounts.Accoun return account, nil } -// SelfDerive implements accounts.Wallet, trying to discover accounts that the -// user used previously (based on the chain state), but ones that he/she did not -// explicitly pin to the wallet manually. To avoid chain head monitoring, self -// derivation only runs during account listing (and even then throttled). -func (w *wallet) SelfDerive(base accounts.DerivationPath, chain ethereum.ChainStateReader) { +// SelfDerive sets a base account derivation path from which the wallet attempts +// to discover non zero accounts and automatically add them to list of tracked +// accounts. +// +// Note, self derivaton will increment the last component of the specified path +// opposed to decending into a child path to allow discovering accounts starting +// from non zero components. +// +// Some hardware wallets switched derivation paths through their evolution, so +// this method supports providing multiple bases to discover old user accounts +// too. Only the last base will be used to derive the next empty account. +// +// You can disable automatic account discovery by calling SelfDerive with a nil +// chain state reader. +func (w *wallet) SelfDerive(bases []accounts.DerivationPath, chain ethereum.ChainStateReader) { w.stateLock.Lock() defer w.stateLock.Unlock() - w.deriveNextPath = make(accounts.DerivationPath, len(base)) - copy(w.deriveNextPath[:], base[:]) - - w.deriveNextAddr = common.Address{} + w.deriveNextPaths = make([]accounts.DerivationPath, len(bases)) + for i, base := range bases { + w.deriveNextPaths[i] = make(accounts.DerivationPath, len(base)) + copy(w.deriveNextPaths[i][:], base[:]) + } + w.deriveNextAddrs = make([]common.Address, len(bases)) w.deriveChain = chain } diff --git a/cmd/geth/main.go b/cmd/geth/main.go index 4f3849a41..5425606f1 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -328,11 +328,13 @@ func startNode(ctx *cli.Context, stack *node.Node) { status, _ := event.Wallet.Status() log.Info("New wallet appeared", "url", event.Wallet.URL(), "status", status) - derivationPath := accounts.DefaultBaseDerivationPath + var derivationPaths []accounts.DerivationPath if event.Wallet.URL().Scheme == "ledger" { - derivationPath = accounts.DefaultLedgerBaseDerivationPath + derivationPaths = append(derivationPaths, accounts.LegacyLedgerBaseDerivationPath) } - event.Wallet.SelfDerive(derivationPath, stateReader) + derivationPaths = append(derivationPaths, accounts.DefaultBaseDerivationPath) + + event.Wallet.SelfDerive(derivationPaths, stateReader) case accounts.WalletDropped: log.Info("Old wallet dropped", "url", event.Wallet.URL()) diff --git a/signer/core/api.go b/signer/core/api.go index 9da6ee2a2..b662744bb 100644 --- a/signer/core/api.go +++ b/signer/core/api.go @@ -306,12 +306,10 @@ func (api *SignerAPI) startUSBListener() { status, _ := event.Wallet.Status() log.Info("New wallet appeared", "url", event.Wallet.URL(), "status", status) - derivationPath := accounts.DefaultBaseDerivationPath - if event.Wallet.URL().Scheme == "ledger" { - derivationPath = accounts.DefaultLedgerBaseDerivationPath - } - var nextPath = derivationPath // Derive first N accounts, hardcoded for now + var nextPath = make(accounts.DerivationPath, len(accounts.DefaultBaseDerivationPath)) + copy(nextPath[:], accounts.DefaultBaseDerivationPath[:]) + for i := 0; i < numberOfAccountsToDerive; i++ { acc, err := event.Wallet.Derive(nextPath, true) if err != nil {