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

@@ -25,6 +25,10 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/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
+	config      *Config                    // Global account manager configurations
-	backends map[reflect.Type][]Backend // Index of backends currently registered
+	backends    map[reflect.Type][]Backend // Index of backends currently registered
-	updaters []event.Subscription       // Wallet update subscriptions for all backends
+	updaters    []event.Subscription       // Wallet update subscriptions for all backends
-	updates  chan WalletEvent           // Subscription sink for backend wallet changes
+	updates     chan WalletEvent           // Subscription sink for backend wallet changes
-	wallets  []Wallet                   // Cache of all wallets from all registered backends
+	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,
+		config:      config,
-		backends: make(map[reflect.Type][]Backend),
+		backends:    make(map[reflect.Type][]Backend),
-		updaters: subs,
+		updaters:    subs,
-		updates:  updates,
+		updates:     updates,
-		wallets:  wallets,
+		newBackends: make(chan newBackendEvent),
-		quit:     make(chan chan error),
+		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]
 }
 

cmd/XDC/accountcmd.go

@@ -302,11 +302,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 := getPassPhrase("Your new account is locked with a password. Please give a password. Do not forget this password.", true, 0, utils.MakePasswordList(ctx))
 

cmd/XDC/config.go

@@ -28,6 +28,9 @@ import (
 	"unicode"
 
 	"github.com/XinFinOrg/XDPoSChain/XDCx"
+	"github.com/XinFinOrg/XDPoSChain/accounts/keystore"
+	"github.com/XinFinOrg/XDPoSChain/accounts/scwallet"
+	"github.com/XinFinOrg/XDPoSChain/accounts/usbwallet"
 	"github.com/XinFinOrg/XDPoSChain/cmd/utils"
 	"github.com/XinFinOrg/XDPoSChain/common"
 	"github.com/XinFinOrg/XDPoSChain/eth/ethconfig"
@@ -207,6 +210,11 @@ func makeConfigNode(ctx *cli.Context) (*node.Node, XDCConfig) {
 	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.IsSet(utils.EthStatsURLFlag.Name) {
 		cfg.Ethstats.URL = ctx.String(utils.EthStatsURLFlag.Name)
@@ -335,3 +343,51 @@ func applyMetricConfig(ctx *cli.Context, cfg *XDCConfig) {
 		}
 	}
 }
+
+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
+	}
+
+	// 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
+}

node/config.go

@@ -24,10 +24,6 @@ import (
 	"runtime"
 	"strings"
 
-	"github.com/XinFinOrg/XDPoSChain/accounts"
-	"github.com/XinFinOrg/XDPoSChain/accounts/keystore"
-	"github.com/XinFinOrg/XDPoSChain/accounts/scwallet"
-	"github.com/XinFinOrg/XDPoSChain/accounts/usbwallet"
 	"github.com/XinFinOrg/XDPoSChain/common"
 	"github.com/XinFinOrg/XDPoSChain/crypto"
 	"github.com/XinFinOrg/XDPoSChain/log"
@@ -389,15 +385,8 @@ func (c *Config) parsePersistentNodes(path string) []*discover.Node {
 	return nodes
 }
 
-// AccountConfig determines the settings for scrypt and keydirectory
+// KeyDirConfig determines the settings for keydirectory
-func (c *Config) AccountConfig() (int, int, string, error) {
+func (c *Config) KeyDirConfig() (string, error) {
-	scryptN := keystore.StandardScryptN
-	scryptP := keystore.StandardScryptP
-	if c.UseLightweightKDF {
-		scryptN = keystore.LightScryptN
-		scryptP = keystore.LightScryptP
-	}
-
 	var (
 		keydir string
 		err    error
@@ -414,61 +403,29 @@ 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) {
+// getKeyStoreDir retrieves the key directory and will create
-	scryptN, scryptP, keydir, err := conf.AccountConfig()
+// and ephemeral one if necessary.
-	var ephemeral string
+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 = os.MkdirTemp("", "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
+		return "", false, err
-	}
-	// Assemble the account manager and supported backends
-	var backends []accounts.Backend
-	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.NoUSB {
-			// 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 accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed}, backends...), ephemeral, nil
+	return keydir, isEphemeral, nil
 }

node/node.go

@@ -41,9 +41,12 @@ import (
 
 // Node is a container on which services can be registered.
 type Node struct {
-	eventmux *event.TypeMux // Event multiplexer used between the services of a stack
+	eventmux   *event.TypeMux // Event multiplexer used between the services of a stack
-	config   *Config
+	config     *Config
-	accman   *accounts.Manager
+	accman     *accounts.Manager
+	log        log.Logger
+	keyDir     string // key store directory
+	keyDirTemp bool   // If true, key directory will be removed by Stop
 
 	ephemeralKeystore string         // if non-empty, the key directory that will be removed by Stop
 	instanceDirLock   flock.Releaser // prevents concurrent use of instance directory
@@ -75,8 +78,6 @@ type Node struct {
 
 	stop chan struct{} // Channel to wait for termination notifications
 	lock sync.RWMutex
-
-	log log.Logger
 }
 
 const (
@@ -96,6 +97,10 @@ func New(conf *Config) (*Node, error) {
 		}
 		conf.DataDir = absdatadir
 	}
+	if conf.Logger == nil {
+		conf.Logger = log.New()
+	}
+
 	// Ensure that the instance name doesn't cause weird conflicts with
 	// other files in the data directory.
 	if strings.ContainsAny(conf.Name, `/\`) {
@@ -107,28 +112,28 @@ func New(conf *Config) (*Node, error) {
 	if strings.HasSuffix(conf.Name, ".ipc") {
 		return nil, errors.New(`Config.Name cannot end in ".ipc"`)
 	}
-	// Ensure that the AccountManager method works before the node has started.
+
-	// We rely on this in cmd/geth.
+	node := &Node{
-	am, ephemeralKeystore, err := makeAccountManager(conf)
+		config:       conf,
+		eventmux:     new(event.TypeMux),
+		log:          conf.Logger,
+		serviceFuncs: []ServiceConstructor{},
+		ipcEndpoint:  conf.IPCEndpoint(),
+		httpEndpoint: conf.HTTPEndpoint(),
+		wsEndpoint:   conf.WSEndpoint(),
+	}
+
+	keyDir, isEphem, err := getKeyStoreDir(conf)
 	if err != nil {
 		return nil, err
 	}
-	if conf.Logger == nil {
+	node.keyDir = keyDir
-		conf.Logger = log.New()
+	node.keyDirTemp = isEphem
-	}
+	// Creates an empty AccountManager with no backends. Callers (e.g. cmd/geth)
-	// Note: any interaction with Config that would create/touch files
+	// are required to add the backends later on.
-	// in the data directory or instance directory is delayed until Start.
+	node.accman = accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed})
-	return &Node{
+
-		accman:            am,
+	return node, nil
-		ephemeralKeystore: ephemeralKeystore,
-		config:            conf,
-		serviceFuncs:      []ServiceConstructor{},
-		ipcEndpoint:       conf.IPCEndpoint(),
-		httpEndpoint:      conf.HTTPEndpoint(),
-		wsEndpoint:        conf.WSEndpoint(),
-		eventmux:          new(event.TypeMux),
-		log:               conf.Logger,
-	}, nil
 }
 
 // Close stops the Node and releases resources acquired in
@@ -143,6 +148,12 @@ func (n *Node) Close() error {
 	if err := n.accman.Close(); err != nil {
 		errs = append(errs, err)
 	}
+	if n.keyDirTemp {
+		if err := os.RemoveAll(n.keyDir); err != nil {
+			errs = append(errs, err)
+		}
+	}
+
 	// Report any errors that might have occurred
 	switch len(errs) {
 	case 0:
@@ -569,6 +580,11 @@ func (n *Node) RPCHandler() (*rpc.Server, error) {
 	return n.inprocHandler, nil
 }
 
+// Config returns the configuration of node.
+func (n *Node) Config() *Config {
+	return n.config
+}
+
 // Server retrieves the currently running P2P network layer. This method is meant
 // only to inspect fields of the currently running server, life cycle management
 // should be left to this Node entity.
@@ -608,6 +624,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