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

block, common: set constants block headers fields for The Merge #1393

Merged
merged 16 commits into from Aug 7, 2021

Conversation

gabrocheleau
Copy link
Contributor

@gabrocheleau gabrocheleau commented Aug 5, 2021

This PR

  • Adds EIP-3675 to the supported EIPs
  • Sets the relevant block header fields as constants if EIP-3675 is activated
  • Adds some basic tests for merge blocks

@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #1393 (1af4adc) into master (3a989d0) will decrease coverage by 1.32%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 86.71% <100.00%> (+0.59%) ⬆️
blockchain 83.49% <ø> (ø)
client 83.45% <ø> (-0.25%) ⬇️
common 94.02% <100.00%> (+0.44%) ⬆️
devp2p 83.39% <ø> (-0.25%) ⬇️
ethash 82.83% <ø> (ø)
tx 88.36% <ø> (+0.11%) ⬆️
vm 79.31% <ø> (-3.46%) ⬇️

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

@gabrocheleau gabrocheleau changed the title Block & Common: set constants block headers for The Merge block, common: set constants block headers for The Merge Aug 6, 2021
@gabrocheleau gabrocheleau changed the title block, common: set constants block headers for The Merge block, common: set constants block headers fields for The Merge Aug 6, 2021
packages/block/src/header.ts Outdated Show resolved Hide resolved
packages/block/src/header.ts Outdated Show resolved Hide resolved
packages/block/src/header.ts Outdated Show resolved Hide resolved
packages/block/test/util.ts Outdated Show resolved Hide resolved
@holgerd77 holgerd77 force-pushed the feat/the-merge-constants branch 3 times, most recently from 38f64d8 to a201a59 Compare August 6, 2021 07:54
@holgerd77
Copy link
Member

Ok, I've rebased here and squashed several commits.

I already removed the CHANGELOG entries again as per my comment on Discord. It is also generally far too early for release notes on this I guess since we are far far from the situation that we have even a somewhat clear picture on where this will lead too. 😋

Sorry that I am cutting so harshly through the woods here and also don't worry. It's actually super couragious that you gave this some start, great that we even already have some initial test cases! 😀

I've some more comments, will do separately a bit later.

@holgerd77
Copy link
Member

holgerd77 commented Aug 6, 2021

This EIP-3675 config in Common definitely needs to be added as some base selector, just to confirm the existing work. And I think we can (and should) also directly add "The Merge" as a hardfork. As some note on the switch discussion: if you want to do this "by hardfork" you can use the gte* functionality from Common (instead of ===), so this is e.g. from the VM if (this._common.gteHardfork('byzantium'), just for illustration.

What I had in mind as an additional switch is the ability to switch the consensus engine (type) setting in Common (see e.g. goerli.json also on a per-hardfork basis (in addition to the existing per-chain basis) and adopt the associated Common methods consensusType(), consensusAlgorithm() and consensusConfig() accordingly.

The advantage of this is that this frees us from this process of always needing to go through this Merge HF for getting into a PoS state and it would allow e.g. also for things like setting up a pure PoS test network which just runs on PoS from the beginning.

So the differentiation would be: for everything which is just pure PoS and is not just triggered by this Merge transition we should use a `common.consensusType() === 'pos' switch (and at some point it would really be nice to also switch to enums there, especially with this close 'poa', 'pos',... strings). For everthing which just one-time happens if there is an Eth1 chain available which need to be transitioned we should use checks based on the EIP or hardfork (I would actually prefer the hardfork checks since in this case we will (likely?) only have one EIP anyhow and this would improve readability I guess).

So in the case of these validation checks and header field adjustments these would clearly be consensusType checks since this is just about a block being instantiated as a PoS block.

Does this make sense?

I would directly integrate this in Common now and push here later on.

@holgerd77
Copy link
Member

(please read the last comment on site, many later-on updates and corrections)

@holgerd77
Copy link
Member

@gabrocheleau Ok, handinging it over again. 😄

I know I've edited an insane amount of the initial code, but really nothing to worry about. Getting a start on these specs is pretty pretty hard, especially when it comes to such unchartered territory like the Merge and it's really great that we have gotten a start on really substantially modfiying and updating our base libraries.

@holgerd77
Copy link
Member

Ok, I just checked from this list from the specification:

Remove verification of the block’s difficulty value with respect to the difficulty formula.
Remove verification of the block’s nonce and mixHash values with respect to the Ethash function.
Remove all validation rules that are evaluated over the list of ommers and each member of this list, except for enforcing this list to be empty.
Add verification of the fields noted in block structure as their specified constant value, including the enforcement of empty ommers list.
Add verification of block validity with respect to the PoS consensus rules, i.e. assert that there MUST be a corresponding event for the block to consider it valid. Such events are limited to the following:
    A POS_CONSENSUS_VALIDATED event considering the block or any of its descendants as valid.
    A POS_CHAINHEAD_SET event, including the POS_BLOCK_FINALIZED form of this event, designating the block or any of its descendants as the head of the canonical chain.

I guess apart from this event based check in the last rule - which is likely happening on another level together with client communication or something - we already have the relevant checks covered (some are just implicitly covered, e.g. these uncle checks we have in our library now always pass since uncles are guaranteed to be null.

So eventually if things are otherwise accepted on review we might already be able to merge this in.

packages/common/src/index.ts Outdated Show resolved Hide resolved
@@ -59,6 +59,7 @@ export enum Hardfork {
Berlin = 'berlin',
London = 'london',
Shanghai = 'shanghai',
TheMerge = 'theMerge',
Copy link
Contributor

@ryanio ryanio Aug 6, 2021

Choose a reason for hiding this comment

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

@holgerd77 what do you think about just calling it merge for short? (what do others think?)

Copy link
Contributor

Choose a reason for hiding this comment

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

either merge or theMerge(tm) Them (tm) is important!

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's do merge 😛 😅 (feel free everyone to update on occasion).

Comment on lines +960 to +962
* e.g. "ethash" for "pow" consensus type,
* "clique" for "poa" consensus type or
* "casper" for "pos" consensus type.
Copy link
Contributor

Choose a reason for hiding this comment

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

after using enums for chain and hardfork I wish we also had them for these 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, for sure. Note sure, maybe we keep the strings in our json files, but expose these enums for users and also use these internally for comparisons in the code base?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds nice to me, we took the same approach with the chain/hardfork enums by just introducing them as additional input options first.

packages/common/src/index.ts Outdated Show resolved Hide resolved
@ryanio ryanio marked this pull request as ready for review August 7, 2021 02:18
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

wohoo, looks great, nice work everyone @gabrocheleau @holgerd77 @acolytec3!

@ryanio ryanio merged commit ae96604 into master Aug 7, 2021
@ryanio ryanio deleted the feat/the-merge-constants branch August 7, 2021 02:26
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.

None yet

5 participants