enh/1231-add-gossipsub-1.3-support#1237
Conversation
…e function completions
IronJam11
force-pushed
the
enh/1231-add-gossipsub-1.3-support
branch
from
149337d to
8af34e4
Compare
|
@Winter-Soren , @IronJam11 : Neat progress, friends. Lets arrive at a good conclusion on this PR this week. Please resolve CI/CD issues too. This is a substantial and very thoughtful upgrade. Bringing full GossipSub v1.3 support (including proper Extensions handling, “at most once” semantics, misbehaviour scoring, and Topic Observation with immediate IHAVE notifications) is a big step forward for py-libp2p interoperability. The separation via I especially appreciate:
Once CI issues and merge conflicts are resolved, this will be a major milestone for pubsub maturity in py-libp2p. Strong progress 🚀 |
…o and Topic Observation, add safe_bytes_from_hex and fix tests
|
Thanks for putting this together. Implementing v1.3 extensions and Topic Observation is a major step forward for The overall structure and test additions are solid, but there are a few blockers to address before merge. 🔴 Critical Blockers1) Protobuf Schema / Interop Target Needs ClarificationIn
2) Missing Newsfragment
🟡 Major Issues3) Message ID Stringification Bug in Observer PathIn msg_id_str = str(msg_id)
await self.emit_ihave(topic, [msg_id_str], observer_peer)
4) Protocol-Gating for Hello ExtensionsIn
📝 Minor / Follow-up
Validation Snapshot
Overall Assessment
|
|
Hey @Winter-Soren do you need help here ? |
|
@Winter-Soren : Hi Soham, this is a substantial and very thoughtful piece of work 👏 Bringing full GossipSub v1.3 support (including Extensions, “at most once” semantics, misbehaviour handling, and Topic Observation) is a big step forward for interoperability and overall pubsub maturity in py-libp2p. The structure with Let’s aim to close this out this week, happy to help unblock on CI or final polish if needed. CCing @acul71 for his review and feedback. We should land it in the coming release. |
Hi @seetadev, @Winter-Soren Is not responding, should I take over ? |
Thank you so much @acul71 for your comprehensive feedback, |
Hey @acul71 |
Thank you so much @seetadev |
… align pubsub IHAVE tests with hex message IDs
|
The Read the Docs job is failing before Sphinx/docx even starts: RTD is trying to check out commit |
|
I wanted to align on a few open points before we treat this as merge-ready—not as a blocker list, but so expectations are clear on both sides. 1. PR description checklist 2. 3. Protobuf / Topic Observation vs other implementations Merge readiness (not only the above) Thanks—happy to unblock once these are clarified. Full review document:
# AI PR Review: [#1237](https://github.com//pull/1237) — GossipSub v1.3 Extensions & Topic Observation
|
| Area | Change |
|---|---|
| Protobuf | rpc.proto updated with ControlExtensions (field 6), top-level TestExtension, ControlObserve / ControlUnobserve on ControlMessage; regenerated rpc_pb2.py / rpc_pb2.pyi. |
| Extensions state | New libp2p/pubsub/extensions.py: PeerExtensions, ExtensionsState (hello injection, receive path, duplicate-Extensions misbehaviour), TopicObservationState. |
| PubSub | _handle_new_peer() injects extensions only when the stream’s negotiated protocol is in _MESHSUB_V13_PLUS and the router exposes extensions_state + supports_v13_features. |
| GossipSub | PROTOCOL_ID_V13, routing through extensions_state.handle_rpc, Topic Observation (handle_observe / handle_unobserve, emit_*, _notify_observers with hex message IDs), scorer penalty on duplicate Extensions. |
| RPC queue | Splitting logic adjusted for optional ControlExtensions (commit ba224a8). |
| Tests | New test_gossipsub_v1_3_extensions.py; updates to test_gossipsub_v14_extensions.py, test_rpc_queue.py. |
| Docs | New docs/gossipsub-1.3.rst (lifecycle + example), index/examples wiring. |
Breaking changes: None called out; protocol negotiation remains backward compatible for peers on older meshsub versions.
2. Branch Sync Status and Merge Conflicts
Branch sync status
git rev-list --left-right --count origin/main...HEAD:0 33- Interpretation: The PR branch includes all of
origin/main(0 commits behind) and adds 33 commits not yet onmain(ahead). - Status: Up to date with
main; no rebase urgency for sync alone.
Merge conflict analysis
- Test merge:
git merge --no-commit --no-ff origin/main→ Already up to date (exit 0). - Conflicts: None.
✅ **No merge conflicts detected.** The PR branch can be merged cleanly into origin/main.
3. Strengths
- Spec alignment:
ControlExtensionsas a single message (field 6), experimental field numbers (e.g.6492434for test extension), and comments linking gossipsub-v1.3.md and extensions.proto match the documented v1.3 shape. - Interop documentation in-tree:
rpc.protoexplicitly notes that Topic Observation is not yet in upstreamextensions.protoand points to the ethresearch proposal and go-libp2p alignment — this addresses maintainer concern about wire-format intent. - Stateful design:
ExtensionsStatemirrors go-libp2p’s pattern (sent/received extensions, misbehaviour callback), which should ease cross-implementation reasoning. - Hello path: Extensions are added only when
negotiated_protocol in _MESHSUB_V13_PLUS— fixes the earlier “gate by negotiated protocol” review comment. - Observer IHAVE path:
_notify_observersusesmsg_id.hex()with an explicit comment tying it tosafe_bytes_from_hexinhandle_ihave— fixes thestr(bytes)bug from review. - Tests: Large dedicated suite for v1.3 extensions; rpc_queue tests updated for the new control shape.
- User-facing docs:
docs/gossipsub-1.3.rstcovers lifecycle (start_observing_topic/stop_observing_topic) and IHAVE-only semantics — addresses the “minor / follow-up” doc request from acul71’s review. - Newsfragment:
newsfragments/1231.feature.rstis present, user-focused, and newline-terminated.
4. Issues Found
Critical
- None identified in the current branch for core correctness paths reviewed (extensions hello, observer notifications, duplicate Extensions).
Major
- Maintainer feedback — protobuf registry vs. Topic Observation: acul71 asked for clarity when Topic Observation field layout differs from published upstream schemas. The PR now documents this in
rpc.proto(interop note + ethresearch link). Residual risk: other clients must share the same experimental layout for Topic Observation to interoperate; the PR description could still add one sentence pointing to the in-file note for reviewers who only read GitHub (not blocking if maintainers accept the proto comments).
Minor
- PR body checklist: The PR description still lists unchecked items (“Clean up commit history”, “Add or update documentation…”, “Add entry to the release notes”). Documentation and
newsfragments/1231.feature.rstlargely satisfy user-facing release notes via Towncrier; the checklist is stale and should be updated so reviewers do not assume missing work. - Commit history: Many merge commits from
main; not a code defect, but the author’s own TODO mentions cleanup — optional squash/rebase for history readability (maintainer preference).
- **File:** `libp2p/pubsub/extensions.py` (conceptual)
- **Line(s):** N/A
- **Issue:** `_report_misbehaviour` is typed loosely (`object`); works but reduces static assurance.
- **Suggestion:** Use `Callable[[ID], None] | None` if mypy/pyrefly allow without circular imports.
5. Security Review
-
Risk: Malicious peer sends repeated Extensions control messages.
Impact: Low (peer scoring / penalty path).
Mitigation: Implemented via_report_extensions_misbehaviour→scorer.penalize_behavior. -
Risk: OBSERVE / UNOBSERVE without advertising Topic Observation.
Impact: Low (ignored or no extra data leaked).
Mitigation:handle_observecheckspeer_supports_topic_observation(sender_peer_id). -
Risk: IHAVE message IDs to observers.
Impact: Low — IDs are hex strings consistent with the rest of the stack; avoids parse failures fromstr(bytes). -
Risk: Large or hostile protobuf control messages.
Impact: Medium in general for pubsub; not introduced uniquely by this PR beyond additional control fields. Existing queue/size limits apply; no new unbounded allocation pattern was flagged in the reviewed diff.
6. Documentation and Examples
- Docstrings: GossipSub methods (
handle_observe,_notify_observers, etc.) are documented; Sphinx formatting was fixed in an earlier commit per CI. - Guide:
docs/gossipsub-1.3.rstprovides overview, lifecycle, and a code example. - Examples index:
docs/examples.rst/docs/libp2p.pubsub.rstupdated to surface the new page. - Gap: None material for merge; optional follow-up is a runnable example under
examples/if the project wants parity with other protocols (not required by the prompt’s minimum).
7. Newsfragment Requirement
- Issue reference: #1231 is referenced in the PR body and issue text.
- File:
newsfragments/1231.feature.rst— present, ReST bullet list, user-facing, ends with newline. - Verdict: Satisfies the mandatory newsfragment requirement for this issue.
8. Tests and Validation
Local runs on checked-out PR branch (source venv/bin/activate):
| Command | Result | Notes |
|---|---|---|
make lint |
Pass (exit 0) | Pre-commit hooks including ruff, mdformat, path audit, mypy/pyrefly hooks — all passed. |
make typecheck |
Pass (exit 0) | mypy + pyrefly passed. |
make test |
Pass (exit 0) | 2605 passed, 15 skipped, ~99s. |
make linux-docs |
Pass (exit 0) | Sphinx -W build succeeded; gossipsub-1.3 included. |
Test output warnings: 24× PytestCollectionWarning in tests/core/records/test_ipns_validator.py (TestVector / TypedDict) — pre-existing / unrelated to this PR.
GitHub Actions (gh pr checks 1237): At snapshot time, Read the Docs and many tox jobs reported pass; some core matrix jobs were still pending. Re-run or wait for green across Python versions before declaring CI fully green. Prior maintainer comments about CI referred to earlier failures; current local + RTD success is encouraging.
9. Recommendations for Improvement
- Update the PR description checklist to reflect completed items (newsfragment, docs page, hex/protocol fixes).
- After CI fully completes, paste a short “validation matrix” comment (Python versions × core) for maintainers.
- Optionally tighten
ExtensionsStatecallback typing for maintainability. - If squashing is desired, coordinate with maintainers before force-pushing (affects RTD “commit not found” issues after rebases).
10. Questions for the Author
- Interop testing: Has Topic Observation been exercised against go-libp2p or rust-libp2p with the same
rpc.protolayout, or is this validated only within py-libp2p tests for now? - Future upstream: If
libp2p/specsadds Topic Observation toextensions.protowith different numbers, is the plan to version-gate or migrate in a follow-up PR?
11. Overall Assessment
- Quality Rating: Good — Large, coherent feature with tests, docs, newsfragment, and prior review feedback addressed in code (hex ID, protocol gating, newsfragment, interop notes).
- Security Impact: Low — Standard pubsub trust model; duplicate-Extensions and OBSERVE gating reduce abuse surface.
- Merge Readiness: Ready from local validation (lint, typecheck, tests, docs) and a clean merge with
main; confirm remaining pending CI matrix jobs on GitHub and maintainer acceptance of the experimental Topic Observation wire format documented inrpc.proto. - Confidence: High for the reviewed areas; Medium for cross-client Topic Observation until interop is explicitly confirmed.
Maintainer feedback tracking
| Item (from acul71 / seetadev) | Status |
|---|---|
| Newsfragment | Done (1231.feature.rst) |
| Hex message IDs for observers | Done (msg_id.hex() in _notify_observers) |
| Protocol gating for hello extensions | Done (negotiated_protocol in _MESHSUB_V13_PLUS) |
| Protobuf / interop clarification | Addressed in rpc.proto header + ethresearch / go reference |
| Docs example for Topic Observation | Done (docs/gossipsub-1.3.rst) |
| CI green | Verify pending GitHub jobs; local + RTD pass |
12. Protobuf fix and cross-implementation comparison (nim / go / js)
12.1 What “the protobuf fix” means in py-libp2p (PR #1237)
The GossipSub v1.3 spec requires:
- A single
ControlExtensionsmessage onControlMessage(field 6), advertising which extensions the sender supports. - Optional top-level
RPCfields for extension payloads (with experimental field numbers > 0x200000 where required). - Extensions only in the first RPC on the stream, at most once — that part is code (
extensions.py, hello injection), but the wire has to match the.proto.
So the “fix” is not a one-line tweak: it is bringing rpc.proto in line with that wire layout, regenerating rpc_pb2.py / rpc_pb2.pyi, and then implementing behaviour on top.
Relevant excerpts from libp2p/pubsub/pb/rpc.proto:
message ControlMessage {
repeated ControlIHave ihave = 1;
repeated ControlIWant iwant = 2;
repeated ControlGraft graft = 3;
repeated ControlPrune prune = 4;
repeated ControlIDontWant idontwant = 5;
optional ControlExtensions extensions = 6;
repeated ControlObserve observe = 7;
repeated ControlUnobserve unobserve = 8;
}
message ControlExtensions {
optional bool topicObservation = 1;
optional bool testExtension = 6492434;
}
message RPC {
// ...
optional TestExtension testExtension = 6492434;
}The file header explains the maintainer concern (Topic Observation not in upstream extensions.proto yet): document interop intent and field-number stability — that is the “protobuf / interop” fix in the review sense, not changing numbers after release.
12.2 go-libp2p (go-libp2p-pubsub)
The canonical Go schema is go-libp2p-pubsub pb/rpc.proto (pubsub lives in a separate module from the main go-libp2p repo).
It does have GossipSub v1.3–style pieces:
ControlMessage.extensions = 6→ControlExtensionswith e.g.partialMessages = 10,testExtension = 6492434.- Top-level
RPC.testExtension = 6492434, plus PartialMessages extension types.
It does not include the Topic Observation observe / unobserve / topicObservation fields from py’s rpc.proto in the upstream master copy reviewed — those are extra in py, aligned with a reference / experimental layout (as the py comments say).
How Go does it: generate Go code from .proto, plus an extensions state machine in Go (same idea as extensions.py).
12.3 nim-libp2p
GossipSub 1.3 is present as a protocol id (GossipSubCodec_13 = "/meshsub/1.3.0" in libp2p/protocols/pubsub/gossipsub/types.nim).
They model ControlExtensions and wire encode/decode in Nim (e.g. libp2p/protocols/pubsub/rpc/messages.nim, protobuf.nim), with fields such as partialMessageExtension, testExtension, pingpongExtension, preambleExtension on ControlExtensions, and extensions on ControlMessage.
They also have tests under tests/libp2p/pubsub/extensions/ for “extensions not first message”, duplicate ControlExtensions, etc. — same family of rules as v1.3, with a different extension set than py’s Topic Observation–centric rpc.proto.
12.4 js-libp2p (@libp2p/gossipsub)
No GossipSub 1.3 protocol in the sense of /meshsub/1.3.0: constants.ts stops at /meshsub/1.2.0, and gossipsub.ts registers [GossipsubIDv12, GossipsubIDv11, GossipsubIDv10].
The generated ControlMessage codec in packages/gossipsub/src/message/rpc.ts only encodes/decodes fields 1–5 (ihave / iwant / graft / prune / idontwant) — no field 6 extensions.
So js-libp2p does not implement GossipSub v1.3 Extensions on the wire in this package today.
12.5 Side-by-side
| Implementation | /meshsub/1.3.0 |
ControlMessage.extensions (field 6) |
Topic Observation | Notes |
|---|---|---|---|---|
| py-libp2p (PR) | Yes | Yes | Yes (custom fields + doc) | Matches spec shape + experimental TO; TestExtension 6492434 |
| go-libp2p-pubsub | (negotiated via pubsub) | Yes | Not in upstream rpc.proto cited |
Reference for partial messages + testExtension |
| nim-libp2p | Yes | Yes (in Nim types) | Not the same as py | Other extensions (partial, ping-pong, …) |
| js-libp2p | No | No (codec) | No | Stops at v1.2 |
13. Interop with Nim and Go (does this protobuf setup work?)
Short answer
Partially. The parts that match the shared v1.3 / go rpc.proto story are the ones that can interoperate with Go and Nim in a meaningful way. The Topic Observation and py-specific ControlExtensions fields are not something you should assume work end-to-end with stock go-libp2p-pubsub or nim-libp2p unless they implement the same experimental wire layout (or only ignore it).
What actually lines up
-
ControlMessage.extensions(field 6) — Go has this; Nim hasextensionson the control message. So the idea of v1.3 (“extensions blob on control”) is aligned. -
testExtension = 6492434onControlExtensionsand the emptyTestExtensiononRPC— Go uses the same kind of registration for the interop test extension. Py does too. That path is the right one to say: “does the v1.3 extension plumbing interoperate?” — yes, in principle, with Go, for that test extension, if both negotiate a 1.3-capable meshsub and send compatible RPCs. -
Unknown fields — Protobuf implementations are expected to ignore unknown fields. So if py sends extra fields (Topic Observation) and the other side does not define them, the usual outcome is: they are ignored, not a decode failure — unless something in the stack assumes “only known fields” in a non-standard way (unusual).
Where it does not “just work”
With Go (go-libp2p-pubsub)
- Upstream Go
ControlExtensionsuses things likepartialMessages = 10andtestExtension = 6492434, not the same set of fields as py’stopicObservation = 1onControlExtensions. - Go’s
rpc.proto(as typically shipped) does not define py’sobserve/unobserveonControlMessage(fields 7 / 8). - So:
- GossipSub at
/meshsub/1.3.0+ generic pubsub +ControlExtensions+ test extension: reasonable to expect interop for what Go actually implements. - Topic Observation (OBSERVE/UNOBSERVE,
topicObservationflag, immediate IHAVE to observers): not something you can assume works with vanilla Go until Go implements the same experimental schema (or you run a fork that does).
- GossipSub at
With Nim
- Nim has
/meshsub/1.3.0andControlExtensions, but their extension flags are oriented around partial message / ping-pong / preamble-style features, not py’s Topic Observation set. - Same story: shared mechanism (field 6, rules about first message / duplicates) can conceptually align; feature-by-feature interop requires matching definitions for each extension you care about.
Practical takeaway
| Goal | Py ↔ Go | Py ↔ Nim |
|---|---|---|
| “We’re on meshsub 1.3+ and exchange normal GossipSub” | Yes, if negotiated | Yes, if negotiated |
| “v1.3 ControlExtensions + testExtension interop” | Plausible (same field numbers as Go for that bit) | Depends on Nim’s encoding of the same fields |
| “Topic Observation as implemented in py” | Do not assume without matching Go code/proto | Do not assume without matching Nim code |
So the concern is valid: this protobuf setup is “correct” for py and for the spec-shaped core, but Topic Observation + py’s extra control fields are an explicit experimental / cross-client agreement, not something guaranteed with arbitrary Nim/Go builds. For real confidence, run interop tests against a specific Go commit and Nim commit (or document “py-only” / “py + fork X”).
One sentence for a PR: Interop with Go/Nim on base v1.3 extensions is plausible; Topic Observation requires the same experimental wire definitions on both sides or peers that only ignore unknown fields without breaking higher-level logic.
Artifacts: downloads/AI-PR-REVIEWS/1237/ — lint_output.log, typecheck_output.log, test_output.log, docs_output.log, branch_sync_status.txt, merge_conflict_check.log (regenerated for this review).
|
Thank you @acul71 for the comprehensive review 1. Checklist - Those items are stale. Docs are in 2. Typing - I’ve narrowed 3. Interop - Agreed: base GossipSub v1.3 extension mechanics are what we’re aligning with the spec and with other implementations where they support Merge readiness - Understood: green CI on the current head, resolved review threads, and maintainer sign-off remain the bar. |
|
@Winter-Soren : Hi Soham. Appreciate the thorough and thoughtful follow-up here. The clarifications around the checklist and documentation help remove any ambiguity on PR completeness, and the update to typing for The explicit callout on interop expectations is especially valuable, it sets the right context for reviewers and downstream implementers, particularly given the experimental nature of Topic Observation and related wiring. Pointing to Overall, this feels well-aligned with the GossipSub v1.3 spec, and the PR is in a good state from a maintainability and reviewability standpoint. Thanks for proactively addressing feedback and tightening the edges. Please share a detailed discussion page sharing demo screencasts and implementation improvements, steps. |
|
Please make sure the demo code in the docs works. Also there's a GossipSub v1.3 TODO at line 2330 in Once these are handled I can approve for merge. |
Co-authored-by: Paul Robinson <5199899+pacrob@users.noreply.github.com>
Thank you so much @pacrob for the review!! |
|
You can find the working demonstration of Gossipsub v1.3 here |
5 participants
py-libp2p currently lacks full implementation of the GossipSub v1.3 specification (Extensions Control Message). The protocol ID
/meshsub/1.3.0is present and some extension plumbing exists, but the wire format, first-message semantics, and “at most once” rules from the spec are not satisfied. This work implements v1.3, and the Topic Observation extension from the ethresear.ch proposal, so py-libp2p nodes can use the spec-shaped control plane and support observers that receive IHAVE-style notifications without full message payloads when both peers use this implementation’s Topic Observation wire layout (see Interop expectations below).Interop expectations
GossipSub v1.3 extensions (e.g.
ControlMessage.extensionsfield 6, first-message / at-most-once behaviour, test extension where field numbers match) are intended to align with the spec and can plausibly interoperate with other stacks that implement the same v1.3 pieces. Topic Observation (includingControlExtensions.topicObservation, OBSERVE/UNOBSERVE control fields, and immediate IHAVE to observers) follows the ethresearch proposal and is documented in-tree inlibp2p/pubsub/pb/rpc.proto; end-to-end Topic Observation with arbitrary go-libp2p-pubsub / nim-libp2p / js-libp2p peers is not guaranteed until those implementations share the same experimental wire definitions or we add explicit interop tests. Unknown protobuf fields should still be ignored by typical decoders, but feature-level interop requires matching schemas.What was wrong?
Issue #1231 — py-libp2p did not fully implement the GossipSub v1.3 specification (Extensions Control Message) or the Topic Observation extension.
Gaps included:
rpc.protodid not match the spec: noControlExtensions(single message with optional fields per extension), and no Topic Observation / TestExtension messages.How was it fixed?
Spec-aligned protobuf: Updated
libp2p/pubsub/pb/rpc.prototo addControlExtensions(field 6 onControlMessage) withtopicObservationandtestExtensionplusControlObserve,ControlUnobserve, andTestExtensiononRPC. Regeneratedrpc_pb2.pyandrpc_pb2.pyi.Extensions state machine: Added
libp2p/pubsub/extensions.pywithPeerExtensions,ExtensionsState, andTopicObservationState.ExtensionsStatehandles:build_hello_extensions(peer_id, hello).First message injection: In
pubsub.py’s_handle_new_peer(), after building the hello packet, we call the router’sextensions_state.build_hello_extensions(peer_id, hello)when the router hasextensions_stateandsupports_v13_features(duck-typing +castfor type-checking). Only v1.3-capable routers add Extensions to the first message.GossipSub v1.3 wiring in
gossipsub.py: IntroducedPROTOCOL_ID_V13(/meshsub/1.3.0), included it inadd_peerand in protocol checks (e.g._get_in_topic_gossipsub_peers_from_minus). Router now hasextensions_stateandtopic_observation(TopicObservationState), initialised with optionalmy_extensions: PeerExtensions. Inhandle_rpc, we callextensions_state.handle_rpc(rpc, sender_peer_id)first for v1.3 peers, then dispatchhandle_observe/handle_unobservefor Topic Observation. Onremove_peer, we callextensions_state.remove_peer()andtopic_observation.remove_peer().Topic Observation behaviour: Subscribers record observers via
topic_observation.add_observer(topic, peer)on OBSERVE and remove them on UNOBSERVE. Inpublish(), after validating and caching a message, we call_notify_observers(topic_ids, msg_id)to send IHAVE to all observers of those topics immediately (not only at heartbeat). Addedemit_observe/emit_unobserveand the corresponding handlers; observers are only accepted when the peer advertised the Topic Observation extension.Misbehaviour on duplicate Extensions: When a peer sends Extensions and we already have their extensions, we call
_report_extensions_misbehaviour(peer_id), which usesscorer.penalize_behavior(peer_id, 1.0).Result: v1.3 protocol ID is supported, Extensions are sent only in the first message and at most once per peer, duplicate Extensions are penalised, and Topic Observation (OBSERVE/UNOBSERVE + immediate IHAVE to observers) is implemented, with a small set of well-scoped stub methods left for new contributors.
To-Do
docs/gossipsub-1.3.rstnewsfragments/1231.feature.rst(Towncrier / #1231)Cute Animal Picture