node: remove dependency on wallet backend packages #23019

s1na · 2021-06-08T16:36:40Z

After this PR those importing github.com/ethereum/go-ethereum/node won't need to pull in github.com/ethereum/go-ethereum/accounts/*.

API changes:

Node has an additional method KeyStoreDir
In NodeConfig, AccountConfig is replaced by KeyDirConfig (which doesn't return scrypt config)
Manager's Backends method now needs to acquire a read lock

fjl · 2021-06-09T05:59:03Z

accounts/manager.go

+type NewBackendEvent struct {
+	backend Backend
+	wg      *sync.WaitGroup
+}


This type doesn't need to be exported. Also, it is not common to use WaitGroup for response signaling, it might be better to use a channel.

Good catch re export. Also replaced the wg with a channel.

holiman

I have not tested it, but the code looks good to me

minor fix

s1na · 2021-07-16T12:57:29Z

I rebased the branch and did some more testing. The account manager correctly detects the wallet and adds the accounts for clef and a hdwallet. I did send a tx via clef but getting "transaction type not supported" with a ledger which shouldn't be related to this PR. I didn't have a smart card to test with.

rjl493456442 · 2021-08-03T08:36:41Z

accounts/manager.go

+	done := make(chan struct{}, len(backends))
+	for _, backend := range backends {
+		am.newBackends <- newBackendEvent{backend, done}
+		<-done


We should also check if the quit is closed, otherwise this can be blocked forever.

Quit is not closed ever though, it's a chan chan error. You'd need to add a term chan struct{} that gets closed if you want to check for it. Also, I think the block can only happen in am.newBackends <-, since once we read the event, we will surely respond.

select { case am.newBackends <- newBackendEvent{backend, done}: case <-am.term: return }

karalabe · 2021-08-03T08:37:21Z

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


keyDirTemp

karalabe · 2021-08-03T08:45:07Z

accounts/manager.go

+			kind := reflect.TypeOf(backend)
+			am.backends[kind] = append(am.backends[kind], backend)
+			am.lock.Unlock()
+			event.processed <- struct{}{}


I think you can do close(event.processed) if there's no actual error being returned.

karalabe · 2021-08-03T08:47:24Z

accounts/manager.go

+func (am *Manager) AddBackends(backends ...Backend) {
+	done := make(chan struct{}, len(backends))
+	for _, backend := range backends {
+		am.newBackends <- newBackendEvent{backend, done}


done := make(chan struct)

karalabe · 2021-08-03T09:00:48Z

accounts/manager.go

@@ -93,6 +106,16 @@ func (am *Manager) Config() *Config {
 	return am.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) {


I think it would be cleaner to have just AddBackend. The constructor needs all the backends in one batch, but if we support adding them from the outside async, might as well add one-by-one.

s1na · 2021-08-03T17:53:21Z

Updated with the suggested reviews

* 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

s1na requested review from fjl and renaynay as code owners June 8, 2021 16:36

fjl reviewed Jun 9, 2021

View reviewed changes

holiman approved these changes Jun 28, 2021

View reviewed changes

s1na added 8 commits July 13, 2021 18:19

accounts: new AddBackends method in manager

2526a48

node,cmd/geth: mv accman backend init to cmd/geth

30a45b2

node,cmd/geth: mv scrypt config downstreawm from node

15eab98

accounts: use static buffer size for accman sub chan

a7bfeed

minor fix

accounts,cmd/geth: update accman backends through its event loop

d3f613d

accounts,node: add comments

fae215d

accounts: un-export newBackendEvent

6c28591

accounts: use chan instead of wg in newBlockEvent

39b3d40

s1na force-pushed the node-wallet-deps branch from f54dbc0 to 39b3d40 Compare July 16, 2021 12:48

s1na added the status:triage label Aug 2, 2021

rjl493456442 reviewed Aug 3, 2021

View reviewed changes

karalabe reviewed Aug 3, 2021

View reviewed changes

karalabe modified the milestones: 1.10.6, 1.10.7 Aug 3, 2021

s1na removed the status:triage label Aug 3, 2021

s1na added 3 commits August 3, 2021 17:53

node: rename isKeyDirEphem

fbb0975

accounts,cmd: AddBackends->AddBackend

d67a331

accounts: fix potential blocking when adding backend

0bd22e9

fjl changed the title ~~accounts,node,cmd/geth: remove dependency on wallet backend packages from node~~ node: remove dependency on wallet backend packages Aug 9, 2021

fjl approved these changes Aug 9, 2021

View reviewed changes

karalabe modified the milestones: 1.10.7, 1.10.8 Aug 10, 2021

fjl modified the milestones: 1.10.8, 1.10.9 Aug 24, 2021

fjl self-assigned this Aug 24, 2021

holiman merged commit 108eec3 into ethereum:master Aug 25, 2021

s1na deleted the node-wallet-deps branch August 26, 2021 08:23

fjl mentioned this pull request Aug 26, 2021

blocking issue found at test #23355

Closed

fjl mentioned this pull request Oct 8, 2021

miner/stress: initialize account backends explicitly #23699

Merged

chappjc mentioned this pull request Oct 25, 2021

update geth to at least 1.10.11 decred/dcrdex#1252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: remove dependency on wallet backend packages #23019

node: remove dependency on wallet backend packages #23019

s1na commented Jun 8, 2021

fjl Jun 9, 2021

s1na Jun 9, 2021

holiman left a comment

s1na commented Jul 16, 2021

rjl493456442 Aug 3, 2021

karalabe Aug 3, 2021

karalabe Aug 3, 2021

karalabe Aug 3, 2021

karalabe Aug 3, 2021

karalabe Aug 3, 2021

karalabe Aug 3, 2021

s1na commented Aug 3, 2021

node: remove dependency on wallet backend packages #23019

node: remove dependency on wallet backend packages #23019

Conversation

s1na commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

s1na commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na commented Aug 3, 2021