Add Tor support for outbound connections via SOCKS

[jharveyb]

Addresses #178 . Builds on lightningdevkit/rust-lightning#4305 .

As mentioned there, I see two drawbacks with this patch as is:

• We record the peer remote address as the proxy address, not the actual onionv3 of that peer:

https://github.com/lightningdevkit/rust-lightning/blob/f9ad3450b7d8b722b440f0a5e3d9be8bd7a696ae/lightning-net-tokio/src/lib.rs#L332

Which affects list_peers() output:

{
			"pubkey": {
				"pubkey": "03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f"
			},
			"socket_addr": {
				"address": "3.33.236.230:9735"
			},
			"is_connected": true
		},
		{
			"pubkey": {
				"pubkey": "03fe47fdfea0f25fad0013498e8d6cec348ae3d673841ec25ee94f87c21af16ed8"
			},
			"socket_addr": {
				"address": "127.0.0.1:9050"
			},
			"is_connected": true
		}

But more significantly, AFAICT affects our setup messages with our peer:

https://github.com/lightningdevkit/rust-lightning/blob/f9ad3450b7d8b722b440f0a5e3d9be8bd7a696ae/lightning/src/ln/peer_handler.rs#L1905

The filter_addresses() call here means we won't include the proxy address, but we also won't include that peer's onion address, which we may want to do?

I'm testing this actively now and haven't hit any issues yet, though I haven't had much traffic with that peer either.

[ldk-reviews-bot]

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tankyleo]

Thanks for the PR !

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tankyleo]

@jharveyb Thanks for pointing out these quirks. From my reading, we are still onside, so I would expect everything to work on on the current main branch.

For the outbound connection case, we could add an Option<SocketAddr> parameter to fn setup_outbound in lightning-net-tokio, and we would use that parameter in fn tor_connect_outbound to override the default remote_addr with the onion V3 address we are connecting to.

While we don't have native support for inbound tor connections yet, list_peers will always display some 127.0.0.1:xxxxx for those connections.

[tankyleo]

this is the patch I have in mind @tnull curious your thoughts.

diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs
index eec0e424e..104fd8595 100644
--- a/lightning-net-tokio/src/lib.rs
+++ b/lightning-net-tokio/src/lib.rs
@@ -378,12 +378,12 @@ where
 /// futures are freed, though, because all processing futures are spawned with tokio::spawn, you do
 /// not need to poll the provided future in order to make progress.
 pub fn setup_outbound<PM: Deref + 'static + Send + Sync + Clone>(
-       peer_manager: PM, their_node_id: PublicKey, stream: StdTcpStream,
+       peer_manager: PM, their_node_id: PublicKey, stream: StdTcpStream, remote_addr_override: Option<SocketAddress>,
 ) -> impl std::future::Future<Output = ()>
 where
        PM::Target: APeerManager<Descriptor = SocketDescriptor>,
 {
-       let remote_addr = get_addr_from_stream(&stream);
+       let remote_addr = remote_addr_override.or(get_addr_from_stream(&stream));
        let (reader, mut write_receiver, read_receiver, us) = Connection::new(stream);
        #[cfg(test)]
        let last_us = Arc::clone(&us);
@@ -469,7 +469,7 @@ where
        if let Ok(Ok(stream)) =
                time::timeout(Duration::from_secs(CONNECT_OUTBOUND_TIMEOUT), connect_fut).await
        {
-               Some(setup_outbound(peer_manager, their_node_id, stream))
+               Some(setup_outbound(peer_manager, their_node_id, stream, None))
        } else {
                None
        }
@@ -488,12 +488,12 @@ where
        PM::Target: APeerManager<Descriptor = SocketDescriptor>,
 {
        let connect_fut = async {
-               tor_connect(addr, tor_proxy_addr, entropy_source).await.map(|s| s.into_std().unwrap())
+               tor_connect(addr.clone(), tor_proxy_addr, entropy_source).await.map(|s| s.into_std().unwrap())
        };
        if let Ok(Ok(stream)) =
                time::timeout(Duration::from_secs(TOR_CONNECT_OUTBOUND_TIMEOUT), connect_fut).await
        {
-               Some(setup_outbound(peer_manager, their_node_id, stream))
+               Some(setup_outbound(peer_manager, their_node_id, stream, Some(addr)))
        } else {
                None
        }
@@ -1015,7 +1015,7 @@ mod tests {
                // 127.0.0.1.
                let (conn_a, conn_b) = make_tcp_connection();

-               let fut_a = super::setup_outbound(Arc::clone(&a_manager), b_pub, conn_a);
+               let fut_a = super::setup_outbound(Arc::clone(&a_manager), b_pub, conn_a, None);
                let fut_b = super::setup_inbound(b_manager, conn_b);

                tokio::time::timeout(Duration::from_secs(10), a_connected.recv()).await.unwrap();
@@ -1085,7 +1085,7 @@ mod tests {
                // Call connection setup inside new tokio tasks.
                let manager_reference = Arc::clone(&a_manager);
                tokio::spawn(async move { super::setup_inbound(manager_reference, conn_a).await });
-               tokio::spawn(async move { super::setup_outbound(a_manager, b_pub, conn_b).await });
+               tokio::spawn(async move { super::setup_outbound(a_manager, b_pub, conn_b, None).await });
        }

        #[tokio::test(flavor = "multi_thread")]

[tnull]

Cool, thanks for taking a look!

Already looks pretty good, but here are a few comments after the first pass. Do you think there is a good way to add test coverage for this to CI?

[tnull]

this is the patch I have in mind @tnull curious your thoughts.

Hmm, could make sense, but maybe we should only expose the override field on the Tor-specific API?

[tankyleo]

Hmm, could make sense, but maybe we should only expose the override field on the Tor-specific API?

The general setup_outbound function currently sets the remote_addr field with get_addr_from_stream, so we have to make a modification to the general API unfortunately.

[tankyleo]

@jharveyb upstream PR merged here lightningdevkit/rust-lightning#4372 should fix the issues you raised above

[jharveyb]

@jharveyb upstream PR merged here lightningdevkit/rust-lightning#4372 should fix the issues you raised above

Thanks for that! Updated here, added the TorConfig struct, and updated bindings. I haven't tested the proxy_all_outbound behavior live yet.

[tnull]

Maybe we should move this check to the build step so it happens also when set via Builder::from_config? Not sure if we even need this setter then?

[jharveyb]

I'm fine with also having the check there, though I'll note that there are checks in other setters, like set_announcement_addresses() and set_listening_addresses(), that aren't mirrored in build_with_store_internal() right now.

I'd prefer to keep the setter (since I'm using the others as well), but can remove it if needed.

[tnull]

Basically looks good, pinged @tankyleo to see if he has any comments.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

Unfortunately also needs a rebase now.

[tankyleo]

LGTM, just would like to make even clearer to people a tor config does not mean we route IP connections via tor.

[tankyleo]

I think it would be worth underscoring again here that setting this proxy only enables connecting to Onion V3 addresses; it is not used to connect to clearnet addresses.

[jharveyb]

Thanks, forgot to update comments after dropping the logic for clearnet addresses.

[tankyleo]

Likewise here, maybe something along "...to use a Tor SOCKS proxy to connect to Onion V3 addresses"?

[jharveyb]

Using the same comment for the setter in both places; lmk if you think I should revise further!

[tankyleo]

"Setting [TorConfig] only enables connecting to OnionV3 peers, it does not route IP connections via the tor network" ?

[jharveyb]

Rebased + updated docs.