From 5d13b2ae9927261d95f12c44dd36478a881ebc78 Mon Sep 17 00:00:00 2001 From: marioevz Date: Tue, 26 Apr 2022 10:56:02 +0000 Subject: [PATCH 1/2] simulators/ethereum/engine: add invalid payload attributes test --- simulators/ethereum/engine/README.md | 3 + simulators/ethereum/engine/enginetests.go | 73 +++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/simulators/ethereum/engine/README.md b/simulators/ethereum/engine/README.md index 2d16b3c3b2..4da79ea1e9 100644 --- a/simulators/ethereum/engine/README.md +++ b/simulators/ethereum/engine/README.md @@ -64,6 +64,9 @@ Perform a forkchoiceUpdated call with an unknown (random) SafeBlockHash, the cli - Unknown FinalizedBlockHash: Perform a forkchoiceUpdated call with an unknown (random) FinalizedBlockHash, the client should throw an error. +- Invalid Payload Attributes: +Perform a forkchoiceUpdated call with valid forkchoice but invalid payload attributes. + - Pre-TTD Block Hash: Perform a forkchoiceUpdated call using a block hash part of the canonical chain that precedes the block where the TTD occurred. (Behavior is undefined for this edge case and not verified, but should not produce unrecoverable error) diff --git a/simulators/ethereum/engine/enginetests.go b/simulators/ethereum/engine/enginetests.go index b957fa5f64..0562c9972f 100644 --- a/simulators/ethereum/engine/enginetests.go +++ b/simulators/ethereum/engine/enginetests.go @@ -50,6 +50,14 @@ var engineTests = []TestSpec{ Name: "Unknown FinalizedBlockHash", Run: unknownFinalizedBlockHash, }, + { + Name: "ForkchoiceUpdated Invalid Payload Attributes", + Run: invalidPayloadAttributesGen(false), + }, + { + Name: "ForkchoiceUpdated Invalid Payload Attributes (Syncing)", + Run: invalidPayloadAttributesGen(true), + }, { Name: "Pre-TTD ForkchoiceUpdated After PoS Switch", Run: preTTDFinalizedBlockHash, @@ -377,6 +385,71 @@ func unknownHeadBlockHash(t *TestEnv) { } +// Verify behavior on a forkchoiceUpdated with invalid payload attributes +func invalidPayloadAttributesGen(syncing bool) func(*TestEnv) { + + return func(t *TestEnv) { + // Wait until TTD is reached by this client + t.CLMock.waitForTTD() + + // Produce blocks before starting the test + t.CLMock.produceBlocks(5, BlockProcessCallbacks{}) + + // Send a forkchoiceUpdated with invalid PayloadAttributes + t.CLMock.produceSingleBlock(BlockProcessCallbacks{ + OnNewPayloadBroadcast: func() { + // Try to apply the new payload with invalid attributes + var blockHash common.Hash + if syncing { + // Setting a random hash will put the client into `SYNCING` + rand.Read(blockHash[:]) + } else { + // Set the block hash to the next payload that was broadcasted + blockHash = t.CLMock.LatestPayloadBuilt.BlockHash + } + t.Logf("INFO (%s): Sending EngineForkchoiceUpdatedV1 (Syncing=%s) with invalid payload attributes", t.TestName, syncing) + fcu := ForkchoiceStateV1{ + HeadBlockHash: blockHash, + SafeBlockHash: blockHash, + FinalizedBlockHash: blockHash, + } + attr := PayloadAttributesV1{ + Timestamp: 0, + PrevRandao: common.Hash{}, + SuggestedFeeRecipient: common.Address{}, + } + // Outcome for this test is not final, at the moment these two options are being discussed: + // Option 1 + // 0) Check headBlock is known and there is no missing data, if not respond with SYNCING + // 1) Check headBlock is VALID, if not respond with INVALID + // 2) Apply forkchoiceState + // 3) Check payloadAttributes, if invalid respond with error: code: Invalid payload attributes + // 4) Start payload build process and respond with VALID + // + // Option 2 + // 0) Check headBlock is known and there is no missing data, if not respond with SYNCING + // 1) Check payloadAttributes (requires a lookup to database to get a timestamp of yet to be applied headBlock), if invalid respond with error: code: Invalid payload attributes + // 2) Check headBlock is VALID, if not respond with INVALID + // 3) Start payload build process and respond with VALID + if syncing { + // If we are SYNCING, the outcome should be SYNCING regardless of the validity of the payload atttributes + r := t.TestEngine.TestEngineForkchoiceUpdatedV1(&fcu, &attr) + r.ExpectPayloadStatus(Syncing) + r.ExpectPayloadID(nil) + } else { + r := t.TestEngine.TestEngineForkchoiceUpdatedV1(&fcu, &attr) + r.ExpectError() + + // TBD: Check that the forkchoice not applied (Option 2 is implemented here) + s := t.TestEth.TestHeaderByNumber(nil) + s.ExpectHash(t.CLMock.LatestFinalizedHeader.Hash()) + } + }, + }) + } + +} + // Verify that a forkchoiceUpdated fails on hash being set to a pre-TTD block after PoS change func preTTDFinalizedBlockHash(t *TestEnv) { // Wait until TTD is reached by this client From cb2814d63bb33555d29c3f1e77c987ac33db9d9a Mon Sep 17 00:00:00 2001 From: marioevz Date: Fri, 29 Apr 2022 18:34:49 +0000 Subject: [PATCH 2/2] simulators/ethereum/engine: update invalid attributes test --- simulators/ethereum/engine/README.md | 1 + simulators/ethereum/engine/enginetests.go | 12 ++---------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/simulators/ethereum/engine/README.md b/simulators/ethereum/engine/README.md index 4da79ea1e9..61456f7a95 100644 --- a/simulators/ethereum/engine/README.md +++ b/simulators/ethereum/engine/README.md @@ -66,6 +66,7 @@ Perform a forkchoiceUpdated call with an unknown (random) FinalizedBlockHash, th - Invalid Payload Attributes: Perform a forkchoiceUpdated call with valid forkchoice but invalid payload attributes. +Expected outcome is that the forkchoiceUpdate proceeds, but the call returns an error. - Pre-TTD Block Hash: Perform a forkchoiceUpdated call using a block hash part of the canonical chain that precedes the block where the TTD occurred. (Behavior is undefined for this edge case and not verified, but should not produce unrecoverable error) diff --git a/simulators/ethereum/engine/enginetests.go b/simulators/ethereum/engine/enginetests.go index 0562c9972f..5bd94cc16c 100644 --- a/simulators/ethereum/engine/enginetests.go +++ b/simulators/ethereum/engine/enginetests.go @@ -418,19 +418,11 @@ func invalidPayloadAttributesGen(syncing bool) func(*TestEnv) { PrevRandao: common.Hash{}, SuggestedFeeRecipient: common.Address{}, } - // Outcome for this test is not final, at the moment these two options are being discussed: - // Option 1 // 0) Check headBlock is known and there is no missing data, if not respond with SYNCING // 1) Check headBlock is VALID, if not respond with INVALID // 2) Apply forkchoiceState // 3) Check payloadAttributes, if invalid respond with error: code: Invalid payload attributes // 4) Start payload build process and respond with VALID - // - // Option 2 - // 0) Check headBlock is known and there is no missing data, if not respond with SYNCING - // 1) Check payloadAttributes (requires a lookup to database to get a timestamp of yet to be applied headBlock), if invalid respond with error: code: Invalid payload attributes - // 2) Check headBlock is VALID, if not respond with INVALID - // 3) Start payload build process and respond with VALID if syncing { // If we are SYNCING, the outcome should be SYNCING regardless of the validity of the payload atttributes r := t.TestEngine.TestEngineForkchoiceUpdatedV1(&fcu, &attr) @@ -440,9 +432,9 @@ func invalidPayloadAttributesGen(syncing bool) func(*TestEnv) { r := t.TestEngine.TestEngineForkchoiceUpdatedV1(&fcu, &attr) r.ExpectError() - // TBD: Check that the forkchoice not applied (Option 2 is implemented here) + // Check that the forkchoice was applied, regardless of the error s := t.TestEth.TestHeaderByNumber(nil) - s.ExpectHash(t.CLMock.LatestFinalizedHeader.Hash()) + s.ExpectHash(blockHash) } }, })