net-tokio: add `fn socks5_connect_outbound` #4305

tankyleo · 2026-01-12T08:29:32Z

No description provided.

ldk-reviews-bot · 2026-01-12T08:29:35Z

👋 Thanks for assigning @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.

tnull · 2026-01-12T08:52:51Z

lightning-net-tokio/src/lib.rs

+		SocketAddress::OnionV2 { .. } => unreachable!(),
+	});
+
+	match addr {


nit: Might be a bit cleaner if we do one match addr and then push both the type and the following fields accordingly? If we do that, we could also avoid the unreachable!s below, the early-return if above, and simply return an error for OnionV2?

tnull · 2026-01-12T09:00:00Z

lightning-net-tokio/src/lib.rs

+
+	tcp_stream.write_all(&socks5_request).await.map_err(|_| ())?;
+
+	let mut buffer: Vec<u8> = Vec::with_capacity(SOCKS5_REPLY_MIN_LEN);


This will probably immediately re-allocate since the reply should be larger than min reply len in the common case if we read_to_end, no? Should we rather pick a reasonable bufsize for this? Might also make sense to only read the 3 first bytes we look at in the first place?

This will probably immediately re-allocate since the reply should be larger than min reply len in the common case if we read_to_end, no?

If we take the common case to be Tor, this reply will always be 10 bytes.

Should we rather pick a reasonable bufsize for this?

I have nonetheless set this to the max possible reply given my reading of the RFC spec.

Might also make sense to only read the 3 first bytes we look at in the first place?

Want to make sure any bytes after the first 3 get discarded. Didn't find a better solution other than just reading them into the buffer and ignoring them.

I have nonetheless set this to the max possible reply given my reading of the RFC spec.

I'm confused, you're saying SOCKS5_REPLY_MIN_LEN is the maximum reply length? Edit: nevermind only now saw that you actually changed the code to include SOCKS5_REQUEST_REPLY_MAX_LEN.

Want to make sure any bytes after the first 3 get discarded. Didn't find a better solution other than just reading them into the buffer and ignoring them.

Hmm, if we're positive this ~always 10 bytes, why not actually discard 10 bytes (or read the fields and keep discarding until we've seen all expected fields)? FWIW, seems we could def. avoid the allocations at all if we make buffer a small fixed-size byte array?

I'm confused, you're saying SOCKS5_REPLY_MIN_LEN is the maximum reply length?

I changed the length of the buffer we preallocate to the max length of the reply given my reading of the RFC spec.

Hmm, if we're positive this ~always 10 bytes, why not actually discard 10 bytes (or read the fields and keep discarding until we've seen all expected fields)? FWIW, seems we could def. avoid the allocations at all if we make buffer a small fixed-size byte array?

I want to avoid being too tor-specific here. Another socks5 setup could definitely return more bytes in this response, up to 262 SOCKS5_REQUEST_REPLY_MAX_LEN.

Would you rather we use a [0u8; SOCKS5_REQUEST_REPLY_MAX_LEN] to avoid any heap allocations ? We could make it work yes, but seems to me Vec<u8> is good enough.

tnull · 2026-01-12T09:03:43Z

lightning-net-tokio/src/lib.rs

+	tcp_stream.write_all(&socks5_request).await.map_err(|_| ())?;
+
+	let mut buffer: Vec<u8> = Vec::with_capacity(SOCKS5_REPLY_MIN_LEN);
+	let n_read = tcp_stream.read_to_end(&mut buffer).await.map_err(|_| ())?;


Here we read what's available from the stream and then discard the buffer. Are we positive that this would never hold data we actually want to use and bubble up to handlers? As mentioned above, maybe we should just read 3 bytes rather than read_to_end?

Are we positive that this would never hold data we actually want to use and bubble up to handlers?

In the common case where we talk to Tor, any bytes after this will always be set to ATYP IPV4 and then ADDR 0.0.0.0 and PORT 0. None of this information is useful to callers, so I ignore it. Happy to update this in the future should this information be needed by callers in other SOCKS5 setups.

As mentioned above, maybe we should just read 3 bytes rather than read_to_end?

See above, I want to make sure any bytes after the first 3 get properly discarded, and not read by future calls to read.

In the common case where we talk to Tor, any bytes after this will always be set to ATYP IPV4 and then ADDR 0.0.0.0 and PORT 0. None of this information is useful to callers, so I ignore it. Happy to update this in the future should this information be needed by callers in other SOCKS5 setups.

Hmm, if we expect these fields exactly, why not then read the expected number of bytes only, and error out if there's anything unexpected? I'm still wondering if it would be ensured we'd never be able to read actual payload on this first read after connection establishment. FWIW, that kinda violates TCP's streaming model, no?

why not then read the expected number of bytes only, and error out if there's anything unexpected?

As mentioned above, I want to match the socks5 spec, and not constrain the use case to the tor proxy only.

I'm still wondering if it would be ensured we'd never be able to read actual payload on this first read after connection establishment. FWIW, that kinda violates TCP's streaming model, no?

I don't follow this one, can you clarify ?

I don't follow this one, can you clarify ?

TCP is a stream-based protocol. If you 'read to end' that only means you read what's currently there to your buffer. That might be the full response, only a part of it, or even the response followed by the some bytes of following packets. See for example Tor's 'optimistic data' mode: given that they allow to pipeline sending requests even when the connection attempt is still inflight could potentially mean that the first response bytes are already queued when we get to read_to_end, so we might end up discarding some valid goodput? It might actually be unlikely, but such races happen, so it would be safer to read only what we need to the buffer?

This is a good point thank you ! See the fixup below, I now read the socks5 reply to the exact byte in case the payload follows immediately in the stream.

ldk-reviews-bot · 2026-01-12T09:04:22Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

codecov · 2026-01-12T10:02:17Z

Codecov Report

❌ Patch coverage is 0% with 104 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (c722443) to head (7fdaeb2).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-net-tokio/src/lib.rs	0.00%	104 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4305      +/-   ##
==========================================
- Coverage   86.59%   86.51%   -0.08%     
==========================================
  Files         158      158              
  Lines      102408   102511     +103     
  Branches   102408   102511     +103     
==========================================
+ Hits        88678    88688      +10     
- Misses      11309    11406      +97     
+ Partials     2421     2417       -4

Flag	Coverage Δ
fuzzing	`35.90% <ø> (-0.98%)`	⬇️
tests	`85.80% <0.00%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tankyleo · 2026-01-12T16:45:39Z

lightning-net-tokio/src/lib.rs

+	let mut buffer: Vec<u8> = Vec::with_capacity(SOCKS5_REQUEST_REPLY_MAX_LEN);
+	let n_read = tcp_stream.read_to_end(&mut buffer).await.map_err(|_| ())?;
+	if n_read < SOCKS5_REQUEST_REPLY_MIN_LEN
+		|| n_read > SOCKS5_REQUEST_REPLY_MAX_LEN


Maybe this is too strict ? Anything longer than this deviates from the spec, so I lean towards returning Err in that case.

IMO fine to error if it doesn't adhere to the spec.

tankyleo

Also aware we have no tests for this code now, would it be ok to add tests that try connections to actual endpoints on the internet ? ie torproject.org, some famous lightning node onion V3 address...

i have tested socks5_connect in a toy binary and everything works.

tnull · 2026-01-12T17:51:44Z

Also aware we have no tests for this code now, would it be ok to add tests that try connections to actual endpoints on the internet ? ie torproject.org, some famous lightning node onion V3 address...

i have tested socks5_connect in a toy binary and everything works.

Yeah, tests towards some known endpoints on the internet are notoriously flaky (especially in Github CI), however, they are better than no tests!

tnull · 2026-01-12T17:55:27Z

lightning-net-tokio/src/lib.rs

+
+	tcp_stream.write_all(&socks5_request).await.map_err(|_| ())?;
+
+	let mut buffer: Vec<u8> = Vec::with_capacity(SOCKS5_REQUEST_REPLY_MAX_LEN);


As mentioned in the other comment: now that we have SOCKS5_REQUEST_REPLY_MAX_LEN, coudl we make this a [u8; SOCKS5_REQUEST_REPLY_MAX_LEN]?

tankyleo · 2026-01-12T18:48:39Z

Yeah, tests towards some known endpoints on the internet are notoriously flaky (especially in Github CI), however, they are better than no tests!

Let me see that would require installing C tor in CI right ? Do we want to do this still ?

tnull · 2026-01-12T19:53:54Z

Yeah, tests towards some known endpoints on the internet are notoriously flaky (especially in Github CI), however, they are better than no tests!

Let me see that would require installing C tor in CI right ? Do we want to do this still ?

Hmm, it would likely not be too bad, though indeed it might have some overhead if run every time. It would be a good candidate for a nightly CI job, IMO. For now it might be fine to add some tests, if they all reuse the same setup?

tnull · 2026-01-12T20:05:16Z

lightning-net-tokio/src/lib.rs

+	// Constants defined in RFC 1928
+	const VERSION: u8 = 5;
+	const NMETHODS: u8 = 1;
+	const NO_AUTH: u8 = 0;


While NO_AUTH is fine, we should still support setting USERNAME/PASSWORD fields as Tor uses that to let users establish new circuits. FWIW, we might even want to allow users to manually change username/password during runtime for following connections for that reason.

(see https://spec.torproject.org/socks-extensions.html#extent-of-support)

- remove heap allocations - read socks5 reply to the exact byte in case the payload follows immediately - add user-password authentication - add tests

TheBlueMatt · 2026-01-13T11:53:52Z

lightning-net-tokio/src/lib.rs

+	let method_selection_request = [VERSION, NMETHODS, selected_auth];
+	let mut tcp_stream = TcpStream::connect(&socks5_proxy_addr).await.map_err(|_| ())?;
+	tcp_stream.write_all(&method_selection_request).await.map_err(|_| ())?;
+
+	let mut method_selection_reply = [0u8; METHOD_SELECT_REPLY_LEN];
+	let n_read = tcp_stream.read_exact(&mut method_selection_reply).await.map_err(|_| ())?;
+	if n_read != METHOD_SELECT_REPLY_LEN || method_selection_reply != [VERSION, selected_auth] {
+		return Err(());
+	}


Wat? No, just send the CONNECT command and be done with it. We don't need the vast majority of this code.

Send something like this - https://github.com/rust-bitcoin/corepc/blob/master/bitreq/src/proxy.rs#L104 and then just check the status code response https://github.com/rust-bitcoin/corepc/blob/master/bitreq/src/proxy.rs#L113

Discussed offline, HTTP CONNECT is gated behind the HTTPTunnelPort setting, itself not enabled by default in C Tor, so we go with SOCKS5.

tankyleo force-pushed the 26-01-socks5-outbound branch from a7f498c to 42cbfd4 Compare January 12, 2026 08:36

tankyleo requested a review from tnull January 12, 2026 08:36

tnull reviewed Jan 12, 2026

View reviewed changes

net-tokio: add fn socks5_connect_outbound

e5f6359

tankyleo force-pushed the 26-01-socks5-outbound branch from 42cbfd4 to e5f6359 Compare January 12, 2026 16:37

tankyleo requested a review from tnull January 12, 2026 16:43

tankyleo commented Jan 12, 2026

View reviewed changes

tnull reviewed Jan 12, 2026

View reviewed changes

tankyleo force-pushed the 26-01-socks5-outbound branch from e9a864d to 8d22deb Compare January 13, 2026 03:58

fixup

7fdaeb2

- remove heap allocations - read socks5 reply to the exact byte in case the payload follows immediately - add user-password authentication - add tests

tankyleo force-pushed the 26-01-socks5-outbound branch from 8d22deb to 7fdaeb2 Compare January 13, 2026 05:20

tankyleo requested a review from tnull January 13, 2026 06:41

TheBlueMatt reviewed Jan 13, 2026

View reviewed changes


		tcp_stream.write_all(&socks5_request).await.map_err(\|_\| ())?;

		let mut buffer: Vec<u8> = Vec::with_capacity(SOCKS5_REPLY_MIN_LEN);

net-tokio: add fn socks5_connect_outbound #4305

Are you sure you want to change the base?

net-tokio: add fn socks5_connect_outbound #4305

Conversation

tankyleo commented Jan 12, 2026

Uh oh!

ldk-reviews-bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jan 12, 2026

Uh oh!

codecov bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Jan 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Jan 12, 2026

Uh oh!

tnull commented Jan 12, 2026

Uh oh!

tnull Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

net-tokio: add `fn socks5_connect_outbound` #4305

net-tokio: add `fn socks5_connect_outbound` #4305

ldk-reviews-bot commented Jan 12, 2026 •

edited

Loading

tnull Jan 12, 2026 •

edited

Loading

tnull Jan 12, 2026 •

edited

Loading

tnull Jan 12, 2026 •

edited

Loading

codecov bot commented Jan 12, 2026 •

edited

Loading

tankyleo left a comment •

edited

Loading

tnull Jan 12, 2026 •

edited

Loading