Add Tor proxy support for node connections#682
Add Tor proxy support for node connections#682guptapratykshh wants to merge 1 commit intobitcoinppl:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds Tor support across Android, iOS, and Rust components of the application. It introduces TorConfig data structures, extends node parsing to accept Tor configurations, updates HTTP clients to support SOCKS5 proxies, and adds UI elements for toggling Tor and configuring proxy addresses on both platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Mobile UI
participant NodeSel as NodeSelector
participant NodeModel as Node Model
participant HTTPClient as HTTP Client
participant Proxy as SOCKS5 Proxy
User->>UI: Enable Tor & Enter Proxy Address
User->>UI: Save Custom Node
UI->>NodeSel: parseCustomNodeWithTor(url, name, torConfig)
NodeSel->>NodeModel: Parse URL, Create Node with TorConfig
NodeModel-->>NodeSel: Return Node(tor=TorConfig)
NodeSel-->>UI: Node with Tor enabled
Note over User,Proxy: Later: Making HTTP Request
User->>UI: Fetch Fee/Price Data
UI->>HTTPClient: build_client(tor.enabled)
alt Tor Enabled
HTTPClient->>HTTPClient: new_client_with_proxy(socks5_url)
HTTPClient->>Proxy: Connect via SOCKS5
Proxy-->>HTTPClient: Proxy connection ready
else Tor Disabled
HTTPClient->>HTTPClient: new_client()
HTTPClient->>HTTPClient: Direct connection
end
HTTPClient-->>UI: Client ready
UI->>HTTPClient: Make request
HTTPClient->>Proxy: Forward request through SOCKS5
Proxy->>Proxy: Route to destination (possibly .onion)
Proxy-->>HTTPClient: Response
HTTPClient-->>UI: Data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds SOCKS5 Tor proxy support for Electrum, Esplora, and HTTP (fee/fiat) clients, routing traffic through a local proxy (e.g., Orbot) when enabled. The
Confidence Score: 3/5Not safe to merge: three P1 issues — a production panic, a silently stale proxy config, and a broken Esplora-over-Tor path. All three P1 findings are present in changed code: the rust/crates/cove-http/src/lib.rs (expect panic), rust/src/node_connect.rs (wrong URL scheme for Esplora onion), rust/src/fee_client.rs and rust/src/fiat/client.rs (stale OnceCell proxy client) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant NS as NodeSelector
participant PNU as parse_node_url
participant EC as ElectrumClient
participant ESC as EsploraClient
participant HC as cove-http (Fee/Fiat)
participant TOR as Tor SOCKS5 Proxy
UI->>NS: parseCustomNodeWithTor(url, tor)
NS->>PNU: parse_node_url(url, is_onion)
Note over PNU: http:// → tcp://<br/>https:// → ssl://<br/>[onion] none/ssl/https → tcp
PNU-->>NS: Node { url: tcp://... or ssl://..., tor }
alt Electrum node + Tor enabled
NS->>EC: new_from_node(node)
EC->>TOR: TCP via Socks5Config (proxy_address)
TOR-->>EC: Electrum response ✅
end
alt Esplora node + Tor enabled
NS->>ESC: new_from_node(node)
Note over ESC: node.url = tcp://xxx.onion/api/ ❌
ESC->>ESC: Builder::new(tcp://xxx.onion) FAILS
end
alt Fee/Fiat client (OnceCell issue)
HC->>HC: get_or_try_init (first call, Tor disabled)
Note over HC: Client cached without proxy
UI->>UI: User enables Tor in settings
HC->>HC: get_or_try_init (already initialized)
Note over HC: Stale client — no proxy ❌
end
Reviews (1): Last reviewed commit: "Add Tor proxy support for node connectio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
guptapratykshh
force-pushed
the
feat/tor-support
branch
from
a348eb7 to
662fbae
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift (1)
233-248:⚠️ Potential issue | 🟡 MinorReset Tor defaults before restoring a saved custom node.
When switching into a custom node option with no matching saved custom node,
torEnabledandtorProxyAddresskeep their previous values and can be saved into the new node.🛠️ Proposed adjustment
.onChange(of: selectedNodeName) { _, newSelectedNodeName in - if selectedNodeName.hasPrefix("Custom") { + if newSelectedNodeName.hasPrefix("Custom") { + torEnabled = false + torProxyAddress = "127.0.0.1:9050" + if case let .custom(savedSelectedNode) = nodeSelector.selectedNode() { - if savedSelectedNode.apiType == .electrum, selectedNodeName.contains("Electrum") { + if savedSelectedNode.apiType == .electrum, newSelectedNodeName.contains("Electrum") { customUrl = savedSelectedNode.url customNodeName = savedSelectedNode.name torEnabled = savedSelectedNode.tor.enabled torProxyAddress = savedSelectedNode.tor.proxyAddress } - if savedSelectedNode.apiType == .esplora, selectedNodeName.contains("Esplora") { + if savedSelectedNode.apiType == .esplora, newSelectedNodeName.contains("Esplora") { customUrl = savedSelectedNode.url customNodeName = savedSelectedNode.name torEnabled = savedSelectedNode.tor.enabled torProxyAddress = savedSelectedNode.tor.proxyAddressAlso applies to: 135-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift` around lines 233 - 248, When handling selectedNodeName changes in NodeSelectionView's .onChange closure (and the similar block around lines 135-146), ensure torEnabled and torProxyAddress are reset to their default values before attempting to restore a saved custom node; specifically, when selectedNodeName.hasPrefix("Custom") but nodeSelector.selectedNode() does not return a matching .custom(savedSelectedNode), set torEnabled = false (or the app's default) and torProxyAddress = "" (or default) so stale values are not carried into a new custom node, and only overwrite these defaults if a matching .custom(savedSelectedNode) is found and its values are applied.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.kt (1)
138-142:⚠️ Potential issue | 🟡 MinorReset Tor fields when clearing custom-node state.
selectPresetNode()clearscustomUrlandcustomNodeName, but leavestorEnabled/torProxyAddressintact. Returning to a custom node flow can then save stale Tor settings throughparseCustomNodeWithTor(...).🛠️ Proposed adjustment
+ val defaultTorProxyAddress = "127.0.0.1:9050" + fun selectPresetNode(nodeName: String) { selectedNodeName = nodeName customUrl = "" customNodeName = "" + torEnabled = false + torProxyAddress = defaultTorProxyAddressval torConfig = TorConfig( enabled = torEnabled, - proxyAddress = torProxyAddress.ifEmpty { "127.0.0.1:9050" }, + proxyAddress = torProxyAddress.ifBlank { defaultTorProxyAddress }, )Also applies to: 191-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.kt` around lines 138 - 142, selectPresetNode currently clears customUrl and customNodeName but leaves Tor settings intact, so update selectPresetNode to also reset torEnabled to false and clear torProxyAddress (empty string or null consistent with your state types) and do the same in the other method that clears custom node state (the function that currently clears customUrl/customNodeName) so that parseCustomNodeWithTor cannot reuse stale tor settings; reference selectPresetNode and parseCustomNodeWithTor when making these changes.
🧹 Nitpick comments (3)
rust/crates/cove-http/src/lib.rs (1)
9-31: Optional: Consider documenting SOCKS5H requirement for .onion proxies.The technical distinction between SOCKS5 (local DNS) and SOCKS5H (remote DNS via proxy) is correct —
.onionaddresses require remote DNS resolution. However, the current codebase does not route.oniontraffic throughcove_http. Esplora nodes useesplora_client::Builder::proxy()and Electrum nodes useSocks5Config::new()directly, both with independent proxy mechanisms. The clients usingcove_http(fiat and fee) connect only to hardcoded clearnet URLs (https://mempool.space), which do not require remote DNS.If
cove_httpis used for.onionhosts in the future, normalizing tosocks5h://at this layer would be beneficial. For now, this could be addressed via documentation or left to the caller to normalize the scheme as needed based on their destination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-http/src/lib.rs` around lines 9 - 31, Note the distinction between SOCKS5 and SOCKS5H and add a short comment in new_client_with_proxy / build_client documenting that .onion hosts require remote DNS (SOCKS5H) and callers must pass a socks5h:// URL if they intend to reach .onion addresses; mention that the current clients (fiat/fee) use clearnet URLs so this is left to callers but future use for .onion should normalize the scheme to socks5h:// at this layer.rust/src/node_connect.rs (2)
156-158: Usemap_err_strfor the parse error conversion.This follows the repo-wide Rust error-conversion helper and keeps this new path consistent.
Suggested change
+use cove_util::ResultExt; + let url = - parse_node_url(&url, is_onion, is_esplora).map_err(|error| Error::ParseNodeUrlError(error.to_string()))?; + parse_node_url(&url, is_onion, is_esplora).map_err_str(Error::ParseNodeUrlError)?;As per coding guidelines, use
cove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string())).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/node_connect.rs` around lines 156 - 158, Replace the current explicit error mapping when calling parse_node_url with the repository helper: instead of using .map_err(|error| Error::ParseNodeUrlError(error.to_string())), call .map_err_str(Error::ParseNodeUrlError) (from cove_util::ResultExt) so the parse_node_url call converts errors via map_err_str; update the use/imports if needed to bring ResultExt into scope and ensure Error::ParseNodeUrlError remains the target constructor.
153-157: Extract.oniondetection from the parsed URL domain, not the raw string.
url.contains(".onion")is case-sensitive and matches substrings in paths and hostnames—for example,http://attacker.com/.onion/apiorhttp://evil.onion.comwould be incorrectly classified, whilehttp://example.ONIONwould be missed. Afterparse_node_url()parses the URL, checkurl.domain().ends_with(".onion")to reliably detect actual.onionTLDs.This would require restructuring to determine
is_onionfrom the parsedUrlobject rather than before parsing, or refactoring the function signature to pass back the detection status.Also applies to: 282-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/node_connect.rs` around lines 153 - 157, Determine .onion from the parsed Url instead of the raw string: call parse_node_url(&url, /*is_onion*/ false, is_esplora) (or refactor parse_node_url to return a Url) and then set is_onion by checking the parsed url's domain() with something like parsed.domain().map(|d| d.ends_with(".onion")).unwrap_or(false); update downstream uses to use this computed is_onion (or change parse_node_url signature to return the detection) and remove the original url.contains(".onion") check; apply the same change for the other occurrence around lines 282-290 referencing parse_node_url, is_onion and node_type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/node.rs`:
- Around line 87-90: The Testnet match arm currently takes (name, url) from
TESTNET_ESPLORA but calls Self::new_electrum, creating an Electrum client for an
HTTP Esplora URL; change the constructor call to the Esplora/HTTP API
initializer (e.g., Self::new_esplora(name.to_string(), url.to_string(),
network)) so Node::default(Network::Testnet) builds an Esplora client that
matches TESTNET_ESPLORA.
In `@rust/src/node/client/esplora.rs`:
- Around line 32-37: The Esplora client is currently configured with a SOCKS5
proxy that uses node.tor.socks5_url() which must use the "socks5h://" scheme to
force proxy-side DNS resolution for .onion addresses; update the proxy usage in
the Esplora client setup (the esplora_client::Builder proxy call) and the other
occurrence in this file to use a socks5h URL, ideally by changing
TorConfig::socks5_url() to return a "socks5h://" URL so all calls (including the
builder.proxy(...) and the second usage) consistently use proxy-side DNS
resolution for .onion endpoints.
---
Outside diff comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.kt`:
- Around line 138-142: selectPresetNode currently clears customUrl and
customNodeName but leaves Tor settings intact, so update selectPresetNode to
also reset torEnabled to false and clear torProxyAddress (empty string or null
consistent with your state types) and do the same in the other method that
clears custom node state (the function that currently clears
customUrl/customNodeName) so that parseCustomNodeWithTor cannot reuse stale tor
settings; reference selectPresetNode and parseCustomNodeWithTor when making
these changes.
In `@ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift`:
- Around line 233-248: When handling selectedNodeName changes in
NodeSelectionView's .onChange closure (and the similar block around lines
135-146), ensure torEnabled and torProxyAddress are reset to their default
values before attempting to restore a saved custom node; specifically, when
selectedNodeName.hasPrefix("Custom") but nodeSelector.selectedNode() does not
return a matching .custom(savedSelectedNode), set torEnabled = false (or the
app's default) and torProxyAddress = "" (or default) so stale values are not
carried into a new custom node, and only overwrite these defaults if a matching
.custom(savedSelectedNode) is found and its values are applied.
---
Nitpick comments:
In `@rust/crates/cove-http/src/lib.rs`:
- Around line 9-31: Note the distinction between SOCKS5 and SOCKS5H and add a
short comment in new_client_with_proxy / build_client documenting that .onion
hosts require remote DNS (SOCKS5H) and callers must pass a socks5h:// URL if
they intend to reach .onion addresses; mention that the current clients
(fiat/fee) use clearnet URLs so this is left to callers but future use for
.onion should normalize the scheme to socks5h:// at this layer.
In `@rust/src/node_connect.rs`:
- Around line 156-158: Replace the current explicit error mapping when calling
parse_node_url with the repository helper: instead of using .map_err(|error|
Error::ParseNodeUrlError(error.to_string())), call
.map_err_str(Error::ParseNodeUrlError) (from cove_util::ResultExt) so the
parse_node_url call converts errors via map_err_str; update the use/imports if
needed to bring ResultExt into scope and ensure Error::ParseNodeUrlError remains
the target constructor.
- Around line 153-157: Determine .onion from the parsed Url instead of the raw
string: call parse_node_url(&url, /*is_onion*/ false, is_esplora) (or refactor
parse_node_url to return a Url) and then set is_onion by checking the parsed
url's domain() with something like parsed.domain().map(|d|
d.ends_with(".onion")).unwrap_or(false); update downstream uses to use this
computed is_onion (or change parse_node_url signature to return the detection)
and remove the original url.contains(".onion") check; apply the same change for
the other occurrence around lines 282-290 referencing parse_node_url, is_onion
and node_type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c938660c-9b91-4538-9e7c-0c51c644e3c7
📒 Files selected for processing (11)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swiftrust/crates/cove-http/Cargo.tomlrust/crates/cove-http/src/lib.rsrust/src/fee_client.rsrust/src/fiat/client.rsrust/src/node.rsrust/src/node/client/electrum.rsrust/src/node/client/esplora.rsrust/src/node_connect.rs
|
@praveenperera is yet not commited to adding TOR support yet he need to know how may deps it adds and how much heavier it makes the app bundle on ios and android |
guptapratykshh
force-pushed
the
feat/tor-support
branch
from
662fbae to
bb9354d
Compare
Route Electrum, Esplora, and HTTP client traffic through a SOCKS5 proxy when Tor is enabled. This lets users connect to their own .onion Electrum/Esplora servers via a local Tor proxy such as Orbot. - Add TorConfig (enabled, proxy_address) to the Node model - Use electrum-client's Client::from_config with Socks5Config - Pass socks5 proxy URL to esplora-client Builder::proxy - Add reqwest socks feature to cove-http for fee/fiat clients - Force tcp:// scheme for .onion addresses (Tor encrypts) - Add Tor toggle and proxy address fields to iOS and Android node settings UI - Backward-compatible: serde(default) on tor field Closes bitcoinppl#256 )
guptapratykshh
force-pushed
the
feat/tor-support
branch
from
db163db to
3afaae4
Compare
|
hey @guptapratykshh, @Justxd22 is also working on TOR, can you guys combine efforts? |
3 participants
Route Electrum, Esplora, and HTTP client traffic through a SOCKS5 proxy when Tor is enabled. This lets users connect to their own .onion Electrum/Esplora servers via a local Tor proxy such as Orbot.
Closes #256
Summary
Testing
Platform Coverage
Checklist
Summary by CodeRabbit
Release Notes
.onionaddresses).