Merge pull request #1088 from ethereumjs/client-block-poa-fixes

Client/Block: Clique PoA fixes

Author: holgerd77

packages/block/src/block.ts

@@ -25,7 +25,11 @@ export class Block {
     // parse transactions
     const transactions = []
     for (const txData of txsData || []) {
-      const tx = Transaction.fromTxData(txData, opts as TxOptions)
+      const tx = Transaction.fromTxData(txData, {
+        ...opts,
+        // Use header common in case of hardforkByBlockNumber being activated
+        common: header._common,
+      } as TxOptions)
       transactions.push(tx)
     }
 
@@ -34,7 +38,10 @@ export class Block {
     for (const uhData of uhsData || []) {
       const uh = BlockHeader.fromHeaderData(uhData, {
         ...opts,
-        // Disable this option here (all other options carried over), since this overwrites the provided Difficulty to an incorrect value
+        // Use header common in case of hardforkByBlockNumber being activated
+        common: header._common,
+        // Disable this option here (all other options carried over), since this overwrites
+        // the provided Difficulty to an incorrect value
         calcDifficultyFromHeader: undefined,
       })
       uncleHeaders.push(uh)
@@ -65,7 +72,13 @@ export class Block {
     // parse transactions
     const transactions = []
     for (const txData of txsData || []) {
-      transactions.push(Transaction.fromValuesArray(txData, opts))
+      transactions.push(
+        Transaction.fromValuesArray(txData, {
+          ...opts,
+          // Use header common in case of hardforkByBlockNumber being activated
+          common: header._common,
+        })
+      )
     }
 
     // parse uncle headers
@@ -74,6 +87,8 @@ export class Block {
       uncleHeaders.push(
         BlockHeader.fromValuesArray(uncleHeaderData, {
           ...opts,
+          // Use header common in case of hardforkByBlockNumber being activated
+          common: header._common,
           // Disable this option here (all other options carried over), since this overwrites the provided Difficulty to an incorrect value
           calcDifficultyFromHeader: undefined,
         })
@@ -263,7 +278,8 @@ export class Block {
   async validateData(): Promise<void> {
     const txErrors = this.validateTransactions(true)
     if (txErrors.length > 0) {
-      throw new Error(`invalid transactions: ${txErrors.join(' ')}`)
+      const msg = `invalid transactions: ${txErrors.join(' ')}`
+      throw this.header._error(msg)
     }
 
     const validateTxTrie = await this.validateTransactionsTrie()
@@ -358,6 +374,16 @@ export class Block {
     }
   }
 
+  /**
+   * Internal helper function to create an annotated error message
+   *
+   * @param msg Base error message
+   * @hidden
+   */
+  _error(msg: string) {
+    return this.header._error(msg)
+  }
+
   /**
    * The following rules are checked in this method:
    * Uncle Header is a valid header.

packages/block/src/header.ts

@@ -46,6 +46,7 @@ export class BlockHeader {
   public readonly nonce: Buffer
 
   public readonly _common: Common
+  public _errorPostfix = ''
 
   public static fromHeaderData(headerData: HeaderData = {}, opts?: BlockOptions) {
     const {
@@ -172,7 +173,10 @@ export class BlockHeader {
     options: BlockOptions = {}
   ) {
     if (options.common) {
-      this._common = options.common
+      this._common = Object.assign(
+        Object.create(Object.getPrototypeOf(options.common)),
+        options.common
+      )
     } else {
       const chain = 'mainnet' // default
       if (options.initWithGenesisHeader) {
@@ -245,6 +249,10 @@ export class BlockHeader {
       this.extraData = this.cliqueSealBlock(options.cliqueSigner)
     }
 
+    this._errorPostfix = `block number=${this.number.toNumber()} hash=${this.hash().toString(
+      'hex'
+    )}`
+
     const freeze = options?.freeze ?? true
     if (freeze) {
       Object.freeze(this)
@@ -455,37 +463,37 @@ export class BlockHeader {
       if (
         this.extraData.length > this._common.paramByHardfork('vm', 'maxExtraDataSize', hardfork)
       ) {
-        throw new Error('invalid amount of extra data')
+        const msg = 'invalid amount of extra data'
+        throw this._error(msg)
       }
     } else {
       const minLength = CLIQUE_EXTRA_VANITY + CLIQUE_EXTRA_SEAL
       if (!this.cliqueIsEpochTransition()) {
         // ExtraData length on epoch transition
         if (this.extraData.length !== minLength) {
-          throw new Error(
-            `extraData must be ${minLength} bytes on non-epoch transition blocks, received ${this.extraData.length} bytes`
-          )
+          const msg = `extraData must be ${minLength} bytes on non-epoch transition blocks, received ${this.extraData.length} bytes`
+          throw this._error(msg)
         }
       } else {
         const signerLength = this.extraData.length - minLength
         if (signerLength % 20 !== 0) {
-          throw new Error(
-            `invalid signer list length in extraData, received signer length of ${signerLength} (not divisible by 20)`
-          )
+          const msg = `invalid signer list length in extraData, received signer length of ${signerLength} (not divisible by 20)`
+          throw this._error(msg)
         }
         // coinbase (beneficiary) on epoch transition
         if (!this.coinbase.isZero()) {
-          throw new Error(
-            `coinbase must be filled with zeros on epoch transition blocks, received ${this.coinbase.toString()}`
-          )
+          const msg = `coinbase must be filled with zeros on epoch transition blocks, received ${this.coinbase.toString()}`
+          throw this._error(msg)
         }
       }
       // MixHash format
       if (!this.mixHash.equals(Buffer.alloc(32))) {
-        throw new Error(`mixHash must be filled with zeros, received ${this.mixHash}`)
+        const msg = `mixHash must be filled with zeros, received ${this.mixHash}`
+        throw this._error(msg)
       }
       if (!this.validateCliqueDifficulty(blockchain)) {
-        throw new Error('invalid clique difficulty')
+        const msg = `invalid clique difficulty`
+        throw this._error(msg)
       }
     }
 
@@ -557,9 +565,6 @@ export class BlockHeader {
    * Returns the hash of the block header.
    */
   hash(): Buffer {
-    if (this._common.consensusAlgorithm() === 'clique' && !this.isGenesis()) {
-      return this.cliqueHash()
-    }
     return rlphash(this.raw())
   }
 
@@ -577,10 +582,9 @@ export class BlockHeader {
   }
 
   /**
-   * Hash for PoA clique blocks is created without the seal.
-   * @hidden
+   * PoA clique signature hash without the seal.
    */
-  private cliqueHash() {
+  cliqueSigHash() {
     const raw = this.raw()
     raw[12] = this.extraData.slice(0, this.extraData.length - CLIQUE_EXTRA_SEAL)
     return rlphash(raw)
@@ -623,7 +627,7 @@ export class BlockHeader {
    */
   private cliqueSealBlock(privateKey: Buffer) {
     this._requireClique('cliqueSealBlock')
-    const signature = ecsign(this.hash(), privateKey)
+    const signature = ecsign(this.cliqueSigHash(), privateKey)
     const signatureB = Buffer.concat([signature.r, signature.s, intToBuffer(signature.v - 27)])
 
     let extraDataWithoutSeal = this.extraData.slice(0, this.extraData.length - CLIQUE_EXTRA_SEAL)
@@ -684,10 +688,14 @@ export class BlockHeader {
   cliqueSigner(): Address {
     this._requireClique('cliqueSigner')
     const extraSeal = this.cliqueExtraSeal()
+    // Reasonable default for default blocks
+    if (extraSeal.length === 0) {
+      return Address.zero()
+    }
     const r = extraSeal.slice(0, 32)
     const s = extraSeal.slice(32, 64)
     const v = bufferToInt(extraSeal.slice(64, 65)) + 27
-    const pubKey = ecrecover(this.hash(), v, r, s)
+    const pubKey = ecrecover(this.cliqueSigHash(), v, r, s)
     return Address.fromPublicKey(pubKey)
   }
 
@@ -721,6 +729,18 @@ export class BlockHeader {
     }
   }
 
+  /**
+   * Internal helper function to create an annotated error message
+   *
+   * @param msg Base error message
+   * @hidden
+   */
+  _error(msg: string) {
+    msg += ` (${this._errorPostfix})`
+    const e = new Error(msg)
+    return e
+  }
+
   private _getHardfork(): string {
     return this._common.hardfork() || this._common.activeHardfork(this.number.toNumber())
   }

packages/block/src/types.ts

@@ -13,6 +13,9 @@ export interface BlockOptions {
   /**
    * A Common object defining the chain and the hardfork a block/block header belongs to.
    *
+   * Object will be internally copied so that tx behavior don't incidentally
+   * change on future HF changes.
+   *
    * Default: `Common` object set to `mainnet` and the HF currently defined as the default
    * hardfork in the `Common` class.
    *

packages/block/test/clique.spec.ts

@@ -80,7 +80,7 @@ tape('[Header]: Clique PoA Functionality', function (t) {
   t.test('Signing', function (st) {
     const cliqueSigner = A.privateKey
 
-    const header = BlockHeader.fromHeaderData(
+    let header = BlockHeader.fromHeaderData(
       { number: 1, extraData: Buffer.alloc(97) },
       { common, freeze: false, cliqueSigner }
     )
@@ -89,6 +89,13 @@ tape('[Header]: Clique PoA Functionality', function (t) {
     st.ok(header.cliqueVerifySignature([A.address]), 'should verify signature')
     st.ok(header.cliqueSigner().equals(A.address), 'should recover the correct signer address')
 
+    header = BlockHeader.fromHeaderData({}, { common })
+    st.deepEqual(
+      header.cliqueSigner(),
+      Address.zero(),
+      'should return zero address on default block'
+    )
+
     st.end()
   })
 })

packages/block/test/header.spec.ts

@@ -5,6 +5,8 @@ import { BlockHeader } from '../src/header'
 import { Block } from '../src'
 import { Mockchain } from './mockchain'
 const testData = require('./testdata/testdata.json')
+const blocksMainnet = require('./testdata/blocks_mainnet.json')
+const blocksGoerli = require('./testdata/blocks_goerli.json')
 
 tape('[Block]: Header functions', function (t) {
   t.test('should create with default constructor', function (st) {
@@ -39,6 +41,14 @@ tape('[Block]: Header functions', function (t) {
     const common = new Common({ chain: 'ropsten', hardfork: 'chainstart' })
     let header = BlockHeader.genesis(undefined, { common })
     st.ok(header.hash().toString('hex'), 'genesis block should initialize')
+    st.equal(header._common.hardfork(), 'chainstart', 'should initialize with correct HF provided')
+
+    common.setHardfork('byzantium')
+    st.equal(
+      header._common.hardfork(),
+      'chainstart',
+      'should stay on correct HF if outer common HF changes'
+    )
 
     header = BlockHeader.fromHeaderData({}, { common })
     st.ok(header.hash().toString('hex'), 'default block should initialize')
@@ -137,7 +147,7 @@ tape('[Block]: Header functions', function (t) {
       await header.validate(blockchain)
       st.fail(testCase)
     } catch (error) {
-      st.equal(error.message, 'invalid amount of extra data', testCase)
+      st.ok(error.message.includes('invalid amount of extra data'), testCase)
     }
 
     // PoA
@@ -171,9 +181,10 @@ tape('[Block]: Header functions', function (t) {
       await header.validate(blockchain)
       t.fail(testCase)
     } catch (error) {
-      t.equal(
-        error.message,
-        'extraData must be 97 bytes on non-epoch transition blocks, received 32 bytes',
+      t.ok(
+        error.message.includes(
+          'extraData must be 97 bytes on non-epoch transition blocks, received 32 bytes'
+        ),
         testCase
       )
     }
@@ -192,9 +203,10 @@ tape('[Block]: Header functions', function (t) {
       await header.validate(blockchain)
       st.fail(testCase)
     } catch (error) {
-      st.equals(
-        error.message,
-        'invalid signer list length in extraData, received signer length of 41 (not divisible by 20)',
+      st.ok(
+        error.message.includes(
+          'invalid signer list length in extraData, received signer length of 41 (not divisible by 20)'
+        ),
         testCase
       )
     }
@@ -271,7 +283,7 @@ tape('[Block]: Header functions', function (t) {
     st.end()
   })
 
-  t.test('should test validateGasLimit', function (st) {
+  t.test('should test validateGasLimit()', function (st) {
     const testData = require('./testdata/bcBlockGasLimitTest.json').tests
     const bcBlockGasLimitTestData = testData.BlockGasLimit2p63m1
 
@@ -313,4 +325,23 @@ tape('[Block]: Header functions', function (t) {
     st.strictEqual(genesis.stateRoot.toString('hex'), ropstenStateRoot, 'genesis stateRoot match')
     st.end()
   })
+
+  t.test('should test hash() function', function (st) {
+    let common = new Common({ chain: 'mainnet', hardfork: 'chainstart' })
+    let header = BlockHeader.fromHeaderData(blocksMainnet[0]['header'], { common })
+    st.equal(
+      header.hash().toString('hex'),
+      '88e96d4537bea4d9c05d12549907b32561d3bf31f45aae734cdc119f13406cb6',
+      'correct PoW hash (mainnet block 1)'
+    )
+
+    common = new Common({ chain: 'goerli', hardfork: 'chainstart' })
+    header = BlockHeader.fromHeaderData(blocksGoerli[0]['header'], { common })
+    st.equal(
+      header.hash().toString('hex'),
+      '8f5bab218b6bb34476f51ca588e9f4553a3a7ce5e13a66c660a5283e97e9a85a',
+      'correct PoA clique hash (goerli block 1)'
+    )
+    st.end()
+  })
 })