From e46f8120432e7deaa921c8525c6d6372be530c29 Mon Sep 17 00:00:00 2001 From: Justin Florentine Date: Fri, 13 Oct 2023 17:05:36 -0400 Subject: [PATCH] more error handling Signed-off-by: Justin Florentine --- .../AbstractEngineForkchoiceUpdated.java | 81 +++++++++++-------- .../engine/EngineForkchoiceUpdatedV1.java | 14 ++++ .../engine/EngineForkchoiceUpdatedV2.java | 10 +-- .../engine/EngineForkchoiceUpdatedV3.java | 11 +-- 4 files changed, 67 insertions(+), 49 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 7c7cfca2c44..da16c978975 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 @@ -93,37 +93,26 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash()); - if (mergeCoordinator.isBadBlock(forkChoice.getHeadBlockHash())) { - logForkchoiceUpdatedCall(INVALID, forkChoice); - return new JsonRpcSuccessResponse( - requestId, - new EngineUpdateForkchoiceResult( - INVALID, - mergeCoordinator - .getLatestValidHashOfBadBlock(forkChoice.getHeadBlockHash()) - .orElse(Hash.ZERO), - null, - Optional.of(forkChoice.getHeadBlockHash() + " is an invalid block"))); - } - final Optional maybeNewHead = mergeCoordinator.getOrSyncHeadByHash( forkChoice.getHeadBlockHash(), forkChoice.getFinalizedBlockHash()); + ForkchoiceResult forkchoiceResult = null; if (maybeNewHead.isEmpty()) { return syncingResponse(requestId, forkChoice); - } - Optional> withdrawals = Optional.empty(); - final BlockHeader newHead = maybeNewHead.get(); - if (!isValidForkchoiceState( - forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), newHead)) { + } else if (!isValidForkchoiceState( + forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), maybeNewHead.get())) { logForkchoiceUpdatedCall(INVALID, forkChoice); return new JsonRpcErrorResponse(requestId, RpcErrorType.INVALID_FORKCHOICE_STATE); + } else { + forkchoiceResult = + mergeCoordinator.updateForkChoice( + maybeNewHead.get(), + forkChoice.getFinalizedBlockHash(), + forkChoice.getSafeBlockHash()); } - ForkchoiceResult result = - mergeCoordinator.updateForkChoice( - newHead, forkChoice.getFinalizedBlockHash(), forkChoice.getSafeBlockHash()); + Optional> withdrawals = Optional.empty(); if (maybePayloadAttributes.isPresent()) { final EnginePayloadAttributesParameter payloadAttributes = maybePayloadAttributes.get(); withdrawals = @@ -136,7 +125,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) .map(WithdrawalParameter::toWithdrawal) .collect(toList()))); Optional maybeError = - isPayloadAttributesValid(requestId, payloadAttributes, withdrawals, newHead); + isPayloadAttributesValid(requestId, payloadAttributes, withdrawals); if (maybeError.isPresent()) { LOG.atWarn() .setMessage("RpcError {}: {}") @@ -156,6 +145,33 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) } } + if (mergeCoordinator.isBadBlock(forkChoice.getHeadBlockHash())) { + logForkchoiceUpdatedCall(INVALID, forkChoice); + return new JsonRpcSuccessResponse( + requestId, + new EngineUpdateForkchoiceResult( + INVALID, + mergeCoordinator + .getLatestValidHashOfBadBlock(forkChoice.getHeadBlockHash()) + .orElse(Hash.ZERO), + null, + Optional.of(forkChoice.getHeadBlockHash() + " is an invalid block"))); + } + + final BlockHeader newHead = maybeNewHead.get(); + if (maybePayloadAttributes.isPresent()) { + Optional maybeError = + isPayloadAttributeRelevantToNewHead(requestId, maybePayloadAttributes.get(), newHead); + if (maybeError.isPresent()) { + return maybeError.get(); + } + if (!getWithdrawalsValidator( + protocolSchedule.get(), newHead, maybePayloadAttributes.get().getTimestamp()) + .validateWithdrawals(withdrawals)) { + return new JsonRpcErrorResponse(requestId, getInvalidPayloadError()); + } + } + ValidationResult parameterValidationResult = validateParameter(forkChoice, maybePayloadAttributes); if (!parameterValidationResult.isValid()) { @@ -169,13 +185,13 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) maybePayloadAttributes.ifPresentOrElse( this::logPayload, () -> LOG.debug("Payload attributes are null")); - if (result.shouldNotProceedToPayloadBuildProcess()) { - if (ForkchoiceResult.Status.IGNORE_UPDATE_TO_OLD_HEAD.equals(result.getStatus())) { + if (forkchoiceResult.shouldNotProceedToPayloadBuildProcess()) { + if (ForkchoiceResult.Status.IGNORE_UPDATE_TO_OLD_HEAD.equals(forkchoiceResult.getStatus())) { logForkchoiceUpdatedCall(VALID, forkChoice); } else { logForkchoiceUpdatedCall(INVALID, forkChoice); } - return handleNonValidForkchoiceUpdate(requestId, result); + return handleNonValidForkchoiceUpdate(requestId, forkchoiceResult); } // begin preparing a block if we have a non-empty payload attributes param @@ -205,15 +221,19 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) requestId, new EngineUpdateForkchoiceResult( VALID, - result.getNewHead().map(BlockHeader::getHash).orElse(null), + forkchoiceResult.getNewHead().map(BlockHeader::getHash).orElse(null), payloadId.orElse(null), Optional.empty())); } - protected Optional isPayloadAttributesValid( + protected abstract Optional isPayloadAttributesValid( + final Object requestId, + final EnginePayloadAttributesParameter payloadAttributes, + final Optional> maybeWithdrawals); + + protected Optional isPayloadAttributeRelevantToNewHead( final Object requestId, final EnginePayloadAttributesParameter payloadAttributes, - final Optional> maybeWithdrawals, final BlockHeader headBlockHeader) { if (payloadAttributes.getTimestamp() <= headBlockHeader.getTimestamp()) { @@ -221,11 +241,6 @@ protected Optional isPayloadAttributesValid( "Payload attributes timestamp is smaller than timestamp of header in fork choice update"); return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadError())); } - if (!getWithdrawalsValidator( - protocolSchedule.get(), headBlockHeader, payloadAttributes.getTimestamp()) - .validateWithdrawals(maybeWithdrawals)) { - return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadError())); - } return Optional.empty(); } 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 a5b9eeb9ed2..24d63fbd434 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,9 +17,15 @@ 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.parameters.EnginePayloadAttributesParameter; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType; +import org.hyperledger.besu.ethereum.core.Withdrawal; import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; +import java.util.List; +import java.util.Optional; + import io.vertx.core.Vertx; public class EngineForkchoiceUpdatedV1 extends AbstractEngineForkchoiceUpdated { @@ -33,6 +39,14 @@ public EngineForkchoiceUpdatedV1( super(vertx, protocolSchedule, protocolContext, mergeCoordinator, engineCallListener); } + @Override + protected Optional isPayloadAttributesValid( + final Object requestId, + final EnginePayloadAttributesParameter payloadAttributes, + final Optional> maybeWithdrawals) { + return Optional.empty(); + } + @Override public String getName() { return RpcMethod.ENGINE_FORKCHOICE_UPDATED_V1.getMethodName(); diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2.java index 3b2e1bfcd66..c4daa688d47 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2.java @@ -20,7 +20,6 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadAttributesParameter; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType; -import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Withdrawal; import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; @@ -55,8 +54,7 @@ public String getName() { protected Optional isPayloadAttributesValid( final Object requestId, final EnginePayloadAttributesParameter payloadAttributes, - final Optional> maybeWithdrawals, - final BlockHeader headBlockHeader) { + final Optional> maybeWithdrawals) { if (payloadAttributes.getTimestamp() >= cancunTimestamp) { if (payloadAttributes.getParentBeaconBlockRoot() == null || payloadAttributes.getParentBeaconBlockRoot().isEmpty()) { @@ -64,14 +62,12 @@ protected Optional isPayloadAttributesValid( } else { return Optional.of(new JsonRpcErrorResponse(requestId, RpcErrorType.INVALID_PARAMS)); } - } else if (payloadAttributes.getParentBeaconBlockRoot() != null - || !payloadAttributes.getParentBeaconBlockRoot().isEmpty()) { + } else if (payloadAttributes.getParentBeaconBlockRoot() != null) { LOG.error( "Parent beacon block root hash present in payload attributes before cancun hardfork"); return Optional.of(new JsonRpcErrorResponse(requestId, RpcErrorType.INVALID_PARAMS)); } else { - return super.isPayloadAttributesValid( - requestId, payloadAttributes, maybeWithdrawals, headBlockHeader); + return Optional.empty(); } } } diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV3.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV3.java index 4284fdd01dc..5b33f653fb3 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV3.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV3.java @@ -21,7 +21,6 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadAttributesParameter; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType; -import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Withdrawal; import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; import org.hyperledger.besu.ethereum.mainnet.ScheduledProtocolSpec; @@ -94,14 +93,8 @@ protected ValidationResult validateForkSupported(final long blockT protected Optional isPayloadAttributesValid( final Object requestId, final EnginePayloadAttributesParameter payloadAttributes, - final Optional> maybeWithdrawals, - final BlockHeader headBlockHeader) { - Optional maybeError = - super.isPayloadAttributesValid( - requestId, payloadAttributes, maybeWithdrawals, headBlockHeader); - if (maybeError.isPresent()) { - return maybeError; - } else if (payloadAttributes.getParentBeaconBlockRoot() == null) { + final Optional> maybeWithdrawals) { + if (payloadAttributes.getParentBeaconBlockRoot() == null) { LOG.error( "Parent beacon block root hash not present in payload attributes after cancun hardfork"); return Optional.of(new JsonRpcErrorResponse(requestId, RpcErrorType.INVALID_PARAMS));