test(interop): go-libp2p WebRTC-Direct interop tests - #1471
Open
yashksaini-coder wants to merge 21 commits into
Open
test(interop): go-libp2p WebRTC-Direct interop tests#1471yashksaini-coder wants to merge 21 commits into
yashksaini-coder wants to merge 21 commits into
Conversation
…rity by DTLS role go-libp2p (go-msgio pbio fallback) writes the uvarint prefix and the protobuf body as two SCTP messages, so every py<->go WebRTC-Direct connection died at Noise msg#1 with "malformed handshake frame length". New stream._decode_frames pops complete frames off an accumulating buffer (frames may be split or batched across SCTP messages, bounded by MAX_MESSAGE_SIZE); DataChannelReadWriter and WebRTCStream.on_data both use it. A malformed frame now resets the stream instead of dropping one message and desynchronising. With the handshake passing, a py listener then hit aiortc's `assert stream_id not in self._data_channels`: aiortc picks DCEP stream id parity from the ICE role, which is inverted for a WebRTC-Direct listener (ICE-controlled, DTLS server), so it used even ids like go's dialer. _create_channel now passes an explicit id by DTLS role (RFC 8832: client even, server odd). Verified live against go-libp2p v0.49.0: go->py and py->go, v1 and v2. Refs libp2p#1437
- keep aiortc allocating (and recycling) SCTP stream ids; only re-seed the parity from the DTLS role (explicit ids tied the 16-bit space to our never-recycled counter) - on_data: deliver frames decoded before a malformed one, reset the peer (RESET + cleanup) instead of a local-only reset, ignore bytes after reset, and apply a whole SCTP message in one trio hop - _decode_frames: derive the max prefix length from MAX_MESSAGE_SIZE Refs libp2p#1437
The test performs two full dials and two bounded peer-connection closes; on Windows each close can take ~5 s when a datagram write is in flight, so 30 s was flaky (seen once on CI, no socket errors logged). Refs libp2p#1437
… collides Harness mode binds TCP on the same port number as the OS-chosen UDP mux port; Windows reserves port ranges per protocol, so that TCP bind can fail with WinError 10013 (seen on CI in the harness loopback test). Retry up to five times with a new UDP port before giving up; explicit ports still fail loudly. Refs libp2p#1437
…epts v2, dialer opt-in Listener: stop dropping libp2p+webrtc+v2/ first contacts. The server ufrag minus the prefix is the dialer's ICE pwd (must be a valid one, 22..256 ice-chars, else rejected in parse_direct_username); the inferred offer uses client_ufrag + that pwd, and our local ufrag/pwd stay server_ufrag verbatim as in v1. _accept_v1 -> _accept(client_pwd=...). Dialer: new WebRTCTransportConfig.webrtc_direct_dial_version (default 1). v2 keeps the aioice ufrag/pwd (no munging) and sets the synthetic answer's ufrag == pwd to "libp2p+webrtc+v2/" + local_password. Default stays v1 while specs#715 is unmerged; unknown values raise WebRTCConnectionError. Tests: v2 parse/reject cases, SDP shape, v2 loopback echo, no-munge check, short-suffix rejection, mixed v1+v2 dialers on one listener. Refs libp2p#1437
- validate webrtc_direct_dial_version in the config (was only checked mid-dial after the PC was built) - parse_direct_username returns the validated client pwd; make_v2_credential; one pwd check instead of two version branches - inline the v1 credential writes; trim the dial() comment Refs libp2p#1437
… RESET guards - _create_channel: a missing RTCDtlsTransport._role now degrades to aiortc's default stream-id parity with a warning instead of asserting (an assert in a live connection path, also stripped under -O) - _decode_frames: reject consumed == 0 from the varint decoder rather than silently indexing head[-1] - _reset_locally is idempotent; _apply_batch_on_trio_thread stops delivering a batch's remaining frames once the stream is RESET - note in DataChannelReadWriter._fill that frames after FIN/RESET are dropped by design; wrap the newsfragment Refs libp2p#1437
Review follow-up on libp2p#1459: README readers should see that libp2p/specs#715 is unmerged and cross-implementation v2 interop is not yet guaranteed (config.py already said so). Refs libp2p#1437
27 tasks
A small pinned go-libp2p v0.49 harness (listen / dial -version N), built on demand under a lock and skipped when the Go toolchain is missing, drives our WebRTC-Direct transport against a real go peer. py -> go is verified live for v1 and v2; go -> py is xfail (strict) pending libp2p#1470, where the listener tears the peer connection down right after ICE completes. Refs libp2p#1437, libp2p#1470
yashksaini-coder
force-pushed
the
feat/webrtc-go-interop-tests
branch
from
September 3, 2026 18:12
cb9db7f to
c01c64c
Compare
Collaborator
Hi @yashksaini-coder , you used claude code with which LLM ? |
acul71
requested changes
Sep 3, 2026
acul71
left a comment
acul71
left a comment
Collaborator
There was a problem hiding this comment.
Review readiness
Thanks for the clean interop harness — scope matches the description (pinned go-libp2p v0.49 listen/dial, py→go live, go→py strict xfail for #1470), and the #1459 dependency is already on main.
Not ready to dig into a full review pass yet; a couple of blockers first:
Blockers
-
Lint is red. Recent CI fails typecheck on
test_webrtc_direct_interop.py:Object of class `NoneType` has no attribute `receive_some`(
proc.stdoutis optional afteropen_process). Please assert/narrowstdout(or otherwise satisfy the checker) and get lint green. -
Maintainer question still open — which Claude model did you use with Claude Code? (comment above)
Follow-ups (can land with or right after the lint fix)
- Go is not installed in CI.
.github/workflows/tox.ymlsets up Nim/Node for interop, but not Go. Withoutactions/setup-go(matchinggo.mod/ go-libp2p v0.49), these tests will skip on Actions and CI won’t actually exercise the harness. - Newsfragment is
1470.internal.rst(issue id); consider1471if that’s the project convention for the PR that adds the tests. - Commit history is noisy (already-merged framing/v2 work + merges); net diff vs
mainis fine — squash-on-merge is enough.
Happy to do a deeper review once lint is green (and ideally once Go is wired into the interop job so CI runs the new tests for real).
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
2 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.
The formal go-libp2p interop tests for #1437.
A small pinned go-libp2p v0.49 harness (
tests/interop/go_libp2p/webrtc_direct/,listen/dial -version N) is built on demand under a file lock and skipped when the Go toolchain is missing, then driven against our WebRTC-Direct transport in both directions and both versions.xfail(strict) — the inbound path tears the peer connection down right after ICE completes, tracked in WebRTC-Direct: go-libp2p dialer can't connect to the py listener (ICE succeeds, then the connection is torn down) #1470. The strict xfail means the suite will flag it the moment that's fixed.Best on top of #1459 (needs the v2 dialer + framing fixes), but the harness/tests stand alone. go → py is a real listener-side bug, not something this PR introduces.
Refs #1437
🤖 Generated with Claude Code