You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PostEthV1BeaconPoolSyncCommittees published sync-committee messages to gossip synchronously, on the HTTP request's own context, before writing the HTTP response, and swallowed any publish error at Debug. The handler blocks for the full duration of gossip validation/handoff, coupling HTTP response latency to a single-threaded event loop shared with the much higher-volume attestation topics, and whatever error Publish returns never surfaced to the caller, an HTTP status, or any metric.
Cross-referencing a Lighthouse VC log against the beacon API's rewards/sync_committee endpoint confirmed real, on-chain misses on a mainnet node — see #24258 for the evidence.
Correction (2026-09-24): an earlier version of this description and #24258 claimed the request context being cancelled on handler return could abort an in-flight publish. That's wrong — the handler cannot return (and so net/http cannot cancel r.Context()) while it's still blocked on the synchronous Publish call, and go-libp2p-pubsub v0.11.0's Topic.Publish only consults its ctx parameter inside the optional WithReadiness path, which Erigon's no-options call never takes. Caught by an independent third-party review of this PR; full detail in the correction section of #24258. The on-chain misses are real; this PR does not establish their specific original cause, only that decoupling the publish call removes the HTTP-latency coupling and makes every failure path observable and, where the failure is known before the response is written, reported to the caller. A later review raised a further, specific, unresolved doubt about the original cross-reference's slot alignment — see the follow-up comment on #24258. Also unresolved either way in this session.
Prior art
Both geth (eth/handler.go's txBroadcastLoop) and reth (TransactionsManager) decouple RPC-accept from network-broadcast the same way: the RPC handler only inserts into a local pool and returns; a single long-lived background worker, subscribed to a pool/feed channel at node startup, does the actual broadcast, independent of any individual request's context.
Fix
GossipManager already runs lifetime-scoped background goroutines off its own internally-owned context - this reuses that instead of introducing a new context-derivation pattern:
A bounded publishQueue channel plus one dedicated worker goroutine, started alongside the existing ones. The queue is sized to beaconConfig.SyncCommitteeSize (with a 64 floor) so it can absorb a full sync-committee burst without dropping.
PublishBackground(name string, data []byte, expiry time.Time, logCtx ...any) error on the Gossip interface: enqueues non-blockingly and returns a stable sentinel error (never nil-but-silently-dropped) when the message was never admitted - full queue, shutdown in progress, an unresolvable fork digest, or already past expiry. The fork digest is resolved and captured at enqueue time, not re-resolved when the worker drains the job, so a message accepted just before a fork activates still publishes to the topic it was actually validated against.
expiry (slot end plus the protocol's maximum gossip clock disparity, computed with exact time arithmetic) is checked both at admission and again immediately before the actual publish - a message that goes stale while queued behind other work is dropped instead of spending a compress/peer-lookup/publish cycle on it, without blocking fresh work behind it. This is a staleness check, not a validity/authentication one - see Known limitations below.
PostEthV1BeaconPoolSyncCommittees calls PublishBackground instead of the synchronous Publish(r.Context(), ...). A non-nil return - a known failure, not an unknowable later network one - is now surfaced as HTTP 500 for an otherwise-valid batch, instead of a silent 200. Validation failures (the existing indexed 400 behavior) take precedence when a batch has both, without discarding the admission failure: PublishBackground still logs and counts it either way.
Every drop path is observable and correctly categorized: caplin_gossip_publish_queue_rejected_total{topic,reason} for admission-time rejections (never queued), separate from caplin_gossip_publish_accepted_total{topic} and caplin_gossip_publish_outcome_total{topic,outcome} (handoff_ok/publish_error/panic/expired) for work that was admitted.
PublishBackground never blocks the caller - not on queue capacity, not on shutdown: it checks the manager's own lifetime context directly (a plain, already non-blocking read) and the worker simply returns on ctx.Done() without an explicit drain step. A message admitted right at shutdown may end up queued with nothing left to process it, which has no functional effect - it was never going to publish either way, the same as one correctly rejected at admission. An earlier version of this PR added an admission gate (atomics, a drain step, dedicated test-only hooks) purely to keep the accepted/outcome counters exactly equal at process exit; per review from domiwei and AskAlexSharov, that complexity wasn't worth what it bought, so it was removed - the counters still hold the invariant during normal operation, just not necessarily at the exact moment of shutdown.
Ordering across queued messages doesn't matter here - gossip has no ordering contract between different validators/subnets.
Scoped to sync-committee messages only, where the on-chain evidence is. The same synchronous-publish-with-swallowed-error pattern exists at 13 other call sites in pool.go, block_production.go, and epbs.go - left as a separate follow-up to keep this PR focused. Full histograms (queue-wait/publish-call duration) and an in-flight gauge were considered and deliberately left out of this PR's scope - the accepted/outcome counters above already fix the modeling gap (a rejected admission being counted the same as an accepted-then-dropped job); histograms are a real but separate enhancement.
handoff_ok in the new outcome metric means the local pubsub library accepted the publish call - it is not evidence of remote peer receipt or sync-aggregate inclusion. Neither this metric nor the two-host delivery test below establishes that a duty reliably arrives before its useful deadline; treat the two-host test as a transport smoke test, not a timing/reliability guarantee.
The captured fork digest is read from the wall clock at enqueue time, not derived from the validated message's own epoch; this predates the queue and is unchanged by it.
cl/phase1/network/gossip: expired queued jobs can occupy the publish queue and block fresh admission #24319: if the single publish worker gets stuck on one call (e.g. the shared pubsub event loop is congested enough to back up its internal send buffer), the bounded queue can fill with jobs that expire before the worker reaches them, and admission then rejects fresh, still-valid messages with no path to reclaim that wasted capacity. A per-job context deadline doesn't fix this - go-libp2p-pubsub's local-publish path doesn't consult the caller's context past the WithReadiness option we don't use. The real fix (evicting expired entries from the queue on full-queue admission) is a genuine change to the queue's concurrency model, tracked separately.
Testing
cl/phase1/network/gossip/gossip_manager_test.go and cl/beacon/handler/pool_test.go, each verified by mutation (the corresponding regression fails reliably when the fix is reverted):
PublishBackground never blocks its caller on queue capacity or shutdown; drops rather than blocks once the queue is full, sized to hold a full SyncCommitteeSize burst.
The fork digest used to publish is the one captured at enqueue time, even if it changes before the worker drains the job.
Expiry: the exact inclusive boundary (just before/at/just after the deadline), a job going stale while queued behind other work without blocking fresh work behind it.
A panic in one queued publish doesn't take down the worker, and its recovery log includes the same validator/subnet/slot context the ordinary failure path already logs.
A queued message reaches the real gossip Publish path end-to-end, including a two-host test proving some bytes cross a real connection (see Known limitations - this is a transport smoke test, not a full protocol/timing test).
A message is dropped observably once the manager has shut down, whether triggered via Close or the parent context passed into NewGossipManager being cancelled directly (the actual production path); PublishBackground racing concurrently with either never panics or deadlocks, and the worker always terminates.
The accepted/outcome counters satisfy the invariant that at quiescence during normal operation, a topic's accepted count equals the sum of its terminal-outcome counts - compared as deltas against a baseline, since these are process-global collectors never reset between test runs (an earlier version of this test compared raw values and flaked under -count=2; fixed and verified up to -count=5).
PostEthV1BeaconPoolSyncCommittees calls PublishBackground, never the blocking Publish; a full queue returns HTTP 500 for an otherwise-valid batch; a validation failure takes precedence over an admission failure when a batch has both.
go build ./..., go test ./cl/beacon/handler/... ./cl/phase1/network/gossip/... -race, and make lint all pass.
The queued job stores only the topic name and payload, so Publish recomputes CurrentForkDigest when the worker drains it. If the queue spans an epoch fork, a message accepted for a pre-fork slot can be sent to the new fork's topic (or fail topic lookup) instead of the digest active when it was received. Capture the digest/topic at enqueue time, or explicitly discard jobs that became stale, rather than resolving it only in the worker.
Addressed the "Capture fork digest when queuing messages" finding from the latest Copilot review in 9190417: PublishBackground now resolves the fork digest at enqueue time and carries it with the job, instead of Publish re-resolving whatever digest happens to be current when the worker drains the queue. Added a regression test (TestPublishBackground_CapturesForkDigestAtEnqueueTime) that flips the mocked fork digest between enqueue and drain and asserts the message still lands on the topic active when it was accepted - confirmed it fails without the fix (message gets misrouted to a topic nobody subscribed to) and passes with it.
Addressed the shutdown-race finding from the latest Copilot review in 185e320: Close cancels the worker's context and it stops draining the queue, but PublishBackground had no awareness of that - a message enqueued afterward (while queue capacity remained) would sit in the channel with nothing left to consume it, silently, with no log or metric (unlike the queue-full case). PublishBackground now checks the manager's own lifetime context first and drops observably (Debug log + caplin_gossip_publish_queue_rejected_total{reason="shutdown"}) instead of leaving it stranded. New regression test (TestPublishBackground_DropsAfterClose) confirmed to fail without the fix and pass with it.
Note this closes the deterministic case (anything called after Close() returns is always caught, since context cancellation is synchronous within cancel()). A PublishBackground call already in flight at the exact instant Close() runs has an unavoidable, much narrower interleaving window in common with any finite-precision shutdown handshake - not chasing that further given the existing codebase doesn't hold other concurrent-with-shutdown call sites to a stricter bar either, and the worst case there is identical to the queue-full/best-effort drop this PR already accepts.
This path drops the message when fork-digest resolution fails, but it never increments publishQueueDroppedCounter. As a result, caplin_gossip_publish_queue_rejected_total under-reports one of the rejection paths described by this change, making these drops invisible to the metric. Increment the counter with a dedicated reason before logging.
After the parent context is cancelled, this gate remains open until the worker is scheduled and executes drainPublishQueueOnShutdown. A request arriving in that window can resolve the digest, enqueue work that is guaranteed to be drained as shutdown, and still receive nil/HTTP 200. Check lifetimeCtx.Err() in this lock-free admission check as well; calls that start after cancellation should be rejected, while the existing in-flight counter still preserves the intended race for producers that already passed the check.
…on gate
shutdownClosed is only set once the worker is scheduled and observes
ctx.Done() inside drainPublishQueueOnShutdown, which can lag real
cancellation by however long the worker is busy with other work. A
call arriving in that window would resolve the digest, enqueue work
only ever destined to be drained as outcome=shutdown, and still report
success - misleading a caller that treats a nil return as an HTTP 200.
lifetimeCtx.Err() is already a non-blocking check and reflects
cancellation immediately, closing the gap without affecting the
in-flight producer accounting the shutdown drain relies on.
Re "Reject requests after parent cancellation before shutdown drain" from the latest Copilot review (no anchored thread, so replying here): confirmed and fixed in efcab17.
Real gap: shutdownClosed is only set once the worker is scheduled and actually observes ctx.Done() inside drainPublishQueueOnShutdown, which can lag real cancellation by however long the worker is still busy with other work. A PublishBackground call arriving in that window resolved the digest, enqueued the message, and returned nil - a caller treating that as success (and pool.go does, as an HTTP 200) would be misled, since that job could only ever be drained with outcome=shutdown, never actually published.
Fixed by also checking lifetimeCtx.Err() directly in the admission gate alongside shutdownClosed - it's already a plain, non-blocking check (same as the rest of the gate) and reflects cancellation the instant it happens, independent of worker scheduling. Doesn't affect the in-flight producer accounting the shutdown drain relies on: a producer that already passed the check before cancellation still gets accounted for and drained correctly (unaffected - it's the same admission call, this only adds an earlier-exit condition).
New test TestPublishBackground_RejectsAdmissionAfterCancellationBeforeDrainObserved: occupies the worker so it can't reach ctx.Done() yet, cancels the parent context, then asserts a fresh call is rejected with ErrGossipManagerShutdown rather than succeeding. Confirmed it fails without the fix (returns nil) and passes with it. Full cl/beacon/handler + cl/phase1/network/gossip suites (20x fresh runs of the gossip package), -race, and make lint all green.
…hBackground
The queued job retained the caller's data slice across the async
boundary to the worker, which reads it later when it compresses and
publishes. A caller reusing or mutating its buffer after
PublishBackground returns - safe with the synchronous Publish, which
reads data before returning - could change the bytes actually gossiped,
or race with the worker reading them concurrently.
Clone data before enqueueing so the worker owns an independent copy;
documented the ownership contract on both the interface and the
implementation.
The comment claimed the queue absorbs a full sync-committee burst
"while the worker is still draining the previous slot's burst" - not
true for a fixed-capacity channel: it only holds a full burst when
starting empty. Leftover jobs from a previous burst plus a new one can
exceed capacity and get part of the new burst rejected.
The maxFutureSlotLookahead guard and its unit tests cover the MaxUint64 and timestamp-aliasing cases.
Verified as correct:
Validation (ProcessMessage with ImmediateVerification) stays synchronous. The indexed 400 semantics are preserved, and the message reaches the local sync contribution pool before publish, so local aggregation is not affected.
A single bounded worker publishes on the manager's lifetime context, not the request context. No goroutine is spawned per request.
go test -race -count=5 ./cl/phase1/network/gossip/ and the sync-committee handler tests pass locally.
Needs a change before approval:
M1:publishToDigest now calls ListPeers for every sync-committee publish. See the inline comment.
Design question:
M2: The shutdown admission gate (admissionsInFlight/shutdownClosed), the Gosched spin, drainPublishQueueOnShutdown, rejectShutdown and the four *HookForTest fields add about 260 lines. They only keep the accepted == sum(outcomes) metric invariant exact at process exit. A job left in the queue at shutdown has no functional effect. A non-blocking select send at admission, plus the worker returning on ctx.Done(), would remove most of this code and several of its tests. Please consider simplifying.
Minor:
L1: A seen-cache hit in ProcessMessage returns nil, so duplicate submissions still call PublishBackground and use queue slots. Examples are a VC resending a batch after a 500, or several VCs submitting the same messages.
L3:publish queue full is logged at Warn once per message, which can be hundreds of lines per slot under congestion. Please log it once per request. Please also reconsider moving publish_error from Debug to Warn.
L4: Several comments are long and describe call sites or scenarios, which the repo comment rules discourage. Two examples: the block before PublishBackground in pool.go, and "Close, which production code never calls" in drainPublishQueueOnShutdown.
Out of scope, possible follow-up issue:
IsSlotCurrentSlotWithMaximumClockDisparity / GetSlotTime use unchecked uint64 arithmetic. On mainnet, slot 4611686018442700405 maps to the current slot start and passes the current-slot check. The sync-committee signature does not cover the slot, so a forged slot can enter the pool, and gossip can forward it. This PR's guard blocks it only on the publish side.
The reason will be displayed to describe this comment to others. Learn more.
Adding gossip.IsTopicSyncCommittee(name) here means every sync-committee publish now calls p2p.Pubsub().ListPeers(topic). In go-libp2p-pubsub v0.11.0 that is a synchronous round-trip on the unbuffered getPeers channel through the pubsub event loop. The single worker now makes two event-loop round-trips per message, in the same congested path this PR tries to relieve, which makes a full queue (and a 500) more likely during a 512-message burst. When a subnet has no peers, it also logs one Warn per message. Please remove the sync-committee branch or move it off the hot path. The change to the attestation log text is also out of scope for this PR.
The reason will be displayed to describe this comment to others. Learn more.
Confirmed - and thanks for tracing through the pubsub internals, that's exactly right: ListPeers sends on an unbuffered channel into the same PubSub.processLoop that Publish's eventual sendMsg goes through, so this was a second synchronous round-trip into the exact congested loop this PR is decoupling from. Reverted the sync-committee branch entirely in a4b11af, back to the original attestation-only scope (including the log text) - this diagnostic was never actually needed for the PR's purpose, it was scope creep on my part while touching this function.
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, fixed in a4b11af. Downgraded the per-call log to Debug at the source (publishQueueDroppedCounter{topic,reason="queue_full"} remains the aggregate signal) and added a single per-request Warn in the handler with a count. Also checked: publish_error (gossip_manager.go's runPublishJob) was already at Warn, not Debug - unaffected by this change either way. New test TestPoolSyncCommitteesLogsAdmissionFailuresOncePerRequest confirms exactly one summary log line for a batch with multiple admission failures, with the correct count.
The reason will be displayed to describe this comment to others. Learn more.
This spin-wait and the drain below only keep the metric invariant exact at process exit (see M2 in the summary). A simpler worker that returns on ctx.Done() would be enough.
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and done in 07b683e. You and AskAlexSharov independently landed on the same conclusion, which was the deciding factor - a job still queued at shutdown has no functional effect, so the admission gate (admissionsInFlight/shutdownClosed), the Gosched spin, drainPublishQueueOnShutdown, and rejectShutdown are gone, replaced with a plain lifetimeCtx.Err() check in PublishBackground and a worker that just returns on ctx.Done(). Also removed 3 of the 4 test-only hooks and the 5 tests that existed solely to verify the removed machinery, keeping a trimmed concurrency smoke test (PublishBackground racing Close must not panic/deadlock) and the existing admission-after-close test (unaffected, since it was never about the drain). Net -346 lines. Updated the PR description and publishOutcomeCounter's doc comment to match - the accepted==sum(outcomes) invariant now holds during normal operation, not necessarily at the exact moment of shutdown.
… aggregate admission-failure logging, trim comments
Adding IsTopicSyncCommittee to the peer-count diagnostic branch meant
every sync-committee publish now made a second synchronous round-trip
through the pubsub event loop (ListPeers blocks on an unbuffered
channel into the same loop Publish uses), doubling the single worker's
exposure to congestion in exactly the path this PR relieves. Reverted
to the original attestation-only scope; this diagnostic was never
needed for the PR's actual purpose.
The queue-full admission log was Warn per rejected message, which
under congestion (up to a full sync-committee burst) could be hundreds
of lines for one request. Downgraded to Debug at the source
(publishQueueDroppedCounter is the aggregate signal) and added a
single Warn per request in the handler with a failure count.
Also shortened two comments describing design/call-site detail the
repo's comment conventions discourage in source.
Re L1 (seen-cache hit lets duplicate submissions use queue slots): confirmed as described, and agreed it's minor. This touches the same seenSyncCommitteeMessages cache mechanism #24305 already tracks for a more thorough redesign (distinguishing "seen for dedup" from "these exact bytes were verified"). Adding it there rather than a standalone patch here, since a quick fix risks conflicting with whatever shape that redesign takes.
…ion gate
Per domiwei's and AskAlexSharov's independent reviews: the
admissionsInFlight/shutdownClosed gate, the Gosched spin-wait,
drainPublishQueueOnShutdown, and rejectShutdown existed only to keep
the accepted==sum(outcomes) metric invariant exact at process exit. A
job still queued when the manager shuts down has no functional
effect - it was never going to publish either way, exactly like one
correctly rejected at admission.
Replaced with a plain lifetimeCtx.Err() check in PublishBackground and
a worker that simply returns on ctx.Done() without draining. Removed
the three test-only hook fields (enqueueHookForTest,
shutdownObservedHookForTest, drainItemHookForTest) that existed solely
to make the removed machinery's races deterministic, along with the
five tests that existed solely to verify it. Kept a trimmed
concurrency smoke test (PublishBackground racing Close must not panic
or deadlock) and TestPublishBackground_DropsAfterClose (admission
after shutdown is still rejected immediately - unaffected, since it
was never about the drain).
Updated publishOutcomeCounter's doc comment: the invariant now holds
during normal operation, not necessarily at shutdown.
Re the out-of-scope finding (unchecked uint64 arithmetic in IsSlotCurrentSlotWithMaximumClockDisparity/GetSlotTime letting a forged slot alias to "current"): agreed this is real and correctly out of scope here - it's a gossip/pool-ingestion-path issue, independent of #24259's own publish-admission guard (which only bounds what reaches PublishBackground, not what ProcessMessage accepts in the first place). Filed #24360 to track it, with the repro slot value and scope note from your review.
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
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.
domiwei
left a comment
Fixes #24258.
Problem
PostEthV1BeaconPoolSyncCommitteespublished sync-committee messages to gossip synchronously, on the HTTP request's own context, before writing the HTTP response, and swallowed any publish error atDebug. The handler blocks for the full duration of gossip validation/handoff, coupling HTTP response latency to a single-threaded event loop shared with the much higher-volume attestation topics, and whatever errorPublishreturns never surfaced to the caller, an HTTP status, or any metric.Cross-referencing a Lighthouse VC log against the beacon API's
rewards/sync_committeeendpoint confirmed real, on-chain misses on a mainnet node — see #24258 for the evidence.Correction (2026-09-24): an earlier version of this description and #24258 claimed the request context being cancelled on handler return could abort an in-flight publish. That's wrong — the handler cannot return (and so
net/httpcannot cancelr.Context()) while it's still blocked on the synchronousPublishcall, and go-libp2p-pubsub v0.11.0'sTopic.Publishonly consults itsctxparameter inside the optionalWithReadinesspath, which Erigon's no-options call never takes. Caught by an independent third-party review of this PR; full detail in the correction section of #24258. The on-chain misses are real; this PR does not establish their specific original cause, only that decoupling the publish call removes the HTTP-latency coupling and makes every failure path observable and, where the failure is known before the response is written, reported to the caller. A later review raised a further, specific, unresolved doubt about the original cross-reference's slot alignment — see the follow-up comment on #24258. Also unresolved either way in this session.Prior art
Both geth (
eth/handler.go'stxBroadcastLoop) and reth (TransactionsManager) decouple RPC-accept from network-broadcast the same way: the RPC handler only inserts into a local pool and returns; a single long-lived background worker, subscribed to a pool/feed channel at node startup, does the actual broadcast, independent of any individual request's context.Fix
GossipManageralready runs lifetime-scoped background goroutines off its own internally-owned context - this reuses that instead of introducing a new context-derivation pattern:publishQueuechannel plus one dedicated worker goroutine, started alongside the existing ones. The queue is sized tobeaconConfig.SyncCommitteeSize(with a 64 floor) so it can absorb a full sync-committee burst without dropping.PublishBackground(name string, data []byte, expiry time.Time, logCtx ...any) erroron theGossipinterface: enqueues non-blockingly and returns a stable sentinel error (never nil-but-silently-dropped) when the message was never admitted - full queue, shutdown in progress, an unresolvable fork digest, or already pastexpiry. The fork digest is resolved and captured at enqueue time, not re-resolved when the worker drains the job, so a message accepted just before a fork activates still publishes to the topic it was actually validated against.expiry(slot end plus the protocol's maximum gossip clock disparity, computed with exact time arithmetic) is checked both at admission and again immediately before the actual publish - a message that goes stale while queued behind other work is dropped instead of spending a compress/peer-lookup/publish cycle on it, without blocking fresh work behind it. This is a staleness check, not a validity/authentication one - see Known limitations below.PostEthV1BeaconPoolSyncCommitteescallsPublishBackgroundinstead of the synchronousPublish(r.Context(), ...). A non-nil return - a known failure, not an unknowable later network one - is now surfaced as HTTP 500 for an otherwise-valid batch, instead of a silent 200. Validation failures (the existing indexed 400 behavior) take precedence when a batch has both, without discarding the admission failure:PublishBackgroundstill logs and counts it either way.caplin_gossip_publish_queue_rejected_total{topic,reason}for admission-time rejections (never queued), separate fromcaplin_gossip_publish_accepted_total{topic}andcaplin_gossip_publish_outcome_total{topic,outcome}(handoff_ok/publish_error/panic/expired) for work that was admitted.PublishBackgroundnever blocks the caller - not on queue capacity, not on shutdown: it checks the manager's own lifetime context directly (a plain, already non-blocking read) and the worker simply returns onctx.Done()without an explicit drain step. A message admitted right at shutdown may end up queued with nothing left to process it, which has no functional effect - it was never going to publish either way, the same as one correctly rejected at admission. An earlier version of this PR added an admission gate (atomics, a drain step, dedicated test-only hooks) purely to keep the accepted/outcome counters exactly equal at process exit; per review from domiwei and AskAlexSharov, that complexity wasn't worth what it bought, so it was removed - the counters still hold the invariant during normal operation, just not necessarily at the exact moment of shutdown.Ordering across queued messages doesn't matter here - gossip has no ordering contract between different validators/subnets.
Scoped to sync-committee messages only, where the on-chain evidence is. The same synchronous-publish-with-swallowed-error pattern exists at 13 other call sites in
pool.go,block_production.go, andepbs.go- left as a separate follow-up to keep this PR focused. Full histograms (queue-wait/publish-call duration) and an in-flight gauge were considered and deliberately left out of this PR's scope - the accepted/outcome counters above already fix the modeling gap (a rejected admission being counted the same as an accepted-then-dropped job); histograms are a real but separate enhancement.Known limitations (tracked, not fixed here)
ErrIgnoreexempted then published anyway - is now fixed here:PostEthV1BeaconPoolSyncCommitteesskipsPublishBackgroundentirely onErrIgnore, verified against the real service.)handoff_okin the new outcome metric means the local pubsub library accepted the publish call - it is not evidence of remote peer receipt or sync-aggregate inclusion. Neither this metric nor the two-host delivery test below establishes that a duty reliably arrives before its useful deadline; treat the two-host test as a transport smoke test, not a timing/reliability guarantee.WithReadinessoption we don't use. The real fix (evicting expired entries from the queue on full-queue admission) is a genuine change to the queue's concurrency model, tracked separately.Testing
cl/phase1/network/gossip/gossip_manager_test.goandcl/beacon/handler/pool_test.go, each verified by mutation (the corresponding regression fails reliably when the fix is reverted):PublishBackgroundnever blocks its caller on queue capacity or shutdown; drops rather than blocks once the queue is full, sized to hold a fullSyncCommitteeSizeburst.Publishpath end-to-end, including a two-host test proving some bytes cross a real connection (see Known limitations - this is a transport smoke test, not a full protocol/timing test).Closeor the parent context passed intoNewGossipManagerbeing cancelled directly (the actual production path);PublishBackgroundracing concurrently with either never panics or deadlocks, and the worker always terminates.-count=2; fixed and verified up to-count=5).PostEthV1BeaconPoolSyncCommitteescallsPublishBackground, never the blockingPublish; a full queue returns HTTP 500 for an otherwise-valid batch; a validation failure takes precedence over an admission failure when a batch has both.go build ./...,go test ./cl/beacon/handler/... ./cl/phase1/network/gossip/... -race, andmake lintall pass.