Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/geth: remove unlock commandline flag #30737

Merged
merged 9 commits into from
Nov 15, 2024
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 6, 2024

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

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

?

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 {
Copy link
Contributor

@fjl fjl Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make it an any ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not, for the same reason. Ugh

@holiman holiman added this to the 1.14.12 milestone Nov 15, 2024
@holiman holiman merged commit a5f0001 into ethereum:master Nov 15, 2024
3 checks passed
@holiman holiman deleted the no_unlock branch November 15, 2024 09:15
holiman added a commit that referenced this pull request Nov 19, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants