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
17 changes: 1 addition & 16 deletions accounts/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,6 @@ import (
// 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
// is removed in favor of Clef.
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 {
Expand All @@ -47,7 +39,6 @@ type newBackendEvent struct {
// Manager is an overarching account manager that can communicate with various
// backends for signing transactions.
type Manager struct {
config *Config // Global account manager configurations
backends map[reflect.Type][]Backend // Index of backends currently registered
updaters []event.Subscription // Wallet update subscriptions for all backends
updates chan WalletEvent // Subscription sink for backend wallet changes
Expand All @@ -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

// Retrieve the initial list of wallets from the backends and sort by URL
var wallets []Wallet
for _, backend := range backends {
Expand All @@ -78,7 +69,6 @@ func NewManager(config *Config, backends ...Backend) *Manager {
}
// Assemble the account manager and return
am := &Manager{
config: config,
backends: make(map[reflect.Type][]Backend),
updaters: subs,
updates: updates,
Expand Down Expand Up @@ -106,11 +96,6 @@ func (am *Manager) Close() error {
return <-errc
}

// Config returns the configuration of account manager.
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) {
Expand Down
2 changes: 1 addition & 1 deletion cmd/geth/accountcmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ nodes.
// makeAccountManager creates an account manager with backends
func makeAccountManager(ctx *cli.Context) *accounts.Manager {
cfg := loadBaseConfig(ctx)
am := accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: cfg.Node.InsecureUnlockAllowed})
am := accounts.NewManager()
keydir, isEphemeral, err := cfg.Node.GetKeyStoreDir()
if err != nil {
utils.Fatalf("Failed to get the keystore directory: %v", err)
Expand Down
7 changes: 1 addition & 6 deletions cmd/utils/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -510,11 +510,6 @@ var (
Value: "",
Category: flags.AccountCategory,
}
InsecureUnlockAllowedFlag = &cli.BoolFlag{
Name: "allow-insecure-unlock",
Usage: "Allow insecure account unlocking when account-related RPCs are exposed by http",
Category: flags.AccountCategory,
}
// EVM settings
VMEnableDebugFlag = &cli.BoolFlag{
Name: "vmdebug",
Expand Down Expand Up @@ -1361,7 +1356,7 @@ func SetNodeConfig(ctx *cli.Context, cfg *node.Config) {
cfg.USB = ctx.Bool(USBFlag.Name)
}
if ctx.IsSet(InsecureUnlockAllowedFlag.Name) {
cfg.InsecureUnlockAllowed = ctx.Bool(InsecureUnlockAllowedFlag.Name)
log.Warn(fmt.Sprintf("Option %q is deprecated and has no effect", InsecureUnlockAllowedFlag.Name))
}
if ctx.IsSet(DBEngineFlag.Name) {
dbEngine := ctx.String(DBEngineFlag.Name)
Expand Down
5 changes: 5 additions & 0 deletions cmd/utils/flags_legacy.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ var (
Value: "",
Category: flags.DeprecatedCategory,
}
InsecureUnlockAllowedFlag = &cli.BoolFlag{
Name: "allow-insecure-unlock",
Usage: "Allow insecure account unlocking when account-related RPCs are exposed by http (deprecated)",
Category: flags.DeprecatedCategory,
}
)

// showDeprecated displays deprecated flags that will be soon removed from the codebase.
Expand Down
2 changes: 1 addition & 1 deletion internal/ethapi/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ func allBlobTxs(addr common.Address, config *params.ChainConfig) []txData {
func newTestAccountManager(t *testing.T) (*accounts.Manager, accounts.Account) {
var (
dir = t.TempDir()
am = accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: true})
am = accounts.NewManager()
b = keystore.NewKeyStore(dir, 2, 1)
testKey, _ = crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
)
Expand Down
2 changes: 1 addition & 1 deletion node/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ type Config struct {
// scrypt KDF at the expense of security.
UseLightweightKDF bool `toml:",omitempty"`

// InsecureUnlockAllowed allows user to unlock accounts in unsafe http environment.
// InsecureUnlockAllowed is a deprecated option to allow users to accounts in unsafe http environment.
InsecureUnlockAllowed bool `toml:",omitempty"`

// NoUSB disables hardware wallet monitoring and connectivity.
Expand Down
2 changes: 1 addition & 1 deletion node/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func New(conf *Config) (*Node, error) {
node.keyDirTemp = isEphem
// Creates an empty AccountManager with no backends. Callers (e.g. cmd/geth)
// are required to add the backends later on.
node.accman = accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed})
node.accman = accounts.NewManager()

// Initialize the p2p server. This creates the node key and discovery databases.
node.server.Config.PrivateKey = node.config.NodeKey()
Expand Down
2 changes: 1 addition & 1 deletion signer/core/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ func StartClefAccountManager(ksLocation string, nousb, lightKDF bool, scpath str
}

// Clef doesn't allow insecure http account unlock.
return accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: false}, backends...)
return accounts.NewManager(backends...)
}

// MetadataFromContext extracts Metadata from a given context.Context
Expand Down