From 219e6f6eb47e948f71f4f11f3899623c2b6f7a20 Mon Sep 17 00:00:00 2001 From: Mario Vega Date: Thu, 28 Sep 2023 13:19:45 -0600 Subject: [PATCH] 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 | 19 +++++-- .../ethereum/engine/helper/customizer.go | 12 ++-- simulators/ethereum/engine/helper/tx.go | 6 +- .../ethereum/engine/suites/cancun/steps.go | 2 +- .../ethereum/engine/suites/cancun/tests.go | 17 ++++++ .../engine/suites/engine/payload_id.go | 9 ++- .../ethereum/engine/suites/engine/tests.go | 33 ++++++----- .../engine/suites/engine/versioning.go | 56 ------------------- 8 files changed, 66 insertions(+), 88 deletions(-) diff --git a/simulators/ethereum/engine/clmock/clmock.go b/simulators/ethereum/engine/clmock/clmock.go index 4da0b0e553..61ae26b235 100644 --- a/simulators/ethereum/engine/clmock/clmock.go +++ b/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 diff --git a/simulators/ethereum/engine/helper/customizer.go b/simulators/ethereum/engine/helper/customizer.go index b2be55f56c..35d7299936 100644 --- a/simulators/ethereum/engine/helper/customizer.go +++ b/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. diff --git a/simulators/ethereum/engine/helper/tx.go b/simulators/ethereum/engine/helper/tx.go index 5f1b66c5bc..d12dabbe92 100644 --- a/simulators/ethereum/engine/helper/tx.go +++ b/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) } diff --git a/simulators/ethereum/engine/suites/cancun/steps.go b/simulators/ethereum/engine/suites/cancun/steps.go index cdece68c71..a29cba3b54 100644 --- a/simulators/ethereum/engine/suites/cancun/steps.go +++ b/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) } } }, diff --git a/simulators/ethereum/engine/suites/cancun/tests.go b/simulators/ethereum/engine/suites/cancun/tests.go index 71c42a82a3..8461e895f8 100644 --- a/simulators/ethereum/engine/suites/cancun/tests.go +++ b/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, diff --git a/simulators/ethereum/engine/suites/engine/payload_id.go b/simulators/ethereum/engine/suites/engine/payload_id.go index 259c9a29a4..4e696a9e4c 100644 --- a/simulators/ethereum/engine/suites/engine/payload_id.go +++ b/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) }, }) } diff --git a/simulators/ethereum/engine/suites/engine/tests.go b/simulators/ethereum/engine/suites/engine/tests.go index 91e79e6264..5f67acdafe 100644 --- a/simulators/ethereum/engine/suites/engine/tests.go +++ b/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 diff --git a/simulators/ethereum/engine/suites/engine/versioning.go b/simulators/ethereum/engine/suites/engine/versioning.go index 52230455a6..a28517a72b 100644 --- a/simulators/ethereum/engine/suites/engine/versioning.go +++ b/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) - } - }, - }) -}