Hole Punching Interop Between Go-libp2p And Py-libp2p.

[asmit27rai]

Description

Hole Punching Interop between go-libp2p and py-libp2p.

Test

cd py-libp2p/tests/interop/go_libp2p/hole_punching

chmod +x test_local.sh
./test_local.sh

Thanks

Fix : #733

[asmit27rai]

@seetadev Please Review This PR.
Thanks

[seetadev]

@asmit27rai : Thank you for submitting the PR. Appreciate it.

CCing @sukhman-sukh, @acul71 and @Winter-Soren, who will review your PR and share feedback points soon.

I'll review it on my side as well.

[seetadev]

@asmit27rai : Re-ran the CI/CD pipeline. Please resolve the test failure issue.

[acul71]

Hello @asmit27rai
I've been trying this PR, the test struct is working.
I've seen you're testing circuit relays, I'm guessing next you'll implement

• Use /libp2p/dcutr protocol
• Implement DCUtR message exchange (CONNECT/SYNC messages)
• Add observed address exchange logic
• Implement direct connection attempt after DCUtR exchange

[seetadev]

@asmit27rai : Kindly reply to Luca's feedback. We should get this PR ready soon.

[asmit27rai]

Hello @asmit27rai I've been trying this PR, the test struct is working. I've seen you're testing circuit relays, I'm guessing next you'll implement

• Use /libp2p/dcutr protocol
• Implement DCUtR message exchange (CONNECT/SYNC messages)
• Add observed address exchange logic
• Implement direct connection attempt after DCUtR exchange

Hello @asmit27rai I've been trying this PR, the test struct is working. I've seen you're testing circuit relays, I'm guessing next you'll implement

• Use /libp2p/dcutr protocol
• Implement DCUtR message exchange (CONNECT/SYNC messages)
• Add observed address exchange logic
• Implement direct connection attempt after DCUtR exchange

Yes. I am planning in same way.
Once this PR merged I will start working on this and may be raise a PR for this.
@acul71

[seetadev]

@asmit27rai and @acul71 : Please resolve the CI/CD issues.

[acul71]

@asmit27rai and @acul71 : Please resolve the CI/CD issues.

Sorry for some reason I can't see the failing CI/CD issues here. checking out asmit27rai:Hole_punch_gopy
I'll check the PR

[asmit27rai]

@seetadev @acul71 Please have a look and review the PR.

[acul71]

@seetadev @acul71 Please have a look and review the PR.

AI Pull Request Review: PR #936 - Hole Punching Interop Between Go-libp2p And Py-libp2p

Review Date: 2025-01-27
PR Number: 936
Author: asmit27rai
Status: OPEN
Related Issue: #733

---

1. Summary of Changes

This PR adds interoperability tests for hole punching functionality between go-libp2p and py-libp2p. The implementation includes:

• New test infrastructure in tests/interop/go_libp2p/hole_punching/:

  • Python implementations: hole_punch_client.py, hole_punch_server.py, relay_server.py
  • Go implementations: hole-punch-client, hole-punch-server, relay-server
  • Test orchestration script: test_local.sh

• Functionality:

  • Python client that connects to a Go server through a relay
  • Python server that can accept connections from Go clients
  • Python relay server implementation
  • Test script that orchestrates all components and validates interop

• Modules affected:

  • tests/interop/go_libp2p/hole_punching/ (new directory)
  • No changes to core libp2p modules

This PR addresses issue #733, which requests interoperability tests for hole punching between py-libp2p and other libp2p implementations (go-libp2p, js-libp2p, etc.). The implementation focuses on go-libp2p interop as a first step.

No breaking changes or deprecations are introduced.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

• Status: ✅ Ahead of origin/main
• Details: Branch is 0 commits behind and 8 commits ahead of origin/main
• Interpretation: The PR branch contains 8 new commits that are not yet in main. This is expected for a feature branch.

Merge Conflict Analysis

• Conflicts Detected: ✅ No conflicts - PR can be merged cleanly
• Details: The test merge completed successfully with no conflicts detected. The PR branch can be merged cleanly into origin/main.

---

3. Strengths

1. Well-structured test infrastructure:

   • Clear separation between Python and Go components
   • Comprehensive test script (test_local.sh) that orchestrates all components
   • Good logging and error handling in the test script

2. Defensive programming:

   • Python code handles missing DCUtR protocol gracefully with fallback behavior
   • Proper exception handling throughout
   • Defensive checks for None values before accessing attributes

3. Good test coverage:

   • Tests both client and server roles
   • Includes relay server implementation
   • Validates connection establishment, stream opening, and message exchange

4. Clear documentation:

   • Scripts include helpful comments
   • Test script provides clear output and error messages
   • Good use of logging for debugging

5. Follows project patterns:

   • Uses new_host() correctly (synchronous, not async)
   • Proper use of trio for async operations
   • Consistent with other interop test patterns in the codebase

---

4. Issues Found

Critical

None identified.

Major

4.1 Missing Type Annotations (Type Checking Errors)

• File: tests/interop/go_libp2p/hole_punching/py_node/hole_punch_client.py

• Line(s): 20-22, 27-28, 30-31, 53, 68, 137-139, 142

• Issue: Type checker (pyrefly) reports 44 type errors. All attributes are initialized as None but then assigned non-None values, causing type checker to complain about NoneType attribute access.

• Suggestion: Add proper type annotations using Optional[IHost], Optional[ID], etc., or use type: ignore comments if the defensive None checks are sufficient. Example:

  from typing import Optional
  from libp2p.host.interface import IHost
  from libp2p.peer.id import ID
  
  class SimpleHolePunchClient:
      def __init__(self):
          self.host: Optional[IHost] = None
          self.target_peer_id: Optional[ID] = None
          self.connected_via_relay = False

• File: tests/interop/go_libp2p/hole_punching/py_node/hole_punch_server.py

• Line(s): 33-34, 45, 47, 52, 56, 58, 67, 69-70, 73, 75, 82, 109, 132, 193, 196

• Issue: Same type annotation issues as above.

• Suggestion: Add proper type annotations for host and dcutr_protocol attributes.

• File: tests/interop/go_libp2p/hole_punching/py_node/relay_server.py

• Line(s): 23-24, 31, 68-69, 73-74, 84-85, 87-88, 91, 104-105, 158, 161

• Issue: Same type annotation issues as above.

• Suggestion: Add proper type annotations for host and relay_protocol attributes.

4.2 Incorrect Type for listen_addrs Parameter

• File: tests/interop/go_libp2p/hole_punching/py_node/hole_punch_server.py

• Line(s): 45, 193

• Issue: new_host(listen_addrs=listen_addrs) is called with list[str] but expects Sequence[Multiaddr] | None. The code passes string multiaddrs instead of Multiaddr objects.

• Suggestion: Convert string multiaddrs to Multiaddr objects:

  from multiaddr import Multiaddr
  
  listen_addrs = []
  if self.port > 0:
      listen_addrs = [Multiaddr(f"/ip4/0.0.0.0/tcp/{self.port}")]

• File: tests/interop/go_libp2p/hole_punching/py_node/relay_server.py

• Line(s): 31, 158

• Issue: Same issue - passing string instead of Multiaddr object.

• Suggestion: Convert to Multiaddr objects as shown above.

4.3 Missing Newsfragment

• File: newsfragments/733.* (missing)
• Issue: ⚠️ CRITICAL BLOCKER - No newsfragment file exists for issue Hole Punching Interop Tests for py-libp2p with other libp2p modules #733. According to the project requirements, every PR that fixes an issue MUST have a corresponding newsfragment file.
• Suggestion: Create newsfragments/733.feature.rst (or 733.misc.rst if this is considered a test-only change) with content:

  Added interoperability tests for hole punching between py-libp2p and go-libp2p.
  

  The file must end with a newline character.

Minor

4.4 Line Length Violations (E501)

• File: tests/interop/go_libp2p/hole_punching/py_node/hole_punch_client.py

• Line(s): 80, 111, 112, 113

• Issue: 4 lines exceed 88 characters (project limit is 88).

• Suggestion: Break long lines appropriately. The linting tool auto-fixed some issues but 21 remain.

• File: tests/interop/go_libp2p/hole_punching/py_node/hole_punch_server.py

• Line(s): 47, 49, 50, 65, 73, 101, 103, 152, 153, 154

• Issue: 10 lines exceed 88 characters.

• Suggestion: Break long lines appropriately.

• File: tests/interop/go_libp2p/hole_punching/py_node/relay_server.py

• Line(s): 54, 65, 68, 70, 82, 127, 128

• Issue: 7 lines exceed 88 characters.

• Suggestion: Break long lines appropriately.

4.5 Inconsistent Error Handling

• File: tests/interop/go_libp2p/hole_punching/py_node/hole_punch_client.py
• Line(s): 61-62
• Issue: Generic Exception catch-all without specific error types.
• Suggestion: While acceptable for test code, consider catching more specific exceptions where possible.

4.6 Missing Docstrings

• File: All Python files
• Issue: Classes and methods have docstrings, but some could be more detailed (e.g., explaining the hole punching protocol flow).
• Suggestion: Add more detailed docstrings explaining the DCUtR protocol and hole punching flow for future maintainers.

---

5. Security Review

No security vulnerabilities identified.

The code:

• ✅ Does not handle sensitive data
• ✅ Uses standard libp2p APIs correctly
• ✅ Does not expose any unsafe operations
• ✅ Properly validates peer IDs and multiaddrs
• ✅ Uses defensive programming (None checks, exception handling)

Security Impact: None

---

6. Documentation and Examples

Strengths

• ✅ Test script includes helpful comments
• ✅ Python files have docstrings for classes and main functions
• ✅ Test script provides clear output messages

Missing Documentation

• ⚠️ No README.md in tests/interop/go_libp2p/hole_punching/ explaining:
  • How to run the tests
  • Prerequisites (Go installation, etc.)
  • What the tests validate
  • Expected behavior
• ⚠️ No documentation about the hole punching protocol flow in the code
• ⚠️ No examples in the main documentation about using hole punching

Suggestion: Add a README.md file in the test directory explaining:

1. Prerequisites (Go 1.18+, Python 3.8+, etc.)
2. How to run the tests
3. What the tests validate
4. Troubleshooting tips

---

7. Newsfragment Requirement

⚠️ CRITICAL: Missing Newsfragment - BLOCKER

• Status: ❌ MISSING

• Issue: PR references issue Hole Punching Interop Tests for py-libp2p with other libp2p modules #733 but no newsfragment file exists

• Required File: newsfragments/733.feature.rst (or 733.misc.rst if this is test-only)

• Required Format:

  Added interoperability tests for hole punching between py-libp2p and go-libp2p.
  

  (Must end with newline)

• Action Required: PR approval is blocked until:

  1. A newsfragment file is created using the issue number (Hole Punching Interop Tests for py-libp2p with other libp2p modules #733)
  2. The file follows the format <ISSUE_NUMBER>.<TYPE>.rst
  3. The file contains a user-facing description and ends with a newline

Note: Since this is a test addition (not a user-facing feature), consider using .misc.rst instead of .feature.rst. However, if hole punching interop is considered a feature, use .feature.rst.

---

8. Tests and Validation

Linting (make lint)

Status: ❌ FAILED (Exit code: 1)

Issues Found:

1. Trailing whitespace: Fixed automatically (5 files)
2. Line length violations (E501): 21 remaining errors across 3 Python files
   • hole_punch_client.py: 4 lines
   • hole_punch_server.py: 10 lines
   • relay_server.py: 7 lines
3. Ruff format: Fixed automatically (3 files reformatted)

Action Required: Fix remaining line length violations.

Type Checking (make typecheck)

Status: ❌ FAILED (Exit code: 1)

Issues Found:

• 44 type errors reported by pyrefly:
  • All related to None type annotations
  • Attributes initialized as None but then assigned non-None values
  • Type checker cannot infer that defensive None checks ensure non-None values
  • Incorrect type for listen_addrs parameter (expects Multiaddr objects, not strings)

Action Required:

1. Add proper type annotations using Optional[Type] or Type | None
2. Convert string multiaddrs to Multiaddr objects before passing to new_host()

Test Execution (make test)

Status: ✅ PASSED

Results:

• Total tests: 1806 passed, 13 skipped, 25 warnings
• Execution time: 90.12 seconds
• No test failures related to this PR
• Warnings:
  • 24 warnings about unknown pytest mark @pytest.mark.integration in test_proxy.py (unrelated to this PR)
  • 1 RuntimeWarning about unawaited coroutine in test_muxer_multistream.py (unrelated to this PR)

Note: The new interop tests are not automatically run by make test (they require Go installation and manual execution). This is acceptable for interop tests.

Documentation Build (make linux-docs)

Status: ✅ PASSED

Results:

• Documentation built successfully
• No errors or warnings related to this PR
• No new documentation was added for this feature (acceptable for test-only changes)

---

9. Recommendations for Improvement

1. Fix type annotations (HIGH PRIORITY):

   • Add Optional[IHost], Optional[ID], etc. type annotations
   • Convert string multiaddrs to Multiaddr objects
   • This will resolve all 44 type checking errors

2. Fix line length violations (MEDIUM PRIORITY):

   • Break long lines to comply with 88-character limit
   • 21 violations remain after auto-formatting

3. Add newsfragment (CRITICAL BLOCKER):

   • Create newsfragments/733.feature.rst or newsfragments/733.misc.rst
   • Include user-facing description
   • Ensure file ends with newline

4. Add README.md (NICE TO HAVE):

   • Document prerequisites, usage, and troubleshooting
   • Explain the test flow and expected behavior

5. Consider adding pytest integration:

   • While manual execution is fine, consider adding a pytest wrapper that can run the tests if Go is available
   • This would allow the tests to run in CI

6. Improve error messages:

   • Add more specific error handling where possible
   • Provide clearer error messages for common failure scenarios

---

10. Questions for the Author

1. Type annotations: The code uses defensive None checks but the type checker still complains. Should we add Optional[Type] annotations or use # type: ignore comments? What's the project's preferred approach?

2. Newsfragment type: Should this be .feature.rst (if hole punching interop is a feature) or .misc.rst (if it's just test infrastructure)?

3. Multiaddr conversion: The code passes string multiaddrs to new_host(), but the type signature expects Multiaddr objects. Does new_host() accept strings and convert them internally, or should we convert them explicitly?

4. CI integration: Are there plans to integrate these interop tests into CI? If so, how will Go installation be handled?

5. Test coverage: The tests validate basic connection and stream opening. Are there plans to add more comprehensive tests (e.g., actual hole punching, not just relayed connections)?

---

11. Overall Assessment

Quality Rating: Needs Work

The PR adds valuable interop test infrastructure, but has several issues that must be addressed before approval:

• ❌ Missing newsfragment (BLOCKER)
• ❌ Type checking failures (44 errors)
• ❌ Linting failures (21 line length violations)
• ✅ Tests pass
• ✅ Documentation builds
• ✅ No merge conflicts

Security Impact: None

No security vulnerabilities identified.

Merge Readiness: Needs fixes

Blockers:

1. Missing newsfragment for issue Hole Punching Interop Tests for py-libp2p with other libp2p modules #733
2. Type checking errors (44 errors)
3. Linting errors (21 line length violations)

Recommended fixes before merge:

1. Add newsfragment file
2. Fix type annotations
3. Fix line length violations
4. Convert string multiaddrs to Multiaddr objects

Confidence: High

The code structure is sound and follows project patterns. The issues are straightforward to fix (type annotations, line length, newsfragment). The test infrastructure is well-designed and will be valuable for validating hole punching interop.

---

Summary

This PR adds important interoperability test infrastructure for hole punching between py-libp2p and go-libp2p. The implementation is well-structured and follows project patterns, but requires fixes for:

1. CRITICAL: Missing newsfragment for issue Hole Punching Interop Tests for py-libp2p with other libp2p modules #733
2. MAJOR: Type checking errors (44 errors) - need proper type annotations
3. MINOR: Line length violations (21 errors) - need to break long lines

Once these issues are addressed, the PR should be ready for approval. The test infrastructure will be valuable for ensuring hole punching interoperability across libp2p implementations.

[asmit27rai]

@acul71 I worked on the issues you mentioned please have a look.

[acul71]

Hello @asmit27rai thank you for this PR.

Can you answer to this questions ?

1. Are there plans to add more comprehensive hole punching tests (actual NAT traversal, not just relayed connections)?
2. Are there plans to integrate these interop tests into CI? (Currently, these tests are manual integration tests run via test_local.sh and are not automatically executed by the CI/CD pipeline, which only runs pytest-based tests in tests/interop.)
3. Are there plans to add interop tests with other libp2p implementations (js-libp2p, nim-libp2p, etc.)?
4. Is the PR ready to be merged, or needs other features ?

[asmit27rai]

Can you answer to this questions ?

1. Are there plans to add more comprehensive hole punching tests (actual NAT traversal, not just relayed connections)?
2. Are there plans to integrate these interop tests into CI? (Currently, these tests are manual integration tests run via test_local.sh and are not automatically executed by the CI/CD pipeline, which only runs pytest-based tests in tests/interop.)
3. Are there plans to add interop tests with other libp2p implementations (js-libp2p, nim-libp2p, etc.)?
4. Is the PR ready to be merged, or needs other features ?

Will add interop tests with other libp2p in other PR.
For 1 and 2, i will try to implement it.
@acul71

[acul71]

Still valuable and mergeable (CLEAN, CI green). Please answer the open scope questions (NAT-depth vs relay-only) so we can finish review and land this for #733. Note: long-term cross-impl interop preference is unified-testing + root interop/.