diff --git a/.changeset/new-nails-arrive.md b/.changeset/new-nails-arrive.md new file mode 100644 index 0000000000000..86c417806d92f --- /dev/null +++ b/.changeset/new-nails-arrive.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/l2geth': patch +--- + +fixes empty block detection and removes empty worker tasks diff --git a/l2geth/miner/worker.go b/l2geth/miner/worker.go index 43b01d1a5ae74..52174b668dd3f 100644 --- a/l2geth/miner/worker.go +++ b/l2geth/miner/worker.go @@ -110,7 +110,6 @@ const ( // newWorkReq represents a request for new sealing work submitting with relative interrupt notifier. type newWorkReq struct { interrupt *int32 - noempty bool timestamp int64 } @@ -307,12 +306,12 @@ func (w *worker) newWorkLoop(recommit time.Duration) { <-timer.C // discard the initial tick // commit aborts in-flight transaction execution with given signal and resubmits a new one. - commit := func(noempty bool, s int32) { + commit := func(s int32) { if interrupt != nil { atomic.StoreInt32(interrupt, s) } interrupt = new(int32) - w.newWorkCh <- &newWorkReq{interrupt: interrupt, noempty: noempty, timestamp: timestamp} + w.newWorkCh <- &newWorkReq{interrupt: interrupt, timestamp: timestamp} timer.Reset(recommit) atomic.StoreInt32(&w.newTxs, 0) } @@ -352,7 +351,7 @@ func (w *worker) newWorkLoop(recommit time.Duration) { select { case <-w.startCh: clearPending(w.chain.CurrentBlock().NumberU64()) - commit(false, commitInterruptNewHead) + commit(commitInterruptNewHead) // Remove this code for the OVM implementation. It is responsible for // cleaning up memory with the call to `clearPending`, so be sure to @@ -361,7 +360,7 @@ func (w *worker) newWorkLoop(recommit time.Duration) { case <-w.chainHeadCh: clearPending(head.Block.NumberU64()) timestamp = time.Now().Unix() - commit(false, commitInterruptNewHead) + commit(commitInterruptNewHead) */ case <-timer.C: @@ -373,7 +372,7 @@ func (w *worker) newWorkLoop(recommit time.Duration) { timer.Reset(recommit) continue } - commit(true, commitInterruptResubmit) + commit(commitInterruptResubmit) } case interval := <-w.resubmitIntervalCh: @@ -421,7 +420,7 @@ func (w *worker) mainLoop() { for { select { case req := <-w.newWorkCh: - w.commitNewWork(req.interrupt, req.noempty, req.timestamp) + w.commitNewWork(req.interrupt, req.timestamp) case ev := <-w.chainSideCh: // Short circuit for duplicate side blocks @@ -458,7 +457,7 @@ func (w *worker) mainLoop() { uncles = append(uncles, uncle.Header()) return false }) - w.commit(uncles, nil, true, start) + w.commit(uncles, nil, start) } } // Read from the sync service and mine single txs @@ -544,7 +543,7 @@ func (w *worker) mainLoop() { // If clique is running in dev mode(period is 0), disable // advance sealing here. if w.chainConfig.Clique != nil && w.chainConfig.Clique.Period == 0 { - w.commitNewWork(nil, true, time.Now().Unix()) + w.commitNewWork(nil, time.Now().Unix()) } } atomic.AddInt32(&w.newTxs, int32(len(ev.Txs))) @@ -784,11 +783,6 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin } var coalescedLogs []*types.Log - // UsingOVM - // Keep track of the number of transactions being added to the block. - // Blocks should only have a single transaction. This value is used to - // compute a success return value - var txCount int for { // In the following three cases, we will interrupt the execution of the transaction. @@ -809,7 +803,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin inc: true, } } - return atomic.LoadInt32(interrupt) == commitInterruptNewHead + return w.current.tcount == 0 || atomic.LoadInt32(interrupt) == commitInterruptNewHead } // If we don't have enough gas for any further transactions then we're done if w.current.gasPool.Gas() < params.TxGas { @@ -822,8 +816,6 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin break } - txCount++ - // Error may be ignored here. The error has already been checked // during transaction acceptance is the transaction pool. // @@ -891,7 +883,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin if interrupt != nil { w.resubmitAdjustCh <- &intervalAdjust{inc: false} } - return txCount == 0 + return w.current.tcount == 0 } // commitNewTx is an OVM addition that mines a block with a single tx in it. @@ -956,11 +948,11 @@ func (w *worker) commitNewTx(tx *types.Transaction) error { if w.commitTransactions(txs, w.coinbase, nil) { return errors.New("Cannot commit transaction in miner") } - return w.commit(nil, w.fullTaskHook, true, tstart) + return w.commit(nil, w.fullTaskHook, tstart) } // commitNewWork generates several new sealing tasks based on the parent block. -func (w *worker) commitNewWork(interrupt *int32, noempty bool, timestamp int64) { +func (w *worker) commitNewWork(interrupt *int32, timestamp int64) { w.mu.RLock() defer w.mu.RUnlock() @@ -1036,12 +1028,6 @@ func (w *worker) commitNewWork(interrupt *int32, noempty bool, timestamp int64) commitUncles(w.localUncles) commitUncles(w.remoteUncles) - if !noempty { - // Create an empty block based on temporary copied state for sealing in advance without waiting block - // execution finished. - w.commit(uncles, nil, false, tstart) - } - // Fill the block with all available pending transactions. pending, err := w.eth.TxPool().Pending() if err != nil { @@ -1073,12 +1059,12 @@ func (w *worker) commitNewWork(interrupt *int32, noempty bool, timestamp int64) return } } - w.commit(uncles, w.fullTaskHook, true, tstart) + w.commit(uncles, w.fullTaskHook, tstart) } // commit runs any post-transaction state modifications, assembles the final block // and commits new work if consensus engine is running. -func (w *worker) commit(uncles []*types.Header, interval func(), update bool, start time.Time) error { +func (w *worker) commit(uncles []*types.Header, interval func(), start time.Time) error { // Deep copy receipts here to avoid interaction between different tasks. receipts := make([]*types.Receipt, len(w.current.receipts)) for i, l := range w.current.receipts { @@ -1090,6 +1076,15 @@ func (w *worker) commit(uncles []*types.Header, interval func(), update bool, st if err != nil { return err } + + // As a sanity check, ensure all new blocks have exactly one + // transaction. This check is done here just in case any of our + // higher-evel checks failed to catch empty blocks passed to commit. + txs := block.Transactions() + if len(txs) != 1 { + return fmt.Errorf("Block created with %d transactions rather than 1 at %d", len(txs), block.NumberU64()) + } + if w.isRunning() { if interval != nil { interval() @@ -1106,10 +1101,6 @@ func (w *worker) commit(uncles []*types.Header, interval func(), update bool, st } feesEth := new(big.Float).Quo(new(big.Float).SetInt(feesWei), new(big.Float).SetInt(big.NewInt(params.Ether))) - txs := block.Transactions() - if len(txs) != 1 { - return fmt.Errorf("Block created with not %d transactions at %d", len(txs), block.NumberU64()) - } tx := txs[0] bn := tx.L1BlockNumber() if bn == nil { @@ -1122,9 +1113,7 @@ func (w *worker) commit(uncles []*types.Header, interval func(), update bool, st log.Info("Worker has exited") } } - if update { - w.updateSnapshot() - } + w.updateSnapshot() return nil } diff --git a/l2geth/miner/worker_test.go b/l2geth/miner/worker_test.go index 2b09d6e53f642..c55dc089a7adc 100644 --- a/l2geth/miner/worker_test.go +++ b/l2geth/miner/worker_test.go @@ -328,15 +328,35 @@ func TestStreamUncleBlock(t *testing.T) { taskIndex := 0 w.newTaskHook = func(task *task) { if task.block.NumberU64() == 2 { - // The first task is an empty task, the second - // one has 1 pending tx, the third one has 1 tx - // and 1 uncle. - if taskIndex == 2 { + // The first task has 1 pending tx, the second one has 1 + // tx and 1 uncle. + numTxs := len(task.block.Transactions()) + numUncles := len(task.block.Uncles()) + + switch taskIndex { + case 0: + if numTxs != 1 { + t.Errorf("expected 1 tx in first task, got: %d", numTxs) + } + if numUncles != 0 { + t.Errorf("expected no uncles in first task, got: %d", numUncles) + } + + case 1: + if numTxs != 1 { + t.Errorf("expected 1 tx in second task, got: %d", numTxs) + } + if numUncles != 1 { + t.Errorf("expected 1 uncle in second task, got: %d", numUncles) + } have := task.block.Header().UncleHash want := types.CalcUncleHash([]*types.Header{b.uncleBlock.Header()}) if have != want { t.Errorf("uncle hash mismatch: have %s, want %s", have.Hex(), want.Hex()) } + + default: + t.Errorf("only expected two tasks") } taskCh <- struct{}{} taskIndex += 1 @@ -350,12 +370,10 @@ func TestStreamUncleBlock(t *testing.T) { } w.start() - for i := 0; i < 2; i += 1 { - select { - case <-taskCh: - case <-time.NewTimer(time.Second).C: - t.Error("new task timeout") - } + select { + case <-taskCh: + case <-time.NewTimer(time.Second).C: + t.Error("new task timeout") } w.postSideBlock(core.ChainSideEvent{Block: b.uncleBlock})