Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Tor embedded library and use onion addresses #662

Merged
merged 20 commits into from
Feb 11, 2025
Merged

Conversation

dpad85
Copy link
Member

@dpad85 dpad85 commented Dec 12, 2024

This PR removes the tor-mobile-kmp library embedded inside Phoenix. Access to Tor is not done by Phoenix anymore, instead, when Tor is enabled in the application, Phoenix will exclusively use onion addresses when connecting to the Lightning peer and to Electrum.

Access to the Tor network is delegated to a Tor proxy application that needs to be installed by the user (such as Orbot).

This provides several improvements:

  • Receiving payments in the background with Tor enabled will be more reliable, because the Tor proxy app will be able to run continuously (since it's a VPN application enjoying specific privileges).
  • Using onion addresses ensures that the app cannot connect if Tor is unavailable for some reason.
  • The previous Tor option only covered Lightning and Electrum connections, but not HTTP ones (which include API calls for feerates or btc/fiat rates).

Besides, the tor-mobile-kmp library was difficult to maintain. For example, it did not contain a recent Tor binary. Orbot will do a much better job at that. Our library also caused issues with the Kotlin 2.0 update, and with M3 chips.

robbiehanson and others added 6 commits February 6, 2025 11:00
The Tor proxy status UI has been removed. Phoenix now controls
that the Electrum server is an onion address, or shows a message
otherwise. The "enable tor" switch and the confirmation dialog
have been updated.

Some warning about background payments have been amended or
removed since using a Tor VPN app should be more reliable.

Translation have been removed and need to be fixed.
When tor is enabled, Phoenix will require electrum servers to use an
onion address. This way, if the Tor VPN fails, connection will not
be established. However, some users may want to opt-out, which is
valid if they are connecting to their own server and don't mind
connecting to it on clearnet.
@dpad85 dpad85 force-pushed the tor-onion-service branch from 555a7c9 to 08f7882 Compare February 6, 2025 14:30
@dpad85 dpad85 mentioned this pull request Feb 10, 2025
@robbiehanson
Copy link
Contributor

robbiehanson commented Feb 11, 2025

iOS screenshots:

New configuration screens:

Notifying users of invalid Electrum configuration:

Modifying Electrum configuration:

When upgrading to the new version, IF Tor is enabled, we display this notice to the user:

@robbiehanson robbiehanson marked this pull request as ready for review February 11, 2025 16:27
this fixes an issue where peer connection could hang when the
handshake timed out. The connectionLoop job would not detect
the state change.
@dpad85
Copy link
Member Author

dpad85 commented Feb 11, 2025

Note that (on Android at least) we need to let users access the Settings screen from the initialisation screen, so they can activate Tor/set Electrum before they actually create or restore their wallet. But we'll do that in another PR, since this one is blocking other important PRs.

@dpad85 dpad85 requested a review from robbiehanson February 11, 2025 18:14
@dpad85 dpad85 merged commit 6be7437 into master Feb 11, 2025
@dpad85 dpad85 deleted the tor-onion-service branch February 11, 2025 18:38
@05nelsonm
Copy link

This breaks my heart.

I maintain the kmp-tor library and would love to see this feature reintegrated into Phoenix. Orbot has a plethora of issues and is unreliable.

@MasterixCZ
Copy link
Contributor

What about Arti? It's official Tor reimplementation in Rust, which aims to be more modular, safe and so on. It's already in version 1.4.0 (but it still isn't 100% alternative of Tor C implementation though). That would be an interesting alternative. Most user's probably will not want to keep starting and shutting off Orbot (for example, if I'm using VPN, I can't use Orbot, because there can be only 1 VPN running on Android, so I need to keep switching between them ans that's annoying. With the current Tor implementation in Phoenix I don't need to do that).

@dpad85
Copy link
Member Author

dpad85 commented Feb 20, 2025

There are indeed other good options which could replace our library (although note that Arti is officially not ready for production). But embedding Tor directly in the app inherently has drawbacks.

The issue is that Lightning is an interactive protocol. You need to respond to payment messages, and do so quickly.

But when the app is in the background or stopped (which is often), the OS gives you a limited budget to process notifications in a timely manner (and they tend to shrink that budget in each new version). Having to first start Tor and connect before processing a payment is too much; it means that in most cases, background payments will fail.

Delegating the Tor connection to Orbot not only removes "work" from the Phoenix app, it is also persistent and more reliable because it is treated as a VPN connection.

@05nelsonm
Copy link

05nelsonm commented Feb 20, 2025

Yeah I hear ya on the timeliness. It's completely understandable as it relates to the function of the app.

Unfortunately, I do not, nor shall I ever, use Orbot as a VPN because you can only run 1 on mobile (among other reasons). What are your thoughts on adding the ability to define a SOCKS5 port for tor connectivity? Otherwise, Phoenix users MUST use a VPN app that forces traffic over tor (currently, only offered by 1 app, Orbot, AFAIK).

EDIT: Also, regarding removal of "work" from Phoenix developers. Yes, tor functionality is pretty crazy to implement in a reliable, sound manner. Maintaining that is also trying. And this is exactly why I created kmp-tor. Neither The Tor Project or Guardian Project have reliable, production ready solutions, but I wanted to use apps over tor to connect back to my own hardware. So, work wise, you have very little to do. I put a lot into 2.0.0 to make it plug & play. It just works.

pm47 added a commit to ACINQ/lightning-kmp that referenced this pull request Feb 24, 2025
We instantiated a new `SelectorManager` at each connection attempt, leaking resources even if each socket was properly cleaned up on disconnection. The leak affects both jvm and native.

Also removes the `startTls` function that is not used anymore since we moved Tor outside phoenix (ACINQ/phoenix#662).
@CaverneCrypto
Copy link

I totally agree with both @05nelsonm and @MasterixCZ . This is very not UX friendly. Users do not want to switch app to enable Orbot each time they want to process a payment.

Yes, the previous implementation was long to start and to connect to tor but I worked (without any click of the user)!

In my opinion, remove tor from phoenix is not going in the right direction for the privacy of user, because they will now just not enable tor on their wallet...

I already have some complain about this from friend who use Phoenix often.

@eup4
Copy link

eup4 commented Mar 12, 2025

First of all, thanks for developing Phoenix. It makes a difference 💟

I just went here because of removal of this feature. As others mentioned, on mobile it's not possible to keep Orbot when running other VPN.

Looking at Moon feature requests (muun/apollo#21 (comment)), people are requesting built-in Tor support like in Phoenix:

Before setting up the wallet, you'd be able to enable Tor, and it would work from within the wallet without needing a third party app like Orbot. (Phoenix wallet works this way).

So this really was a competitive advantage over others. For instance, Trezor Suite has similar useful built-in Tor support.

Would be nice to have it reverted or re-implemented. Thanks!

P.S. I couldn't find a donation link on the website. Consider it as a feature request too.

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.

6 participants