Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: Parse post-merge hardfork config in parseGethParams and handle post-merge genesis block #2427

Merged
merged 18 commits into from
Nov 22, 2022

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented Nov 17, 2022

In order to get the geth genesis working for 4844 work, the sharding block needs to be configured post merge which is configured post block 0 (Currently merge if present is the last hf configured)

Similar setup is required to have ethereumjs shandong branch start off withdrawals at non zero block number after the merge.

This PR enhances the same
Geth genesis now carries a flag terminalTotalDifficultyPassed for this which indicates that the merge is before any other hardforks configured on non zero block number

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #2427 (5c4f5d8) into master (161a402) will increase coverage by 0.01%.
The diff coverage is 95.65%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 89.51% <ø> (ø)
blockchain 90.03% <ø> (ø)
client 87.47% <100.00%> (+0.01%) ⬆️
common 98.16% <100.00%> (ø)
devp2p 91.63% <ø> (+0.05%) ⬆️
ethash ∅ <ø> (∅)
evm 79.73% <95.00%> (+<0.01%) ⬆️
rlp ?
statemanager 89.46% <ø> (ø)
trie 90.36% <ø> (ø)
tx 97.80% <ø> (ø)
util 84.59% <ø> (ø)
vm 85.54% <ø> (ø)

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

@g11tech g11tech changed the title common: Set merge hardfork in parseGethParams before non zero blocks based on terminalTotalDifficultyPassed flag common: Set merge hardfork in parseGethParams before hardforks scheduled post genesis if terminalTotalDifficultyPassed Nov 17, 2022
nonzeroIndex !== -1
) {
// find index where block > 0
params.hardforks.splice(nonzeroIndex, 0, mergeConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's safe to assume the first nonzero hardfork will always be where the merge should go. We could very easily have an earlier hardfork (like ArrowGlacier/GrayGlacier which isn't even in this list) be the first nonzero one. Unless I'm not thinking clearly, we should always put Merge after London (if ArrowGlacier/GrayGlacier aren't specified), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ummm, doesn't actually matter in scheme of things if those are all at 0
the issue is when terminalTotalDifficultyPassed is false or not present, then where should we place Merge?

The order we assume is the order they showup in the config section, and it would have been nice to also have mergeBlock there even if its block is null

So that there is no confusion about the order, may be we can give this feedback to the geth guys and if the mergeBlock is not present in the config (like it doesn't for now) we can use this logic to place merge

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K, this is the PR where they add it. I'm honestly not sure how to handle this one. The main place they reference this new parameter is here where the sync type is being decided (so akin to how we differentiate between full sync and beacon sync). Can you do beaconsync forward before the merge block or does the client have to continue with full sync till it hits the merge block? If the latter, I still think we should explicitly look for the london fork block number and insert this one after it. I can't think of a reason we should have GrayGlacier or ArrowGlacier in a custom network so we shouldn't need to check for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the PoS or PoW consensus actually has nothing to do with pre/post london, it can be placed anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, looking at the geths PR has got me confused about this flag, but we defintely need post merge hardforks differentiation which certainly needs merge, like withdrawals, and 4844. I am not clear if thats the way to go especially with mergeForkID confusion of it bein pre/post merge.
But since kiln, no network has had mergeForkId before merge so may be i am over thinking it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i have tried to make this thing a bit more intelligenct, placing merge before the first postmerge hardfork, and is still dynamic with respect to mergeForkId block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Geth not just giving a block number for the Merge?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this dynamic handling and wild-guessing for this one time Merge event which has now passed seems totally overblown to me and doesn't really solve any problems any more (right?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, may be they haven't had yet added handling for merge block
Also the genesis file seems weird, so the genesis block has difficulty and extraData belonging to PoA consensus, but shanghai is also at 0 for withdrawals so obviously merge hf has passed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holgerd77 i think the latest implementation is logical:

place the merge hardfork before any merge dependent hardforks specified on non zero block as genesis can't be a PoS block.

Our dynamic handling of hardfork sequences and hardfork changes can handle the rest.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@g11tech g11tech force-pushed the g11tech/fix-geth-genesis-parse branch 2 times, most recently from 055c9fb to 3aa4ad6 Compare November 19, 2022 07:57
@g11tech g11tech changed the title common: Set merge hardfork in parseGethParams before hardforks scheduled post genesis if terminalTotalDifficultyPassed common: Parse post-merge hardfork config in parseGethParams and handle post-merge genesis block Nov 19, 2022
@g11tech g11tech force-pushed the g11tech/fix-geth-genesis-parse branch from b1d4a79 to e54b1f7 Compare November 19, 2022 10:37
@holgerd77
Copy link
Member

Some general notes here: I generally find/found this Geth genesis file format only half-thought-through from the beginning, that was actually one main motivation why I introduced our own format within Common at the time.

So, my overall fear is always a bit that we are bringing in some clutter in our main (Common, partly other) code logic by trying to cope with this semi-defined structure from Geth genesis, particularly when new things are added there.

So in this case the (Merge) HF order should somewhat be extractable/readable from the Geth config file itself, since this is just some basic structural information piece one would need to build upon. Apparently it is not though, with these Merge-parameters extracted/separated from the other HF definitions.

Just some context/thinking from my side. Otherwise, long story short: since this PR only touches the Geth import logic in Common, I would be ok with merging.

Have I otherwise forgotten or overlooked something in my summary above or is there still some outstanding question which needs to be adressed? 🤔

@g11tech
Copy link
Contributor Author

g11tech commented Nov 22, 2022

Some general notes here: I generally find/found this Geth genesis file format only half-thought-through from the beginning, that was actually one main motivation why I introduced our own format within Common at the time.

So, my overall fear is always a bit that we are bringing in some clutter in our main (Common, partly other) code logic by trying to cope with this semi-defined structure from Geth genesis, particularly when new things are added there.

So in this case the (Merge) HF order should somewhat be extractable/readable from the Geth config file itself, since this is just some basic structural information piece one would need to build upon. Apparently it is not though, with these Merge-parameters extracted/separated from the other HF definitions.

Just some context/thinking from my side. Otherwise, long story short: since this PR only touches the Geth import logic in Common, I would be ok with merging.

Have I otherwise forgotten or overlooked something in my summary above or is there still some outstanding question which needs to be adressed? thinking

Yes very valid points @holgerd77 , hopefully with this PR the merge hardfork placement is now very deterministic and is based on these factors:

  • Merge hardfork can't be placed at genesis
  • Place merge hf before any hardforks that require CL participation for e.g. withdrawals
  • Merge hardfork has to be placed just after genesis if any of the genesis hardforks make CL necessary for e.g. withdrawals

Have added the same in the comment in parseGeth..

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using these changes in my EIP4844 PR and it works like a charm with the latest changes.

@holgerd77
Copy link
Member

I'm using these changes in my EIP4844 PR and it works like a charm with the latest changes.

Ok, nice 🙂, I am also ok with merging here!

@holgerd77 holgerd77 merged commit 33f68c6 into master Nov 22, 2022
@holgerd77 holgerd77 deleted the g11tech/fix-geth-genesis-parse branch November 22, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants