Skip to content

Revert #2237 — forkid wire-format change is a breaking p2p change#2248

Open
lucca30 wants to merge 1 commit into
developfrom
lmartins/revert-2237-forkid-breaking-change
Open

Revert #2237 — forkid wire-format change is a breaking p2p change#2248
lucca30 wants to merge 1 commit into
developfrom
lmartins/revert-2237-forkid-breaking-change

Conversation

@lucca30
Copy link
Copy Markdown
Contributor

@lucca30 lucca30 commented May 27, 2026

Summary

Reverts #2237 ("core/forkid: include polygon-specific forks in wire forkid").

This is a breaking p2p change that was not flagged as such when #2237 merged.

What went wrong

#2237 swapped gatherForks (lowercase, reflection-based, outer ChainConfig only) for GatherForks (uppercase, polygon-aware — also walks config.Bor.{Jaipur,Delhi,Indore,Ahmedabad,Bhilai,Rio,Madhugiri,MadhugiriPro,Dandeli,Lisovo,LisovoPro,Giugliano,Chicago}Block) in the wire-forkid path (core/forkid/forkid.go::NewID, NewStaticFilter).

This is correct in intent — bor's old forkid was lying about which forks were active and erigon ≥3.6.0 already includes these blocks — but changing what gets hashed in the wire forkid is, by definition, a coordinated-upgrade event. Any node running the new code computes a different forkid than nodes running the old code on the same chain, and the eth handshake fails with fork ID rejected: local incompatible or needs update.

Live evidence

Deployed v2.8.3-beta3 (which contains #2237) to an Amoy observation node (posdevnodes-amoy-2) on 2026-05-27. Symptoms within 5 minutes:

  • Peer count oscillated 0 ↔ 1 (no stable handshakes).
  • Looking for peers peercount=0 tried=10–35 static=13 every 10 s — static peers being dialed continuously, all dropped immediately.
  • Chain stuck at the same block; no new imports.
  • No errors at INFO level — silent rejection at the handshake layer.

Rolling the same node back to v2.8.0-beta2 (which does not contain #2237) restored peers to ~18 within seconds. Confirms the diagnosis: it's the forkid hash mismatch, not anything else in the beta3 delta.

This will affect mainnet identically the moment a v2.8.3 (with #2237) bor talks to any v2.8.2-or-older bor, because every Polygon-specific fork above is also activated on mainnet at non-zero blocks.

Why a plain revert, not a fix-forward

There is no algorithmic trick that makes a forkid hash change non-breaking on a chain where the affected forks have already activated. The only valid rollout strategies are:

  1. Gate the new GatherForks behavior on a future fork block. Introduce a new BorConfig.ForkidFixBlock (or piggyback on the next planned consensus fork). Before that block: use legacy gatherForks for wire forkid computation. At and after: use GatherForks. Everyone deploys the binary well in advance; the protocol-level switchover is atomic at the block boundary. This is the canonical Ethereum pattern.
  2. Coordinate a flag-day rollout with a config flag defaulting to legacy, flipped chain-wide on a known date.

Either approach should be a follow-up PR, designed and reviewed as a network upgrade. This PR just unblocks the existing release line.

What needs to follow

  • Follow-up PR re-introducing the polygon-aware forkid behavior gated on a ForkidFixBlock (or equivalent) — must include chain-config entries for mainnet and amoy, an explicit announcement to operators, and a soak period on devnet first.
  • Release notes for the upcoming v2.8.x lineage should call out that the forkid fix is deferred to a coordinated upgrade.

Test plan

  • go build ./... passes after revert
  • Same revert applied to v2.8.3-candidate and shipped as v2.8.3-beta4 — node recovered peers + chain progression on Amoy
  • Reviewer to confirm core/forkid/forkid.go matches pre-core/forkid: include polygon-specific forks in wire forkid #2237 state (only the two GatherForksgatherForks call sites)

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@sonarqubecloud
Copy link
Copy Markdown

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.85%. Comparing base (d99ab21) to head (0f03fad).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2248      +/-   ##
===========================================
- Coverage    52.86%   52.85%   -0.02%     
===========================================
  Files          886      886              
  Lines       156811   156811              
===========================================
- Hits         82900    82883      -17     
- Misses       68663    68677      +14     
- Partials      5248     5251       +3     
Files with missing lines Coverage Δ
core/forkid/forkid.go 76.69% <100.00%> (ø)

... and 22 files with indirect coverage changes

Files with missing lines Coverage Δ
core/forkid/forkid.go 76.69% <100.00%> (ø)

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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