miner: Add block building interruption on payload resolution (getPayl…

…oad) (#186)

* miner: Add block building interruption on payload resolution (getPayload)

* miner: Change full payload resolution, fix and add test

* miner: Add parameter validation if skipping empty block

We only build the empty block if we don't use the tx pool.
So if we use the tx pool, a forkchoiceUpdated call would miss
the implicit validation that's happening during empty block building,
so we need to add it back.

* miner: Always wait for block builder result after interrupting

This commit changes the way the block builder/update routine and the
resolution functions Resolve and ResolveFull synchronize.
Resolve(Full) now signal the payload builder to pause and set the
interrupt signal in case any block building is ongoing. They then
wait for the interrupted block building to complete.

This allowed to simplify the Payload implementation somewhat because the
builder routine is now guaranteed to return before the resulting fields
(full, fullFees etc) are read, and closing of the `stop` channel is now
synchronized with a sync.Once. So the mutex and conditional variable
could be removed and we only use two simple signalling channels
`stop` and `done` for synchronization.

* miner: Add testing mode to module

Some test in the miner and catalyst package assume that getPayload
can be immediately called after forkchoiceUpdated and then to return
some built block. Because of the new behavior of payload resolution to
interrupt any ongoing payload building process, this creates a race
condition on block building.

The new testing mode, which can be enabled by setting the package variable
IsPayloadBuildingTest to true, guarantees that always at least one
full block is built.

It's hacky, but seems to be the easiest and less-intrusive way to enable
the new behavior of payload resolution while still keeping all tests
happy.

* miner: Further improve block building interruption

- Priotize stop signal over recommit
- Don't start payload building update if last update duration
  doesn't fit until slot timeout.

* miner: Partially revert rework of payload build stopping

When resolving, we don't want to wait for the latest update.
If a full block is available, we just return that one, as before.
Payload building is still interrupted, but exits in the background.

* miner: Return early when building interrupted payload updates

* Remove global variable to change miner behaviour.
Use a longer wait in tests for the payload to build.

* miner: Interrupt first payload building job

Also added interrupt test.
Had to add sleep to make non-interrupt test work.

* eth/catalyst: Add even more sleeps to make tests get over payload interruption

* Deterministically wait for payloads to build the first full block

* eth/catalyst,miner: Improve payload full block waiting in tests

Also fix a bug in TestNilWithdrawals where the withdrawals weren't added
to the ephemeral BuildPayloadArgs instance for re-calculating the
payload id.

* miner: Calculate sane block building time in validateParams

Also always stop interrupt timer after fillTransactions in generateWork.

---------

Co-authored-by: Adrian Sutton <adrian@oplabs.co>

eth/catalyst/api_test.go

@@ -28,6 +28,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/ethereum/go-ethereum/beacon/engine"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
@@ -198,19 +200,19 @@ func TestEth2PrepareAndGetPayload(t *testing.T) {
 		SafeBlockHash:      common.Hash{},
 		FinalizedBlockHash: common.Hash{},
 	}
-	_, err := api.ForkchoiceUpdatedV1(fcState, &blockParams)
+	resp, err := api.ForkchoiceUpdatedV1(fcState, &blockParams)
 	if err != nil {
 		t.Fatalf("error preparing payload, err=%v", err)
 	}
-	// give the payload some time to be built
-	time.Sleep(100 * time.Millisecond)
 	payloadID := (&miner.BuildPayloadArgs{
 		Parent:       fcState.HeadBlockHash,
 		Timestamp:    blockParams.Timestamp,
 		FeeRecipient: blockParams.SuggestedFeeRecipient,
 		Random:       blockParams.Random,
 		BeaconRoot:   blockParams.BeaconRoot,
 	}).Id()
+	require.Equal(t, payloadID, *resp.PayloadID)
+	require.NoError(t, waitForApiPayloadToBuild(api, *resp.PayloadID))
 	execData, err := api.GetPayloadV1(payloadID)
 	if err != nil {
 		t.Fatalf("error getting payload, err=%v", err)
@@ -635,8 +637,7 @@ func TestNewPayloadOnInvalidChain(t *testing.T) {
 			if resp.PayloadStatus.Status != engine.VALID {
 				t.Fatalf("error preparing payload, invalid status: %v", resp.PayloadStatus.Status)
 			}
-			// give the payload some time to be built
-			time.Sleep(50 * time.Millisecond)
+			require.NoError(t, waitForApiPayloadToBuild(api, *resp.PayloadID))
 			if payload, err = api.GetPayloadV1(*resp.PayloadID); err != nil {
 				t.Fatalf("can't get payload: %v", err)
 			}
@@ -684,6 +685,7 @@ func assembleBlock(api *ConsensusAPI, parentHash common.Hash, params *engine.Pay
 	if err != nil {
 		return nil, err
 	}
+	waitForPayloadToBuild(payload)
 	return payload.ResolveFull().ExecutionPayload, nil
 }
 
@@ -922,6 +924,7 @@ func TestNewPayloadOnInvalidTerminalBlock(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error preparing payload, err=%v", err)
 	}
+	waitForPayloadToBuild(payload)
 	data := *payload.Resolve().ExecutionPayload
 	// We need to recompute the blockhash, since the miner computes a wrong (correct) blockhash
 	txs, _ := decodeTransactions(data.Transactions)
@@ -1082,6 +1085,8 @@ func TestWithdrawals(t *testing.T) {
 		Withdrawals:  blockParams.Withdrawals,
 		BeaconRoot:   blockParams.BeaconRoot,
 	}).Id()
+	require.Equal(t, payloadID, *resp.PayloadID)
+	require.NoError(t, waitForApiPayloadToBuild(api, payloadID))
 	execData, err := api.GetPayloadV2(payloadID)
 	if err != nil {
 		t.Fatalf("error getting payload, err=%v", err)
@@ -1116,7 +1121,7 @@ func TestWithdrawals(t *testing.T) {
 		},
 	}
 	fcState.HeadBlockHash = execData.ExecutionPayload.BlockHash
-	_, err = api.ForkchoiceUpdatedV2(fcState, &blockParams)
+	resp, err = api.ForkchoiceUpdatedV2(fcState, &blockParams)
 	if err != nil {
 		t.Fatalf("error preparing payload, err=%v", err)
 	}
@@ -1130,6 +1135,8 @@ func TestWithdrawals(t *testing.T) {
 		Withdrawals:  blockParams.Withdrawals,
 		BeaconRoot:   blockParams.BeaconRoot,
 	}).Id()
+	require.Equal(t, payloadID, *resp.PayloadID)
+	require.NoError(t, waitForApiPayloadToBuild(api, payloadID))
 	execData, err = api.GetPayloadV2(payloadID)
 	if err != nil {
 		t.Fatalf("error getting payload, err=%v", err)
@@ -1242,7 +1249,7 @@ func TestNilWithdrawals(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		_, err := api.ForkchoiceUpdatedV2(fcState, &test.blockParams)
+		resp, err := api.ForkchoiceUpdatedV2(fcState, &test.blockParams)
 		if test.wantErr {
 			if err == nil {
 				t.Fatal("wanted error on fcuv2 with invalid withdrawals")
@@ -1260,7 +1267,10 @@ func TestNilWithdrawals(t *testing.T) {
 			FeeRecipient: test.blockParams.SuggestedFeeRecipient,
 			Random:       test.blockParams.Random,
 			BeaconRoot:   test.blockParams.BeaconRoot,
+			Withdrawals:  test.blockParams.Withdrawals,
 		}).Id()
+		require.Equal(t, payloadID, *resp.PayloadID)
+		require.NoError(t, waitForApiPayloadToBuild(api, payloadID))
 		execData, err := api.GetPayloadV2(payloadID)
 		if err != nil {
 			t.Fatalf("error getting payload, err=%v", err)
@@ -1609,6 +1619,8 @@ func TestParentBeaconBlockRoot(t *testing.T) {
 		Withdrawals:  blockParams.Withdrawals,
 		BeaconRoot:   blockParams.BeaconRoot,
 	}).Id()
+	require.Equal(t, payloadID, *resp.PayloadID)
+	require.NoError(t, waitForApiPayloadToBuild(api, *resp.PayloadID))
 	execData, err := api.GetPayloadV3(payloadID)
 	if err != nil {
 		t.Fatalf("error getting payload, err=%v", err)
@@ -1647,3 +1659,11 @@ func TestParentBeaconBlockRoot(t *testing.T) {
 		t.Fatalf("incorrect root stored: want %s, got %s", *blockParams.BeaconRoot, root)
 	}
 }
+
+func waitForPayloadToBuild(payload *miner.Payload) {
+	payload.WaitFull()
+}
+
+func waitForApiPayloadToBuild(api *ConsensusAPI, id engine.PayloadID) error {
+	return api.localBlocks.waitFull(id)
+}

eth/catalyst/queue.go

@@ -17,6 +17,7 @@
 package catalyst
 
 import (
+	"errors"
 	"sync"
 
 	"github.com/ethereum/go-ethereum/beacon/engine"
@@ -91,6 +92,24 @@ func (q *payloadQueue) get(id engine.PayloadID, full bool) *engine.ExecutionPayl
 	return nil
 }
 
+// waitFull waits until the first full payload has been built for the specified payload id
+// The method returns immediately if the payload is unknown.
+func (q *payloadQueue) waitFull(id engine.PayloadID) error {
+	q.lock.RLock()
+	defer q.lock.RUnlock()
+
+	for _, item := range q.payloads {
+		if item == nil {
+			return errors.New("unknown payload")
+		}
+		if item.id == id {
+			item.payload.WaitFull()
+			return nil
+		}
+	}
+	return errors.New("unknown payload")
+}
+
 // has checks if a particular payload is already tracked.
 func (q *payloadQueue) has(id engine.PayloadID) bool {
 	q.lock.RLock()

eth/catalyst/simulated_beacon.go

@@ -165,6 +165,9 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal) error {
 		return errors.New("chain rewind prevented invocation of payload creation")
 	}
 
+	if err := c.engineAPI.localBlocks.waitFull(*fcResponse.PayloadID); err != nil {
+		return err
+	}
 	envelope, err := c.engineAPI.getPayload(*fcResponse.PayloadID, true)
 	if err != nil {
 		return err