[ethereum/go-ethereum] cmd, eth, miner: make recommit configurable (#17444)

karalabe requested changes on this pull request.

Generally looks good, still need to think through the formula. Please fix the few issues I mentioned above.

> @@ -360,6 +360,11 @@ var (
Name: «extradata»,
Usage: «Block extra data set by the miner (default = client version, deprecated, use —miner.extradata)»,
}
+ MinerRecommitIntervalFlag = cli.IntFlag{
+ Name: «miner.recommit»,
+ Usage: «Sealing work recommit interval(ms), will be dynamically adjusted by the system if it is too short»,

Lets shorted this description a bit so it doesn’t wrap on smaller width terminals. Eg.

`Time interval (ms) to recreate the block being mined.`

I don’t think there’s a need to mention the automatic adjustments. That’s a protective measure, the user can’t change it anyway, so lets keep the help cleaner/clearer.

> @@ -360,6 +360,11 @@ var (
Name: «extradata»,
Usage: «Block extra data set by the miner (default = client version, deprecated, use —miner.extradata)»,
}
+ MinerRecommitIntervalFlag = cli.IntFlag{
+ Name: «miner.recommit»,
+ Usage: «Sealing work recommit interval(ms), will be dynamically adjusted by the system if it is too short»,

Also, I think we should rather use a `cli.DurationFlag` instead. Those support nicer formats, like `—miner.recommit=1.5s`

> @@ -95,11 +95,12 @@ type Config struct {
TrieTimeout time.Duration

// Mining-related options
— Etherbase common.Address `toml:»,omitempty»`
— MinerThreads int `toml:»,omitempty»`
— MinerNotify []string `toml:»,omitempty»`
— ExtraData []byte `toml:»,omitempty»`
— GasPrice *big.Int
+ Etherbase common.Address `toml:»,omitempty»`
+ MinerThreads int `toml:»,omitempty»`
+ MinerNotify []string `toml:»,omitempty»`
+ ExtraData []byte `toml:»,omitempty»`
+ GasPrice *big.Int
+ RecommitInterval time.Duration

Please call this `MinerRecommit`. Bonus points if you rename `ExtraData` and `GasPRice` too to have the `Miner` prefix. Long term we could separate these out into an own config struct.

> if dec.ExtraData != nil {
c.ExtraData = *dec.ExtraData
}
if dec.GasPrice != nil {
c.GasPrice = dec.GasPrice
}
+ if dec.RecommitInterval != nil {
+ c.RecommitInterval = *dec.RecommitInterval
+ }

Oh, nice. Good catch regenerating the config toml!

> @@ -51,13 +52,13 @@ type Miner struct {
shouldStart int32 // should start indicates whether we should start after sync
}

-func New(eth Backend, config *params.ChainConfig, mux *event.TypeMux, engine consensus.Engine) *Miner {
+func New(eth Backend, config *params.ChainConfig, mux *event.TypeMux, engine consensus.Engine, recommitInterval time.Duration) *Miner {

I think `recommit` is enough. Shorter, cleaner.

> @@ -144,6 +145,10 @@ func (self *Miner) SetExtra(extra []byte) error {
return nil
}

+func (self *Miner) SetRecommitInterval(interval time.Duration) {

Please add a doc.

> }

-func newWorker(config *params.ChainConfig, engine consensus.Engine, eth Backend, mux *event.TypeMux) *worker {
+func newWorker(config *params.ChainConfig, engine consensus.Engine, eth Backend, mux *event.TypeMux, recommitInterval time.Duration) *worker {

`recommitInterval` -> `recommit`

> }
// Subscribe NewTxsEvent for tx pool
worker.txsSub = eth.TxPool().SubscribeNewTxsEvent(worker.txsCh)
// Subscribe events for blockchain
worker.chainHeadSub = eth.BlockChain().SubscribeChainHeadEvent(worker.chainHeadCh)
worker.chainSideSub = eth.BlockChain().SubscribeChainSideEvent(worker.chainSideCh)

+ // Recap recommit interval if the user-specified one is too short.
+ if recommitInterval < minRecommitInterval { + log.Info("Recap miner recommit interval", "from", recommitInterval, "to", minRecommitInterval) Lets call this `Sanitizing` instead of `Recap`, it's clearer and also what we use in txpool. Instead of `from`/`to` please use `provided` and `updated`. Also please raise the log level from INFO to WARN. It's definitely an error, even though we can handle it. > @@ -269,6 +335,35 @@ func (w *worker) newWorkLoop() {
recommit(true, commitInterruptResubmit)
}

+ case interval := <-w.resubmitIntervalCh: + // Adjust resubmit interval explicitly by user. + if interval < minRecommitInterval { + log.Info("Recap miner recommit interval", "from", interval, "to", minRecommitInterval) Lets call this Sanitizing instead of Recap, it's clearer and also what we use in txpool. Instead of from/to please use provided and updated. Also please raise the log level from INFO to WARN. It's definitely an error, even though we can handle it. > @@ -251,7 +292,32 @@ func (w *worker) newWorkLoop() {
}
interrupt = new(int32)
w.newWorkCh <- &newWorkReq{interrupt: interrupt, noempty: noempty} - timer.Reset(blockRecommitInterval) + timer.Reset(recommitInterval) + } + // recalcuInterval recalculates the resubmitting interval upon feedback. `recalcuInterval` -> `recalcRecommit`

> @@ -251,7 +292,32 @@ func (w *worker) newWorkLoop() {
}
interrupt = new(int32)
w.newWorkCh <- &newWorkReq{interrupt: interrupt, noempty: noempty} - timer.Reset(blockRecommitInterval) + timer.Reset(recommitInterval) + } + // recalcuInterval recalculates the resubmitting interval upon feedback. + recalcuInterval := func(target float64, inc bool) { + var ( + old = float64(recommitInterval.Nanoseconds()) + new float64 Please avoid using `new`. It's a Go keyword and makes the code a lot harder to follow. Perhaps instead of `old` and `new`, let's use `prev` and `next`. > @@ -238,8 +276,11 @@ func (w *worker) close() {
}

// newWorkLoop is a standalone goroutine to submit new mining work upon received events.
-func (w *worker) newWorkLoop() {
— var interrupt *int32
+func (w *worker) newWorkLoop(recommitInterval time.Duration) {
+ var (
+ interrupt *int32
+ minInterval = recommitInterval // minimal resubmit interval specified by user.

`minInterval` doesn’t really say what it’s used for, lets rename both to:

`recommitInterval` -> `recommit`
`minInterval` -> `minRecommit`