Skip to content

Commit

Permalink
Test against EngineNewPayloadV4 in EngineNewPayloadV4Test (#7837)
Browse files Browse the repository at this point in the history 
Also change "Missing" -> "Unexpected" message when V3 blob params sent to V2

Signed-off-by: Simon Dudley <[email protected]>
  • Loading branch information
@siladu
siladu authored Oct 31, 2024
1 parent c5020be commit ec46074
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 159 deletions.
4 changes: 2 additions & 2 deletions ...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV2.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ protected ValidationResult<RpcErrorType> validateParameters(
final Optional<List<String>> 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();
}
Expand Down
88 changes: 40 additions & 48 deletions ...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -51,7 +52,6 @@
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;
Expand All @@ -62,7 +62,6 @@
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;
Expand Down Expand Up @@ -123,25 +122,23 @@ public void shouldReturnValid() {
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);
}

@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);
Expand All @@ -152,14 +149,14 @@ public void shouldReturnInvalidOnBlockExecutionError() {

@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();
Expand All @@ -170,28 +167,27 @@ public void shouldReturnAcceptedOnLatestValidAncestorEmpty() {

@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());
when(mergeCoordinator.isBadBlock(mockHeader.getHash())).thenReturn(true);
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));
Expand All @@ -204,12 +200,11 @@ public void shouldNotReturnInvalidOnStorageException() {
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();
Expand All @@ -220,13 +215,12 @@ public void shouldNotReturnInvalidOnHandledMerkleTrieException() {
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();

Expand All @@ -235,15 +229,15 @@ public void shouldNotReturnInvalidOnHandledMerkleTrieException() {

@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)));
when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class)))
.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();

Expand All @@ -252,15 +246,15 @@ public void shouldNotReturnInvalidOnThrownMerkleTrieException() {

@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());
lenient()
.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());
Expand All @@ -269,11 +263,11 @@ public void shouldReturnInvalidBlockHashOnBadHashParameter() {

@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();
Expand All @@ -283,7 +277,7 @@ public void shouldCheckBlockValidityBeforeCheckingByHashForExisting() {

@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));

Expand All @@ -298,9 +292,9 @@ public void shouldReturnInvalidOnMalformedTransactions() {

@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();
Expand All @@ -311,10 +305,10 @@ public void shouldRespondWithSyncingDuringForwardSync() {

@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();
Expand All @@ -325,12 +319,12 @@ public void shouldRespondWithSyncingDuringBackwardsSync() {

@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();
Expand All @@ -342,8 +336,8 @@ public void shouldRespondWithInvalidIfExtraDataIsNull() {
@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);
Expand All @@ -359,12 +353,11 @@ public void shouldReturnValidIfProtocolScheduleIsEmpty() {
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);
}
Expand Down Expand Up @@ -408,11 +401,9 @@ protected EnginePayloadParameter mockEnginePayload(
}

protected BlockHeader setupValidPayload(
final BlockProcessingResult value,
final Optional<List<Withdrawal>> maybeWithdrawals,
final Optional<List<Request>> maybeRequests) {
final BlockProcessingResult value, final Optional<List<Withdrawal>> 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));
Expand All @@ -425,6 +416,11 @@ protected ExecutionEngineJsonRpcMethod.EngineStatus getExpectedInvalidBlockHashS
}

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)
Expand All @@ -441,15 +437,12 @@ protected JsonRpcError fromErrorResp(final JsonRpcResponse resp) {
.get();
}

protected BlockHeader createBlockHeader(
final Optional<List<Withdrawal>> maybeWithdrawals,
final Optional<List<Request>> maybeRequests) {
return createBlockHeaderFixture(maybeWithdrawals, maybeRequests).buildHeader();
protected BlockHeader createBlockHeader(final Optional<List<Withdrawal>> maybeWithdrawals) {
return createBlockHeaderFixture(maybeWithdrawals).buildHeader();
}

protected BlockHeaderTestFixture createBlockHeaderFixture(
final Optional<List<Withdrawal>> maybeWithdrawals,
final Optional<List<Request>> maybeRequests) {
final Optional<List<Withdrawal>> maybeWithdrawals) {
BlockHeader parentBlockHeader =
new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
return new BlockHeaderTestFixture()
Expand All @@ -458,8 +451,7 @@ protected BlockHeaderTestFixture createBlockHeaderFixture(
.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) {
Expand Down
Loading

0 comments on commit ec46074

Please sign in to comment.