Better Error Messages and Default ChainId For More Tx Types (#2401)

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
4 years ago · 7a125b3db9
parent 285d9cd947
commit 7a125b3db9
11 changed files with 60 additions and 24 deletions
--- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/graphql/AbstractEthGraphQLHttpServiceTest.java
+++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/graphql/AbstractEthGraphQLHttpServiceTest.java
@ -45,6 +45,7 @@ import org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason;
 import org.hyperledger.besu.ethereum.util.RawBlockIterator;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
 import org.hyperledger.besu.plugin.data.SyncStatus;
+import org.hyperledger.besu.plugin.data.TransactionType;
 import org.hyperledger.besu.testutil.BlockTestUtil;

 import java.net.URL;
@ -143,7 +144,11 @@ public abstract class AbstractEthGraphQLHttpServiceTest {
        .thenReturn(
            Collections.singleton(
                new PendingTransactions.TransactionInfo(
-                    Transaction.builder().nonce(42).gasLimit(654321).build(),
+                    Transaction.builder()
+                        .type(TransactionType.FRONTIER)
+                        .nonce(42)
+                        .gasLimit(654321)
+                        .build(),
                    true,
                    Instant.ofEpochSecond(Integer.MAX_VALUE))));

--- 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
@ -14,6 +14,7 @@
 */
 package org.hyperledger.besu.ethereum.core;

+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static org.hyperledger.besu.crypto.Hash.keccak256;

@ -146,18 +147,23 @@ public class Transaction implements org.hyperledger.besu.plugin.data.Transaction
      final Optional<BigInteger> chainId,
      final Optional<BigInteger> v) {
    if (v.isPresent() && chainId.isPresent()) {
-      throw new IllegalStateException(
+      throw new IllegalArgumentException(
          String.format("chainId '%s' and v '%s' cannot both be provided", chainId.get(), v.get()));
    }

+    if (transactionType.requiresChainId()) {
+      checkArgument(
+          chainId.isPresent(), "Chain id must be present for transaction type %s", transactionType);
+    }
+
    if (maybeAccessList.isPresent()) {
-      checkState(
+      checkArgument(
          transactionType.supportsAccessList(),
          "Must not specify access list for transaction not supporting it");
    }

    if (Objects.equals(transactionType, TransactionType.ACCESS_LIST)) {
-      checkState(
+      checkArgument(
          maybeAccessList.isPresent(), "Must specify access list for access list transaction");
    }

@ -587,6 +593,9 @@ public class Transaction implements org.hyperledger.besu.plugin.data.Transaction
      final Bytes payload,
      final Optional<List<AccessListEntry>> accessList,
      final Optional<BigInteger> chainId) {
+    if (transactionType.requiresChainId()) {
+      checkArgument(chainId.isPresent(), "Transaction type %s requires chainId", transactionType);
+    }
    final Bytes preimage;
    switch (transactionType) {
      case FRONTIER:
@ -878,7 +887,12 @@ public class Transaction implements org.hyperledger.besu.plugin.data.Transaction
      return this;
    }

+    public TransactionType getTransactionType() {
+      return transactionType;
+    }
+
    public Transaction build() {
+      if (transactionType == null) guessType();
      return new Transaction(
          transactionType,
          nonce,
--- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/TransactionEncoder.java
+++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/TransactionEncoder.java
@ -119,13 +119,7 @@ public class TransactionEncoder {
      final Bytes payload,
      final List<AccessListEntry> accessList,
      final RLPOutput rlpOutput) {
-    rlpOutput.writeLongScalar(
-        chainId
-            .orElseThrow(
-                () ->
-                    new IllegalArgumentException(
-                        "chainId is required for access list transactions"))
-            .longValue());
+    rlpOutput.writeLongScalar(chainId.orElseThrow().longValue());
    rlpOutput.writeLongScalar(nonce);
    rlpOutput.writeUInt256Scalar(gasPrice);
    rlpOutput.writeLongScalar(gasLimit);
@ -153,12 +147,7 @@ public class TransactionEncoder {

  static void encodeEIP1559(final Transaction transaction, final RLPOutput out) {
    out.startList();
-    out.writeLongScalar(
-        transaction
-            .getChainId()
-            .orElseThrow(
-                () -> new IllegalArgumentException("chainId is required for EIP-1559 transactions"))
-            .longValue());
+    out.writeLongScalar(transaction.getChainId().orElseThrow().longValue());
    out.writeLongScalar(transaction.getNonce());
    out.writeUInt256Scalar(
        transaction
--- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionSimulator.java
+++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionSimulator.java
@ -40,8 +40,8 @@ import org.hyperledger.besu.ethereum.vm.BlockHashLookup;
 import org.hyperledger.besu.ethereum.vm.OperationTracer;
 import org.hyperledger.besu.ethereum.worldstate.GoQuorumMutablePrivateAndPublicWorldStateUpdater;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
-import org.hyperledger.besu.plugin.data.TransactionType;

+import java.math.BigInteger;
 import java.util.Optional;

 import com.google.common.base.Supplier;
@ -172,7 +172,6 @@ public class TransactionSimulator {

    final Transaction.Builder transactionBuilder =
        Transaction.builder()
-            .type(TransactionType.FRONTIER)
            .nonce(nonce)
            .gasPrice(gasPrice)
            .gasLimit(gasLimit)
@ -184,6 +183,10 @@ public class TransactionSimulator {
    callParams.getMaxPriorityFeePerGas().ifPresent(transactionBuilder::maxPriorityFeePerGas);
    callParams.getMaxFeePerGas().ifPresent(transactionBuilder::maxFeePerGas);

+    transactionBuilder.guessType();
+    if (transactionBuilder.getTransactionType().requiresChainId()) {
+      transactionBuilder.chainId(BigInteger.ONE); // needed to make some transactions valid
+    }
    final Transaction transaction = transactionBuilder.build();

    final TransactionProcessingResult result =
--- a/ethereum/core/src/test-support/java/org/hyperledger/besu/ethereum/core/TestCodeExecutor.java
+++ b/ethereum/core/src/test-support/java/org/hyperledger/besu/ethereum/core/TestCodeExecutor.java
@ -23,6 +23,7 @@ import org.hyperledger.besu.ethereum.vm.Code;
 import org.hyperledger.besu.ethereum.vm.MessageFrame;
 import org.hyperledger.besu.ethereum.vm.OperationTracer;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
+import org.hyperledger.besu.plugin.data.TransactionType;

 import java.math.BigInteger;
 import java.util.ArrayDeque;
@ -56,6 +57,7 @@ public class TestCodeExecutor {

    final Transaction transaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .value(Wei.ZERO)
            .sender(SENDER_ADDRESS)
            .signature(
--- 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
@ -39,7 +39,7 @@ public class TransactionBuilderTest {

    final Set<TransactionType> guessedTypes =
        Stream.of(frontierBuilder, eip1559Builder, accessListBuilder)
-            .map(transactionBuilder -> transactionBuilder.guessType().build().getType())
+            .map(transactionBuilder -> transactionBuilder.guessType().getTransactionType())
            .collect(toUnmodifiableSet());

    assertThat(guessedTypes).containsExactlyInAnyOrder(TransactionType.values());
--- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/fees/TransactionPriceCalculatorTest.java
+++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/fees/TransactionPriceCalculatorTest.java
@ -23,6 +23,7 @@ import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.Wei;
 import org.hyperledger.besu.plugin.data.TransactionType;

+import java.math.BigInteger;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@ -128,6 +129,7 @@ public class TransactionPriceCalculatorTest {
                    .gasPrice(gasPrice)
                    .maxPriorityFeePerGas(maxPriorityFeePerGas)
                    .maxFeePerGas(maxFeePerGas)
+                    .chainId(BigInteger.ONE)
                    .build(),
                baseFee))
        .isEqualByComparingTo(expectedPrice);
--- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/transaction/TransactionSimulatorTest.java
+++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/transaction/TransactionSimulatorTest.java
@ -111,6 +111,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(callParameter.getGasPrice())
            .gasLimit(callParameter.getGasLimit())
@ -138,6 +139,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(callParameter.getGasPrice())
            .gasLimit(callParameter.getGasLimit())
@ -171,6 +173,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(callParameter.getGasPrice())
            .gasLimit(callParameter.getGasLimit())
@ -204,6 +207,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(Wei.ZERO)
            .gasLimit(0L)
@ -229,6 +233,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(0L)
            .gasPrice(Wei.ZERO)
            .gasLimit(0L)
@ -254,6 +259,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(callParameter.getGasPrice())
            .gasLimit(callParameter.getGasLimit())
@ -319,6 +325,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(Wei.ZERO)
            .gasLimit(0L)
@ -344,6 +351,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(0L)
            .gasPrice(Wei.ZERO)
            .gasLimit(0L)
@ -369,6 +377,7 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.FRONTIER)
            .nonce(1L)
            .gasPrice(callParameter.getGasPrice())
            .gasLimit(callParameter.getGasLimit())
@ -396,6 +405,8 @@ public class TransactionSimulatorTest {

    final Transaction expectedTransaction =
        Transaction.builder()
+            .type(TransactionType.EIP1559)
+            .chainId(BigInteger.ONE)
            .nonce(1L)
            .gasPrice(callParameter.getGasPrice())
            .gasLimit(callParameter.getGasLimit())
--- a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/StateTestVersionedTransaction.java
+++ b/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/StateTestVersionedTransaction.java
@ -135,12 +135,18 @@ public class StateTestVersionedTransaction {
            .to(to)
            .value(values.get(indexes.value))
            .payload(payloads.get(indexes.data));
+
    Optional.ofNullable(gasPrice).ifPresent(transactionBuilder::gasPrice);
    Optional.ofNullable(maxFeePerGas).ifPresent(transactionBuilder::maxFeePerGas);
    Optional.ofNullable(maxPriorityFeePerGas).ifPresent(transactionBuilder::maxPriorityFeePerGas);
    maybeAccessLists.ifPresent(
-        accessLists ->
-            transactionBuilder.accessList(accessLists.get(indexes.data)).chainId(BigInteger.ONE));
-    return transactionBuilder.guessType().signAndBuild(keys);
+        accessLists -> transactionBuilder.accessList(accessLists.get(indexes.data)));
+
+    transactionBuilder.guessType();
+    if (transactionBuilder.getTransactionType().requiresChainId()) {
+      transactionBuilder.chainId(BigInteger.ONE);
+    }
+
+    return transactionBuilder.signAndBuild(keys);
  }
 }
--- a/plugin-api/build.gradle
+++ b/plugin-api/build.gradle
@ -64,7 +64,7 @@ Calculated : ${currentHash}
 tasks.register('checkAPIChanges', FileStateChecker) {
  description = "Checks that the API for the Plugin-API project does not change without deliberate thought"
  files = sourceSets.main.allJava.files
-  knownHash = 'IFz5V3JG2AEZo9wCv6hEh5HugU5Afd0JfPe50cygvqo='
+  knownHash = 'U0V/Yrf6sGuSSgCu/T8GwhiEkABioLF+hb311yeSH9I='
 }
 check.dependsOn('checkAPIChanges')

--- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionType.java
+++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionType.java
@ -60,4 +60,8 @@ public enum TransactionType {
  public boolean supports1559FeeMarket() {
    return !LEGACY_FEE_MARKET_TRANSACTION_TYPES.contains(this);
  }
+
+  public boolean requiresChainId() {
+    return !this.equals(FRONTIER);
+  }
 }