Skip to content

Conversation

lla-dane
Copy link
Contributor

@lla-dane lla-dane commented Oct 7, 2025

Tracks: multiformats/multiaddr#181

Added SNI, NOISE, CERTHASH, WEBRTC, WEBRTC-DIRECT protocol in py-multiaddr in reference with go-multiaddr

SNI had the same codec configs as DNS in go-multiaddr

protoSNI = Protocol{
	Name:       "sni",
	Size:       LengthPrefixedVarSize,
	Code:       P_SNI,
	VCode:      CodeToVarint(P_SNI),
	Transcoder: TranscoderDns,
}

So just added this line in the protocol registry

Protocol(P_SNI, "sni", "domain"),

Did the same thing with NOISE

Added the CERTHASH protocol as per go implementation

var TranscoderCertHash = NewTranscoderFromFunctions(certHashStB, certHashBtS, validateCertHash)

func certHashStB(s string) ([]byte, error) {
	_, data, err := multibase.Decode(s)
	if err != nil {
		return nil, err
	}
	if _, err := mh.Decode(data); err != nil {
		return nil, err
	}
	return data, nil
}

func certHashBtS(b []byte) (string, error) {
	return multibase.Encode(multibase.Base64url, b)
}

func validateCertHash(b []byte) error {
	_, err := mh.Decode(b)
	return err
}

@lla-dane
Copy link
Contributor Author

lla-dane commented Oct 7, 2025

@acul71: Can you please take a look why these checks are failing with my changes, I am not understanding.

@lla-dane lla-dane changed the title Add sni protocol in py-multiaddr Add SNI and NOISE protocol in py-multiaddr Oct 7, 2025
- Added the protocol utils in certhash.py
- Added the internal logic test suite in test_protocols.py
- Added the certhash multiaddr test cases in test_multiaddr.py
@lla-dane
Copy link
Contributor Author

Added the certhash protcol, along with the test-suite

@lla-dane lla-dane changed the title Add SNI and NOISE protocol in py-multiaddr Add SNI, NOISE and CERTHASH protocol in py-multiaddr Oct 10, 2025
- Added the protocols in the registry in protcols.py
- Added the maddr test cases in test_multiaddr.py
@lla-dane lla-dane changed the title Add SNI, NOISE and CERTHASH protocol in py-multiaddr Add SNI, NOISE, CERTHASH, WEBRTC, WEBRTC-DIRECT in py-multiaddr Oct 11, 2025
@lla-dane
Copy link
Contributor Author

lla-dane commented Oct 11, 2025

Added webrtc and webrtc-direct in 4507543, along with maddr test cases in test_multiaddr.py

@lla-dane
Copy link
Contributor Author

@seetadev @acul71: Did some tweaks in aa2202d to resolve the CI checks, please take a look and see if anything else remains, other than make docs content addition in this PR.

Will add the doc content for all protocols in a separate PR.

- Fix multihash.encode() to multihash.digest().encode() in tests
- Rename newsfragment from 97.feature.rst to 181.feature.rst to match issue #181
- All tests now pass with correct multihash API usage
- Use multihash.encode() instead of multihash.digest().encode()
- This matches the py-multihash library API used in CI
- All tests now pass with correct multihash library version
@acul71
Copy link
Contributor

acul71 commented Oct 13, 2025

@lla-dane

PR #97 Review: Add SNI, NOISE, CERTHASH, WEBRTC, WEBRTC-DIRECT in py-multiaddr

Summary

This PR adds support for 5 new multiaddr protocols to py-multiaddr:

  • SNI (Server Name Indication)
  • NOISE (Noise Protocol Framework)
  • CERTHASH (Certificate Hash)
  • WEBRTC (WebRTC)
  • WEBRTC-DIRECT (WebRTC Direct)

The implementation follows the go-multiaddr reference implementation and includes comprehensive test coverage.

Detailed Review

✅ Protocol Codes Verification

All protocol codes match the go-multiaddr reference implementation:

Protocol Go Code Python Code Status
SNI 449 0x01C1 (449) ✅ Correct
NOISE 454 0x01C6 (454) ✅ Correct
CERTHASH 466 0x1D2 (466) ✅ Correct
WEBRTC_DIRECT 280 0x118 (280) ✅ Correct
WEBRTC 281 0x119 (281) ✅ Correct

✅ Protocol Configurations

Protocol configurations correctly match the go-multiaddr implementation:

Protocol Go Transcoder Python Codec Status
SNI TranscoderDns "domain" ✅ Correct
NOISE None None ✅ Correct
CERTHASH TranscoderCertHash "certhash" ✅ Correct
WEBRTC_DIRECT None None ✅ Correct
WEBRTC None None ✅ Correct

✅ CERTHASH Implementation

The CERTHASH codec implementation correctly matches the go-multiaddr transcoder:

Go Implementation:

func certHashStB(s string) ([]byte, error) {
    _, data, err := multibase.Decode(s)
    if err != nil {
        return nil, err
    }
    if _, err := mh.Decode(data); err != nil {
        return nil, err
    }
    return data, nil
}

func certHashBtS(b []byte) (string, error) {
    return multibase.Encode(multibase.Base64url, b)
}

func validateCertHash(b []byte) error {
    _, err := mh.Decode(b)
    return err
}

Python Implementation:

def to_bytes(self, proto: Any, string: str) -> bytes:
    decoded_bytes = multibase.decode(string)
    self.validate(decoded_bytes)
    return decoded_bytes

def to_string(self, proto: Any, buf: bytes) -> str:
    self.validate(buf)
    encoded_string = multibase.encode("base64url", buf)
    return encoded_string.decode("utf-8")

def validate(self, b: bytes) -> None:
    multihash.decode(b)

Analysis:

  • ✅ String-to-bytes conversion matches go implementation
  • ✅ Bytes-to-string conversion matches go implementation
  • ✅ Validation logic matches go implementation
  • ✅ Uses base64url encoding as specified in go implementation

✅ Test Coverage

Comprehensive test coverage has been added:

Multiaddr String Tests

# SNI test case
"/ip4/127.0.0.1/tcp/443/tls/sni/example.com/http/http-path/foo"

# NOISE test case  
"/ip4/127.0.0.1/tcp/127/noise"

# CERTHASH test cases
"/ip4/127.0.0.1/udp/1234/quic-v1/webtransport/certhash/b2uaraocy6yrdblb4sfptaddgimjmmpy"
"/ip4/127.0.0.1/udp/1234/quic-v1/webtransport/certhash/b2uaraocy6yrdblb4sfptaddgimjmmpy/certhash/zQmbWTwYGcmdyK9CYfNBcfs9nhZs17a6FQ4Y8oea278xx41"

# WEBRTC test cases
"/ip4/127.0.0.1/tcp/127/webrtc-direct"
"/ip4/127.0.0.1/tcp/127/webrtc"

CERTHASH Codec Tests

  • ✅ Valid roundtrip conversion
  • ✅ Invalid multihash bytes handling
  • ✅ Invalid multibase content handling
  • ✅ Invalid multibase string handling
  • ✅ Direct validation function testing

✅ Code Quality

  • Documentation: CERTHASH codec includes comprehensive docstrings
  • Error Handling: Proper exception handling with descriptive error messages
  • Type Hints: Consistent use of type hints throughout
  • Code Style: Follows existing codebase patterns

✅ Dependencies

The implementation correctly uses existing dependencies:

  • multibase for base64url encoding/decoding
  • multihash for multihash validation
  • No new external dependencies introduced

Issues Found

✅ Fixed Issues

  1. Multihash API Usage: The test was using multihash.encode() which is correct for the py-multihash library used in CI, but was initially using multihash.digest().encode() which is from a different library.

  2. Newsfragment Issue Number: The newsfragment was initially named 97.feature.rst (PR number) but was correctly renamed to 181.feature.rst to match issue #181.

⚠️ Minor Issues

  1. Test Function Naming: The test function test_certhash_memory_validate_function() has an incorrect name - it should be test_certhash_validate_function() since it's testing the certhash codec, not memory.

  2. Missing Documentation: The new protocols could benefit from documentation in the main module docstring or examples.

Recommendations

✅ Approve with Minor Changes

This PR is ready for approval with the following minor suggestions:

  1. Fix test function name: Rename test_certhash_memory_validate_function() to test_certhash_validate_function()

  2. Consider adding documentation: Add brief documentation for the new protocols in the main module or examples.

Note: The multihash API usage issue has been fixed during review.

Conclusion

This PR successfully implements all 5 new multiaddr protocols with:

  • ✅ Correct protocol codes matching go-multiaddr
  • ✅ Proper protocol configurations
  • ✅ Accurate CERTHASH implementation
  • ✅ Comprehensive test coverage
  • ✅ Good code quality

The implementation is faithful to the go-multiaddr reference implementation and maintains consistency with the existing py-multiaddr codebase.

Recommendation: APPROVE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants