Block, Tx: Caching for hash method (#1445)

* block: caching for hash method * tx: caching for hash method * block, tx: updates to caching for hash method * block, tx: updates to caching for hash method * block: refactor caching mechanism in block header * block: lint fixes * block: hash caching refactor * tx: hash caching refactor * tx: test for hash throwing on unsigned tx * tx: updated tests for hash caching * tx: revert throwing when hash() is called on unsigned legacy txs due to eventual compatibility issues, added longer explanatory comment Co-authored-by: holgerd77 <[email protected]>
ethereumjs · Sep 8, 2021 · 46797fe · 46797fe
1 parent 2e8dff0
commit 46797fe
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 5 deletions.
diff --git a/packages/block/src/header.ts b/packages/block/src/header.ts
@@ -14,14 +14,18 @@ import {
   toBuffer,
   zeros,
 } from 'ethereumjs-util'
-import { HeaderData, JsonHeader, BlockHeaderBuffer, Blockchain, BlockOptions } from './types'
+import { Blockchain, BlockHeaderBuffer, BlockOptions, HeaderData, JsonHeader } from './types'
 import {
   CLIQUE_EXTRA_VANITY,
   CLIQUE_EXTRA_SEAL,
   CLIQUE_DIFF_INTURN,
   CLIQUE_DIFF_NOTURN,
 } from './clique'
 
+interface HeaderCache {
+  hash: Buffer | undefined
+}
+
 const DEFAULT_GAS_LIMIT = new BN(Buffer.from('ffffffffffffff', 'hex'))
 
 /**
@@ -48,6 +52,10 @@ export class BlockHeader {
   public readonly _common: Common
   public _errorPostfix = ''
 
+  private cache: HeaderCache = {
+    hash: undefined,
+  }
+
   /**
    * Static constructor to create a block header from a header data dictionary
    *
@@ -725,6 +733,13 @@ export class BlockHeader {
    * Returns the hash of the block header.
    */
   hash(): Buffer {
+    if (Object.isFrozen(this)) {
+      if (!this.cache.hash) {
+        this.cache.hash = rlphash(this.raw())
+      }
+      return this.cache.hash
+    }
+
     return rlphash(this.raw())
   }
 

diff --git a/packages/block/src/types.ts b/packages/block/src/types.ts
@@ -52,6 +52,7 @@ export interface BlockOptions {
   /**
    * A block object by default gets frozen along initialization. This gives you
    * strong additional security guarantees on the consistency of the block parameters.
+   * It also enables block hash caching when the `hash()` method is called multiple times.
    *
    * If you need to deactivate the block freeze - e.g. because you want to subclass block and
    * add aditional properties - it is strongly encouraged that you do the freeze yourself

diff --git a/packages/tx/src/baseTransaction.ts b/packages/tx/src/baseTransaction.ts
@@ -21,6 +21,10 @@ import {
   Capability,
 } from './types'
 
+interface TransactionCache {
+  hash: Buffer | undefined
+}
+
 /**
  * This base class will likely be subject to further
  * refactoring along the introduction of additional tx types
@@ -43,6 +47,10 @@ export abstract class BaseTransaction<TransactionObject> {
 
   public readonly common!: Common
 
+  protected cache: TransactionCache = {
+    hash: undefined,
+  }
+
   /**
    * List of tx type defining EIPs,
    * e.g. 1559 (fee market) and 2930 (access lists)

diff --git a/packages/tx/src/eip1559Transaction.ts b/packages/tx/src/eip1559Transaction.ts
@@ -330,6 +330,13 @@ export default class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMark
       throw new Error('Cannot call hash method if transaction is not signed')
     }
 
+    if (Object.isFrozen(this)) {
+      if (!this.cache.hash) {
+        this.cache.hash = keccak256(this.serialize())
+      }
+      return this.cache.hash
+    }
+
     return keccak256(this.serialize())
   }
 

diff --git a/packages/tx/src/eip2930Transaction.ts b/packages/tx/src/eip2930Transaction.ts
@@ -300,6 +300,13 @@ export default class AccessListEIP2930Transaction extends BaseTransaction<Access
       throw new Error('Cannot call hash method if transaction is not signed')
     }
 
+    if (Object.isFrozen(this)) {
+      if (!this.cache.hash) {
+        this.cache.hash = keccak256(this.serialize())
+      }
+      return this.cache.hash
+    }
+
     return keccak256(this.serialize())
   }
 

diff --git a/packages/tx/src/legacyTransaction.ts b/packages/tx/src/legacyTransaction.ts
@@ -232,6 +232,27 @@ export default class Transaction extends BaseTransaction<Transaction> {
    * Use {@link Transaction.getMessageToSign} to get a tx hash for the purpose of signing.
    */
   hash(): Buffer {
+    // In contrast to the tx type transaction implementations the `hash()` function
+    // for the legacy tx does not throw if the tx is not signed.
+    // This has been considered for inclusion but lead to unexpected backwards
+    // compatibility problems (no concrete reference found, needs validation).
+    //
+    // For context see also https://github.com/ethereumjs/ethereumjs-monorepo/pull/1445,
+    // September, 2021 as well as work done before.
+    //
+    // This should be updated along the next major version release by adding:
+    //
+    //if (!this.isSigned()) {
+    //  throw new Error('Cannot call hash method if transaction is not signed')
+    //}
+
+    if (Object.isFrozen(this)) {
+      if (!this.cache.hash) {
+        this.cache.hash = rlphash(this.raw())
+      }
+      return this.cache.hash
+    }
+
     return rlphash(this.raw())
   }
 

diff --git a/packages/tx/src/types.ts b/packages/tx/src/types.ts
@@ -52,6 +52,7 @@ export interface TxOptions {
   /**
    * A transaction object by default gets frozen along initialization. This gives you
    * strong additional security guarantees on the consistency of the tx parameters.
+   * It also enables tx hash caching when the `hash()` method is called multiple times.
    *
    * If you need to deactivate the tx freeze - e.g. because you want to subclass tx and
    * add aditional properties - it is strongly encouraged that you do the freeze yourself

diff --git a/packages/tx/test/eip1559.spec.ts b/packages/tx/test/eip1559.spec.ts
@@ -55,13 +55,16 @@ tape('[FeeMarketEIP1559Transaction]', function (t) {
   t.test('hash()', function (st) {
     const data = testdata[0]
     const pkey = Buffer.from(data.privateKey.slice(2), 'hex')
-    const txn = FeeMarketEIP1559Transaction.fromTxData(data, { common })
-    const signed = txn.sign(pkey)
+    let txn = FeeMarketEIP1559Transaction.fromTxData(data, { common })
+    let signed = txn.sign(pkey)
     const expectedHash = Buffer.from(
       '2e564c87eb4b40e7f469b2eec5aa5d18b0b46a24e8bf0919439cfb0e8fcae446',
       'hex'
     )
-    st.ok(signed.hash().equals(expectedHash), 'Should provide the correct hash')
+    st.ok(signed.hash().equals(expectedHash), 'Should provide the correct hash when frozen')
+    txn = FeeMarketEIP1559Transaction.fromTxData(data, { common, freeze: false })
+    signed = txn.sign(pkey)
+    st.ok(signed.hash().equals(expectedHash), 'Should provide the correct hash when not frozen')
     st.end()
   })
 

diff --git a/packages/tx/test/legacy.spec.ts b/packages/tx/test/legacy.spec.ts
@@ -173,7 +173,22 @@ tape('[Transaction]', function (t) {
       chain: Chain.Mainnet,
       hardfork: Hardfork.TangerineWhistle,
     })
-    const tx = Transaction.fromValuesArray(txFixtures[3].raw.map(toBuffer), {
+    // Test currently commented out, see comment on legacy tx hash() function
+    /*let tx = Transaction.fromValuesArray(txFixtures[3].raw.slice(0, 6).map(toBuffer), {
+      common,
+    })
+    st.throws(() => {
+      tx.hash()
+    }, 'should throw calling hash with unsigned tx')*/
+    let tx = Transaction.fromValuesArray(txFixtures[3].raw.map(toBuffer), {
+      common,
+      freeze: false,
+    })
+    st.deepEqual(
+      tx.hash(),
+      Buffer.from('375a8983c9fc56d7cfd118254a80a8d7403d590a6c9e105532b67aca1efb97aa', 'hex')
+    )
+    tx = Transaction.fromValuesArray(txFixtures[3].raw.map(toBuffer), {
       common,
     })
     st.deepEqual(