p2p/sentry/libsentry, node/direct: collapse multiplexer fan-out boilerplate with generics#22166
Queued
yperbasis wants to merge 5 commits into
Queued
p2p/sentry/libsentry, node/direct: collapse multiplexer fan-out boilerplate with generics#22166yperbasis wants to merge 5 commits into
yperbasis wants to merge 5 commits into
Conversation
…n methods Characterization test ahead of a multiplexer fan-out refactor: AddPeer, RemovePeer, AddTrustedPeer and RemoveTrustedPeer must report success when any underlying client succeeds, and propagate the first client error. Only AddPeer had (indirect) coverage before.
Move the generic SentryStreamS/SentryStreamC/StreamReply adapters from sentrymultiplexer.go into their own libsentry/stream.go, and use them from SentryClientDirect.Messages/PeerEvents instead of the hand-monomorphized SentryMessagesStreamS/C and SentryPeersStreamS/C copies (identical Send with EvictOldestIfHalfFull, Err, Recv, RecvMsg with proto.Merge), which are deleted. The exported copies were referenced only by their own tests. The two node/direct eviction tests pinned one monomorphic copy each; with a single generic implementation left, they collapse into one test moved to libsentry/stream_test.go, keeping the stronger drop-oldest assertions.
…enerics Every unary sentryMultiplexer method open-coded the same errgroup scaffold: spawn a goroutine per client, optionally skip clients below a protocol threshold, collect replies under a mutex, wait. Replace ~16 copies with - fanOut: concurrent per-client call with protocol gating (-1 admits un-handshaken clients, 0 reproduces the SetStatus negotiated-only filter) and mutex-guarded collect that can return an error to cancel the group; - fanOutSuccess: reply.GetSuccess() OR-reduction for the four peer admin methods; - streamFanIn: the Messages/PeerEvents stream fan-in, which was duplicated verbatim modulo the message type. Deliberate variants are preserved: PeerById still early-stops via the errFound sentinel, HandShake still skips already-handshaken clients while folding their cached protocol into the max and stores each fresh reply on the client, and SendMessageByMinBlock stays intentionally serial. Pure refactor pinned by the existing multiplexer tests plus the any-client-success characterization test added beforehand.
yperbasis
requested review from
AskAlexSharov,
anacrolix and
taratorio
as code owners
This was referenced Jul 2, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the sentry multiplexer and direct sentry client to remove duplicated fan-out / stream-adapter boilerplate by introducing reusable generic helpers in libsentry, while adding a characterization test to pin the “any client success” semantics of peer-admin calls.
Changes:
- Introduce generic helpers in
libsentryfor concurrent fan-out (fanOut,fanOutSuccess) and stream fan-in (streamFanIn). - Move generic stream adapter types (
StreamReply,SentryStreamS,SentryStreamC) into a newlibsentry/stream.goand updatenode/directto use them. - Add tests validating peer-admin “any success” semantics and update stream eviction test to target the new generic stream wrapper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| p2p/sentry/sentrymultiplexer_test.go | Adds characterization test coverage for peer-admin any-success behavior across multiple clients. |
| p2p/sentry/libsentry/stream.go | Introduces generic stream wrapper types used by multiplexer and direct client. |
| p2p/sentry/libsentry/stream_test.go | Updates eviction test to use the new generic SentryStreamS. |
| p2p/sentry/libsentry/sentrymultiplexer.go | Collapses repeated errgroup fan-out and stream fan-in patterns into generic helpers and rewires methods to use them. |
| node/direct/sentry_client.go | Removes local hand-monomorphized stream adapters and reuses libsentry generic stream wrappers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nOut gate Review follow-ups for the generics refactor: - Name the -1 "no protocol" sentinel (noProtocol) and use it for the client initializer, the admit-everyone minProtocol call sites, and MinProtocol's unknown-message return. - Skip fanOut's gating read when minProtocol is noProtocol: the gate cannot exclude anyone (client.protocol >= noProtocol always), the RLock was new overhead the pre-refactor ungated loops never had, and HandShake no longer reads client.protocol twice per client. - Idiomatic errors.Is argument order in PeerById. - Drop inferable streamFanIn type arguments (infertypeargs).
Sahil-4555
pushed a commit
to Sahil-4555/erigon
that referenced
this pull request
Jul 3, 2026
…ontech#22168) The five `GetOrCreate{Histogram,Counter,Gauge,SummaryExt,GaugeVec}` methods repeated the same ~32-line double-checked-locking block; each copy differed only in the constructor and type assertion. They collapse onto `getOrCreate[T prometheus.Metric]` / `getOrCreateVec[T prometheus.Collector]`, and each method is now a 3-liner. The GaugeVec copy is where clone mutation had gone wrong: its type guard compared `reflect.TypeFor[*prometheus.GaugeVec]()` to itself — always false, i.e. dead (and unreachable while `namedMetricVec.metric` was concretely typed `*prometheus.GaugeVec`). The generic widens the field to `prometheus.Collector` and performs a real assertion with the same error shape as the scalar family (`TestSetGetOrCreateGaugeVecWrongType`). `registerMetricVec` is removed: its re-check-and-insert was a duplicate of the inline double-checked insert. First commit adds characterization tests (+127 lines) pinning existing GetOrCreate* behavior — same-instance returns, wrong-type errors, describe-once semantics — green against the old code before the refactor. **Update (third commit):** `getOrCreate`/`getOrCreateVec` were themselves clones differing only in which registry (`m`/`a` vs `vecs`/`av`) they touched. They now delegate to a single `getOrCreateIn` over a shared `namedEntry[M]` entry type (`namedMetric`/`namedMetricVec` become type aliases), with the wrappers widening `T` through an adapter closure so the store stays compile-time safe. The double-checked locking becomes one critical section: the fast path took the same mutex anyway, and the create funcs are pure `parseMetric` + constructor calls, so creating under the lock eliminates the create-twice-drop-loser race. The unlock is deferred because prometheus constructors can panic on inputs reachable through the public API (a `quantile`/`le` const label, a negative summary window); a non-deferred unlock would strand the set's mutex. Also drops the dead scalar `namedMetric.isAux` — never assigned, so the `ListMetricNames` filter on it was unreachable. Net −169 lines in set.go. Verified: package tests incl. `-race`, `make lint` clean, full `go build ./...`. Part of a dedup series; siblings erigontech#22165, erigontech#22166, erigontech#22167.
AskAlexSharov
approved these changes
Jul 7, 2026
Any commits made after this event will not be merged.
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
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.
The sentry multiplexer repeated the same errgroup fan-out/collect scaffold in 16 methods (~350 duplicated lines), and
node/directcarried two hand-monomorphized copies of libsentry's own generic stream adapters. This collapses both:fanOut[R]— protocol-gated concurrent call with a mutex-guarded collect that can cancel the group;fanOutSuccess[R]— the GetSuccess OR-reduction used by the four peer-admin methods;streamFanIn[T,S]— the Messages/PeerEvents stream fan-in.StreamReply/SentryStreamS[T]/SentryStreamC[T]moved tolibsentry/stream.go; theSentryMessagesStream*/SentryPeersStream*copies innode/direct/sentry_client.goare deleted (no external references).Preserved deliberately:
PeerById's errFound early-stop,SetStatus/peersByClientprotocol gating,HandShake's per-client protocol caching and max-reduction, andSendMessageByMinBlockstays serial (untouched, along with the other send-path methods).Net −218 lines including +99 of new tests. Verified:
go test ./p2p/sentry/... ./node/direct/...plus-raceon the sentry packages, scopedgolangci-lintclean (×2),make erigon.Part of a dedup series; sibling defect fixes in #22165.