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

feat!: use libtor to spawn in-process Tor for base node and console wallet #3641

Closed
wants to merge 3 commits into from

Conversation

delta1
Copy link
Contributor

@delta1 delta1 commented Dec 6, 2021

Description

  • base node and console wallet default to starting their own Tor instance
  • 2 adjacent ports are selected from given range and tested to be available (this is for control and socks ports, not for the onion port)
  • random password is generated and configured
  • data directory and logging file are temporary
  • new config option use_libtor for [base_node.network] and [wallet.network] defaults to true, when set to false allows Tor comms transport to be manually defined as before

Motivation and Context

Improved user experience - ease of getting started without requiring separate Tor

How Has This Been Tested?

Tested by setting up new base nodes and wallets and running them using libtor, and also tested that they still work using a system level Tor.
TODO: still to be tested on Windows

Breaking change: old config files might no longer work

…allet

BREAKING CHANGE: old config files *might* not work correctly
@delta1
Copy link
Contributor Author

delta1 commented Dec 6, 2021

Going to need an update for build base node binaries CI

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Awesome, very cool that tor can be embedded. Changing the address each time will cause connectivity issues that can be hard to debug (nodes not connecting when they are online, not noticing the address has changed)

common/src/tor.rs Outdated Show resolved Hide resolved
common/src/tor.rs Outdated Show resolved Hide resolved
applications/tari_console_wallet/src/main.rs Show resolved Hide resolved
common/src/tor.rs Show resolved Hide resolved
@delta1 delta1 added the P-do_not_merge Process - Not ready for merging label Dec 7, 2021
@delta1
Copy link
Contributor Author

delta1 commented Dec 7, 2021

Breaks the build on windows, looking for a solution

@delta1
Copy link
Contributor Author

delta1 commented Dec 7, 2021

Awesome, very cool that tor can be embedded. Changing the address each time will cause connectivity issues that can be hard to debug (nodes not connecting when they are online, not noticing the address has changed)

Just to clarify - the public address remains the same and is loaded from the node identity file, it is just the Tor control port and socks port that are checked for availability in the port range and selected.

@sdbondi
Copy link
Member

sdbondi commented Dec 8, 2021

it is just the Tor control port and socks port that are checked for availability in the port range and selected.

Ok cool, sounds good. Then downgraded to a nit, maybe Tor::randomize could be Tor::initialize and remove the port range.

Warning about port bind test (from experience), the port may stay bound after the test even if it's dropped on linux (unless SO_REUSEPORT is used subsequently). You can let the OS assign an open port by using port 0


// check for unused ports to assign
let (socks_port, control_port) = get_available_ports(port_range)?;
instance.socks_port = socks_port;
Copy link
Member

@sdbondi sdbondi Dec 8, 2021

Choose a reason for hiding this comment

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

Just remembered, if you set both of these to 0 the OS will assign an open port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case does the bind return the port number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I see I can use local_addr to get it from the listener. Will do

@delta1
Copy link
Contributor Author

delta1 commented Dec 8, 2021

it is just the Tor control port and socks port that are checked for availability in the port range and selected.

Ok cool, sounds good. Then downgraded to a nit, maybe Tor::randomize could be Tor::initialize and remove the port range.

Warning about port bind test (from experience), the port may stay bound after the test even if it's dropped on linux (unless SO_REUSEPORT is used subsequently). You can let the OS assign an open port by using port 0

Agreed sounds good

@stringhandler stringhandler changed the base branch from development to weatherwax December 10, 2021 10:36
@delta1 delta1 marked this pull request as draft January 3, 2022 07:18
@delta1
Copy link
Contributor Author

delta1 commented Jan 6, 2022

Windows compilation issues would either require building via cygwin/mingw/msys2, or maintaining a fork and migrating the tor source to cmake for windows, both of which are undesirable for this feature.

Closing this PR and will re-open as an opt-in feature for linux/macos only, unfortunately.

@delta1 delta1 closed this Jan 6, 2022
stringhandler pushed a commit that referenced this pull request Jan 17, 2022
Description
---

Continues from #3641 

Optionally use a built-in Tor process for macos/linux only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-do_not_merge Process - Not ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants