cmd/geth: remove unlock commandline flag #30737

holiman · 2024-11-06T14:17:32Z

This is one further step towards removing account management from geth. This PR deprecates the flag unlock, and makes the flag moot: unlock via geth is no longer possible.

In order to remove some unneeded code, I also rewrote accountUpdate a bit. I have tested it in both happy-path and various failure modes, e..g

[user@work go-ethereum]$ go run ./cmd/geth --keystore /tmp/foo account update 0xC415017A10aDf514afF75e44748fB28b52588f2D
INFO [11-06|15:10:25.027] Maximum peer count                       ETH=50 total=50
Please give a NEW password. Do not forget this password.
Password: 
Repeat password: 
Please provide the OLD password for account 0xC415017A10aDf514afF75e44748fB28b52588f2D | Attempt 1/3
Password: 
Please provide the OLD password for account 0xC415017A10aDf514afF75e44748fB28b52588f2D | Attempt 2/3
Password: 
Please provide the OLD password for account 0xC415017A10aDf514afF75e44748fB28b52588f2D | Attempt 3/3
Password: 
could not update account: could not decrypt key with given password
exit status 1

[user@work go-ethereum]$ go run ./cmd/geth --keystore /tmp/foo account update blahonga
INFO [11-06|15:16:56.786] Maximum peer count                       ETH=50 total=50
could not locate account: invalid account address or index "blahonga"
exit status 1

This PR also removes the the This functionality is deprecated and will be removed in the future! from here:

[user@work go-ethereum]$ go run ./cmd/geth --keystore /tmp/foo account update 0
INFO [11-06|15:16:12.315] Maximum peer count                       ETH=50 total=50
WARN [11-06|15:16:12.354] -------------------------------------------------------------------
WARN [11-06|15:16:12.355] Referring to accounts by order in the keystore folder is dangerous!
WARN [11-06|15:16:12.355] This functionality is deprecated and will be removed in the future!
WARN [11-06|15:16:12.355] Please use explicit addresses! (can search via `geth account list`)
WARN [11-06|15:16:12.355] -------------------------------------------------------------------
Please give a NEW password. Do not forget this password.
Password: 
Repeat password: 
Please provide the OLD password for account 0 | Attempt 1/3
Password: 
Please provide the OLD password for account 0 | Attempt 2/3
Password:

is now:

[user@work go-ethereum]$ go run ./cmd/geth --keystore /tmp/foo account update 0
INFO [11-06|15:27:08.459] Maximum peer count                       ETH=50 total=50
address must be specified in hexadecimal form
exit status 1

MariusVanDerWijden · 2024-11-07T04:51:57Z

cmd/utils/flags.go

+				Fatalf("Failed to read password file: %v", err)
+			} else {
+				if lines := strings.Split(string(text), "\n"); len(lines) > 0 {
+					passphrase = strings.TrimRight(lines[0], "\r") // Sanitise DOS line endings.


I'm wondering if it makes sense to log something here, if we cut of additional passwords? I guess we didn't log anything here previously, so it might not be needed

MariusVanDerWijden · 2024-11-07T05:04:50Z

cmd/geth/accountcmd.go

+			return errors.New("address must be specified in hexadecimal form")
+		}
+		account := accounts.Account{Address: common.HexToAddress(addr)}
+		newPassword := utils.GetPassPhrase("Please give a NEW password. Do not forget this password.", true)


This seems to be the only major change here, you turned the prompts around, so it asks for the NEW password before it asks for the OLD one to update. This way it only asks you once for the new one and not multiple times if the old fails.
Seemed a bit counter-intuitive to me at first, but I think I agree that this makes most sense

I don't follow. Change or order, yes, but what do you mean I changed about the confirmations?

This way it only asks you once for the new one and not multiple times if the old fails.

So, with the old pw, you still have three retries. And there's a confirmation on the new password, so you do have to enter that twice. So afaict the only change really is that it now asks for the new password before it asks for the old one.

?

MariusVanDerWijden

LGTM, a few people might complain, but I think we should do it. Another step in the right direction!

fjl · 2024-11-07T12:59:20Z

accounts/manager.go

@@ -63,7 +54,7 @@ type Manager struct {

 // NewManager creates a generic account manager to sign transaction via various
 // supported backends.
-func NewManager(config *Config, backends ...Backend) *Manager {
+func NewManager(backends ...Backend) *Manager {


I hate to be that guy again, but this function has had a stable signature for a long time. So I would prefer not to change it. We can simply ignore the setting.

It's a bit weird that we even added it. In hindsight it would've been better to add a method like SetInsecureUnlockAllowed on the Manager.

Can we just make it an any ?

I guess not, for the same reason. Ugh

This is one further step towards removing account management from `geth`. This PR deprecates the flag `unlock`, and makes the flag moot: unlock via geth is no longer possible.

hasti0073 · 2024-12-04T06:24:23Z

0x383De340d7fe28975dD2C230ef74723fb4D677F0

holiman added 6 commits November 6, 2024 15:13

cmd/geth: remove unlock commandline flag

f7088ba

cmd/geth: remove account indexing by number

e1a7cc7

cmd/geth: remove unused code

3eadab6

cmd/geth, cmd/utils: remove some code related to account mgmt

c503ac8

cmd/geth: remove unused + docs

8e1a7d9

cmd/geth, cmd/utils: refactor to reduce common code

9bb772b

MariusVanDerWijden reviewed Nov 7, 2024

View reviewed changes

MariusVanDerWijden approved these changes Nov 7, 2024

View reviewed changes

cmd/geth: remove flag and config related to insecure unlock

5a8c4ff

holiman requested review from fjl and lightclient as code owners November 7, 2024 12:50

fjl reviewed Nov 7, 2024

View reviewed changes

accounts: undo API-change to account manager

afa2247

rjl493456442 approved these changes Nov 10, 2024

View reviewed changes

accounts: no-op change to reduce diff

bc201f7

holiman added this to the 1.14.12 milestone Nov 15, 2024

holiman merged commit a5f0001 into ethereum:master Nov 15, 2024
3 checks passed

holiman deleted the no_unlock branch November 15, 2024 09:15

BrewTestBot mentioned this pull request Nov 19, 2024

ethereum 1.14.12 Homebrew/homebrew-core#198233

Merged

johnpillai80 approved these changes Nov 27, 2024

View reviewed changes

This was referenced Dec 6, 2024

upstream: merge geth v1.14.12 bnb-chain/bsc#2786

Closed

upstream: merge geth v1.14.12 bnb-chain/bsc#2791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/geth: remove unlock commandline flag #30737

cmd/geth: remove unlock commandline flag #30737

holiman commented Nov 6, 2024 •

edited

Loading

MariusVanDerWijden Nov 7, 2024

MariusVanDerWijden Nov 7, 2024

namiloh Nov 7, 2024

holiman Nov 7, 2024

MariusVanDerWijden left a comment

fjl Nov 7, 2024 •

edited

Loading

fjl Nov 7, 2024

holiman Nov 7, 2024

holiman Nov 7, 2024

hasti0073 commented Dec 4, 2024

cmd/geth: remove unlock commandline flag #30737

cmd/geth: remove unlock commandline flag #30737

Conversation

holiman commented Nov 6, 2024 • edited Loading

MariusVanDerWijden Nov 7, 2024

Choose a reason for hiding this comment

MariusVanDerWijden Nov 7, 2024

Choose a reason for hiding this comment

namiloh Nov 7, 2024

Choose a reason for hiding this comment

holiman Nov 7, 2024

Choose a reason for hiding this comment

MariusVanDerWijden left a comment

Choose a reason for hiding this comment

fjl Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

fjl Nov 7, 2024

Choose a reason for hiding this comment

holiman Nov 7, 2024

Choose a reason for hiding this comment

holiman Nov 7, 2024

Choose a reason for hiding this comment

hasti0073 commented Dec 4, 2024

holiman commented Nov 6, 2024 •

edited

Loading

fjl Nov 7, 2024 •

edited

Loading