miner: Change full payload resolution, fix and add test

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

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