From 94618ea3f7f7e80c8eebbcb9d8c7a7963754808f Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Wed, 8 Nov 2023 16:01:50 -0700 Subject: [PATCH 1/5] Clamp up-front gas calculation Clamp upfront gas calculations to uint256 max instead of throwing an exception. Fix related encoding changes and tests to validate. Signed-off-by: Danno Ferrin --- .../methods/EthGetTransactionByHashTest.java | 3 +- .../besu/ethereum/core/Transaction.java | 17 ++------- .../BlobPooledTransactionDecoder.java | 7 +++- .../encoding/TransactionRLPDecoderTest.java | 37 +++++++++++++++---- .../besu/evmtool/StateTestSubCommandTest.java | 20 +++++----- .../besu/ethereum/rlp/AbstractRLPInput.java | 30 ++++++++------- 6 files changed, 65 insertions(+), 49 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java index 34ab6ba93eb..66bd74d4121 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java @@ -21,6 +21,7 @@ import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.datatypes.Transaction; +import org.hyperledger.besu.datatypes.TransactionType; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse; @@ -192,7 +193,7 @@ private Set getTransactionPool() { final BlockDataGenerator gen = new BlockDataGenerator(); Transaction pendingTransaction = gen.transaction(); System.out.println(pendingTransaction.getHash()); - return gen.transactionsWithAllTypes(4).stream() + return gen.transactions(4, TransactionType.EIP1559).stream() .map(PendingTransaction.Local::new) .collect(Collectors.toUnmodifiableSet()); } 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 9d51e1a3c1f..a7616438861 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 @@ -226,10 +226,6 @@ private Transaction( this.chainId = chainId; this.versionedHashes = versionedHashes; this.blobsWithCommitments = blobsWithCommitments; - - if (!forCopy && isUpfrontGasCostTooHigh()) { - throw new IllegalArgumentException("Upfront gas cost exceeds UInt256"); - } } /** @@ -566,15 +562,6 @@ private Wei getMaxUpfrontGasCost(final long blobGasPerBlock) { getMaxGasPrice(), getMaxFeePerBlobGas().orElse(Wei.ZERO), blobGasPerBlock); } - /** - * Check if the upfront gas cost is over the max allowed - * - * @return true is upfront data cost overflow uint256 max value - */ - private boolean isUpfrontGasCostTooHigh() { - return calculateUpfrontGasCost(getMaxGasPrice(), Wei.ZERO, 0L).bitLength() > 256; - } - /** * Calculates the up-front cost for the gas and blob gas the transaction can use. * @@ -619,7 +606,9 @@ private BigInteger calculateUpfrontGasCost( * @return the up-front gas cost for the transaction */ public Wei getUpfrontCost(final long totalBlobGas) { - return getMaxUpfrontGasCost(totalBlobGas).addExact(getValue()); + Wei maxUpfrontGasCost = getMaxUpfrontGasCost(totalBlobGas); + Wei result = maxUpfrontGasCost.add(getValue()); + return (maxUpfrontGasCost.compareTo(result) > 0) ? Wei.MAX_WEI : result; } /** diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java index 1ccd5c4ad42..bac3d472aca 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java @@ -30,6 +30,10 @@ */ public class BlobPooledTransactionDecoder { + private BlobPooledTransactionDecoder() { + // no instances + } + /** * Decodes a blob transaction from the provided RLP input. * @@ -44,7 +48,6 @@ public static Transaction decode(final RLPInput input) { List commitments = input.readList(KZGCommitment::readFrom); List proofs = input.readList(KZGProof::readFrom); input.leaveList(); - builder.kzgBlobs(commitments, blobs, proofs); - return builder.build(); + return builder.kzgBlobs(commitments, blobs, proofs).build(); } } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java index 245561158b0..4a84a5d56f0 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java @@ -17,6 +17,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.hyperledger.besu.evm.account.Account.MAX_NONCE; +import static org.junit.jupiter.api.Assumptions.assumeTrue; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.core.Transaction; @@ -32,7 +33,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; -import org.junit.jupiter.params.provider.ValueSource; class TransactionRLPDecoderTest { @@ -82,15 +82,22 @@ void shouldDecodeWithHighNonce() { private static Collection dataTransactionSize() { return Arrays.asList( new Object[][] { - {FRONTIER_TX_RLP, "FRONTIER_TX_RLP"}, - {EIP1559_TX_RLP, "EIP1559_TX_RLP"}, - {NONCE_64_BIT_MAX_MINUS_2_TX_RLP, "NONCE_64_BIT_MAX_MINUS_2_TX_RLP"} + {FRONTIER_TX_RLP, "FRONTIER_TX_RLP", true}, + {EIP1559_TX_RLP, "EIP1559_TX_RLP", true}, + {NONCE_64_BIT_MAX_MINUS_2_TX_RLP, "NONCE_64_BIT_MAX_MINUS_2_TX_RLP", true}, + { + "01f89a0130308263309430303030303030303030303030303030303030303030f838f7943030303030303030303030303030303030303030e0a0303030303030303030303030303030303030303030303030303030303030303001a03130303130303031313031313031303130303030323030323030323030313030a03030303030303030303030303030303030303030303030303030303030303030", + "EIP1559 list too small", + false + } }); } @ParameterizedTest(name = "[{index}] {1}") @MethodSource("dataTransactionSize") - void shouldCalculateCorrectTransactionSize(final String rlp_tx, final String ignoredName) { + void shouldCalculateCorrectTransactionSize( + final String rlp_tx, final String ignoredName, final boolean valid) { + assumeTrue(valid); // Create bytes from String final Bytes bytes = Bytes.fromHexString(rlp_tx); // Decode bytes into a transaction @@ -101,13 +108,27 @@ void shouldCalculateCorrectTransactionSize(final String rlp_tx, final String ign assertThat(transaction.getSize()).isEqualTo(transactionBytes.size()); } - @ParameterizedTest - @ValueSource(strings = {FRONTIER_TX_RLP, EIP1559_TX_RLP, NONCE_64_BIT_MAX_MINUS_2_TX_RLP}) - void shouldReturnCorrectEncodedBytes(final String txRlp) { + @ParameterizedTest(name = "[{index}] {1}") + @MethodSource("dataTransactionSize") + void shouldReturnCorrectEncodedBytes( + final String txRlp, final String ignoredName, final boolean valid) { + assumeTrue(valid); final Transaction transaction = decodeRLP(RLP.input(Bytes.fromHexString(txRlp))); assertThat(transaction.encoded()).isEqualTo(Bytes.fromHexString(txRlp)); } + @ParameterizedTest(name = "[{index}] {1}") + @MethodSource("dataTransactionSize") + void shouldDecodeRLP(final String txRlp, final String ignoredName, final boolean valid) { + if (valid) { + // thrown exceptions will break test + decodeRLP(RLP.input(Bytes.fromHexString(txRlp))); + } else { + assertThatThrownBy(() -> decodeRLP(RLP.input(Bytes.fromHexString(txRlp)))) + .isInstanceOf(RLPException.class); + } + } + private Transaction decodeRLP(final RLPInput input) { return TransactionDecoder.decodeRLP(input, EncodingContext.POOLED_TRANSACTION); } diff --git a/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java b/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java index 3a893898884..00ac7d52ae0 100644 --- a/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java +++ b/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java @@ -27,10 +27,10 @@ import org.junit.jupiter.api.Test; import picocli.CommandLine; -public class StateTestSubCommandTest { +class StateTestSubCommandTest { @Test - public void shouldDetectUnsupportedFork() { + void shouldDetectUnsupportedFork() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); EvmToolCommand parentCommand = new EvmToolCommand(System.in, new PrintWriter(baos, true, UTF_8)); @@ -44,7 +44,7 @@ public void shouldDetectUnsupportedFork() { } @Test - public void shouldWorkWithValidStateTest() { + void shouldWorkWithValidStateTest() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); EvmToolCommand parentCommand = new EvmToolCommand(System.in, new PrintWriter(baos, true, UTF_8)); @@ -55,7 +55,7 @@ public void shouldWorkWithValidStateTest() { } @Test - public void shouldWorkWithValidAccessListStateTest() { + void shouldWorkWithValidAccessListStateTest() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); EvmToolCommand parentCommand = new EvmToolCommand(System.in, new PrintWriter(baos, true, UTF_8)); @@ -66,7 +66,7 @@ public void shouldWorkWithValidAccessListStateTest() { } @Test - public void noJsonTracer() { + void noJsonTracer() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); EvmToolCommand parentCommand = new EvmToolCommand(System.in, new PrintWriter(baos, true, UTF_8)); @@ -80,7 +80,7 @@ public void noJsonTracer() { } @Test - public void testsInvalidTransactions() { + void testsInvalidTransactions() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); final ByteArrayInputStream bais = new ByteArrayInputStream( @@ -91,11 +91,11 @@ public void testsInvalidTransactions() { final StateTestSubCommand stateTestSubCommand = new StateTestSubCommand(new EvmToolCommand(bais, new PrintWriter(baos, true, UTF_8))); stateTestSubCommand.run(); - assertThat(baos.toString(UTF_8)).contains("Transaction had out-of-bounds parameters"); + assertThat(baos.toString(UTF_8)).contains("exceeds transaction sender account balance"); } @Test - public void shouldStreamTests() { + void shouldStreamTests() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); final ByteArrayInputStream bais = new ByteArrayInputStream( @@ -110,7 +110,7 @@ public void shouldStreamTests() { } @Test - public void failStreamMissingFile() { + void failStreamMissingFile() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); final ByteArrayInputStream bais = new ByteArrayInputStream("./file-dose-not-exist.json".getBytes(UTF_8)); @@ -121,7 +121,7 @@ public void failStreamMissingFile() { } @Test - public void failStreamBadFile() { + void failStreamBadFile() { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); final ByteArrayInputStream bais = new ByteArrayInputStream( diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java index 93a0bcebdf4..b75ae3224aa 100644 --- a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java +++ b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java @@ -147,17 +147,16 @@ private void prepareCurrentItem() { } private void validateCurrentItem() { - if (currentKind == RLPDecodingHelpers.Kind.SHORT_ELEMENT) { - // Validate that a single byte SHORT_ELEMENT payload is not <= 0x7F. If it is, is should have - // been written as a BYTE_ELEMENT. - if (currentPayloadSize == 1 - && currentPayloadOffset < size - && (payloadByte(0) & 0xFF) <= 0x7F) { - throwMalformed( - "Malformed RLP item: single byte value 0x%s should have been " - + "written without a prefix", - hex(currentPayloadOffset, currentPayloadOffset + 1)); - } + // Validate that a single byte SHORT_ELEMENT payload is not <= 0x7F. If it is, is should have + // been written as a BYTE_ELEMENT. + if (currentKind == RLPDecodingHelpers.Kind.SHORT_ELEMENT + && currentPayloadSize == 1 + && currentPayloadOffset < size + && (payloadByte(0) & 0xFF) <= 0x7F) { + throwMalformed( + "Malformed RLP item: single byte value 0x%s should have been " + + "written without a prefix", + hex(currentPayloadOffset, currentPayloadOffset + 1)); } if (currentPayloadSize > 0 && currentPayloadOffset >= size) { @@ -186,9 +185,9 @@ public boolean isDone() { private String hex(final long start, final long taintedEnd) { final long end = Math.min(taintedEnd, size); - final long size = end - start; - if (size < 10) { - return inputHex(start, Math.toIntExact(size)); + final long length = end - start; + if (length < 10) { + return inputHex(start, Math.toIntExact(length)); } else { return String.format("%s...%s", inputHex(start, 4), inputHex(end - 4, 4)); } @@ -245,6 +244,9 @@ private void checkElt(final String what) { if (currentItem >= size) { throw error("Cannot read a %s, input is fully consumed", what); } + if (depth > 0 && currentPayloadOffset + currentPayloadSize > endOfListOffset[depth - 1]) { + throw error("Cannot read a %s, too large for enclosing list", what); + } if (isEndOfCurrentList()) { throw error("Cannot read a %s, reached end of current list", what); } From 42e851ad78660de88ead632e3c1dc02747d3c280 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Wed, 22 Nov 2023 07:57:21 -0700 Subject: [PATCH 2/5] Add uint256 overflow check in transaction validation Signed-off-by: Danno Ferrin --- .../besu/ethereum/core/Transaction.java | 4 +- .../AccessListTransactionDecoder.java | 8 +- .../core/encoding/BlobTransactionDecoder.java | 8 +- .../encoding/EIP1559TransactionDecoder.java | 8 +- .../mainnet/BlockHeaderValidator.java | 47 +++-- .../mainnet/MainnetTransactionValidator.java | 7 + .../transaction/TransactionInvalidReason.java | 1 + .../encoding/TransactionRLPDecoderTest.java | 19 ++- ...TransactionHashesMessageProcessorTest.java | 160 +++++++----------- .../besu/ethereum/rlp/AbstractRLPInput.java | 25 +-- .../besu/ethereum/rlp/RLPInput.java | 20 ++- 11 files changed, 152 insertions(+), 155 deletions(-) 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 62cfe99334b..79261159498 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 @@ -99,7 +99,7 @@ public class Transaction private final Optional chainId; // Caches a "hash" of a portion of the transaction used for sender recovery. - // Note that this hash does not include the transaction signature so it does not + // Note that this hash does not include the transaction signature, so it does not // fully identify the transaction (use the result of the {@code hash()} for that). // It is only used to compute said signature and recover the sender from it. private volatile Bytes32 hashNoSignature; @@ -584,7 +584,7 @@ public Wei getUpfrontGasCost( } } - private BigInteger calculateUpfrontGasCost( + public BigInteger calculateUpfrontGasCost( final Wei gasPrice, final Wei blobGasPrice, final long totalBlobGas) { var cost = new BigInteger(1, Longs.toByteArray(getGasLimit())).multiply(gasPrice.getAsBigInteger()); diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java index 477ad57fe60..502c566157e 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java @@ -32,6 +32,10 @@ class AccessListTransactionDecoder { private static final Supplier SIGNATURE_ALGORITHM = Suppliers.memoize(SignatureAlgorithmFactory::getInstance); + private AccessListTransactionDecoder() { + // private constructor + } + public static Transaction decode(final RLPInput rlpInput) { rlpInput.enterList(); final Transaction.Builder preSignatureTransactionBuilder = @@ -43,7 +47,7 @@ public static Transaction decode(final RLPInput rlpInput) { .gasLimit(rlpInput.readLongScalar()) .to( rlpInput.readBytes( - addressBytes -> addressBytes.size() == 0 ? null : Address.wrap(addressBytes))) + addressBytes -> addressBytes.isEmpty() ? null : Address.wrap(addressBytes))) .value(Wei.of(rlpInput.readUInt256Scalar())) .payload(rlpInput.readBytes()) .accessList( @@ -57,7 +61,7 @@ public static Transaction decode(final RLPInput rlpInput) { accessListEntryRLPInput.leaveList(); return accessListEntry; })); - final byte recId = (byte) rlpInput.readIntScalar(); + final byte recId = (byte) rlpInput.readUnsignedByte(); final Transaction transaction = preSignatureTransactionBuilder .signature( diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java index 4e556ad1f98..3ee24c73dcf 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java @@ -32,6 +32,10 @@ public class BlobTransactionDecoder { private static final Supplier SIGNATURE_ALGORITHM = Suppliers.memoize(SignatureAlgorithmFactory::getInstance); + private BlobTransactionDecoder() { + // private constructor + } + /** * Decodes a blob transaction from the provided RLP input. * @@ -68,7 +72,7 @@ static void readTransactionPayloadInner(final Transaction.Builder builder, final .maxPriorityFeePerGas(Wei.of(input.readUInt256Scalar())) .maxFeePerGas(Wei.of(input.readUInt256Scalar())) .gasLimit(input.readLongScalar()) - .to(input.readBytes(v -> v.size() == 0 ? null : Address.wrap(v))) + .to(input.readBytes(v -> v.isEmpty() ? null : Address.wrap(v))) .value(Wei.of(input.readUInt256Scalar())) .payload(input.readBytes()) .accessList( @@ -86,7 +90,7 @@ static void readTransactionPayloadInner(final Transaction.Builder builder, final .versionedHashes( input.readList(versionedHashes -> new VersionedHash(versionedHashes.readBytes32()))); - final byte recId = (byte) input.readIntScalar(); + final byte recId = (byte) input.readUnsignedByte(); builder.signature( SIGNATURE_ALGORITHM .get() diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java index 35a549409e7..c0e89486d93 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java @@ -32,6 +32,10 @@ public class EIP1559TransactionDecoder { private static final Supplier SIGNATURE_ALGORITHM = Suppliers.memoize(SignatureAlgorithmFactory::getInstance); + private EIP1559TransactionDecoder() { + // private constructor + } + public static Transaction decode(final RLPInput input) { input.enterList(); final BigInteger chainId = input.readBigIntegerScalar(); @@ -43,7 +47,7 @@ public static Transaction decode(final RLPInput input) { .maxPriorityFeePerGas(Wei.of(input.readUInt256Scalar())) .maxFeePerGas(Wei.of(input.readUInt256Scalar())) .gasLimit(input.readLongScalar()) - .to(input.readBytes(v -> v.size() == 0 ? null : Address.wrap(v))) + .to(input.readBytes(v -> v.isEmpty() ? null : Address.wrap(v))) .value(Wei.of(input.readUInt256Scalar())) .payload(input.readBytes()) .accessList( @@ -57,7 +61,7 @@ public static Transaction decode(final RLPInput input) { accessListEntryRLPInput.leaveList(); return accessListEntry; })); - final byte recId = (byte) input.readIntScalar(); + final byte recId = (byte) input.readUnsignedByte(); final Transaction transaction = builder .signature( diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BlockHeaderValidator.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BlockHeaderValidator.java index 53937ba4261..56b1d4f0dbf 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BlockHeaderValidator.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BlockHeaderValidator.java @@ -41,31 +41,24 @@ public boolean validateHeader( final BlockHeader parent, final ProtocolContext protocolContext, final HeaderValidationMode mode) { - switch (mode) { - case NONE: - return true; - case LIGHT_DETACHED_ONLY: - return applyRules( - header, - parent, - protocolContext, - rule -> rule.includeInLightValidation() && rule.isDetachedSupported()); - case LIGHT_SKIP_DETACHED: - return applyRules( - header, - parent, - protocolContext, - rule -> rule.includeInLightValidation() && !rule.isDetachedSupported()); - case LIGHT: - return applyRules(header, parent, protocolContext, Rule::includeInLightValidation); - case DETACHED_ONLY: - return applyRules(header, parent, protocolContext, Rule::isDetachedSupported); - case SKIP_DETACHED: - return applyRules(header, parent, protocolContext, rule -> !rule.isDetachedSupported()); - case FULL: - return applyRules(header, parent, protocolContext, rule -> true); - } - throw new IllegalArgumentException("Unknown HeaderValidationMode: " + mode); + return switch (mode) { + case NONE -> true; + case LIGHT_DETACHED_ONLY -> applyRules( + header, + parent, + protocolContext, + rule -> rule.includeInLightValidation() && rule.isDetachedSupported()); + case LIGHT_SKIP_DETACHED -> applyRules( + header, + parent, + protocolContext, + rule -> rule.includeInLightValidation() && !rule.isDetachedSupported()); + case LIGHT -> applyRules(header, parent, protocolContext, Rule::includeInLightValidation); + case DETACHED_ONLY -> applyRules(header, parent, protocolContext, Rule::isDetachedSupported); + case SKIP_DETACHED -> applyRules( + header, parent, protocolContext, rule -> !rule.isDetachedSupported()); + case FULL -> applyRules(header, parent, protocolContext, rule -> true); + }; } public boolean validateHeader( @@ -90,7 +83,9 @@ private boolean applyRules( .allMatch( rule -> { boolean worked = rule.validate(header, parent, protocolContext); - if (!worked) LOG.debug("{} rule failed", rule.innerRuleClass().getCanonicalName()); + if (!worked) { + LOG.debug("{} rule failed", rule.innerRuleClass().getCanonicalName()); + } return worked; }); } diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java index 339e4c1d9c3..5fd841c1c39 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java @@ -182,6 +182,13 @@ private ValidationResult validateCostAndFee( intrinsicGasCost, transaction.getGasLimit())); } + if (transaction.calculateUpfrontGasCost(transaction.getMaxGasPrice(), Wei.ZERO, 0).bitLength() + > 256) { + return ValidationResult.invalid( + TransactionInvalidReason.UPFRONT_COST_EXCEEDS_UINT256, + "Upfront gas cost cannot exceed 2^256 Wei"); + } + return ValidationResult.valid(); } diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionInvalidReason.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionInvalidReason.java index e7b01f96757..aef98171b42 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionInvalidReason.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionInvalidReason.java @@ -23,6 +23,7 @@ public enum TransactionInvalidReason { REPLAY_PROTECTED_SIGNATURE_REQUIRED, INVALID_SIGNATURE, UPFRONT_COST_EXCEEDS_BALANCE, + UPFRONT_COST_EXCEEDS_UINT256, NONCE_TOO_LOW, NONCE_TOO_HIGH, NONCE_OVERFLOW, diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java index 4a84a5d56f0..80c6b1d4df5 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java @@ -86,8 +86,18 @@ private static Collection dataTransactionSize() { {EIP1559_TX_RLP, "EIP1559_TX_RLP", true}, {NONCE_64_BIT_MAX_MINUS_2_TX_RLP, "NONCE_64_BIT_MAX_MINUS_2_TX_RLP", true}, { - "01f89a0130308263309430303030303030303030303030303030303030303030f838f7943030303030303030303030303030303030303030e0a0303030303030303030303030303030303030303030303030303030303030303001a03130303130303031313031313031303130303030323030323030323030313030a03030303030303030303030303030303030303030303030303030303030303030", - "EIP1559 list too small", + "b89d01f89a0130308263309430303030303030303030303030303030303030303030f838f7943030303030303030303030303030303030303030e0a0303030303030303030303030303030303030303030303030303030303030303001a03130303130303031313031313031303130303030323030323030323030313030a03030303030303030303030303030303030303030303030303030303030303030", + "too large for enclosing list", + false + }, + { + "b84401f8410130308330303080308430303030d6d5943030303030303030303030303030303030303030c0808230309630303030303030303030303030303030303030303030", + "list ends outside of enclosing list", + false + }, + { + "9602d4013030308430303030803080c084303030013030", + "Cannot read a byte, expecting 1 bytes but current element is 4 bytes long", false } }); @@ -119,13 +129,14 @@ void shouldReturnCorrectEncodedBytes( @ParameterizedTest(name = "[{index}] {1}") @MethodSource("dataTransactionSize") - void shouldDecodeRLP(final String txRlp, final String ignoredName, final boolean valid) { + void shouldDecodeRLP(final String txRlp, final String name, final boolean valid) { if (valid) { // thrown exceptions will break test decodeRLP(RLP.input(Bytes.fromHexString(txRlp))); } else { assertThatThrownBy(() -> decodeRLP(RLP.input(Bytes.fromHexString(txRlp)))) - .isInstanceOf(RLPException.class); + .isInstanceOf(RLPException.class) + .hasMessageContaining(name); } } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/NewPooledTransactionHashesMessageProcessorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/NewPooledTransactionHashesMessageProcessorTest.java index 5c3f08ddfa1..556ccc677d2 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/NewPooledTransactionHashesMessageProcessorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/NewPooledTransactionHashesMessageProcessorTest.java @@ -18,9 +18,9 @@ import static java.time.Duration.ofMinutes; import static java.time.Instant.now; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.hyperledger.besu.ethereum.eth.encoding.TransactionAnnouncementDecoder.getDecoder; import static org.hyperledger.besu.ethereum.eth.encoding.TransactionAnnouncementEncoder.getEncoder; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Answers.RETURNS_DEEP_STUBS; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -64,7 +64,7 @@ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) -public class NewPooledTransactionHashesMessageProcessorTest { +class NewPooledTransactionHashesMessageProcessorTest { @Mock private TransactionPool transactionPool; @@ -105,7 +105,7 @@ public void setup() { } @Test - public void shouldAddInitiatedRequestingTransactions() { + void shouldAddInitiatedRequestingTransactions() { messageHandler.processNewPooledTransactionHashesMessage( peer1, @@ -120,7 +120,7 @@ public void shouldAddInitiatedRequestingTransactions() { } @Test - public void shouldNotAddAlreadyPresentTransactions() { + void shouldNotAddAlreadyPresentTransactions() { when(transactionPool.getTransactionByHash(hash1)).thenReturn(Optional.of(transaction1)); when(transactionPool.getTransactionByHash(hash2)).thenReturn(Optional.of(transaction2)); @@ -138,7 +138,7 @@ public void shouldNotAddAlreadyPresentTransactions() { } @Test - public void shouldAddInitiatedRequestingTransactionsWhenOutOfSync() { + void shouldAddInitiatedRequestingTransactionsWhenOutOfSync() { messageHandler.processNewPooledTransactionHashesMessage( peer1, @@ -149,7 +149,7 @@ public void shouldAddInitiatedRequestingTransactionsWhenOutOfSync() { } @Test - public void shouldNotMarkReceivedExpiredTransactionsAsSeen() { + void shouldNotMarkReceivedExpiredTransactionsAsSeen() { messageHandler.processNewPooledTransactionHashesMessage( peer1, NewPooledTransactionHashesMessage.create(transactionList, EthProtocol.ETH66), @@ -164,7 +164,7 @@ public void shouldNotMarkReceivedExpiredTransactionsAsSeen() { } @Test - public void shouldNotAddReceivedTransactionsToTransactionPoolIfExpired() { + void shouldNotAddReceivedTransactionsToTransactionPoolIfExpired() { messageHandler.processNewPooledTransactionHashesMessage( peer1, NewPooledTransactionHashesMessage.create(transactionList, EthProtocol.ETH66), @@ -179,8 +179,7 @@ public void shouldNotAddReceivedTransactionsToTransactionPoolIfExpired() { } @Test - public void - shouldScheduleGetPooledTransactionsTaskWhenNewTransactionAddedFromPeerForTheFirstTime() { + void shouldScheduleGetPooledTransactionsTaskWhenNewTransactionAddedFromPeerForTheFirstTime() { final EthScheduler ethScheduler = mock(EthScheduler.class); when(ethContext.getScheduler()).thenReturn(ethScheduler); @@ -198,7 +197,7 @@ public void shouldNotAddReceivedTransactionsToTransactionPoolIfExpired() { } @Test - public void shouldNotScheduleGetPooledTransactionsTaskTwice() { + void shouldNotScheduleGetPooledTransactionsTaskTwice() { messageHandler.processNewPooledTransactionHashesMessage( peer1, @@ -220,7 +219,7 @@ public void shouldNotScheduleGetPooledTransactionsTaskTwice() { } @Test - public void shouldCreateAndDecodeForEth66() { + void shouldCreateAndDecodeForEth66() { final List expectedAnnouncementList = transactionList.stream().map(TransactionAnnouncement::new).toList(); @@ -233,8 +232,8 @@ public void shouldCreateAndDecodeForEth66() { .pendingTransactions() .forEach( t -> { - assertThat(t.getSize().isPresent()).isFalse(); - assertThat(t.getType().isPresent()).isFalse(); + assertThat(t.getSize()).isEmpty(); + assertThat(t.getType()).isEmpty(); }); // assert all transaction hashes are the same as announcement message @@ -246,7 +245,7 @@ public void shouldCreateAndDecodeForEth66() { } @Test - public void shouldCreateAndDecodeForEth68() { + void shouldCreateAndDecodeForEth68() { final List expectedTransactions = transactionList.stream().map(TransactionAnnouncement::new).collect(Collectors.toList()); @@ -258,25 +257,25 @@ public void shouldCreateAndDecodeForEth68() { } @Test - public void shouldThrowRLPExceptionIfIncorrectVersion() { + void shouldThrowRLPExceptionIfIncorrectVersion() { // message for Eth/68 with 66 data should throw RLPException final NewPooledTransactionHashesMessage message66 = new NewPooledTransactionHashesMessage( getEncoder(EthProtocol.ETH68).encode(transactionList), EthProtocol.ETH66); // assert RLPException - assertThrows(RLPException.class, message66::pendingTransactions); + assertThatThrownBy(message66::pendingTransactions).isInstanceOf(RLPException.class); // message for Eth/66 with 68 data should throw RLPException final NewPooledTransactionHashesMessage message68 = new NewPooledTransactionHashesMessage( getEncoder(EthProtocol.ETH68).encode(transactionList), EthProtocol.ETH66); // assert RLPException - assertThrows(RLPException.class, message68::pendingTransactions); + assertThatThrownBy(message68::pendingTransactions).isInstanceOf(RLPException.class); } @Test - public void shouldEncodeTransactionsCorrectly_Eth68() { + void shouldEncodeTransactionsCorrectly_Eth68() { final String expected = "0xf86d83000102c3010203f863a00000000000000000000000000000000000000000000000000000000000000001a00000000000000000000000000000000000000000000000000000000000000002a00000000000000000000000000000000000000000000000000000000000000003"; @@ -297,7 +296,7 @@ public void shouldEncodeTransactionsCorrectly_Eth68() { } @Test - public void shouldDecodeBytesCorrectly_Eth68() { + void shouldDecodeBytesCorrectly_Eth68() { /* * [ * "0x0000102"] @@ -341,7 +340,7 @@ public void shouldDecodeBytesCorrectly_Eth68() { } @Test - public void shouldDecodeBytesCorrectly_PreviousImplementations_Eth68() { + void shouldDecodeBytesCorrectly_PreviousImplementations_Eth68() { /* * [ * "0x0000102"] @@ -385,7 +384,7 @@ public void shouldDecodeBytesCorrectly_PreviousImplementations_Eth68() { } @Test - public void shouldEncodeAndDecodeTransactionAnnouncement_Eth66() { + void shouldEncodeAndDecodeTransactionAnnouncement_Eth66() { final Transaction t1 = generator.transaction(TransactionType.FRONTIER); final Transaction t2 = generator.transaction(TransactionType.ACCESS_LIST); final Transaction t3 = generator.transaction(TransactionType.EIP1559); @@ -404,7 +403,7 @@ public void shouldEncodeAndDecodeTransactionAnnouncement_Eth66() { } @Test - public void shouldEncodeAndDecodeTransactionAnnouncement_Eth68() { + void shouldEncodeAndDecodeTransactionAnnouncement_Eth68() { final Transaction t1 = generator.transaction(TransactionType.FRONTIER); final Transaction t2 = generator.transaction(TransactionType.ACCESS_LIST); final Transaction t3 = generator.transaction(TransactionType.EIP1559); @@ -415,7 +414,7 @@ public void shouldEncodeAndDecodeTransactionAnnouncement_Eth68() { final List announcementList = getDecoder(EthProtocol.ETH68).decode(RLP.input(bytes)); - assertThat(announcementList.size()).isEqualTo(list.size()); + assertThat(announcementList).hasSameSizeAs(list); for (final Transaction transaction : list) { final TransactionAnnouncement announcement = announcementList.get(list.indexOf(transaction)); @@ -426,124 +425,91 @@ public void shouldEncodeAndDecodeTransactionAnnouncement_Eth68() { } @Test - public void shouldThrowInvalidArgumentExceptionWhenCreatingListsWithDifferentSizes() { - final Exception exception = - assertThrows( - IllegalArgumentException.class, - () -> - TransactionAnnouncement.create(new ArrayList<>(), List.of(1L), new ArrayList<>())); - final String expectedMessage = "Hashes, sizes and types must have the same number of elements"; - final String actualMessage = exception.getMessage(); - assertThat(actualMessage).isEqualTo(expectedMessage); + void shouldThrowInvalidArgumentExceptionWhenCreatingListsWithDifferentSizes() { + assertThatThrownBy( + () -> TransactionAnnouncement.create(new ArrayList<>(), List.of(1L), new ArrayList<>())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Hashes, sizes and types must have the same number of elements"); } @Test - public void shouldThrowInvalidArgumentExceptionWhenEncodingListsWithDifferentSizes() { - final Exception exception = - assertThrows( - IllegalArgumentException.class, + void shouldThrowInvalidArgumentExceptionWhenEncodingListsWithDifferentSizes() { + assertThatThrownBy( () -> TransactionAnnouncementEncoder.encodeForEth68( - new ArrayList<>(), List.of(1), new ArrayList<>())); - final String expectedMessage = "Hashes, sizes and types must have the same number of elements"; - final String actualMessage = exception.getMessage(); - assertThat(actualMessage).isEqualTo(expectedMessage); + new ArrayList<>(), List.of(1), new ArrayList<>())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Hashes, sizes and types must have the same number of elements"); } @Test @SuppressWarnings("UnusedVariable") - public void shouldThrowRLPExceptionWhenDecodingListsWithDifferentSizes() { + void shouldThrowRLPExceptionWhenDecodingListsWithDifferentSizes() { // ["0x000102",[],["0x881699519a25b0e32db9b1ba9981f3fbec93fbc0726c3e096af89e5ada2b1351"]] final Bytes invalidMessageBytes = Bytes.fromHexString( "0xe783000102c0e1a0881699519a25b0e32db9b1ba9981f3fbec93fbc0726c3e096af89e5ada2b1351"); - final Exception exception = - assertThrows( - RLPException.class, + assertThatThrownBy( () -> TransactionAnnouncementDecoder.getDecoder(EthProtocol.ETH68) - .decode(RLP.input(invalidMessageBytes))); - - final String expectedMessage = "Hashes, sizes and types must have the same number of elements"; - final String actualMessage = exception.getMessage(); - assertThat(actualMessage).isEqualTo(expectedMessage); + .decode(RLP.input(invalidMessageBytes))) + .isInstanceOf(RLPException.class) + .hasMessage("Hashes, sizes and types must have the same number of elements"); } @Test - public void shouldThrowRLPExceptionWhenTypeIsInvalid() { + void shouldThrowRLPExceptionWhenTypeIsInvalid() { final Bytes invalidMessageBytes = Bytes.fromHexString( // ["0x07",["0x00000002"],["0x881699519a25b0e32db9b1ba9981f3fbec93fbc0726c3e096af89e5ada2b1351"]] "0xe907c58400000002e1a0881699519a25b0e32db9b1ba9981f3fbec93fbc0726c3e096af89e5ada2b1351"); - final Exception exception = - assertThrows( - IllegalArgumentException.class, + assertThatThrownBy( () -> TransactionAnnouncementDecoder.getDecoder(EthProtocol.ETH68) - .decode(RLP.input(invalidMessageBytes))); - - final String expectedMessage = "Unsupported transaction type"; - final String actualMessage = exception.getMessage(); - assertThat(actualMessage).contains(expectedMessage); + .decode(RLP.input(invalidMessageBytes))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported transaction type"); } @Test - public void shouldThrowRLPExceptionWhenSizeSizeGreaterThanFourBytes() { + void shouldThrowRLPExceptionWhenSizeSizeGreaterThanFourBytes() { final Bytes invalidMessageBytes = Bytes.fromHexString( // ["0x02",["0xffffffff01"],["0x881699519a25b0e32db9b1ba9981f3fbec93fbc0726c3e096af89e5ada2b1351"]] "0xea02c685ffffffff00e1a0881699519a25b0e32db9b1ba9981f3fbec93fbc0726c3e096af89e5ada2b1351"); - final Exception exception = - assertThrows( - RLPException.class, + assertThatThrownBy( () -> TransactionAnnouncementDecoder.getDecoder(EthProtocol.ETH68) - .decode(RLP.input(invalidMessageBytes))); - - final String expectedMessage = "Expected max 4 bytes for unsigned int, but got 5 bytes"; - assertThat(exception) - .hasCauseInstanceOf(RLPException.class) - .cause() - .hasMessage(expectedMessage); + .decode(RLP.input(invalidMessageBytes))) + .isInstanceOf(RLPException.class) + .hasMessageContaining("Expected max 4 bytes for unsigned int, but got 5 bytes"); } @Test - public void shouldThrowNullPointerIfArgumentsAreNull() { + void shouldThrowNullPointerIfArgumentsAreNull() { final Hash hash = Hash.hash(Bytes.random(32)); - assertThat( - assertThrows(NullPointerException.class, () -> new TransactionAnnouncement((Hash) null)) - .getMessage()) - .isEqualTo("Hash cannot be null"); + assertThatThrownBy(() -> new TransactionAnnouncement((Hash) null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("Hash cannot be null"); - assertThat( - assertThrows( - NullPointerException.class, - () -> new TransactionAnnouncement(null, TransactionType.EIP1559, 0L)) - .getMessage()) - .isEqualTo("Hash cannot be null"); + assertThatThrownBy(() -> new TransactionAnnouncement(null, TransactionType.EIP1559, 0L)) + .isInstanceOf(NullPointerException.class) + .hasMessage("Hash cannot be null"); - assertThat( - assertThrows( - NullPointerException.class, () -> new TransactionAnnouncement(hash, null, 0L)) - .getMessage()) - .isEqualTo("Type cannot be null"); + assertThatThrownBy(() -> new TransactionAnnouncement(hash, null, 0L)) + .isInstanceOf(NullPointerException.class) + .hasMessage("Type cannot be null"); - assertThat( - assertThrows( - NullPointerException.class, - () -> new TransactionAnnouncement(hash, TransactionType.EIP1559, null)) - .getMessage()) - .isEqualTo("Size cannot be null"); + assertThatThrownBy(() -> new TransactionAnnouncement(hash, TransactionType.EIP1559, null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("Size cannot be null"); - assertThat( - assertThrows( - NullPointerException.class, - () -> new TransactionAnnouncement((Transaction) null)) - .getMessage()) - .isEqualTo("Transaction cannot be null"); + assertThatThrownBy(() -> new TransactionAnnouncement((Transaction) null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("Transaction cannot be null"); } } diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java index b75ae3224aa..796837ab85a 100644 --- a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java +++ b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java @@ -32,6 +32,8 @@ abstract class AbstractRLPInput implements RLPInput { + private static final String errorMessageSuffix = " (at bytes %d-%d: %s%s[%s]%s%s)"; + private final boolean lenient; protected long size; // The number of bytes in this rlp-encoded byte string @@ -73,7 +75,7 @@ protected void init(final long inputSize, final boolean shouldFitInputSizeExactl // input is corrupted. if (size > inputSize) { // Our error message include a snippet of the input and that code assume size is not set - // outside of the input, and that's exactly the case we're testing, so resetting the size + // outside the input, and that's exactly the case we're testing, so resetting the size // simply for the sake of the error being properly generated. final long itemEnd = size; size = inputSize; @@ -140,14 +142,13 @@ private void prepareCurrentItem() { currentPayloadSize = elementMetadata.payloadSize; } catch (final RLPException exception) { final String message = - String.format( - exception.getMessage() + getErrorMessageSuffix(), getErrorMessageSuffixParams()); + String.format(exception.getMessage() + errorMessageSuffix, getErrorMessageSuffixParams()); throw new RLPException(message, exception); } } private void validateCurrentItem() { - // Validate that a single byte SHORT_ELEMENT payload is not <= 0x7F. If it is, is should have + // Validate that a single byte SHORT_ELEMENT payload is not <= 0x7F. If it is, it should have // been written as a BYTE_ELEMENT. if (currentKind == RLPDecodingHelpers.Kind.SHORT_ELEMENT && currentPayloadSize == 1 @@ -211,11 +212,7 @@ private RLPException error(final Throwable cause, final String msg, final Object private String errorMsg(final String message, final Object... params) { return String.format( - message + getErrorMessageSuffix(), concatParams(params, getErrorMessageSuffixParams())); - } - - private String getErrorMessageSuffix() { - return " (at bytes %d-%d: %s%s[%s]%s%s)"; + message + errorMessageSuffix, concatParams(params, getErrorMessageSuffixParams())); } private Object[] getErrorMessageSuffixParams() { @@ -410,7 +407,7 @@ public InetAddress readInetAddress() { return InetAddress.getByAddress(address); } catch (final UnknownHostException e) { // InetAddress.getByAddress() only throws for an address of illegal length, and we have - // validated that length already, this this genuinely shouldn't throw. + // validated that length already, this genuinely shouldn't throw. throw new AssertionError(e); } } @@ -485,7 +482,7 @@ public int enterList(final boolean skipCount) { if (depth > endOfListOffset.length) { endOfListOffset = Arrays.copyOf(endOfListOffset, (endOfListOffset.length * 3) / 2); } - // The first list element is the beginning of the payload. It's end is the end of this item. + // The first list element is the beginning of the payload. Its end is the end of this item. final long listStart = currentPayloadOffset; final long listEnd = nextItem(); @@ -495,6 +492,12 @@ public int enterList(final boolean skipCount) { listEnd, size); } + if (depth > 1 && (listEnd > endOfListOffset[depth - 2])) { + throw corrupted( + "Invalid RLP item: list ends outside of enclosing list (inner: %d, outer: %d)", + listEnd, endOfListOffset[depth - 2]); + } + endOfListOffset[depth - 1] = listEnd; int count = -1; diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java index 808c8f74d54..2066b0ce701 100644 --- a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java +++ b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java @@ -41,9 +41,9 @@ * *

A {@link RLPInput} thus provides methods to decode both lists and binary values. A list in the * input is "entered" by calling {@link #enterList()} and left by calling {@link #leaveList()}. - * Binary values can be read directly with {@link #readBytes()} ()}, but the {@link RLPInput} - * interface provides a wealth of convenience methods to read specific types of data that are in - * specific encoding. + * Binary values can be read directly with {@link #readBytes()}, but the {@link RLPInput} interface + * provides a wealth of convenience methods to read specific types of data that are in specific + * encoding. * *

Amongst the methods to read binary data, some methods are provided to read "scalar". A scalar * should simply be understood as a positive integer that is encoded with no leading zeros. In other @@ -121,8 +121,8 @@ public interface RLPInput { * Exits the current list after all its items have been consumed. * *

Note that this method technically doesn't consume any input but must be called after having - * read the last element of a list. This allow to ensure the structure of the input is indeed the - * one expected. + * read the last element of a list. This allows it to ensure the structure of the input is indeed + * the one expected. * * @throws RLPException if the current list is not finished (it has more items). */ @@ -132,8 +132,8 @@ public interface RLPInput { * Exits the current list, ignoring any remaining unconsumed elements. * *

Note that this method technically doesn't consume any input but must be called after having - * read the last element of a list. This allow to ensure the structure of the input is indeed the - * one expected. + * read the last element of a list. This allows it to ensure the structure of the input is indeed + * the one expected. */ void leaveListLenient(); @@ -272,6 +272,7 @@ default long readUnsignedInt() { * fit an unsigned int or has leading zeros. */ long readUnsignedIntScalar(); + /** * Reads an inet address from this input. * @@ -374,10 +375,11 @@ default List readList(final Function valueReader) { for (int i = 0; i < size; i++) { try { res.add(valueReader.apply(this)); + } catch (final RLPException e) { + throw e; } catch (final Exception e) { throw new RLPException( - String.format( - "Error applying element decoding function on " + "element %d of the list", i), + String.format("Error applying element decoding function on element %d of the list", i), e); } } From ae4b49be24fc16b0279baab493233fd4c63458a2 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Wed, 22 Nov 2023 08:28:43 -0700 Subject: [PATCH 3/5] update tests Signed-off-by: Danno Ferrin --- .../jsonrpc/internal/methods/EthGetTransactionByHashTest.java | 3 +-- .../org/hyperledger/besu/evmtool/StateTestSubCommandTest.java | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java index b509499d775..42018f2aebc 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java @@ -22,7 +22,6 @@ import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.datatypes.Transaction; -import org.hyperledger.besu.datatypes.TransactionType; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse; @@ -210,7 +209,7 @@ private Set getTransactionPool() { final BlockDataGenerator gen = new BlockDataGenerator(); Transaction pendingTransaction = gen.transaction(); System.out.println(pendingTransaction.getHash()); - return gen.transactions(4, TransactionType.EIP1559).stream() + return gen.transactionsWithAllTypes(4).stream() .map(PendingTransaction.Local::new) .collect(Collectors.toUnmodifiableSet()); } diff --git a/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java b/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java index 00ac7d52ae0..0683823369e 100644 --- a/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java +++ b/ethereum/evmtool/src/test/java/org/hyperledger/besu/evmtool/StateTestSubCommandTest.java @@ -91,7 +91,7 @@ void testsInvalidTransactions() { final StateTestSubCommand stateTestSubCommand = new StateTestSubCommand(new EvmToolCommand(bais, new PrintWriter(baos, true, UTF_8))); stateTestSubCommand.run(); - assertThat(baos.toString(UTF_8)).contains("exceeds transaction sender account balance"); + assertThat(baos.toString(UTF_8)).contains("Upfront gas cost cannot exceed 2^256 Wei"); } @Test From e9fef8c53723b12bbd037e8369dfa43d7e40a9ac Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Mon, 27 Nov 2023 10:03:02 -0700 Subject: [PATCH 4/5] unsigned byte scalar Signed-off-by: Danno Ferrin --- .../core/encoding/AccessListTransactionDecoder.java | 2 +- .../ethereum/core/encoding/BlobTransactionDecoder.java | 2 +- .../core/encoding/EIP1559TransactionDecoder.java | 2 +- .../besu/ethereum/rlp/AbstractRLPInput.java | 10 ++++++++++ .../org/hyperledger/besu/ethereum/rlp/RLPInput.java | 10 ++++++++++ 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java index 502c566157e..e9fbf828174 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java @@ -61,7 +61,7 @@ public static Transaction decode(final RLPInput rlpInput) { accessListEntryRLPInput.leaveList(); return accessListEntry; })); - final byte recId = (byte) rlpInput.readUnsignedByte(); + final byte recId = (byte) rlpInput.readUnsignedByteScalar(); final Transaction transaction = preSignatureTransactionBuilder .signature( diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java index 3ee24c73dcf..13240930b94 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java @@ -90,7 +90,7 @@ static void readTransactionPayloadInner(final Transaction.Builder builder, final .versionedHashes( input.readList(versionedHashes -> new VersionedHash(versionedHashes.readBytes32()))); - final byte recId = (byte) input.readUnsignedByte(); + final byte recId = (byte) input.readUnsignedByteScalar(); builder.signature( SIGNATURE_ALGORITHM .get() diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java index c0e89486d93..6e419a9ed22 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java @@ -61,7 +61,7 @@ public static Transaction decode(final RLPInput input) { accessListEntryRLPInput.leaveList(); return accessListEntry; })); - final byte recId = (byte) input.readUnsignedByte(); + final byte recId = (byte) input.readUnsignedByteScalar(); final Transaction transaction = builder .signature( diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java index 796837ab85a..7e70ca7733e 100644 --- a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java +++ b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java @@ -325,6 +325,16 @@ public long readUnsignedIntScalar() { return readLongScalar(); } + @Override + public int readUnsignedByteScalar() { + checkScalar("unsigned byte scalar", 1); + if (currentPayloadSize == 0) { + return 0; + } else { + return payloadByte(0) & 0xff; + } + } + @Override public BigInteger readBigIntegerScalar() { checkScalar("arbitrary precision scalar"); diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java index 2066b0ce701..fba43e2a1c6 100644 --- a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java +++ b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java @@ -273,6 +273,16 @@ default long readUnsignedInt() { */ long readUnsignedIntScalar(); + /** + * Reads a scalar from the input and return is as an unsigned byte contained in an int + * + * @return The next scalar item of this input as an unsigned byte value as int + * @throws RLPException if the next item to read is a list, the input is at the end of its current + * list (and {@link #leaveList()} hasn't been called) or if the next item is either too big to + * fit an unsigned byte or has leading zeros. + */ + int readUnsignedByteScalar(); + /** * Reads an inet address from this input. * From 1ebc705f474b2ab8ccfd3ad8262f5b55752ef174 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Tue, 28 Nov 2023 07:07:47 -0700 Subject: [PATCH 5/5] next item Signed-off-by: Danno Ferrin --- .../ethereum/core/encoding/TransactionRLPDecoderTest.java | 2 +- .../hyperledger/besu/ethereum/rlp/AbstractRLPInput.java | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java index 80c6b1d4df5..19f3e5bf7a5 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java @@ -97,7 +97,7 @@ private static Collection dataTransactionSize() { }, { "9602d4013030308430303030803080c084303030013030", - "Cannot read a byte, expecting 1 bytes but current element is 4 bytes long", + "Cannot read a unsigned byte scalar, expecting a maximum of 1 bytes but current element is 4 bytes long", false } }); diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java index 7e70ca7733e..51a7f87e12e 100644 --- a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java +++ b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java @@ -328,11 +328,9 @@ public long readUnsignedIntScalar() { @Override public int readUnsignedByteScalar() { checkScalar("unsigned byte scalar", 1); - if (currentPayloadSize == 0) { - return 0; - } else { - return payloadByte(0) & 0xff; - } + int result = (currentPayloadSize == 0) ? 0 : payloadByte(0) & 0xff; + setTo(nextItem()); + return result; } @Override