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 00b0f8305..8d6bd7e43 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 @@ -61,11 +61,11 @@ public class EngineNewPayloadV2 extends AbstractEngineNewPayload { final Optional> maybeRequestsParam) { if (payloadParameter.getBlobGasUsed() != null) { return ValidationResult.invalid( - RpcErrorType.INVALID_BLOB_GAS_USED_PARAMS, "Missing blob gas used field"); + RpcErrorType.INVALID_BLOB_GAS_USED_PARAMS, "Unexpected blob gas used field present"); } if (payloadParameter.getExcessBlobGas() != null) { return ValidationResult.invalid( - RpcErrorType.INVALID_EXCESS_BLOB_GAS_PARAMS, "Missing excess blob gas field"); + RpcErrorType.INVALID_EXCESS_BLOB_GAS_PARAMS, "Unexpected excess blob gas field present"); } return ValidationResult.valid(); } 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 e793ee8b5..0b81ac111 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 @@ -14,6 +14,7 @@ */ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; +import static java.util.Collections.emptyList; import static org.assertj.core.api.Assertions.assertThat; import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.ACCEPTED; import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID; @@ -51,7 +52,6 @@ import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockBody; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; -import org.hyperledger.besu.ethereum.core.Request; import org.hyperledger.besu.ethereum.core.Withdrawal; import org.hyperledger.besu.ethereum.eth.manager.EthPeers; import org.hyperledger.besu.ethereum.mainnet.BodyValidation; @@ -62,7 +62,6 @@ import org.hyperledger.besu.ethereum.trie.MerkleTrieException; import org.hyperledger.besu.plugin.services.exception.StorageException; import org.hyperledger.besu.plugin.services.rpc.RpcResponseType; -import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; @@ -123,12 +122,11 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))), - Optional.empty(), Optional.empty()); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); assertValidResponse(mockHeader, resp); } @@ -136,12 +134,11 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldReturnInvalidOnBlockExecutionError() { BlockHeader mockHeader = - setupValidPayload( - new BlockProcessingResult("error 42"), Optional.empty(), Optional.empty()); + setupValidPayload(new BlockProcessingResult("error 42"), Optional.empty()); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash().get()).isEqualTo(mockHash); @@ -152,14 +149,14 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldReturnAcceptedOnLatestValidAncestorEmpty() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.empty()); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); @@ -170,20 +167,19 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldReturnSuccessOnAlreadyPresent() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); - Block mockBlock = - new Block(mockHeader, new BlockBody(Collections.emptyList(), Collections.emptyList())); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); + Block mockBlock = new Block(mockHeader, new BlockBody(emptyList(), emptyList())); when(blockchain.getBlockByHash(any())).thenReturn(Optional.of(mockBlock)); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); assertValidResponse(mockHeader, resp); } @Test public void shouldReturnInvalidWithLatestValidHashIsABadBlock() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); Hash latestValidHash = Hash.hash(Bytes32.fromHexStringLenient("0xcafebabe")); when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); @@ -191,7 +187,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT when(mergeCoordinator.getLatestValidHashOfBadBlock(mockHeader.getHash())) .thenReturn(Optional.of(latestValidHash)); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEqualTo(Optional.of(latestValidHash)); @@ -204,12 +200,11 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.empty(), new StorageException("database bedlam")), - Optional.empty(), Optional.empty()); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); fromErrorResp(resp); verify(engineCallListener, times(1)).executionEngineCalled(); @@ -220,13 +215,12 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.empty(), new MerkleTrieException("missing leaf")), - Optional.empty(), Optional.empty()); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); verify(engineCallListener, times(1)).executionEngineCalled(); @@ -235,7 +229,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldNotReturnInvalidOnThrownMerkleTrieException() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); @@ -243,7 +237,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT .thenReturn(Optional.of(mockHash)); when(mergeCoordinator.rememberBlock(any())).thenThrow(new MerkleTrieException("missing leaf")); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); verify(engineCallListener, times(1)).executionEngineCalled(); @@ -252,7 +246,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldReturnInvalidBlockHashOnBadHashParameter() { - BlockHeader mockHeader = spy(createBlockHeader(Optional.empty(), Optional.empty())); + BlockHeader mockHeader = spy(createBlockHeader(Optional.empty())); lenient() .when(mergeCoordinator.getLatestValidAncestor(mockHeader.getBlockHash())) .thenReturn(Optional.empty()); @@ -260,7 +254,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); lenient().when(mockHeader.getHash()).thenReturn(Hash.fromHexStringLenient("0x1337")); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getStatusAsString()).isEqualTo(getExpectedInvalidBlockHashStatus().name()); @@ -269,11 +263,11 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldCheckBlockValidityBeforeCheckingByHashForExisting() { - BlockHeader realHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader realHeader = createBlockHeader(Optional.empty()); BlockHeader paramHeader = spy(realHeader); when(paramHeader.getHash()).thenReturn(Hash.fromHexStringLenient("0x1337")); - var resp = resp(mockEnginePayload(paramHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(paramHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); @@ -283,7 +277,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldReturnInvalidOnMalformedTransactions() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); when(mergeCoordinator.getLatestValidAncestor(any(Hash.class))) .thenReturn(Optional.of(mockHash)); @@ -298,9 +292,9 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldRespondWithSyncingDuringForwardSync() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); when(mergeContext.isSyncing()).thenReturn(Boolean.TRUE); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getError()).isNull(); @@ -311,10 +305,10 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldRespondWithSyncingDuringBackwardsSync() { - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); when(mergeCoordinator.appendNewPayloadToSync(any())) .thenReturn(CompletableFuture.completedFuture(null)); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); @@ -325,12 +319,12 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldRespondWithInvalidIfExtraDataIsNull() { - BlockHeader realHeader = createBlockHeader(Optional.empty(), Optional.empty()); + BlockHeader realHeader = createBlockHeader(Optional.empty()); BlockHeader paramHeader = spy(realHeader); when(paramHeader.getHash()).thenReturn(Hash.fromHexStringLenient("0x1337")); when(paramHeader.getExtraData().toHexString()).thenReturn(null); - var resp = resp(mockEnginePayload(paramHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(paramHeader, emptyList())); EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); @@ -342,8 +336,8 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT @Test public void shouldReturnInvalidWhenBadBlock() { when(mergeCoordinator.isBadBlock(any(Hash.class))).thenReturn(true); - BlockHeader mockHeader = createBlockHeader(Optional.empty(), Optional.empty()); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + BlockHeader mockHeader = createBlockHeader(Optional.empty()); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); when(protocolSpec.getWithdrawalsValidator()) .thenReturn(new WithdrawalsValidator.AllowedWithdrawals()); EnginePayloadStatusResult res = fromSuccessResp(resp); @@ -359,12 +353,11 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))), - Optional.empty(), Optional.empty()); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); assertValidResponse(mockHeader, resp); } @@ -408,11 +401,9 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT } protected BlockHeader setupValidPayload( - final BlockProcessingResult value, - final Optional> maybeWithdrawals, - final Optional> maybeRequests) { + final BlockProcessingResult value, final Optional> maybeWithdrawals) { - BlockHeader mockHeader = createBlockHeader(maybeWithdrawals, maybeRequests); + BlockHeader mockHeader = createBlockHeader(maybeWithdrawals); when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); @@ -425,6 +416,11 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT } protected EnginePayloadStatusResult fromSuccessResp(final JsonRpcResponse resp) { + if (resp.getType().equals(RpcResponseType.ERROR)) { + final JsonRpcError jsonRpcError = fromErrorResp(resp); + throw new AssertionError( + "Expected success but was error with message: " + jsonRpcError.getMessage()); + } assertThat(resp.getType()).isEqualTo(RpcResponseType.SUCCESS); return Optional.of(resp) .map(JsonRpcSuccessResponse.class::cast) @@ -441,15 +437,12 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT .get(); } - protected BlockHeader createBlockHeader( - final Optional> maybeWithdrawals, - final Optional> maybeRequests) { - return createBlockHeaderFixture(maybeWithdrawals, maybeRequests).buildHeader(); + protected BlockHeader createBlockHeader(final Optional> maybeWithdrawals) { + return createBlockHeaderFixture(maybeWithdrawals).buildHeader(); } protected BlockHeaderTestFixture createBlockHeaderFixture( - final Optional> maybeWithdrawals, - final Optional> maybeRequests) { + final Optional> maybeWithdrawals) { BlockHeader parentBlockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader(); return new BlockHeaderTestFixture() @@ -458,8 +451,7 @@ public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiT .number(parentBlockHeader.getNumber() + 1) .timestamp(parentBlockHeader.getTimestamp() + 1) .withdrawalsRoot(maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null)) - .parentBeaconBlockRoot(maybeParentBeaconBlockRoot) - .requestsHash(maybeRequests.map(BodyValidation::requestsHash).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 abf2c2976..171ee0499 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 @@ -82,8 +82,7 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))), - Optional.of(withdrawals), - Optional.empty()); + Optional.of(withdrawals)); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); @@ -100,7 +99,6 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))), - Optional.empty(), Optional.empty()); lenient() .when(blockchain.getBlockHeader(mockHeader.getParentHash())) @@ -120,7 +118,7 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { var resp = resp( mockEnginePayload( - createBlockHeader(Optional.of(Collections.emptyList()), Optional.empty()), + createBlockHeader(Optional.of(Collections.emptyList())), Collections.emptyList(), withdrawals)); @@ -133,14 +131,14 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { public void shouldValidateBlobGasUsedCorrectly() { // V2 should return error if non-null blobGasUsed BlockHeader blockHeader = - createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty()) + createBlockHeaderFixture(Optional.of(Collections.emptyList())) .blobGasUsed(100L) .buildHeader(); var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of())); final JsonRpcError jsonRpcError = fromErrorResp(resp); assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_BLOB_GAS_USED_PARAMS.getCode()); - assertThat(jsonRpcError.getData()).isEqualTo("Missing blob gas used field"); + assertThat(jsonRpcError.getData()).isEqualTo("Unexpected blob gas used field present"); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -148,7 +146,7 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { public void shouldValidateExcessBlobGasCorrectly() { // V2 should return error if non-null ExcessBlobGas BlockHeader blockHeader = - createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty()) + createBlockHeaderFixture(Optional.of(Collections.emptyList())) .excessBlobGas(BlobGas.MAX_BLOB_GAS) .buildHeader(); @@ -156,7 +154,7 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { final JsonRpcError jsonRpcError = fromErrorResp(resp); assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode()); - assertThat(jsonRpcError.getData()).isEqualTo("Missing excess blob gas field"); + assertThat(jsonRpcError.getData()).isEqualTo("Unexpected excess blob gas field present"); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -169,9 +167,7 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { var resp = resp( mockEnginePayload( - createBlockHeader(Optional.empty(), Optional.empty()), - Collections.emptyList(), - withdrawals)); + createBlockHeader(Optional.empty()), Collections.emptyList(), withdrawals)); assertThat(fromErrorResp(resp).getCode()).isEqualTo(INVALID_PARAMS.getCode()); verify(engineCallListener, times(1)).executionEngineCalled(); @@ -182,7 +178,7 @@ public class EngineNewPayloadV2Test extends AbstractEngineNewPayloadTest { // Cancun starte at timestamp 30 final long blockTimestamp = 31L; BlockHeader blockHeader = - createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty()) + createBlockHeaderFixture(Optional.of(Collections.emptyList())) .timestamp(blockTimestamp) .buildHeader(); 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 b79cb587e..1810ce945 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 @@ -14,6 +14,7 @@ */ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; +import static java.util.Collections.emptyList; 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; @@ -44,7 +45,6 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.EnginePayloadS import org.hyperledger.besu.ethereum.core.BlobTestFixture; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; -import org.hyperledger.besu.ethereum.core.Request; import org.hyperledger.besu.ethereum.core.Transaction; import org.hyperledger.besu.ethereum.core.TransactionTestFixture; import org.hyperledger.besu.ethereum.core.Withdrawal; @@ -56,7 +56,6 @@ import org.hyperledger.besu.ethereum.mainnet.ValidationResult; import org.hyperledger.besu.evm.gascalculator.CancunGasCalculator; import java.math.BigInteger; -import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.function.Supplier; @@ -112,8 +111,18 @@ public class EngineNewPayloadV3Test extends EngineNewPayloadV2Test { when(payload.getExcessBlobGas()).thenReturn("99"); when(payload.getBlobGasUsed()).thenReturn(9l); + // TODO locking this as V3 otherwise this breaks the EngineNewPayloadV4Test subclass when method + // field is V4 + final EngineNewPayloadV3 methodV3 = + new EngineNewPayloadV3( + vertx, + protocolSchedule, + protocolContext, + mergeCoordinator, + ethPeers, + engineCallListener); final JsonRpcResponse badParam = - method.response( + methodV3.response( new JsonRpcRequestContext( new JsonRpcRequest( "2.0", @@ -133,24 +142,20 @@ public class EngineNewPayloadV3Test extends EngineNewPayloadV2Test { final BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))), - Optional.empty(), Optional.empty()); - final EnginePayloadParameter payload = - mockEnginePayload(mockHeader, Collections.emptyList(), null); + final EnginePayloadParameter payload = mockEnginePayload(mockHeader, emptyList(), null); ValidationResult res = method.validateParameters( payload, Optional.of(List.of()), Optional.of("0x0000000000000000000000000000000000000000000000000000000000000000"), - Optional.empty()); + Optional.of(emptyList())); assertThat(res.isValid()).isTrue(); } @Override - protected BlockHeader createBlockHeader( - final Optional> maybeWithdrawals, - final Optional> maybeRequests) { + protected BlockHeader createBlockHeader(final Optional> maybeWithdrawals) { BlockHeader parentBlockHeader = new BlockHeaderTestFixture() .baseFeePerGas(Wei.ONE) @@ -186,12 +191,12 @@ public class EngineNewPayloadV3Test extends EngineNewPayloadV2Test { public void shouldValidateBlobGasUsedCorrectly() { // V3 must return error if null blobGasUsed BlockHeader blockHeader = - createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty()) + createBlockHeaderFixture(Optional.of(emptyList())) .excessBlobGas(BlobGas.MAX_BLOB_GAS) .blobGasUsed(null) .buildHeader(); - var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of())); + var resp = resp(mockEnginePayload(blockHeader, emptyList(), List.of())); final JsonRpcError jsonRpcError = fromErrorResp(resp); assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode()); @@ -204,12 +209,12 @@ public class EngineNewPayloadV3Test extends EngineNewPayloadV2Test { public void shouldValidateExcessBlobGasCorrectly() { // V3 must return error if null excessBlobGas BlockHeader blockHeader = - createBlockHeaderFixture(Optional.of(Collections.emptyList()), Optional.empty()) + createBlockHeaderFixture(Optional.of(emptyList())) .excessBlobGas(null) .blobGasUsed(100L) .buildHeader(); - var resp = resp(mockEnginePayload(blockHeader, Collections.emptyList(), List.of())); + var resp = resp(mockEnginePayload(blockHeader, emptyList(), List.of())); final JsonRpcError jsonRpcError = fromErrorResp(resp); assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode()); @@ -229,7 +234,6 @@ public class EngineNewPayloadV3Test extends EngineNewPayloadV2Test { BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))), - Optional.empty(), Optional.empty()); var resp = resp(mockEnginePayload(mockHeader, transactions)); @@ -265,7 +269,7 @@ public class EngineNewPayloadV3Test extends EngineNewPayloadV2Test { protected JsonRpcResponse resp(final EnginePayloadParameter payload) { Object[] params = maybeParentBeaconBlockRoot - .map(bytes32 -> new Object[] {payload, Collections.emptyList(), bytes32.toHexString()}) + .map(bytes32 -> new Object[] {payload, emptyList(), bytes32.toHexString()}) .orElseGet(() -> new Object[] {payload}); return method.response( new JsonRpcRequestContext(new JsonRpcRequest("2.0", this.method.getName(), params))); diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV4Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV4Test.java index c8504e745..fe7c38917 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV4Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV4Test.java @@ -14,8 +14,10 @@ */ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; +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.response.RpcErrorType.INVALID_EXECUTION_REQUESTS_PARAMS; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -42,7 +44,6 @@ import org.hyperledger.besu.ethereum.mainnet.requests.MainnetRequestsValidator; import org.hyperledger.besu.ethereum.mainnet.requests.ProhibitedRequestValidator; import org.hyperledger.besu.evm.gascalculator.PragueGasCalculator; -import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -59,14 +60,19 @@ public class EngineNewPayloadV4Test extends EngineNewPayloadV3Test { public EngineNewPayloadV4Test() {} + private static final List VALID_REQUESTS = + List.of( + new Request(RequestType.DEPOSIT, Bytes.of(1)), + new Request(RequestType.WITHDRAWAL, Bytes.of(1)), + new Request(RequestType.CONSOLIDATION, Bytes.of(1))); + @BeforeEach @Override public void before() { super.before(); maybeParentBeaconBlockRoot = Optional.of(Bytes32.ZERO); - // TODO this should be using NewPayloadV4 this.method = - new EngineNewPayloadV3( + new EngineNewPayloadV4( vertx, protocolSchedule, protocolContext, @@ -75,95 +81,63 @@ public class EngineNewPayloadV4Test extends EngineNewPayloadV3Test { engineCallListener); lenient().when(protocolSchedule.hardforkFor(any())).thenReturn(Optional.of(pragueHardfork)); lenient().when(protocolSpec.getGasCalculator()).thenReturn(new PragueGasCalculator()); + mockAllowedRequestsValidator(); } @Override public void shouldReturnExpectedMethodName() { - assertThat(method.getName()).isEqualTo("engine_newPayloadV3"); - } - - @Test - public void shouldReturnValidIfRequestsIsNull_WhenRequestsProhibited() { - mockProhibitedRequestsValidator(); - - BlockHeader mockHeader = - setupValidPayload( - new BlockProcessingResult( - Optional.of(new BlockProcessingOutputs(null, List.of(), Optional.empty()))), - Optional.empty(), - Optional.empty()); - when(blockchain.getBlockHeader(mockHeader.getParentHash())) - .thenReturn(Optional.of(mock(BlockHeader.class))); - when(mergeCoordinator.getLatestValidAncestor(mockHeader)) - .thenReturn(Optional.of(mockHeader.getHash())); - - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList())); - - assertValidResponse(mockHeader, resp); + assertThat(method.getName()).isEqualTo("engine_newPayloadV4"); } @Test public void shouldReturnInvalidIfRequestsIsNull_WhenRequestsAllowed() { - mockAllowedRequestsValidator(); var resp = - resp( - mockEnginePayload( - createBlockHeader(Optional.empty(), Optional.empty()), Collections.emptyList())); + respWithInvalidRequests( + mockEnginePayload(createValidBlockHeaderForV4(Optional.empty()), 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() { - final List requests = - List.of( - new Request(RequestType.DEPOSIT, Bytes.of(1)), - new Request(RequestType.WITHDRAWAL, Bytes.of(1)), - new Request(RequestType.CONSOLIDATION, Bytes.of(1))); - - mockAllowedRequestsValidator(); BlockHeader mockHeader = setupValidPayload( new BlockProcessingResult( - Optional.of(new BlockProcessingOutputs(null, List.of(), Optional.of(requests)))), - Optional.empty(), + Optional.of( + new BlockProcessingOutputs(null, List.of(), Optional.of(VALID_REQUESTS)))), Optional.empty()); when(blockchain.getBlockHeader(mockHeader.getParentHash())) .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(mockHeader)) .thenReturn(Optional.of(mockHeader.getHash())); - var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList()), requests); + var resp = resp(mockEnginePayload(mockHeader, emptyList())); assertValidResponse(mockHeader, resp); } @Test public void shouldReturnInvalidIfRequestsIsNotNull_WhenRequestsProhibited() { - final List requests = - List.of( - new Request(RequestType.DEPOSIT, Bytes.of(1)), - new Request(RequestType.WITHDRAWAL, Bytes.of(1)), - new Request(RequestType.CONSOLIDATION, Bytes.of(1))); - mockProhibitedRequestsValidator(); - var resp = - resp( - mockEnginePayload( - createBlockHeader(Optional.empty(), Optional.of(Collections.emptyList())), - Collections.emptyList()), - requests); + var resp = resp(mockEnginePayload(createValidBlockHeaderForV4(Optional.empty()), emptyList())); final JsonRpcError jsonRpcError = fromErrorResp(resp); assertThat(jsonRpcError.getCode()).isEqualTo(INVALID_PARAMS.getCode()); verify(engineCallListener, times(1)).executionEngineCalled(); } - @Override - protected BlockHeader createBlockHeader( - final Optional> maybeWithdrawals, - final Optional> maybeRequests) { + private BlockHeader createValidBlockHeaderForV4( + final Optional> maybeWithdrawals) { + return createBlockHeaderFixtureForV3(maybeWithdrawals) + .requestsHash(BodyValidation.requestsHash(VALID_REQUESTS)) + .buildHeader(); + } + + private BlockHeaderTestFixture createBlockHeaderFixtureForV3( + final Optional> maybeWithdrawals) { BlockHeader parentBlockHeader = new BlockHeaderTestFixture() .baseFeePerGas(Wei.ONE) @@ -172,36 +146,27 @@ public class EngineNewPayloadV4Test extends EngineNewPayloadV3Test { .blobGasUsed(0L) .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)) - .excessBlobGas(BlobGas.ZERO) - .blobGasUsed(0L) - .requestsHash(maybeRequests.map(BodyValidation::requestsHash).orElse(null)) - .parentBeaconBlockRoot( - maybeParentBeaconBlockRoot.isPresent() ? maybeParentBeaconBlockRoot : null) - .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)) + .excessBlobGas(BlobGas.ZERO) + .blobGasUsed(0L) + .parentBeaconBlockRoot( + maybeParentBeaconBlockRoot.isPresent() ? maybeParentBeaconBlockRoot : null); + } + + @Override + protected BlockHeader createBlockHeader(final Optional> maybeWithdrawals) { + return createValidBlockHeaderForV4(maybeWithdrawals); } @Override protected JsonRpcResponse resp(final EnginePayloadParameter payload) { - Object[] params = - maybeParentBeaconBlockRoot - .map(bytes32 -> new Object[] {payload, Collections.emptyList(), bytes32.toHexString()}) - .orElseGet(() -> new Object[] {payload}); - return method.response( - new JsonRpcRequestContext(new JsonRpcRequest("2.0", this.method.getName(), params))); - } - - protected JsonRpcResponse resp( - final EnginePayloadParameter payload, final List requests) { final List requestsWithoutRequestId = - requests.stream() + VALID_REQUESTS.stream() .sorted(Comparator.comparing(Request::getType)) .map(r -> r.getData().toHexString()) .toList(); @@ -210,10 +175,20 @@ public class EngineNewPayloadV4Test extends EngineNewPayloadV3Test { .map( bytes32 -> new Object[] { - payload, - Collections.emptyList(), - bytes32.toHexString(), - requestsWithoutRequestId + payload, emptyList(), bytes32.toHexString(), requestsWithoutRequestId + }) + .orElseGet(() -> new Object[] {payload}); + return method.response( + new JsonRpcRequestContext(new JsonRpcRequest("2.0", this.method.getName(), params))); + } + + protected JsonRpcResponse respWithInvalidRequests(final EnginePayloadParameter payload) { + Object[] params = + maybeParentBeaconBlockRoot + .map( + bytes32 -> + new Object[] {payload, emptyList(), bytes32.toHexString() + // empty requests param is invalid }) .orElseGet(() -> new Object[] {payload}); return method.response(