[4844] Add excessDataGas and dataGasUsed validation to EngineNewPayloadV3 #5683
Conversation
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Gabriel-Trintinalia
changed the title
| ValidationResult<NewPayloadValidationResult> forkValidationResult = | ||
| validateForkSupported(reqId, blockParam); | ||
| if (!forkValidationResult.isValid()) { | ||
| return new JsonRpcErrorResponse(reqId, UNSUPPORTED_FORK); |
There was a problem hiding this comment.
This is an improvement in readability IMO and good reuse of the ValidationResult. Not really for this PR, but wonder if we should move ValidationResult to a more generic package since it's already being used outside of the ethereum.mainnet
| protected Optional<JsonRpcResponse> validateTransactions( | ||
| final EnginePayloadParameter blockParam, | ||
| final Object reqId, | ||
| protected ValidationResult<NewPayloadValidationResult> validateBlobs( |
| final Optional<BlockHeader> maybeParentHeader, | ||
| final Optional<List<String>> maybeVersionedHashParam, | ||
| final ProtocolSpec protocolSpec) { | ||
| return ValidationResult.valid(); |
There was a problem hiding this comment.
nit: Since Cancun will be the new default, I think it is preferable to put the actual blob validation code in the Abstract class and override with valid() for V1 and V2.
That means when we no longer want to support V2, it's a simple delete of EngineNewPayloadV2 class without any refactoring.
For example, this was done for V1 with these two overrides https://github.com/hyperledger/besu/blob/main/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1.java#L44-L52
(indeed, we could probably delete those V1 classes now that Shanghai is live)
| validateTransactions(blockParam, reqId, transactions, maybeVersionedHashParam); | ||
| if (transactionValidationResult.isPresent()) { | ||
| return transactionValidationResult.get(); | ||
| ValidationResult<NewPayloadValidationResult> transactionValidationResult = |
There was a problem hiding this comment.
| ValidationResult<NewPayloadValidationResult> transactionValidationResult = | |
| ValidationResult<NewPayloadValidationResult> blobValidationResult = |
| } | ||
| } else { | ||
| if (payloadParameter.getDataGasUsed() != null | ||
| || payloadParameter.getExcessDataGas() != null) { | ||
| return Optional.of(new JsonRpcErrorResponse(reqId, UNSUPPORTED_FORK)); | ||
| return ValidationResult.invalid(NewPayloadValidationResult.INVALID_NEW_PAYLOAD_PARAMS); |
There was a problem hiding this comment.
There's been a semantic change from UNSUPPORTED_FORK to INVALID_NEW_PAYLOAD_PARAMS here? Just checking it's not copy/paste error.
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Gabriel-Trintinalia
merged commit d3faf00
into
hyperledger:4844-devnet-5b
…adV3 (hyperledger#5683) Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
PR description
Add excessDataGas and dataGasUsed validation to EngineNewPayloadV3