Revert "feat: quic try (#32)" - #49
Conversation
This reverts commit adf27e3.
📝 WalkthroughWalkthroughThe P2P node now uses TCP transport with mplex multiplexing and Noise security instead of QUIC. Advertised and listen addresses use TCP. Peer address prediction uses the general advertised-address builder. The QUIC startup smoke test was removed. Mplex stream reset timeouts are set to five seconds. Third-party license links were updated for changed dependency versions. Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Bootstrapper
participant Libp2pHost
participant TCPPeer
Bootstrapper->>Libp2pHost: Build advertised TCP addresses
Libp2pHost->>TCPPeer: Listen on TCP address
TCPPeer-->>Libp2pHost: Establish Noise-secured mplex streams
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/service/bootstrapper/service.go`:
- Line 185: Add focused tests for predictMumP2PAddrInfo covering IPv4-only,
IPv6-only, and dual-stack inputs, and assert each advertised address uses the
/tcp/<AgentMumP2PPort> protocol matching the TCP listener in mum_p2p service.
In `@pkg/service/mum_p2p/service.go`:
- Around line 90-102: Add a focused two-node test for NewNode that constructs
both nodes with the changed TCP transport, Noise security, mplex muxer, and
advertised-address configuration instead of NewNodeWithHost. Assert the
resulting advertised addresses include the expected /tcp/ endpoint, then connect
the nodes and exchange data over a stream to exercise the configured transport
options.
- Line 117: Remove the duplicate gomplex.ResetStreamTimeout assignment from the
shown initialization path, leaving the existing assignment in Service.Run as the
sole configuration of this process-wide variable before the gateway host is
created.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a9cc9e8-1991-4593-8051-fa3fc8757663
⛔ Files ignored due to path filters (4)
docs/sbom-full.jsonis excluded by none and included by nonedocs/sbom.jsonis excluded by none and included by nonego.modis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (4)
THIRD-PARTY-NOTICES.mdpkg/service/bootstrapper/service.gopkg/service/mum_p2p/node_test.gopkg/service/mum_p2p/service.go
💤 Files with no reviewable changes (1)
- pkg/service/mum_p2p/node_test.go
| libp2p.BandwidthReporter(telemetry.NewBandwidthCollector()), | ||
| ) | ||
| } | ||
| gomplex.ResetStreamTimeout = 5 * time.Second // Configures stream timeouts on mplex |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 'ResetStreamTimeout\s*=|NewNode\s*\(|setupLibP2PHost\s*\(' .Repository: getoptimum/optimum-gateway
Length of output: 5612
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- all gomplex references ---'
rg -n -C 3 'gomplex|ResetStreamTimeout' --glob '*.go' --glob 'go.mod' --glob 'go.sum' .
printf '%s\n' '--- NewNode callers and service startup ---'
rg -n -C 8 'mum_p2p\.NewNode|\.Run\(\)|setupLibP2PHost' pkg --glob '*.go'
printf '%s\n' '--- relevant service.go ---'
sed -n '1,180p' pkg/service/mum_p2p/service.go
printf '%s\n' '--- gateway startup ---'
sed -n '120,190p' pkg/service/gossipsub-gateway/service.go
sed -n '1,155p' pkg/service/gossipsub-gateway/setup_libp2p_host.go
sed -n '1,90p' pkg/service/gossipsub-gateway/setup_mump2p_host.goRepository: getoptimum/optimum-gateway
Length of output: 39772
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- dependency versions ---'
rg -n 'go-mplex|go-libp2p' go.mod go.sum
printf '%s\n' '--- startup call sites and concurrency markers ---'
rg -n -C 6 'NewService\(|func \(s \*Service\) Run|go .*Run\(|go .*setup|sync\.Once|oncer' pkg cmd main.go --glob '*.go' 2>/dev/null || true
printf '%s\n' '--- local module cache source, if present ---'
find "${GOMODCACHE:-$HOME/go/pkg/mod}" -path '*go-mplex*' -type f \( -name '*.go' -o -name 'go.mod' \) -print 2>/dev/null | head -20Repository: getoptimum/optimum-gateway
Length of output: 34723
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL 'https://api.github.com/repos/libp2p/go-mplex/git/trees/v0.7.0?recursive=1' >"$tmp"
jq -r '.tree[] | select(.path | endswith(".go")) | .path' "$tmp"
printf '%s\n' '--- files that mention ResetStreamTimeout ---'
while IFS= read -r path; do
body=$(curl -fsSL "https://raw.githubusercontent.com/libp2p/go-mplex/v0.7.0/$path")
if grep -q 'ResetStreamTimeout' <<<"$body"; then
printf '%s\n' "### $path"
grep -n -C 8 'ResetStreamTimeout' <<<"$body"
fi
done < <(jq -r '.tree[] | select(.path | endswith(".go")) | .path' "$tmp")Repository: getoptimum/optimum-gateway
Length of output: 215
🌐 Web query:
github libp2p go-mplex v0.7.0 ResetStreamTimeout source
💡 Result:
In the libp2p/go-mplex library, ResetStreamTimeout is a package-level variable that defines the timeout duration for sending a stream reset message [1][2]. It is explicitly set to 2 minutes (2 * time.Minute) [1][2]. This variable is used within the multiplexer's internal logic to ensure that stream reset operations do not block indefinitely [3]. Specifically, when the library attempts to send a reset message, it uses a context derived with this timeout [1]. If the operation fails to complete within the specified 2-minute window, the library logs the error and, in cases of a hard reset, closes the connection entirely to maintain system stability [1][3]. The implementation was introduced to address issues where calls to close or reset streams could otherwise block forever [3]. As of version v0.7.0 and beyond, the library remains maintained in this state, though it is officially deprecated in favor of yamux or QUIC for new implementations [4][5].
Citations:
- 1: https://github.com/libp2p/go-mplex/blob/master/multiplex.go
- 2: https://pkg.go.dev/gitlab.dms3.io/p2p/go-mplex
- 3: use timeouts when sending messages for stream open, close, and reset. libp2p/go-mplex#52
- 4: https://pkg.go.dev/github.com/libp2p/go-mplex
- 5: https://github.com/libp2p/go-mplex
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import io
import urllib.request
import zipfile
url = "https://proxy.golang.org/github.com/libp2p/go-mplex/@v/v0.7.0.zip"
data = urllib.request.urlopen(url).read()
with zipfile.ZipFile(io.BytesIO(data)) as archive:
for name in archive.namelist():
if name.endswith(".go"):
text = archive.read(name).decode()
if "ResetStreamTimeout" in text:
print(f"### {name}")
lines = text.splitlines()
for i, line in enumerate(lines):
if "ResetStreamTimeout" in line:
start = max(0, i - 8)
end = min(len(lines), i + 9)
for number in range(start, end):
print(f"{number + 1}: {lines[number]}")
PYRepository: getoptimum/optimum-gateway
Length of output: 2450
Remove the duplicate global assignment. Service.Run already sets gomplex.ResetStreamTimeout before creating the gateway host. This line writes the same process-wide variable after that host is live, which can race with mplex stream-reset handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/service/mum_p2p/service.go` at line 117, Remove the duplicate
gomplex.ResetStreamTimeout assignment from the shown initialization path,
leaving the existing assignment in Service.Run as the sole configuration of this
process-wide variable before the gateway host is created.
Source: Path instructions
2 participants
This reverts commit adf27e3.
Summary by CodeRabbit
Changes
Documentation
Tests