EngineNewPayloadV2 - Reject non-null Blob fields (hyperledger#5802)

* Validate parameters pre cancun Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Add unit tests Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Fix unit test Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Fix unit tests Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Simplify Tests Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Update ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Update ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Update ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> * Update ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> --------- Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> Signed-off-by: garyschulte <garyschulte@gmail.com>
garyschulte · Aug 28, 2023 · cb639c7 · cb639c7
1 parent 9d57ca0
commit cb639c7
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 11 deletions.
diff --git a/...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java b/...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java
@@ -18,6 +18,7 @@
 import org.hyperledger.besu.datatypes.VersionedHash;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadParameter;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.Transaction;
@@ -48,6 +49,22 @@ public String getName() {
     return RpcMethod.ENGINE_NEW_PAYLOAD_V2.getMethodName();
   }
 
+  @Override
+  protected ValidationResult<RpcErrorType> validateParameters(
+      final EnginePayloadParameter payloadParameter,
+      final Optional<List<String>> maybeVersionedHashParam,
+      final Optional<String> maybeBeaconBlockRootParam) {
+    if (payloadParameter.getBlobGasUsed() != null) {
+      return ValidationResult.invalid(
+          RpcErrorType.INVALID_PARAMS, "non-null BlobGasUsed pre-cancun");
+    }
+    if (payloadParameter.getExcessBlobGas() != null) {
+      return ValidationResult.invalid(
+          RpcErrorType.INVALID_PARAMS, "non-null ExcessBlobGas pre-cancun");
+    }
+    return ValidationResult.valid();
+  }
+
   @Override
   protected ValidationResult<RpcErrorType> validateBlobs(
       final List<Transaction> transactions,

diff --git a/...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java b/...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java
@@ -468,19 +468,22 @@ protected JsonRpcError fromErrorResp(final JsonRpcResponse resp) {
   protected BlockHeader createBlockHeader(
       final Optional<List<Withdrawal>> maybeWithdrawals,
       final Optional<List<Deposit>> maybeDeposits) {
+    return createBlockHeaderFixture(maybeWithdrawals, maybeDeposits).buildHeader();
+  }
+
+  protected BlockHeaderTestFixture createBlockHeaderFixture(
+      final Optional<List<Withdrawal>> maybeWithdrawals,
+      final Optional<List<Deposit>> maybeDeposits) {
     BlockHeader parentBlockHeader =
         new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
-    BlockHeader mockHeader =
-        new BlockHeaderTestFixture()
-            .baseFeePerGas(Wei.ONE)
-            .parentHash(parentBlockHeader.getParentHash())
-            .number(parentBlockHeader.getNumber() + 1)
-            .timestamp(parentBlockHeader.getTimestamp() + 1)
-            .withdrawalsRoot(maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null))
-            .depositsRoot(maybeDeposits.map(BodyValidation::depositsRoot).orElse(null))
-            .parentBeaconBlockRoot(maybeParentBeaconBlockRoot)
-            .buildHeader();
-    return mockHeader;
+    return new BlockHeaderTestFixture()
+        .baseFeePerGas(Wei.ONE)
+        .parentHash(parentBlockHeader.getParentHash())
+        .number(parentBlockHeader.getNumber() + 1)
+        .timestamp(parentBlockHeader.getTimestamp() + 1)
+        .withdrawalsRoot(maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null))
+        .depositsRoot(maybeDeposits.map(BodyValidation::depositsRoot).orElse(null))
+        .parentBeaconBlockRoot(maybeParentBeaconBlockRoot);
   }
 
   protected void assertValidResponse(final BlockHeader mockHeader, final JsonRpcResponse resp) {

diff --git a/...hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java b/...hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java
@@ -24,6 +24,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import org.hyperledger.besu.datatypes.BlobGas;
 import org.hyperledger.besu.ethereum.BlockProcessingOutputs;
 import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod;
@@ -127,6 +128,37 @@ public void shouldReturnInvalidIfWithdrawalsIsNotNull_WhenWithdrawalsProhibited(
     verify(engineCallListener, times(1)).executionEngineCalled();
   }
 
+  @Test
+  public void shouldValidateBlobGasUsedCorrectly() {
+    // V2 should return error if non-null blobGasUsed
+    BlockHeader blockHeader =
+        createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty())
+            .blobGasUsed(100L)
+            .buildHeader();
+
+    var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of(), null));
+    final JsonRpcError jsonRpcError = fromErrorResp(resp);
+    assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode());
+    assertThat(jsonRpcError.getData()).isEqualTo("non-null BlobGasUsed pre-cancun");
+    verify(engineCallListener, times(1)).executionEngineCalled();
+  }
+
+  @Test
+  public void shouldValidateExcessBlobGasCorrectly() {
+    // V2 should return error if non-null ExcessBlobGas
+    BlockHeader blockHeader =
+        createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty())
+            .excessBlobGas(BlobGas.MAX_BLOB_GAS)
+            .buildHeader();
+
+    var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of(), null));
+
+    final JsonRpcError jsonRpcError = fromErrorResp(resp);
+    assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode());
+    assertThat(jsonRpcError.getData()).isEqualTo("non-null ExcessBlobGas pre-cancun");
+    verify(engineCallListener, times(1)).executionEngineCalled();
+  }
+
   @Test
   public void shouldReturnInvalidIfWithdrawalsIsNull_WhenWithdrawalsAllowed() {
     final List<WithdrawalParameter> withdrawals = null;

diff --git a/...hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3Test.java b/...hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3Test.java
@@ -16,8 +16,11 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID;
+import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.INVALID_PARAMS;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.datatypes.BlobGas;
@@ -28,6 +31,7 @@
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadParameter;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.EnginePayloadStatusResult;
@@ -154,4 +158,40 @@ protected BlockHeader createBlockHeader(
   public void shouldReturnValidIfProtocolScheduleIsEmpty() {
     // no longer the case, blob validation requires a protocol schedule
   }
+
+  @Test
+  @Override
+  public void shouldValidateBlobGasUsedCorrectly() {
+    // V3 must return error if null blobGasUsed
+    BlockHeader blockHeader =
+        createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty())
+            .excessBlobGas(BlobGas.MAX_BLOB_GAS)
+            .blobGasUsed(null)
+            .buildHeader();
+
+    var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of(), null));
+
+    final JsonRpcError jsonRpcError = fromErrorResp(resp);
+    assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode());
+    assertThat(jsonRpcError.getData()).isEqualTo("Missing blob gas fields");
+    verify(engineCallListener, times(1)).executionEngineCalled();
+  }
+
+  @Test
+  @Override
+  public void shouldValidateExcessBlobGasCorrectly() {
+    // V3 must return error if null excessBlobGas
+    BlockHeader blockHeader =
+        createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty())
+            .excessBlobGas(null)
+            .blobGasUsed(100L)
+            .buildHeader();
+
+    var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of(), null));
+
+    final JsonRpcError jsonRpcError = fromErrorResp(resp);
+    assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode());
+    assertThat(jsonRpcError.getData()).isEqualTo("Missing blob gas fields");
+    verify(engineCallListener, times(1)).executionEngineCalled();
+  }
 }