sync: stop wedging on a false "peer does not support blocks_by_range" - #1046
Merged
Conversation
…default)
Add an optional Reed-Solomon erasure-coded broadcast transport that runs
alongside libp2p/gossipsub, mirroring ethlambda's ethp2p-broadcast-adapter.
It is doubly gated and off by default on both axes:
* compile-time: `-Dethp2p=true` (build_options, `has_risc0` precedent).
When false, `ethp2p.zig` selects an inert stub and the `zig_ethp2p`
dependency is never fetched or compiled — the default binary is unchanged.
* run-time: the `ZEAM_ETHP2P` env var (zeam's env-var toggle convention;
a CLI flag would trip zigcli's comptime branch quota).
Adapter (`pkgs/network/src/ethp2p_broadcast.zig`): wraps zig-ethp2p's
`BroadcastNode`, subscribing channels block/aggregation/attestation. Outbound
gossip is teed at the single `Network.publish` chokepoint — SSZ-serialized,
keyed by `message_id = hex(sha256(ssz))`, snappy-compressed, and published
into an RS origin session. The adapter is driven from `onInterval` (same
libxev thread as publish, so no lock), and reconstructed messages are
snappy-decompressed, SSZ-decoded, and reinjected via the gossip handler.
Docker: `--build-arg ETHP2P=true` builds the ethp2p-enabled image.
Depends on zig-ethp2p v0.1.3, whose zquic is sourced from zigstack with a
matching `-Dshadow` option so a build that pulls both zig-ethp2p and
zig-libp2p resolves a single shared zquic module.
A node that fell ~13k slots behind on devnet froze its head for 5+ hours and stormed its peers with 2.4M blocks_by_root requests. Root cause was a chain of sync-layer defects that turned a transient transport blip into an unrecoverable per-block catch-up: 1. peerSupportsBlocksByRange() returned `false` for a peer simply MISSING from the connected map (`orelse return false`), conflating "unknown peer" with "peer lacks the protocol". A peer-map miss was then logged as "peer does not support blocks_by_range" and dropped catch-up into blocks_by_root. Fix: add `blocksByRangeSupport` returning a tri-state (.supported / .unsupported / .unknown); only `.unsupported` (positively observed) blocks range sync. An unknown peer stays eligible — the RPC fails cleanly if it is gone, which is far cheaper than degrading to per-block catch-up. 2. The `.unsupported` flag was set on ANY blocks_by_range send-side failure (node.zig ~1821) — including transport errors (Disconnected/IoError) and local errors (NoBlocksRequested/OOM), none of which say anything about protocol support. The flag is sticky (only reset on reconnect), so a transient blip permanently disabled range sync for that peer; once every peer had been blipped, catch-up could only ever use blocks_by_root. Fix: do not mark on send failure — only the genuine protocol-error path in onReqRespResponse (gated on isBlocksByRangeUnavailable) may set it. 3. Even when correctly flagged unsupported, a LARGE gap still fell through to blocks_by_root, which fetches one block per request and can never outrun block production. Fix: MAX_BLOCKS_BY_ROOT_CATCHUP_GAP (64) — beyond it we refuse to by-root and wait for a range-capable peer. Being visibly stuck is strictly better than a self-DoS that cannot converge. Also documents the sticky-flag hazard on markBlocksByRangeUnavailable so it is not rewired to a transient error again. The by-root retry path already has immediate-retry-then-exponential-backoff (scheduleUnservedRetry); capping the gap bounds how many distinct roots can enter it, which is what collapses the observed request storm.
g11tech
approved these changes
Jul 17, 2026
Contributor
|
Adversarial review result: not approving this as-is. The sync wedge fix itself looks directionally right: unknown peers no longer get treated as range-incapable, send-side failures no longer poison the sticky capability flag, and the by-root gap cap is the right kind of guardrail for the observed retry storm. Findings:
Process/risk note: this PR title/body are about the sync wedge, but the diff also includes the ethp2p adapter, release workflow changes, and hashsig signing-window changes. Those may each be good, but they materially widen the blast radius. I would split or at least retitle the PR and make the validation matrix explicit before merging. I did not submit a formal GitHub approval; per repo convention this is a regular review comment. |
Contributor
Author
|
Thanks for the adversarial review — all three points are valid. Since this PR was already merged, the ethp2p findings are addressed in a focused follow-up: #1047.
The |
ch4r10t33r
added a commit
that referenced
this pull request
Jul 17, 2026
… + CI (#1047) Addresses review on #1046. The experimental ethp2p adapter was wired in the CLI with a hard-coded `.local_peer_id = "zeam-ethp2p"` and no other config, so every node that enabled it (e.g. via the Dockerfile `--build-arg ETHP2P=true` + `ZEAM_ETHP2P=1`) started the transport with the SAME identity and dial-only / no-peers — an identity collision that made the transport unusable and confusing across a fleet. - Derive a unique identity: `ZEAM_ETHP2P_PEER_ID` if set, else `zeam-ethp2p-<node_key_index>`. - Wire the previously-inaccessible peering fields from env (matching the existing `ZEAM_ETHP2P` env-toggle convention): `ZEAM_ETHP2P_LISTEN`, `ZEAM_ETHP2P_SERVER_CERT`, `ZEAM_ETHP2P_SERVER_KEY`, `ZEAM_ETHP2P_STATIC_PEERS` (comma-separated), `ZEAM_ETHP2P_SERVER_NAME`. Strings are allocated from the node's long-lived allocator (borrowed by the adapter for the process; startup-once). - CI: add a compile-only `zig build -Dethp2p=true` step. The adapter and its lazy `zig_ethp2p` dep are only imported under that flag, so default CI could stay green while the path was broken. Runtime activation is unchanged: still off unless built with `-Dethp2p=true` AND `ZEAM_ETHP2P` is truthy.
ch4r10t33r
added a commit
that referenced
this pull request
Jul 17, 2026
…evnet cert Revives #1045 on top of main (which already carries the ethp2p adapter via #1046 and the env-config layer via #1047). Adds the pieces that were only on the old #1045 branch, so an operator can enable ethp2p with just `ZEAM_ETHP2P=1` — no manual port/cert wiring: - `buildEthp2pConfig` derives the ethp2p listen address and static peers from the node's OWN libp2p QUIC addresses, shifted by `ZEAM_ETHP2P_PORT_OFFSET` (default +1, the ethlambda "ethp2p = gossipsub port + 1" convention). Every field keeps explicit-env precedence (`ZEAM_ETHP2P_LISTEN`, `ZEAM_ETHP2P_STATIC_PEERS`, ...) over the derived default. - Identity is the node's `node_key` (unique per node; retained by the RS engine and outlives the process — so the shared TLS cert below is NOT the peer id and cannot cause identity collisions). - Bundled self-signed devnet TLS cert/key at `/app/resources/ethp2p/{cert,key}.pem` (shipped via the existing `COPY resources/`), overridable with `ZEAM_ETHP2P_SERVER_CERT` / `ZEAM_ETHP2P_SERVER_KEY`. - Owned strings (listen_addr, static_peers) are freed via `freeEthp2pConfig` after `beam_node.init` — the adapter's `start` binds/dials synchronously and retains only `local_peer_id` (= long-lived `node_key`). Deliberately does NOT include the old #1045 `pkgs/node/src/node.zig` hunk, which predated and would revert the blocks_by_range sync-wedge fix (#1046). Runtime activation unchanged and doubly gated: `-Dethp2p=true` build AND `ZEAM_ETHP2P` truthy. Default build/test unaffected (adapter comptime-excluded); `-Dethp2p=true` compiles.
ch4r10t33r
added a commit
that referenced
this pull request
Jul 18, 2026
…ts + per-node runtime TLS cert (#1049) * cli(ethp2p): auto-derive endpoints from libp2p QUIC ports + bundled devnet cert Revives #1045 on top of main (which already carries the ethp2p adapter via #1046 and the env-config layer via #1047). Adds the pieces that were only on the old #1045 branch, so an operator can enable ethp2p with just `ZEAM_ETHP2P=1` — no manual port/cert wiring: - `buildEthp2pConfig` derives the ethp2p listen address and static peers from the node's OWN libp2p QUIC addresses, shifted by `ZEAM_ETHP2P_PORT_OFFSET` (default +1, the ethlambda "ethp2p = gossipsub port + 1" convention). Every field keeps explicit-env precedence (`ZEAM_ETHP2P_LISTEN`, `ZEAM_ETHP2P_STATIC_PEERS`, ...) over the derived default. - Identity is the node's `node_key` (unique per node; retained by the RS engine and outlives the process — so the shared TLS cert below is NOT the peer id and cannot cause identity collisions). - Bundled self-signed devnet TLS cert/key at `/app/resources/ethp2p/{cert,key}.pem` (shipped via the existing `COPY resources/`), overridable with `ZEAM_ETHP2P_SERVER_CERT` / `ZEAM_ETHP2P_SERVER_KEY`. - Owned strings (listen_addr, static_peers) are freed via `freeEthp2pConfig` after `beam_node.init` — the adapter's `start` binds/dials synchronously and retains only `local_peer_id` (= long-lived `node_key`). Deliberately does NOT include the old #1045 `pkgs/node/src/node.zig` hunk, which predated and would revert the blocks_by_range sync-wedge fix (#1046). Runtime activation unchanged and doubly gated: `-Dethp2p=true` build AND `ZEAM_ETHP2P` truthy. Default build/test unaffected (adapter comptime-excluded); `-Dethp2p=true` compiles. * cli(ethp2p): generate per-node TLS cert at runtime, drop the committed keypair Follow-up to review feedback: shipping a single self-signed cert/key for all nodes is wrong — libp2p mints its QUIC TLS cert at runtime from the node's identity, and ethp2p must do the same. - Remove the committed resources/ethp2p/{cert,key}.pem. - Add EthLibp2p.generateAuxQuicCertPems() — mints a fresh self-signed cert bound to the node's secp256k1 host identity via the SAME facility the primary libp2p QUIC transport uses (libp2p_tls_cert.generate + a fresh ephemeral cert key). Each call is unique per node and per process; nothing is shipped or shared. - buildEthp2pConfig now, when it listens and no explicit cert env is set, generates the cert at startup and writes the PEMs under the data dir (ethp2p wants file paths — it has no in-memory PEM entry point). Explicit ZEAM_ETHP2P_SERVER_CERT / _KEY still override. Cert/key paths are heap-owned and freed by freeEthp2pConfig after beam_node.init. The runtime cert can't be *literally* reused: the primary transport's cert is generated in EthLibp2p.run(), which happens after beam_node.init sets up the ethp2p listener — so at that point no cert exists yet. Generating a dedicated per-node cert from the same host identity via the same facility gives the identical security property (unique, runtime, never committed). zig build -Dethp2p=true ✅ · default zig build ✅ · zig fmt --check ✅ * ethp2p: skip self-entry when deriving static peers from genesis The genesis peer list (nodes.yaml) always contains this node itself, so the derived ethp2p static_peers included our own listen port. connect() is synchronous and blocks start() in its handshake-poll loop; a self-dial never completes because the server-side accept only runs later in tick(). That stalled the dial loop before it reached the real peers, so no ethp2p peer ever connected. Skip any connect-peer whose libp2p QUIC port matches our own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A node that fell far behind on the devnet froze its head for 5+ hours while its current slot advanced ~18k slots past it, and stormed its peers with 2.4M
blocks_by_rootrequests at ~28/sec. It never recovered. The network itself was healthy and finalizing throughout — this is a single-node sync wedge.The tell in the logs:
The peers do support
blocks_by_range(every other client range-syncs from them fine). This message is a lie caused by a chain of sync-layer defects.Root cause (three compounding defects)
peerSupportsBlocksByRange()conflated "unknown peer" with "unsupported." It didself.map.get(peer_id) orelse return false— a peer simply missing from the connected map was reported as lacking the protocol, and catch-up degraded toblocks_by_root.The sticky
.unavailableflag was set on ANY send failure (node.zig~1821), including transport errors (Disconnected/IoError) and local errors (NoBlocksRequested/OOM) — none of which say anything about protocol support. The flag only resets on reconnect, so a transient blip permanently disabled range sync for that peer; once every peer had been blipped, catch-up could only ever useblocks_by_root.A large gap still fell through to
blocks_by_root, which fetches one block per request and can never outrun block production. A 13,429-slot gap by-root = unrecoverable + self-DoS.Fix
ConnectedPeers.blocksByRangeSupport()now returns a tri-state (.supported/.unsupported/.unknown). Only a positively-observed.unsupportedblocks range sync; an unknown peer stays eligible (the RPC fails cleanly if it's gone — far cheaper than degrading).onReqRespResponse, gated onisBlocksByRangeUnavailable(code, message)) may set the flag.MAX_BLOCKS_BY_ROOT_CATCHUP_GAP = 64: beyond it we refuse to by-root and wait for a range-capable peer. Being visibly stuck is strictly better than a self-DoS that cannot converge.markBlocksByRangeUnavailableso it isn't rewired to a transient error again.The by-root retry path already has immediate-retry-then-exponential-backoff (
scheduleUnservedRetry); capping the gap bounds how many distinct roots can enter it, which collapses the request storm.Tests
ConnectedPeers: blocksByRangeSupport distinguishes unknown from unsupportedpins the tri-state and the reconnect-clears-flag recovery path.zig build testgreen.Note on the transport side
The trigger underneath this — a peer's connection dying and requests then failing with
Disconnected— points at a zig-libp2p / conn-lifecycle question (inbound-only peers not re-dialed; whether a silently-dead conn is always surfaced). The redial + 30s-idle-timeout + 20s-keepalive machinery there is already correct forknownpeers; the residual inbound-only recovery gap is being tracked separately and needs a live repro to fix without reintroducing dial churn. This PR fixes the sync-layer amplifier that turns a transient blip into a multi-hour wedge.