Skip to content

Conversation

@rorp
Copy link
Contributor

@rorp rorp commented Apr 1, 2025

Fixes #3051

@t-bast
Copy link
Member

t-bast commented Apr 1, 2025

At first glance, this seems to be incorrect: it will incorrectly use Tor in some configurations. But I'll need to spend more time looking at this again and how we configure it to be sure.

@sstone does that ring a bell? I'm pretty sure we had these distinct cases for a good reason.

@sstone
Copy link
Member

sstone commented Apr 1, 2025

The proposed change do not check useForIPv4 and useForIPv6 anymore which does seem wrong. I commented on the linked issue which describes 2 different problems (the first one being the expected behaviour when the socks5 proxy is configured to handle all connections).

@rorp
Copy link
Contributor Author

rorp commented Apr 2, 2025

At first glance, this seems to be incorrect

It appears to be a logical approach. When outgoing Tor connections are disabled, we are effectively unable to connect to Tor addresses, so we default to selecting only clearnet addresses.

Conversely, if Tor connections are enabled, we consider all available addresses - both Tor and clearnet - and randomly select one. Client actor then establishes a connection to the chosen address via either Tor or clearnet, depending on the configuration settings.

@rorp
Copy link
Contributor Author

rorp commented Apr 2, 2025

It can be incorrect if use-for-tor is set to false. In this case, even if Tor is enabled, it will attempt to connect to Tor addresses via clearnet, which doesn't make sense at all.

I think we can remove use-for-tor configuration parameter.

@rorp rorp force-pushed the fix_reconnection_to_clearnet_addresses_via_tor branch from 6ad6c83 to 9aaffa2 Compare April 3, 2025 16:51
@rorp
Copy link
Contributor Author

rorp commented Apr 3, 2025

It can be incorrect if use-for-tor is set to false. In this case, even if Tor is enabled, it will attempt to connect to Tor addresses via clearnet, which doesn't make sense at all.

I added a handler for this unlikely case, too.

@rorp rorp force-pushed the fix_reconnection_to_clearnet_addresses_via_tor branch from 9aaffa2 to add2616 Compare April 4, 2025 12:28
@t-bast
Copy link
Member

t-bast commented May 12, 2025

@sstone can you take another look at this?

@sstone sstone self-assigned this May 12, 2025
@rorp rorp force-pushed the fix_reconnection_to_clearnet_addresses_via_tor branch from add2616 to 1f35971 Compare July 2, 2025 16:05
@rorp
Copy link
Contributor Author

rorp commented Oct 10, 2025

Any updates about this fix? I had to manually patch the second major release to have a usable Eclair version...

@t-bast
Copy link
Member

t-bast commented Oct 10, 2025

@sstone will look into it shortly, we've been busy with taproot work but should have time to finalize this next week. Sorry for the delay.

Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

I think your reasoning makes sense but I added a comment because it was not really obvious: the decision to use a proxy or not is made later when we try to connect, so we should only filter out addresses that we know we won't able to connect to, and it's only true for tor addresses when we don't have a proxy with useForTor set.

I checked our documentation and it does not need to be updated.

sstone
sstone previously approved these changes Oct 14, 2025
@t-bast t-bast merged commit 701f229 into ACINQ:master Oct 27, 2025
1 check passed
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.

Issue with Tor configuration causing unnecessary force closures

4 participants