From 2526a48636ef015fc2480a3b610afc441f05ee4e Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Mon, 7 Jun 2021 17:19:50 +0200 Subject: [PATCH 01/11] accounts: new AddBackends method in manager --- accounts/manager.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/accounts/manager.go b/accounts/manager.go index acf41ed8e9e2..1f5670355b36 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -93,6 +93,35 @@ func (am *Manager) Config() *Config { return am.config } +// AddBackends starts the tracking of additional backends for wallet updates. +func (am *Manager) AddBackends(backends ...Backend) error { + // Tear down event loop to update channels + if err := am.Close(); err != nil { + return err + } + + for _, backend := range backends { + am.wallets = merge(am.wallets, backend.Wallets()...) + } + // Subscribe to wallet notifications from all backends + am.updates = make(chan WalletEvent, 4*(len(backends)+len(am.backends))) + + subs := make([]event.Subscription, len(backends)) + for i, backend := range backends { + subs[i] = backend.Subscribe(am.updates) + } + am.updaters = append(am.updaters, subs...) + + for _, backend := range backends { + kind := reflect.TypeOf(backend) + am.backends[kind] = append(am.backends[kind], backend) + } + + // Restart event loop + go am.update() + return nil +} + // update is the wallet event loop listening for notifications from the backends // and updating the cache of wallets. func (am *Manager) update() { From 30a45b261e2b8761902d0fc546a76c17dfe41f6f Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 8 Jun 2021 12:40:31 +0200 Subject: [PATCH 02/11] node,cmd/geth: mv accman backend init to cmd/geth --- cmd/geth/config.go | 64 ++++++++++++++++++++++++++++++++++++ node/config.go | 81 +++++++++++----------------------------------- node/node.go | 23 ++++++++----- 3 files changed, 98 insertions(+), 70 deletions(-) diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 117be96ae971..8a056df88255 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -27,6 +27,11 @@ import ( "gopkg.in/urfave/cli.v1" + "github.com/ethereum/go-ethereum/accounts" + "github.com/ethereum/go-ethereum/accounts/external" + "github.com/ethereum/go-ethereum/accounts/keystore" + "github.com/ethereum/go-ethereum/accounts/scwallet" + "github.com/ethereum/go-ethereum/accounts/usbwallet" "github.com/ethereum/go-ethereum/cmd/utils" "github.com/ethereum/go-ethereum/eth/catalyst" "github.com/ethereum/go-ethereum/eth/ethconfig" @@ -135,6 +140,11 @@ func makeConfigNode(ctx *cli.Context) (*node.Node, gethConfig) { if err != nil { utils.Fatalf("Failed to create the protocol stack: %v", err) } + // Set account manager backends + if err := setAccountManagerBackends(stack); err != nil { + utils.Fatalf("Failed to set account manager backends: %v", err) + } + utils.SetEthConfig(ctx, stack, &cfg.Eth) if ctx.GlobalIsSet(utils.EthStatsURLFlag.Name) { cfg.Ethstats.URL = ctx.GlobalString(utils.EthStatsURLFlag.Name) @@ -245,3 +255,57 @@ func deprecated(field string) bool { return false } } + +func setAccountManagerBackends(stack *node.Node) error { + conf := stack.Config() + am := stack.AccountManager() + scryptN, scryptP := conf.ScryptConfig() + keydir := stack.KeyStoreDir() + // Assemble the supported backends + var backends []accounts.Backend + if len(conf.ExternalSigner) > 0 { + log.Info("Using external signer", "url", conf.ExternalSigner) + if extapi, err := external.NewExternalBackend(conf.ExternalSigner); err == nil { + backends = append(backends, extapi) + } else { + return fmt.Errorf("error connecting to external signer: %v", err) + } + } + if len(backends) == 0 { + // For now, we're using EITHER external signer OR local signers. + // If/when we implement some form of lockfile for USB and keystore wallets, + // we can have both, but it's very confusing for the user to see the same + // accounts in both externally and locally, plus very racey. + backends = append(backends, keystore.NewKeyStore(keydir, scryptN, scryptP)) + if conf.USB { + // Start a USB hub for Ledger hardware wallets + if ledgerhub, err := usbwallet.NewLedgerHub(); err != nil { + log.Warn(fmt.Sprintf("Failed to start Ledger hub, disabling: %v", err)) + } else { + backends = append(backends, ledgerhub) + } + // Start a USB hub for Trezor hardware wallets (HID version) + if trezorhub, err := usbwallet.NewTrezorHubWithHID(); err != nil { + log.Warn(fmt.Sprintf("Failed to start HID Trezor hub, disabling: %v", err)) + } else { + backends = append(backends, trezorhub) + } + // Start a USB hub for Trezor hardware wallets (WebUSB version) + if trezorhub, err := usbwallet.NewTrezorHubWithWebUSB(); err != nil { + log.Warn(fmt.Sprintf("Failed to start WebUSB Trezor hub, disabling: %v", err)) + } else { + backends = append(backends, trezorhub) + } + } + if len(conf.SmartCardDaemonPath) > 0 { + // Start a smart card hub + if schub, err := scwallet.NewHub(conf.SmartCardDaemonPath, scwallet.Scheme, keydir); err != nil { + log.Warn(fmt.Sprintf("Failed to start smart card hub, disabling: %v", err)) + } else { + backends = append(backends, schub) + } + } + } + + return am.AddBackends(backends...) +} diff --git a/node/config.go b/node/config.go index ef1da15d7076..2a74f58ab404 100644 --- a/node/config.go +++ b/node/config.go @@ -26,11 +26,7 @@ import ( "strings" "sync" - "github.com/ethereum/go-ethereum/accounts" - "github.com/ethereum/go-ethereum/accounts/external" "github.com/ethereum/go-ethereum/accounts/keystore" - "github.com/ethereum/go-ethereum/accounts/scwallet" - "github.com/ethereum/go-ethereum/accounts/usbwallet" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/log" @@ -431,12 +427,7 @@ func (c *Config) parsePersistentNodes(w *bool, path string) []*enode.Node { // AccountConfig determines the settings for scrypt and keydirectory func (c *Config) AccountConfig() (int, int, string, error) { - scryptN := keystore.StandardScryptN - scryptP := keystore.StandardScryptP - if c.UseLightweightKDF { - scryptN = keystore.LightScryptN - scryptP = keystore.LightScryptP - } + scryptN, scryptP := c.ScryptConfig() var ( keydir string @@ -457,68 +448,34 @@ func (c *Config) AccountConfig() (int, int, string, error) { return scryptN, scryptP, keydir, err } -func makeAccountManager(conf *Config) (*accounts.Manager, string, error) { - scryptN, scryptP, keydir, err := conf.AccountConfig() - var ephemeral string +// ScryptConfig determines the settings for scrypt +func (c *Config) ScryptConfig() (int, int) { + scryptN := keystore.StandardScryptN + scryptP := keystore.StandardScryptP + if c.UseLightweightKDF { + scryptN = keystore.LightScryptN + scryptP = keystore.LightScryptP + } + return scryptN, scryptP +} + +func getKeyStoreDir(conf *Config) (string, bool, error) { + _, _, keydir, err := conf.AccountConfig() + isEphemeral := false if keydir == "" { // There is no datadir. keydir, err = ioutil.TempDir("", "go-ethereum-keystore") - ephemeral = keydir + isEphemeral = true } if err != nil { - return nil, "", err + return "", false, err } if err := os.MkdirAll(keydir, 0700); err != nil { - return nil, "", err - } - // Assemble the account manager and supported backends - var backends []accounts.Backend - if len(conf.ExternalSigner) > 0 { - log.Info("Using external signer", "url", conf.ExternalSigner) - if extapi, err := external.NewExternalBackend(conf.ExternalSigner); err == nil { - backends = append(backends, extapi) - } else { - return nil, "", fmt.Errorf("error connecting to external signer: %v", err) - } - } - if len(backends) == 0 { - // For now, we're using EITHER external signer OR local signers. - // If/when we implement some form of lockfile for USB and keystore wallets, - // we can have both, but it's very confusing for the user to see the same - // accounts in both externally and locally, plus very racey. - backends = append(backends, keystore.NewKeyStore(keydir, scryptN, scryptP)) - if conf.USB { - // Start a USB hub for Ledger hardware wallets - if ledgerhub, err := usbwallet.NewLedgerHub(); err != nil { - log.Warn(fmt.Sprintf("Failed to start Ledger hub, disabling: %v", err)) - } else { - backends = append(backends, ledgerhub) - } - // Start a USB hub for Trezor hardware wallets (HID version) - if trezorhub, err := usbwallet.NewTrezorHubWithHID(); err != nil { - log.Warn(fmt.Sprintf("Failed to start HID Trezor hub, disabling: %v", err)) - } else { - backends = append(backends, trezorhub) - } - // Start a USB hub for Trezor hardware wallets (WebUSB version) - if trezorhub, err := usbwallet.NewTrezorHubWithWebUSB(); err != nil { - log.Warn(fmt.Sprintf("Failed to start WebUSB Trezor hub, disabling: %v", err)) - } else { - backends = append(backends, trezorhub) - } - } - if len(conf.SmartCardDaemonPath) > 0 { - // Start a smart card hub - if schub, err := scwallet.NewHub(conf.SmartCardDaemonPath, scwallet.Scheme, keydir); err != nil { - log.Warn(fmt.Sprintf("Failed to start smart card hub, disabling: %v", err)) - } else { - backends = append(backends, schub) - } - } + return "", false, err } - return accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed}, backends...), ephemeral, nil + return keydir, isEphemeral, nil } var warnLock sync.Mutex diff --git a/node/node.go b/node/node.go index 1e65fff1c271..2673fec34b2f 100644 --- a/node/node.go +++ b/node/node.go @@ -42,7 +42,8 @@ type Node struct { config *Config accman *accounts.Manager log log.Logger - ephemKeystore string // if non-empty, the key directory that will be removed by Stop + keydir string // key store directory + isKeyDirEphem bool // If true, key directory will be removed by Stop dirLock fileutil.Releaser // prevents concurrent use of instance directory stop chan struct{} // Channel to wait for termination notifications server *p2p.Server // Currently running P2P networking layer @@ -112,14 +113,15 @@ func New(conf *Config) (*Node, error) { if err := node.openDataDir(); err != nil { return nil, err } - // Ensure that the AccountManager method works before the node has started. We rely on - // this in cmd/geth. - am, ephemeralKeystore, err := makeAccountManager(conf) + keydir, isEphem, err := getKeyStoreDir(conf) if err != nil { return nil, err } - node.accman = am - node.ephemKeystore = ephemeralKeystore + node.keydir = keydir + node.isKeyDirEphem = isEphem + // Creates an empty AccountManager with no backends. Callers (e.g. cmd/geth) + // are required to add the backends later on. + node.accman = accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed}) // Initialize the p2p server. This creates the node key and discovery databases. node.server.Config.PrivateKey = node.config.NodeKey() @@ -233,8 +235,8 @@ func (n *Node) doClose(errs []error) error { if err := n.accman.Close(); err != nil { errs = append(errs, err) } - if n.ephemKeystore != "" { - if err := os.RemoveAll(n.ephemKeystore); err != nil { + if n.isKeyDirEphem { + if err := os.RemoveAll(n.keydir); err != nil { errs = append(errs, err) } } @@ -514,6 +516,11 @@ func (n *Node) InstanceDir() string { return n.config.instanceDir() } +// KeyStoreDir retrieves the key directory +func (n *Node) KeyStoreDir() string { + return n.keydir +} + // AccountManager retrieves the account manager used by the protocol stack. func (n *Node) AccountManager() *accounts.Manager { return n.accman From 15eab98148be2e5647ce32efb142e2883ed625a9 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 8 Jun 2021 12:54:14 +0200 Subject: [PATCH 03/11] node,cmd/geth: mv scrypt config downstreawm from node --- cmd/geth/accountcmd.go | 9 +++++++-- cmd/geth/config.go | 8 +++++++- node/config.go | 25 +++++++------------------ 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/cmd/geth/accountcmd.go b/cmd/geth/accountcmd.go index 6d45c88763ca..e33b9eb0fb0f 100644 --- a/cmd/geth/accountcmd.go +++ b/cmd/geth/accountcmd.go @@ -268,11 +268,16 @@ func accountCreate(ctx *cli.Context) error { } } utils.SetNodeConfig(ctx, &cfg.Node) - scryptN, scryptP, keydir, err := cfg.Node.AccountConfig() - + keydir, err := cfg.Node.KeyDirConfig() if err != nil { utils.Fatalf("Failed to read configuration: %v", err) } + scryptN := keystore.StandardScryptN + scryptP := keystore.StandardScryptP + if cfg.Node.UseLightweightKDF { + scryptN = keystore.LightScryptN + scryptP = keystore.LightScryptP + } password := utils.GetPassPhraseWithList("Your new account is locked with a password. Please give a password. Do not forget this password.", true, 0, utils.MakePasswordList(ctx)) diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 8a056df88255..701019dd4270 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -259,8 +259,14 @@ func deprecated(field string) bool { func setAccountManagerBackends(stack *node.Node) error { conf := stack.Config() am := stack.AccountManager() - scryptN, scryptP := conf.ScryptConfig() keydir := stack.KeyStoreDir() + scryptN := keystore.StandardScryptN + scryptP := keystore.StandardScryptP + if conf.UseLightweightKDF { + scryptN = keystore.LightScryptN + scryptP = keystore.LightScryptP + } + // Assemble the supported backends var backends []accounts.Backend if len(conf.ExternalSigner) > 0 { diff --git a/node/config.go b/node/config.go index 2a74f58ab404..2ea569f6f13e 100644 --- a/node/config.go +++ b/node/config.go @@ -26,7 +26,6 @@ import ( "strings" "sync" - "github.com/ethereum/go-ethereum/accounts/keystore" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/log" @@ -425,10 +424,8 @@ func (c *Config) parsePersistentNodes(w *bool, path string) []*enode.Node { return nodes } -// AccountConfig determines the settings for scrypt and keydirectory -func (c *Config) AccountConfig() (int, int, string, error) { - scryptN, scryptP := c.ScryptConfig() - +// KeyDirConfig determines the settings for keydirectory +func (c *Config) KeyDirConfig() (string, error) { var ( keydir string err error @@ -445,22 +442,14 @@ func (c *Config) AccountConfig() (int, int, string, error) { case c.KeyStoreDir != "": keydir, err = filepath.Abs(c.KeyStoreDir) } - return scryptN, scryptP, keydir, err -} - -// ScryptConfig determines the settings for scrypt -func (c *Config) ScryptConfig() (int, int) { - scryptN := keystore.StandardScryptN - scryptP := keystore.StandardScryptP - if c.UseLightweightKDF { - scryptN = keystore.LightScryptN - scryptP = keystore.LightScryptP - } - return scryptN, scryptP + return keydir, err } func getKeyStoreDir(conf *Config) (string, bool, error) { - _, _, keydir, err := conf.AccountConfig() + keydir, err := conf.KeyDirConfig() + if err != nil { + return "", false, err + } isEphemeral := false if keydir == "" { // There is no datadir. From a7bfeedf5bac8df82e75e0e952dd56ef4f801c8f Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 8 Jun 2021 16:15:34 +0200 Subject: [PATCH 04/11] accounts: use static buffer size for accman sub chan minor fix --- accounts/manager.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/accounts/manager.go b/accounts/manager.go index 1f5670355b36..750b181fb570 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -25,6 +25,8 @@ import ( "github.com/ethereum/go-ethereum/event" ) +const managerSubBufferSize = 50 + // Config contains the settings of the global account manager. // // TODO(rjl493456442, karalabe, holiman): Get rid of this when account management @@ -57,7 +59,7 @@ func NewManager(config *Config, backends ...Backend) *Manager { wallets = merge(wallets, backend.Wallets()...) } // Subscribe to wallet notifications from all backends - updates := make(chan WalletEvent, 4*len(backends)) + updates := make(chan WalletEvent, managerSubBufferSize) subs := make([]event.Subscription, len(backends)) for i, backend := range backends { @@ -103,8 +105,6 @@ func (am *Manager) AddBackends(backends ...Backend) error { for _, backend := range backends { am.wallets = merge(am.wallets, backend.Wallets()...) } - // Subscribe to wallet notifications from all backends - am.updates = make(chan WalletEvent, 4*(len(backends)+len(am.backends))) subs := make([]event.Subscription, len(backends)) for i, backend := range backends { From d3f613d388aa5075ad8dffbab396e5a557fb28cf Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 8 Jun 2021 18:19:14 +0200 Subject: [PATCH 05/11] accounts,cmd/geth: update accman backends through its event loop --- accounts/manager.go | 74 ++++++++++++++++++++++++--------------------- cmd/geth/config.go | 5 +-- 2 files changed, 43 insertions(+), 36 deletions(-) diff --git a/accounts/manager.go b/accounts/manager.go index 750b181fb570..0842e7d75a63 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -35,14 +35,22 @@ type Config struct { InsecureUnlockAllowed bool // Whether account unlocking in insecure environment is allowed } +// NewBackendEvent lets the manager know it should +// track the given backend for wallet updates. +type NewBackendEvent struct { + backend Backend + wg *sync.WaitGroup +} + // Manager is an overarching account manager that can communicate with various // backends for signing transactions. type Manager struct { - config *Config // Global account manager configurations - backends map[reflect.Type][]Backend // Index of backends currently registered - updaters []event.Subscription // Wallet update subscriptions for all backends - updates chan WalletEvent // Subscription sink for backend wallet changes - wallets []Wallet // Cache of all wallets from all registered backends + config *Config // Global account manager configurations + backends map[reflect.Type][]Backend // Index of backends currently registered + updaters []event.Subscription // Wallet update subscriptions for all backends + updates chan WalletEvent // Subscription sink for backend wallet changes + newBackends chan NewBackendEvent // Incoming backends to be tracked by the manager + wallets []Wallet // Cache of all wallets from all registered backends feed event.Feed // Wallet feed notifying of arrivals/departures @@ -67,12 +75,13 @@ func NewManager(config *Config, backends ...Backend) *Manager { } // Assemble the account manager and return am := &Manager{ - config: config, - backends: make(map[reflect.Type][]Backend), - updaters: subs, - updates: updates, - wallets: wallets, - quit: make(chan chan error), + config: config, + backends: make(map[reflect.Type][]Backend), + updaters: subs, + updates: updates, + newBackends: make(chan NewBackendEvent), + wallets: wallets, + quit: make(chan chan error), } for _, backend := range backends { kind := reflect.TypeOf(backend) @@ -96,30 +105,15 @@ func (am *Manager) Config() *Config { } // AddBackends starts the tracking of additional backends for wallet updates. -func (am *Manager) AddBackends(backends ...Backend) error { - // Tear down event loop to update channels - if err := am.Close(); err != nil { - return err - } - +func (am *Manager) AddBackends(backends ...Backend) { + var wg sync.WaitGroup for _, backend := range backends { - am.wallets = merge(am.wallets, backend.Wallets()...) + wg.Add(1) + am.newBackends <- NewBackendEvent{backend, &wg} } - - subs := make([]event.Subscription, len(backends)) - for i, backend := range backends { - subs[i] = backend.Subscribe(am.updates) - } - am.updaters = append(am.updaters, subs...) - - for _, backend := range backends { - kind := reflect.TypeOf(backend) - am.backends[kind] = append(am.backends[kind], backend) - } - - // Restart event loop - go am.update() - return nil + // cmd/geth assumes after the invocation of AddBackends + // manager is already tracking those backends. + wg.Wait() } // update is the wallet event loop listening for notifications from the backends @@ -151,7 +145,16 @@ func (am *Manager) update() { // Notify any listeners of the event am.feed.Send(event) - + case event := <-am.newBackends: + am.lock.Lock() + // Update caches + backend := event.backend + am.wallets = merge(am.wallets, backend.Wallets()...) + am.updaters = append(am.updaters, backend.Subscribe(am.updates)) + kind := reflect.TypeOf(backend) + am.backends[kind] = append(am.backends[kind], backend) + am.lock.Unlock() + event.wg.Done() case errc := <-am.quit: // Manager terminating, return errc <- nil @@ -162,6 +165,9 @@ func (am *Manager) update() { // Backends retrieves the backend(s) with the given type from the account manager. func (am *Manager) Backends(kind reflect.Type) []Backend { + am.lock.RLock() + defer am.lock.RUnlock() + return am.backends[kind] } diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 701019dd4270..87c47455b012 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -140,7 +140,7 @@ func makeConfigNode(ctx *cli.Context) (*node.Node, gethConfig) { if err != nil { utils.Fatalf("Failed to create the protocol stack: %v", err) } - // Set account manager backends + // Node doesn't by default populate account manager backends if err := setAccountManagerBackends(stack); err != nil { utils.Fatalf("Failed to set account manager backends: %v", err) } @@ -313,5 +313,6 @@ func setAccountManagerBackends(stack *node.Node) error { } } - return am.AddBackends(backends...) + am.AddBackends(backends...) + return nil } From fae215dd0fef30480eb14a7a6591a29ff48d6eb3 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 8 Jun 2021 18:31:34 +0200 Subject: [PATCH 06/11] accounts,node: add comments --- accounts/manager.go | 2 ++ node/config.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/accounts/manager.go b/accounts/manager.go index 0842e7d75a63..1b752933835b 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -25,6 +25,8 @@ import ( "github.com/ethereum/go-ethereum/event" ) +// managerSubBufferSize determines how many incoming wallet events +// the manager will buffer in its channel. const managerSubBufferSize = 50 // Config contains the settings of the global account manager. diff --git a/node/config.go b/node/config.go index 2ea569f6f13e..789d057a19b0 100644 --- a/node/config.go +++ b/node/config.go @@ -445,6 +445,8 @@ func (c *Config) KeyDirConfig() (string, error) { return keydir, err } +// getKeyStoreDir retrieves the key directory and will create +// and ephemeral one if necessary. func getKeyStoreDir(conf *Config) (string, bool, error) { keydir, err := conf.KeyDirConfig() if err != nil { From 6c28591fd31bd638c5e23a5f754391d1f7e4b1d4 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Wed, 9 Jun 2021 09:26:17 +0200 Subject: [PATCH 07/11] accounts: un-export newBackendEvent --- accounts/manager.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/accounts/manager.go b/accounts/manager.go index 1b752933835b..624656c00df8 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -37,9 +37,9 @@ type Config struct { InsecureUnlockAllowed bool // Whether account unlocking in insecure environment is allowed } -// NewBackendEvent lets the manager know it should +// newBackendEvent lets the manager know it should // track the given backend for wallet updates. -type NewBackendEvent struct { +type newBackendEvent struct { backend Backend wg *sync.WaitGroup } @@ -51,7 +51,7 @@ type Manager struct { backends map[reflect.Type][]Backend // Index of backends currently registered updaters []event.Subscription // Wallet update subscriptions for all backends updates chan WalletEvent // Subscription sink for backend wallet changes - newBackends chan NewBackendEvent // Incoming backends to be tracked by the manager + newBackends chan newBackendEvent // Incoming backends to be tracked by the manager wallets []Wallet // Cache of all wallets from all registered backends feed event.Feed // Wallet feed notifying of arrivals/departures @@ -81,7 +81,7 @@ func NewManager(config *Config, backends ...Backend) *Manager { backends: make(map[reflect.Type][]Backend), updaters: subs, updates: updates, - newBackends: make(chan NewBackendEvent), + newBackends: make(chan newBackendEvent), wallets: wallets, quit: make(chan chan error), } @@ -111,7 +111,7 @@ func (am *Manager) AddBackends(backends ...Backend) { var wg sync.WaitGroup for _, backend := range backends { wg.Add(1) - am.newBackends <- NewBackendEvent{backend, &wg} + am.newBackends <- newBackendEvent{backend, &wg} } // cmd/geth assumes after the invocation of AddBackends // manager is already tracking those backends. From 39b3d401e71e0332b48120a2e826867574d91feb Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Wed, 9 Jun 2021 09:39:56 +0200 Subject: [PATCH 08/11] accounts: use chan instead of wg in newBlockEvent --- accounts/manager.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/accounts/manager.go b/accounts/manager.go index 624656c00df8..94d93120827c 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -40,8 +40,8 @@ type Config struct { // newBackendEvent lets the manager know it should // track the given backend for wallet updates. type newBackendEvent struct { - backend Backend - wg *sync.WaitGroup + backend Backend + processed chan struct{} // Informs event emitter that backend has been integrated } // Manager is an overarching account manager that can communicate with various @@ -107,15 +107,13 @@ func (am *Manager) Config() *Config { } // AddBackends starts the tracking of additional backends for wallet updates. +// cmd/geth assumes once this func returns the backends have been already integrated. func (am *Manager) AddBackends(backends ...Backend) { - var wg sync.WaitGroup + done := make(chan struct{}, len(backends)) for _, backend := range backends { - wg.Add(1) - am.newBackends <- newBackendEvent{backend, &wg} + am.newBackends <- newBackendEvent{backend, done} + <-done } - // cmd/geth assumes after the invocation of AddBackends - // manager is already tracking those backends. - wg.Wait() } // update is the wallet event loop listening for notifications from the backends @@ -156,7 +154,7 @@ func (am *Manager) update() { kind := reflect.TypeOf(backend) am.backends[kind] = append(am.backends[kind], backend) am.lock.Unlock() - event.wg.Done() + event.processed <- struct{}{} case errc := <-am.quit: // Manager terminating, return errc <- nil From fbb0975f5505c6c1e3f41c0c8beeb81c6dc2b180 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 3 Aug 2021 17:53:27 +0200 Subject: [PATCH 09/11] node: rename isKeyDirEphem --- node/node.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/node/node.go b/node/node.go index 2673fec34b2f..ceab1c90902b 100644 --- a/node/node.go +++ b/node/node.go @@ -42,8 +42,8 @@ type Node struct { config *Config accman *accounts.Manager log log.Logger - keydir string // key store directory - isKeyDirEphem bool // If true, key directory will be removed by Stop + keyDir string // key store directory + keyDirTemp bool // If true, key directory will be removed by Stop dirLock fileutil.Releaser // prevents concurrent use of instance directory stop chan struct{} // Channel to wait for termination notifications server *p2p.Server // Currently running P2P networking layer @@ -113,12 +113,12 @@ func New(conf *Config) (*Node, error) { if err := node.openDataDir(); err != nil { return nil, err } - keydir, isEphem, err := getKeyStoreDir(conf) + keyDir, isEphem, err := getKeyStoreDir(conf) if err != nil { return nil, err } - node.keydir = keydir - node.isKeyDirEphem = isEphem + node.keyDir = keyDir + node.keyDirTemp = isEphem // Creates an empty AccountManager with no backends. Callers (e.g. cmd/geth) // are required to add the backends later on. node.accman = accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed}) @@ -235,8 +235,8 @@ func (n *Node) doClose(errs []error) error { if err := n.accman.Close(); err != nil { errs = append(errs, err) } - if n.isKeyDirEphem { - if err := os.RemoveAll(n.keydir); err != nil { + if n.keyDirTemp { + if err := os.RemoveAll(n.keyDir); err != nil { errs = append(errs, err) } } @@ -518,7 +518,7 @@ func (n *Node) InstanceDir() string { // KeyStoreDir retrieves the key directory func (n *Node) KeyStoreDir() string { - return n.keydir + return n.keyDir } // AccountManager retrieves the account manager used by the protocol stack. From d67a3311e301d04773ba37d0b951766ccdf7ffcf Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 3 Aug 2021 18:07:49 +0200 Subject: [PATCH 10/11] accounts,cmd: AddBackends->AddBackend --- accounts/manager.go | 12 ++++---- cmd/geth/config.go | 69 ++++++++++++++++++++++----------------------- 2 files changed, 38 insertions(+), 43 deletions(-) diff --git a/accounts/manager.go b/accounts/manager.go index 94d93120827c..0ca7cd7d1204 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -106,14 +106,12 @@ func (am *Manager) Config() *Config { return am.config } -// AddBackends starts the tracking of additional backends for wallet updates. +// AddBackend starts the tracking of an additional backend for wallet updates. // cmd/geth assumes once this func returns the backends have been already integrated. -func (am *Manager) AddBackends(backends ...Backend) { - done := make(chan struct{}, len(backends)) - for _, backend := range backends { - am.newBackends <- newBackendEvent{backend, done} - <-done - } +func (am *Manager) AddBackend(backend Backend) { + done := make(chan struct{}) + am.newBackends <- newBackendEvent{backend, done} + <-done } // update is the wallet event loop listening for notifications from the backends diff --git a/cmd/geth/config.go b/cmd/geth/config.go index 87c47455b012..4445de066ccc 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -27,7 +27,6 @@ import ( "gopkg.in/urfave/cli.v1" - "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/accounts/external" "github.com/ethereum/go-ethereum/accounts/keystore" "github.com/ethereum/go-ethereum/accounts/scwallet" @@ -268,51 +267,49 @@ func setAccountManagerBackends(stack *node.Node) error { } // Assemble the supported backends - var backends []accounts.Backend if len(conf.ExternalSigner) > 0 { log.Info("Using external signer", "url", conf.ExternalSigner) if extapi, err := external.NewExternalBackend(conf.ExternalSigner); err == nil { - backends = append(backends, extapi) + am.AddBackend(extapi) + return nil } else { return fmt.Errorf("error connecting to external signer: %v", err) } } - if len(backends) == 0 { - // For now, we're using EITHER external signer OR local signers. - // If/when we implement some form of lockfile for USB and keystore wallets, - // we can have both, but it's very confusing for the user to see the same - // accounts in both externally and locally, plus very racey. - backends = append(backends, keystore.NewKeyStore(keydir, scryptN, scryptP)) - if conf.USB { - // Start a USB hub for Ledger hardware wallets - if ledgerhub, err := usbwallet.NewLedgerHub(); err != nil { - log.Warn(fmt.Sprintf("Failed to start Ledger hub, disabling: %v", err)) - } else { - backends = append(backends, ledgerhub) - } - // Start a USB hub for Trezor hardware wallets (HID version) - if trezorhub, err := usbwallet.NewTrezorHubWithHID(); err != nil { - log.Warn(fmt.Sprintf("Failed to start HID Trezor hub, disabling: %v", err)) - } else { - backends = append(backends, trezorhub) - } - // Start a USB hub for Trezor hardware wallets (WebUSB version) - if trezorhub, err := usbwallet.NewTrezorHubWithWebUSB(); err != nil { - log.Warn(fmt.Sprintf("Failed to start WebUSB Trezor hub, disabling: %v", err)) - } else { - backends = append(backends, trezorhub) - } + + // For now, we're using EITHER external signer OR local signers. + // If/when we implement some form of lockfile for USB and keystore wallets, + // we can have both, but it's very confusing for the user to see the same + // accounts in both externally and locally, plus very racey. + am.AddBackend(keystore.NewKeyStore(keydir, scryptN, scryptP)) + if conf.USB { + // Start a USB hub for Ledger hardware wallets + if ledgerhub, err := usbwallet.NewLedgerHub(); err != nil { + log.Warn(fmt.Sprintf("Failed to start Ledger hub, disabling: %v", err)) + } else { + am.AddBackend(ledgerhub) + } + // Start a USB hub for Trezor hardware wallets (HID version) + if trezorhub, err := usbwallet.NewTrezorHubWithHID(); err != nil { + log.Warn(fmt.Sprintf("Failed to start HID Trezor hub, disabling: %v", err)) + } else { + am.AddBackend(trezorhub) } - if len(conf.SmartCardDaemonPath) > 0 { - // Start a smart card hub - if schub, err := scwallet.NewHub(conf.SmartCardDaemonPath, scwallet.Scheme, keydir); err != nil { - log.Warn(fmt.Sprintf("Failed to start smart card hub, disabling: %v", err)) - } else { - backends = append(backends, schub) - } + // Start a USB hub for Trezor hardware wallets (WebUSB version) + if trezorhub, err := usbwallet.NewTrezorHubWithWebUSB(); err != nil { + log.Warn(fmt.Sprintf("Failed to start WebUSB Trezor hub, disabling: %v", err)) + } else { + am.AddBackend(trezorhub) + } + } + if len(conf.SmartCardDaemonPath) > 0 { + // Start a smart card hub + if schub, err := scwallet.NewHub(conf.SmartCardDaemonPath, scwallet.Scheme, keydir); err != nil { + log.Warn(fmt.Sprintf("Failed to start smart card hub, disabling: %v", err)) + } else { + am.AddBackend(schub) } } - am.AddBackends(backends...) return nil } From 0bd22e9f40e09f172a7fd3768f0ea6270086ee32 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Tue, 3 Aug 2021 18:50:04 +0200 Subject: [PATCH 11/11] accounts: fix potential blocking when adding backend --- accounts/manager.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/accounts/manager.go b/accounts/manager.go index 0ca7cd7d1204..1e111d19487b 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -57,6 +57,7 @@ type Manager struct { feed event.Feed // Wallet feed notifying of arrivals/departures quit chan chan error + term chan struct{} // Channel is closed upon termination of the update loop lock sync.RWMutex } @@ -84,6 +85,7 @@ func NewManager(config *Config, backends ...Backend) *Manager { newBackends: make(chan newBackendEvent), wallets: wallets, quit: make(chan chan error), + term: make(chan struct{}), } for _, backend := range backends { kind := reflect.TypeOf(backend) @@ -152,10 +154,13 @@ func (am *Manager) update() { kind := reflect.TypeOf(backend) am.backends[kind] = append(am.backends[kind], backend) am.lock.Unlock() - event.processed <- struct{}{} + close(event.processed) case errc := <-am.quit: // Manager terminating, return errc <- nil + // Signals event emitters the loop is not receiving values + // to prevent them from getting stuck. + close(am.term) return } }