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 <martin@swende.se>

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
+	}
+}

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",
+		})
+}

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