From 4913d615acf9e731da422a57b58e3efcbdf0df8b Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Fri, 30 Jun 2023 17:43:25 +1000 Subject: [PATCH] [4844] Check params earlier and move blob validation to a new method (#5663) Signed-off-by: Gabriel-Trintinalia --- .../engine/AbstractEngineNewPayload.java | 97 +++++++++---------- 1 file changed, 44 insertions(+), 53 deletions(-) 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 89c9f8805f0..096ef76da6f 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 @@ -27,6 +27,7 @@ import org.hyperledger.besu.consensus.merge.blockcreation.MergeMiningCoordinator; import org.hyperledger.besu.datatypes.DataGas; import org.hyperledger.besu.datatypes.Hash; +import org.hyperledger.besu.datatypes.VersionedHash; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.BlockProcessingResult; import org.hyperledger.besu.ethereum.ProtocolContext; @@ -101,16 +102,6 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) Optional> maybeVersionedHashParam = requestContext.getOptionalList(1, String.class); - Optional> maybeVersionedHashes = Optional.empty(); - if (maybeVersionedHashParam.isPresent()) { - List versionedHashes = - maybeVersionedHashParam.get().stream() - .map(Bytes32::fromHexStringStrict) - .collect(Collectors.toList()); - if (versionedHashes.size() > 0) { - maybeVersionedHashes = Optional.of(versionedHashes); - } - } Object reqId = requestContext.getRequest().getId(); @@ -204,6 +195,20 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) LOG.debug(errorMessage); return respondWithInvalid(reqId, blockParam, null, getInvalidBlockHashStatus(), errorMessage); } + + var blobTransactions = + transactions.stream().filter(transaction -> transaction.getType().supportsBlob()).toList(); + + // Validate Blob Transactions + var blobTransactionsInvalidResult = + validateBlobTransactions(blobTransactions, maybeVersionedHashParam); + + if (blobTransactionsInvalidResult.isPresent()) { + LOG.debug(blobTransactionsInvalidResult.get()); + return respondWithInvalid( + reqId, blockParam, null, INVALID, blobTransactionsInvalidResult.get()); + } + // do we already have this payload if (protocolContext.getBlockchain().getBlockByHash(newBlockHeader.getBlockHash()).isPresent()) { LOG.debug("block already present"); @@ -220,49 +225,6 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) "Block already present in bad block manager."); } - final List transactionVersionedHashes = new ArrayList<>(); - // get versioned hashes, in order, from all blob tx - transactions.stream() - .filter(tx -> tx.getBlobCount() > 0) - .forEachOrdered( - tx -> - transactionVersionedHashes.addAll( - tx.getVersionedHashes().get().stream() - .map(vh -> vh.toBytes()) - .collect(toList()))); - // and compare with expected versioned hashes param - - // check if one is empty - if (maybeVersionedHashes.isPresent() && transactionVersionedHashes.isEmpty()) { - return respondWithInvalid( - reqId, - blockParam, - null, - INVALID, - "Versioned hashes from blob transactions (empty) do not match expected values"); - } - if (maybeVersionedHashes.isEmpty() && !transactionVersionedHashes.isEmpty()) { - return respondWithInvalid( - reqId, - blockParam, - null, - INVALID, - "Versioned hashes from blob transactions do not match expected values (empty)"); - } - if (maybeVersionedHashes.isEmpty() && transactionVersionedHashes.isEmpty()) { - LOG.trace("Versioned hashes from blob tx (empty) matches expected values (empty)"); - } else { - // otherwise, check list contents - if (!maybeVersionedHashes.get().equals(transactionVersionedHashes)) { - return respondWithInvalid( - reqId, - blockParam, - null, - INVALID, - "Versioned hashes from blob transactions do not match expected values (empty)"); - } - } - final Optional maybeParentHeader = protocolContext.getBlockchain().getBlockHeader(blockParam.getParentHash()); if (maybeParentHeader.isPresent() @@ -337,6 +299,35 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) } } + private Optional validateBlobTransactions( + final List blobTransactions, + final Optional> maybeVersionedHashParam) { + + List versionedHashesParam = + maybeVersionedHashParam + .map(strings -> strings.stream().map(Bytes32::fromHexStringStrict).toList()) + .orElseGet(ArrayList::new); + + final List transactionVersionedHashes = new ArrayList<>(); + + for (Transaction transaction : blobTransactions) { + if (transaction.getType().supportsBlob()) { + var versionedHashes = transaction.getVersionedHashes(); + if (versionedHashes.isEmpty()) { + return Optional.of("There must be at least one blob"); + } + transactionVersionedHashes.addAll( + versionedHashes.get().stream().map(VersionedHash::toBytes).toList()); + } + } + + // check list contents + if (!versionedHashesParam.equals(transactionVersionedHashes)) { + return Optional.of("Versioned hashes from blob transactions do not match expected values"); + } + return Optional.empty(); + } + JsonRpcResponse respondWith( final Object requestId, final EnginePayloadParameter param,