cmd/geth: accountcmd no need to acquire the datadir lock

[jsvisa]

Failed to run geth account list when the datadir was locked with a running geth instance:

$ ./build/bin/geth --datadir ~/.ethereum/ account list
INFO [04-11|15:46:02.859] Maximum peer count                       ETH=50 LES=0 total=50
Fatal: Failed to create the protocol stack: datadir already used by another process

[s1na]

When a node is running it can modify the accounts in the keystore, e.g. through API or console. Maybe this would make sense after personal namespace has been fully deleted? (#26390)

[s1na]

This is kind of a hack but you can temporarily check for EnablePersonal flag and only lock datadir then

[jsvisa]

This is kind of a hack but you can temporarily check for EnablePersonal flag and only lock datadir then

this datadir is used for chain data, not the keystore dir, so this is not related to EnablePersonal flag

[holiman]

Regarding the concurrency model, right now, the semantics are as follows:

• We have a mutex protecting the datadir. Typically, if geth is started with --datadir=/tmp/foo, then
  • /tmp/foo
    • keystore
    • geth <-- mutexed area
      • LOCK
• If an empty datadir is used, the mutexing is avoided (ephemeral run)
• Any operation which in any way reads/writes the mutexed area obtains the mutex.
• The mutex description says

Lock the instance directory to prevent concurrent use by another instance as well as
accidental use of the instance directory as a database.

The mutex does not seem to be meant to cover the keystore area. But neither does it only access chaindata -- it's all leveldb databases (e.g. light), it's also the triecache, the jwtsecret and nodekey.

---

Now, the changes in this PR modifies it, such that the mutex is simply not checked in certain conditions.

• accountList
• accountUpdate
• importWallet
• accountImport

I think the PR is semantically correct -- it does respect the existing concurrency model.
I also think that the PR is a bit clunky, adding a true at 7 callsites and a false at 4 callsites, and without diving deeper into how the concurrency model works (as I did), it's by no means trivial for a developer to know what openDataDir means, exactly -- when to pass true, and when to pass false.

All in all, I'm not in favour of this added complexity for what I believe is a very niche usecase (afaiu, it's been the way it for at least 5+ years and I've never heard any complaints about it).

[holiman]

An alternative fix that might be less hacky could be to add, so in addition to the existing

// makeConfigNode loads geth configuration and creates a blank node instance.
func makeConfigNode(ctx *cli.Context) (*node.Node, gethConfig) 

// makeFullNode loads geth configuration and creates the Ethereum backend.
func makeFullNode(ctx *cli.Context) (*node.Node, ethapi.Backend) 

We add a third one, which is even more minimal, e.g. :

// makeSlimNode loads geth configuration and creates a blank node instance which is not chain-aware.
func makeSlimNode(ctx *cli.Context) (*node.Node, gethConfig) 

That one would have to use a different node constructor, e.g:

// NewNonfunctional creates a new P2P node which is incapable of networking, and is not chain-aware.
func NewNonfunctional(conf *Config) (*Node, error) 

And that new constructor would only configure the e.g. keystore, but not initialize the http endpoints or open the chaindata.

I don't know if that would be better, but it's possible. Consider it an idea, not a command :)

[jsvisa]

@holiman Thank you very much for your detailed explanation, as well as the usage scenarios of mutex locks in the code, and your understanding of the tradeoffs in this PR. I will try to simplify the code change based on your suggestions.

[jsvisa]

@holiman PTAL, thanks

[s1na]

I like to build on @holiman's idea and suggest not creating a Node instance at all for account cmds. Only a small portion of its logic will be used anyway. We need to compute the keyDir which can be exposed from node.Config and then do a accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed}). This can be done in a function similar to makeSlimNode maybe named makeAccountManager.

[jsvisa]

@s1na thanks for your advice, please take a look.

[s1na]

Please refactor setAccountManagerBackends so instead of taking a Node, it takes config, am, keydir as params. Then in a new function makeAccountManager you should read in config file if present similar to how makeConfigNode does it and then set the manager backends.

We also need getKeyStoreDir similar to how Node constructor uses it. Please make it a method of node.Config and expose publicly.

[holiman]

This PR turned out pretty clean in the end, LGTM

[s1na]

Nice, thanks!

[s1na]

LGTM