common: Refactor and update common.getHardforkByBlockNumber to address post merge hfs (#2313)

* common: Refactor and update the hardfork cal to address post merge hfs

* make strick check optional for post merge hardforks

* add tests for post merge hardforks using sepolia

* simplify getHardforkByBlockNumber

* remove duplicate hf

* clarify comment

* add post merge hf

* further simplify the fn and add test to enhance coverage

* update comments

* remove hash to add it as a separate pr to not pollute this one

* fix typo

Co-authored-by: acolytec3 <[email protected]>

* comment improvement

* undo mainnet json changes

* simplify dup ttd check

* fix spec

* fix typo

Co-authored-by: acolytec3 <[email protected]>

* fix typo

Co-authored-by: acolytec3 <[email protected]>

* improve clarity

* simpilfy futher based on acolytec3s logic

* fix some more validations

* remove extra if

* improve wording

Co-authored-by: acolytec3 <[email protected]>

* move spec, add test for merge hf with block num

* relocate test

* relocate test

* lint

* add merge block numbers

* add merge forkhash

* handle no hardfork found case

* Fix nits

Co-authored-by: acolytec3 <[email protected]>

Author: g11tech

packages/client/test/rpc/util/CLConnectionManager.spec.ts

@@ -53,6 +53,7 @@ tape('[CLConnectionManager]', (t) => {
     manager = new CLConnectionManager({ config })
     st.ok(manager.running, 'starts on instantiation if hardfork is MergeForkBlock')
     manager.stop()
+    const prevMergeForkBlock = (genesisJSON.config as any).mergeForkBlock
     ;(genesisJSON.config as any).mergeForkBlock = 10
     common = new Common({
       chain: params.name,
@@ -69,6 +70,8 @@ tape('[CLConnectionManager]', (t) => {
     })
     config.events.emit(Event.CHAIN_UPDATED)
     config.events.emit(Event.CLIENT_SHUTDOWN)
+    // reset prevMergeForkBlock as it seems to be polluting other tests
+    ;(genesisJSON.config as any).mergeForkBlock = prevMergeForkBlock
   })
 
   t.test('Status updates', async (st) => {

packages/common/src/chains/goerli.json

@@ -72,7 +72,7 @@
       "forkHash": "0xb8c6299d"
     },
     {
-      "//_comment": "The forkHash will remain same as mergeForkIdTransition is post merge",
+      "//_comment": "The forkHash will remain same as mergeForkIdTransition is post merge, terminal block: https://goerli.etherscan.io/block/7382818",
       "name": "merge",
       "ttd": "10790000",
       "block": null,

packages/common/src/chains/mainnet.json

@@ -88,13 +88,14 @@
       "forkHash": "0xf0afd0e3"
     },
     {
-      "name": "mergeForkIdTransition",
+      "//_comment": "The forkHash will remain same as mergeForkIdTransition is post merge, terminal block: https://etherscan.io/block/15537393",
+      "name": "merge",
+      "ttd": "58750000000000000000000",
       "block": null,
-      "forkHash": null
+      "forkHash": "0xf0afd0e3"
     },
     {
-      "name": "merge",
-      "ttd": "58750000000000000000000",
+      "name": "mergeForkIdTransition",
       "block": null,
       "forkHash": null
     },

packages/common/src/chains/sepolia.json

@@ -74,7 +74,7 @@
       "forkHash": "0xfe3366e7"
     },
     {
-      "//_comment": "The forkHash will remain same as mergeForkIdTransition is post merge",
+      "//_comment": "The forkHash will remain same as mergeForkIdTransition is post merge, terminal block: https://sepolia.etherscan.io/block/1450408",
       "name": "merge",
       "ttd": "17000000000000000",
       "block": null,

packages/common/src/common.ts

@@ -285,57 +285,63 @@ export class Common extends EventEmitter {
    * will be thrown).
    *
    * @param blockNumber
-   * @param td
+   * @param td : total difficulty of the parent block (for block hf) OR of the the chain latest (for chain hf)
    * @returns The name of the HF
    */
   getHardforkByBlockNumber(blockNumber: BigIntLike, td?: BigIntLike): string {
     blockNumber = toType(blockNumber, TypeOutput.BigInt)
     td = toType(td, TypeOutput.BigInt)
 
-    let hardfork = Hardfork.Chainstart
-    let minTdHF
-    let maxTdHF
-    let previousHF
-    for (const hf of this.hardforks()) {
-      // Skip comparison for not applied HFs
-      if (hf.block === null) {
-        if (td !== undefined && td !== null && hf.ttd !== undefined && hf.ttd !== null) {
-          if (td >= BigInt(hf.ttd)) {
-            return hf.name
-          }
-        }
-        continue
-      }
-      if (blockNumber >= BigInt(hf.block)) {
-        hardfork = hf.name as Hardfork
-      }
-      if (td && (typeof hf.ttd === 'string' || typeof hf.ttd === 'bigint')) {
-        if (td >= BigInt(hf.ttd)) {
-          minTdHF = hf.name
-        } else {
-          maxTdHF = previousHF
-        }
-      }
-      previousHF = hf.name
-    }
-    if (td) {
-      let msgAdd = `block number: ${blockNumber} (-> ${hardfork}), `
-      if (minTdHF !== undefined) {
-        if (!this.hardforkGteHardfork(hardfork, minTdHF)) {
-          const msg = 'HF determined by block number is lower than the minimum total difficulty HF'
-          msgAdd += `total difficulty: ${td} (-> ${minTdHF})`
-          throw new Error(`${msg}: ${msgAdd}`)
-        }
+    // Filter out hardforks with no block number and no ttd (i.e. unapplied hardforks)
+    const hfs = this.hardforks().filter(
+      (hf) => hf.block !== null || (hf.ttd !== null && hf.ttd !== undefined)
+    )
+    const mergeIndex = hfs.findIndex((hf) => hf.ttd !== null && hf.ttd !== undefined)
+    const doubleTTDHF = hfs
+      .slice(mergeIndex + 1)
+      .findIndex((hf) => hf.ttd !== null && hf.ttd !== undefined)
+    if (doubleTTDHF >= 0) {
+      throw Error(`More than one merge hardforks found with ttd specified`)
+    }
+
+    // Find the first hardfork that has a block number greater than `blockNumber` (skips the merge hardfork since
+    // it cannot have a block number specified).
+    let hfIndex = hfs.findIndex((hf) => hf.block !== null && hf.block > blockNumber)
+
+    // Move hfIndex one back to arrive at candidate hardfork
+    if (hfIndex === -1) {
+      // all hardforks apply, set hfIndex to the last one as thats the candidate
+      hfIndex = hfs.length - 1
+    } else if (hfIndex === 0) {
+      // cannot have a case where a block number is before all applied hardforks
+      // since the chain has to start with a hardfork
+      throw Error('Must have at least one hardfork at block 0')
+    } else {
+      // The previous hardfork is the candidate here
+      hfIndex = hfIndex - 1
+    }
+
+    let hardfork
+    if (hfs[hfIndex].block === null) {
+      // We're on the merge hardfork.  Let's check the TTD
+      if (td === undefined || td === null || BigInt(hfs[hfIndex].ttd!) > td) {
+        // Merge ttd greater than current td so we're on hardfork before merge
+        hardfork = hfs[hfIndex - 1]
+      } else {
+        // Merge ttd equal or less than current td so we're on merge hardfork
+        hardfork = hfs[hfIndex]
       }
-      if (maxTdHF !== undefined) {
-        if (!this.hardforkGteHardfork(maxTdHF, hardfork)) {
-          const msg = 'Maximum HF determined by total difficulty is lower than the block number HF'
-          msgAdd += `total difficulty: ${td} (-> ${maxTdHF})`
-          throw new Error(`${msg}: ${msgAdd}`)
+    } else {
+      if (mergeIndex >= 0 && td !== undefined && td !== null) {
+        if (hfIndex >= mergeIndex && BigInt(hfs[mergeIndex].ttd!) > td) {
+          throw Error('Maximum HF determined by total difficulty is lower than the block number HF')
+        } else if (hfIndex < mergeIndex && BigInt(hfs[mergeIndex].ttd!) <= td) {
+          throw Error('HF determined by block number is lower than the minimum total difficulty HF')
         }
       }
+      hardfork = hfs[hfIndex]
     }
-    return hardfork
+    return hardfork.name
   }
 
   /**

packages/common/tests/hardforks.spec.ts

@@ -63,6 +63,36 @@ tape('[Common]: Hardfork logic', function (t: tape.Test) {
     st.end()
   })
 
+  t.test('should throw if no hardfork qualifies', function (st) {
+    const hardforks = [
+      {
+        name: 'homestead',
+        block: 3,
+      },
+      {
+        name: 'tangerineWhistle',
+        block: 3,
+      },
+      {
+        name: 'spuriousDragon',
+        block: 3,
+      },
+    ]
+    const c = Common.custom({ hardforks }, { baseChain: Chain.Sepolia })
+
+    try {
+      c.getHardforkByBlockNumber(0)
+      t.fail('should have thrown since no hardfork should qualify')
+    } catch (e) {
+      t.pass('throw since no hardfork qualifies')
+    }
+
+    const msg = 'should return correct value'
+    st.equal(c.setHardforkByBlockNumber(3), Hardfork.SpuriousDragon, msg)
+
+    t.end()
+  })
+
   t.test('setHardfork(): hardforkChanged event', function (st) {
     const c = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Istanbul })
     c.on('hardforkChanged', (hardfork: string) => {

packages/common/tests/mergePOS.spec.ts

@@ -1,6 +1,6 @@
 import * as tape from 'tape'
 
-import { Common, Hardfork } from '../src'
+import { Chain, Common, Hardfork } from '../src'
 
 import * as testnetMerge from './data/merge/testnetMerge.json'
 import * as testnetPOS from './data/merge/testnetPOS.json'
@@ -142,13 +142,15 @@ tape('[Common]: Merge/POS specific logic', function (t: tape.Test) {
 
       try {
         c.setHardforkByBlockNumber(16, 4999)
+        st.fail(`should have thrown td < ttd validation error`)
       } catch (e: any) {
         msg = 'block number > last HF block number set, TD set and smaller (should throw)'
         const eMsg = 'Maximum HF determined by total difficulty is lower than the block number HF'
         st.ok(e.message.includes(eMsg), msg)
       }
       try {
         c.setHardforkByBlockNumber(14, 5000)
+        st.fail(`should have thrown td > ttd validation error`)
       } catch (e: any) {
         msg = 'block number < last HF block number set, TD set and higher (should throw)'
         const eMsg = 'HF determined by block number is lower than the minimum total difficulty HF'
@@ -187,4 +189,122 @@ tape('[Common]: Merge/POS specific logic', function (t: tape.Test) {
     st.equal(c.getHardforkByBlockNumber(5, 0), 'shanghai', msg)
     st.end()
   })
+
+  t.test('should get the correct merge hardfork at genesis', async (st) => {
+    const json = require(`../../client/test/testdata/geth-genesis/post-merge.json`)
+    const c = Common.fromGethGenesis(json, { chain: 'post-merge' })
+    const msg = 'should get HF correctly'
+    st.equal(c.getHardforkByBlockNumber(0), Hardfork.London, msg)
+    st.equal(c.getHardforkByBlockNumber(0, BigInt(0)), Hardfork.Merge, msg)
+  })
+
+  t.test('test post merge hardforks using Sepolia with block null', function (st: tape.Test) {
+    const c = new Common({ chain: Chain.Sepolia })
+    let msg = 'should get HF correctly'
+
+    st.equal(c.getHardforkByBlockNumber(0), Hardfork.London, msg)
+    // Make it null manually as config could be updated later
+    const mergeHf = c.hardforks().filter((hf) => hf.ttd !== undefined && hf.ttd !== null)[0]
+    const prevMergeBlockVal = mergeHf.block
+    mergeHf.block = null
+
+    // should get Hardfork.London even though happened with 1450408 as terminal as config doesn't have that info
+    st.equal(c.getHardforkByBlockNumber(1450409), Hardfork.London, msg)
+    // however with correct td in input it should select merge
+    st.equal(c.getHardforkByBlockNumber(1450409, BigInt('17000000000000000')), Hardfork.Merge, msg)
+    // should select MergeForkIdTransition even without td specified as the block is set for this hardfork
+    st.equal(c.getHardforkByBlockNumber(1735371), Hardfork.MergeForkIdTransition, msg)
+    // also with td specified
+    st.equal(
+      c.getHardforkByBlockNumber(1735371, BigInt('17000000000000000')),
+      Hardfork.MergeForkIdTransition,
+      msg
+    )
+    try {
+      st.equal(
+        c.getHardforkByBlockNumber(1735371, BigInt('15000000000000000')),
+        Hardfork.MergeForkIdTransition,
+        msg
+      )
+      st.fail('should have thrown as specified td < merge ttd for a post merge hardfork')
+    } catch (error) {
+      st.pass('throws error as specified td < merge ttd for a post merge hardfork')
+    }
+
+    msg = 'should set HF correctly'
+
+    st.equal(c.setHardforkByBlockNumber(0), Hardfork.London, msg)
+    st.equal(c.setHardforkByBlockNumber(1450409), Hardfork.London, msg)
+    st.equal(c.setHardforkByBlockNumber(1450409, BigInt('17000000000000000')), Hardfork.Merge, msg)
+    st.equal(c.setHardforkByBlockNumber(1735371), Hardfork.MergeForkIdTransition, msg)
+    st.equal(
+      c.setHardforkByBlockNumber(1735371, BigInt('17000000000000000')),
+      Hardfork.MergeForkIdTransition,
+      msg
+    )
+    try {
+      st.equal(
+        c.setHardforkByBlockNumber(1735371, BigInt('15000000000000000')),
+        Hardfork.MergeForkIdTransition,
+        msg
+      )
+      st.fail('should have thrown as specified td < merge ttd for a post merge hardfork')
+    } catch (error) {
+      st.pass('throws error as specified td < merge ttd for a post merge hardfork')
+    }
+
+    // restore value
+    mergeHf.block = prevMergeBlockVal
+
+    st.end()
+  })
+
+  t.test(
+    'should get correct merge and post merge hf with merge block specified ',
+    function (st: tape.Test) {
+      const c = new Common({ chain: Chain.Sepolia })
+
+      const mergeHf = c.hardforks().filter((hf) => hf.ttd !== undefined && hf.ttd !== null)[0]
+      const prevMergeBlockVal = mergeHf.block
+      // the terminal block on sepolia is 1450408
+      mergeHf.block = 1450409
+      const msg = 'should get HF correctly'
+
+      // should get merge even without td supplied as the merge hf now has the block specified
+      st.equal(c.setHardforkByBlockNumber(1450409), Hardfork.Merge, msg)
+      st.equal(
+        c.setHardforkByBlockNumber(1450409, BigInt('17000000000000000')),
+        Hardfork.Merge,
+        msg
+      )
+      st.equal(c.setHardforkByBlockNumber(1735371), Hardfork.MergeForkIdTransition, msg)
+      st.equal(
+        c.setHardforkByBlockNumber(1735371, BigInt('17000000000000000')),
+        Hardfork.MergeForkIdTransition,
+        msg
+      )
+      // restore value
+      mergeHf.block = prevMergeBlockVal
+
+      st.end()
+    }
+  )
+
+  t.test(
+    'should throw if encounters a double ttd hardfork specification',
+    function (st: tape.Test) {
+      const c = new Common({ chain: Chain.Sepolia })
+      // Add the ttd to mergeForkIdTransition which occurs post merge in sepolia
+      c.hardforks().filter((hf) => hf.name === 'mergeForkIdTransition')[0]!['ttd'] =
+        '17000000000000000'
+
+      try {
+        c.setHardforkByBlockNumber(1735371)
+        st.fail('should have thrown as two hardforks with ttd specified')
+      } catch (error) {
+        st.pass('throws error as two hardforks with ttd specified')
+      }
+      st.end()
+    }
+  )
 })