Refactor some transaction calculations (#6150)

Refactor some transaction calculations, including moving upfront overflow checks from the constructor to the validator and optimizing some RLP calculations. Also fix all lint errors. Signed-off-by: Danno Ferrin <[email protected]>
hyperledger · Nov 29, 2023 · 3765aaf · 3765aaf
1 parent fa8751c
commit 3765aaf
Show file tree

Hide file tree

Showing 12 changed files with 183 additions and 179 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
@@ -99,7 +99,7 @@ public class Transaction
   private final Optional<BigInteger> 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;
@@ -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.
    *
@@ -597,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());
@@ -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/...c/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java b/...c/main/java/org/hyperledger/besu/ethereum/core/encoding/AccessListTransactionDecoder.java
@@ -32,6 +32,10 @@ class AccessListTransactionDecoder {
   private static final Supplier<SignatureAlgorithm> 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.readUnsignedByteScalar();
     final Transaction transaction =
         preSignatureTransactionBuilder
             .signature(

diff --git a/...ore/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java b/...ore/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionDecoder.java
@@ -32,6 +32,10 @@ public class BlobTransactionDecoder {
   private static final Supplier<SignatureAlgorithm> 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.readUnsignedByteScalar();
     builder.signature(
         SIGNATURE_ALGORITHM
             .get()

diff --git a/.../src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java b/.../src/main/java/org/hyperledger/besu/ethereum/core/encoding/EIP1559TransactionDecoder.java
@@ -32,6 +32,10 @@ public class EIP1559TransactionDecoder {
   private static final Supplier<SignatureAlgorithm> 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.readUnsignedByteScalar();
     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
@@ -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/...core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java b/...core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionValidator.java
@@ -182,6 +182,13 @@ private ValidationResult<TransactionInvalidReason> 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/...ore/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionInvalidReason.java b/...ore/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/.../src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java b/.../src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java
@@ -86,8 +86,18 @@ private static Collection<Object[]> 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 unsigned byte scalar, expecting a maximum of 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);
     }
   }