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

Implement EIP-7642: eth/69 - Drop pre-merge fields #13010

Open
yperbasis opened this issue Dec 5, 2024 · 2 comments
Open

Implement EIP-7642: eth/69 - Drop pre-merge fields #13010

yperbasis opened this issue Dec 5, 2024 · 2 comments
Labels
imp2 Medium importance

Comments

@yperbasis
Copy link
Member

yperbasis commented Dec 5, 2024

No description provided.

@yperbasis yperbasis self-assigned this Dec 5, 2024
@yperbasis yperbasis added imp1 High importance imp2 Medium importance and removed imp1 High importance labels Dec 5, 2024
@yperbasis
Copy link
Member Author

yperbasis commented Dec 12, 2024

Warning: eth/69 is probably not compatible with Polygon:

Andrew Ashikhmin — 18.11.2024, 16:10
@mark Holt would eth/69 (https://eips.ethereum.org/EIPS/eip-7642) be OK for Polygon?

Mark Holt — 18.11.2024, 18:44
This will break the pos sync process. I think it also uses the TD field but am less certain how usable this is. So I guess that pos nodes will just not move to eth/69. So I guess it will mean that polygon nodes just stick at eth/68.
I think it would be better to just drop the Bloom, which seems like a gain and not make the additional changes. Removing td seems of minor benefit, and removing the unused messages seems to be of no practical benefit.

funnygiulio — 19.11.2024, 00:25
talk to polygon guys and ask them to drop TD on the handshake
i gurantee they will say yes

Mark Holt — 19.11.2024, 10:41
I have just checked the code and agree we can safely drop TD we're not referencing it anywhere other than tests.
NewBlockHashes and NewBlock are used to announce blocks so should not be dropped.

milen — 19.11.2024, 10:54
you also need to check the maticnetwork/bor codebase, not just ours
and once you start looking you will find that they are using it 🙂
we've had this conversation before

Mark Holt — 19.11.2024, 10:56
I think that is different, they are using the TD in the header - I'll check what they do with the TD on the status message.

milen — 19.11.2024, 10:57
No, it is the TD from the devp2p handshake

Mark Holt — 19.11.2024, 11:09
Ok I see they use it for peer selection. I'll talk to them this afternoon. See if they know about this eip and want to raise it as an issue.

Manav Darji — 19.11.2024, 15:51
Yes, right. We use TD from the handshake in maticnetwork/bor for peer selection during sync.
Can you remind me how does erigon choose which peer to sync from? If we can implement something similar for bor, it should be managable.

milen — 19.11.2024, 17:06
the current way for polygon (pending decommission) is via the PoW headers stage which:

  • uses a tree of anchors to poll new headers from peers here, essentially polling many peers (the leafs of this tree)
  • it uses parentTd + header.Difficulty to determine which fork to take here and here
  • this differs slightly with how bor does it with the peerWithHighestTD as the Erigon approach does not use the TD from the devp2p handshake

in Astrid (new way, does not use headers stage) we went even further and use a "localised td" approach where we reset the td=0 when we reach the latest milestone and reset it back to 0 again after every new milestone received after that, so overall it looks like this:

  • on initial sync fetch all checkpoints from heimdall, then start downloading blocks in parallel from all peers (1 checkpoint block range per peer), penalizing any peers that give us blocks that do not match the checkpoint root hash
  • do the same for milestones until we reach the latest one
  • then go into chain tip mode that manages the forks using the localised td idea and listens for new block and new milestone events
  • code starting point for that is here

@yperbasis yperbasis changed the title Finalize spec and implement eth/69 Implement EIP-7642: eth/69 - Drop pre-merge fields Dec 12, 2024
@yperbasis
Copy link
Member Author

ethereum/EIPs#9133

@yperbasis yperbasis removed their assignment Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imp2 Medium importance
Projects
None yet
Development

No branches or pull requests

1 participant