From 5f987b5eb0d4c43ffe74a52f9fabe2b73999a65e Mon Sep 17 00:00:00 2001 From: Zhenyang Shi Date: Tue, 17 Jan 2023 12:14:01 +1000 Subject: [PATCH 1/6] Update v2 method Signed-off-by: Zhenyang Shi --- .../engine/AbstractEngineForkchoiceUpdated.java | 7 ++++++- .../methods/engine/AbstractEngineNewPayload.java | 13 +++++++++++-- .../methods/engine/EngineForkchoiceUpdatedV1.java | 5 +++++ .../internal/methods/engine/EngineNewPayloadV1.java | 12 ++++++++++++ 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java index 469382cfd4c..2dc795bd33e 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java @@ -119,7 +119,8 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) } // TODO: post-merge cleanup, this should be unnecessary after merge - if (!mergeContext.get().isCheckpointPostMergeSync() + if (requireTerminalPoWBlockValidation() + && !mergeContext.get().isCheckpointPostMergeSync() && !mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead) && !mergeContext.get().isChainPruningEnabled()) { logForkchoiceUpdatedCall(INVALID, forkChoice); @@ -315,6 +316,10 @@ private JsonRpcResponse syncingResponse( requestId, new EngineUpdateForkchoiceResult(SYNCING, null, null, Optional.empty())); } + protected boolean requireTerminalPoWBlockValidation() { + return false; + } + // fcU calls are synchronous, no need to make volatile private long lastFcuInfoLog = System.currentTimeMillis(); private static final String logMessage = diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java index 92549fd177c..cd45613f438 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java @@ -148,7 +148,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) "Computed block hash %s does not match block hash parameter %s", newBlockHeader.getBlockHash(), blockParam.getBlockHash()); LOG.debug(errorMessage); - return respondWithInvalid(reqId, blockParam, null, INVALID_BLOCK_HASH, errorMessage); + return respondWithInvalid(reqId, blockParam, null, getInvalidBlockHashStatus(), errorMessage); } // do we already have this payload if (protocolContext.getBlockchain().getBlockByHash(newBlockHeader.getBlockHash()).isPresent()) { @@ -190,7 +190,8 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) } // TODO: post-merge cleanup - if (!mergeContext.get().isCheckpointPostMergeSync() + if (requireTerminalPoWBlockValidation() + && !mergeContext.get().isCheckpointPostMergeSync() && !mergeCoordinator.latestValidAncestorDescendsFromTerminal(newBlockHeader) && !mergeContext.get().isChainPruningEnabled()) { mergeCoordinator.addBadBlock(block, Optional.empty()); @@ -290,6 +291,14 @@ JsonRpcResponse respondWithInvalid( invalidStatus, latestValidHash, Optional.of(validationError))); } + protected boolean requireTerminalPoWBlockValidation() { + return false; + } + + protected EngineStatus getInvalidBlockHashStatus() { + return INVALID; + } + private void logImportedBlockInfo(final Block block, final double timeInS) { LOG.info( String.format( diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java index b43052b1554..22070a5c5f6 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java @@ -36,4 +36,9 @@ public EngineForkchoiceUpdatedV1( public String getName() { return RpcMethod.ENGINE_FORKCHOICE_UPDATED_V1.getMethodName(); } + + @Override + protected boolean requireTerminalPoWBlockValidation() { + return true; + } } diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1.java index 42a9f9edd65..2c9e2af093a 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1.java @@ -14,6 +14,8 @@ */ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; +import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID_BLOCK_HASH; + import org.hyperledger.besu.consensus.merge.blockcreation.MergeMiningCoordinator; import org.hyperledger.besu.ethereum.ProtocolContext; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; @@ -36,4 +38,14 @@ public EngineNewPayloadV1( public String getName() { return RpcMethod.ENGINE_NEW_PAYLOAD_V1.getMethodName(); } + + @Override + protected boolean requireTerminalPoWBlockValidation() { + return true; + } + + @Override + protected EngineStatus getInvalidBlockHashStatus() { + return INVALID_BLOCK_HASH; + } } From ba28dd787051e7a081a16f3a1a769d36bd10f30b Mon Sep 17 00:00:00 2001 From: Zhenyang Shi Date: Tue, 17 Jan 2023 13:58:28 +1000 Subject: [PATCH 2/6] Fix unit tests Signed-off-by: Zhenyang Shi --- .../engine/AbstractEngineNewPayloadTest.java | 45 ++++++++++++------- .../engine/EngineNewPayloadV1Test.java | 13 ++++++ 2 files changed, 43 insertions(+), 15 deletions(-) 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 ed5f2cfbae6..a2b8517074d 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 @@ -17,7 +17,6 @@ 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; -import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID_BLOCK_HASH; import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.SYNCING; import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.VALID; import static org.mockito.ArgumentMatchers.any; @@ -39,6 +38,7 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; 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.methods.ExecutionEngineJsonRpcMethod; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadParameter; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.UnsignedLongParameter; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError; @@ -126,8 +126,9 @@ public void shouldReturnValid() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(true); when(mergeCoordinator.rememberBlock(any())) .thenReturn( new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of())))); @@ -149,8 +150,9 @@ public void shouldReturnInvalidOnBlockExecutionError() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(true); when(mergeCoordinator.rememberBlock(any())).thenReturn(new BlockProcessingResult("error 42")); var resp = resp(mockPayload(mockHeader, Collections.emptyList())); @@ -170,8 +172,9 @@ public void shouldReturnAcceptedOnLatestValidAncestorEmpty() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.empty()); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(true); var resp = resp(mockPayload(mockHeader, Collections.emptyList())); @@ -201,6 +204,7 @@ public void shouldReturnSuccessOnAlreadyPresent() { @Test public void shouldReturnInvalidOnBadTerminalBlock() { + if (!validateTerminalPoWBlock()) return; BlockHeader mockHeader = createBlockHeader(); when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); when(blockchain.getBlockHeader(mockHeader.getParentHash())) @@ -243,8 +247,9 @@ public void shouldNotReturnInvalidOnStorageException() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(true); when(mergeCoordinator.rememberBlock(any())) .thenReturn( new BlockProcessingResult(Optional.empty(), new StorageException("database bedlam"))); @@ -265,8 +270,9 @@ public void shouldNotReturnInvalidOnHandledMerkleTrieException() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(true); when(mergeCoordinator.rememberBlock(any())) .thenReturn( new BlockProcessingResult(Optional.empty(), new MerkleTrieException("missing leaf"))); @@ -287,8 +293,9 @@ public void shouldNotReturnInvalidOnThrownMerkleTrieException() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(true); when(mergeCoordinator.rememberBlock(any())).thenThrow(new MerkleTrieException("missing leaf")); var resp = resp(mockPayload(mockHeader, Collections.emptyList())); @@ -307,7 +314,7 @@ public void shouldReturnInvalidBlockHashOnBadHashParameter() { EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); - assertThat(res.getStatusAsString()).isEqualTo(INVALID_BLOCK_HASH.name()); + assertThat(res.getStatusAsString()).isEqualTo(getCorrectInvalidBlockHashStatus().name()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -321,7 +328,7 @@ public void shouldCheckBlockValidityBeforeCheckingByHashForExisting() { EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); - assertThat(res.getStatusAsString()).isEqualTo(INVALID_BLOCK_HASH.name()); + assertThat(res.getStatusAsString()).isEqualTo(getCorrectInvalidBlockHashStatus().name()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -425,6 +432,14 @@ public void shouldReturnInvalidWhenBadBlock() { verify(engineCallListener, times(1)).executionEngineCalled(); } + protected boolean validateTerminalPoWBlock() { + return false; + } + + protected ExecutionEngineJsonRpcMethod.EngineStatus getCorrectInvalidBlockHashStatus() { + return INVALID; + } + private JsonRpcResponse resp(final EnginePayloadParameter payload) { return method.response( new JsonRpcRequestContext( diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java index fd739a7b89b..b43fc8885f4 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java @@ -15,6 +15,9 @@ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID_BLOCK_HASH; + +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod; import org.junit.Test; import org.junit.runner.RunWith; @@ -32,4 +35,14 @@ public EngineNewPayloadV1Test() { public void shouldReturnExpectedMethodName() { assertThat(method.getName()).isEqualTo("engine_newPayloadV1"); } + + @Override + protected boolean validateTerminalPoWBlock() { + return true; + } + + @Override + protected ExecutionEngineJsonRpcMethod.EngineStatus getCorrectInvalidBlockHashStatus() { + return INVALID_BLOCK_HASH; + } } From d78a9c2cce2dccfaf8fd3785e80d79c8d6453980 Mon Sep 17 00:00:00 2001 From: Zhenyang Shi Date: Tue, 17 Jan 2023 14:49:10 +1000 Subject: [PATCH 3/6] Update unit tests Signed-off-by: Zhenyang Shi --- .../AbstractEngineForkchoiceUpdatedTest.java | 20 ++++++++++++++----- .../engine/EngineForkchoiceUpdatedV1Test.java | 5 +++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java index 8486b80b869..2482cb2a376 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java @@ -140,6 +140,7 @@ public void shouldReturnSyncingIfMissingNewHead() { @Test public void shouldReturnInvalidOnBadTerminalBlock() { + if (!validateTerminalPoWBlock()) return; BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) @@ -181,7 +182,8 @@ public void shouldReturnValidWithoutFinalizedOrPayload() { BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); assertSuccessWithPayloadForForkchoiceResult( new EngineForkchoiceUpdatedParameter(mockHeader.getHash(), Hash.ZERO, Hash.ZERO), @@ -199,7 +201,8 @@ public void shouldReturnInvalidOnOldTimestamp() { .timestamp(parent.getTimestamp()) .buildHeader(); when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true); when(mergeContext.isSyncing()).thenReturn(false); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), parent.getHash())) @@ -244,7 +247,8 @@ public void shouldReturnValidWithoutFinalizedWithPayload() { BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); var payloadParams = new EnginePayloadAttributesParameter( @@ -427,7 +431,8 @@ public void shouldIgnoreUpdateToOldHeadAndNotPreparePayload() { when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); var ignoreOldHeadUpdateRes = ForkchoiceResult.withIgnoreUpdateToOldHead(mockHeader); when(mergeCoordinator.updateForkChoice(any(), any(), any())).thenReturn(ignoreOldHeadUpdateRes); @@ -667,7 +672,8 @@ private void setupValidForkchoiceUpdate(final BlockHeader mockHeader) { when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + if (validateTerminalPoWBlock()) + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true); } @@ -724,6 +730,10 @@ private EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult return res; } + protected boolean validateTerminalPoWBlock() { + return false; + } + private JsonRpcResponse resp( final EngineForkchoiceUpdatedParameter forkchoiceParam, final Optional payloadParam) { diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java index 5c60d95f57a..d5947d45f51 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java @@ -39,4 +39,9 @@ public void shouldReturnExpectedMethodName() { protected String getMethodName() { return RpcMethod.ENGINE_FORKCHOICE_UPDATED_V1.getMethodName(); } + + @Override + protected boolean validateTerminalPoWBlock() { + return true; + } } From 5c05882171cb89a2d2f8f0dfb235bdd24337a186 Mon Sep 17 00:00:00 2001 From: Zhenyang Shi Date: Wed, 18 Jan 2023 11:19:10 +1000 Subject: [PATCH 4/6] Update FCU error Signed-off-by: Zhenyang Shi --- .../engine/AbstractEngineForkchoiceUpdated.java | 9 +++++---- .../methods/engine/EngineForkchoiceUpdatedV1.java | 6 ++++++ .../engine/AbstractEngineForkchoiceUpdatedTest.java | 10 +++++++--- .../methods/engine/EngineForkchoiceUpdatedV1Test.java | 6 ++++++ 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java index 264c4c6b978..2adff6c49b3 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java @@ -155,7 +155,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) "Invalid payload attributes: {}", () -> maybePayloadAttributes.map(EnginePayloadAttributesParameter::serialize).orElse(null)); - return new JsonRpcErrorResponse(requestId, JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES); + return new JsonRpcErrorResponse(requestId, getInvalidPayloadError()); } if (!result.isValid()) { @@ -218,9 +218,6 @@ private JsonRpcResponse handleNonValidForkchoiceUpdate( new EngineUpdateForkchoiceResult( INVALID, latestValid.orElse(null), null, result.getErrorMessage())); break; - case INVALID_PAYLOAD_ATTRIBUTES: - response = new JsonRpcErrorResponse(requestId, JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES); - break; case IGNORE_UPDATE_TO_OLD_HEAD: response = new JsonRpcSuccessResponse( @@ -305,6 +302,10 @@ protected boolean requireTerminalPoWBlockValidation() { return false; } + protected JsonRpcError getInvalidPayloadError() { + return JsonRpcError.INVALID_PARAMS; + } + // fcU calls are synchronous, no need to make volatile private long lastFcuInfoLog = System.currentTimeMillis(); private static final String logMessage = diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java index 22070a5c5f6..dd965f20bcb 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java @@ -17,6 +17,7 @@ import org.hyperledger.besu.consensus.merge.blockcreation.MergeMiningCoordinator; import org.hyperledger.besu.ethereum.ProtocolContext; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError; import org.hyperledger.besu.ethereum.mainnet.TimestampSchedule; import io.vertx.core.Vertx; @@ -41,4 +42,9 @@ public String getName() { protected boolean requireTerminalPoWBlockValidation() { return true; } + + @Override + protected JsonRpcError getInvalidPayloadError() { + return JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES; + } } diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java index 2482cb2a376..e0f2e6c044d 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java @@ -486,7 +486,7 @@ public void shouldReturnInvalidIfPayloadTimestampNotGreaterThanHead() { mockHeader.getHash(), Hash.ZERO, mockParent.getHash()), Optional.of(payloadParams)); - assertInvalidForkchoiceState(resp, JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES); + assertInvalidForkchoiceState(resp, expectedInvalidPayloadError()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -510,7 +510,7 @@ public void shouldReturnInvalidIfWithdrawalsIsNotNull_WhenWithdrawalsProhibited( mockHeader.getHash(), Hash.ZERO, mockParent.getHash()), Optional.of(payloadParams)); - assertInvalidForkchoiceState(resp, JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES); + assertInvalidForkchoiceState(resp, expectedInvalidPayloadError()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -574,7 +574,7 @@ public void shouldReturnInvalidIfWithdrawalsIsNull_WhenWithdrawalsAllowed() { mockHeader.getHash(), Hash.ZERO, mockParent.getHash()), Optional.of(payloadParams)); - assertInvalidForkchoiceState(resp, JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES); + assertInvalidForkchoiceState(resp, expectedInvalidPayloadError()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -734,6 +734,10 @@ protected boolean validateTerminalPoWBlock() { return false; } + protected JsonRpcError expectedInvalidPayloadError() { + return JsonRpcError.INVALID_PARAMS; + } + private JsonRpcResponse resp( final EngineForkchoiceUpdatedParameter forkchoiceParam, final Optional payloadParam) { diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java index d5947d45f51..6f56dc13dd2 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java @@ -17,6 +17,7 @@ import static org.assertj.core.api.Assertions.assertThat; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError; import org.junit.Test; import org.junit.runner.RunWith; @@ -44,4 +45,9 @@ protected String getMethodName() { protected boolean validateTerminalPoWBlock() { return true; } + + @Override + protected JsonRpcError expectedInvalidPayloadError() { + return JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES; + } } From df5302b2c26adcdba05f4352526707f1a9b4ca9a Mon Sep 17 00:00:00 2001 From: Zhenyang Shi Date: Wed, 18 Jan 2023 11:56:25 +1000 Subject: [PATCH 5/6] Update test Signed-off-by: Zhenyang Shi --- .../AbstractEngineForkchoiceUpdatedTest.java | 15 ++++++++++----- .../engine/AbstractEngineNewPayloadTest.java | 15 +++++++++------ 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java index e0f2e6c044d..95e7c24d660 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java @@ -182,8 +182,9 @@ public void shouldReturnValidWithoutFinalizedOrPayload() { BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + } assertSuccessWithPayloadForForkchoiceResult( new EngineForkchoiceUpdatedParameter(mockHeader.getHash(), Hash.ZERO, Hash.ZERO), @@ -201,8 +202,9 @@ public void shouldReturnInvalidOnOldTimestamp() { .timestamp(parent.getTimestamp()) .buildHeader(); when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + } when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true); when(mergeContext.isSyncing()).thenReturn(false); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), parent.getHash())) @@ -247,8 +249,9 @@ public void shouldReturnValidWithoutFinalizedWithPayload() { BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + } var payloadParams = new EnginePayloadAttributesParameter( @@ -431,8 +434,9 @@ public void shouldIgnoreUpdateToOldHeadAndNotPreparePayload() { when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + } var ignoreOldHeadUpdateRes = ForkchoiceResult.withIgnoreUpdateToOldHead(mockHeader); when(mergeCoordinator.updateForkChoice(any(), any(), any())).thenReturn(ignoreOldHeadUpdateRes); @@ -672,8 +676,9 @@ private void setupValidForkchoiceUpdate(final BlockHeader mockHeader) { when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); + } when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true); } 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 7b887903793..30990dccdef 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 @@ -169,9 +169,10 @@ public void shouldReturnAcceptedOnLatestValidAncestorEmpty() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.empty()); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) .thenReturn(true); + } var resp = resp(mockPayload(mockHeader, Collections.emptyList())); @@ -270,9 +271,10 @@ public void shouldNotReturnInvalidOnThrownMerkleTrieException() { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) .thenReturn(true); + } when(mergeCoordinator.rememberBlock(any())).thenThrow(new MerkleTrieException("missing leaf")); var resp = resp(mockPayload(mockHeader, Collections.emptyList())); @@ -291,7 +293,7 @@ public void shouldReturnInvalidBlockHashOnBadHashParameter() { EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); - assertThat(res.getStatusAsString()).isEqualTo(getCorrectInvalidBlockHashStatus().name()); + assertThat(res.getStatusAsString()).isEqualTo(getExpectedInvalidBlockHashStatus().name()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -305,7 +307,7 @@ public void shouldCheckBlockValidityBeforeCheckingByHashForExisting() { EnginePayloadStatusResult res = fromSuccessResp(resp); assertThat(res.getLatestValidHash()).isEmpty(); - assertThat(res.getStatusAsString()).isEqualTo(getCorrectInvalidBlockHashStatus().name()); + assertThat(res.getStatusAsString()).isEqualTo(getExpectedInvalidBlockHashStatus().name()); verify(engineCallListener, times(1)).executionEngineCalled(); } @@ -529,9 +531,10 @@ private BlockHeader setupValidPayload(final BlockProcessingResult value) { .thenReturn(Optional.of(mock(BlockHeader.class))); when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class))) .thenReturn(Optional.of(mockHash)); - if (validateTerminalPoWBlock()) + if (validateTerminalPoWBlock()) { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) .thenReturn(true); + } when(mergeCoordinator.rememberBlock(any())).thenReturn(value); return mockHeader; } @@ -540,7 +543,7 @@ protected boolean validateTerminalPoWBlock() { return false; } - protected ExecutionEngineJsonRpcMethod.EngineStatus getCorrectInvalidBlockHashStatus() { + protected ExecutionEngineJsonRpcMethod.EngineStatus getExpectedInvalidBlockHashStatus() { return INVALID; } From 04c1f9a11b0f0082f016601c5241b939b9db96e3 Mon Sep 17 00:00:00 2001 From: Zhenyang Shi Date: Wed, 18 Jan 2023 12:04:57 +1000 Subject: [PATCH 6/6] Update unit tests Signed-off-by: Zhenyang Shi --- .../AbstractEngineForkchoiceUpdatedTest.java | 22 ++---------- .../engine/AbstractEngineNewPayloadTest.java | 34 ++++--------------- .../engine/EngineForkchoiceUpdatedV1Test.java | 24 +++++++++++++ .../engine/EngineNewPayloadV1Test.java | 33 +++++++++++++++++- 4 files changed, 66 insertions(+), 47 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java index 95e7c24d660..09dd87042e6 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java @@ -95,7 +95,7 @@ public AbstractEngineForkchoiceUpdatedTest(final MethodFactory methodFactory) { private static final EngineForkchoiceUpdatedParameter mockFcuParam = new EngineForkchoiceUpdatedParameter(mockHash, mockHash, mockHash); - private static final BlockHeaderTestFixture blockHeaderBuilder = + protected static final BlockHeaderTestFixture blockHeaderBuilder = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE); @Mock private ProtocolSpec protocolSpec; @@ -104,7 +104,7 @@ public AbstractEngineForkchoiceUpdatedTest(final MethodFactory methodFactory) { @Mock private MergeContext mergeContext; - @Mock private MergeMiningCoordinator mergeCoordinator; + @Mock protected MergeMiningCoordinator mergeCoordinator; @Mock private MutableBlockchain blockchain; @@ -138,22 +138,6 @@ public void shouldReturnSyncingIfMissingNewHead() { mockFcuParam, Optional.empty(), mock(ForkchoiceResult.class), SYNCING); } - @Test - public void shouldReturnInvalidOnBadTerminalBlock() { - if (!validateTerminalPoWBlock()) return; - BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); - - when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) - .thenReturn(Optional.of(mockHeader)); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(false); - assertSuccessWithPayloadForForkchoiceResult( - new EngineForkchoiceUpdatedParameter(mockHeader.getHash(), Hash.ZERO, Hash.ZERO), - Optional.empty(), - mock(ForkchoiceResult.class), - INVALID, - Optional.of(Hash.ZERO)); - } - @Test public void shouldReturnInvalidWithLatestValidHashOnBadBlock() { BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); @@ -691,7 +675,7 @@ protected EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResu fcuParam, payloadParam, forkchoiceResult, expectedStatus, Optional.empty()); } - private EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult( + protected EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult( final EngineForkchoiceUpdatedParameter fcuParam, final Optional payloadParam, final ForkchoiceResult forkchoiceResult, 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 30990dccdef..c5cf02e9ecf 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 @@ -23,7 +23,6 @@ import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError.INVALID_PARAMS; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -108,13 +107,13 @@ public AbstractEngineNewPayloadTest(final MethodFactory methodFactory) { @Mock private MergeContext mergeContext; - @Mock private MergeMiningCoordinator mergeCoordinator; + @Mock protected MergeMiningCoordinator mergeCoordinator; - @Mock private MutableBlockchain blockchain; + @Mock protected MutableBlockchain blockchain; @Mock private EthPeers ethPeers; - @Mock private EngineCallListener engineCallListener; + @Mock protected EngineCallListener engineCallListener; @Before public void before() { @@ -198,25 +197,6 @@ public void shouldReturnSuccessOnAlreadyPresent() { assertValidResponse(mockHeader, resp); } - @Test - public void shouldReturnInvalidOnBadTerminalBlock() { - if (!validateTerminalPoWBlock()) return; - BlockHeader mockHeader = createBlockHeader(); - when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); - when(blockchain.getBlockHeader(mockHeader.getParentHash())) - .thenReturn(Optional.of(mock(BlockHeader.class))); - when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) - .thenReturn(false); - - var resp = resp(mockPayload(mockHeader, Collections.emptyList())); - - EnginePayloadStatusResult res = fromSuccessResp(resp); - assertThat(res.getLatestValidHash()).isEqualTo(Optional.of(Hash.ZERO)); - assertThat(res.getStatusAsString()).isEqualTo(INVALID.name()); - verify(mergeCoordinator, atLeastOnce()).addBadBlock(any(), any()); - verify(engineCallListener, times(1)).executionEngineCalled(); - } - @Test public void shouldReturnInvalidWithLatestValidHashIsABadBlock() { BlockHeader mockHeader = createBlockHeader(); @@ -475,14 +455,14 @@ public void shouldReturnValidIfTimestampScheduleIsEmpty() { assertValidResponse(mockHeader, resp); } - private JsonRpcResponse resp(final EnginePayloadParameter payload) { + protected JsonRpcResponse resp(final EnginePayloadParameter payload) { return method.response( new JsonRpcRequestContext( new JsonRpcRequest( "2.0", RpcMethod.ENGINE_EXECUTE_PAYLOAD.getMethodName(), new Object[] {payload}))); } - private EnginePayloadParameter mockPayload(final BlockHeader header, final List txs) { + protected EnginePayloadParameter mockPayload(final BlockHeader header, final List txs) { return new EnginePayloadParameter( header.getHash(), header.getParentHash(), @@ -547,7 +527,7 @@ protected ExecutionEngineJsonRpcMethod.EngineStatus getExpectedInvalidBlockHashS return INVALID; } - private EnginePayloadStatusResult fromSuccessResp(final JsonRpcResponse resp) { + protected EnginePayloadStatusResult fromSuccessResp(final JsonRpcResponse resp) { assertThat(resp.getType()).isEqualTo(JsonRpcResponseType.SUCCESS); return Optional.of(resp) .map(JsonRpcSuccessResponse.class::cast) @@ -564,7 +544,7 @@ private JsonRpcError fromErrorResp(final JsonRpcResponse resp) { .get(); } - private BlockHeader createBlockHeader() { + protected BlockHeader createBlockHeader() { BlockHeader parentBlockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader(); BlockHeader mockHeader = diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java index 6f56dc13dd2..0433b55bae1 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java @@ -15,9 +15,18 @@ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import org.hyperledger.besu.consensus.merge.blockcreation.MergeMiningCoordinator; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EngineForkchoiceUpdatedParameter; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError; +import org.hyperledger.besu.ethereum.core.BlockHeader; + +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -36,6 +45,21 @@ public void shouldReturnExpectedMethodName() { assertThat(method.getName()).isEqualTo("engine_forkchoiceUpdatedV1"); } + @Test + public void shouldReturnInvalidOnBadTerminalBlock() { + BlockHeader mockHeader = blockHeaderBuilder.buildHeader(); + + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) + .thenReturn(Optional.of(mockHeader)); + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(false); + assertSuccessWithPayloadForForkchoiceResult( + new EngineForkchoiceUpdatedParameter(mockHeader.getHash(), Hash.ZERO, Hash.ZERO), + Optional.empty(), + mock(MergeMiningCoordinator.ForkchoiceResult.class), + INVALID, + Optional.of(Hash.ZERO)); + } + @Override protected String getMethodName() { return RpcMethod.ENGINE_FORKCHOICE_UPDATED_V1.getMethodName(); diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java index b43fc8885f4..06c7b8a1830 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java @@ -15,9 +15,22 @@ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; 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.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID_BLOCK_HASH; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atLeastOnce; +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.Hash; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.EnginePayloadStatusResult; +import org.hyperledger.besu.ethereum.core.BlockHeader; + +import java.util.Collections; +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -36,13 +49,31 @@ public void shouldReturnExpectedMethodName() { assertThat(method.getName()).isEqualTo("engine_newPayloadV1"); } + @Test + public void shouldReturnInvalidOnBadTerminalBlock() { + BlockHeader mockHeader = createBlockHeader(); + when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty()); + when(blockchain.getBlockHeader(mockHeader.getParentHash())) + .thenReturn(Optional.of(mock(BlockHeader.class))); + when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class))) + .thenReturn(false); + + var resp = resp(mockPayload(mockHeader, Collections.emptyList())); + + EnginePayloadStatusResult res = fromSuccessResp(resp); + assertThat(res.getLatestValidHash()).isEqualTo(Optional.of(Hash.ZERO)); + assertThat(res.getStatusAsString()).isEqualTo(INVALID.name()); + verify(mergeCoordinator, atLeastOnce()).addBadBlock(any(), any()); + verify(engineCallListener, times(1)).executionEngineCalled(); + } + @Override protected boolean validateTerminalPoWBlock() { return true; } @Override - protected ExecutionEngineJsonRpcMethod.EngineStatus getCorrectInvalidBlockHashStatus() { + protected ExecutionEngineJsonRpcMethod.EngineStatus getExpectedInvalidBlockHashStatus() { return INVALID_BLOCK_HASH; } }