[performance] cherry-pick: p2p/sentry: don't duplicate SendMessageById across shared-store sentries (#21597) - #21949
Merged
Conversation
…ies (#21597) ## Summary Fixes the hive devp2p `BlobViolations` flake (`expected disconnect on blob violation, got msg code: 24`) seen on the CI Gate run for #21524 (parallel leg) and on a runner-experiment branch the day before (serial leg) — and the gossip-duplication regression behind it. ## Root cause Since #21335 all per-eth-version sentry `GrpcServer`s share one `p2p.Server` and one `PeerStore`. `SendMessageById` resolves the target peer in the shared store, so callers that fan a by-id send out across every sentry client — e.g. the txpool's new-peer pool sync (`PropagatePooledTxnsToPeersList`) — now deliver the same frame once per sentry: three identical `NewPooledTransactionHashes` messages per new peer on the default eth/69+70+71 build. Before #21335 each sentry had its own peer set, so the off-sentry sends were silent no-ops. `SendMessageToAll` and `SendMessageToRandomPeers` already guard against this with `protocolVersions.Contains(peerInfo.EthProtocol())`; `SendMessageById` was the only outbound path without the negotiated-version check. The BlobViolations failure sequence, reproduced locally with wire-level logging (hive `--sim devp2p --sim.limit eth` against an instrumented image; failed on the second loop iteration with the exact CI error): 1. The test peers, announces two blob txs (one with a lying size/type), delivers them, and waits for a disconnect. 2. The violating `PooledTransactions` reply sits in the txpool fetcher's 250 ms inbound batch before the announcement check kicks the peer. 3. If the 5 s `syncToNewPeersEvery` tick lands in that window, the all-pool announcement (~70 KB — about 2000 hashes accumulated by earlier suite tests) is written to the test peer three times: ``` 08:16:32.606 inbound NPTH68 peer=de6008eb count=2 08:16:32.607 WRITE GET_POOLED_TXS_66 peer=de6008eb 08:16:32.607 sync-to-new-peers entries=2002 <- 5s tick fires 08:16:32.608 WRITE NPTH68 70084 bytes peer=de6008eb <- one write 08:16:32.608 WRITE NPTH68 70084 bytes peer=de6008eb <- per 08:16:32.608 WRITE NPTH68 70084 bytes peer=de6008eb <- sentry 08:16:32.855 VIOLATION -> PenalizePeer peer=de6008eb <- next 250ms batch flush ``` 4. The devp2p test deliberately tolerates one pre-disconnect hash announcement (go-ethereum commit 88c8459, Sept 2024, "sometimes we'll get a blob transaction hashes announcement before the disconnect"); the duplicated second copy arrives before the kick and fails the test. In the failed CI run the timing lines up exactly: the txpool started at 06:33:04.64 and the test failed at 06:33:14.644 — the tick+10s sync. With the duplication fixed, at most one announcement can precede the disconnect in that window, which the test tolerates by design. Beyond the test, every new peer was receiving the full pool sync in triplicate. ## Fix Apply the same negotiated-eth-version guard in `SendMessageById` that the other outbound paths use. Eth-protocol messages only; wit is unaffected (it is deduplicated to a single `GrpcServer` at shared-server construction). This also restores the routing contract documented at `FetchBlockAccessLists` ("sentryIndex MUST be the sentry where peerID is actually connected"): sends via a non-matching sentry are silent no-ops again, as they were with per-sentry peer sets. Not present on `release/3.4` (#21335 is main-only), so no backport is needed. ## Testing - New regression test `TestGrpcServer_SendMessageById_SharedStore_NoDuplicateWrites` (TDD): three `GrpcServer`s (eth/69/70/71) sharing a `PeerStore`, peer negotiated eth/70, by-id fan-out across all three must produce exactly one write. Red before the fix (`expected: 1, actual: 3`), green after. - `go test ./p2p/sentry/... ./txnprovider/txpool/...` clean. - Local hive validation in a loop: before the fix `not ok 18 BlobViolations` reproduced on iteration 2; after the fix 12/12 iterations green, with instrumented logs showing exactly one `NEW_POOLED_TRANSACTION_HASHES_68` write per new peer where there were three. - `make lint` (repeatedly) and `make erigon integration` clean.
domiwei
approved these changes
Jun 24, 2026
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.
Cherry-pick of #21597 to
performance.performancecarries #21335 (sharedp2p.Server/PeerStoreacross eth versions) but is missing theSendMessageByIdnegotiated-eth-version guard, so by-id sends (txpool new-peer pool sync) fan out across every sentry → triplicateNewPooledTransactionHashesto each new peer. This fails the hive devp2pBlobViolationstest (expected disconnect on blob violation, got msg code: 24), currently the sole CI-gate failure on #21946.Clean cherry-pick, no
performance-specific adaptations. Regression testTestGrpcServer_SendMessageById_SharedStore_NoDuplicateWritesrides along and passes.(#21597's note says no
release/3.4backport is needed because #21335 is main-only — butperformancedoes carry #21335, so it needs this.)