Skip to content

Shanghai Engine API Updates #4945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
siladu merged 12 commits into from
Jan 18, 2023
Merged

Shanghai Engine API Updates #4945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5f987b5
Update v2 method
wcgcyx Jan 17, 2023
ba28dd7
Fix unit tests
wcgcyx Jan 17, 2023
b16eb09
Merge branch 'main' into 4916-engine-api-updates
wcgcyx Jan 17, 2023
d78a9c2
Update unit tests
wcgcyx Jan 17, 2023
c7af1d5
Merge branch 'main' into 4916-engine-api-updates
wcgcyx Jan 17, 2023
3cdd6d1
Merge remote-tracking branch 'upstream/main' into 4916-engine-api-upd…
wcgcyx Jan 18, 2023
ec23294
Merge remote-tracking branch 'upstream/main' into 4916-engine-api-upd…
wcgcyx Jan 18, 2023
5c05882
Update FCU error
wcgcyx Jan 18, 2023
df5302b
Update test
wcgcyx Jan 18, 2023
04c1f9a
Update unit tests
wcgcyx Jan 18, 2023
add7a66
Merge branch 'main' into 4916-engine-api-updates
wcgcyx Jan 18, 2023
76833fb
Merge branch 'main' into 4916-engine-api-updates
siladu Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions ...er/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -154,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);
Copy link
Contributor

@macfarla macfarla Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this change mentioned in ethereum/execution-apis#338 - is this something different?

Copy link
Contributor Author

@wcgcyx wcgcyx Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, bring some changes from ethereum/execution-apis#337 too. Sorry should have updated the description.

Copy link
Contributor

@siladu siladu Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, I got Zhenyang to crowbar that change in too :)

Copy link
Contributor

@macfarla macfarla Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok makes sense

return new JsonRpcErrorResponse(requestId, getInvalidPayloadError());
}

if (!result.isValid()) {
Expand Down Expand Up @@ -217,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(
Expand Down Expand Up @@ -300,6 +298,14 @@ private JsonRpcResponse syncingResponse(
requestId, new EngineUpdateForkchoiceResult(SYNCING, null, null, Optional.empty()));
}

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 =
Expand Down
13 changes: 11 additions & 2 deletions ...perledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,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()) {
Expand Down Expand Up @@ -208,7 +208,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());
Expand Down Expand Up @@ -308,6 +309,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(
Expand Down
11 changes: 11 additions & 0 deletions ...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,4 +37,14 @@ public EngineForkchoiceUpdatedV1(
public String getName() {
return RpcMethod.ENGINE_FORKCHOICE_UPDATED_V1.getMethodName();
}

@Override
protected boolean requireTerminalPoWBlockValidation() {
return true;
}

@Override
protected JsonRpcError getInvalidPayloadError() {
return JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES;
}
}
12 changes: 12 additions & 0 deletions ...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,4 +41,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;
}
}
55 changes: 29 additions & 26 deletions ...esu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -138,21 +138,6 @@ public void shouldReturnSyncingIfMissingNewHead() {
mockFcuParam, Optional.empty(), mock(ForkchoiceResult.class), SYNCING);
}

@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(ForkchoiceResult.class),
INVALID,
Optional.of(Hash.ZERO));
}

@Test
public void shouldReturnInvalidWithLatestValidHashOnBadBlock() {
BlockHeader mockHeader = blockHeaderBuilder.buildHeader();
Expand Down Expand Up @@ -181,7 +166,9 @@ 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),
Expand All @@ -199,7 +186,9 @@ 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()))
Expand Down Expand Up @@ -244,7 +233,9 @@ 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(
Expand Down Expand Up @@ -427,7 +418,9 @@ 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);
Expand Down Expand Up @@ -481,7 +474,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();
}

Expand All @@ -505,7 +498,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();
}

Expand Down Expand Up @@ -569,7 +562,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();
}

Expand Down Expand Up @@ -667,7 +660,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));
when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
if (validateTerminalPoWBlock()) {
when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
}
when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
}

Expand All @@ -680,7 +675,7 @@ protected EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResu
fcuParam, payloadParam, forkchoiceResult, expectedStatus, Optional.empty());
}

private EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult(
protected EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult(
final EngineForkchoiceUpdatedParameter fcuParam,
final Optional<EnginePayloadAttributesParameter> payloadParam,
final ForkchoiceResult forkchoiceResult,
Expand Down Expand Up @@ -724,6 +719,14 @@ private EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult
return res;
}

protected boolean validateTerminalPoWBlock() {
return false;
}

protected JsonRpcError expectedInvalidPayloadError() {
return JsonRpcError.INVALID_PARAMS;
}

private JsonRpcResponse resp(
final EngineForkchoiceUpdatedParameter forkchoiceParam,
final Optional<EnginePayloadAttributesParameter> payloadParam) {
Expand Down
Loading