From 40941712ea3fcf8f20489a60eb0b47dd5d186007 Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 30 Aug 2023 15:39:06 +1000 Subject: [PATCH 1/3] enforce that BlobTransactions have at least one blob Signed-off-by: Stefan --- .../besu/datatypes/BlobsWithCommitments.java | 8 ++- .../datatypes/BlobsWithCommitmentsTest.java | 57 ++++++++++++++++++- .../engine/AbstractEngineNewPayload.java | 2 +- .../besu/ethereum/core/Transaction.java | 15 ++--- .../eth/manager/RespondingEthPeer.java | 2 +- 5 files changed, 70 insertions(+), 14 deletions(-) diff --git a/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java b/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java index ed0a6dcf9a8..ba79b699245 100644 --- a/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java +++ b/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java @@ -38,11 +38,15 @@ public BlobsWithCommitments( final List blobs, final List kzgProofs, final List versionedHashes) { + if (blobs.size() < 1) { + throw new InvalidParameterException( + "There needs to be a minimum of one blob in a blob transaction with commitments"); + } if (blobs.size() != kzgCommitments.size() || blobs.size() != kzgProofs.size() - || kzgCommitments.size() != versionedHashes.size()) { + || blobs.size() != versionedHashes.size()) { throw new InvalidParameterException( - "There must be an equal number of blobs, commitments and proofs"); + "There must be an equal number of blobs, commitments, proofs, and versioned hashes"); } this.kzgCommitments = kzgCommitments; this.blobs = blobs; diff --git a/datatypes/src/test/java/org/hyperledger/besu/datatypes/BlobsWithCommitmentsTest.java b/datatypes/src/test/java/org/hyperledger/besu/datatypes/BlobsWithCommitmentsTest.java index fa5d3066d42..9816288e82a 100644 --- a/datatypes/src/test/java/org/hyperledger/besu/datatypes/BlobsWithCommitmentsTest.java +++ b/datatypes/src/test/java/org/hyperledger/besu/datatypes/BlobsWithCommitmentsTest.java @@ -22,19 +22,70 @@ import java.util.List; import org.apache.tuweni.bytes.Bytes; +import org.apache.tuweni.bytes.Bytes32; import org.junit.jupiter.api.Test; public class BlobsWithCommitmentsTest { @Test - public void blobsWithCommitmentsMustHaveSameNumberOfElements() { + public void blobsWithCommitmentsMustHaveAtLeastOneBlob() { + String actualMessage = + assertThrows( + InvalidParameterException.class, + () -> new BlobsWithCommitments(List.of(), List.of(), List.of(), List.of())) + .getMessage(); + final String expectedMessage = + "There needs to be a minimum of one blob in a blob transaction with commitments"; + assertThat(actualMessage).isEqualTo(expectedMessage); + } + + @Test + public void blobsWithCommitmentsMustHaveSameNumberOfElementsVersionedHashes() { + String actualMessage = + assertThrows( + InvalidParameterException.class, + () -> + new BlobsWithCommitments( + List.of(new KZGCommitment(Bytes.of(1))), + List.of(new Blob(Bytes.EMPTY)), + List.of(new KZGProof(Bytes.EMPTY)), + List.of())) + .getMessage(); + final String expectedMessage = + "There must be an equal number of blobs, commitments, proofs, and versioned hashes"; + assertThat(actualMessage).isEqualTo(expectedMessage); + } + + @Test + public void blobsWithCommitmentsMustHaveSameNumberOfElementsKZGCommitment() { + String actualMessage = + assertThrows( + InvalidParameterException.class, + () -> + new BlobsWithCommitments( + List.of(), + List.of(new Blob(Bytes.EMPTY)), + List.of(new KZGProof(Bytes.EMPTY)), + List.of(new VersionedHash(Bytes32.rightPad(Bytes.fromHexString("0x01")))))) + .getMessage(); + final String expectedMessage = + "There must be an equal number of blobs, commitments, proofs, and versioned hashes"; + assertThat(actualMessage).isEqualTo(expectedMessage); + } + + @Test + public void blobsWithCommitmentsMustHaveSameNumberOfElementsKZGProof() { String actualMessage = assertThrows( InvalidParameterException.class, () -> new BlobsWithCommitments( - List.of(new KZGCommitment(Bytes.of(1))), List.of(), List.of(), List.of())) + List.of(new KZGCommitment(Bytes.of(1))), + List.of(new Blob(Bytes.EMPTY)), + List.of(), + List.of(new VersionedHash(Bytes32.rightPad(Bytes.fromHexString("0x01")))))) .getMessage(); - final String expectedMessage = "There must be an equal number of blobs, commitments and proofs"; + final String expectedMessage = + "There must be an equal number of blobs, commitments, proofs, and versioned hashes"; assertThat(actualMessage).isEqualTo(expectedMessage); } } 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 d70b1cc1997..b7b42d696ca 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 @@ -209,7 +209,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) blockParam.getPrevRandao(), 0, maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null), - blockParam.getBlobGasUsed() == null ? null : blockParam.getBlobGasUsed(), + blockParam.getBlobGasUsed(), blockParam.getExcessBlobGas() == null ? null : BlobGas.fromHexString(blockParam.getExcessBlobGas()), diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java index 77c7ad4131f..e04d036c738 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java @@ -189,14 +189,15 @@ public Transaction( if (versionedHashes.isPresent() || maxFeePerBlobGas.isPresent()) { checkArgument( transactionType.supportsBlob(), - "Must not specify blob versioned hashes of max fee per blob gas for transaction not supporting it"); + "Must not specify blob versioned hashes or max fee per blob gas for transaction not supporting it"); } if (transactionType.supportsBlob()) { checkArgument( versionedHashes.isPresent(), "Must specify blob versioned hashes for blob transaction"); checkArgument( - !versionedHashes.get().isEmpty(), "Blob transaction must have at least one blob"); + !versionedHashes.get().isEmpty(), + "Blob transaction must have at least one versioned hash"); checkArgument( maxFeePerBlobGas.isPresent(), "Must specify max fee per blob gas for blob transaction"); } @@ -680,13 +681,13 @@ private void memoizeHashAndSize() { if (transactionType.supportsBlob()) { if (getBlobsWithCommitments().isPresent()) { - size = TransactionEncoder.encodeOpaqueBytes(this).size(); + size = bytes.size(); + return; } - } else { - final BytesValueRLPOutput rlpOutput = new BytesValueRLPOutput(); - TransactionEncoder.encodeForWire(transactionType, bytes, rlpOutput); - size = rlpOutput.encodedSize(); } + final BytesValueRLPOutput rlpOutput = new BytesValueRLPOutput(); + TransactionEncoder.encodeForWire(transactionType, bytes, rlpOutput); + size = rlpOutput.encodedSize(); } /** diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/RespondingEthPeer.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/RespondingEthPeer.java index abb0d05a4f9..7427545984b 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/RespondingEthPeer.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/RespondingEthPeer.java @@ -307,7 +307,7 @@ public static Responder partialResponder( final TransactionPool transactionPool, final ProtocolSchedule protocolSchedule, final float portion) { - checkArgument(portion >= 0.0 && portion <= 1.0, "Portion is in the range [0.0..1.0]"); + checkArgument(portion >= 0.0 && portion <= 1.0, "Portion is not in the range [0.0..1.0]"); final Responder fullResponder = blockchainResponder(blockchain, worldStateArchive, transactionPool); From 9d2bac7fed54e03ad53cc8e55072ef1391eecc49 Mon Sep 17 00:00:00 2001 From: Stefan Date: Wed, 30 Aug 2023 16:14:55 +1000 Subject: [PATCH 2/3] fix unit test Signed-off-by: Stefan --- .../hyperledger/besu/ethereum/core/TransactionBuilderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java index 6e5c92524d7..b06a63cb95b 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java @@ -73,7 +73,7 @@ public void zeroBlobTransactionIsInvalid() { .createTransaction(senderKeys); fail(); } catch (IllegalArgumentException iea) { - assertThat(iea).hasMessage("Blob transaction must have at least one blob"); + assertThat(iea).hasMessage("Blob transaction must have at least one versioned hash"); } } } From 41896b0ec56b95ee91efd0c02f4ea9e66fcd8ec8 Mon Sep 17 00:00:00 2001 From: Stefan Pingel <16143240+pinges@users.noreply.github.com> Date: Thu, 31 Aug 2023 09:13:11 +1000 Subject: [PATCH 3/3] Update datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java Signed-off-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> --- .../org/hyperledger/besu/datatypes/BlobsWithCommitments.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java b/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java index ba79b699245..7a4f0f4de6c 100644 --- a/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java +++ b/datatypes/src/main/java/org/hyperledger/besu/datatypes/BlobsWithCommitments.java @@ -38,7 +38,7 @@ public BlobsWithCommitments( final List blobs, final List kzgProofs, final List versionedHashes) { - if (blobs.size() < 1) { + if (blobs.size() == 0) { throw new InvalidParameterException( "There needs to be a minimum of one blob in a blob transaction with commitments"); }