From 2b44ef5f93cc7479a77890917a29684b56e9167a Mon Sep 17 00:00:00 2001 From: rjl493456442 Date: Sat, 21 Jan 2023 00:26:01 +0800 Subject: [PATCH] miner, cmd, eth: require explicit etherbase address (#26413) This change introduces a breaking change to miner.etherbase is configured. Previously, users did not need to explicitly set the etherbase address via flag, since 'first' local account was used as etherbase automatically. This change removes the "default first account" feature. In Proof-of-stake world, the fee recipient address is provided by CL, and not configured in Geth any more - meaning that miner.etherbase is mostly for legacy networks(pow, clique networks etc). --- cmd/geth/les_test.go | 2 +- cmd/utils/flags.go | 41 +++++++++++++++++------------------------ eth/backend.go | 14 +------------- miner/miner.go | 31 +++++++++++++------------------ miner/miner_test.go | 33 ++++++++++++++++++--------------- miner/worker.go | 17 +++++++++++++---- 6 files changed, 63 insertions(+), 75 deletions(-) diff --git a/cmd/geth/les_test.go b/cmd/geth/les_test.go index d06c3b91d8..607b454ead 100644 --- a/cmd/geth/les_test.go +++ b/cmd/geth/les_test.go @@ -146,7 +146,7 @@ func startLightServer(t *testing.T) *gethrpc { t.Logf("Importing keys to geth") runGeth(t, "account", "import", "--datadir", datadir, "--password", "./testdata/password.txt", "--lightkdf", "./testdata/key.prv").WaitExit() account := "0x02f0d131f1f97aef08aec6e3291b957d9efe7105" - server := startGethWithIpc(t, "lightserver", "--allow-insecure-unlock", "--datadir", datadir, "--password", "./testdata/password.txt", "--unlock", account, "--mine", "--light.serve=100", "--light.maxpeers=1", "--nodiscover", "--nat=extip:127.0.0.1", "--verbosity=4") + server := startGethWithIpc(t, "lightserver", "--allow-insecure-unlock", "--datadir", datadir, "--password", "./testdata/password.txt", "--unlock", account, "--miner.etherbase=0x02f0d131f1f97aef08aec6e3291b957d9efe7105", "--mine", "--light.serve=100", "--light.maxpeers=1", "--nodiscover", "--nat=extip:127.0.0.1", "--verbosity=4") return server } diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 4c66800d5d..4bee6093ea 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -538,8 +538,7 @@ var ( } MinerEtherbaseFlag = &cli.StringFlag{ Name: "miner.etherbase", - Usage: "Public address for block mining rewards (default = first account)", - Value: "0", + Usage: "0x prefixed public address for block mining rewards", Category: flags.MinerCategory, } MinerExtraDataFlag = &cli.StringFlag{ @@ -1343,25 +1342,15 @@ func MakeAddress(ks *keystore.KeyStore, account string) (accounts.Account, error return accs[index], nil } -// setEtherbase retrieves the etherbase either from the directly specified -// command line flags or from the keystore if CLI indexed. -func setEtherbase(ctx *cli.Context, ks *keystore.KeyStore, cfg *ethconfig.Config) { - // Extract the current etherbase - var etherbase string +// setEtherbase retrieves the etherbase from the directly specified command line flags. +func setEtherbase(ctx *cli.Context, cfg *ethconfig.Config) { if ctx.IsSet(MinerEtherbaseFlag.Name) { - etherbase = ctx.String(MinerEtherbaseFlag.Name) - } - // Convert the etherbase into an address and configure it - if etherbase != "" { - if ks != nil { - account, err := MakeAddress(ks, etherbase) - if err != nil { - Fatalf("Invalid miner etherbase: %v", err) - } - cfg.Miner.Etherbase = account.Address - } else { - Fatalf("No etherbase configured") + b, err := hexutil.Decode(ctx.String(MinerEtherbaseFlag.Name)) + if err != nil || len(b) != common.AddressLength { + log.Info("Failed to decode etherbase", "err", err) + return } + cfg.Miner.Etherbase = common.BytesToAddress(b) } } @@ -1739,11 +1728,7 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) { if ctx.IsSet(LightServeFlag.Name) && ctx.Uint64(TxLookupLimitFlag.Name) != 0 { log.Warn("LES server cannot serve old transaction status and cannot connect below les/4 protocol version if transaction lookup index is limited") } - var ks *keystore.KeyStore - if keystores := stack.AccountManager().Backends(keystore.KeyStoreType); len(keystores) > 0 { - ks = keystores[0].(*keystore.KeyStore) - } - setEtherbase(ctx, ks, cfg) + setEtherbase(ctx, cfg) setGPO(ctx, &cfg.GPO, ctx.String(SyncModeFlag.Name) == "light") setTxPool(ctx, &cfg.TxPool) setEthash(ctx, cfg) @@ -1921,6 +1906,14 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) { // when we're definitely concerned with only one account. passphrase = list[0] } + // Unlock the developer account by local keystore. + var ks *keystore.KeyStore + if keystores := stack.AccountManager().Backends(keystore.KeyStoreType); len(keystores) > 0 { + ks = keystores[0].(*keystore.KeyStore) + } + if ks == nil { + Fatalf("Keystore is not available") + } // setEtherbase has been called above, configuring the miner address from command line flags. if cfg.Miner.Etherbase != (common.Address{}) { developer = accounts.Account{Address: cfg.Miner.Etherbase} diff --git a/eth/backend.go b/eth/backend.go index 6b1c044689..6368c0e03c 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -329,18 +329,6 @@ func (s *Ethereum) Etherbase() (eb common.Address, err error) { if etherbase != (common.Address{}) { return etherbase, nil } - if wallets := s.AccountManager().Wallets(); len(wallets) > 0 { - if accounts := wallets[0].Accounts(); len(accounts) > 0 { - etherbase := accounts[0].Address - - s.lock.Lock() - s.etherbase = etherbase - s.lock.Unlock() - - log.Info("Etherbase automatically configured", "address", etherbase) - return etherbase, nil - } - } return common.Address{}, fmt.Errorf("etherbase must be explicitly specified") } @@ -456,7 +444,7 @@ func (s *Ethereum) StartMining(threads int) error { // introduced to speed sync times. atomic.StoreUint32(&s.handler.acceptTxs, 1) - go s.miner.Start(eb) + go s.miner.Start() } return nil } diff --git a/miner/miner.go b/miner/miner.go index 5102cb523c..c969aec735 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -45,7 +45,7 @@ type Backend interface { // Config is the configuration parameters of mining. type Config struct { - Etherbase common.Address `toml:",omitempty"` // Public address for block mining rewards (default = first account) + Etherbase common.Address `toml:",omitempty"` // Public address for block mining rewards Notify []string `toml:",omitempty"` // HTTP URL list to be notified of new work packages (only useful in ethash). NotifyFull bool `toml:",omitempty"` // Notify with pending block headers instead of work packages ExtraData hexutil.Bytes `toml:",omitempty"` // Block extra data set by the miner @@ -73,25 +73,24 @@ var DefaultConfig = Config{ // Miner creates blocks and searches for proof-of-work values. type Miner struct { - mux *event.TypeMux - worker *worker - coinbase common.Address - eth Backend - engine consensus.Engine - exitCh chan struct{} - startCh chan common.Address - stopCh chan struct{} + mux *event.TypeMux + eth Backend + engine consensus.Engine + exitCh chan struct{} + startCh chan struct{} + stopCh chan struct{} + worker *worker wg sync.WaitGroup } func New(eth Backend, config *Config, chainConfig *params.ChainConfig, mux *event.TypeMux, engine consensus.Engine, isLocalBlock func(header *types.Header) bool) *Miner { miner := &Miner{ - eth: eth, mux: mux, + eth: eth, engine: engine, exitCh: make(chan struct{}), - startCh: make(chan common.Address), + startCh: make(chan struct{}), stopCh: make(chan struct{}), worker: newWorker(config, chainConfig, engine, eth, mux, isLocalBlock, true), } @@ -138,20 +137,17 @@ func (miner *Miner) update() { case downloader.FailedEvent: canStart = true if shouldStart { - miner.SetEtherbase(miner.coinbase) miner.worker.start() } case downloader.DoneEvent: canStart = true if shouldStart { - miner.SetEtherbase(miner.coinbase) miner.worker.start() } // Stop reacting to downloader events events.Unsubscribe() } - case addr := <-miner.startCh: - miner.SetEtherbase(addr) + case <-miner.startCh: if canStart { miner.worker.start() } @@ -166,8 +162,8 @@ func (miner *Miner) update() { } } -func (miner *Miner) Start(coinbase common.Address) { - miner.startCh <- coinbase +func (miner *Miner) Start() { + miner.startCh <- struct{}{} } func (miner *Miner) Stop() { @@ -223,7 +219,6 @@ func (miner *Miner) PendingBlockAndReceipts() (*types.Block, types.Receipts) { } func (miner *Miner) SetEtherbase(addr common.Address) { - miner.coinbase = addr miner.worker.setEtherbase(addr) } diff --git a/miner/miner_test.go b/miner/miner_test.go index 7bf091f375..2e7682acd3 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -86,7 +86,8 @@ func (bc *testBlockChain) SubscribeChainHeadEvent(ch chan<- core.ChainHeadEvent) func TestMiner(t *testing.T) { miner, mux, cleanup := createMiner(t) defer cleanup(false) - miner.Start(common.HexToAddress("0x12345")) + + miner.Start() waitForMiningState(t, miner, true) // Start the downloader mux.Post(downloader.StartEvent{}) @@ -114,7 +115,8 @@ func TestMiner(t *testing.T) { func TestMinerDownloaderFirstFails(t *testing.T) { miner, mux, cleanup := createMiner(t) defer cleanup(false) - miner.Start(common.HexToAddress("0x12345")) + + miner.Start() waitForMiningState(t, miner, true) // Start the downloader mux.Post(downloader.StartEvent{}) @@ -146,7 +148,8 @@ func TestMinerDownloaderFirstFails(t *testing.T) { func TestMinerStartStopAfterDownloaderEvents(t *testing.T) { miner, mux, cleanup := createMiner(t) defer cleanup(false) - miner.Start(common.HexToAddress("0x12345")) + + miner.Start() waitForMiningState(t, miner, true) // Start the downloader mux.Post(downloader.StartEvent{}) @@ -159,7 +162,7 @@ func TestMinerStartStopAfterDownloaderEvents(t *testing.T) { miner.Stop() waitForMiningState(t, miner, false) - miner.Start(common.HexToAddress("0x678910")) + miner.Start() waitForMiningState(t, miner, true) miner.Stop() @@ -170,13 +173,13 @@ func TestStartWhileDownload(t *testing.T) { miner, mux, cleanup := createMiner(t) defer cleanup(false) waitForMiningState(t, miner, false) - miner.Start(common.HexToAddress("0x12345")) + miner.Start() waitForMiningState(t, miner, true) // Stop the downloader and wait for the update loop to run mux.Post(downloader.StartEvent{}) waitForMiningState(t, miner, false) // Starting the miner after the downloader should not work - miner.Start(common.HexToAddress("0x12345")) + miner.Start() waitForMiningState(t, miner, false) } @@ -184,7 +187,7 @@ func TestStartStopMiner(t *testing.T) { miner, _, cleanup := createMiner(t) defer cleanup(false) waitForMiningState(t, miner, false) - miner.Start(common.HexToAddress("0x12345")) + miner.Start() waitForMiningState(t, miner, true) miner.Stop() waitForMiningState(t, miner, false) @@ -194,7 +197,7 @@ func TestCloseMiner(t *testing.T) { miner, _, cleanup := createMiner(t) defer cleanup(true) waitForMiningState(t, miner, false) - miner.Start(common.HexToAddress("0x12345")) + miner.Start() waitForMiningState(t, miner, true) // Terminate the miner and wait for the update loop to run miner.Close() @@ -206,21 +209,21 @@ func TestCloseMiner(t *testing.T) { func TestMinerSetEtherbase(t *testing.T) { miner, mux, cleanup := createMiner(t) defer cleanup(false) - // Start with a 'bad' mining address - miner.Start(common.HexToAddress("0xdead")) + miner.Start() waitForMiningState(t, miner, true) // Start the downloader mux.Post(downloader.StartEvent{}) waitForMiningState(t, miner, false) // Now user tries to configure proper mining address - miner.Start(common.HexToAddress("0x1337")) + miner.Start() // Stop the downloader and wait for the update loop to run mux.Post(downloader.DoneEvent{}) - waitForMiningState(t, miner, true) - // The miner should now be using the good address - if got, exp := miner.coinbase, common.HexToAddress("0x1337"); got != exp { - t.Fatalf("Wrong coinbase, got %x expected %x", got, exp) + + coinbase := common.HexToAddress("0xdeedbeef") + miner.SetEtherbase(coinbase) + if addr := miner.worker.etherbase(); addr != coinbase { + t.Fatalf("Unexpected etherbase want %x got %x", coinbase, addr) } } diff --git a/miner/worker.go b/miner/worker.go index 45450237fe..ee4969623d 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -276,12 +276,14 @@ func newWorker(config *Config, chainConfig *params.ChainConfig, engine consensus chainConfig: chainConfig, engine: engine, eth: eth, - mux: mux, chain: eth.BlockChain(), + mux: mux, isLocalBlock: isLocalBlock, localUncles: make(map[common.Hash]*types.Block), remoteUncles: make(map[common.Hash]*types.Block), unconfirmed: newUnconfirmedBlocks(eth.BlockChain(), sealingLogAtDepth), + coinbase: config.Etherbase, + extra: config.ExtraData, pendingTasks: make(map[common.Hash]*task), txsCh: make(chan core.NewTxsEvent, txChanSize), chainHeadCh: make(chan core.ChainHeadEvent, chainHeadChanSize), @@ -290,8 +292,8 @@ func newWorker(config *Config, chainConfig *params.ChainConfig, engine consensus getWorkCh: make(chan *getWorkReq), taskCh: make(chan *task), resultCh: make(chan *types.Block, resultQueueSize), - exitCh: make(chan struct{}), startCh: make(chan struct{}, 1), + exitCh: make(chan struct{}), resubmitIntervalCh: make(chan time.Duration), resubmitAdjustCh: make(chan *intervalAdjust, resubmitAdjustChanSize), } @@ -340,6 +342,13 @@ func (w *worker) setEtherbase(addr common.Address) { w.coinbase = addr } +// etherbase retrieves the configured etherbase address. +func (w *worker) etherbase() common.Address { + w.mu.RLock() + defer w.mu.RUnlock() + return w.coinbase +} + func (w *worker) setGasCeil(ceil uint64) { w.mu.Lock() defer w.mu.Unlock() @@ -1114,11 +1123,11 @@ func (w *worker) commitWork(interrupt *int32, noempty bool, timestamp int64) { // Set the coinbase if the worker is running or it's required var coinbase common.Address if w.isRunning() { - if w.coinbase == (common.Address{}) { + coinbase = w.etherbase() + if coinbase == (common.Address{}) { log.Error("Refusing to mine without etherbase") return } - coinbase = w.coinbase // Use the preset address as the fee recipient } work, err := w.prepareWork(&generateParams{ timestamp: uint64(timestamp),