erigontech / erigontech/erigon

EIP-2124 fork ID is checked too late, and never during discovery

Open
#23,534 1 comment 0 reactions 0 assignees View on GitHub
Networking tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

EIP-2124 gives us a fork ID so that we can drop incompatible peers early. We check the fork ID, but only after the connection is fully up. We never use it when we choose whom to dial.

*(This body was rewritten after the comparison with go-ethereum below. The earlier version overstated the steady-state cost and suggested copying geth directly. See the comments for the working notes.)*

## Where the check runs today

The filter itself is correct. It lives in `p2p/forkid/forkid.go` and implements all four rules of the EIP.

The problem is when we call it. For every peer we do this in order:

1. TCP connect (`p2p/server.go:954`)
2. RLPx encryption handshake (`p2p/server.go:972`)
3. peer-count and IP checks (`p2p/server.go:1002`)
4. devp2p `Hello` and capability negotiation (`p2p/server.go:1009`)
5. take a peer slot and start the peer (`p2p/server.go:1020`, `p2p/server.go:1075`)
6. start the eth protocol and exchange `Status` (`p2p/sentry/sentry_grpc_server.go:934`)
7. **only now** run the fork filter (`p2p/sentry/eth_handshake.go:88`)

A peer on a different fork costs us a full TCP connection, two handshakes, a peer slot, and one `Status` message each way. We then disconnect it with `DiscUselessPeer` (`p2p/sentry/eth_handshake.go:59`).

We publish our own `eth` ENR entry (`p2p/sentry/sentry_grpc_server.go:1744`, refreshed on every `SetStatus`), but we never read anybody else's. `enrEntry` in `p2p/protocols/eth/discovery.go:29` has no decode path in the repo.

## What go-ethereum does

Geth has the same handshake check (`eth/protocols/eth/handshake.go:100`). It also has a discovery filter, but a narrow one: `eth.NewNodeFilter` is applied to the discv5 source only (`eth/backend.go:409-411`).

It cannot be applied to discv4. A discv4 lookup learns nodes from `neighbors` packets, which carry only ID, IP and ports, so the iterator yields synthetic records with no `eth` entry. ENR retrieval in discv4 exists (EIP-868) but needs an extra round-trip per node.

Geth does not filter its DNS sources either. The reason appears to be that the lists are already filtered when they are published — see below.

## Why our situation is different

We do not use the DHT as a dial source in the normal setup. `p2p/server.go:588-603` adds discv4/discv5 random nodes only when no protocol supplied dial candidates, and on mainnet sentry always supplies a DNS iterator (`node/components/sentry/provider.go:344-345`). So our dialing comes from DNS, plus static and bootnodes.

The DNS list is already fork-filtered at publication time. The `devp2p` crawler fetches the full ENR of every node it finds (`cmd/devp2p/crawl.go:194`), and `devp2p nodeset filter -eth-network mainnet` drops nodes that fail a fork check (`cmd/devp2p/nodesetcmd.go:141`, `228-251`).

**But that filter is static.** It uses `forkid.NewStaticFilter`, which pins the head to `(0, 0)` (`core/forkid/forkid.go:126-129`). At head zero, rule 3 accepts any node whose checksum is a valid prefix of the chain's fork history. So the published list answers "is this node on mainnet", not "is this node compatible with our current head". The crawler cannot answer the second question, because it does not know our head.

## Why it still matters

The gap is the hard-fork case.

Take a node that has not upgraded past fork F, while we have. Its checksum is still a valid mainnet prefix, so the crawl-time filter keeps it in the list. Our live filter rejects it under rule 2 with `ErrRemoteStale`. We keep dialing such nodes from DNS and dropping them at step 7, for as long as the list stays stale.

So the cost is not constant. It is small in steady state and rises around a fork, which is exactly when we least want to waste peer slots.

Two things this cannot fix, worth stating so nobody expects them:

- Inbound connections. We do not choose those, so only the handshake check applies.
- Bootnodes and static peers. We dial them on purpose.

## Suggested work

1. Add a decoder for `eth.enrEntry`, so we can read the `eth` key from a remote record.
2. Wrap the DNS dial iterator with a live fork filter. `enode.Filter` already exists (`p2p/enode/iter.go:113`), so the wiring is small. Note this goes further than geth, on purpose: a live filter catches stale nodes that a static crawl-time filter cannot.
3. Accept, do not reject, when the entry is missing or does not decode. Geth rejects, but geth's filter sits on one source among several. This would be our only source, so a strict rule would cut us off from peers that publish no entry.
4. Keep the handshake check as it is. It is still needed for inbound peers and for stale records.
5. Add counters for peers rejected at discovery and peers rejected at handshake, so we can see whether the filter earns its place.

## Open questions

- How many peers do we actually reject at handshake today? Nobody has measured this. Item 5 should probably land first, on its own, before anyone writes the filter.
- Is the `len(added) == 0` fallback in `p2p/server.go:597` deliberate? It means we drop the discv4 and discv5 sources completely whenever DNS discovery is configured. That is a different question from this one and may deserve its own issue.

Contributor guide

Open the contributing guide

Research direction

Start by reading the current handshake path in p2p/sentry/eth_handshake.go and the discovery wiring in p2p/server.go, then inspect the eth ENR handling in p2p/protocols/eth/discovery.go. Item 5 suggests first adding counters for discovery and handshake rejections so the current impact can be measured. Done should include the requested live DNS filtering, tolerant missing-entry handling, and preserved handshake checks, with tests added where the implementation identifies coverage points.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
blockchain, networking
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.