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]

I've assigned @tnull 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]

Hmm, I do wonder if we want to expose this right now, as it could be misleading given that all other connections (like Esplora/Eleectrum syncing, RGS updates, etc) would still go over clearnet? Maybe we should make this clear in the TorConfig docs more generally, and only expose this flag once we're positive we can switch to tunnel everything over Tor?

[jharveyb]

Mm, good point - and incoming traffic from other nodes would also still be over clearnet.

I'll remove the flag + make the comments for TorConfig more explicit + remove the flag. It can also be a follow-up if we really want it.

[tnull]

Let's just have this take a KeysManager reference and rederive RandomBytes when needed?

[jharveyb]

Sure - is there a reason that isn't done for PeerManager? out of curiousity

[tnull]

Please just move this to the exports in the beginning of src/ffi/types.rs IIUC that should suffice to make uniffi happy.