p2p/protocols/eth, p2p/sentry/libsentry, node/direct: derive msgid tables instead of triple-maintaining them#22175
Draft
yperbasis wants to merge 2 commits into
Draft
p2p/protocols/eth, p2p/sentry/libsentry, node/direct: derive msgid tables instead of triple-maintaining them#22175yperbasis wants to merge 2 commits into
yperbasis wants to merge 2 commits into
Conversation
…sts for message-id tables Pin the exact content of eth.ToProto/eth.FromProto, wit.ToProto/wit.FromProto and libsentry.ProtoIds ahead of deriving them from a single source per protocol version. TestFromProtoOmitsStatus additionally pins the deliberate asymmetry: FromProto for eth/69+ omits the status ids (status is handshake-only and must stay unreachable via SendMessageById).
…ct: single source per version for message-id tables Previously every new eth version required copying a ~14-entry message-id map into three files: eth.ToProto, eth.FromProto (hand-written inverse) and libsentry.ProtoIds (hand-written value-set). Now each eth version's ToProto map is built as a clone of the previous version plus a delta, and the other two tables are derived from it: - eth.FromProto = inversion of ToProto, skipping StatusMsg so that SendMessageById can never emit a status frame (the pre-existing, deliberate asymmetry, pinned by TestFromProtoOmitsStatus); - libsentry.ProtoIds = key sets of eth.FromProto and wit.FromProto, which is exactly what the hand-written table contained (including the status omission for eth/69+); - wit.FromProto = inversion of wit.ToProto. Deriving ProtoIds requires libsentry to import the protocol packages, so the dependency direction is flipped: the ETH68..ETH71 version constants move from node/direct into p2p/protocols/eth (wit already had WIT1), and the version<->Protocol bridge maps move from node/direct into libsentry. node/direct keeps its former API as aliases, so no call sites change. Inversions and bridge lookups panic on collisions/missing versions so a future version bump cannot silently drop or misroute an entry. Golden tests pin all three tables to their exact pre-refactor content.
yperbasis
requested review from
AskAlexSharov,
anacrolix and
taratorio
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes triple-maintained ETH/WIT message-id tables by deriving FromProto (and libsentry.ProtoIds) from a single ToProto source-of-truth, while preserving the deliberate “status is handshake-only” asymmetry.
Changes:
- Refactors
ethandwitprotocol msg-id mappings to deriveFromProtovia inversion (with collision panics) and constructs ETH version tables via “base + deltas”. - Derives
libsentry.ProtoIdsfrometh.FromProto/wit.FromProto, and centralizes protocol-version ↔sentryproto.Protocolbridge maps inlibsentry. - Moves ETH version constants into
p2p/protocols/ethand re-exports them fromnode/directto avoid import cycles without changing external call sites; adds golden tests pinning pre-refactor table contents.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| p2p/sentry/libsentry/protocol.go | Centralizes protocol-version bridge maps and derives ProtoIds from protocol FromProto tables. |
| p2p/sentry/libsentry/protocol_msgid_golden_test.go | Golden test pinning libsentry.ProtoIds content. |
| p2p/protocols/wit/protocol.go | Derives wit.FromProto by inverting ToProto (collision-checked). |
| p2p/protocols/wit/protocol_msgid_golden_test.go | Golden tests pinning wit.ToProto / wit.FromProto. |
| p2p/protocols/eth/protocol.go | Introduces ETH version constants in-package; builds ToProto via base+delta and derives FromProto via inversion omitting status. |
| p2p/protocols/eth/protocol_test.go | Updates tests to use eth.ETH71 constant directly (no direct import). |
| p2p/protocols/eth/protocol_msgid_golden_test.go | Golden tests pinning eth.ToProto / eth.FromProto and asserting status omission contract. |
| p2p/protocols/eth/handlers.go | Updates documentation comment examples to use eth.ETH* constants. |
| p2p/protocols/eth/handlers_receipts_query_test.go | Updates receipt query test to use eth.ETH68 constant (no direct import). |
| node/direct/sentry_client.go | Re-exports ETH/WIT version constants and protocol maps via eth/wit/libsentry to keep external usages unchanged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Every new eth protocol version copied ~14-entry message-id map blocks into three hand-maintained tables:
eth.ToProto,eth.FromProto(its inverse), andlibsentry.ProtoIds. eth/68→71 accumulated four copies in each.Now:
ToProtois a literal eth/68 base plus per-version deltas (3/2/2 entries for 69/70/71);FromProtois derived by inversion with a collision panic;ProtoIdsis derived in libsentry from theFromProtokeysets. The one deliberate asymmetry is preserved and pinned:FromProto/ProtoIdsomitSTATUS_69for eth/69+ (status is handshake-only;SendMessageByIdmust not emit a status frame) —TestFromProtoOmitsStatusderives the omitted set from goldens and asserts it exactly. wit's single-version tables get the same inversion treatment.Golden tests were committed and verified green before the refactor: three external-package test files pin the exact pre-refactor content of all three tables with literal version keys (+325 test lines).
Structural note: the import graph pointed the wrong way for a single-copy derivation (
ethimportednode/directfor the version constants). The version constantsETH68..ETH71move into packageeth(where the protocol lives),directre-exports everything as aliases so its ~109 external usage sites are untouched, and the version↔sentryproto.Protocolbridge maps live once in libsentry. The alias direction makes a futureeth → directre-import a compile error.Net −72 production lines, and the next eth version becomes a one-delta change. Verified: goldens pre/post,
go test ./p2p/... ./node/direct/..., scopedgolangci-lintclean (×2),make erigon.Part of a dedup series; siblings #22165–#22174.