Skip to content

Conversation

@jvsena42
Copy link
Member

@jvsena42 jvsena42 commented Dec 19, 2025

Fixes the "Invalid socket address" error that occurs when connecting to trusted Lightning peers loaded from Blocktank info.

Description

The ILspNode.toPeerDetails() extension function was assuming that the connectionStrings field contained only host:port format (e.g., "34.65.86.104:9400"), but the actual Blocktank API returns the full connection URI in the format nodeId@host:port (e.g., "028a8910...@34.65.86.104:9400").

When this full connection string was passed directly to LDK's node.connect() function as the address parameter, it was rejected with "Invalid socket address" because LDK expects only the socket address portion (host:port).

Changes:

  • Modified toPeerDetails() to parse connection strings and extract just the host:port portion when the full URI format is present
  • The function now handles both formats correctly:
    • "host:port" → used as-is
    • "nodeId@host:port" → extracts "host:port" portion
  • Added comprehensive unit tests (PeerDetailsTest.kt) with 16 test cases covering all extension functions and edge cases

Root Cause:
This issue appeared after PR #536 which introduced loading trusted peers from Blocktank info. Previously, peers were loaded from Env.trustedLnPeers which had the correct format. The new code path exposed the format mismatch.

Preview

Uploading Screen_recording_20251219_132825.webm…

QA Notes

Tests Added:

  • ✅ 16 unit tests in PeerDetailsTest.kt all passing
  • ✅ Tests cover both connection string formats, edge cases, and all extension functions
  • ✅ Detekt linter passes

Manual Testing:

  1. Start the app and verify it connects to trusted peers without "Invalid socket address" errors
  2. Check logs for successful peer connections (should show ✓ Connected to trusted peer: <nodeId>)
  3. Verify no "Peer connect error" logs with LDK "Invalid socket address" errors
  4. Run unit tests: ./gradlew testDevDebugUnitTest --tests PeerDetailsTest

Regression Testing:

  • Existing peer connection functionality remains unchanged
  • All other PeerDetails extension functions (parse, from, host, port, uri) continue to work correctly

@jvsena42 jvsena42 self-assigned this Dec 19, 2025
@claude
Copy link

claude bot commented Dec 19, 2025

PR Review

Summary

This PR adds handling for connection strings in the full URI format (nodeId@host:port) in addition to the existing host:port format. The change is minimal and well-tested.

✅ Strengths

  1. Excellent test coverage - 276 lines of comprehensive tests covering:

    • Both connection string formats (host:port and nodeId@host:port)
    • Edge cases (empty lists, IPv4, IPv6, multiple connection strings)
    • All extension functions and helper methods
    • Error conditions
  2. Minimal code change - The fix is simple and focused, extracting the address portion when the full URI format is provided

  3. Backward compatible - Existing behavior for host:port format is preserved

  4. Follows project conventions:

    • Uses BaseUnitTest pattern
    • Proper test naming with backticks
    • Uses kotlin.test assertions

🐛 Potential Issues

Critical: IPv6 Address Parsing Bug

There's a potential bug with IPv6 addresses in full URI format. The test at line 168-181 tests IPv6 with just host:port format, but what happens with a full URI like nodeId@[2001:db8::1]:9735?

The current implementation uses substringAfter("@") which would correctly extract [2001:db8::1]:9735. However, the host and port extension functions (lines 6-8 in PeerDetails.kt) use simple string splitting which will break for IPv6:

For IPv6 address [2001:db8::1]:9735:

  • host would return [2001 (incorrect - should be [2001:db8::1])
  • port would return db8::1]:9735 (incorrect - should be 9735)

Recommendation: Add a test case for the full URI format with IPv6 to verify the host and port extensions work correctly. If they fail, those extensions need to be updated to handle IPv6 brackets properly.

📝 Minor Observations

  1. Missing PR description - The PR body is empty. Consider adding:

    • Description of the issue this fixes
    • Link to related issue/bug report
    • Preview/QA notes as per the template
  2. Test organization - The tests are well-structured and comprehensive

  3. Code comment clarity - The comment at line 43-44 is helpful. Good practice!

🔒 Security

No security concerns identified. The parsing logic is straightforward and doesn't introduce vulnerabilities.

⚡ Performance

No performance concerns. The string operations are simple and efficient.

📋 Next Steps

  1. Before merging: Add test case for IPv6 with full URI format and verify host/port extensions work correctly
  2. Fill in the PR description with context about what issue this fixes
  3. Verify tests pass with the gradle commands from CLAUDE.md
  4. Run linter to check for any issues

Verdict

Good fix with excellent test coverage! 🎯 Just need to verify the IPv6 edge case before merging.

@jvsena42 jvsena42 marked this pull request as ready for review December 19, 2025 15:26
@jvsena42
Copy link
Member Author

Unit tests
https://github.com/synonymdev/bitkit-android/actions/runs/20374603342

E2E
https://github.com/synonymdev/bitkit-android/actions/runs/20374624194

@jvsena42
Copy link
Member Author

About Claude comment, there is a Unit test for IPV6

@piotr-iohk
Copy link
Collaborator

👏

Tests Added:

✅ 16 unit tests in PeerDetailsTest.kt all passing
✅ Tests cover both connection string formats, edge cases, and all extension functions

@ovitrif ovitrif mentioned this pull request Dec 19, 2025
@jvsena42 jvsena42 enabled auto-merge December 19, 2025 16:39
Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tAck

Tests

  • new wallet > Lightning Node peers 🟢

@jvsena42 jvsena42 merged commit 7093157 into master Dec 19, 2025
18 checks passed
@jvsena42 jvsena42 deleted the fix/peer-unreachable branch December 19, 2025 16:40
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.

4 participants