Conversation
8faf6ef to
8c14140
Compare
83ee8dc to
cf16130
Compare
|
very cool! |
In `workspace`: - New dependencies: `arti_client`, `Cuprate/tokio-socks.git`, `tor-cell`, `tor-config-path`, `tor-hsservice`, `tor-persist`, `tor-proto`, `tor-rtcompat` (yes nothing was exported). - In `deny.toml`, whitelisted `Unlicense` license for `arti_client`. In `cuprate-p2p-transport`: - Implemented `Transport<ClearNet>` and `Transport<Tor>` for `Arti`. New `ArtiClientConfig`, `ArtiServerConfig` configuration. New `OnionListener` for accepting inbound connections from arti's generated onion service. - Implemented `Transport<Tor>` for `Daemon`. New `DaemonClientConfig`, `DaemonServerConfig` configuration. New `DaemonInboundStream` listening for incoming TCP connections from the tor daemon. - `DisabledListener` as a polyfill for transports with inbound disabled, such as `Transport<ClearNet> for Arti` and in the future `Transport<ClearNet> for Socks5`. In `cuprate-p2p-core`: - Removed `Default` and `Debug` bound from `Transport::ClientConfig` and `Transport::ServerConfig`. - Removed `Clone` bound from `Transport::ServerConfig`. In `cuprate-p2p`: - Changed some function visibility to `pub(super)` instead of `pub`. In `cuprate-wire`: - Added `borsh` dependency and `BorshSerialize` and `BorshDeserialize` derived implementation to `OnionAddr` for `BorshNetworkZone` requirement in address book. In `cuprated`: - New `tor` module containing the initialization of Arti, config helpers and context structure `TorContext` to pass down to p2p initialization function and other helpers. - New `config/tor` module containing the `[tor]` configuration table. It define tor daemon related variables, as well as arti settings. - Added `enable_inbound` field to `ClearNetConfig` to disable incoming listener by default. - Added `proxy` field to `ClearNetConfig` for enabling clearnet over arti and in the future proxy urls. - Added `TorNetConfig` for setting `Tor` network zone parameters such as listening ports, enabling arti inbound server, or setting an anonymous inbound onion address from an external daemon. - Modified `initialize_zones_p2p` to now start Tor network zone and use the correct transport depending on user configuration. - In `txpool/*`, generalized `DiffuseService`, `OutboundPeerStream` and `ConcreteDandelionRouter` for `Z: NetworkZone`. Created a new `MainDandelionRouter` service that will route local txs to a Tor router instead of clearnet if available. Adapted initialization to the changes.
binaries/cuprated/src/tor.rs
Outdated
| /// clearnet as a proxy. | ||
| pub async fn initialize_tor_if_enabled(config: &Config) -> TorContext { | ||
| let mode = config.tor.mode; | ||
| let anonymize_clearnet = config.p2p.clear_net.proxy.to_lowercase() == "Tor"; |
There was a problem hiding this comment.
to_lowercase Tor :)
Do you think it might be good idea to make proxy an enum?
There was a problem hiding this comment.
to_lowercaseTor:)
lol
Do you think it might be good idea to make proxy an enum?
It might be but I prefer to think about it in the SOCKS5 PR that comes just after (with docs PR) because fwiw I haven't decided if we should just support SOCKS5 and not SOCKS5H or 4. And therefore if we should rely on some URI library with serde support. we could handroll too. I prefer to think of String as a placeholder atm.
There was a problem hiding this comment.
the enum I was thinking would be something like:
enum {
Tor,
Socks(String)
}with the socks being a catch all, this is a nit but does improve the code over string matching 🤷
There was a problem hiding this comment.
I agree. thx for clarifying
binaries/cuprated/src/tor.rs
Outdated
| let Some(bootstrapped_client) = ctx.bootstrapped_client else { | ||
| panic!("Arti client should be initialized"); | ||
| }; | ||
| let Some(client_config) = ctx.arti_client_config else { | ||
| panic!("Arti client should be initialized"); | ||
| }; |
There was a problem hiding this comment.
bit of nit but these can be combined:
| let Some(bootstrapped_client) = ctx.bootstrapped_client else { | |
| panic!("Arti client should be initialized"); | |
| }; | |
| let Some(client_config) = ctx.arti_client_config else { | |
| panic!("Arti client should be initialized"); | |
| }; | |
| let (Some(bootstrapped_client), Some(client_config)) = (ctx.bootstrapped_client, ctx.arti_client_config) else { | |
| panic!("Arti client should be initialized"); | |
| }; |
There was a problem hiding this comment.
I don't like having for conditions for one panic on the same line. If it panic I'll wonder what was uninitialized.
There was a problem hiding this comment.
well then the comments should be different and mention the field IMO
binaries/cuprated/src/tor.rs
Outdated
| client_config: DaemonClientConfig { | ||
| tor_daemon: config.tor.daemon.address, | ||
| }, | ||
| server_config: (!config.tor.daemon.anonymous_inbound.is_empty()).then_some( |
There was a problem hiding this comment.
should this instead check the bool field?
There was a problem hiding this comment.
It should check bool and warn if anonymous_inbound is empty
| allow = [ | ||
| # Nothing required - free to use without permission. | ||
| "CC0-1.0", # https://creativecommons.org/publicdomain/zero/1.0/ | ||
| "Unlicense", |
There was a problem hiding this comment.
is it possible to instead list the crates to allow?
There was a problem hiding this comment.
There are quite a lot of them. Every subcrates of arti is Unlicense
|
Still have some changes I want to make, will put a comment when I am done. |
|
@SyntheticBird45 If you can review my changes I'll merge |
Approved. (I can't approve my own PR) No comments, changes are clear and reasonable. Thanks for taking finishing it. If you could rebase and update this commit message e66d550 (and also add yourself as co author) that would be perfect. |
What
Part of ongoing Tor support development for Cuprate
Follows: #446 and #481
Changelog
In
workspace:arti_client,Cuprate/tokio-socks.git,tor-cell,tor-config-path,tor-hsservice,tor-persist,tor-proto,tor-rtcompat(yes nothing was exported).deny.toml, whitelistedUnlicenselicense forarti_client.In
cuprate-p2p-transport:Transport<ClearNet>andTransport<Tor>forArti. NewArtiClientConfig,ArtiServerConfigconfiguration. NewOnionListenerfor accepting inbound connections from arti's generated onion service.Transport<Tor>forDaemon. NewDaemonClientConfig,DaemonServerConfigconfiguration. NewDaemonInboundStreamlistening for incoming TCP connections from the tor daemon.DisabledListeneras a polyfill for transports with inbound disabled, such asTransport<ClearNet> for Artiand in the futureTransport<ClearNet> for Socks5.In
cuprate-p2p-core:DefaultandDebugbound fromTransport::ClientConfigandTransport::ServerConfig.Clonebound fromTransport::ServerConfig.In
cuprate-p2p:pub(super)instead ofpub.In
cuprate-wire:borshdependency andBorshSerializeandBorshDeserializederived implementation toOnionAddrforBorshNetworkZonerequirement in address book.In
cuprated:tormodule containing the initialization of Arti, config helpers and context structureTorContextto pass down to p2p initialization function and other helpers.config/tormodule containing the[tor]configuration table. It define tor daemon related variables, as well as arti settings.enable_inboundfield toClearNetConfigto disable incoming listener by default.proxyfield toClearNetConfigfor enabling clearnet over arti and in the future proxy urls.TorNetConfigfor settingTornetwork zone parameters such as listening ports, enabling arti inbound server, or setting an anonymous inbound onion address from an external daemon.initialize_zones_p2pto now start Tor network zone and use the correct transport depending on user configuration.txpool/*, generalizedDiffuseService,OutboundPeerStreamandConcreteDandelionRouterforZ: NetworkZone. Created a newMainDandelionRouterservice that will route local txs to a Tor router instead of clearnet if available. Adapted initialization to the changes.