diff --git a/miner/payload_building.go b/miner/payload_building.go index e57c198c80..eaa7a41f65 100644 --- a/miner/payload_building.go +++ b/miner/payload_building.go @@ -156,7 +156,7 @@ func (payload *Payload) Resolve() *engine.ExecutionPayloadEnvelope { defer payload.lock.Unlock() // Interrupt payload generation - payload.interrupt.Store(commitInterruptResolve) + payload.interruptWorker() // TODO: Since we don't wait for the next payload update via payload.cond.Wait(), // like we do in ResolveFull, we probably wouldn't see the result here, // even though it would be ready very soon. @@ -191,9 +191,6 @@ func (payload *Payload) ResolveFull() *engine.ExecutionPayloadEnvelope { payload.lock.Lock() defer payload.lock.Unlock() - // Interrupt payload generation - payload.interrupt.Store(commitInterruptResolve) - if payload.full == nil { select { case <-payload.stop: @@ -204,6 +201,9 @@ func (payload *Payload) ResolveFull() *engine.ExecutionPayloadEnvelope { // forever if Resolve is called in the meantime which // terminates the background construction process. payload.cond.Wait() + } else { + // Interrupt payload generation only if at least one full block has already been built completely. + payload.interruptWorker() } // Terminate the background payload construction select { @@ -214,6 +214,10 @@ func (payload *Payload) ResolveFull() *engine.ExecutionPayloadEnvelope { return engine.BlockToExecutableData(payload.full, payload.fullFees, payload.sidecars) } +func (payload *Payload) interruptWorker() { + payload.interrupt.Store(commitInterruptResolve) +} + func (w *worker) emptyBlock(args *BuildPayloadArgs) *newPayloadResult { // Build the initial version with no transaction included. It should be fast // enough to run. The empty payload can at least make sure there is something @@ -244,6 +248,9 @@ func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { // we synchronously build the empty block, but when we do have a tx pool, // we build even the first block async. This behavior is less consistent, // but makes payload resolution easier. + // On 2nd thought, I think it makes sense to keep this behavior because we use the NoTxPool case + // when the block's timestamp is out of the sequencer drift window, so getPayload + // can then immediately be called after forkchoiceUpdated. empty := w.emptyBlock(args) if empty.err != nil { return nil, empty.err diff --git a/miner/payload_building_test.go b/miner/payload_building_test.go index 78037b3014..b17ec1ea0f 100644 --- a/miner/payload_building_test.go +++ b/miner/payload_building_test.go @@ -30,6 +30,12 @@ import ( ) func TestBuildPayload(t *testing.T) { + t.Run("with-tx-pool", func(t *testing.T) { testBuildPayload(t, false) }) + t.Run("no-tx-pool", func(t *testing.T) { testBuildPayload(t, true) }) +} + +func testBuildPayload(t *testing.T, noTxPool bool) { + t.Helper() var ( db = rawdb.NewMemoryDatabase() recipient = common.HexToAddress("0xdeadbeef") @@ -38,12 +44,12 @@ func TestBuildPayload(t *testing.T) { defer w.close() timestamp := uint64(time.Now().Unix()) - // TODO: Add NoTxPool = true test case args := &BuildPayloadArgs{ Parent: b.chain.CurrentBlock().Hash(), Timestamp: timestamp, Random: common.Hash{}, FeeRecipient: recipient, + NoTxPool: noTxPool, } payload, err := w.buildPayload(args) if err != nil { @@ -64,14 +70,20 @@ func TestBuildPayload(t *testing.T) { t.Fatal("Unexpect fee recipient") } if len(payload.Transactions) != txs { - t.Fatal("Unexpect transaction set") + t.Fatalf("Unexpect transaction set: got %d, expected %d", len(payload.Transactions), txs) } } - empty := payload.ResolveEmpty() - verify(empty, 0) - full := payload.ResolveFull() - verify(full, len(pendingTxs)) + if noTxPool { + // we only build the empty block when ignoring the tx pool + empty := payload.ResolveEmpty() + verify(empty, 0) + full := payload.ResolveFull() + verify(full, 0) + } else { + full := payload.ResolveFull() + verify(full, len(pendingTxs)) + } // Ensure resolve can be called multiple times and the // result should be unchanged