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

g11tech · 2022-09-26T12:43:43Z

The sepolia network even post this

{
      "//_comment": "Post merge forks should always have ttd set with block to distinguish that they come post merge hf (block = null)",
      "name": "mergeForkIdTransition",
      "block": 1735371,
      "forkHash": "0xb96cbd13"
    },

was only giving merge hardfork on common.getHardforkByBlockNumber(1735371,"17000000000000000")
and would give mergeForkIdTransition on common.getHardforkByBlockNumber(1735371)

This PR refactors the getHardforkByBlockNumber as the case handling was not clear and upfront

TODO:

Add test cases covering the post merge hardforks as well as validations

codecov · 2022-09-26T12:48:53Z

Codecov Report

Merging #2313 (1cf6ed1) into master (88bd15d) will increase coverage by 1.82%.
The diff coverage is 100.00%.

Additional details and impacted files

Flag	Coverage Δ
block	`92.86% <ø> (ø)`
blockchain	`90.21% <ø> (ø)`
client	`86.97% <ø> (ø)`
common	`98.36% <100.00%> (+<0.01%)`	⬆️
devp2p	`?`
ethash	`∅ <ø> (∅)`
evm	`?`
rlp	`∅ <ø> (∅)`
statemanager	`?`
trie	`?`
tx	`97.98% <ø> (ø)`
util	`88.99% <ø> (ø)`
vm	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

acolytec3

Will do more review later but here are some initial questions/feedback.

packages/common/src/common.ts

packages/common/tests/hardforks.spec.ts

packages/common/src/common.ts

acolytec3 · 2022-09-28T10:18:09Z

packages/common/src/common.ts

+
+      // hf qualifies else we would have broken out of the loop till now
+      if (hardfork?.ttd !== undefined && hardfork?.ttd !== null) {
+        passedMergeHF = hardfork!


Why do we need this in addition to preMergeHf? I think I must not be tracking all of the logic yet because there can be only one hardfork with TTD set. So, if we have that, can't we just use that hardfork (set above as preMergeHf). Also, it seems really odd to call the merge hardfork preMergeHf. The "pre-merge hardfork" is London so maybe we can find a better way to name this?

yes, thats what preMergeHf should represent, its london, and passedMergedHF is assigned merge hardfork if its passed, i guess i can change it to boolean and store the merge hardfork as mergeHF as this later needs to be accessed.

acolytec3 · 2022-09-28T10:22:13Z

packages/common/tests/hardforks.spec.ts

+    st.equal(c.getHardforkByBlockNumber(0), Hardfork.London, msg)
+    //using Hardfork.London even though Merge is expected after terminal block 1450408
+    st.equal(c.getHardforkByBlockNumber(1450409), Hardfork.London, msg)
+    st.equal(c.getHardforkByBlockNumber(1450409, BigInt('17000000000000000')), Hardfork.Merge, msg)


I think this result is counterintuitive. The function is called getHardforkByBlockNumber and yet when we set TTD and a block number prior to the actual merge block, it's telling me the hardfork is the Merge hardfork. If we're checking the hardfork and specifying a block number prior to the actual block where the merge happens, we shouldn't report a hardfork that is happening later. I know there's screwiness around Merge and MergeForkTransitionId but this doesn't make a ton of sense to me.

I still think this result is counterintuitive. If you provide a block number and the merge hasn't happened yet, regardless of what you pass in for TD, this method should return the previous hardfork. Block number should trump hardfork in terms of which fork is returned.

merge HAS happened if we provide td > ttd (L73), and hasn't happened if we don't provide td L72

holgerd77 · 2022-09-28T12:12:36Z

TBH: I have no idea what we are actually solving here.

was only giving merge hardfork on common.getHardforkByBlockNumber(1735371,"17000000000000000")
and would give mergeForkIdTransition on common.getHardforkByBlockNumber(1735371)

This seems like the correct behavior to me. When a TTD is given the TTD takes precedence (and therefore returns the Merge HF with the TTD set, otherwise respectively in the only-block-number case the mergeForkIdTransition HF is returned.

What is the problematic behavior respectively what case can not be modelled with the current method behavior? thinking

g11tech · 2022-09-28T12:44:26Z

This seems like the correct behavior to me. When a TTD is given the TTD takes precedence (and therefore returns the Merge HF with the TTD set, otherwise respectively in the only-block-number case the mergeForkIdTransition HF is returned.

What is the problematic behavior respectively what case can not be modelled with the current method behavior? thinking

It (common.getHardforkByBlockNumber(1735371,"17000000000000000")) should give mergeForkIdTransition on/after 1735371 as mergeForkIdTransition is scheduled post merge.
currently if the merge block is ~~not~~ specified in hardfork config, it will always return merge even for hardforks scheduled post merge.

The current change is to get the correct mergeForkIdTransition even if one has not specified the block of the merge hardfork.

holgerd77 · 2022-09-28T12:49:28Z

It (common.getHardforkByBlockNumber(1735371,"17000000000000000")) should give mergeForkIdTransition on/after 1735371 as mergeForkIdTransition is scheduled post merge. i.e. if the merge block is not specified in hardfork config, it will always return merge even for hardforks scheduled post merge.
The current change is to get the correct mergeForkIdTransition even if one has not specified the block of the merge hardfork.

Yes, you should only call the getHardforkByBlockNumber method with TTD if you want to get the Merge HF itself, in all other cases, you should call it without TTD (also for mergeForkIdTransition). That's how I intended it when writing this method. Is this behavior problematic? 🤔

You should just be able to call with TTD on all code parts where you want the Merge HF and otherwise do without.

g11tech · 2022-09-28T14:25:39Z

You should just be able to call with TTD on all code parts where you want the Merge HF and otherwise do without.

in post merge world, how would one know if the current hardfork is a post merge scheduled hardfork? I was under the impression that is interface (with td) would also provide me the same.

Else I can write a wrapper in the client to combine me the functionality of the hardforkByNumber as well as td

g11tech · 2022-09-28T14:28:16Z

OR you mean to say, we selectively call with/without td based on if we want to access some merge related functionality?

acolytec3 · 2022-09-28T15:08:22Z

Yes, you should only call the getHardforkByBlockNumber method with TTD if you want to get the Merge HF itself, in all other cases, you should call it without TTD (also for mergeForkIdTransition). That's how I intended it when writing this method. Is this behavior problematic? thinking

If we pass with TTD only, it just becomes a boolean of provided ttd >= mergefork.ttd, right? That doesn't seem like it fits the intended use case for this method and we should create a special purpose method for such.

To my way of thinking, if a user wants to know if a given block is passed the merge hardfork using this method, we should require them to pass both block number and ttd, run through the list of hardforks and find the one that we're on by block number, then check ttd only if the hf by blocknumber says we're on london. If we find a later hardfork with a block number (e.g. in some future world where Shanghai has passed already), then we shouldn't need to look at ttd at all.

Does that logic make sense at all?

acolytec3 · 2022-09-28T15:33:24Z

Could we replace the whole thing with something like this?

 // Filter out hardforks with no block number and no ttd (i.e. unapplied hardforks)
  const hfs = common
    .hardforks()
    .filter((hf) => hf.block !== null || (hf.ttd !== null && hf.ttd !== undefined))
  // The index of the hardfork in the hf array that matches our hardfork based on block number
  const hfIndex = hfs.findIndex((entry) => entry.block !== null && entry.block > blockNumber) - 1
  if (hfs[hfIndex].block === null) {
    // We're on the merge hardfork.  Let's check the TTD
    if (BigInt(hfs[hfIndex].ttd!) > td) {
      // Merge ttd greater than current td so we're on hardfork before merge
      hf = hfs[hfIndex - 1]
    } else {
      // Merge ttd equal or less than current td so we're on merge hardfork
      hf = hfs[hfIndex]
    }
  }
  return hf

This assumes that the hardfork array is in sequentially ascending order (as today) and that hardforks with a null block number and undefined/null ttd are not applied on the current chain and fulfills the logic I outlined above.

packages/common/src/common.ts

acolytec3 · 2022-09-28T19:09:21Z

packages/common/tests/hardforks.spec.ts

+    st.equal(c.getHardforkByBlockNumber(0), Hardfork.London, msg)
+    //using Hardfork.London even though Merge is expected after terminal block 1450408
+    st.equal(c.getHardforkByBlockNumber(1450409), Hardfork.London, msg)
+    st.equal(c.getHardforkByBlockNumber(1450409, BigInt('17000000000000000')), Hardfork.Merge, msg)


I still think this result is counterintuitive. If you provide a block number and the merge hasn't happened yet, regardless of what you pass in for TD, this method should return the previous hardfork. Block number should trump hardfork in terms of which fork is returned.

g11tech · 2022-09-29T09:09:25Z

packages/common/tests/mergePOS.spec.ts

+    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


@acolytec3 see if these comments help making sense of the situation

as terminal as config doesn't have that info -- I don't think this is true any more. You updated the sepolia config to have a block number for the merge.

yes but for this test case i made block null, see L209, i have made this test to be independent of what we specify config in json

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

acolytec3 · 2022-09-29T14:11:25Z

packages/common/tests/hardforks.spec.ts

+
+    try {
+      c.getHardforkByBlockNumber(0)
+      t.fail('should have thrown since no hardfork should quality')


Suggested change

t.fail('should have thrown since no hardfork should quality')

t.fail('should have thrown since no hardfork should qualify')

acolytec3 · 2022-09-29T14:11:57Z

packages/common/src/common.ts

+      // all hardforks apply, set hfIndex to the last one as thats the candidate
+      hfIndex = hfs.length - 1
+    } else if (hfIndex === 0) {
+      // Cant move back, ideally we should throw??


Let's remove this comment

acolytec3 · 2022-09-29T14:12:37Z

packages/common/src/common.ts

+      hfIndex = hfs.length - 1
+    } else if (hfIndex === 0) {
+      // Cant move back, ideally we should throw??
+      throw Error('No hardfork qualitifies')


Suggested change

throw Error('No hardfork qualitifies')

throw Error('Must have at least one hardfork at block 0')

acolytec3

LGTM!

…s 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]>

holgerd77 · 2022-09-30T07:41:27Z

Ok, cool, thanks for finalizing!

g11tech force-pushed the g11tech/fix-post-merge-get-set-hardforks branch from bdff698 to f14520b Compare September 27, 2022 16:14

g11tech marked this pull request as ready for review September 27, 2022 16:22

acolytec3 requested changes Sep 27, 2022

View reviewed changes

packages/common/src/common.ts Outdated Show resolved Hide resolved

packages/common/src/common.ts Outdated Show resolved Hide resolved

g11tech commented Sep 27, 2022

View reviewed changes

packages/common/tests/hardforks.spec.ts Outdated Show resolved Hide resolved

g11tech added PR state: merge ready package: common target: master Work to be done towards master branch labels Sep 27, 2022

acolytec3 requested changes Sep 28, 2022

View reviewed changes

g11tech commented Sep 29, 2022

View reviewed changes

g11tech and others added 12 commits September 29, 2022 17:06

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

a43d580

make strick check optional for post merge hardforks

8496d71

add tests for post merge hardforks using sepolia

750c37c

simplify getHardforkByBlockNumber

b4a96ce

remove duplicate hf

1fcb0d0

clarify comment

167d546

add post merge hf

ee628f7

further simplify the fn and add test to enhance coverage

0e6827f

update comments

42e5168

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

03a3824

fix typo

c2a67ab

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

comment improvement

da9761a

g11tech and others added 15 commits September 29, 2022 17:06

undo mainnet json changes

699439c

simplify dup ttd check

2de76cf

fix spec

1cbeb43

fix typo

3c5f4b5

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

fix typo

c8f87a5

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

improve clarity

acc111c

simpilfy futher based on acolytec3s logic

6dbb644

fix some more validations

8b7fbca

remove extra if

1927409

improve wording

8206fbb

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

move spec, add test for merge hf with block num

a2e9ab2

relocate test

827c2e3

relocate test

b94f73d

lint

cbe855b

add merge block numbers

d07f8b3

g11tech force-pushed the g11tech/fix-post-merge-get-set-hardforks branch from db7d9ef to d07f8b3 Compare September 29, 2022 11:36

g11tech added 2 commits September 29, 2022 17:18

add merge forkhash

30a8394

handle no hardfork found case

55e0380

g11tech changed the title ~~common: Refactor and update the hardfork cal to address post merge hfs~~ common: Refactor and update common.getHardforkByBlockNumber to address post merge hfs Sep 29, 2022

acolytec3 requested changes Sep 29, 2022

View reviewed changes

Fix nits

1cf6ed1

acolytec3 approved these changes Sep 29, 2022

View reviewed changes

g11tech merged commit 7fb9ad8 into master Sep 29, 2022

holgerd77 deleted the g11tech/fix-post-merge-get-set-hardforks branch September 30, 2022 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

g11tech commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

acolytec3 left a comment

acolytec3 Sep 28, 2022

g11tech Sep 28, 2022

acolytec3 Sep 28, 2022

acolytec3 Sep 28, 2022

g11tech Sep 29, 2022

holgerd77 commented Sep 28, 2022 •

edited by g11tech

Loading

g11tech commented Sep 28, 2022 •

edited

Loading

holgerd77 commented Sep 28, 2022

g11tech commented Sep 28, 2022

g11tech commented Sep 28, 2022

acolytec3 commented Sep 28, 2022

acolytec3 commented Sep 28, 2022 •

edited

Loading

acolytec3 Sep 28, 2022

g11tech Sep 29, 2022

acolytec3 Sep 29, 2022

g11tech Sep 29, 2022

acolytec3 Sep 29, 2022

acolytec3 Sep 29, 2022

acolytec3 Sep 29, 2022

acolytec3 left a comment

holgerd77 commented Sep 30, 2022

	t.fail('should have thrown since no hardfork should quality')
	t.fail('should have thrown since no hardfork should qualify')

	throw Error('No hardfork qualitifies')
	throw Error('Must have at least one hardfork at block 0')

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

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

Conversation

g11tech commented Sep 26, 2022 • edited Loading

codecov bot commented Sep 26, 2022 • edited Loading

Codecov Report

acolytec3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Sep 28, 2022 • edited by g11tech Loading

g11tech commented Sep 28, 2022 • edited Loading

holgerd77 commented Sep 28, 2022

g11tech commented Sep 28, 2022

g11tech commented Sep 28, 2022

acolytec3 commented Sep 28, 2022

acolytec3 commented Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

holgerd77 commented Sep 30, 2022

g11tech commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

holgerd77 commented Sep 28, 2022 •

edited by g11tech

Loading

g11tech commented Sep 28, 2022 •

edited

Loading

acolytec3 commented Sep 28, 2022 •

edited

Loading