From 108eec3feeca2e05d3daf1a198b8286623f56755 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi <1591639+s1na@users.noreply.github.com> Date: Wed, 25 Aug 2021 22:34:22 +0200 Subject: [PATCH] node: remove dependency on wallet backend packages (#23019) * accounts: new AddBackends method in manager * node,cmd/geth: mv accman backend init to cmd/geth * node,cmd/geth: mv scrypt config downstreawm from node * accounts: use static buffer size for accman sub chan minor fix * accounts,cmd/geth: update accman backends through its event loop * accounts,node: add comments * accounts: un-export newBackendEvent * accounts: use chan instead of wg in newBlockEvent * node: rename isKeyDirEphem * accounts,cmd: AddBackends->AddBackend * accounts: fix potential blocking when adding backend --- accounts/manager.go | 64 ++++++++++++++++++++++++++------- cmd/geth/accountcmd.go | 9 +++-- cmd/geth/config.go | 68 +++++++++++++++++++++++++++++++++++ node/config.go | 82 ++++++++---------------------------------- node/node.go | 23 +++++++----- 5 files changed, 156 insertions(+), 90 deletions(-) diff --git a/accounts/manager.go b/accounts/manager.go index acf41ed8e..1e111d194 100644 --- a/accounts/manager.go +++ b/accounts/manager.go @@ -25,6 +25,10 @@ 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. // // TODO(rjl493456442, karalabe, holiman): Get rid of this when account management @@ -33,18 +37,27 @@ 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 + processed chan struct{} // Informs event emitter that backend has been integrated +} + // 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 quit chan chan error + term chan struct{} // Channel is closed upon termination of the update loop lock sync.RWMutex } @@ -57,7 +70,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 { @@ -65,12 +78,14 @@ 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), + term: make(chan struct{}), } for _, backend := range backends { kind := reflect.TypeOf(backend) @@ -93,6 +108,14 @@ func (am *Manager) Config() *Config { return am.config } +// 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) 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 // and updating the cache of wallets. func (am *Manager) update() { @@ -122,10 +145,22 @@ 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() + 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 } } @@ -133,6 +168,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/accountcmd.go b/cmd/geth/accountcmd.go index 6d45c8876..e33b9eb0f 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 604f907b7..c97a64f17 100644 --- a/cmd/geth/config.go +++ b/cmd/geth/config.go @@ -27,6 +27,10 @@ import ( "gopkg.in/urfave/cli.v1" + "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 +139,11 @@ func makeConfigNode(ctx *cli.Context) (*node.Node, gethConfig) { if err != nil { utils.Fatalf("Failed to create the protocol stack: %v", err) } + // 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) + } + utils.SetEthConfig(ctx, stack, &cfg.Eth) if ctx.GlobalIsSet(utils.EthStatsURLFlag.Name) { cfg.Ethstats.URL = ctx.GlobalString(utils.EthStatsURLFlag.Name) @@ -257,3 +266,62 @@ func deprecated(field string) bool { return false } } + +func setAccountManagerBackends(stack *node.Node) error { + conf := stack.Config() + am := stack.AccountManager() + keydir := stack.KeyStoreDir() + scryptN := keystore.StandardScryptN + scryptP := keystore.StandardScryptP + if conf.UseLightweightKDF { + scryptN = keystore.LightScryptN + scryptP = keystore.LightScryptP + } + + // Assemble the supported backends + if len(conf.ExternalSigner) > 0 { + log.Info("Using external signer", "url", conf.ExternalSigner) + if extapi, err := external.NewExternalBackend(conf.ExternalSigner); err == nil { + am.AddBackend(extapi) + return nil + } else { + return fmt.Errorf("error connecting to external signer: %v", err) + } + } + + // 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) + } + // 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) + } + } + + return nil +} diff --git a/node/config.go b/node/config.go index b7e99eb7b..26f00cd67 100644 --- a/node/config.go +++ b/node/config.go @@ -26,11 +26,6 @@ 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" @@ -430,15 +425,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 := keystore.StandardScryptN - scryptP := keystore.StandardScryptP - if c.UseLightweightKDF { - scryptN = keystore.LightScryptN - scryptP = keystore.LightScryptP - } - +// KeyDirConfig determines the settings for keydirectory +func (c *Config) KeyDirConfig() (string, error) { var ( keydir string err error @@ -455,71 +443,31 @@ func (c *Config) AccountConfig() (int, int, string, error) { case c.KeyStoreDir != "": keydir, err = filepath.Abs(c.KeyStoreDir) } - return scryptN, scryptP, keydir, err + return keydir, err } -func makeAccountManager(conf *Config) (*accounts.Manager, string, error) { - scryptN, scryptP, keydir, err := conf.AccountConfig() - var ephemeral string +// 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 { + return "", false, err + } + 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 1e65fff1c..ceab1c909 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 + 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 @@ -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.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}) // 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.keyDirTemp { + 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