From c265fe4440fad4016c00c553cb2bd037530a0d2d Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Mon, 28 Aug 2023 10:29:45 +1000 Subject: [PATCH] EngineNewPayloadV2 - Reject non-null Blob fields (#5802) * Validate parameters pre cancun Signed-off-by: Gabriel-Trintinalia * Add unit tests Signed-off-by: Gabriel-Trintinalia * Fix unit test Signed-off-by: Gabriel-Trintinalia * Fix unit tests Signed-off-by: Gabriel-Trintinalia * Simplify Tests Signed-off-by: Gabriel-Trintinalia * Update ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java Co-authored-by: Sally MacFarlane Signed-off-by: Gabriel-Trintinalia * Update ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java Co-authored-by: Sally MacFarlane Signed-off-by: Gabriel-Trintinalia * Update ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java Co-authored-by: Sally MacFarlane Signed-off-by: Gabriel-Trintinalia * Update ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java Co-authored-by: Sally MacFarlane Signed-off-by: Gabriel-Trintinalia --------- Signed-off-by: Gabriel-Trintinalia Co-authored-by: Sally MacFarlane Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> --- .../methods/engine/EngineNewPayloadV2.java | 17 ++++++++ .../engine/AbstractEngineNewPayloadTest.java | 25 +++++++----- .../engine/EngineNewPayloadV2Test.java | 32 +++++++++++++++ .../engine/EngineNewPayloadV3Test.java | 40 +++++++++++++++++++ 4 files changed, 103 insertions(+), 11 deletions(-) diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java index c2a69349235..65133718ada 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java +++ b/ethereum/api/src/main/java/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 validateParameters( + final EnginePayloadParameter payloadParameter, + final Optional> maybeVersionedHashParam, + final Optional 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 validateBlobs( final List transactions, diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java index d1aa7e3dc1e..a545883a6c8 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java @@ -468,19 +468,22 @@ protected JsonRpcError fromErrorResp(final JsonRpcResponse resp) { protected BlockHeader createBlockHeader( final Optional> maybeWithdrawals, final Optional> maybeDeposits) { + return createBlockHeaderFixture(maybeWithdrawals, maybeDeposits).buildHeader(); + } + + protected BlockHeaderTestFixture createBlockHeaderFixture( + final Optional> maybeWithdrawals, + final Optional> 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/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java index dc7136052a9..f7f2cba533f 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2Test.java +++ b/ethereum/api/src/test/java/org/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 withdrawals = null; diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3Test.java index b4f4cc1765c..56cecbfb7db 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3Test.java +++ b/ethereum/api/src/test/java/org/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(); + } }