tx: simplify validate methods (ethereumjs#2792)

* tx: remove validate method and add isValid and getValidationErrors * tx: update usage of validate() * vm: update usage of tx.validate() * block: update usage of tx.validate() and unify validation method naming pattern * devp2p: update usage of validation methods * block: add missing await * block: fix tests * tx: split getMessageToSign into two methods * tx: split getMessageToSign into two methods * tx: update tx tests * tx: update docs * tx: refactor isValid logic * block: refactor transactionsAreValid logic * client: fix test --------- Co-authored-by: Holger Drewes <[email protected]>
wighawag · Jun 23, 2023 · c51d81e · c51d81e
1 parent 91f7c9d
commit c51d81e
Show file tree

Hide file tree

Showing 28 changed files with 189 additions and 148 deletions.
diff --git a/packages/block/src/block.ts b/packages/block/src/block.ts
@@ -447,8 +447,9 @@ export class Block {
   /**
    * Validates the transaction trie by generating a trie
    * and do a check on the root hash.
+   * @returns True if the transaction trie is valid, false otherwise
    */
-  async validateTransactionsTrie(): Promise<boolean> {
+  async transactionsTrieIsValid(): Promise<boolean> {
     let result
     if (this.transactions.length === 0) {
       result = equalsBytes(this.header.transactionsTrie, KECCAK256_RLP)
@@ -464,21 +465,17 @@ export class Block {
 
   /**
    * Validates transaction signatures and minimum gas requirements.
-   *
-   * @param stringError - If `true`, a string with the indices of the invalid txs is returned.
+   * @returns {string[]} an array of error strings
    */
-  validateTransactions(): boolean
-  validateTransactions(stringError: false): boolean
-  validateTransactions(stringError: true): string[]
-  validateTransactions(stringError = false) {
+  getTransactionsValidationErrors(): string[] {
     const errors: string[] = []
     let dataGasUsed = BigInt(0)
     const dataGasLimit = this._common.param('gasConfig', 'maxDataGasPerBlock')
     const dataGasPerBlob = this._common.param('gasConfig', 'dataGasPerBlob')
 
     // eslint-disable-next-line prefer-const
     for (let [i, tx] of this.transactions.entries()) {
-      const errs = <string[]>tx.validate(true)
+      const errs = tx.getValidationErrors()
       if (this._common.isActivatedEIP(1559) === true) {
         if (tx.supports(Capability.EIP1559FeeMarket)) {
           tx = tx as FeeMarketEIP1559Transaction
@@ -513,7 +510,17 @@ export class Block {
       }
     }
 
-    return stringError ? errors : errors.length === 0
+    return errors
+  }
+
+  /**
+   * Validates transaction signatures and minimum gas requirements.
+   * @returns True if all transactions are valid, false otherwise
+   */
+  transactionsAreValid(): boolean {
+    const errors = this.getTransactionsValidationErrors()
+
+    return errors.length === 0
   }
 
   /**
@@ -526,7 +533,7 @@ export class Block {
    * @param onlyHeader if only passed the header, skip validating txTrie and unclesHash (default: false)
    */
   async validateData(onlyHeader: boolean = false): Promise<void> {
-    const txErrors = this.validateTransactions(true)
+    const txErrors = this.getTransactionsValidationErrors()
     if (txErrors.length > 0) {
       const msg = this._errorMsg(`invalid transactions: ${txErrors.join(' ')}`)
       throw new Error(msg)
@@ -536,18 +543,17 @@ export class Block {
       return
     }
 
-    const validateTxTrie = await this.validateTransactionsTrie()
-    if (!validateTxTrie) {
+    if (!(await this.transactionsTrieIsValid())) {
       const msg = this._errorMsg('invalid transaction trie')
       throw new Error(msg)
     }
 
-    if (!this.validateUnclesHash()) {
+    if (!this.uncleHashIsValid()) {
       const msg = this._errorMsg('invalid uncle hash')
       throw new Error(msg)
     }
 
-    if (this._common.isActivatedEIP(4895) && !(await this.validateWithdrawalsTrie())) {
+    if (this._common.isActivatedEIP(4895) && !(await this.withdrawalsTrieIsValid())) {
       const msg = this._errorMsg('invalid withdrawals trie')
       throw new Error(msg)
     }
@@ -603,17 +609,19 @@ export class Block {
 
   /**
    * Validates the uncle's hash.
+   * @returns true if the uncle's hash is valid, false otherwise.
    */
-  validateUnclesHash(): boolean {
+  uncleHashIsValid(): boolean {
     const uncles = this.uncleHeaders.map((uh) => uh.raw())
     const raw = RLP.encode(uncles)
     return equalsBytes(keccak256(raw), this.header.uncleHash)
   }
 
   /**
    * Validates the withdrawal root
+   * @returns true if the withdrawals trie root is valid, false otherwise
    */
-  async validateWithdrawalsTrie(): Promise<boolean> {
+  async withdrawalsTrieIsValid(): Promise<boolean> {
     if (!this._common.isActivatedEIP(4895)) {
       throw new Error('EIP 4895 is not activated')
     }

diff --git a/packages/block/test/block.spec.ts b/packages/block/test/block.spec.ts
@@ -168,8 +168,8 @@ describe('[Block]: block functions', () => {
   })
 
   async function testTransactionValidation(block: Block) {
-    assert.ok(block.validateTransactions())
-    assert.ok(await block.validateTransactionsTrie())
+    assert.ok(block.transactionsAreValid())
+    assert.ok(block.getTransactionsValidationErrors().length === 0)
   }
 
   it('should test transaction validation', async () => {
@@ -197,7 +197,7 @@ describe('[Block]: block functions', () => {
     const block = Block.fromRLPSerializedBlock(blockRlp, { common, freeze: false })
     await testTransactionValidation(block)
     ;(block.transactions[0] as any).gasPrice = BigInt(0)
-    const result = block.validateTransactions(true)
+    const result = block.getTransactionsValidationErrors()
     assert.ok(
       result[0].includes('tx unable to pay base fee (non EIP-1559 tx)'),
       'should throw when legacy tx is unable to pay base fee'
@@ -208,7 +208,7 @@ describe('[Block]: block functions', () => {
     const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Istanbul })
     const blockRlp = toBytes(testDataPreLondon2.blocks[2].rlp)
     const block = Block.fromRLPSerializedBlock(blockRlp, { common, freeze: false })
-    assert.equal(block.validateUnclesHash(), true)
+    assert.equal(block.uncleHashIsValid(), true)
     ;(block.header as any).uncleHash = new Uint8Array(32)
     try {
       await block.validateData()

diff --git a/packages/block/test/eip1559block.spec.ts b/packages/block/test/eip1559block.spec.ts
@@ -442,7 +442,7 @@ describe('EIP1559 tests', () => {
       }
     )
 
-    const errs = block.validateTransactions(true)
+    const errs = block.getTransactionsValidationErrors()
     assert.ok(
       errs[0].includes('unable to pay base fee'),
       'should throw if transaction is unable to pay base fee'

diff --git a/packages/block/test/eip4844block.spec.ts b/packages/block/test/eip4844block.spec.ts
@@ -250,7 +250,7 @@ describe('transaction validation tests', () => {
 
       assert.ok(
         blockWithTooManyBlobs
-          .validateTransactions(true)
+          .getTransactionsValidationErrors()
           .join(' ')
           .includes('exceed maximum data gas per block'),
         'tx erros includes correct error message when too many blobs in a block'

diff --git a/packages/block/test/eip4895block.spec.ts b/packages/block/test/eip4895block.spec.ts
@@ -130,7 +130,7 @@ describe('EIP4895 tests', () => {
       }
     )
     assert.notOk(
-      await block.validateWithdrawalsTrie(),
+      await block.withdrawalsTrieIsValid(),
       'should invalidate the empty withdrawals root'
     )
     const validHeader = BlockHeader.fromHeaderData(
@@ -148,7 +148,7 @@ describe('EIP4895 tests', () => {
         common,
       }
     )
-    assert.ok(await validBlock.validateWithdrawalsTrie(), 'should validate empty withdrawals root')
+    assert.ok(await validBlock.withdrawalsTrieIsValid(), 'should validate empty withdrawals root')
 
     const withdrawal = <WithdrawalData>{
       index: BigInt(0),
@@ -171,7 +171,7 @@ describe('EIP4895 tests', () => {
       }
     )
     assert.ok(
-      await validBlockWithWithdrawal.validateWithdrawalsTrie(),
+      await validBlockWithWithdrawal.withdrawalsTrieIsValid(),
       'should validate withdrawals root'
     )
 
@@ -196,7 +196,7 @@ describe('EIP4895 tests', () => {
       }
     )
     assert.ok(
-      await validBlockWithWithdrawal2.validateWithdrawalsTrie(),
+      await validBlockWithWithdrawal2.withdrawalsTrieIsValid(),
       'should validate withdrawals root'
     )
     assert.doesNotThrow(() => {

diff --git a/packages/block/test/from-rpc.spec.ts b/packages/block/test/from-rpc.spec.ts
@@ -96,13 +96,13 @@ describe('[fromRPC]:', () => {
   it('should create a block with uncles', () => {
     const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Istanbul })
     const block = blockFromRpc(blockDataWithUncles, [uncleBlockData], { common })
-    assert.ok(block.validateUnclesHash())
+    assert.ok(block.uncleHashIsValid())
   })
 
   it('should create a block with EIP-4896 withdrawals', () => {
     const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Shanghai })
     const block = blockFromRpc(blockDataWithWithdrawals, [], { common })
-    assert.ok(block.validateWithdrawalsTrie())
+    assert.ok(block.withdrawalsTrieIsValid())
   })
 
   it('should create a block header with the correct hash when EIP-4896 withdrawals are present', () => {

diff --git a/packages/client/test/rpc/eth/getStorageAt.spec.ts b/packages/client/test/rpc/eth/getStorageAt.spec.ts
@@ -32,7 +32,7 @@ tape(`${method}: call with valid arguments`, async (t) => {
   // construct block with tx
   const gasLimit = 2000000
   const tx = LegacyTransaction.fromTxData({ gasLimit, data }, { common, freeze: false })
-  const signedTx = tx.sign(tx.getMessageToSign())
+  const signedTx = tx.sign(tx.getHashedMessageToSign())
 
   const parent = await chain.blockchain.getCanonicalHeadHeader()
   const block = Block.fromBlockData(

diff --git a/packages/devp2p/examples/peer-communication-les.cts b/packages/devp2p/examples/peer-communication-les.cts
@@ -211,15 +211,11 @@ function onNewBlock(block: Block, peer: Peer) {
   )
 }
 
-function isValidTx(tx: TypedTransaction) {
-  return tx.validate()
-}
-
 async function isValidBlock(block: Block) {
   return (
-    block.validateUnclesHash() &&
-    block.transactions.every(isValidTx) &&
-    block.validateTransactionsTrie()
+    block.uncleHashIsValid() &&
+    block.transactions.every(({ isValid }) => isValid()) &&
+    block.transactionsTrieIsValid()
   )
 }
 

diff --git a/packages/devp2p/examples/peer-communication.cts b/packages/devp2p/examples/peer-communication.cts
@@ -136,7 +136,7 @@ rlpx.on('peer:added', (peer) => {
 
         for (const item of payload) {
           const tx = TransactionFactory.fromBlockBodyData(item)
-          if (isValidTx(tx)) onNewTx(tx, peer)
+          if (tx.isValid()) onNewTx(tx, peer)
         }
 
         break
@@ -352,15 +352,11 @@ function onNewBlock(block: Block, peer: Peer) {
   for (const tx of block.transactions) onNewTx(tx, peer)
 }
 
-function isValidTx(tx: TypedTransaction) {
-  return tx.validate()
-}
-
 async function isValidBlock(block: Block) {
   return (
-    block.validateUnclesHash() &&
-    block.transactions.every(isValidTx) &&
-    block.validateTransactionsTrie()
+    block.uncleHashIsValid() &&
+    block.transactions.every(({ isValid }) => isValid()) &&
+    block.transactionsTrieIsValid()
   )
 }
 

diff --git a/packages/tx/README.md b/packages/tx/README.md
@@ -340,7 +340,7 @@ const common = Common.custom({ chainId: 1234 })
 
 ### Signing with a hardware or external wallet
 
-To sign a tx with a hardware or external wallet use `tx.getMessageToSign(false)` to return an [EIP-155](https://eips.ethereum.org/EIPS/eip-155) compliant unsigned tx.
+To sign a tx with a hardware or external wallet use `tx.getMessageToSign()` to return an [EIP-155](https://eips.ethereum.org/EIPS/eip-155) compliant unsigned tx.
 
 A legacy transaction will return a Buffer list of the values, and a Typed Transaction ([EIP-2718](https://eips.ethereum.org/EIPS/eip-2718)) will return the serialized output.
 
@@ -365,7 +365,7 @@ const bip32Path = "44'/60'/0'/0/0"
 const run = async () => {
   // Signing a legacy tx
   tx = LegacyTransaction.fromTxData(txData, { common })
-  unsignedTx = tx.getMessageToSign(false)
+  unsignedTx = tx.getMessageToSign()
   unsignedTx = Buffer.from(RLP.encode(bufArrToArr(unsignedTx))) // ledger signTransaction API expects it to be serialized
   let { v, r, s } = await eth.signTransaction(bip32Path, unsignedTx)
   txData = { ...txData, v, r, s }
@@ -376,7 +376,7 @@ const run = async () => {
   // Signing a 1559 tx
   txData = { value: 1 }
   tx = FeeMarketEIP1559Transaction.fromTxData(txData, { common })
-  unsignedTx = tx.getMessageToSign(false)
+  unsignedTx = tx.getMessageToSign()
   ;({ v, r, s } = await eth.signTransaction(bip32Path, unsignedTx)) // this syntax is: object destructuring - assignment without declaration
   txData = { ...txData, v, r, s }
   signedTx = FeeMarketEIP1559Transaction.fromTxData(txData, { common })

diff --git a/packages/tx/docs/classes/AccessListEIP2930Transaction.md b/packages/tx/docs/classes/AccessListEIP2930Transaction.md
@@ -378,7 +378,7 @@ Note: in contrast to the legacy tx the raw message format is already
 serialized and doesn't need to be RLP encoded any more.
 
 ```javascript
-const serializedMessage = tx.getMessageToSign(false) // use this for the HW wallet input
+const serializedMessage = tx.getMessageToSign() // use this for the HW wallet input
 ```
 
 #### Parameters

diff --git a/packages/tx/docs/classes/FeeMarketEIP1559Transaction.md b/packages/tx/docs/classes/FeeMarketEIP1559Transaction.md
@@ -389,7 +389,7 @@ Note: in contrast to the legacy tx the raw message format is already
 serialized and doesn't need to be RLP encoded any more.
 
 ```javascript
-const serializedMessage = tx.getMessageToSign(false) // use this for the HW wallet input
+const serializedMessage = tx.getMessageToSign() // use this for the HW wallet input
 ```
 
 #### Parameters

diff --git a/packages/tx/docs/classes/Transaction.md b/packages/tx/docs/classes/Transaction.md
@@ -317,7 +317,7 @@ and you might need to do yourself with:
 ```javascript
 import { bufArrToArr } from '@ethereumjs/util'
 import { RLP } from '@ethereumjs/rlp'
-const message = tx.getMessageToSign(false)
+const message = tx.getMessageToSign()
 const serializedMessage = Buffer.from(RLP.encode(bufArrToArr(message))) // use this for the HW wallet input
 ```
 

diff --git a/packages/tx/examples/custom-chain-tx.cts b/packages/tx/examples/custom-chain-tx.cts
@@ -40,7 +40,7 @@ const privateKey = hexToBytes('e331b6d69882b4cb4ea581d88e0b604039a3de5967688d3dc
 const signedTx = tx.sign(privateKey)
 const address = Address.fromPrivateKey(privateKey)
 
-if (signedTx.validate() && signedTx.getSenderAddress().equals(address)) {
+if (signedTx.isValid() && signedTx.getSenderAddress().equals(address)) {
   console.log('Valid signature')
 } else {
   console.log('Invalid signature')

diff --git a/packages/tx/examples/ropsten-tx.cts b/packages/tx/examples/ropsten-tx.cts
@@ -10,7 +10,7 @@ const common = new Common({ chain: Chain.Ropsten, hardfork: Hardfork.Petersburg
 const tx = LegacyTransaction.fromSerializedTx(txData, { common })
 
 if (
-  tx.validate() &&
+  tx.isValid() &&
   tx.getSenderAddress().toString() === '0x9dfd2d2b2ed960923f7bf2e8883d73f213f3b24b'
 ) {
   console.log('Correctly created the tx')