From ea6d39bba89ffb92b7bacda7e5626f62f1daad28 Mon Sep 17 00:00:00 2001 From: YoshihitoAso Date: Fri, 15 Mar 2024 16:41:26 +0900 Subject: [PATCH 1/5] eth, miner: add timeout for building sealing block --- cmd/geth/config_test.go | 2 ++ cmd/geth/main.go | 1 + cmd/utils/flags.go | 9 ++++++ eth/config_test.go | 22 -------------- eth/ethconfig/config.go | 15 ++++------ miner/config_test.go | 24 ++++++++++++++++ miner/miner.go | 28 ++++++++++++------ miner/worker.go | 63 ++++++++++++++++++++++++++++++++++------- 8 files changed, 112 insertions(+), 52 deletions(-) delete mode 100644 eth/config_test.go create mode 100644 miner/config_test.go diff --git a/cmd/geth/config_test.go b/cmd/geth/config_test.go index 77f565647..fff4ce56e 100644 --- a/cmd/geth/config_test.go +++ b/cmd/geth/config_test.go @@ -324,6 +324,7 @@ func TestFlagsConfig(t *testing.T) { assert.Equal(t, big.NewInt(1000000000), miner.GasPrice) assert.Equal(t, time.Duration(3000000000), miner.Recommit) assert.Equal(t, false, miner.Noverify) + assert.Equal(t, time.Duration(800000000), miner.NewPayloadTimeout) assert.Equal(t, uint64(0), miner.AllowedFutureBlockTime) // [Eth.GPO] @@ -487,6 +488,7 @@ GasCeil = 800000000 GasPrice = 0 Recommit = 3000000000 Noverify = false +NewPayloadTimeout = 800000000 AllowedFutureBlockTime = 0 [Eth.GPO] diff --git a/cmd/geth/main.go b/cmd/geth/main.go index 003124a5a..572b25303 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -128,6 +128,7 @@ var ( utils.MinerExtraDataFlag, utils.MinerRecommitIntervalFlag, utils.MinerNoVerfiyFlag, + utils.MinerNewPayloadTimeout, utils.NATFlag, utils.NoDiscoverFlag, utils.DiscoveryV5Flag, diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 7d1cc4351..fc827232c 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -487,6 +487,12 @@ var ( Name: "miner.noverify", Usage: "Disable remote sealing verification", } + MinerNewPayloadTimeout = &cli.DurationFlag{ + Name: "miner.newpayload-timeout", + Usage: "Specify the maximum time allowance for creating a new payload", + Value: ethconfig.Defaults.Miner.NewPayloadTimeout, + } + // Account settings UnlockedAccountFlag = cli.StringFlag{ Name: "unlock", @@ -1792,6 +1798,9 @@ func setMiner(ctx *cli.Context, cfg *miner.Config) { if ctx.GlobalIsSet(MinerNoVerfiyFlag.Name) { cfg.Noverify = ctx.GlobalBool(MinerNoVerfiyFlag.Name) } + if ctx.GlobalIsSet(MinerNewPayloadTimeout.Name) { + cfg.NewPayloadTimeout = ctx.Duration(MinerNewPayloadTimeout.Name) + } if ctx.GlobalIsSet(AllowedFutureBlockTimeFlag.Name) { cfg.AllowedFutureBlockTime = ctx.GlobalUint64(AllowedFutureBlockTimeFlag.Name) //Quorum } diff --git a/eth/config_test.go b/eth/config_test.go deleted file mode 100644 index 46bb4dde1..000000000 --- a/eth/config_test.go +++ /dev/null @@ -1,22 +0,0 @@ -package eth - -import ( - "testing" - - "github.com/ethereum/go-ethereum/eth/ethconfig" - "github.com/stretchr/testify/assert" -) - -func TestQuorumDefautConfig(t *testing.T) { - type data struct { - actual uint64 - expected uint64 - } - var testData = map[string]data{ - "eth.DefaultConfig.Miner.GasFloor": {ethconfig.Defaults.Miner.GasFloor, 700000000}, - "eth.DefaultConfig.Miner.GasCeil": {ethconfig.Defaults.Miner.GasCeil, 800000000}, - } - for k, v := range testData { - assert.Equal(t, v.expected, v.actual, k+" value mismatch") - } -} diff --git a/eth/ethconfig/config.go b/eth/ethconfig/config.go index d78d05308..9f14ccd8d 100644 --- a/eth/ethconfig/config.go +++ b/eth/ethconfig/config.go @@ -81,16 +81,11 @@ var Defaults = Config{ TrieDirtyCache: 256, TrieTimeout: 60 * time.Minute, SnapshotCache: 102, - Miner: miner.Config{ - GasFloor: params.DefaultMinGasLimit, - GasCeil: params.GenesisGasLimit, - GasPrice: big.NewInt(params.GWei), - Recommit: 3 * time.Second, - }, - TxPool: core.DefaultTxPoolConfig, - RPCGasCap: 25000000, - GPO: FullNodeGPO, - RPCTxFeeCap: 1, // 1 ether + Miner: miner.DefaultConfig, + TxPool: core.DefaultTxPoolConfig, + RPCGasCap: 25000000, + GPO: FullNodeGPO, + RPCTxFeeCap: 1, // 1 ether // Quorum Istanbul: *istanbul.DefaultConfig, // Quorum diff --git a/miner/config_test.go b/miner/config_test.go new file mode 100644 index 000000000..5c6e6c9c9 --- /dev/null +++ b/miner/config_test.go @@ -0,0 +1,24 @@ +package miner + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestQuorumDefautConfig(t *testing.T) { + type int_data struct { + actual uint64 + expected uint64 + } + var testData = map[string]int_data{ + "DefaultConfig.GasFloor": {DefaultConfig.GasFloor, 700000000}, + "DefaultConfig.GasCeil": {DefaultConfig.GasCeil, 800000000}, + "DefaultConfig.Recommit": {uint64(DefaultConfig.Recommit), uint64(3 * time.Second)}, + "DefaultConfig.NewPayloadTimeout": {uint64(DefaultConfig.NewPayloadTimeout), uint64(800 * time.Millisecond)}, + } + for k, v := range testData { + assert.Equal(t, v.expected, v.actual, k+" value mismatch") + } +} diff --git a/miner/miner.go b/miner/miner.go index fca4b26eb..371400c9b 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -44,20 +44,30 @@ 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) - 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 - GasFloor uint64 // Target gas floor for mined blocks. - GasCeil uint64 // Target gas ceiling for mined blocks. - GasPrice *big.Int // Minimum gas price for mining a transaction - Recommit time.Duration // The time interval for miner to re-create mining work. - Noverify bool // Disable remote mining solution verification(only useful in ethash). + Etherbase common.Address `toml:",omitempty"` // Public address for block mining rewards (default = first account) + 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 + GasFloor uint64 // Target gas floor for mined blocks. + GasCeil uint64 // Target gas ceiling for mined blocks. + GasPrice *big.Int // Minimum gas price for mining a transaction + Recommit time.Duration // The time interval for miner to re-create mining work. + Noverify bool // Disable remote mining solution verification(only useful in ethash). + NewPayloadTimeout time.Duration // The maximum time allowance for creating a new payload // Quorum AllowedFutureBlockTime uint64 // Max time (in seconds) from current time allowed for blocks, before they're considered future blocks } +// DefaultConfig contains default settings for miner. +var DefaultConfig = Config{ + GasFloor: params.DefaultMinGasLimit, + GasCeil: params.GenesisGasLimit, + GasPrice: big.NewInt(params.GWei), + Recommit: 3 * time.Second, // for ibet + NewPayloadTimeout: 800 * time.Millisecond, // for ibet +} + // Miner creates blocks and searches for proof-of-work values. type Miner struct { mux *event.TypeMux diff --git a/miner/worker.go b/miner/worker.go index 9069b5183..4af5b6338 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -117,6 +117,7 @@ const ( commitInterruptNone int32 = iota commitInterruptNewHead commitInterruptResubmit + commitInterruptTimeout ) // newWorkReq represents a request for new sealing work submitting with relative interrupt notifier. @@ -189,6 +190,13 @@ type worker struct { // non-stop and no real transaction will be included. noempty uint32 + // newpayloadTimeout is the maximum timeout allowance for creating payload. + // The default value is 2 seconds but node operator can set it to arbitrary + // large value. A large timeout allowance may cause Geth to fail creating + // a non-empty payload within the specified time and eventually miss the slot + // in case there are some computation expensive transactions in txpool. + newpayloadTimeout time.Duration + // External functions isLocalBlock func(block *types.Block) bool // Function used to determine whether the specified block is mined by local miner. @@ -235,6 +243,16 @@ func newWorker(config *Config, chainConfig *params.ChainConfig, engine consensus log.Warn("Sanitizing miner recommit interval", "provided", recommit, "updated", minRecommitInterval) recommit = minRecommitInterval } + // Sanitize the timeout config for creating payload. + newpayloadTimeout := worker.config.NewPayloadTimeout + if newpayloadTimeout == 0 { + log.Warn("Sanitizing new payload timeout to default", "provided", newpayloadTimeout, "updated", DefaultConfig.NewPayloadTimeout) + newpayloadTimeout = DefaultConfig.NewPayloadTimeout + } + if newpayloadTimeout < time.Millisecond*100 { + log.Warn("Low payload timeout may cause high amount of non-full blocks", "provided", newpayloadTimeout, "default", DefaultConfig.NewPayloadTimeout) + } + worker.newpayloadTimeout = newpayloadTimeout go worker.mainLoop() go worker.newWorkLoop(recommit) @@ -882,18 +900,23 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin var coalescedLogs []*types.Log - loopStartTime := time.Now() // Quorum for { // In the following three cases, we will interrupt the execution of the transaction. // (1) new head block event arrival, the interrupt signal is 1 // (2) worker start or restart, the interrupt signal is 1 // (3) worker recreate the mining block with any newly arrived transactions, the interrupt signal is 2. - // For the first two cases, the semi-finished work will be discarded. - // For the third case, the semi-finished work will be submitted to the consensus engine. + // (4) new payload timeout, the interrupt signal is 3. + // signal-1 -> the semi-finished work will be discarded. + // signal-2, 3 -> the semi-finished work will be submitted to the consensus engine. if interrupt != nil && atomic.LoadInt32(interrupt) != commitInterruptNone { - log.Info("Aborting transaction processing due to 'commitInterruptNewHead',", "elapsed time", time.Since(loopStartTime)) // Quorum - // Notify resubmit loop to increase resubmitting interval due to too frequent commits. - if atomic.LoadInt32(interrupt) == commitInterruptResubmit { + switch { + // Payload timeout + case atomic.LoadInt32(interrupt) == commitInterruptTimeout: + return false + + // Notify resubmit loop to increase resubmitting interval if the + // interruption is due to frequent commits. + case atomic.LoadInt32(interrupt) == commitInterruptResubmit: ratio := float64(w.current.header.GasLimit-w.current.gasPool.Gas()) / float64(w.current.header.GasLimit) if ratio < 0.1 { ratio = 0.1 @@ -902,24 +925,35 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin ratio: ratio, inc: true, } + return false + + // If the block building is interrupted by newhead event, discard it + // totally. Committing the interrupted block introduces unnecessary + // delay, and possibly causes miner to mine on the previous head, + // which could result in higher uncle rate. + case atomic.LoadInt32(interrupt) == commitInterruptNewHead: + return true } - return atomic.LoadInt32(interrupt) == commitInterruptNewHead } - // If we don't have enough gas for any further transactions then we're done + + // If we don't have enough gas for any further transactions then we're done. if w.current.gasPool.Gas() < params.TxGas { log.Trace("Not enough gas for further transactions", "have", w.current.gasPool, "want", params.TxGas) break } - // Retrieve the next transaction and abort if all done + + // Retrieve the next transaction and abort if all done. tx := txs.Peek() if tx == nil { break } + // Error may be ignored here. The error has already been checked // during transaction acceptance is the transaction pool. // // We use the eip155 signer regardless of the current hf. from, _ := types.Sender(w.current.signer, tx) + // Check whether the tx is replay protected. If we're not in the EIP155 hf // phase, start ignoring the sender until we do. if tx.Protected() && !w.chainConfig.IsEIP155(w.current.header.Number) && !tx.IsPrivate() { @@ -928,6 +962,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin txs.Pop() continue } + // Start executing the transaction logs, err := w.commitTransaction(tx, coinbase) switch { @@ -1000,10 +1035,10 @@ func (w *worker) commitNewWork(interrupt *int32, noempty bool, timestamp int64) timestamp = int64(parent.Time() + 1) } minGasLimit := w.chainConfig.GetMinerMinGasLimit(parent.Number(), params.DefaultMinGasLimit) - num := parent.Number() + // Construct the sealing block header. header := &types.Header{ ParentHash: parent.Hash(), - Number: num.Add(num, common.Big1), + Number: new(big.Int).Add(parent.Number(), common.Big1), GasLimit: core.CalcGasLimit(parent, minGasLimit, w.config.GasFloor, w.config.GasCeil), Extra: w.extra, Time: uint64(timestamp), @@ -1096,6 +1131,12 @@ func (w *worker) commitNewWork(interrupt *int32, noempty bool, timestamp int64) localTxs[account] = txs } } + + timer := time.AfterFunc(w.newpayloadTimeout, func() { + atomic.StoreInt32(interrupt, commitInterruptTimeout) + }) + defer timer.Stop() + if len(localTxs) > 0 { txs := types.NewTransactionsByPriceAndNonce(w.current.signer, localTxs) if w.commitTransactions(txs, w.coinbase, interrupt) { From db8967f7cb7491b673d627d2901912146f66e8b3 Mon Sep 17 00:00:00 2001 From: YoshihitoAso Date: Fri, 15 Mar 2024 18:03:07 +0900 Subject: [PATCH 2/5] fix --- miner/miner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/miner.go b/miner/miner.go index 371400c9b..ba5eafa11 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -64,7 +64,7 @@ var DefaultConfig = Config{ GasFloor: params.DefaultMinGasLimit, GasCeil: params.GenesisGasLimit, GasPrice: big.NewInt(params.GWei), - Recommit: 3 * time.Second, // for ibet + Recommit: 3 * time.Second, NewPayloadTimeout: 800 * time.Millisecond, // for ibet } From dc9640f399108bcc6b04a12271da365d3b79d83a Mon Sep 17 00:00:00 2001 From: YoshihitoAso Date: Fri, 15 Mar 2024 19:09:28 +0900 Subject: [PATCH 3/5] Add info log --- miner/worker.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/miner/worker.go b/miner/worker.go index 4af5b6338..3a5466187 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -912,6 +912,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin switch { // Payload timeout case atomic.LoadInt32(interrupt) == commitInterruptTimeout: + log.Info("Aborting transaction processing", "interrupt", interrupt) return false // Notify resubmit loop to increase resubmitting interval if the @@ -925,6 +926,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin ratio: ratio, inc: true, } + log.Info("Aborting transaction processing", "interrupt", interrupt, "ratio", ratio) return false // If the block building is interrupted by newhead event, discard it @@ -932,6 +934,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin // delay, and possibly causes miner to mine on the previous head, // which could result in higher uncle rate. case atomic.LoadInt32(interrupt) == commitInterruptNewHead: + log.Info("Aborting transaction processing", "interrupt", interrupt) return true } } From 2438afa54fc444e7a38965a630f4fb30e7d64323 Mon Sep 17 00:00:00 2001 From: YoshihitoAso Date: Fri, 15 Mar 2024 19:18:11 +0900 Subject: [PATCH 4/5] fix --- miner/worker.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/miner/worker.go b/miner/worker.go index 3a5466187..2ba3cc663 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -912,7 +912,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin switch { // Payload timeout case atomic.LoadInt32(interrupt) == commitInterruptTimeout: - log.Info("Aborting transaction processing", "interrupt", interrupt) + log.Info("Aborting transaction processing", "signal", commitInterruptTimeout) return false // Notify resubmit loop to increase resubmitting interval if the @@ -926,7 +926,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin ratio: ratio, inc: true, } - log.Info("Aborting transaction processing", "interrupt", interrupt, "ratio", ratio) + log.Info("Aborting transaction processing", "signal", commitInterruptResubmit, "ratio", ratio) return false // If the block building is interrupted by newhead event, discard it @@ -934,7 +934,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin // delay, and possibly causes miner to mine on the previous head, // which could result in higher uncle rate. case atomic.LoadInt32(interrupt) == commitInterruptNewHead: - log.Info("Aborting transaction processing", "interrupt", interrupt) + log.Info("Aborting transaction processing", "signal", commitInterruptNewHead) return true } } From c48edb874609be88344f1893ea312e62c9d669d4 Mon Sep 17 00:00:00 2001 From: YoshihitoAso Date: Fri, 15 Mar 2024 19:43:01 +0900 Subject: [PATCH 5/5] rename test case --- miner/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/config_test.go b/miner/config_test.go index 5c6e6c9c9..541509b63 100644 --- a/miner/config_test.go +++ b/miner/config_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestQuorumDefautConfig(t *testing.T) { +func TestMinerDefautConfig(t *testing.T) { type int_data struct { actual uint64 expected uint64