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

core/forkid: implement the forkid EIP, announce via ENR #19738

Merged
merged 7 commits into from
Jul 8, 2019

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Jun 19, 2019

This PR implements the the fork ID EIP specced currently at ethereum/EIPs#2124.

Although the EIP only specifies the fork ID content, this PR also does the ENR integration. As such however, the PR is missing two necessary features:

  • Dynamically update the ENR whenever a fork is passed. This is particularly important when doing an initial sync, since otherwise nodes will advertise themselves being in Frontier until they get restarted.
  • Incorporate the eth.NewENRFilter result somehow into the discovery process to allow blacklisting certain peers and rejecting them already before connection.

Need help from @fjl to finish this up.

eth/enr.go Outdated
for i := 0; i < kind.NumField(); i++ {
// Fetch the next field and skip non-fork rules
field := kind.Field(i)
if !strings.HasSuffix(field.Name, "Block") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a hack. Wouldn't it be better to explicity list the forks instead? But otoh, I guess you want this to 'just work' in the future too... Not sure what the best way is.

@karalabe karalabe changed the title eth: chain config (genesis + fork) ENR entry core/forkid: implement the forkid EIP, announce via ENR Jul 3, 2019
@fjl
Copy link
Contributor

fjl commented Jul 8, 2019

Pushed a big update. The record now auto-updates every block.
This change also disables protocol version eth/62 because it was just too messy
to move the protocol construction and keep the check for fast-sync at the same time.

I will clean up the ENR handling more after 1.9.0 is out. It's a bit annoying that we need to
declare the entry in Attributes and also auto-update it. We need to do this for now because
the local record should be up-to-date when p2p.Server starts.

@karalabe
Copy link
Member Author

karalabe commented Jul 8, 2019

Integration seems good to me. A slight issue is that we don't fire chain head events during fast sync, so that won't trigger the fork reevaluation, but I think we can live with it for now. Will open an issue to track this limitation for 1.9.x.

@karalabe karalabe merged commit 983f923 into ethereum:master Jul 8, 2019
@wanwiset25 wanwiset25 mentioned this pull request Jun 3, 2024
19 tasks
wanwiset25 pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Jun 19, 2024
* eth: chain config (genesis + fork) ENR entry

* core/forkid, eth: protocol independent fork ID, update to CRC32 spec

* core/forkid, eth: make forkid a struct, next uint64, enr struct, RLP

* core/forkid: change forkhash rlp encoding from int to [4]byte

* eth: fixup eth entry a bit and update it every block

* eth: fix lint

* eth: fix crash in ethclient tests
wanwiset25 pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Jun 28, 2024
* eth: chain config (genesis + fork) ENR entry

* core/forkid, eth: protocol independent fork ID, update to CRC32 spec

* core/forkid, eth: make forkid a struct, next uint64, enr struct, RLP

* core/forkid: change forkhash rlp encoding from int to [4]byte

* eth: fixup eth entry a bit and update it every block

* eth: fix lint

* eth: fix crash in ethclient tests
wanwiset25 added a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants