Add remote_addr field to lightning_net_tokio::setup_outbound#4372
Add remote_addr field to lightning_net_tokio::setup_outbound#4372tankyleo wants to merge 2 commits intolightningdevkit:mainfrom
remote_addr field to lightning_net_tokio::setup_outbound#4372Conversation
tankyleo
commented
Feb 2, 2026
|
👋 Thanks for assigning @tnull as a reviewer! |
|
Fixes the issues @jharveyb reported in lightningdevkit/ldk-node#778 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4372 +/- ##
==========================================
+ Coverage 85.98% 86.02% +0.03%
==========================================
Files 156 156
Lines 102641 103012 +371
Branches 102641 103012 +371
==========================================
+ Hits 88258 88615 +357
- Misses 11873 11889 +16
+ Partials 2510 2508 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tnull
left a comment
There was a problem hiding this comment.
Generally makes sense. One question, otherwise CI seems unhappy right now.
| /// [`PeerManager::list_peers`]: lightning::ln::peer_handler::PeerManager::list_peers | ||
| pub fn setup_outbound<PM: Deref + 'static + Send + Sync + Clone>( | ||
| peer_manager: PM, their_node_id: PublicKey, stream: StdTcpStream, | ||
| remote_addr: Option<SocketAddress>, |
There was a problem hiding this comment.
Hmm, rather than adding this on the generic setup_outbound, would it make sense to refactor this to be an setup_outbound_internal and offer two setup_outbound and setup_outbound_tor variants that do the right (tm) thing for the user? Or, put differently, for non-Tor users this remote_addr argument might be confusing?
There was a problem hiding this comment.
Let me know how this sounds below tnull, I avoid adding a setup_outbound_tor, and call setup_outbound_internal directly from connect_outbound_tor. I'd prefer to avoid adding a new function unless we really have to.
TheBlueMatt
left a comment
There was a problem hiding this comment.
Agree with @tnull otherwise lgtm
b28e8a6 to
427b76a
Compare
When `setup_outbound` was used to setup a connection proxied over Tor, it previously set the remote address of the peer to the address of the Tor proxy. This address of the Tor proxy was assigned to the `PeerDetails::socket_address` for that peer in `PeerManager::list_peers`, and if it was not a private IPV4 or IPV6 address, it was also reported to the peer in our init message. This commit refactors `tor_connect_outbound` to pass its own peer address parameter directly to the connection setup code. This peer address will now appear in `PeerManager::list_peers` for outbound Tor connections made using `tor_connect_outbound`, and will be reported to the peer in our init message if it is not a private IPV4 or IPV6 address.
427b76a to
28cf5f5
Compare
tnull
left a comment
There was a problem hiding this comment.
Changes look good, but it would be great if we could add some kind of test coverage (maybe even based on the _internal method to check that the override works as expected?
| // so they are filtered out and not reported to the peer | ||
| remote_network_address: b_addr_override, | ||
| }; | ||
| a_logger.assert_log( |
There was a problem hiding this comment.
Hmm, asserting on log messages is often a bit error prone. Shouldn't we rather assert that the counterparty's PeerManager::list_peers reports the expected value?