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

BUG:No etherbase configured #26407

Closed
wants to merge 1 commit into from
Closed

BUG:No etherbase configured #26407

wants to merge 1 commit into from

Conversation

lochjin
Copy link
Contributor

@lochjin lochjin commented Jan 3, 2023

Errors will be reported when external signer are used:
./geth --signer=clef.ipc --miner.etherbase=0x... ...

@rjl493456442
Copy link
Member

@holiman I think the root cause it once the externalSigner is enabled, all local signers are disabled

// For now, we're using EITHER external signer OR local signers.
// If/when we implement some form of lockfile for USB and keystore wallets,
// we can have both, but it's very confusing for the user to see the same
// accounts in both externally and locally, plus very racey.

However, we use the keystore as the fallback in lots of places, e.g. in this PR, the default etherbase
is the first account in keystore. But apparently this assumption is not held anymore https://github.com/ethereum/go-ethereum/blob/master/cmd/utils/flags.go#L1760

I think we should either:

  • support resolving the first account from the external signer
  • remove the support for resolving the "first account" as the etherbase

What's your opinion?

@karalabe
Copy link
Member

karalabe commented Jan 3, 2023

My vote is that we should start cleaning up notions of account[0] everywhere because it's a magical cosntruct that can be unexpected. Since signing requires a lot of infra nowadays, having to explicitly set etherbase seems like one more step out of many, so not much more effort but a lot more clarity.

@rjl493456442
Copy link
Member

@lochjin Can you please try out this alternative fix? #26413

@lochjin
Copy link
Contributor Author

lochjin commented Jan 4, 2023

@lochjin Can you please try out this alternative fix? #26413
Okay, I'll try

@lochjin
Copy link
Contributor Author

lochjin commented Jan 5, 2023

@lochjin Can you please try out this alternative fix? #26413

@rjl493456442 After testing, it is confirmed that #26413 can work very correctly

@lochjin lochjin closed this Jan 5, 2023
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.

4 participants