simulators/ethereum/engine: Fix several tests (#886)

* simulators/ethereum/engine: fix versioning tests

* simulators/ethereum/engine: fix transaction replacement

* simulators/ethereum/engine: CLMock: Per-client payload id map

* simulators/ethereum/engine: Unique Payload ID tests in Cancun

simulators/ethereum/engine/clmock/clmock.go

@@ -91,7 +91,7 @@ type CLMocker struct {
 	HeaderHistory map[uint64]*types.Header
 
 	// Payload ID History
-	PayloadIDHistory map[api.PayloadID]interface{}
+	payloadIDHistory map[string]map[api.PayloadID]interface{}
 
 	// PoS Chain History Information
 	PrevRandaoHistory      map[uint64]common.Hash
@@ -146,7 +146,7 @@ func NewCLMocker(t *hivesim.T, genesis *core.Genesis, forkConfig *config.ForkCon
 		PayloadProductionClientDelay:    DefaultPayloadProductionClientDelay,
 		BlockTimestampIncrement:         DefaultBlockTimestampIncrement,
 
-		PayloadIDHistory:    make(map[api.PayloadID]interface{}),
+		payloadIDHistory:    make(map[string]map[api.PayloadID]interface{}),
 		LatestHeader:        nil,
 		FirstPoSBlockNumber: nil,
 		LatestHeadNumber:    nil,
@@ -442,14 +442,21 @@ func (cl *CLMocker) GeneratePayloadAttributes() {
 	cl.PrevRandaoHistory[cl.LatestHeader.Number.Uint64()+1] = nextPrevRandao
 }
 
-func (cl *CLMocker) AddPayloadID(newPayloadID *api.PayloadID) error {
+func (cl *CLMocker) AddPayloadID(ec client.EngineClient, newPayloadID *api.PayloadID) error {
 	if newPayloadID == nil {
 		return errors.New("nil payload ID")
 	}
-	if _, ok := cl.PayloadIDHistory[*newPayloadID]; ok {
+	// Get map for given client
+	if _, ok := cl.payloadIDHistory[ec.ID()]; !ok {
+		cl.payloadIDHistory[ec.ID()] = make(map[api.PayloadID]interface{})
+	}
+	// Check if payload ID has been used before
+	if _, ok := cl.payloadIDHistory[ec.ID()][*newPayloadID]; ok {
 		return fmt.Errorf("reused payload ID: %v", *newPayloadID)
 	}
-	cl.PayloadIDHistory[*newPayloadID] = nil
+	// Add payload ID to history
+	cl.payloadIDHistory[ec.ID()][*newPayloadID] = nil
+	fmt.Printf("CLMocker: Added payload ID %v for client %v\n", *newPayloadID, ec.ID())
 	return nil
 }
 
@@ -467,7 +474,7 @@ func (cl *CLMocker) RequestNextPayload() {
 	if resp.PayloadStatus.LatestValidHash == nil || *resp.PayloadStatus.LatestValidHash != cl.LatestForkchoice.HeadBlockHash {
 		cl.Fatalf("CLMocker: Unexpected forkchoiceUpdated LatestValidHash Response from Payload builder: %v != %v", resp.PayloadStatus.LatestValidHash, cl.LatestForkchoice.HeadBlockHash)
 	}
-	if err = cl.AddPayloadID(resp.PayloadID); err != nil {
+	if err = cl.AddPayloadID(cl.NextBlockProducer, resp.PayloadID); err != nil {
 		cl.Fatalf("CLMocker: Payload ID failure: %v", err)
 	}
 	cl.NextPayloadID = resp.PayloadID

simulators/ethereum/engine/helper/customizer.go

@@ -137,12 +137,14 @@ type TimestampDeltaPayloadAttributesCustomizer struct {
 var _ PayloadAttributesCustomizer = (*TimestampDeltaPayloadAttributesCustomizer)(nil)
 
 func (customData *TimestampDeltaPayloadAttributesCustomizer) GetPayloadAttributes(basePayloadAttributes *typ.PayloadAttributes) (*typ.PayloadAttributes, error) {
-	customPayloadAttributes, err := customData.PayloadAttributesCustomizer.GetPayloadAttributes(basePayloadAttributes)
-	if err != nil {
-		return nil, err
+	if customData.PayloadAttributesCustomizer != nil {
+		var err error
+		if basePayloadAttributes, err = customData.PayloadAttributesCustomizer.GetPayloadAttributes(basePayloadAttributes); err != nil {
+			return nil, err
+		}
 	}
-	customPayloadAttributes.Timestamp = uint64(int64(customPayloadAttributes.Timestamp) + customData.TimestampDelta)
-	return customPayloadAttributes, nil
+	basePayloadAttributes.Timestamp = uint64(int64(basePayloadAttributes.Timestamp) + customData.TimestampDelta)
+	return basePayloadAttributes, nil
 }
 
 // Customizer that makes no modifications to the forkchoice directive call.

simulators/ethereum/engine/helper/tx.go

@@ -512,7 +512,11 @@ func (txSender *TransactionSender) GetLastNonce(ctx context.Context, node client
 	if txSender.nonceMap != nil {
 		txSender.nonceMapLock.Lock()
 		defer txSender.nonceMapLock.Unlock()
-		return txSender.nonceMap[sender.GetAddress()], nil
+		nextNonce := txSender.nonceMap[sender.GetAddress()]
+		if nextNonce > 0 {
+			return nextNonce - 1, nil
+		}
+		return 0, fmt.Errorf("no previous nonce found in map for %s", sender.GetAddress().Hex())
 	} else {
 		return node.GetLastAccountNonce(ctx, sender.GetAddress(), header)
 	}

simulators/ethereum/engine/suites/cancun/steps.go

@@ -439,7 +439,7 @@ func (step NewPayloads) Execute(t *CancunTestContext) error {
 						r.ExpectPayloadStatus(expectedStatus)
 					}
 					if r.Response.PayloadID != nil {
-						t.CLMock.AddPayloadID(r.Response.PayloadID)
+						t.CLMock.AddPayloadID(t.Engine, r.Response.PayloadID)
 					}
 				}
 			},

simulators/ethereum/engine/suites/cancun/tests.go

@@ -1827,6 +1827,23 @@ func init() {
 		Tests = append(Tests, t)
 	}
 
+	// Unique Payload ID Tests
+	for _, t := range []suite_engine.PayloadAttributesFieldChange{
+		suite_engine.PayloadAttributesParentBeaconRoot,
+		// TODO: Remove when withdrawals suite is refactored
+		suite_engine.PayloadAttributesAddWithdrawal,
+		suite_engine.PayloadAttributesModifyWithdrawalAmount,
+		suite_engine.PayloadAttributesModifyWithdrawalIndex,
+		suite_engine.PayloadAttributesModifyWithdrawalValidator,
+		suite_engine.PayloadAttributesModifyWithdrawalAddress,
+		suite_engine.PayloadAttributesRemoveWithdrawal,
+	} {
+		Tests = append(Tests, suite_engine.UniquePayloadIDTest{
+			BaseSpec:          baseSpec,
+			FieldModification: t,
+		})
+	}
+
 	// Invalid Payload Tests
 	for _, invalidField := range []helper.InvalidPayloadBlockField{
 		helper.InvalidParentBeaconBlockRoot,

simulators/ethereum/engine/suites/engine/payload_id.go

@@ -80,7 +80,12 @@ func (tc UniquePayloadIDTest) Execute(t *test.Env) {
 				}
 				payloadAttributes.Withdrawals = append(types.Withdrawals{&modifiedWithdrawal}, payloadAttributes.Withdrawals[1:]...)
 			case PayloadAttributesParentBeaconRoot:
-				payloadAttributes.BeaconRoot[0] = payloadAttributes.BeaconRoot[0] + 1
+				if payloadAttributes.BeaconRoot == nil {
+					t.Fatalf("Cannot modify parent beacon root when there is no parent beacon root")
+				}
+				newBeaconRoot := *payloadAttributes.BeaconRoot
+				newBeaconRoot[0] = newBeaconRoot[0] + 1
+				payloadAttributes.BeaconRoot = &newBeaconRoot
 			default:
 				t.Fatalf("Unknown field change: %s", tc.FieldModification)
 			}
@@ -89,7 +94,7 @@ func (tc UniquePayloadIDTest) Execute(t *test.Env) {
 			r := t.TestEngine.TestEngineForkchoiceUpdated(&t.CLMock.
 				LatestForkchoice, &payloadAttributes, t.CLMock.LatestHeader.Time)
 			r.ExpectNoError()
-			t.CLMock.AddPayloadID(r.Response.PayloadID)
+			t.CLMock.AddPayloadID(t.Engine, r.Response.PayloadID)
 		},
 	})
 }

simulators/ethereum/engine/suites/engine/tests.go

@@ -115,30 +115,29 @@ func init() {
 	Tests = append(Tests,
 		ForkchoiceUpdatedOnPayloadRequestTest{
 			BaseSpec: test.BaseSpec{
-				Name:       "Early upgrade",
-				ForkHeight: 2,
+				Name: "Early upgrade",
+				About: `
+				Early upgrade of ForkchoiceUpdated when requesting a payload.
+				The test sets the fork height to 1, and the block timestamp increments to 2
+				seconds each block.
+				CL Mock prepares the payload attributes for the first block, which should contain
+				the attributes of the next fork.
+				The test then reduces the timestamp by 1, but still uses the next forkchoice updated
+				version, which should result in UNSUPPORTED_FORK_ERROR error.
+				`,
+				ForkHeight:              1,
+				BlockTimestampIncrement: 2,
 			},
 			ForkchoiceUpdatedCustomizer: &helper.UpgradeForkchoiceUpdatedVersion{
 				ForkchoiceUpdatedCustomizer: &helper.BaseForkchoiceUpdatedCustomizer{
+					PayloadAttributesCustomizer: &helper.TimestampDeltaPayloadAttributesCustomizer{
+						PayloadAttributesCustomizer: &helper.BasePayloadAttributesCustomizer{},
+						TimestampDelta:              -1,
+					},
 					ExpectedError: globals.UNSUPPORTED_FORK_ERROR,
 				},
 			},
 		},
-		/*
-			TODO: This test is failing because the upgraded version of the ForkchoiceUpdated does not contain the
-			      expected fields of the following version.
-			ForkchoiceUpdatedOnHeadBlockUpdateTest{
-				BaseSpec: test.BaseSpec{
-					Name:       "Early upgrade",
-					ForkHeight: 2,
-				},
-				ForkchoiceUpdatedCustomizer: &helper.UpgradeForkchoiceUpdatedVersion{
-					ForkchoiceUpdatedCustomizer: &helper.BaseForkchoiceUpdatedCustomizer{
-						ExpectedError: globals.UNSUPPORTED_FORK_ERROR,
-					},
-				},
-			},
-		*/
 	)
 
 	// Payload Execution Tests

simulators/ethereum/engine/suites/engine/versioning.go

@@ -1,7 +1,6 @@
 package suite_engine
 
 import (
-	api "github.com/ethereum/go-ethereum/beacon/engine"
 	"github.com/ethereum/hive/simulators/ethereum/engine/clmock"
 	"github.com/ethereum/hive/simulators/ethereum/engine/config"
 	"github.com/ethereum/hive/simulators/ethereum/engine/helper"
@@ -74,58 +73,3 @@ func (tc ForkchoiceUpdatedOnPayloadRequestTest) Execute(t *test.Env) {
 		},
 	})
 }
-
-// Test modifying the ForkchoiceUpdated version on HeadBlockHash update to the previous/upcoming
-// version when the timestamp payload attribute does not match the upgraded/downgraded version.
-type ForkchoiceUpdatedOnHeadBlockUpdateTest struct {
-	test.BaseSpec
-	helper.ForkchoiceUpdatedCustomizer
-}
-
-func (s ForkchoiceUpdatedOnHeadBlockUpdateTest) WithMainFork(fork config.Fork) test.Spec {
-	specCopy := s
-	specCopy.MainFork = fork
-	return specCopy
-}
-
-func (tc ForkchoiceUpdatedOnHeadBlockUpdateTest) GetName() string {
-	return "ForkchoiceUpdated Version on Head Set: " + tc.BaseSpec.GetName()
-}
-
-func (tc ForkchoiceUpdatedOnHeadBlockUpdateTest) Execute(t *test.Env) {
-	// Wait until TTD is reached by this client
-	t.CLMock.WaitForTTD()
-
-	t.CLMock.ProduceSingleBlock(clmock.BlockProcessCallbacks{
-		OnPayloadAttributesGenerated: func() {
-			var (
-				forkchoiceState *api.ForkchoiceStateV1 = &api.ForkchoiceStateV1{
-					HeadBlockHash:      t.CLMock.LatestPayloadBuilt.BlockHash,
-					SafeBlockHash:      t.CLMock.LatestForkchoice.SafeBlockHash,
-					FinalizedBlockHash: t.CLMock.LatestForkchoice.FinalizedBlockHash,
-				}
-				expectedError  *int
-				expectedStatus test.PayloadStatus = test.Valid
-				err            error
-			)
-			tc.SetEngineAPIVersionResolver(t.ForkConfig)
-			testEngine := t.TestEngine.WithEngineAPIVersionResolver(tc.ForkchoiceUpdatedCustomizer)
-			expectedError, err = tc.GetExpectedError()
-			if err != nil {
-				t.Fatalf("FAIL: Error getting custom expected error: %v", err)
-			}
-			if tc.GetExpectInvalidStatus() {
-				expectedStatus = test.Invalid
-			}
-
-			r := testEngine.TestEngineForkchoiceUpdated(forkchoiceState, nil, t.CLMock.LatestPayloadBuilt.Timestamp)
-			r.ExpectationDescription = tc.Expectation
-			if expectedError != nil {
-				r.ExpectErrorCode(*expectedError)
-			} else {
-				r.ExpectNoError()
-				r.ExpectPayloadStatus(expectedStatus)
-			}
-		},
-	})
-}