feat: Arti Tor support (#256)#701
Open
Justxd22 wants to merge 9 commits intobitcoinppl:masterfrom
Open
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (9)
📝 WalkthroughWalkthroughAdds end-to-end Tor support: built-in Arti runtime, Tor-aware node/client plumbing and DB keys, UniFFI bindings for Tor operations/logs, mobile UI flows for Tor configuration/validation (built-in / Orbot / external), BDK submodule + setup automation and CI updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Mobile UI
participant App as AppManager
participant Tor as TorRuntime (arti)
participant Probe as NodeProbe / NodeClient
participant Node as Remote Node
User->>UI: enable Tor or save onion node
UI->>App: set useTor / store pending onion draft
App->>Tor: ensureBuiltInTorBootstrap()
Tor-->>App: SOCKS endpoint or error
UI->>Probe: testSocksEndpoint(host,port)
Probe->>Tor: TCP connect / SOCKS handshake
Tor-->>Probe: reachable / fail
Probe->>Node: connect to node via SOCKS
Node-->>Probe: reachable / unreachable
Probe-->>UI: test results
UI->>App: on success -> persist node / clear pending draft
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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. Review rate limit: 0/1 reviews remaining, refill in 57 minutes and 17 seconds.Comment |
Greptile SummaryThis PR adds end-to-end Tor support (built-in Arti, Orbot, and external SOCKS5) across the Rust core, Android, and iOS, including config persistence, bootstrap progress UI, a three-step connection test, onion-node inference, and Tor-aware routing through
Confidence Score: 4/5Safe to merge after addressing the unsound One P1 finding:
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Mobile UI
participant NCS as NodeConnect
participant NC as NodeClient
participant TR as TorRuntime
participant Arti as Arti SOCKS Proxy
participant EC as ElectrumClient
UI->>NCS: check_and_save_node(onion_node)
NCS->>NCS: node_implies_tor? → true
NCS->>NC: NodeClientOptions.resolve_tor_endpoint()
NC->>TR: built_in_socks_endpoint()
TR->>Arti: launch_built_in_proxy() [once]
TR->>TR: wait_for_socks_listener (40×100ms)
Arti-->>TR: SOCKS listener ready :39050
TR-->>NC: SocketAddr(127.0.0.1:39050)
NC-->>NCS: resolved options
NCS->>NC: new_with_options(node, resolved_options)
NC->>NC: resolve_tor_endpoint() [cache hit]
NC->>EC: new_from_node_and_options
EC->>Arti: Client::from_config socks5=127.0.0.1:39050
Arti-->>EC: connection ok
EC-->>NC: ElectrumClient
NC-->>NCS: NodeClient
NCS->>NCS: set_selected_node + set_use_tor(true)
NCS-->>UI: Ok(())
Reviews (1): Last reviewed commit: "feat: Add iOS Tor settings and wallet UI..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Author
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
rust/src/wallet_scanner.rs-165-165 (1)
165-165:⚠️ Potential issue | 🟡 MinorScanner uses stale Tor configuration snapshot if config changes mid-scan.
NodeClientBuilder::with_defaults()readsGlobalConfigatWalletScanner::try_new()construction time and snapshots Tor settings (use_tor,tor_mode, external host/port). The scanner is created once and never recreated. If a user toggles Tor mid-scan, worker tasks continue using the pre-toggle configuration until they complete or the scanner is destroyed.Unlike
WalletActor, which actively detects config changes via signature comparison and recreates the node client,WalletScannerhas no mechanism to handle config changes. Confirm whether:
- Scans are expected to be restarted when Tor config changes, or
- The scanner should detect config changes and recreate workers mid-scan
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/wallet_scanner.rs` at line 165, WalletScanner currently snapshots Tor settings once via NodeClientBuilder::with_defaults in WalletScanner::try_new and never updates them, so toggling Tor mid-scan leaves workers using stale config; update WalletScanner to detect GlobalConfig changes (reuse the same signature comparison approach used by WalletActor) and recreate the node client/workers when the config signature differs: add a stored config signature field to WalletScanner, compare it before launching or inside the worker loop, and if changed drop/recreate the NodeClientBuilder and restart affected worker tasks so they pick up updated use_tor/tor_mode/host/port settings (ensure NodeClientBuilder::with_defaults is called whenever the signature changes).ios/Cove/AppManager.swift-151-164 (1)
151-164:⚠️ Potential issue | 🟡 MinorFix misleading log message wording in
warmupTorIfConfigured().The log "Built-in Tor warmup started at…" fires after
await; the warmup is already complete. Change to "ready at…" or "bootstrapped to…" for accuracy.Note: The Rust side (
ensureBuiltInTorBootstrap()) usesLazy<Mutex<BuiltInTorState>>with cached endpoints, so overlapping bootstrap calls frominit()andreset()are safe—they either return the cached endpoint or await the in-progress launch. No guarding needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` around lines 151 - 164, Update the log message in warmupTorIfConfigured() to accurately reflect that ensureBuiltInTorBootstrap() has completed: replace the success Log.debug("Built-in Tor warmup started at \(endpoint)") with wording like "Built-in Tor ready at \(endpoint)" or "Built-in Tor bootstrapped to \(endpoint)". Leave the existing await and error handling in place; reference warmupTorIfConfigured() and ensureBuiltInTorBootstrap() when making the change so the log reflects completion rather than initiation.rust/src/manager/wallet_manager/actor.rs-1467-1499 (1)
1467-1499:⚠️ Potential issue | 🟡 MinorUse typed accessors in
node_client_signature_forto matchNodeClient::new.The signature method should read Tor config via the same typed accessors as
NodeClient::new, with matching defaults. Currently it uses raw.get(GlobalConfigKey::...)strings fortor_modeandtor_external_host, which diverges from the actual client construction:
NodeClient::newreadsconfig.tor_mode().unwrap_or_default()(parsed value) andconfig.tor_external_host().ok().filter(...).unwrap_or_else(|| "127.0.0.1").- The signature reads raw strings with empty-string defaults:
get(TorMode).ok().flatten().unwrap_or_default()andget(TorExternalHost).ok().flatten().unwrap_or_default().This mismatch means that if stored representations are normalized (e.g.,
"BUILT_IN"→"BuiltIn"during a schema migration), the signature will spuriously differ from a cached one despite the logical Tor config being identical. Align the signature to useconfig.tor_mode()andconfig.tor_external_host()with the same defaults asNodeClient::new:♻️ Proposed refactor
fn node_client_signature_for(node: &Node) -> String { - let global_config = Database::global().global_config.clone(); + let global_config = &Database::global().global_config; let use_tor = global_config.use_tor(); - let tor_mode = - global_config.get(GlobalConfigKey::TorMode).ok().flatten().unwrap_or_default(); - let tor_external_host = - global_config.get(GlobalConfigKey::TorExternalHost).ok().flatten().unwrap_or_default(); + let tor_mode = global_config.tor_mode().unwrap_or_default(); + let tor_external_host = global_config + .tor_external_host() + .ok() + .filter(|host| !host.is_empty()) + .unwrap_or_else(|| "127.0.0.1".to_string()); let tor_external_port = global_config.tor_external_port(); format!( "node={node:?}|use_tor={use_tor}|tor_mode={tor_mode}|tor_external_host={tor_external_host}|tor_external_port={tor_external_port}" ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 1467 - 1499, The node_client_signature_for function uses raw GlobalConfig::get string accessors which can diverge from NodeClient::new; update node_client_signature_for to use the same typed accessors as NodeClient::new: call global_config.tor_mode().unwrap_or_default() for tor_mode and derive tor_external_host via global_config.tor_external_host().ok().filter(|s| !s.is_empty()).unwrap_or_else(|| "127.0.0.1".to_string()) (matching NodeClient::new defaults), keep use_tor and tor_external_port as before, and then format the signature string using these values so the signature aligns with NodeClient::new's logic.android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt-344-382 (1)
344-382:⚠️ Potential issue | 🟡 MinorStartup Tor check won't re-run after user fixes the proxy externally.
startupTorCheckCompletedis aremembered flag that flips totrueon first run and is never reset. Combined withLaunchedEffect(app.isTermsAccepted)as the only trigger, this means that if the user:
- Opens app, sees the "Orbot is not active" dialog, taps Ignore.
- Switches to Orbot, starts it, returns to Cove.
…the dialog state won't be refreshed. Since the dialog is also dismissed in all branches (including "Ignore"), users have no in-app way to re-validate short of restarting. Consider re-running the probe on
ON_RESUME(lifecycle) or at least resettingstartupTorCheckCompletedwhen the dialog is dismissed with "Ignore", so a second foregrounding re-checks reachability.🤖 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/MainActivity.kt` around lines 344 - 382, The startup Tor probe only runs once because startupTorCheckCompleted (remembered) is set to true and never reset inside the LaunchedEffect(app.isTermsAccepted) block; update the flow so the probe can re-run when the app resumes or the user dismisses the dialog: either subscribe to lifecycle ON_RESUME (or add app lifecycle/state as a LaunchedEffect key) so the LaunchedEffect retriggers and re-evaluates globalConfig/useTor/mode, or reset startupTorCheckCompleted to false when the "Ignore" path dismisses the Tor dialog (the code that sets startupTorUnavailableMode/startupTorUnavailableEndpoint), ensuring testSocksEndpoint(...) is re-invoked and the dialog state is refreshed when the user returns.ios/Cove/CoveMainView.swift-557-575 (1)
557-575:⚠️ Potential issue | 🟡 MinorSignature-based dedupe prevents re-checking after the user fixes Tor externally.
startupTorCheckSignatureis set to the new signature before the async probe runs, and is only reset whenuseTor()becomes false (line 538). When the signature is unchanged (same mode, endpoint, selected node) the function returns early at line 559 — which covers the realistic case where a user sees the alert, taps Ignore, launches Orbot from the app switcher, and returns to Cove. On foregrounding (.active→ line 443) the signature still matches and the probe is skipped, so the warning state (startupTorUnavailable) is never cleared and never re-verified.Consider either:
- Clearing
startupTorCheckSignaturewhen the warning is dismissed (including the "Ignore" path inStartupTorWarningModifier), or- Only using the signature to skip when
startupTorUnavailable == nil(i.e., the previous check succeeded), so we always re-probe after a failure.♻️ Proposed refinement
- let signature = "\(mode.persistedValue)|\(endpointDescription)|\(app.selectedNode.url)" - guard startupTorCheckSignature != signature else { return } - startupTorCheckSignature = signature + let signature = "\(mode.persistedValue)|\(endpointDescription)|\(app.selectedNode.url)" + // Only skip when the last check succeeded; always re-probe after a failure + if startupTorUnavailable == nil, startupTorCheckSignature == signature { return } + startupTorCheckSignature = signature🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveMainView.swift` around lines 557 - 575, The current signature-based dedupe sets startupTorCheckSignature before awaiting testSocksEndpoint, which prevents re-probing after a failure; update the logic so a stored failure does not block re-checks: change the early-return guard that compares startupTorCheckSignature to also require startupTorUnavailable == nil (i.e., only skip when the previous probe succeeded), or alternatively clear startupTorCheckSignature whenever the warning is dismissed (e.g., in StartupTorWarningModifier’s "Ignore" path) so that calling testSocksEndpoint(host:port:timeout:) can run again; touch symbols: startupTorCheckSignature, startupTorUnavailable, testSocksEndpoint, StartupTorWarningModifier, and useTor() when applying the fix.ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift-281-281 (1)
281-281:⚠️ Potential issue | 🟡 MinorFix the success message typo.
Succesfullyshould beSuccessfully. As per coding guidelines,ios/Cove/**/*.swift: “Identify spelling mistakes in comments, string literals, and documentation.”Proposed text fix
- completeLoading(.success("Succesfully connected to \(selected.url)")) + completeLoading(.success("Successfully connected to \(selected.url)"))🤖 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` at line 281, In NodeSelectionView.swift update the success string passed to completeLoading(.success(...)) to correct the typo: change "Succesfully connected to \(selected.url)" to "Successfully connected to \(selected.url)" so the message returned by completeLoading uses the proper spelling; the change should be made where completeLoading is called with the interpolated selected.url.rust/src/node/client/electrum.rs-65-65 (1)
65-65:⚠️ Potential issue | 🟡 MinorRedact sensitive endpoints from debug and error logs.
Lines 65, 88, 106-115, and 357 expose raw node URLs and Tor proxy addresses (
tor_host,tor_port, and fullendpointstrings). When users copy logs from mobile UIs for support, this reveals infrastructure details. Replace full URLs/endpoints with redacted versions (e.g., log only the host scheme and port, omitting the domain) to preserve diagnostic utility while protecting user privacy.Example locations to update in `rust/src/node/client/electrum.rs`
- Line 65: Replace
url = %urlwith redacted URL or omit it- Lines 88, 107-111: Replace
node_url = %urlandtor_host = %options.tor_external_hostwith redacted versions- Line 357: Replace full
endpointwith redacted host/port format (e.g.,redacted_host:port)Similar patterns exist in
rust/src/node/client.rs(lines 117, 139, 150) andrust/src/node/client/esplora.rs(line 48).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/node/client/electrum.rs` at line 65, Debug and error logs in rust/src/node/client/electrum.rs currently print full URLs and Tor endpoints (see debug! at creation, logs referencing node_url, options.tor_external_host/tor_external_port, and the endpoint variable); replace those direct interpolations with a redacted representation by adding and calling a small helper (e.g., redact_url/redact_endpoint) that returns only scheme and port or "redacted_host:port" and use that helper in the debug! and error! invocations (specifically where %url, node_url = %url, options.tor_external_host, options.tor_external_port, and endpoint are logged) so logs retain diagnostic info without exposing domains/hosts.rust/src/node/client.rs-164-200 (1)
164-200:⚠️ Potential issue | 🟡 MinorTor endpoint is resolved twice on the
new()path.
NodeClient::newcallsoptions.resolve_tor_endpoint().await?at line 189, then immediately delegates tonew_with_options, which resolves again at line 200. ForTorMode::BuiltInthat meanstor_runtime::built_in_socks_endpoint()is awaited twice per client construction (along with duplicate INFO logs), and if the Arti runtime is slow to warm up the first call's latency is effectively paid twice.Since
new_with_optionsis authoritative (it's also the entry point fortry_from_builder, where resolution is needed), drop the resolve fromnewand letnew_with_optionshandle it.♻️ Proposed fix
info!(node = %node.url, api_type = ?node.api_type, options = ?options, "creating node client with db-backed options"); - - let options = options.resolve_tor_endpoint().await?; Self::new_with_options(node, options).await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/node/client.rs` around lines 164 - 200, Remove the redundant tor endpoint resolution in NodeClient::new: do not call options.resolve_tor_endpoint().await there; instead pass the plain NodeClientOptions into Self::new_with_options so that new_with_options (which already calls options.resolve_tor_endpoint().await?) is the single place that performs tor resolution. Update NodeClient::new to construct options and call Self::new_with_options(node, options).await without awaiting resolve, keeping logging as-is in new_with_options to avoid duplicated awaits and INFO logs for TorMode::BuiltIn.
🧹 Nitpick comments (12)
android/app/src/main/res/drawable/icon_tor_onion.xml (2)
6-8: Prefer Android color constant for consistency.The fill color uses
#FFFFFFFFwhile other icons in the codebase use@android:color/white. Both are functionally equivalent, but the Android constant is more idiomatic.♻️ Use standard color constant
<path - android:fillColor="#FFFFFFFF" + android:fillColor="@android:color/white" android:pathData="M90.1846,163.6311L90.1846,152.7211C124.7436,152.6213 152.7261,124.5814 152.7261,89.9975C152.7261,55.4161 124.7436,27.3762 90.1846,27.2764L90.1846,16.3664C130.7687,16.4686 163.6336,49.391 163.6336,89.9975C163.6336,130.6065 130.7687,163.5314 90.1846,163.6311ZM90.1846,125.4446C109.6771,125.3424 125.4546,109.5174 125.4546,89.9975C125.4546,70.4801 109.6771,54.6552 90.1846,54.5529L90.1846,43.6453C115.7047,43.7451 136.3647,64.455 136.3647,89.9975C136.3647,115.5425 115.7047,136.2524 90.1846,136.3522L90.1846,125.4446ZM90.1846,70.9167C100.6406,71.0165 109.0908,79.5165 109.0908,89.9975C109.0908,100.481 100.6406,108.981 90.1846,109.0808L90.1846,70.9167ZM0,89.9975C0,139.7053 40.2922,180 90,180C139.7053,180 180,139.7053 180,89.9975C180,40.2922 139.7053,0 90,0C40.2922,0 0,40.2922 0,89.9975Z" /> </vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/res/drawable/icon_tor_onion.xml` around lines 6 - 8, The path element in icon_tor_onion.xml currently hardcodes android:fillColor="#FFFFFFFF"; replace that literal with the Android color constant by setting the fill color to `@android`:color/white to match other icons and maintain idiomatic consistency (update the android:fillColor attribute on the <path> element).
1-5: Consider using standard 24×24 viewport dimensions for consistency.The viewport dimensions (180×180) don't match the standard pattern used by other icons in the codebase, which typically use 24×24 viewports to match the 24dp size. This creates a 7.5× scaling factor that could affect coordinate precision and makes the pathData less intuitive to maintain.
🎨 Align viewport with codebase standard
To match the pattern in
icon_qr_code.xmlandicon_currency_bitcoin.xml, consider updating the viewport and rescaling the pathData accordingly:<vector xmlns:android="http://schemas.android.com/apk/res/android" android:width="24dp" android:height="24dp" - android:viewportWidth="180" - android:viewportHeight="180"> + android:viewportWidth="24" + android:viewportHeight="24"> <path android:fillColor="#FFFFFFFF" - android:pathData="M90.1846,163.6311L90.1846,152.7211C124.7436,152.6213 152.7261,124.5814 152.7261,89.9975C152.7261,55.4161 124.7436,27.3762 90.1846,27.2764L90.1846,16.3664C130.7687,16.4686 163.6336,49.391 163.6336,89.9975C163.6336,130.6065 130.7687,163.5314 90.1846,163.6311ZM90.1846,125.4446C109.6771,125.3424 125.4546,109.5174 125.4546,89.9975C125.4546,70.4801 109.6771,54.6552 90.1846,54.5529L90.1846,43.6453C115.7047,43.7451 136.3647,64.455 136.3647,89.9975C136.3647,115.5425 115.7047,136.2524 90.1846,136.3522L90.1846,125.4446ZM90.1846,70.9167C100.6406,71.0165 109.0908,79.5165 109.0908,89.9975C109.0908,100.481 100.6406,108.981 90.1846,109.0808L90.1846,70.9167ZM0,89.9975C0,139.7053 40.2922,180 90,180C139.7053,180 180,139.7053 180,89.9975C180,40.2922 139.7053,0 90,0C40.2922,0 0,40.2922 0,89.9975Z" /> + android:pathData="[rescaled coordinates dividing all values by 7.5]" /> </vector>Note: The pathData coordinates would need to be divided by 7.5 to match the new 24×24 viewport.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/res/drawable/icon_tor_onion.xml` around lines 1 - 5, The vector uses android:viewportWidth/android:viewportHeight set to 180 which creates a 7.5× scale vs the repo standard; change android:viewportWidth and android:viewportHeight to 24 (keep android:width/android:height at 24dp) and rescale every pathData coordinate in this file (all path elements in icon_tor_onion.xml) by dividing coordinates and control points by 7.5 so the shapes match visually at the new 24×24 viewport; verify stroke widths and any transforms are similarly scaled and update any numeric attributes tied to the old 180 viewport.justfile (1)
24-27: Consider--forcefor idempotent re-runs.If a previous
just setupfailed partway through patch application, thebdksubmodule tree may have uncommitted changes thatgit submodule updatewill refuse to overwrite, leaving setup stuck. Adding--forcemakes re-runs more reliable.♻️ Proposed tweak
- if ! git submodule update --init --recursive --depth 1 --recommend-shallow; then + if ! git submodule update --init --recursive --force --depth 1 --recommend-shallow; then echo "Shallow submodule checkout failed, retrying with full history..." - git submodule update --init --recursive + git submodule update --init --recursive --force fiNote: the reverse-apply check below still correctly reports "Already applied" on clean re-runs, so
--forceis only relevant for recovery from broken states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 24 - 27, The git submodule update commands in the justfile can fail if the bdk submodule has local changes; make the setup idempotent by adding --force to both occurrences: the shallow checkout command (git submodule update --init --recursive --depth 1 --recommend-shallow) and the fallback full-history command (git submodule update --init --recursive) so retries will overwrite dirty submodule state and proceed.android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
424-445: Nit: Fully-qualifiedTorModereferences are redundant.
org.bitcoinppl.cove_core.*is already wildcard-imported at line 13, soTorMode.BUILT_IN.nameworks directly. The explicit FQNs on lines 431-432 are pure visual noise.♻️ Proposed fix
- val mode = globalConfig.get(GlobalConfigKey.TorMode) - ?: org.bitcoinppl.cove_core.TorMode.BUILT_IN.name - if (mode != org.bitcoinppl.cove_core.TorMode.BUILT_IN.name) { + val mode = globalConfig.get(GlobalConfigKey.TorMode) + ?: TorMode.BUILT_IN.name + if (mode != TorMode.BUILT_IN.name) { Log.d(tag, "Tor enabled with mode=$mode; no built-in warmup needed") return }🤖 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/AppManager.kt` around lines 424 - 445, The redundant fully-qualified TorMode references in warmupTorIfConfigured should be simplified: replace org.bitcoinppl.cove_core.TorMode.BUILT_IN.name with TorMode.BUILT_IN.name (and any other occurrences) so the method uses the wildcard-imported TorMode symbol; update the comparisons around GlobalConfigKey.TorMode and the mode variable in the warmupTorIfConfigured function to reference TorMode directly (e.g., TorMode.BUILT_IN.name) to remove visual noise.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OrbotPackageHelper.kt (1)
23-27: Minor:openOrbotshould addFLAG_ACTIVITY_NEW_TASKfor safety.
openInstallPage(lines 30-38) already setsFLAG_ACTIVITY_NEW_TASKon both market and web intents, butopenOrbotdoes not. If a future caller passes a non-ActivityContext(e.g., application context from a service or broadcast receiver),startActivitywill throwAndroidRuntimeException: Calling startActivity() from outside of an Activity context requires the FLAG_ACTIVITY_NEW_TASK flag. Current callers appear to be Activities, so this is defensive, not immediate.♻️ Proposed fix
- fun openOrbot(context: Context): Boolean { - val launchIntent = context.packageManager.getLaunchIntentForPackage(ORBOT_PACKAGE) ?: return false - context.startActivity(launchIntent) - return true - } + fun openOrbot(context: Context): Boolean { + val launchIntent = context.packageManager.getLaunchIntentForPackage(ORBOT_PACKAGE) ?: return false + launchIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + context.startActivity(launchIntent) + return true + }🤖 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/OrbotPackageHelper.kt` around lines 23 - 27, openOrbot currently gets a launchIntent for ORBOT_PACKAGE and calls startActivity without adding FLAG_ACTIVITY_NEW_TASK; update the function (openOrbot) to add Intent.FLAG_ACTIVITY_NEW_TASK to the launchIntent before calling context.startActivity to avoid AndroidRuntimeException when a non-Activity Context is used (use launchIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) with the existing ORBOT_PACKAGE lookup).ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
226-232: Consider throttling the Tor quick-status poll when the popover is closed.The
.task(id: manager.id)effectively keepspollTorQuickStatus()alive for the entire wallet-screen lifetime. Every 3 seconds you:
- Call into Rust (
ensureBuiltInTorBootstrap— cached, cheap) and copy up to 400 Tor log lines (torConnectionLogs()), or- For Orbot/custom SOCKS, perform an actual TCP connect with a 1.5s timeout (
testSocksEndpoint).For a user who never opens the Tor popover, this is continuous socket/FFI work in the background of the main wallet screen. A light optimization: keep a slow cadence (e.g., 30s) when
showTorQuickStatus == falseand switch to 3s only while the popover is presented. The.task(id:)can key on bothmanager.idandshowTorQuickStatusso the loop restarts at the faster rate when opened.Not blocking — just a battery/efficiency refinement on a long-lived screen.
Also applies to: 568-570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift` around lines 226 - 232, The poll is too aggressive when the popover is closed; update pollTorQuickStatus() to use a dynamic interval (e.g., 3s when showTorQuickStatus == true, 30s otherwise) and make the surrounding .task keyed on both manager.id and showTorQuickStatus so the loop restarts at the faster rate when opened; specifically, modify the pollTorQuickStatus() loop to read showTorQuickStatus each iteration (or compute interval = showTorQuickStatus ? 3 : 30) before calling await refreshTorQuickStatus() and sleeping with try? await Task.sleep(for: .seconds(interval)), and update the .task(id:) invocation to include showTorQuickStatus alongside manager.id so the task re-evaluates when the popover is shown.android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt (1)
297-515: Minor: Hardcoded English strings bypass the new Tor string resources.Dialog copy here (e.g., "Orbot is not active", "External Tor proxy is unavailable", "Use clearnet node", "Tor disabled", "Fallback failed", "Could not switch to a clearnet node: …", and the two body messages) is hardcoded, while
strings.xmlin this PR adds a dedicated set of Tor resources (tor_disable_onion_dialog_*,tor_fallback_failed,tor_fallback_applied, etc.). For consistency with the rest of the Tor UI and to preserve future localization, preferstringResource(R.string.…)/context.getString(…)here or add matching resource entries.Not blocking — purely an i18n/consistency nit.
🤖 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/MainActivity.kt` around lines 297 - 515, Replace hardcoded English strings in the Tor UI with the new string resources: update fallbackToClearnetAndDisableTor messages ("Tor disabled", "Fallback failed", "Could not switch...") to use stringResource/context.getString with the PR-added resource keys (e.g., tor_fallback_applied, tor_fallback_failed, tor_disable_onion_dialog_*), and replace all AlertDialog title/text/button literals in the LaunchedEffect/AlertDialog block (e.g., "Orbot is not active", "External Tor proxy is unavailable", body messages, "Use clearnet node", "Open Orbot", "Open network settings", "Ignore") to the corresponding R.string entries; ensure you import androidx.compose.ui.res.stringResource (or use context.getString(context, R.string.xxx) where needed) and keep variable substitutions (startupTorUnavailableEndpoint, fallbackNodeName) by formatting strings with placeholders.rust/crates/cove-common/src/logging.rs (1)
60-71: Heuristic content filter may leak non-Tor logs and miss line-start "Tor" messages.The content heuristic has two small gaps:
" tor "requires whitespace on both sides, so a message that begins withTor(e.g.,"Tor bootstrapped 100%"formatted as"[INFO some::target] Tor bootstrapped 100%") only matches via the preceding space after]— that works — but something like a field-only valuetor=...orTor:at the start of the message portion already matches via"tor:". Consider tightening to word boundaries (e.g., check for" tor"/"tor "/":tor "or use a simple regex) to avoid substring false positives liketerminator,rotor,torch, etc."socks"is broad and will match any event mentioning SOCKS unrelated to Tor (e.g., general proxy code paths).Given the target-based prefix check already covers all
arti*/tor_*crates, you can likely rely primarily on targets and drop/tighten the lexical fallback to reduce noise in the captured buffer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-common/src/logging.rs` around lines 60 - 71, The is_tor_related function should tighten and reduce its lexical heuristics and rely primarily on target-based matches: keep the existing target checks (targets starting with "arti" or "tor_" or containing "tor_runtime") and replace the loose lowercase substring checks with word-boundary-like checks (e.g., match "tor" as a separate token at start/middle/end or prefixed/suffixed by punctuation such as ":" or "=") rather than contains(" tor ") to avoid matching substrings like "terminator" or "torch"; remove or narrow the generic "socks" match (drop it or require "socks5"/"socks5h"/explicit proxy context) and only keep specific tokens like "onion" and explicit "tor:"/"tor=" patterns. Update is_tor_related to perform these tightened checks on line.to_ascii_lowercase() and leave the target-based short-circuit intact.ios/Cove/CoveMainView.swift (1)
604-608: Add completion handler and error feedback for Orbot URL launch.The current implementation silently fails if Orbot isn't installed, leaving the user with no feedback after the alert dismisses. This is inconsistent with
openOrbotBestEffort()in NetworkSettingsView, which already handles this scenario with a completion handler and user notification.Update the method to check the result and provide feedback:
♻️ Proposed fix
private func openOrbotFromStartupWarning() { startupTorUnavailable = nil guard let url = URL(string: "orbot://") else { return } - UIApplication.shared.open(url) + UIApplication.shared.open(url) { success in + if !success { + // Match the pattern in NetworkSettingsView.openOrbotBestEffort() + // Could show an alert or log message to inform user Orbot couldn't be opened + } + } }Note: The Info.plist does not currently declare
orbot://inLSApplicationQueriesSchemes, though adding it is good practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveMainView.swift` around lines 604 - 608, The openOrbotFromStartupWarning() currently calls UIApplication.shared.open(url) without a completion handler and gives no feedback if Orbot isn't installed; update this method to call UIApplication.shared.open(_:options:completionHandler:) (use the same success/failure handling as NetworkSettingsView.openOrbotBestEffort()), clear startupTorUnavailable = nil as before, and on failure present a user-facing alert or toast explaining Orbot could not be opened; also ensure the orbot:// scheme is added to LSApplicationQueriesSchemes in Info.plist to allow canOpenURL/open checks to work reliably.rust/src/node_connect.rs (1)
173-174: Usemap_err_strfor the direct string conversion.This changed parse path hand-rolls the conversion that the project helper already covers.
♻️ Proposed refactor
+use cove_util::ResultExt as _; + - let url = parse_node_url(&url, api_type) - .map_err(|error| Error::ParseNodeUrlError(error.to_string()))?; + let url = parse_node_url(&url, api_type).map_err_str(Error::ParseNodeUrlError)?;As per coding guidelines,
**/*.rs: Usecove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string()))— it's cleaner and equivalent.🤖 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 173 - 174, Replace the manual string-conversion mapping on the parse result with the helper: change the call that invokes parse_node_url(...).map_err(|error| Error::ParseNodeUrlError(error.to_string())) to use cove_util::ResultExt::map_err_str so the parse_node_url result is mapped into Error::ParseNodeUrlError via map_err_str; reference parse_node_url and the Error::ParseNodeUrlError variant and import or use map_err_str from cove_util::ResultExt as needed.rust/src/node/client.rs (1)
144-157: UseResultExt::map_err_strper coding guidelines.The explicit
matchonbuilt_in_socks_endpoint()exists only to convert the error viaerror.to_string(). The repo guideline prefersmap_err_strfor this exact pattern.♻️ Proposed fix
- TorMode::BuiltIn => { - info!("tor mode is built-in; requesting Arti socks endpoint"); - match tor_runtime::built_in_socks_endpoint().await { - Ok(endpoint) => { - self.tor_external_host = endpoint.ip().to_string(); - self.tor_external_port = endpoint.port(); - info!(%endpoint, "resolved built-in tor socks endpoint"); - } - Err(error) => { - tracing::error!("failed to initialize built-in tor runtime: {error}"); - return Err(Error::ResolveTorEndpoint(error.to_string())); - } - } - } + TorMode::BuiltIn => { + info!("tor mode is built-in; requesting Arti socks endpoint"); + let endpoint = tor_runtime::built_in_socks_endpoint() + .await + .map_err_str(Error::ResolveTorEndpoint)?; + self.tor_external_host = endpoint.ip().to_string(); + self.tor_external_port = endpoint.port(); + info!(%endpoint, "resolved built-in tor socks endpoint"); + }As per coding guidelines: "Use
cove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string()))— it's cleaner and equivalent".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/node/client.rs` around lines 144 - 157, The match on tor_runtime::built_in_socks_endpoint() in the TorMode::BuiltIn branch exists only to convert the error to a string; replace that explicit match with a call that uses cove_util::ResultExt::map_err_str to map errors into Error::ResolveTorEndpoint, then await the endpoint and set self.tor_external_host = endpoint.ip().to_string() and self.tor_external_port = endpoint.port(), and use info!(%endpoint, ...) as before; keep the tracing::error path removed because map_err_str will produce the mapped Error::ResolveTorEndpoint on failure.android/app/src/main/java/org/bitcoinppl/cove/tor/TorStatusSupport.kt (1)
243-245:isTor/ipdetection is stricter than the iOS counterpart.The substring checks
"\"istor\":true"and"\"is_tor\":true"(line 244) won't match JSON with spacing variants like"IsTor": trueor"IsTor" : true. The iOS equivalent (parseTorApiJsoninios/Cove/TorSupport.swift) uses#""istor"\s*:\s*true"#plus actualJSONSerialization, so the two platforms can disagree on the same response body. Similarly, the regex forIPis case-sensitive on"IP"— fine for this endpoint today, but brittle.Prefer JSON parsing (e.g.,
JSONObject) with a regex fallback, mirroring the iOS logic:♻️ Proposed fix
- val normalized = raw.lowercase() - val isTor = "\"istor\":true" in normalized || "\"is_tor\":true" in normalized - val ip = Regex(""""IP"\s*:\s*"([^"]+)"""").find(raw)?.groupValues?.getOrNull(1) + val json = runCatching { org.json.JSONObject(raw) }.getOrNull() + val isTor = json?.optBoolean("IsTor") + ?: json?.optBoolean("is_tor") + ?: Regex(""""is_?tor"\s*:\s*true""", RegexOption.IGNORE_CASE).containsMatchIn(raw) + val ip = json?.optString("IP")?.takeIf { it.isNotEmpty() } + ?: Regex(""""IP"\s*:\s*"([^"]+)"""", RegexOption.IGNORE_CASE) + .find(raw)?.groupValues?.getOrNull(1)🤖 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/tor/TorStatusSupport.kt` around lines 243 - 245, The current detection in TorStatusSupport (variables normalized / isTor / ip) is too strict and should parse JSON first like iOS does: update the parsing logic in the function containing normalized/isTor/ip to try JSONObject (or Kotlinx.serialization) to read boolean keys like "isTor"/"is_tor" case-insensitively and to read the IP value without relying on a case-sensitive "IP" literal; if JSON parsing fails, fall back to a looser regex (use case-insensitive flags and allow optional whitespace) to detect the boolean istor/is_tor and IP value (replace the current Regex(""""IP"\s*:\s*"([^"]+)"""") and substring checks with these robust alternatives). Ensure you still return the same outputs from that function and keep Regex only as fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12011b7b-a8e3-43d4-924b-fd0d902b3c31
⛔ Files ignored due to path filters (3)
ios/Cove/Assets.xcassets/iconTorOnion.imageset/onion.svgis excluded by!**/*.svgios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.gitmodulesCONTRIBUTING.mdandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletContainer.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OrbotPackageHelper.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/TorNodeSupport.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/TorUiState.ktandroid/app/src/main/java/org/bitcoinppl/cove/tor/TorStatusSupport.ktandroid/app/src/main/java/org/bitcoinppl/cove/views/SettingsItem.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/res/drawable/icon_tor_onion.xmlandroid/app/src/main/res/values/strings.xmlios/Cove/AppManager.swiftios/Cove/Assets.xcassets/iconTorOnion.imageset/Contents.jsonios/Cove/CoveMainView.swiftios/Cove/Extention/Result+Ext.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/Flows/SettingsFlow/NetworkSettingsView.swiftios/Cove/Flows/SettingsFlow/SettingsContainer.swiftios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swiftios/Cove/TorSupport.swiftios/Cove/WalletManager.swiftjustfilerust/Cargo.tomlrust/crates/cove-common/src/logging.rsrust/external/bdkrust/patches/bdk/0001-bdk-chain-rusqlite-sqlite-int64-conversions.patchrust/patches/bdk/0002-bdk-remove-workspace-lints-in-patched-crates.patchrust/patches/bdk/0003-bdk-chain-align-rusqlite-with-arti.patchrust/patches/bdk/0004-bdk-chain-allow-coverage-nightly-cfg.patchrust/src/database/global_config.rsrust/src/database/global_flag.rsrust/src/database/macros.rsrust/src/lib.rsrust/src/manager/wallet_manager/actor.rsrust/src/node.rsrust/src/node/client.rsrust/src/node/client/electrum.rsrust/src/node/client/esplora.rsrust/src/node/client_builder.rsrust/src/node_connect.rsrust/src/tor_runtime.rsrust/src/wallet_scanner.rs
Author
|
@praveenperera please approve workflow |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 62-63: The rustfmt CI job fails on fresh checkouts because it runs
`cargo fmt` before the repo is initialized; add the same "Set up repository"
step that runs `just setup` to the rustfmt job so the `rust/external/bdk` patch
paths in `rust/Cargo.toml` are populated; specifically, insert the step that
executes `just setup` (the same step used by other CI jobs) before the `cargo
fmt` step in the rustfmt job so formatting can run against a resolved workspace.
🪄 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: 43afeb97-297f-41bc-8433-fa4a68ac2ab6
📒 Files selected for processing (1)
.github/workflows/ci.yml
Sandipmandal25
suggested changes
Apr 23, 2026
Collaborator
Sandipmandal25
left a comment
Sandipmandal25
left a comment
There was a problem hiding this comment.
hey been going through this from some time 53 files and 10k lines is a lot tho in one shot especially since its touching some pretty core parts of the app (node connectivity, logging, the whole tor runtime). if you re open to it might be worth splitting into smaller pieces so it s easier to reason about each part. only if @praveenperera says!
went through a few of the main files and had some thoughts inline below. thanks!
Introduce Arti-backed Tor runtime integration, node client routing updates, and related Rust + BDK patch plumbing rusqlite to fix conflicts with arti.
Wire Kotlin settings and wallet flows for Tor mode selection, status display, and supporting Android-side Tor UI/resources.
Integrate SwiftUI Tor mode controls and status surfaces across settings and wallet views, including generated bindings and Tor support helpers.
Use structured Arti bootstrap status, Polish Tor quick status, SOCKS availability handling, custom onion node saves, and Tor disable fallback behavior.
Justxd22
force-pushed
the
feat/tor-support
branch
from
af57b5c to
540b35c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/database/global_config.rs (1)
356-383:⚠️ Potential issue | 🟠 MajorDatabase commit occurs before proxy shutdown—timeout failure leaves inconsistent state.
The current flow writes the config value to the database (line 376) before attempting to stop the built-in Tor proxy (line 378). If
request_stop_built_in_proxy()times out (returnsfalse), the error is returned but the database value is already committed. This leaves the system in an inconsistent state where the config says Tor is disabled but the proxy may still be running.Consider reversing the order: stop the proxy first, then commit the database change. This ensures the config only reflects the actual state.
♻️ Suggested reorder
pub(crate) fn set(&self, key: GlobalConfigKey, value: String) -> Result<()> { let should_stop_built_in_tor = match key { GlobalConfigKey::TorMode => !matches!(value.parse::<TorMode>(), Ok(TorMode::BuiltIn)), GlobalConfigKey::UseTor => value.eq_ignore_ascii_case("false"), _ => false, }; + // Stop proxy before committing config change to maintain consistency + if should_stop_built_in_tor && !crate::tor_runtime::request_stop_built_in_proxy() { + return Err(GlobalConfigTableError::BuiltInTorStop( + "timed out waiting for proxy shutdown".to_string(), + ) + .into()); + } + let write_txn = self.db.begin_write().map_err(|error| Error::DatabaseAccess(error.to_string()))?; // ... write and commit ... - - if should_stop_built_in_tor && !crate::tor_runtime::request_stop_built_in_proxy() { - return Err(GlobalConfigTableError::BuiltInTorStop( - "timed out waiting for proxy shutdown".to_string(), - ) - .into()); - } Updater::send_update(Update::DatabaseUpdated); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/global_config.rs` around lines 356 - 383, The code currently commits the DB change before attempting to stop the built-in Tor proxy, which can leave state inconsistent if request_stop_built_in_proxy() times out; fix this by deferring the write_txn.commit() until after a successful proxy shutdown: keep the begin_write()/table.insert(...) as-is (so the change is staged in the transaction), then call crate::tor_runtime::request_stop_built_in_proxy() and if it returns false return GlobalConfigTableError::BuiltInTorStop without calling commit, otherwise call write_txn.commit() and proceed; ensure you still compute should_stop_built_in_tor using GlobalConfigKey::TorMode/GlobalConfigKey::UseTor and TorMode::BuiltIn so the shutdown check logic remains unchanged.
♻️ Duplicate comments (4)
.github/workflows/ci.yml (1)
19-26:⚠️ Potential issue | 🟠 Major
rustfmtjob still skips repository initialization.Line 25 still runs
cargo fmtbeforejust setup, so fresh checkouts can fail when patched BDK paths are unresolved.Proposed fix
rustfmt: runs-on: ubuntu-latest steps: - name: Checkout sources uses: actions/checkout@v6 + - name: Set up just + uses: extractions/setup-just@v1 + + - name: Set up repository + run: just setup + - name: Install stable toolchain with rustfmt available uses: actions-rust-lang/setup-rust-toolchain@v1 with: toolchain: stable components: rustfmt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 19 - 26, The CI step "Run rustfmt" is executed before the repository is initialized causing format checks to fail on fresh checkouts; reorder the workflow so that the "Run rustfmt" step runs after the repository initialization step (e.g., after the step that runs the setup command `just setup`), or add an explicit step to run `just setup` before the "Run rustfmt" step; update the steps named "Install stable toolchain with rustfmt available" and "Run rustfmt" to ensure the toolchain install remains but rustfmt is invoked only after the `just setup`/repository initialization step so patched BDK paths are resolved.rust/src/node_connect.rs (1)
289-305:⚠️ Potential issue | 🟠 MajorClearnet reachability checks still bypass the configured Tor path.
When
config.use_tor()is true for a non-onion node, this branch still constructs the client withNodeClient::new(node). That skips the Tor-specific options you build below, so a Tor-enabled user can still validate a clearnet node over clearnet. Reuse the same tor-configuredNodeClientOptions/new_with_optionspath whenever global Tor is enabled.🤖 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 289 - 305, The check_node_with_tor_inference path uses NodeClient::new(node) for non-onion nodes even when config.use_tor() is true, which bypasses the Tor options; fix check_node_with_tor_inference so that when config.use_tor() returns true you build the Tor-enabled NodeClientOptions (the same options used later) and call NodeClient::new_with_options(options, node) (or equivalent constructor) instead of NodeClient::new(node), ensuring the Tor-configured client is used for clearnet nodes as well; reference functions/types: check_node_with_tor_inference, node_implies_tor, Database::global, config.use_tor, NodeClientOptions, NodeClient::new_with_options, NodeClient::new.ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift (1)
170-190:⚠️ Potential issue | 🟠 MajorCancelled node tasks can still mutate view state.
Tracking these tasks in
checkUrlTaskis only half the fix. After the awaited Rust calls return, each block still updates SwiftUI state unconditionally, so closing the loading popup or leaving the screen can still apply a late success/failure, clear drafts, orpopRoute(). Addtry Task.checkCancellation()/guard !Task.isCancelled else { return }before the post-await mutations in each block.Also applies to: 221-233, 256-266, 285-296
🤖 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 170 - 190, The Task assigned to checkUrlTask performs async work (nodeSelector.parseCustomNode / nodeSelector.checkAndSaveNode) then mutates SwiftUI state unconditionally; add cancellation checks (e.g., try Task.checkCancellation() or guard !Task.isCancelled else { return }) immediately after awaiting the async calls and before any state updates (calls to setCustomSelection, clearPendingNodeDraft, suppressCustomDraftActions = true, customUrl = "", customNodeName = "", completeLoading, app.popRoute, and the error-path completeLoading) so cancelled tasks do not apply late changes; apply the same fix to the other Task blocks referenced around the other ranges (the Tasks at the 221-233, 256-266, and 285-296 locations) ensuring each post-await section validates Task cancellation before mutating view state.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.kt (1)
203-265:⚠️ Potential issue | 🟠 MajorRedact custom node names from Logcat too.
The URL redaction is better, but Line 203, Line 233, Line 243, Line 254, Line 264, Line 290, Line 362, and the catch blocks still log raw
nodeName/selectedNodeName. Those can be user-entered custom labels, so this still leaks private node metadata into device logs. Emit only API type, onion/non-onion, and a short hash for names as well.Also applies to: 290-290, 362-379
🤖 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 203 - 265, The logs currently print raw user-provided node labels (e.g. nodeName/selectedNodeName) in selectPresetNode, selectCustomNodeType, checkAndSaveCustomNode and the related catch blocks; replace those raw prints with a redaction that emits only the node API type, whether the URL is onion/non-onion, and a short deterministic hash of the name. Implement or reuse a helper (e.g. redactedNameForLog(name): String) and use it in all Log.d/Log.e calls that currently interpolate nodeName/selectedNodeName (including inside selectPresetNode, the catch blocks for NodeSelectorException.* and Exception, selectCustomNodeType, and checkAndSaveCustomNode) so logs never contain the full label but do include apiType + onionFlag + shortHash.
🧹 Nitpick comments (7)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
75-80: Consider grouping pending node state into a data class.Five related
mutableStateOffields for pending node draft state could be consolidated into a single data class for cleaner state management and atomic updates:♻️ Optional refactor
data class PendingNodeDraft( val url: String = "", val name: String = "", val typeName: String = "", val awaitingTorSetup: Boolean = false, val torValidated: Boolean = false, ) // Replace the 5 fields with: var pendingNodeDraft by mutableStateOf(PendingNodeDraft())🤖 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/AppManager.kt` around lines 75 - 80, Group the five related mutable states (pendingNodeUrl, pendingNodeName, pendingNodeTypeName, pendingNodeAwaitingTorSetup, pendingNodeTorValidated) into a single immutable data holder by introducing a data class PendingNodeDraft(url:String="", name:String="", typeName:String="", awaitingTorSetup:Boolean=false, torValidated:Boolean=false) and replace the separate properties with one var pendingNodeDraft by mutableStateOf(PendingNodeDraft()); update all usages to read/write the single pendingNodeDraft and perform atomic copies when changing individual fields (e.g., pendingNodeDraft = pendingNodeDraft.copy(url = newUrl)).rust/crates/cove-common/src/logging.rs (1)
86-92: Consider returningResultor usinglock().ok()to avoid panics on poisoned mutex.The
expect("tor log buffer poisoned")calls at lines 87, 95, and 100 will panic if a previous thread panicked while holding the lock. For a logging subsystem that should be resilient, consider graceful degradation:♻️ Suggested approach
fn push_tor_log(line: String) { - let mut buffer = TOR_LOG_BUFFER.lock().expect("tor log buffer poisoned"); + let Ok(mut buffer) = TOR_LOG_BUFFER.lock() else { + return; // silently drop log if mutex poisoned + }; buffer.push_back(line); while buffer.len() > MAX_TOR_LOG_LINES { let _ = buffer.pop_front(); } }Apply similar pattern to
tor_connection_logs()(return emptyVec) andclear_tor_connection_logs()(no-op).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-common/src/logging.rs` around lines 86 - 92, The logging mutex currently uses expect(...) and will panic if poisoned; update push_tor_log, tor_connection_logs, and clear_tor_connection_logs to handle poisoned locks gracefully by using lock().ok() (or returning Result) instead of expect: in push_tor_log, skip pushing/trimming if lock() returns None (or return Err) so logging degrades without panicking; in tor_connection_logs return an empty Vec when the lock is poisoned; and in clear_tor_connection_logs make it a no-op when lock() is None (or return Ok(())), ensuring all three functions handle lock poisoning safely and consistently.android/app/src/main/java/org/bitcoinppl/cove/tor/TorStatusSupport.kt (1)
278-286:optBooleanreturnsfalsefor missing keys—distinguish missing fromfalse.
JSONObject.optBoolean(key)returnsfalseboth when the key is missing and when it's explicitlyfalse. This meanscaseInsensitiveBooleancannot distinguish between "key not found" and "key is false", which may cause incorrectisTordetection if the API response changes format.♻️ Suggested fix using has() check
private fun JSONObject.caseInsensitiveBoolean(key: String): Boolean? { val actualKey = keys().asSequence().firstOrNull { it.equals(key, ignoreCase = true) } - return actualKey?.let { optBoolean(it) } + return actualKey?.takeIf { has(it) }?.let { getBoolean(it) } }🤖 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/tor/TorStatusSupport.kt` around lines 278 - 286, The caseInsensitiveBoolean function currently uses optBoolean which returns false for absent keys, so change its logic to first locate the actual key (as in keys().asSequence().firstOrNull { it.equals(key, ignoreCase = true) }) and then check JSONObject.has(actualKey) to decide presence; if has(actualKey) return optBoolean(actualKey) else return null, leaving caseInsensitiveString unchanged; update the function named caseInsensitiveBoolean in TorStatusSupport.kt accordingly so callers can distinguish missing keys from an explicit false.rust/src/manager/wallet_manager/actor.rs (1)
1467-1479: Consider using explicit node fields instead of Debug format in signature.The signature uses
{node:?}which relies on theDebugimplementation. IfNode's Debug format changes or includes non-deterministic data (like internal state), the signature could become unstable, causing unnecessary client recreation.Consider explicitly formatting the relevant node fields:
fn node_client_signature_for(node: &Node) -> String { // ... config reads ... format!( - "node={node:?}|use_tor={use_tor}|tor_mode={tor_mode}|tor_external_host={tor_external_host}|tor_external_port={tor_external_port}" + "node_url={}|node_api={:?}|use_tor={use_tor}|tor_mode={tor_mode}|tor_external_host={tor_external_host}|tor_external_port={tor_external_port}", + node.url, node.api_type ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 1467 - 1479, The node_client_signature_for function currently embeds the entire Node via Debug ("{node:?}"), which can produce unstable signatures; replace that with an explicit, deterministic concatenation of Node's stable identifying fields (use the Node struct's public accessors or fields such as id, host/address, port, and any public key or auth identifier) instead of Debug formatting, keeping the rest of the signature (use_tor, tor_mode, tor_external_host, tor_external_port) unchanged so the signature remains stable and only reflects intended connection-relevant properties.ios/Cove/CoveMainView.swift (1)
542-555: Clean up duplicated guard and force unwrap.
- The
builtIncase is guarded twice (line 543 and again at line 547-548)- The force unwrap
host!on line 554 works but could be simplified♻️ Suggested simplification
let mode = TorMode.fromConfig(try? config.get(key: .torMode)) guard mode != .builtIn else { return } let endpoint: (host: String, port: Int) switch mode { - case .builtIn: - return case .orbot: endpoint = ("127.0.0.1", 9050) case .external: - let host = (try? config.get(key: .torExternalHost))? - .trimmingCharacters(in: .whitespacesAndNewlines) - endpoint = (host?.isEmpty == false ? host! : "127.0.0.1", Int(config.torExternalPort())) + let host = (try? config.get(key: .torExternalHost)) + .flatMap { $0.trimmingCharacters(in: .whitespacesAndNewlines) } + .flatMap { $0.isEmpty ? nil : $0 } ?? "127.0.0.1" + endpoint = (host, Int(config.torExternalPort())) + case .builtIn: + return // Already guarded above, but required for exhaustive switch }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveMainView.swift` around lines 542 - 555, Remove the duplicated builtIn early-return by keeping the existing guard (let mode = TorMode.fromConfig...) and deleting the redundant .builtIn case in the switch, and eliminate the force-unwrap of host by normalizing the external host with optional binding: read (try? config.get(key: .torExternalHost))?.trimmingCharacters(...), then set a finalHost via if let h = hostTrimmed, !h.isEmpty { finalHost = h } else { finalHost = "127.0.0.1" } and use finalHost when constructing endpoint (referencing TorMode.fromConfig, config.get(key: .torExternalHost), and the endpoint variable).android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.kt (1)
952-1008: Consider hoisting the blink animation to avoid redundant transitions.Each
TorStatusRowcreates its ownrememberInfiniteTransitionwith identical animation parameters (also duplicated from lines 280-291). Since all rows share the same blink timing, you could compute the alpha once and pass it as a parameter.♻️ Suggested refactor: pass blinkAlpha as parameter
`@Composable` private fun TorStatusRow( title: String, dot: TorStatusDot, subtitle: String, + blinkAlpha: Float, ) { - val blinkTransition = rememberInfiniteTransition(label = "torRowBlink") - val blinkAlpha = - blinkTransition.animateFloat( - initialValue = 0.35f, - targetValue = 1f, - animationSpec = - infiniteRepeatable( - animation = tween(durationMillis = 1400, easing = LinearEasing), - repeatMode = RepeatMode.Reverse, - ), - label = "torRowBlinkAlpha", - ) - val dotAlpha = if (dot == TorStatusDot.Yellow) blinkAlpha.value else 1f + val dotAlpha = if (dot == TorStatusDot.Yellow) blinkAlpha else 1f // ... rest unchangedThen at the call site (lines 587-589), pass
torBusyBlinkAlpha.valuefrom the existing transition.🤖 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/SelectedWalletFlow/SelectedWalletScreen.kt` around lines 952 - 1008, TorStatusRow currently creates its own rememberInfiniteTransition (blinkAlpha) duplicated across rows; change TorStatusRow signature to accept a blinkAlpha: Float parameter (or blinkAlphaValue) and remove the internal rememberInfiniteTransition and blinkAlpha variables, using the passed blinkAlpha to compute dotAlpha; then update callers of TorStatusRow to pass the shared torBusyBlinkAlpha.value (or equivalent) from the single rememberInfiniteTransition used elsewhere so all rows reuse the same animation timing.rust/src/node_connect.rs (1)
208-222: Usemap_err_strfor these string-wrapping conversions.These three
SetSelectedNodeErrormappings are the exact boilerplate the repo guideline asks to replace withmap_err_str(...).Based on learnings: "Applies to **/*.rs : 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 208 - 222, Replace the three occurrences of .map_err(|error| Error::SetSelectedNodeError(error.to_string())) with the repository utility map_err_str helper: call Result::map_err_str from cove_util::ResultExt (e.g., after database.global_flag.set_bool_config, database.global_config.set_use_tor, and database.global_config.set_selected_node) so you pass Error::SetSelectedNodeError as the variant constructor instead of manually converting to string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 47883-47903: The serializer for TorBootstrapException is
incomplete: read() correctly deserializes BuiltInTor with
FfiConverterString.read(buf) but allocationSize() returns only 4UL and write()
never writes the payload. Fix allocationSize(value: TorBootstrapException) to
return 4UL plus the payload size for BuiltInTor (e.g. 4UL +
FfiConverterString.allocationSize(value.message) when value is BuiltInTor) and
update write(value: TorBootstrapException, buf: ByteBuffer) so the BuiltInTor
branch calls buf.putInt(1) then FfiConverterString.write(value.message, buf);
keep the when as expression for exhaustiveness and preserve ULong return types.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.kt`:
- Around line 312-316: The current flow sets builtInWarmupRequested = true
before calling ensureBuiltInTorBootstrap(), so any failure leaves the flag set
and prevents retries; modify the logic in SelectedWalletScreen where
builtInWarmupRequested, ensureBuiltInTorBootstrap(), and builtInEndpoint are
used so the flag is only set on success (move builtInWarmupRequested = true into
the onSuccess block where builtInEndpoint is assigned) or implement a
retry/backoff around ensureBuiltInTorBootstrap() and only mark
builtInWarmupRequested true after a successful bootstrap to allow transient
failures to be retried.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.kt`:
- Around line 148-153: The Tor settings UI is hidden for fresh installs because
torSettingsDiscovered uses
globalFlag.getBoolConfig(GlobalFlagKey.TOR_SETTINGS_DISCOVERED).getOrDefault(false);
change the default to true so first-run users can access NetworkSettingsScreen's
Tor section: update the remember block that defines torSettingsDiscovered (and
keep persistedUseTor logic) to use getOrDefault(true) or otherwise initialize
GlobalFlagKey.TOR_SETTINGS_DISCOVERED=true on first launch; this targets the
NetworkSettingsScreen
torSettingsDiscovered/persistedUseTor/globalFlag.getBoolConfig usage so the Tor
UI gate (the section guarded by torSettingsDiscovered) is no longer closed for
new installs.
In `@rust/src/node/client/electrum.rs`:
- Line 65: Replace any debug!/info! logs that print raw endpoints (e.g., the
debug!(url = %url, ... ) call and other occurrences around the ranges you noted)
so they do not emit node_url, url, tor_host, or endpoint values; instead compute
and log (1) a tor_mode boolean or enum (true if Tor feature/connector is used),
(2) the API/type string (e.g., "electrum"), (3) an onion/non-onion flag (e.g.,
url contains ".onion"), and (4) a short non-reversible ID derived from the
endpoint (e.g., SHA-256 or blake2 hash of url/node_url/endpoint truncated to N
hex chars) — update all debug!/info! usages in electrum.rs (including the debug
at the top and the sections around 87-115 and 355-357) to use these fields
instead of printing raw network endpoints and ensure the ID generation is
deterministic but not reversible so endpoints are not leaked.
---
Outside diff comments:
In `@rust/src/database/global_config.rs`:
- Around line 356-383: The code currently commits the DB change before
attempting to stop the built-in Tor proxy, which can leave state inconsistent if
request_stop_built_in_proxy() times out; fix this by deferring the
write_txn.commit() until after a successful proxy shutdown: keep the
begin_write()/table.insert(...) as-is (so the change is staged in the
transaction), then call crate::tor_runtime::request_stop_built_in_proxy() and if
it returns false return GlobalConfigTableError::BuiltInTorStop without calling
commit, otherwise call write_txn.commit() and proceed; ensure you still compute
should_stop_built_in_tor using GlobalConfigKey::TorMode/GlobalConfigKey::UseTor
and TorMode::BuiltIn so the shutdown check logic remains unchanged.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 19-26: The CI step "Run rustfmt" is executed before the repository
is initialized causing format checks to fail on fresh checkouts; reorder the
workflow so that the "Run rustfmt" step runs after the repository initialization
step (e.g., after the step that runs the setup command `just setup`), or add an
explicit step to run `just setup` before the "Run rustfmt" step; update the
steps named "Install stable toolchain with rustfmt available" and "Run rustfmt"
to ensure the toolchain install remains but rustfmt is invoked only after the
`just setup`/repository initialization step so patched BDK paths are resolved.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.kt`:
- Around line 203-265: The logs currently print raw user-provided node labels
(e.g. nodeName/selectedNodeName) in selectPresetNode, selectCustomNodeType,
checkAndSaveCustomNode and the related catch blocks; replace those raw prints
with a redaction that emits only the node API type, whether the URL is
onion/non-onion, and a short deterministic hash of the name. Implement or reuse
a helper (e.g. redactedNameForLog(name): String) and use it in all Log.d/Log.e
calls that currently interpolate nodeName/selectedNodeName (including inside
selectPresetNode, the catch blocks for NodeSelectorException.* and Exception,
selectCustomNodeType, and checkAndSaveCustomNode) so logs never contain the full
label but do include apiType + onionFlag + shortHash.
In `@ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift`:
- Around line 170-190: The Task assigned to checkUrlTask performs async work
(nodeSelector.parseCustomNode / nodeSelector.checkAndSaveNode) then mutates
SwiftUI state unconditionally; add cancellation checks (e.g., try
Task.checkCancellation() or guard !Task.isCancelled else { return }) immediately
after awaiting the async calls and before any state updates (calls to
setCustomSelection, clearPendingNodeDraft, suppressCustomDraftActions = true,
customUrl = "", customNodeName = "", completeLoading, app.popRoute, and the
error-path completeLoading) so cancelled tasks do not apply late changes; apply
the same fix to the other Task blocks referenced around the other ranges (the
Tasks at the 221-233, 256-266, and 285-296 locations) ensuring each post-await
section validates Task cancellation before mutating view state.
In `@rust/src/node_connect.rs`:
- Around line 289-305: The check_node_with_tor_inference path uses
NodeClient::new(node) for non-onion nodes even when config.use_tor() is true,
which bypasses the Tor options; fix check_node_with_tor_inference so that when
config.use_tor() returns true you build the Tor-enabled NodeClientOptions (the
same options used later) and call NodeClient::new_with_options(options, node)
(or equivalent constructor) instead of NodeClient::new(node), ensuring the
Tor-configured client is used for clearnet nodes as well; reference
functions/types: check_node_with_tor_inference, node_implies_tor,
Database::global, config.use_tor, NodeClientOptions,
NodeClient::new_with_options, NodeClient::new.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt`:
- Around line 75-80: Group the five related mutable states (pendingNodeUrl,
pendingNodeName, pendingNodeTypeName, pendingNodeAwaitingTorSetup,
pendingNodeTorValidated) into a single immutable data holder by introducing a
data class PendingNodeDraft(url:String="", name:String="", typeName:String="",
awaitingTorSetup:Boolean=false, torValidated:Boolean=false) and replace the
separate properties with one var pendingNodeDraft by
mutableStateOf(PendingNodeDraft()); update all usages to read/write the single
pendingNodeDraft and perform atomic copies when changing individual fields
(e.g., pendingNodeDraft = pendingNodeDraft.copy(url = newUrl)).
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.kt`:
- Around line 952-1008: TorStatusRow currently creates its own
rememberInfiniteTransition (blinkAlpha) duplicated across rows; change
TorStatusRow signature to accept a blinkAlpha: Float parameter (or
blinkAlphaValue) and remove the internal rememberInfiniteTransition and
blinkAlpha variables, using the passed blinkAlpha to compute dotAlpha; then
update callers of TorStatusRow to pass the shared torBusyBlinkAlpha.value (or
equivalent) from the single rememberInfiniteTransition used elsewhere so all
rows reuse the same animation timing.
In `@android/app/src/main/java/org/bitcoinppl/cove/tor/TorStatusSupport.kt`:
- Around line 278-286: The caseInsensitiveBoolean function currently uses
optBoolean which returns false for absent keys, so change its logic to first
locate the actual key (as in keys().asSequence().firstOrNull { it.equals(key,
ignoreCase = true) }) and then check JSONObject.has(actualKey) to decide
presence; if has(actualKey) return optBoolean(actualKey) else return null,
leaving caseInsensitiveString unchanged; update the function named
caseInsensitiveBoolean in TorStatusSupport.kt accordingly so callers can
distinguish missing keys from an explicit false.
In `@ios/Cove/CoveMainView.swift`:
- Around line 542-555: Remove the duplicated builtIn early-return by keeping the
existing guard (let mode = TorMode.fromConfig...) and deleting the redundant
.builtIn case in the switch, and eliminate the force-unwrap of host by
normalizing the external host with optional binding: read (try? config.get(key:
.torExternalHost))?.trimmingCharacters(...), then set a finalHost via if let h =
hostTrimmed, !h.isEmpty { finalHost = h } else { finalHost = "127.0.0.1" } and
use finalHost when constructing endpoint (referencing TorMode.fromConfig,
config.get(key: .torExternalHost), and the endpoint variable).
In `@rust/crates/cove-common/src/logging.rs`:
- Around line 86-92: The logging mutex currently uses expect(...) and will panic
if poisoned; update push_tor_log, tor_connection_logs, and
clear_tor_connection_logs to handle poisoned locks gracefully by using
lock().ok() (or returning Result) instead of expect: in push_tor_log, skip
pushing/trimming if lock() returns None (or return Err) so logging degrades
without panicking; in tor_connection_logs return an empty Vec when the lock is
poisoned; and in clear_tor_connection_logs make it a no-op when lock() is None
(or return Ok(())), ensuring all three functions handle lock poisoning safely
and consistently.
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 1467-1479: The node_client_signature_for function currently embeds
the entire Node via Debug ("{node:?}"), which can produce unstable signatures;
replace that with an explicit, deterministic concatenation of Node's stable
identifying fields (use the Node struct's public accessors or fields such as id,
host/address, port, and any public key or auth identifier) instead of Debug
formatting, keeping the rest of the signature (use_tor, tor_mode,
tor_external_host, tor_external_port) unchanged so the signature remains stable
and only reflects intended connection-relevant properties.
In `@rust/src/node_connect.rs`:
- Around line 208-222: Replace the three occurrences of .map_err(|error|
Error::SetSelectedNodeError(error.to_string())) with the repository utility
map_err_str helper: call Result::map_err_str from cove_util::ResultExt (e.g.,
after database.global_flag.set_bool_config, database.global_config.set_use_tor,
and database.global_config.set_selected_node) so you pass
Error::SetSelectedNodeError as the variant constructor instead of manually
converting to string.
🪄 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: 1736b79d-089b-47db-ab5f-d33d36b8d885
⛔ Files ignored due to path filters (3)
ios/Cove/Assets.xcassets/iconTorOnion.imageset/onion.svgis excluded by!**/*.svgios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
.github/workflows/ci.yml.gitmodulesCONTRIBUTING.mdandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletContainer.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OrbotPackageHelper.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/TorNodeSupport.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/TorUiState.ktandroid/app/src/main/java/org/bitcoinppl/cove/tor/TorModeParsing.ktandroid/app/src/main/java/org/bitcoinppl/cove/tor/TorStatusSupport.ktandroid/app/src/main/java/org/bitcoinppl/cove/views/SettingsItem.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/res/drawable/icon_tor_onion.xmlandroid/app/src/main/res/values/strings.xmlios/Cove/AppManager.swiftios/Cove/Assets.xcassets/iconTorOnion.imageset/Contents.jsonios/Cove/CoveMainView.swiftios/Cove/Extention/Result+Ext.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/Flows/SettingsFlow/NetworkSettingsView.swiftios/Cove/Flows/SettingsFlow/SettingsContainer.swiftios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swiftios/Cove/Info.plistios/Cove/TorSupport.swiftios/Cove/WalletManager.swiftjustfilerust/Cargo.tomlrust/crates/cove-common/src/logging.rsrust/external/bdkrust/patches/bdk/0001-bdk-chain-rusqlite-sqlite-int64-conversions.patchrust/patches/bdk/0002-bdk-remove-workspace-lints-in-patched-crates.patchrust/patches/bdk/0003-bdk-chain-align-rusqlite-with-arti.patchrust/patches/bdk/0004-bdk-chain-allow-coverage-nightly-cfg.patchrust/src/database/global_config.rsrust/src/database/global_flag.rsrust/src/database/macros.rsrust/src/lib.rsrust/src/manager/wallet_manager/actor.rsrust/src/node.rsrust/src/node/client.rsrust/src/node/client/electrum.rsrust/src/node/client/esplora.rsrust/src/node/client_builder.rsrust/src/node_connect.rsrust/src/tor_runtime.rsrust/src/wallet_scanner.rs
✅ Files skipped from review due to trivial changes (12)
- .gitmodules
- rust/patches/bdk/0004-bdk-chain-allow-coverage-nightly-cfg.patch
- ios/Cove/Info.plist
- ios/Cove/Assets.xcassets/iconTorOnion.imageset/Contents.json
- ios/Cove/Flows/SettingsFlow/SettingsContainer.swift
- android/app/src/main/res/drawable/icon_tor_onion.xml
- rust/src/database/macros.rs
- rust/src/wallet_scanner.rs
- rust/patches/bdk/0002-bdk-remove-workspace-lints-in-patched-crates.patch
- CONTRIBUTING.md
- android/app/src/main/res/values/strings.xml
- rust/external/bdk
🚧 Files skipped from review as they are similar to previous changes (13)
- rust/src/database/global_flag.rs
- android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletContainer.kt
- ios/Cove/WalletManager.swift
- android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt
- rust/patches/bdk/0003-bdk-chain-align-rusqlite-with-arti.patch
- justfile
- rust/src/node.rs
- android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/TorUiState.kt
- ios/Cove/Flows/SettingsFlow/NetworkSettingsView.swift
- rust/src/lib.rs
- ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
- rust/patches/bdk/0001-bdk-chain-rusqlite-sqlite-int64-conversions.patch
- android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OrbotPackageHelper.kt
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
rust/src/node/client/electrum.rs (1)
107-133:⚠️ Potential issue | 🟠 MajorRedact
built_in_tor_statusbefore logging or returning it.Line 120 logs
built_in_status_summary()verbatim, and Lines 128-132 splice the same string into the returned error.rust/src/tor_runtime.rs:83-90currently formats that summary withendpoint=andlast_error=, so this path still leaks proxy details into logs and any surfaced error text. Please switch this to a redacted helper or only emit coarse state fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/node/client/electrum.rs` around lines 107 - 133, The built_in_tor_status string from crate::tor_runtime::built_in_status_summary() must be redacted before being logged or included in returned errors; update the electrum client error handling in Self::is_built_in_tor_bootstrap_socks_failure branch so that you call a redaction helper (e.g., add or use a function like crate::tor_runtime::redact_built_in_status or create one that strips endpoint/last_error details) and use that redacted value for the error! macro and when constructing the enriched Error::CreateElectrumClient message instead of the raw built_in_tor_status; ensure references to built_in_status_summary() only feed into the redactor, not directly into logs or returned strings.
🧹 Nitpick comments (1)
rust/src/lib.rs (1)
127-130: Add a blank line between setup and returnAdd a blank line after the endpoint binding in
ensure_built_in_tor_bootstrapfor the repo’s function-body spacing rule.Proposed refactor
async fn ensure_built_in_tor_bootstrap() -> Result<String, TorBootstrapError> { let endpoint = tor_runtime::built_in_socks_endpoint().await.map_err_str(TorBootstrapError::BuiltInTor)?; + Ok(endpoint.to_string()) }As per coding guidelines: "Add blank lines between logical steps in function bodies: after setup/result bindings ... before the next independent statement."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/lib.rs` around lines 127 - 130, In ensure_built_in_tor_bootstrap, insert a single blank line after the endpoint binding statement (the let endpoint = ... map_err_str(...)?; line) and before the Ok(endpoint.to_string()) return so the function follows the repo's function-body spacing rule separating setup/result bindings from the return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.kt`:
- Around line 355-367: The Tor status UI strings are hardcoded in
SelectedWalletScreen.kt (e.g., the TorMode.ORBOT branch that sets
torMessage/torConnection and uses TorStatusDot enums), which prevents
localization; move all hardcoded labels/messages/accessibility strings used
across the SelectedWalletScreen composable (including the examples around
TorStatusDot usage, torMessage assignments, and other occurrences noted at lines
400-419, 555-563, 583-635, 981-989) into Android resources (strings.xml), add
appropriate resource keys, and replace the literal strings with
stringResource()/context.getString(...) lookups in the composable; ensure you
keep placeholders (e.g., bootstrapPercent, bootstrapMessage) and accessibility
content descriptions as format strings in strings.xml so they are localized
correctly.
- Around line 170-176: parseEndpointHostPort currently splits on the first ':'
so bracketed IPv6 endpoints like "[::1]:39050" fail; update
parseEndpointHostPort to detect a leading '[' then find the matching ']' and
parse the host as the substring between '[' and ']' and the port as the
substring after the ':' that follows the closing ']', validate port with
toIntOrNull() and >0, otherwise fall back to the existing logic for
non-bracketed hosts; ensure the returned host does not include the brackets and
keep the function signature parseEndpointHostPort(endpoint: String):
Pair<String, Int>? unchanged.
In `@rust/src/lib.rs`:
- Around line 102-105: The BuiltInTor variant of TorBootstrapError is being
constructed with an error string that already includes the "failed to initialize
built-in tor proxy:" prefix (from the tor_runtime::Error mapping), causing the
message to be duplicated; change the code that maps tor_runtime::Error into the
BuiltInTor payload so it only passes the underlying error message (without the
prefix) or, better, store the original tor_runtime::Error (or its raw message)
in the BuiltInTor variant instead of a prefixed String so the
TorBootstrapError's #[error("failed to initialize built-in tor proxy: {0}")]
formatting adds the prefix exactly once (update any From/impl conversions that
create BuiltInTor accordingly).
---
Duplicate comments:
In `@rust/src/node/client/electrum.rs`:
- Around line 107-133: The built_in_tor_status string from
crate::tor_runtime::built_in_status_summary() must be redacted before being
logged or included in returned errors; update the electrum client error handling
in Self::is_built_in_tor_bootstrap_socks_failure branch so that you call a
redaction helper (e.g., add or use a function like
crate::tor_runtime::redact_built_in_status or create one that strips
endpoint/last_error details) and use that redacted value for the error! macro
and when constructing the enriched Error::CreateElectrumClient message instead
of the raw built_in_tor_status; ensure references to built_in_status_summary()
only feed into the redactor, not directly into logs or returned strings.
---
Nitpick comments:
In `@rust/src/lib.rs`:
- Around line 127-130: In ensure_built_in_tor_bootstrap, insert a single blank
line after the endpoint binding statement (the let endpoint = ...
map_err_str(...)?; line) and before the Ok(endpoint.to_string()) return so the
function follows the repo's function-body spacing rule separating setup/result
bindings from the return.
🪄 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: b0a2ba38-c7c0-4d57-a15e-53762ccb818b
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (5)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/src/lib.rsrust/src/node/client/electrum.rsrust/src/node/client/esplora.rs
…v6 SOCKS endpoints
Author
|
@praveenperera Ready!
|
Author
|
@praveenperera please allow ci |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Tor support end-to-end across the Rust core, Android, and iOS.
This supports three Tor modes:
Rust changes:
.onionnodes so onion Electrum nodes are checked and used through Tor.BDK submodule / patches:
Arti introduces
tor-dirmgr, which depends onrusqlite. Cove usesBDKwithrusqlite, and Cargo only allows one final crate withlinks = "sqlite3"in the dependency graph. Which caused conflicts, NowBDKis included as a submodule underrust/external/bdkand patched fromrust/patches/bdkbyjust setup, instead of relying on local Cargo registry hacks or maintaining a separate fork, most secure and effiecint wayAndroid changes:
iOS changes:
Testing
just setupjust citcp://xotqmhnei2wy7fk423tekp62ilcxpawnf4aiqmnkfhuutfkimgpqk5qd.onion:50001Platform Coverage
Checklist
closes #256
Summary by CodeRabbit
New Features
Bug Fixes
Documentation