feat: enable QUIC on devp2p - #2392
vbhattaccmu wants to merge 21 commits into
Conversation
…s and witness-gated imports
…lback verification
…yle bulk paths, doc updates
…h-state Amoy soak harness
Preserve transaction request IDs across the QUIC-routed fetch lane and update the witness-manager test for the current WIT2 callbacks.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ServerName: u.Hostname(), | ||
| } | ||
| if isLocalhost(u.Hostname()) { | ||
| tlsConf.InsecureSkipVerify = true |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new transport path and wide protocol-routing changes in critical networking code, and there are blocking issues identified in the current diffs that should be addressed before merge.
Pull request overview
This PR introduces an opt-in QUIC “bulk sidecar” transport alongside the existing devp2p/RLPx connection, and adds an opt-in HTTP/3 (QUIC) client path for Bor→Heimdall communication (h3:// URLs). It touches Bor’s P2P server lifecycle, per-protocol message routing for eth/snap/wit, and adds Docker/scripts/docs to exercise the new transport.
Changes:
- Add a QUIC sidecar to
p2p.Serverand route selected protocol messages over named “lanes”, with fallback to the primary RLPx lane. - Add Heimdall HTTP client support for
h3://endpoints (HTTP/3 transport) and propagate the new constructor signature ((*Client, error)). - Add integration/soak harnesses (Docker + scripts) and new tests validating routing and fallback behavior.
File summaries
| File | Description |
|---|---|
| scripts/test_heimdall_quic_integration.sh | Local integration harness to run Heimdall QUIC sidecar + Bor client tests/soak. |
| packaging/docker/test-bulk-sidecar.sh | Docker-based smoke test that peers two nodes via ENR and checks sidecar channel bring-up via logs. |
| packaging/docker/README-sidecar-vm.md | Operator-facing instructions for enabling and validating the QUIC sidecar on VMs. |
| packaging/docker/README-amoy-sidecar-pair.md | Documentation for reusable Amoy sidecar test pair workflow. |
| packaging/docker/bor-bulk.toml | Example config enabling sidecar + JSON-RPC for a single-node docker-compose. |
| packaging/docker/bor-bulk-b.toml | Two-node bulk test config (node B) with sidecar enabled. |
| packaging/docker/bor-bulk-a.toml | Two-node bulk test config (node A) with sidecar enabled. |
| packaging/docker/bor-amoy-sidecar-observer.toml | Observer node config for Amoy sidecar test/bench harness. |
| packaging/docker/bor-amoy-sidecar-b.toml | Amoy sidecar pair node B config pinned to hash scheme. |
| packaging/docker/bor-amoy-sidecar-a.toml | Amoy sidecar pair node A config pinned to hash scheme. |
| packaging/docker/amoy-sidecar-pair.sh | Automation for pairing/checking/soaking/recovering an Amoy sidecar node pair. |
| packaging/docker/amoy-sidecar-bench.sh | Benchmark harness for comparing TCP vs QUIC sidecar behavior in docker. |
| p2p/server.go | Adds BulkSidecar lifecycle management and publishes QUIC endpoint in ENR. |
| p2p/server_test.go | Tests ENR QUIC endpoint preservation and QUIC record publishing for unspecified listeners. |
| p2p/routed_message.go | Adds routed/multi-lane MsgReadWriter to multiplex reads and route writes by message code/channel. |
| p2p/metrics.go | Adds p2p/bulk/* metrics for sessions, channels, read errors/timeouts, and write fallbacks. |
| p2p/config.go | Adds config fields to enable sidecar and configure sidecar listen address. |
| p2p/config_toml.go | Adds TOML marshal/unmarshal support for new sidecar config fields. |
| p2p/bulk_sidecar_test.go | Unit tests for sidecar session/channel behavior and routed read/write semantics. |
| p2p/bulk_sidecar_status.go | Adds sidecar status snapshot + counters and qlog-based wire counters. |
| node/defaults.go | Defaults EnableBulkSidecar to false in node config. |
| miner/worker_test.go | Stabilizes goroutine-leak assertions by pre-initializing core.SenderCacher() and removing timing jitter. |
| internal/cli/server/testdata/default.toml | Adds P2P sidecar config keys to the default CLI config template. |
| internal/cli/server/flags.go | Adds --bulk-sidecar and --bulk-port flags; documents h3:// Heimdall URL option. |
| internal/cli/server/config.go | Wires CLI config to P2P sidecar enablement + bulk listen port derivation. |
| internal/cli/server/config_test.go | Tests buildNode wiring for sidecar enablement and bulk listen address. |
| go.mod | Bumps Go version; adds quic-go dependency; bumps go.uber.org/mock. |
| go.sum | Adds quic-go/qpack sums and updates go.uber.org/mock sums. |
| eth/protocols/wit/peer.go | Wraps wit peer RW with routed writer and adds bulk RW attach/health helpers. |
| eth/protocols/wit/handler.go | Removes debug logging noise from witness handler paths. |
| eth/protocols/wit/bulk_rw_test.go | Adds tests validating wit traffic routing over bulk lane and handler consumption. |
| eth/protocols/snap/requests.go | Adds request/response tracking primitives for snap responses delivered via sinks. |
| eth/protocols/snap/protocol.go | Adds requestID() helpers for snap response packets. |
| eth/protocols/snap/peer.go | Adds snap multi-lane routing + bulk RW attach helpers + sink-based request helpers. |
| eth/protocols/snap/handler.go | Dispatches snap responses to pending requests before backend handling. |
| eth/protocols/snap/bulk_rw_test.go | Adds tests validating snap lane routing and handler consumption via bulk lanes. |
| eth/protocols/eth/peer.go | Adds eth multi-lane routing + bulk RW attach helpers + message→lane mapping. |
| eth/protocols/eth/peer_test.go | Adds tests validating eth traffic routing per lane and status remaining on primary. |
| eth/protocols/eth/handlers.go | Adds concurrency bounding for heavy responses; returns empty body for genesis when needed; adds trace logs for announcements. |
| eth/protocols/eth/handler.go | Introduces a global worker-limit channel for heavy body/receipt serving. |
| eth/protocols/eth/handler_test.go | Tests that genesis empty body queries still return a body payload. |
| eth/handler.go | Attaches bulk sidecar channels for eth/snap/wit peers post-handshake. |
| eth/fetcher/witness_manager.go | Handles nil witness request returns defensively and improves pending-block error path. |
| eth/fetcher/witness_manager_test.go | Adds test ensuring nil witness request keeps pending entry for retry. |
| eth/fetcher/tx_fetcher_test.go | Adds test ensuring pooled-tx fetch requests route over bulk lane when attached. |
| eth/fetcher/block_fetcher_test.go | Adds tests for bulk-lane header/body fetching and witness-required import gating. |
| eth/ethconfig/config.go | Propagates Heimdall client constructor errors instead of ignoring them. |
| docs/cli/server.md | Updates CLI docs to mention h3:// Heimdall option; formatting tweak. |
| Dockerfile.alltools | Bumps Go image version. |
| Dockerfile | Switches build step to explicit go build with optional PGO and GitCommit ldflag; adds UDP 30304 exposure; installs wget. |
| docker-compose.yml | Adds a simple Bor service exposing UDP 30304 and mounting a sidecar-enabled config. |
| docker-compose.bulk-test.yml | Adds two-node compose harness for bulk sidecar bring-up checks. |
| docker-compose.amoy-sidecar-pair.yml | Adds three-node compose harness for Amoy sidecar pair + observer workflows. |
| consensus/bor/heimdallgrpc/client.go | Handles Heimdall REST client constructor errors when building gRPC client wrapper. |
| consensus/bor/heimdall/failover_client_test.go | Updates tests to handle Heimdall client constructor errors. |
| consensus/bor/heimdall/client.go | Adds HTTP/3 (h3://) Heimdall support with TLS config and transport close handling. |
| consensus/bor/heimdall/client_test.go | Updates tests for new Heimdall client constructor signature. |
| consensus/bor/heimdall/client_h3_test.go | Adds QUIC (HTTP/3) unit/integration tests for Heimdall client. |
| cmd/utils/bor_flags.go | Updates Heimdall flag help text to mention h3:// option. |
| cmd/keeper/go.sum | Updates keeper module dependency sums (x/time, protobuf). |
| cmd/keeper/go.mod | Bumps keeper module Go version and x/time dependency. |
| builder/files/config.toml | Documents new P2P bulk-sidecar settings in the example config template. |
| .dockerignore | Adds ignores for .git, local caches, and prod.pprof. |
Review details
Suppressed comments (1)
scripts/test_heimdall_quic_integration.sh:315
- This mktemp invocation hard-codes /private/tmp, which is not available on all platforms. Use the TMPROOT/TMPDIR-derived base directory instead.
- Files reviewed: 65/67 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| listenAddr := srv.BulkListenAddr | ||
| if listenAddr == "" { | ||
| listenAddr = srv.ListenAddr | ||
| if listenAddr == "" { | ||
| listenAddr = ":0" | ||
| } | ||
| } |
| .git | ||
| .gocache | ||
| .gomodcache | ||
| prod.pprof |
| compose_file="${COMPOSE_FILE:-docker-compose.amoy-sidecar-pair.yml}" | ||
| project_name="${COMPOSE_PROJECT_NAME:-bor}" | ||
| log_tail_lines="${LOG_TAIL_LINES:-2000}" | ||
| rounds="${ROUNDS:-5}" | ||
| benchmark_dir="${BENCHMARK_DIR:-/private/tmp/bor-amoy-sidecar-bench}" | ||
|
|
||
| template_a="${TEMPLATE_A:-/Users/djones/Github/bor/packaging/docker/bor-amoy-sidecar-a.toml}" | ||
| template_b="${TEMPLATE_B:-/Users/djones/Github/bor/packaging/docker/bor-amoy-sidecar-b.toml}" | ||
| template_observer="${TEMPLATE_OBSERVER:-/Users/djones/Github/bor/packaging/docker/bor-amoy-sidecar-observer.toml}" |
| compose_file="${COMPOSE_FILE:-docker-compose.amoy-sidecar-pair.yml}" | ||
| gocache_dir="${GOCACHE_DIR:-/Users/djones/Github/bor/.gocache}" | ||
| gomodcache_dir="${GOMODCACHE_DIR:-/Users/djones/Github/bor/.gomodcache}" | ||
| log_tail_lines="${LOG_TAIL_LINES:-2000}" | ||
| preserve_volumes="${PRESERVE_VOLUMES:-0}" |
| BOR_GOCACHE="${BOR_GOCACHE:-/private/tmp/bor-gocache}" | ||
| BOR_GOMODCACHE="${BOR_GOMODCACHE:-/private/tmp/bor-gomodcache}" | ||
| HEIMDALL_GOCACHE="${HEIMDALL_GOCACHE:-/private/tmp/heimdall-v2-gocache}" | ||
| HEIMDALL_GOMODCACHE="${HEIMDALL_GOMODCACHE:-/private/tmp/heimdall-v2-gomodcache}" | ||
| TEST_CHAIN="${TEST_CHAIN:-local}" | ||
| SOAK_ITERATIONS="${SOAK_ITERATIONS:-1}" | ||
| SOAK_SLEEP_SECONDS="${SOAK_SLEEP_SECONDS:-0}" | ||
| RUN_REAL_BOR="${RUN_REAL_BOR:-false}" | ||
| BOR_SERVER_SECONDS="${BOR_SERVER_SECONDS:-120}" | ||
|
|
||
| HEIMDALL_HOME="$(mktemp -d /private/tmp/heimdall-quic-home.XXXXXX)" |
…cate check' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
@claude review |
There was a problem hiding this comment.
🔵 Needs a closer look
There are correctness/concurrency issues (notably non-idempotent Heimdall client Close and potential routed bulk-lane replacement leaks) plus portability problems in new harness scripts that should be resolved before approval.
Review details
Suppressed comments (9)
Previously missed (1) — in code that hasn't changed since the last review.
p2p/routed_message.go:205
AttachBulkChannelcan be called repeatedly (e.g., during reconnect/recovery).setBulkoverwrites the lane but never closes the previous read-writer, so old read loops may remain blocked onReadMsg()and leak goroutines/resources. Consider closing the previous lane when it supportsio.Closerbefore replacing it.
.dockerignore:9
prod.pprofis ignored by.dockerignore, so the Dockerfile's optional PGO path (if [ -f prod.pprof ]) can never activate when building from a working tree. Either allowprod.pprofinto the build context or switch the Dockerfile to ingest it via a BuildKit mount/secret.
.git
.gocache
.gomodcache
prod.pprof
packaging/docker/amoy-sidecar-bench.sh:13
- These defaults are developer-machine specific (
/Users/djones/...) and will break for other contributors/CI. Prefer repo-relative defaults so the script runs from any checkout without extra env vars.
packaging/docker/amoy-sidecar-pair.sh:7 - These defaults hardcode a local filesystem path (
/Users/djones/...) which makes the script non-portable. Consider defaulting to a path relative to the current working directory (or require explicit env vars).
scripts/test_heimdall_quic_integration.sh:11 - The script defaults to macOS-specific
/private/tmppaths, which will fail on many Linux environments. Using${TMPDIR:-/tmp}keeps it portable while still allowing overrides.
scripts/test_heimdall_quic_integration.sh:18 mktemp -d /private/tmp/...is also macOS-specific. If you definetmp_dir=${TMPDIR:-/tmp}earlier, reuse it here so the home dir can be created portably.
consensus/bor/heimdall/client.go:17- The import block needs to include
syncifHeimdallClient.Closeis made idempotent (e.g., viasync.Once) to avoid panics on double-close in retry/failover paths.
This issue also appears in the following locations of the same file:
- line 65
- line 540
"path"
"reflect"
"sort"
"time"
consensus/bor/heimdall/client.go:69
HeimdallClient.Closecurrently closescloseChunconditionally; callingClosetwice will panic. Adding async.Once(or equivalent) makes shutdown safe when multiple components attempt cleanup.
type HeimdallClient struct {
urlString string
client http.Client
transportCloser io.Closer
closeCh chan struct{}
consensus/bor/heimdall/client.go:544
Closeis not idempotent:close(h.closeCh)will panic on a second call. Wrapping the body inh.closeOnce.Do(...)makes cleanup safe and ensures the QUIC transport is only closed once.
// Close sends a signal to stop the running process
func (h *HeimdallClient) Close() {
close(h.closeCh)
h.client.CloseIdleConnections()
if h.transportCloser != nil {
- Files reviewed: 65/67 changed files
- Comments generated: 0 new
- Review effort level: Lite
| func (rw *routedMsgReadWriter) WriteMsg(msg Msg) error { | ||
| if channel := rw.route(msg.Code); channel != "" { | ||
| if bulk, ok := rw.bulk(channel); ok { | ||
| if err := bulk.WriteMsg(msg); err == nil { | ||
| return nil | ||
| } else { | ||
| bulkSidecarWriteFallbackMeter.Mark(1) | ||
| bulkSidecarStats.markChannelWriteFallback(channel) | ||
| } | ||
| } | ||
| } | ||
| return rw.primary.WriteMsg(msg) |
There was a problem hiding this comment.
🔴 On bulk-lane write failure, routedMsgReadWriter.WriteMsg falls back to rw.primary.WriteMsg(msg) reusing the same msg.Payload reader, which the failed bulk write already partially drained via io.CopyN — the fallback then fails too (short read), so the message is silently lost and the write error propagates to Peer.run(), tearing down the whole peer connection instead of degrading gracefully to RLPx as intended. Fix: buffer/clone the payload bytes before attempting the bulk write (or seek/reset it) so the RLPx fallback always sees the full, unconsumed payload.
Extended reasoning...
bulkStreamMsgRW.WriteMsg (p2p/bulk_sidecar.go:719) calls io.CopyN(rw.stream, msg.Payload, msg.Size); on a mid-copy network error/write-deadline expiry it returns after having already read n<Size bytes from msg.Payload, leaving the reader partially consumed. routedMsgReadWriter.WriteMsg (routed_message.go:143-155) sees the non-nil error and calls rw.primary.WriteMsg(msg) with the same msg, whose Payload now yields only the remaining bytes. rlpxTransport.WriteMsg (transport.go:94-96) does io.CopyN(&t.wbuf, msg.Payload, msg.Size) expecting Size bytes but gets fewer, returning io.ErrUnexpectedEOF before writing anything to the wire. That error is returned up through protoRW.WriteMsg (peer.go:562) which sends it on rw.werr, and Peer.run() initiates shutdown of the whole connection on any non-nil werr — so a single transient bulk-lane hiccup drops the peer entirely, worse than the base branch (no bulk lane, no such failure mode) and contrary to the PR's claimed graceful RLPx fallback.
Verification: normal. The bulk-lane fallback reuses a partially-drained payload reader. routed_message.go:143-155 WriteMsg: after bulk.WriteMsg(msg) returns non-nil it marks the fallback meter and returns rw.primary.WriteMsg(msg) with the SAME msg. bulk_sidecar.go:719 io.CopyN(rw.stream, msg.Payload, int64(msg.Size)) — on a mid-copy failure (write-deadline set at line 710, or QUIC error) it returns…
| heavyResponseServeSlots <- struct{}{} | ||
| defer func() { <-heavyResponseServeSlots }() |
There was a problem hiding this comment.
🔴 New global semaphore heavyResponseServeSlots (cap = min(GOMAXPROCS,4)) is shared across ALL peers for GetBlockBodies/GetReceipts68/69, and is held not just during the DB read but across the blocking network write (p2p.Send -> RLPx write with a 20s deadline in p2p/transport.go). On base, each peer's body/receipt requests were served independently; now a handful of slow-reading or malicious peers can occupy every slot for up to 20s each, stalling body/receipt service for every other honest syncing peer node-wide. Fix: scope the limiter per-peer (or make it non-blocking with a fallback/timeout) so one slow peer cannot starve service to unrelated peers, and release the slot before/without covering the network write. [also at: eth/protocols/eth/handler.go:68 - The new node-wide semaphore heavyResponseServeSlots (min(GOMAXPROCS,4) slots) is acquired synchronously inside each…]
Extended reasoning...
handleGetBlockBodies/handleGetReceipts68/69 (eth/protocols/eth/handlers.go:258-259,312-313,325-326) acquire the package-level heavyResponseServeSlots channel (eth/protocols/eth/handler.go:57-68, capacity <=4) before calling ServiceGetBlockBodiesQuery/ServiceGetReceiptsQuery and peer.ReplyBlockBodiesRLP/ReplyReceiptsRLP, releasing only via defer after the reply's p2p.Send returns. p2p.Send ultimately calls the RLPx frame writer which sets a 20s write deadline (p2p/transport.go:101, frameWriteTimeout=20s in p2p/server.go:60), so a peer that reads slowly (TCP backpressure) keeps the write - and thus the slot - held for up to 20s. Each connected peer's protocol messages are dispatched sequentially in that peer's own goroutine via the handler table (handler.go:197/212), so 4 such peers (well within maxPeers, no QUIC/bulk-sidecar needed) can exhaust all global slots simultaneously and repeatedly, making every other peer's GetBlockBodies/GetReceipts request queue behind the global channel send at…
Verification: normal — This change introduces a shared, bounded, blind-blocking semaphore held across a blocking network write, creating cross-peer head-of-line blocking / DoS amplification that the base branch does not have. The semaphore is package-global and small: var heavyResponseServeSlots = make(chan struct{}, heavyResponseWorkerLimit()) where heavyResponseWorkerLimit() returns min(GOMAXPROCS,4)…
| func (s *bulkSession) acceptChannel(stream *quic.Stream) error { | ||
| var hello bulkChannelHello | ||
| if err := readBulkControl(stream, bulkChannelControlMaxSize, &hello); err != nil { | ||
| return err | ||
| } | ||
| if hello.Version != bulkSidecarVersion { | ||
| return fmt.Errorf("unsupported bulk channel version %d", hello.Version) | ||
| } | ||
| if hello.Channel == "" || len(hello.Channel) > 64 { | ||
| return errors.New("invalid bulk channel name") | ||
| } | ||
| s.storeChannel(hello.Channel, &bulkStreamMsgRW{ | ||
| stream: stream, | ||
| channel: hello.Channel, | ||
| log: log.New("peer", s.remoteID, "channel", hello.Channel), | ||
| }) | ||
| return nil | ||
| } | ||
|
|
||
| func (s *bulkSession) getChannel(channel string) (MsgReadWriter, bool) { | ||
| s.lock.Lock() | ||
| defer s.lock.Unlock() | ||
| rw, ok := s.channels[channel] | ||
| return rw, ok | ||
| } | ||
|
|
||
| func (s *bulkSession) waitChannel(ctx context.Context, channel string) (MsgReadWriter, error) { | ||
| if rw, ok := s.getChannel(channel); ok { | ||
| return rw, nil | ||
| } | ||
| waiter := make(chan bulkChannelResult, 1) | ||
| s.lock.Lock() | ||
| if rw, ok := s.channels[channel]; ok { | ||
| s.lock.Unlock() | ||
| return rw, nil | ||
| } | ||
| s.waiters[channel] = append(s.waiters[channel], waiter) | ||
| s.lock.Unlock() | ||
|
|
||
| select { | ||
| case result := <-waiter: | ||
| return result.rw, result.err | ||
| case <-ctx.Done(): | ||
| return nil, errBulkChannelTimeout | ||
| } | ||
| } | ||
|
|
||
| func (s *bulkSession) storeChannel(channel string, rw MsgReadWriter) { | ||
| s.lock.Lock() | ||
| _, exists := s.channels[channel] | ||
| s.channels[channel] = rw | ||
| waiters := s.waiters[channel] | ||
| delete(s.waiters, channel) | ||
| s.lock.Unlock() | ||
|
|
||
| if exists { | ||
| bulkSidecarChannelReplaceMeter.Mark(1) | ||
| bulkSidecarStats.markChannelReplaced(channel) | ||
| } else { | ||
| bulkSidecarChannelOpenMeter.Mark(1) | ||
| bulkSidecarStats.markChannelOpened(channel) | ||
| } | ||
| s.sidecar.log.Debug("Bulk sidecar channel opened", "peer", s.remoteID, "channel", channel) | ||
|
|
||
| for _, waiter := range waiters { | ||
| waiter <- bulkChannelResult{rw: rw} | ||
| close(waiter) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 acceptChannel/storeChannel never close the previous QUIC stream when a peer re-opens a bulk channel with a name already registered on the session — the old *quic.Stream is just dropped from the map, leaking its stream credit. A malicious/misbehaving peer can repeat this to exhaust the connection's MaxIncomingStreams=32 budget, permanently jamming AcceptStream in runConn so no new bulk channels can ever be opened for that peer session (only fixed by a full peer disconnect). Fix: in storeChannel, when replacing an existing channel entry, explicitly cancel/close the superseded stream (e.g. CancelRead/CancelWrite or Close on the old bulkStreamMsgRW's stream) before overwriting, and/or reject duplicate channel-hellos for a name that is already active on the session.
Extended reasoning...
acceptChannel (bulk_sidecar.go:606) is invoked for every incoming QUIC stream that carries a bulkChannelHello, with no check that hello.Channel isn't already present in s.channels. storeChannel (line 653) then does s.channels[channel] = rw, discarding the old MsgReadWriter/stream reference without calling anything that closes or resets the underlying *quic.Stream. Since quic-go only replenishes MAX_STREAMS credit once a stream is closed/reset, repeatedly opening new streams with a reused channel name (a protocol violation any authenticated peer that completed the bulk-auth handshake can perform) consumes the 32-stream-per-connection budget (quic.Config.MaxIncomingStreams=32 in newBulkSidecar) without ever freeing it. Once exhausted, conn.AcceptStream in runConn (line 441) blocks indefinitely and legitimate channel opens time out via bulkChannelOpenTimeout, permanently degrading that peer's bulk sidecar to RLPx fallback until DropPeer tears down the whole session — a resource-exhaustion DoS that didn't exist before this feature.
Verification: nit. The code defect is real: storeChannel (p2p/bulk_sidecar.go:653-659) does s.channels[channel] = rw, and even measures the replacement (exists branch, markChannelReplaced at 661-663), yet never closes/resets the previously stored stream. bulkStreamMsgRW (declared line 137; only ReadMsg/WriteMsg at 676/703) has no Close method, and the grep for close calls shows every close is… | Severity:…
| tlsConf := &tls.Config{ | ||
| InsecureSkipVerify: false, | ||
| NextProtos: []string{bulkSidecarNextProto}, | ||
| MinVersion: tls.VersionTLS13, | ||
| } | ||
| conn, err := quic.DialAddr(dialCtx, endpoint.String(), tlsConf, b.config) |
There was a problem hiding this comment.
🔴 dialConn's TLS config sets InsecureSkipVerify: false but supplies no RootCAs/VerifyPeerCertificate, while the peer's cert is a fresh self-signed cert (generateBulkSidecarCertificate) never rooted in any trust store. Every outbound QUIC dial (quic.DialAddr) will fail x509 chain verification, breaking the sidecar for the lower-ID peer in every pair. Fix: add a VerifyPeerCertificate/VerifyConnection callback that checks the cert against the peer's known enode pubkey (or otherwise skip default chain verification), since peer identity is already authenticated via the challenge-response handshake, not the TLS chain.
Extended reasoning...
In bulkSession.ensureConn, whichever side has the lexicographically smaller localID dials out via sidecar.dialConn (bulk_sidecar.go:347-370), which builds tls.Config{InsecureSkipVerify:false, NextProtos:..., MinVersion: TLS1.3} with no RootCAs and no VerifyPeerCertificate/VerifyConnection override, then calls quic.DialAddr. Go's crypto/tls performs default x509 verification against the system root pool when RootCAs is nil; the remote's cert is a self-signed cert freshly minted per-process by generateBulkSidecarCertificate (line 813), never installed in any trust store. The handshake therefore fails with an unknown-authority error on every real dial, so openChannel/ensureConn always returns an error for the dialing side, and OpenChannel (used by the wit/snap bulk paths) never succeeds — the entire opt-in QUIC sidecar silently falls back to RLPx (or errors) despite the PR's claim that lanes establish successfully. The app-layer bulkAuthHello/Challenge/Response exchange (acceptAuth/initiateAuth) already authenticates peers by devp2p key signature, so the TLS chain check is redundant but, as…
Verification: normal. dialConn (p2p/bulk_sidecar.go:356-361) uses tls.Config{InsecureSkipVerify:false, NextProtos, MinVersion:TLS1.3} with no RootCAs and no VerifyPeerCertificate/VerifyConnection, then quic.DialAddr. The peer's server cert is self-signed and SAN-less (generateBulkSidecarCertificate, line 813-834: x509.CreateCertificate(..., template, template, ...) at line 830, template has no… | normal.…
| } | ||
| p.pendingLock.Unlock() | ||
|
|
||
| requestTracker.Fulfil(p.id, p.version, code, id) |
There was a problem hiding this comment.
🟡 (optional) dispatchResponse unconditionally calls requestTracker.Fulfil even when the response isn't tracked in peer.pending (the normal, non-WithSink request path used by all current snap sync traffic), then handler.go calls requestTracker.Fulfil again for the same id since dispatchResponse returned handled=false. The second call always misses the already-deleted tracker entry and marks the 'stale response' meter, so every legitimate AccountRange/StorageRanges/ByteCodes/TrieNodes reply now falsely increments the stale/collision metric used to spot malicious peers sending unsolicited data. …
Extended reasoning...
…Fix: only call requestTracker.Fulfil once per response — either skip the Fulfil call inside dispatchResponse when req==nil (peer.pending miss) and let the caller's existing Fulfil handle it, or drop handler.go's redundant Fulfil call.
In eth/protocols/snap/handler.go, e.g. AccountRangeMsg case: dispatchResponse(AccountRangeMsg, res) runs first (new code) and internally does requestTracker.Fulfil(p.id, p.version, code, id) unconditionally, which correctly matches and deletes the entry created by the normal RequestAccountRange -> requestTracker.Track path, then returns handled=false because peer.pending (the new Peer.pending map, only populated by RequestXWithSink helpers) has no entry for this id. Because handled=false and err=nil, the if handled || err != nil { return err } guard does not return, so the pre-existing requestTracker.Fulfil(peer.id, peer.version, AccountRangeMsg, res.ID) line runs a second time for the same id. tracker.Fulfil (p2p/tracker/tracker.go:168) looks up t.pending[id]; it's already deleted from the first call, so it falls into the !ok branch and…
Verification: nit. Real double-Fulfil on the normal snap path, but metrics-only impact (no functional break). In requests.go:88-103, dispatchResponse calls requestTracker.Fulfil(p.id, p.version, code, id) at line 103 UNCONDITIONALLY, before the if req == nil check at line 104. On the normal snap request path (RequestAccountRange, peer.go:150), the request is registered only via requestTracker.Track(...)…
Summary
This change adds an opt-in QUIC sidecar to Bor alongside the existing devp2p/RLPx connection, plus an independently opt-in HTTP/3 client path for Heimdall. The sidecar advertises its UDP endpoint in the node ENR and multiplexes
eth,snap, andwittraffic across 10 named QUIC lanes. Traffic remains on—or falls back to—RLPx when QUIC is unavailable.The implementation commit is 1a7ada30d4c740217c45919d737ba8e4565c0af8.
Executed tests
go test ./eth/protocols/eth -run '^TestPeerAttachBulkRWRoutesEthTraffic$' -count=1go test ./eth/fetcher -run '^TestTransactionFetcherRequestsTransactionsOverBulkLane$' -count=1go test ./eth -run '^TestTriggerTxFetchToPeers$' -count=1TCP → QUIC → QUIC → TCPfetcher comparison with 20 samples per profile. No meaningful local latency difference was observed.polycliat:kurtosis lint .,kurtosis lint -c ., and its local Starlark tests.Rollout notes
bulk-sidecar = false. Existing and mixed-version peers continue to use RLPx.bulk-sidecar = truebulk-port(default:30304)enode://records do not carry the QUIC endpointadmin_bulkSidecarStatusand thep2p/bulk/*metrics for active sessions, channels, packet drops, timeouts, read errors, and fallbacks.h3://URL and requires a matching Heimdall HTTP/3 endpoint.