Revert "EngineNewPayload - respond with error if params invalid (besu-eth#8729)" (besu-eth#9108)

This reverts commit 7a3ca6b.

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Author: Gabriel-Trintinalia

CHANGELOG.md

@@ -65,7 +65,6 @@
 
 ### Bug fixes
 - Fix bug with `eth_estimateGas` on QBFT - use zero address when doing simulation against `pending` block [#9031](https://github.com/hyperledger/besu/pull/9031)
-- Fix bug with handling of invalid requests for `engine_newPayloadV4` [#8729](https://github.com/hyperledger/besu/pull/8729)
 
 ## 25.7.0
 ### Breaking Changes

acceptance-tests/tests/src/test/resources/jsonrpc/engine/prague/test-cases/17_prague_newPayloadV4_invalid_duplicate_requests.json

@@ -173,7 +173,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
     try {
       maybeVersionedHashes = extractVersionedHashes(maybeVersionedHashParam);
     } catch (RuntimeException ex) {
-      return respondWithError(
+      return respondWithInvalid(
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
@@ -203,7 +203,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
     try {
       maybeRequests = extractRequests(maybeRequestsParam);
     } catch (RequestType.InvalidRequestTypeException ex) {
-      return respondWithError(
+      return respondWithInvalid(
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
@@ -285,15 +285,6 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
               "Computed block hash %s does not match block hash parameter %s",
               newBlockHeader.getBlockHash(), blockParam.getBlockHash());
       LOG.debug(errorMessage);
-      // If the block is invalid, we want to return INVALID
-      // from the spec:
-      // https://github.com/ethereum/execution-apis/blob/main/src/engine/prague.md#engine_newpayloadv4
-      // Given the executionRequests, client software MUST compute the execution requests commitment
-      // and incorporate it into the blockHash validation process.
-      // That is, if the computed commitment does not match the corresponding commitment in the
-      // execution layer block header,
-      // the call MUST return {status: INVALID, latestValidHash: null, validationError: errorMessage
-      // | null}.
       return respondWithInvalid(reqId, blockParam, null, getInvalidBlockHashStatus(), errorMessage);
     }
 
@@ -308,7 +299,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
             maybeVersionedHashes,
             protocolSchedule.get().getByBlockHeader(newBlockHeader));
     if (!blobValidationResult.isValid()) {
-      return respondWithError(
+      return respondWithInvalid(
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
@@ -335,7 +326,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
     if (maybeParentHeader.isPresent()
         && (Long.compareUnsigned(maybeParentHeader.get().getTimestamp(), blockParam.getTimestamp())
             >= 0)) {
-      return respondWithError(
+      return respondWithInvalid(
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
@@ -382,7 +373,8 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
         Throwable causedBy = executionResult.causedBy().get();
         if (causedBy instanceof StorageException || causedBy instanceof MerkleTrieException) {
           RpcErrorType error = RpcErrorType.INTERNAL_ERROR;
-          return new JsonRpcErrorResponse(reqId, error);
+          JsonRpcErrorResponse response = new JsonRpcErrorResponse(reqId, error);
+          return response;
         }
       }
       LOG.debug("New payload is invalid: {}", executionResult.errorMessage.get());
@@ -464,7 +456,7 @@ JsonRpcResponse respondWith(
   JsonRpcResponse respondWithInvalid(
       final Object requestId,
       final EnginePayloadParameter param,
-      final Hash latestValidHash, // this should be null for invalid
+      final Hash latestValidHash,
       final EngineStatus invalidStatus,
       final String validationError) {
     if (!INVALID.equals(invalidStatus) && !INVALID_BLOCK_HASH.equals(invalidStatus)) {
@@ -493,36 +485,6 @@ JsonRpcResponse respondWithInvalid(
             invalidStatus, latestValidHash, Optional.of(validationError)));
   }
 
-  JsonRpcResponse respondWithError(
-      final Object requestId,
-      final EnginePayloadParameter param,
-      final Hash latestValidHash,
-      final EngineStatus invalidStatus,
-      final String validationError) {
-    if (!INVALID.equals(invalidStatus) && !INVALID_BLOCK_HASH.equals(invalidStatus)) {
-      throw new IllegalArgumentException(
-          "Don't call respondWithError() with non-invalid status of " + invalidStatus.toString());
-    }
-    final String invalidBlockLogMessage =
-        String.format(
-            "Invalid new payload: number: %s, hash: %s, parentHash: %s, latestValidHash: %s, status: %s, validationError: %s",
-            param.getBlockNumber(),
-            param.getBlockHash(),
-            param.getParentHash(),
-            latestValidHash == null ? null : latestValidHash.toHexString(),
-            invalidStatus.name(),
-            validationError);
-    // always log invalid at DEBUG
-    LOG.debug(invalidBlockLogMessage);
-    // periodically log at WARN
-    if (lastInvalidWarn + ENGINE_API_LOGGING_THRESHOLD < System.currentTimeMillis()) {
-      lastInvalidWarn = System.currentTimeMillis();
-      LOG.warn(invalidBlockLogMessage);
-    }
-    // return ErrorResponse
-    return new JsonRpcErrorResponse(requestId, RpcErrorType.INVALID_ENGINE_PARAMS);
-  }
-
   protected EngineStatus getInvalidBlockHashStatus() {
     return INVALID;
   }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java

@@ -61,7 +61,6 @@ public enum RpcErrorType implements RpcMethodError {
   INVALID_EXCESS_BLOB_GAS_PARAMS(
       INVALID_PARAMS_ERROR_CODE, "Invalid excess blob gas params (missing or invalid)"),
   INVALID_EXECUTION_REQUESTS_PARAMS(INVALID_PARAMS_ERROR_CODE, "Invalid execution requests params"),
-  INVALID_ENGINE_PARAMS(INVALID_PARAMS_ERROR_CODE, "Invalid engine params"),
   INVALID_EXTRA_DATA_PARAMS(INVALID_PARAMS_ERROR_CODE, "Invalid extra data params"),
   INVALID_FILTER_PARAMS(INVALID_PARAMS_ERROR_CODE, "Invalid filter params"),
   INVALID_HASH_RATE_PARAMS(INVALID_PARAMS_ERROR_CODE, "Invalid hash rate params"),

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/RpcErrorType.java

@@ -16,10 +16,9 @@
 
 import static java.util.Collections.emptyList;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.hyperledger.besu.ethereum.api.graphql.internal.response.GraphQLError.INVALID_PARAMS;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine.EngineTestSupport.fromErrorResp;
-import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.INVALID_ENGINE_PARAMS;
+import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.INVALID_PARAMS;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.UNSUPPORTED_FORK;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
@@ -176,12 +175,9 @@ public void shouldInvalidVersionedHash_whenShortVersionedHash() {
                       List.of(shortHash.toHexString()),
                       "0x0000000000000000000000000000000000000000000000000000000000000000"
                     })));
-
-    // Should return an error response with INVALID_EXECUTION_REQUESTS_PARAMS
-    final JsonRpcError jsonRpcError = fromErrorResp(badParam);
-    assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode());
-    assertThat(jsonRpcError.getMessage()).isEqualTo(INVALID_ENGINE_PARAMS.getMessage());
-    verify(engineCallListener, times(1)).executionEngineCalled();
+    final EnginePayloadStatusResult res = fromSuccessResp(badParam);
+    assertThat(res.getStatusAsString()).isEqualTo(INVALID.name());
+    assertThat(res.getError()).isEqualTo("Invalid versionedHash");
   }
 
   @Test
@@ -230,16 +226,19 @@ protected BlockHeader createBlockHeader(final Optional<List<Withdrawal>> maybeWi
     when(blockchain.getBlockHeader(parentBlockHeader.getBlockHash()))
         .thenReturn(Optional.of(parentBlockHeader));
     when(protocolContext.getBlockchain()).thenReturn(blockchain);
-    return new BlockHeaderTestFixture()
-        .baseFeePerGas(Wei.ONE)
-        .parentHash(parentBlockHeader.getParentHash())
-        .number(parentBlockHeader.getNumber() + 1)
-        .timestamp(parentBlockHeader.getTimestamp() + 12)
-        .withdrawalsRoot(maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null))
-        .excessBlobGas(BlobGas.ZERO)
-        .blobGasUsed(0L)
-        .parentBeaconBlockRoot(maybeParentBeaconBlockRoot)
-        .buildHeader();
+    BlockHeader mockHeader =
+        new BlockHeaderTestFixture()
+            .baseFeePerGas(Wei.ONE)
+            .parentHash(parentBlockHeader.getParentHash())
+            .number(parentBlockHeader.getNumber() + 1)
+            .timestamp(parentBlockHeader.getTimestamp() + 12)
+            .withdrawalsRoot(maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null))
+            .excessBlobGas(BlobGas.ZERO)
+            .blobGasUsed(0L)
+            .parentBeaconBlockRoot(
+                maybeParentBeaconBlockRoot.isPresent() ? maybeParentBeaconBlockRoot : null)
+            .buildHeader();
+    return mockHeader;
   }
 
   @Override

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3Test.java

@@ -78,11 +78,6 @@ protected Set<ScheduledProtocolSpec.Hardfork> supportedHardforks() {
           new Request(RequestType.WITHDRAWAL, Bytes.of(1)),
           new Request(RequestType.CONSOLIDATION, Bytes.of(1)));
 
-  private static final List<Request> INVALID_REQUESTS_DUPLICATES =
-      List.of(
-          new Request(RequestType.CONSOLIDATION, Bytes.of(1)),
-          new Request(RequestType.CONSOLIDATION, Bytes.of(1)));
-
   @BeforeEach
   @Override
   public void before() {
@@ -135,27 +130,6 @@ public void shouldReturnInvalidIfRequestsIsNull_WhenRequestsAllowed() {
     verify(engineCallListener, times(1)).executionEngineCalled();
   }
 
-  @Test
-  public void shouldReturnInvalidIfRequestsInvalidWithDuplicates() {
-    BlockHeader mockHeader =
-        setupValidPayload(
-            new BlockProcessingResult(
-                Optional.of(
-                    new BlockProcessingOutputs(
-                        null, List.of(), Optional.of(INVALID_REQUESTS_DUPLICATES)))),
-            Optional.empty());
-    when(blockchain.getBlockHeader(mockHeader.getParentHash()))
-        .thenReturn(Optional.of(mock(BlockHeader.class)));
-    when(mergeCoordinator.getLatestValidAncestor(mockHeader))
-        .thenReturn(Optional.of(mockHeader.getHash()));
-    var resp = respWithInvalidDuplicateRequests(mockEnginePayload(mockHeader, emptyList()));
-
-    assertThat(fromErrorResp(resp).getCode()).isEqualTo(INVALID_PARAMS.getCode());
-    assertThat(fromErrorResp(resp).getMessage())
-        .isEqualTo(INVALID_EXECUTION_REQUESTS_PARAMS.getMessage());
-    verify(engineCallListener, times(1)).executionEngineCalled();
-  }
-
   @Test
   public void shouldReturnValidIfRequestsIsNotNull_WhenRequestsAllowed() {
     BlockHeader mockHeader =
@@ -290,26 +264,6 @@ protected JsonRpcResponse respWithInvalidRequests(final EnginePayloadParameter p
         new JsonRpcRequestContext(new JsonRpcRequest("2.0", this.method.getName(), params)));
   }
 
-  protected JsonRpcResponse respWithInvalidDuplicateRequests(final EnginePayloadParameter payload) {
-    final List<String> requestsWithoutRequestId =
-        INVALID_REQUESTS_DUPLICATES.stream() // don't sort them
-            .map(
-                r ->
-                    Bytes.concatenate(Bytes.of(r.getType().getSerializedType()), r.getData())
-                        .toHexString())
-            .toList();
-    Object[] params =
-        maybeParentBeaconBlockRoot
-            .map(
-                bytes32 ->
-                    new Object[] {
-                      payload, emptyList(), bytes32.toHexString(), requestsWithoutRequestId
-                    })
-            .orElseGet(() -> new Object[] {payload});
-    return method.response(
-        new JsonRpcRequestContext(new JsonRpcRequest("2.0", this.method.getName(), params)));
-  }
-
   private void mockProhibitedRequestsValidator() {
     var validator = new ProhibitedRequestValidator();
     when(protocolSpec.getRequestsValidator()).thenReturn(validator);