simulators/ethereum/engine: add bad hash payload tests (#521)

Adds first batch of new tests based on the coverage document by @mkalinin (https://hackmd.io/vjgC9hV_TrK1ZuftVm1rZA). Tests added: - EL Not SYNCING: Invalid hash payload extending side chain - EL SYNCING: Invalid hash payload extending canonical chain - EL SYNCING: Invalid hash payload extending side chain chain
ethereum · Apr 25, 2022 · 2c672f6 · 2c672f6
1 parent b3eec72
commit 2c672f6
Show file tree

Hide file tree

Showing 2 changed files with 119 additions and 40 deletions.
diff --git a/simulators/ethereum/engine/README.md b/simulators/ethereum/engine/README.md
@@ -68,7 +68,11 @@ Perform a forkchoiceUpdated call with an unknown (random) FinalizedBlockHash, th
 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)
 
 - Bad blockhash on NewPayload:  
-Send a NewPayload directive to the client including an incorrect BlockHash, should result in an error.
+Send a NewPayload directive to the client including an incorrect BlockHash, should result in an error in all the following cases:
+   - NewPayload while not syncing, on canonical chain
+   - NewPayload while not syncing, on side chain
+   - NewPayload while syncing, on canonical chain
+   - NewPayload while syncing, on side chain
 
 - ParentHash==BlockHash on NewPayload:  
 Send a NewPayload directive to the client including ParentHash that is equal to the BlockHash (Incorrect hash).

diff --git a/simulators/ethereum/engine/enginetests.go b/simulators/ethereum/engine/enginetests.go
@@ -57,7 +57,19 @@ var engineTests = []TestSpec{
 	},
 	{
 		Name: "Bad Hash on NewPayload",
-		Run:  badHashOnExecPayload,
+		Run:  badHashOnNewPayloadGen(false, false),
+	},
+	{
+		Name: "Bad Hash on NewPayload Syncing",
+		Run:  badHashOnNewPayloadGen(true, false),
+	},
+	{
+		Name: "Bad Hash on NewPayload Side Chain",
+		Run:  badHashOnNewPayloadGen(false, true),
+	},
+	{
+		Name: "Bad Hash on NewPayload Side Chain Syncing",
+		Run:  badHashOnNewPayloadGen(true, true),
 	},
 	{
 		Name: "ParentHash==BlockHash on NewPayload",
@@ -388,49 +400,112 @@ func preTTDFinalizedBlockHash(t *TestEnv) {
 
 }
 
-// Corrupt the hash of a valid payload, client should reject the payload
-func badHashOnExecPayload(t *TestEnv) {
-	// Wait until TTD is reached by this client
-	t.CLMock.waitForTTD()
+// Corrupt the hash of a valid payload, client should reject the payload.
+// All possible scenarios:
+//    (fcU)
+//	┌────────┐        ┌────────────────────────┐
+//	│  HEAD  │◄───────┤ Bad Hash (!Sync,!Side) │
+//	└────┬───┘        └────────────────────────┘
+//		 │
+//		 │
+//	┌────▼───┐        ┌────────────────────────┐
+//	│ HEAD-1 │◄───────┤ Bad Hash (!Sync, Side) │
+//	└────┬───┘        └────────────────────────┘
+//		 │
+//
+//
+//	  (fcU)
+//	********************  ┌───────────────────────┐
+//	*  (Unknown) HEAD  *◄─┤ Bad Hash (Sync,!Side) │
+//	********************  └───────────────────────┘
+//		 │
+//		 │
+//	┌────▼───┐            ┌───────────────────────┐
+//	│ HEAD-1 │◄───────────┤ Bad Hash (Sync, Side) │
+//	└────┬───┘            └───────────────────────┘
+//		 │
+//
+
+func badHashOnNewPayloadGen(syncing bool, sidechain bool) func(*TestEnv) {
 
-	// Produce blocks before starting the test
-	t.CLMock.produceBlocks(5, BlockProcessCallbacks{})
+	return func(t *TestEnv) {
+		// Wait until TTD is reached by this client
+		t.CLMock.waitForTTD()
 
-	var invalidPayloadHash common.Hash
+		// Produce blocks before starting the test
+		t.CLMock.produceBlocks(5, BlockProcessCallbacks{})
 
-	t.CLMock.produceSingleBlock(BlockProcessCallbacks{
-		// Run test after the new payload has been obtained
-		OnGetPayload: func() {
-			// Alter hash on the payload and send it to client, should produce an error
-			alteredPayload := t.CLMock.LatestPayloadBuilt
-			invalidPayloadHash = alteredPayload.BlockHash
-			invalidPayloadHash[common.HashLength-1] = byte(255 - invalidPayloadHash[common.HashLength-1])
-			alteredPayload.BlockHash = invalidPayloadHash
-			// Execution specification::
-			// - {status: INVALID_BLOCK_HASH, latestValidHash: null, validationError: null} if the blockHash validation has failed
-			r := t.TestEngine.TestEngineNewPayloadV1(&alteredPayload)
-			r.ExpectStatus(InvalidBlockHash)
-		},
-	})
+		var (
+			alteredPayload     ExecutableDataV1
+			invalidPayloadHash common.Hash
+		)
 
-	// Lastly, attempt to build on top of the invalid payload
-	t.CLMock.produceSingleBlock(BlockProcessCallbacks{
-		// Run test after the new payload has been obtained
-		OnGetPayload: func() {
-			alteredPayload, err := customizePayload(&t.CLMock.LatestPayloadBuilt, &CustomPayloadData{
-				ParentHash: &invalidPayloadHash,
-			})
-			if err != nil {
-				t.Fatalf("FAIL (%s): Unable to modify payload: %v", t.TestName, err)
-			}
-			// Response status can be ACCEPTED (since parent payload could have been thrown out by the client)
-			// or INVALID (client still has the payload and can verify that this payload is incorrectly building on top of it),
-			// but a VALID response is incorrect.
-			r := t.TestEngine.TestEngineNewPayloadV1(alteredPayload)
-			r.ExpectStatusEither(Accepted, Invalid)
-		},
-	})
+		t.CLMock.produceSingleBlock(BlockProcessCallbacks{
+			// Run test after the new payload has been obtained
+			OnGetPayload: func() {
+				// Alter hash on the payload and send it to client, should produce an error
+				alteredPayload = t.CLMock.LatestPayloadBuilt
+				invalidPayloadHash = alteredPayload.BlockHash
+				invalidPayloadHash[common.HashLength-1] = byte(255 - invalidPayloadHash[common.HashLength-1])
+				alteredPayload.BlockHash = invalidPayloadHash
+
+				if !syncing && sidechain {
+					// We alter the payload by setting the parent to a known past block in the
+					// canonical chain, which makes this payload a side chain payload, and also an invalid block hash
+					// (because we did not update the block hash appropriately)
+					alteredPayload.ParentHash = t.CLMock.LatestFinalizedHeader.ParentHash
+				} else if syncing {
+					// We need to send an fcU to put the client in SYNCING state.
+					randomHeadBlock := common.Hash{}
+					rand.Read(randomHeadBlock[:])
+					fcU := ForkchoiceStateV1{
+						HeadBlockHash:      randomHeadBlock,
+						SafeBlockHash:      t.CLMock.LatestFinalizedHeader.Hash(),
+						FinalizedBlockHash: t.CLMock.LatestFinalizedHeader.Hash(),
+					}
+					r := t.TestEngine.TestEngineForkchoiceUpdatedV1(&fcU, nil)
+					r.ExpectPayloadStatus(Syncing)
+
+					if sidechain {
+						// Syncing and sidechain, the caonincal head is an unknown payload to us,
+						// but this specific bad hash payload is in theory part of a side chain.
+						// Therefore the parent we use is the head hash.
+						alteredPayload.ParentHash = t.CLMock.LatestFinalizedHeader.Hash()
+					} else {
+						// The invalid bad-hash payload points to the unknown head, but we know it is
+						// indeed canonical because the head was set using forkchoiceUpdated.
+						alteredPayload.ParentHash = randomHeadBlock
+					}
+				}
 
+				// Execution specification::
+				// - {status: INVALID_BLOCK_HASH, latestValidHash: null, validationError: null} if the blockHash validation has failed
+				r := t.TestEngine.TestEngineNewPayloadV1(&alteredPayload)
+				r.ExpectStatus(InvalidBlockHash)
+			},
+		})
+
+		// Lastly, attempt to build on top of the invalid payload
+		t.CLMock.produceSingleBlock(BlockProcessCallbacks{
+			// Run test after the new payload has been obtained
+			OnGetPayload: func() {
+				alteredPayload, err := customizePayload(&t.CLMock.LatestPayloadBuilt, &CustomPayloadData{
+					ParentHash: &invalidPayloadHash,
+				})
+				if err != nil {
+					t.Fatalf("FAIL (%s): Unable to modify payload: %v", t.TestName, err)
+				}
+
+				// Response status can be ACCEPTED (since parent payload could have been thrown out by the client)
+				// or INVALID (client still has the payload and can verify that this payload is incorrectly building on top of it),
+				// but a VALID response is incorrect.
+				r := t.TestEngine.TestEngineNewPayloadV1(alteredPayload)
+				r.ExpectStatusEither(Accepted, Invalid)
+
+			},
+		})
+
+	}
 }
 
 // Copy the parentHash into the blockHash, client should reject the payload