Skip to content

fix(spv): disable features not available in spv mode#837

Merged
lklimek merged 12 commits into
v1.0-devfrom
fix/spv-rpc-path-audit
Apr 23, 2026
Merged

fix(spv): disable features not available in spv mode#837
lklimek merged 12 commits into
v1.0-devfrom
fix/spv-rpc-path-audit

Conversation

@lklimek
Copy link
Copy Markdown
Contributor

@lklimek lklimek commented Apr 22, 2026

User Story

Imagine you are running Dash Evo Tool in SPV mode. You open the wallet and try to send a payment, or click "Refresh" on a single-key wallet, or dig into the "Masternode List Diff" developer tool. Today, some of those silently try to call Dash Core RPC — even though you have no local node — and you get an opaque "ConnectionRefused" error or, worse, a transaction that drops into a black hole because the broadcast path unconditionally hit core_client.send_raw_transaction.

After this PR, SPV-mode users get a clean experience: the broadcast paths route through the shared backend-mode dispatcher, the operations that genuinely need Dash Core surface a typed error (TaskError::OperationRequiresDashCore), and the UI greys out affordances that can't work on SPV with a "Requires a local Dash Core node" tooltip instead of letting users fire a doomed action.

Summary

Addresses the documented "backend tasks ignore core_backend_mode()" bug class originally flagged in the PR #827 full-suite verification table. Seven focused commits, all off origin/v1.0-dev.

Backend fixes (audit round)

Commit Scope
902863dc New TaskError::OperationRequiresDashCore { operation: &'static str } typed variant for "needs Dash Core" errors. User-facing message is specific; Display/Debug stay separated.
8975d5d3 shielded/bundle.rs: inline core_client.send_raw_transaction → shared broadcast_raw_transaction() helper that already branches on core_backend_mode(). Plus finality-tracker cleanup on failure.
7e74e698 backend_task/core/send_single_key_wallet_payment.rs: same inline-RPC → helper swap. Dead _via_rpc wrapper removed.
4ee1a5a8 backend_task/core/refresh_single_key_wallet_info.rs: early-return OperationRequiresDashCore in SPV mode instead of letting the RPC call fail with ConnectionRefused. core_tasks::tc_003 / tc_009 now branch-assert on backend mode.
b29f416b backend_task/core/recover_asset_locks.rs: early-return empty RecoveredAssetLocks{0,0} in SPV mode — finality events already reconcile asset locks in SPV; explicit recovery is an RPC-only affordance.

UI fixes (preventive)

Commit Scope
99bec756 MasternodeListDiff developer tool: ToolsSubscreen::requires_core_rpc() predicate + ui.add_enabled gate + on_disabled_hover_text "Requires a local Dash Core node" tooltip. 2 new unit tests asserting it's the only RPC-gated tool today.
e118d2c3 Single-key wallet actions: disabled Send button + warning frame in both the wallet detail view (single_key_view.rs) and the dedicated send screen (single_key_send_screen.rs) when not in RPC mode. Receive stays enabled (local address display only).

RPC-required vs SPV-OK disambiguation (single-key wallet)

Action Needs Core RPC? In SPV
Send Yes (UTXO discovery + broadcast; SDK Account mismatch) Greyed out + tooltip + banner
Refresh Yes (RPC import_address + list_unspent) Already hidden + typed backend error
Receive No — cached address display Enabled
Rename No — local DB only Enabled
Asset-lock recovery HD-wallet feature, doesn't apply to single-key N/A

Coordination with #836

This branch is built off origin/v1.0-dev (not off #836). Consequences:

Limitations (intentional)

  • Single-key wallet SPV parity is a deeper architectural effort (SDK Account model doesn't yet represent arbitrary single keys, needs work in rust-dashcore). This PR ships the preventive UI + typed error; full parity is a follow-up.
  • Mid-session backend-mode switch (Expert user toggles SPV↔RPC via the combobox without restarting) still has pre-existing gaps — set_core_backend_mode_inner() doesn't manage ZMQ listener lifecycle. Partly pre-existing; partly made slightly more visible by feat: default to SPV connection; gate Dash Core RPC behind Expert mode #836. Worth a separate tracking issue; out of scope here.
  • MasternodeListDiff SPV reimplementation is not worth the effort — it's a developer diagnostic tool, correctly gated behind "Requires Dash Core" for now.

Test plan

  • `cargo build --all-features` — clean
  • `cargo clippy --all-features --all-targets -- -D warnings` — clean, zero warnings
  • `cargo +nightly fmt --all -- --check` — clean
  • `cargo test --all-features --workspace --lib --bins` — 457 passed, 0 failed, 2 ignored
  • `cargo test --doc --all-features --workspace` — 3 passed, 14 ignored
  • 2 new unit tests on `ToolsSubscreen::requires_core_rpc()` — assert `MasternodeListDiff` is the only RPC-gated tool today
  • Backend E2E on testnet: after feat: default to SPV connection; gate Dash Core RPC behind Expert mode #836 + this PR merge, re-run `core_tasks::tc_003/tc_006/tc_009` and `wallet_tasks::tc_014/tc_018` and update the failure tables in the tracking doc
  • Manual QA — SPV single-key wallet: Send button greyed out + tooltip visible; Receive still works; switching to RPC re-enables Send
  • Manual QA — SPV + Expert mode: MasternodeListDiff menu item greyed with "Requires Dash Core" tooltip; switching to RPC re-enables
  • Manual QA — shielded asset-lock broadcast under SPV: payment routes through shared broadcaster; no direct `send_raw_transaction` call, IS-lock arrival reconciles correctly

Related

  • #836 — "SPV is the default connection" flip. This PR is complementary.
  • #827 full-suite verification — originally flagged these failures as pre-existing.
  • #799 — asset-lock unification effort; adjacent to the `shielded/bundle.rs` change here.

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features

    • Added informational warnings and tooltips indicating operations requiring Dash Core node
    • Improved error messaging for unavailable features in light-wallet (SPV) mode
  • Bug Fixes

    • Enhanced transaction broadcast error handling with better mutex recovery
  • Style

    • Disabled send functionality and certain tools in SPV mode
    • Added warning banners on screens with Core-dependent operations

lklimek and others added 7 commits April 22, 2026 14:50
Introduces a typed TaskError for backend operations that cannot run
when the app is connected via SPV. The variant carries the static
operation name so the user-facing message is specific without any
string concatenation, and the `#[error(...)]` template keeps the
Display/Debug separation consistent with the rest of TaskError.

Used by follow-up commits that gate single-key wallet refresh on
Dash Core availability and will also back future UI "disable in SPV"
affordances.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…helper

The `shield_from_asset_lock()` bundle was reaching directly for
`app_context.core_client.read()?.send_raw_transaction(...)`, which
would unconditionally hit Dash Core even when the user had selected
SPV mode. In SPV the `core_client` is still constructed for
legacy/devtool reasons but there is no expectation that it can reach
a live `dashd` — the broadcast failed with `ConnectionRefused`.

Switch to the shared `AppContext::broadcast_raw_transaction()`
dispatcher (used by `create_registration_asset_lock`, `send_wallet_
payment`, `broadcast_and_commit_asset_lock`, etc.). It picks the
right path per `core_backend_mode()` — Core RPC or
`SpvManager::broadcast_transaction` — and keeps the finality tracker
leak-free by cleaning the waiting entry when the broadcast itself
fails.

Addresses the shielded half of the backend-E2E failures flagged for
`tc_014_wallet_platform_lifecycle` and `tc_018_fund_platform_address
_from_asset_lock` (`ConfirmationTimeout` in SPV mode triggered by the
RPC path never actually broadcasting in SPV).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`send_single_key_wallet_payment()` built and signed the transaction
locally but then dispatched the broadcast through
`core_client.send_raw_transaction()` without checking
`core_backend_mode()`. In SPV mode that blew up with
`ConnectionRefused` from the unused Core RPC socket instead of going
out on the wire via `SpvManager::broadcast_transaction`.

Replace the direct RPC call with the shared
`AppContext::broadcast_raw_transaction()` dispatcher, which routes
per backend mode. The rest of the function — UTXO selection and
signing from the wallet's in-memory cache — is unchanged and works
in both modes; the caller is now responsible for populating those
UTXOs (see `refresh_single_key_wallet_info`, still Core-RPC only).

Addresses the broadcast half of `core_tasks::tc_009_send_single_key_
wallet_payment`. The UTXO-refresh prerequisite is handled in a
follow-up commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t in SPV

`refresh_single_key_wallet_info()` relies exclusively on Core RPC
(`import_address`, `list_unspent`) to discover UTXOs for a
standalone WIF/private-key wallet. The SPV subsystem only tracks
addresses derived from HD-wallet account paths registered with its
bloom filter, so there is no SPV-side equivalent for an imported
single-key wallet.

Previously calling the task in SPV mode hit the unused Core RPC
socket and reported `ConnectionRefused` via a raw
`CoreRpcConnectionFailed` banner. Guard the task with an
early-return that produces the new
`TaskError::OperationRequiresDashCore` variant, so the UI shows an
actionable "connect to Dash Core and retry" message instead of a
connection-level error the user cannot map to the actual cause.

Update `core_tasks::tc_003_refresh_single_key_wallet_info` and the
`tc_009` pre-flight refresh to assert the mode-dependent outcome:
typed `OperationRequiresDashCore` in SPV, normal `RefreshedWallet`
in RPC.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`recover_asset_locks()` rescans the Core wallet's transaction and
UTXO indices for asset-lock payloads that weren't tracked locally.
In SPV mode none of the RPC queries it needs (`list_unspent`,
`get_raw_transaction`, `get_raw_transaction_info`) are available,
so the whole pass collapsed into a `ConnectionRefused` failure.

In SPV the reconciliation that this pass performs on RPC is handled
automatically by the InstantLock/ChainLock event stream — whenever
an asset-lock transaction arrives, `received_asset_lock_finality()`
populates `unused_asset_locks` and the DB row directly, so a
standalone "recover" pass has nothing to do. Return an empty
`RecoveredAssetLocks { 0, 0 }` result in SPV mode instead of
failing.

Addresses `core_tasks::tc_006_recover_asset_locks`, which expects
a `RecoveredAssetLocks` response in both backends (0 recoveries is
documented as a valid outcome in the test).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MasternodeListDiff inspector issues ~16 direct Dash Core RPC calls
(see audit section C-1 and `src/backend_task/mnlist.rs:81`). Running it
while the app is on its built-in SPV backend produces a storm of
connection-refused errors, so dim the entry in the Tools side panel and
show a tooltip explaining the constraint instead.

The check is a plain `core_backend_mode() == CoreBackendMode::Rpc`
comparison per the dim-the-menu design chosen for v1.0-dev; the
`FeatureGate::RpcBackend` predicate lands separately with #836.

Complements backend commits 8975d5d / 7e74e69 / 4ee1a5a / b29f416
which surface typed "requires Core" errors for wallet flows.
Per audit section C-2: single-key wallets are not in the SPV bloom
filter (no `Account` entry to feed addresses from), so `refresh` and
`send` fail even though broadcast itself can now route via
`broadcast_raw_transaction`. Commit 4ee1a5a already surfaces a typed
`OperationRequiresDashCore` error — this commit adds the matching UI
preventive step so users cannot reach that error from the GUI.

Changes:
* In the single-key wallet detail panel, grey out the Send button with
  a disabled-hover tooltip and surface an in-panel warning label
  explaining the constraint. Receive stays enabled — it only displays
  the cached local address.
* On the single-key send screen, prepend an info banner and disable the
  Send action with the same tooltip. Guards against users already sitting
  on the send screen when they switch the backend mode mid-session.

Raw `core_backend_mode() == CoreBackendMode::Rpc` comparisons per the
v1.0-dev pattern; `FeatureGate::RpcBackend` lands separately with #836.

Complements backend commits 8975d5d / 7e74e69 / 4ee1a5a / b29f416.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

📝 Walkthrough

Walkthrough

The changes add SPV (light-wallet) backend mode support by introducing mode-aware checks that prevent Core-dependent operations in SPV mode, returning appropriate errors or skipping functionality. UI components are updated to disable features requiring local Dash Core when in SPV mode, and a shared broadcast mechanism replaces direct RPC calls in transaction handling.

Changes

Cohort / File(s) Summary
Backend Task Mode Gating
src/backend_task/core/recover_asset_locks.rs, src/backend_task/core/refresh_single_key_wallet_info.rs
Added early SPV mode checks: recover_asset_locks returns success with zero counts; refresh_single_key_wallet_info returns OperationRequiresDashCore error. Both skip Core-dependent operations in SPV mode.
Transaction Broadcasting Refactor
src/backend_task/core/send_single_key_wallet_payment.rs
Removed RPC-specific helper method, inlined logic, and replaced direct Core RPC calls with async broadcast_raw_transaction() to use a shared broadcast mechanism.
Shielded Transaction Flow
src/backend_task/shielded/bundle.rs
Updated asset-lock transaction broadcast to use broadcast_raw_transaction() with improved finality-tracking cleanup and enhanced error handling for lock poisoning scenarios.
Error Handling
src/backend_task/error.rs
Added new error variant OperationRequiresDashCore { operation: &'static str } with user-facing message instructing users to switch to Dash Core in Settings.
UI Component Disabling
src/ui/components/tools_subscreen_chooser_panel.rs
Implemented ToolsSubscreen::requires_core_rpc() method and added mode-aware button disabling with hover tooltips for Core-dependent tools in SPV mode.
Single-Key Send Screen Gating
src/ui/wallets/single_key_send_screen.rs
Added RPC mode checks to gate Send button interactivity, force white label styling only when enabled, and render persistent warning banner for non-RPC mode.
Wallet View Enhancements
src/ui/wallets/wallets_screen/mod.rs, src/ui/wallets/wallets_screen/single_key_view.rs
Changed single_key_view module visibility to pub(crate) and added mode-aware UI gating: disabled Send button in SPV mode with warning banner and tooltip, exported SINGLE_KEY_REQUIRES_CORE constant.
Test Coverage
tests/backend-e2e/core_tasks.rs
Updated TC-003, TC-006 comment, and TC-009 to branch on backend mode: SPV paths assert OperationRequiresDashCore errors; RPC paths verify successful task execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 The backend hops both light and deep,
In SPV mode it takes a leap,
No Core to scan, no RPC to call,
Just warnings bright to one and all,
The buttons pause with gentle grace,
When Core's not in its proper place! 🌙

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main objective: disabling features unavailable in SPV mode through backend-mode-aware checks and UI updates.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/spv-rpc-path-audit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lklimek lklimek marked this pull request as ready for review April 22, 2026 13:20
@lklimek lklimek requested review from Copilot and thepastaclaw and removed request for Copilot April 22, 2026 13:21
@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Apr 22, 2026

⏳ Review in progress (commit a34970b)

Comment thread src/ui/wallets/wallets_screen/single_key_view.rs
Comment thread src/ui/wallets/single_key_send_screen.rs Outdated
Comment thread tests/backend-e2e/core_tasks.rs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Routes backend operations through backend-mode-aware dispatchers and prevents SPV users from triggering Core-RPC-only actions by surfacing typed errors and disabling unsupported UI affordances.

Changes:

  • Added a typed backend error (TaskError::OperationRequiresDashCore) and updated backend tasks to return/route appropriately in SPV mode.
  • Updated backend-e2e tests to assert mode-specific outcomes (success in RPC mode, typed failure/empty results in SPV mode).
  • Disabled/gated UI actions and tools that require Core RPC, with explanatory tooltips/banners in SPV mode.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/backend-e2e/core_tasks.rs Updates e2e test expectations to branch on backend mode and assert typed errors/early exits in SPV.
src/ui/wallets/wallets_screen/single_key_view.rs Disables single-key “Send” in SPV mode and adds an explanatory banner + tooltip.
src/ui/wallets/single_key_send_screen.rs Disables send flow in SPV mode and adds a warning banner + tooltip.
src/ui/components/tools_subscreen_chooser_panel.rs Gates Core-RPC-only dev tools in SPV mode and adds unit tests for the gating predicate.
src/backend_task/shielded/bundle.rs Routes broadcast through backend-mode-aware helper and cleans up finality tracking on broadcast failure.
src/backend_task/error.rs Introduces OperationRequiresDashCore typed error for Core-RPC-required operations.
src/backend_task/core/send_single_key_wallet_payment.rs Broadcasts via shared helper instead of direct RPC call; updates docs accordingly.
src/backend_task/core/refresh_single_key_wallet_info.rs Early-returns a typed error in SPV mode instead of failing with a connection error.
src/backend_task/core/recover_asset_locks.rs Returns an empty success result in SPV mode (since finality events reconcile asset locks).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ui/wallets/wallets_screen/single_key_view.rs Outdated
Comment thread src/ui/wallets/single_key_send_screen.rs Outdated
Comment thread src/ui/wallets/wallets_screen/single_key_view.rs Outdated
Comment thread src/backend_task/shielded/bundle.rs Outdated
- Consolidate duplicate SINGLE_KEY_REQUIRES_CORE copy between single_key_view
  and single_key_send_screen.
- Render the SPV-mode warning through MessageBanner instead of inline RichText /
  Frame styling (lklimek review feedback).
- Drop the dead CoreBackendMode::Rpc arm in backend-e2e tc_009 — backend-e2e is
  SPV-only, so only the typed-error path is exercised.
- Gate explicit button text color on enabled state so egui's disabled visuals
  apply correctly (Copilot review).
- Log a warning and recover through the poisoned guard when the
  transactions_waiting_for_finality mutex is poisoned, so finality tracking
  entries cannot leak silently across retries.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thepastaclaw
Copy link
Copy Markdown
Collaborator

I picked up the Copilot/Lukasz follow-up items in helper PR #838 so the fixes are reviewable separately without pushing to this branch directly.

thepastaclaw added a commit to thepastaclaw/dash-evo-tool that referenced this pull request Apr 22, 2026
- Extract shared SINGLE_KEY_REQUIRES_CORE_MESSAGE constant and a
  render_single_key_requires_core_banner() helper in ui::wallets so the
  "single-key wallets require Dash Core" notice lives in one place and
  uses MessageBanner-consistent warning styling.
- Replace the ad-hoc warning label in single_key_view.rs and the inline
  warning Frame in single_key_send_screen.rs with the shared helper.
- Dim the disabled Send button text colour in both screens so buttons
  visibly read as disabled when the app is in SPV mode (forcing the text
  colour was overriding egui's built-in disabled dimming).
- Simplify tests/backend-e2e/core_tasks.rs TC-003 and TC-009 to the
  SPV-only path: the harness always runs in SPV mode (see README), so
  the RPC branches were dead. TC-009 also drops the now-unnecessary
  funding transfer and 5s sleep since the test only asserts the typed
  OperationRequiresDashCore error from RefreshSingleKeyWalletInfo, with
  no actual send step to exercise.
- In backend_task/shielded/bundle.rs, switch the broadcast-failure
  cleanup of transactions_waiting_for_finality to try_lock() with a
  tracing::warn! fallback, matching the timeout-path cleanup and the
  existing pattern in src/context/transaction_processing.rs so a
  contended mutex never blocks the error return path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek requested a review from Copilot April 22, 2026 16:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Routes Core-dependent backend operations through backend-mode-aware helpers and improves SPV-mode UX by disabling UI actions/tools that require Dash Core RPC, returning a typed backend error instead of opaque connection failures.

Changes:

  • Introduces TaskError::OperationRequiresDashCore and uses it for single-key wallet refresh in SPV mode.
  • Routes raw-transaction broadcast paths through AppContext::broadcast_raw_transaction() (RPC/SPV dispatch), including shielded asset-lock broadcast cleanup.
  • Greys out/blocks SPV-incompatible UI actions (single-key wallet Send, Masternode List Diff tool) with explanatory tooltips/banners, and updates backend-e2e assertions accordingly.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/backend-e2e/core_tasks.rs Updates TC-003/TC-009 assertions to expect mode-specific typed errors for single-key wallets in SPV.
src/ui/wallets/wallets_screen/single_key_view.rs Disables single-key Send in SPV mode and shows a persistent warning banner + tooltip copy.
src/ui/wallets/wallets_screen/mod.rs Exposes single_key_view module for reuse of shared single-key warning copy.
src/ui/wallets/single_key_send_screen.rs Disables Send button in SPV mode and shows the same persistent warning/tooltip as the detail view.
src/ui/components/tools_subscreen_chooser_panel.rs Adds requires_core_rpc() gating and disables Masternode List Diff in SPV with a tooltip; adds unit tests.
src/backend_task/shielded/bundle.rs Uses broadcast_raw_transaction() for asset-lock tx broadcast; adds finality-tracker cleanup on broadcast failure/timeout.
src/backend_task/error.rs Adds TaskError::OperationRequiresDashCore typed error variant.
src/backend_task/core/send_single_key_wallet_payment.rs Switches broadcast to broadcast_raw_transaction() and removes RPC-only wrapper.
src/backend_task/core/refresh_single_key_wallet_info.rs Returns OperationRequiresDashCore early in SPV mode instead of attempting RPC calls.
src/backend_task/core/recover_asset_locks.rs Returns an empty RecoveredAssetLocks success result in SPV mode (no RPC scan).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/backend_task/shielded/bundle.rs Outdated
Comment thread src/ui/wallets/wallets_screen/mod.rs Outdated
The removed Rpc-arm assertions were the shape of the full single-key send
test. Keep them commented out next to the SPV typed-error check so the
intent and the assertion shape survive the SPV-only gap — when single-key
wallets gain SPV parity, un-comment the block.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend_task/error.rs`:
- Around line 576-579: The variant OperationRequiresDashCore currently composes
the user-facing message from the callsite-provided field operation; change its
Display text to a complete, static sentence and keep the operation field only
for debug context. Update the #[error(...)] attribute on
OperationRequiresDashCore to a fixed sentence such as "This feature is only
available when connected to Dash Core. Switch to Dash Core in Settings and
retry." and do not interpolate the operation field into the user-facing message
(leave the operation: &'static str field on the enum variant so Debug output
still includes it).

In `@src/backend_task/shielded/bundle.rs`:
- Around line 556-575: The timeout path uses try_lock() which can return
WouldBlock and skip removing tx_id; change the timeout branch in bundle.rs to
use lock() (like the other finality cleanup) on
app_context.transactions_waiting_for_finality so it always acquires the mutex
and removes the entry, but still handle Poisoned errors by calling into_inner()
on the poisoned guard and removing tx_id; ensure the function still returns the
same ShieldedAssetLockTimeout error after cleanup.

In `@src/ui/wallets/wallets_screen/single_key_view.rs`:
- Around line 12-15: The banner string SINGLE_KEY_REQUIRES_CORE is misleading —
it implies the whole wallet is unusable under SPV while Receive stays enabled;
update the constant value to clearly state only Core-dependent actions (e.g.,
sending, certain RPC features) require a Local Dash Core node and include the
concrete action "Open Settings → Expert mode → select Local Dash Core node."
Also update the other user-facing strings in this file that duplicate that
message (the banner and tooltip occurrences referenced at lines 72-75 and 91-96)
so they match the new wording and remain calm, direct, and brief.
🪄 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: c3e62913-e510-4c03-a6a0-6d04f99d9eef

📥 Commits

Reviewing files that changed from the base of the PR and between cd4b52f and 0c84294.

📒 Files selected for processing (10)
  • src/backend_task/core/recover_asset_locks.rs
  • src/backend_task/core/refresh_single_key_wallet_info.rs
  • src/backend_task/core/send_single_key_wallet_payment.rs
  • src/backend_task/error.rs
  • src/backend_task/shielded/bundle.rs
  • src/ui/components/tools_subscreen_chooser_panel.rs
  • src/ui/wallets/single_key_send_screen.rs
  • src/ui/wallets/wallets_screen/mod.rs
  • src/ui/wallets/wallets_screen/single_key_view.rs
  • tests/backend-e2e/core_tasks.rs

Comment thread src/backend_task/error.rs
Comment thread src/backend_task/shielded/bundle.rs Outdated
Comment thread src/ui/wallets/wallets_screen/single_key_view.rs Outdated
@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Apr 22, 2026

I updated helper PR #838 for the new Copilot follow-ups too: it now keeps the single_key_view module private again and hardens the shield timeout cleanup so the finality-tracking entry gets removed on timeout instead of being left behind on lock contention.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Routes backend operations through backend-mode-aware helpers so SPV users don’t hit Dash Core RPC paths, and updates UI to disable/tooltip actions that require a local Dash Core node.

Changes:

  • Add typed backend error TaskError::OperationRequiresDashCore and return it for RPC-only operations in SPV mode.
  • Route raw-tx broadcasting through AppContext::broadcast_raw_transaction() and clean up finality tracking on broadcast failure.
  • Grey out SPV-incompatible UI actions/tools with consistent tooltips/banners; update backend E2E assertions accordingly.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/backend-e2e/core_tasks.rs Update E2E expectations to assert mode-specific outcomes (typed errors / SPV no-op results).
src/backend_task/error.rs Add OperationRequiresDashCore typed error variant with user-facing message.
src/backend_task/core/refresh_single_key_wallet_info.rs Early-return typed error in SPV mode instead of attempting Core RPC.
src/backend_task/core/recover_asset_locks.rs In SPV mode return an empty recovery result instead of performing RPC-only scans.
src/backend_task/core/send_single_key_wallet_payment.rs Broadcast via broadcast_raw_transaction() instead of direct Core RPC.
src/backend_task/shielded/bundle.rs Switch asset-lock broadcast to backend-mode-aware broadcaster and add cleanup on broadcast failure.
src/ui/components/tools_subscreen_chooser_panel.rs Disable RPC-only tool(s) in SPV mode with hover tooltip; add unit tests for gating predicate.
src/ui/wallets/wallets_screen/single_key_view.rs Disable single-key Send in SPV mode; show persistent warning banner + tooltip copy constant.
src/ui/wallets/wallets_screen/mod.rs Export single_key_view module to reuse shared tooltip/banner copy.
src/ui/wallets/single_key_send_screen.rs Disable Send action in SPV mode; show persistent warning banner + tooltip.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/backend_task/shielded/bundle.rs Outdated
Comment thread src/ui/wallets/wallets_screen/single_key_view.rs Outdated
…g spam

The persistent SPV-mode warning on the single-key wallet detail view and
the single-key send screen was constructed as a fresh `MessageBanner`
every frame. Each fresh `BannerState` starts with `logged: false`, so
`process_banner()` emits a `tracing::warn!` once and then flips the
local flag — but the local is dropped at end of frame and the next frame
repeats the whole dance. Result: a banner warn per repaint (~60/sec when
egui is active) for the entire time the screen is visible on SPV.

Store the banner as a screen-struct field instead, populate it once per
SPV-mode entry (guarded by `has_message()`), clear it on mode change,
and disable auto-dismiss so the state notice persists without needing
the fresh-each-frame hack. Adds a small `MessageBanner::disable_auto_dismiss()`
helper since the component previously only exposed an overriding setter.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@thepastaclaw
Copy link
Copy Markdown
Collaborator

I updated helper PR #838 again for the latest Copilot/CodeRabbit follow-ups: it now uses a static full-sentence Dash Core error, guarantees timeout cleanup with a blocking lock, and rewords the single-key SPV banner so it only claims Core-dependent actions are unavailable while Receive still works.

lklimek and others added 2 commits April 22, 2026 19:22
…eout

On the asset-lock timeout path, `try_lock()` could return `WouldBlock`
and silently skip cleanup of `transactions_waiting_for_finality`. That
leaves a stale entry behind for a tx this flow has already abandoned,
so the finality listener keeps servicing it across retries/runs.

Switch to blocking `lock()` with the same poisoned-guard handling the
broadcast-failure branch already uses. The critical section is a single
`BTreeMap::remove`, so holding the std::sync Mutex across the await
boundary is bounded and matches the surrounding pattern.

Addresses copilot review on PR #837.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Narrow single_key_view module scope: re-export SINGLE_KEY_REQUIRES_CORE
  via pub(crate) use instead of publicising the whole submodule.
- Replace TaskError::OperationRequiresDashCore Display fragment with a
  complete sentence; keep `operation` as a Debug-only field so log
  output still identifies the originating callsite.
- Reword the single-key SPV banner to be action-specific
  (sending/refresh need Core; Receive still works) per coderabbit +
  copilot-pull-request-reviewer feedback.
@lklimek lklimek changed the title fix(spv): route backend operations through backend-mode-aware helpers and grey out unsupported UI in SPV mode fix(spv): disable features not available in spv mode Apr 23, 2026
@lklimek lklimek merged commit 5afd79e into v1.0-dev Apr 23, 2026
5 checks passed
@lklimek lklimek deleted the fix/spv-rpc-path-audit branch April 23, 2026 07:16
lklimek added a commit that referenced this pull request Apr 25, 2026
* fix(spv): no-op ensure_address_imported when backend is SPV

The QR funding flow for identity creation (and identity top-up, and the
Create Asset Lock wallet tool) called AppContext::ensure_address_imported
unconditionally to register the derived receive address with Dash Core's
wallet. In SPV mode there is no Dash Core RPC to talk to, so the call
failed with CoreRpcConnectionFailed(127.0.0.1:19998) and the user saw
"Could not connect to Dash Core" instead of a QR code.

The RPC import is not needed in SPV mode: Wallet::receive_address feeds
Wallet::register_address, which populates known_addresses and the SPV
bloom filter. Incoming UTXOs on those addresses are already delivered to
wallet.utxos via received_transaction_finality(), which is the exact map
the QR flow polls through capture_qr_funding_utxo_if_available().

Gate the RPC body of ensure_address_imported on
core_backend_mode() == CoreBackendMode::Rpc, matching the precedent
already in register_address (src/model/wallet/mod.rs) and the broadcast
dispatcher shipped in PR #837. Fixes the reported identity-creation-by-
QR bug and the same issue in identity top-up QR and Create Asset Lock.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: gitignore claude worktrees

* chore: set default password input width

* fix(ui): tighten private key input char limit from 66 to 64

A 66-character hex input decodes to 33 bytes, which always fails the
32-byte validator in validate_and_add_key(). Correct limit is 64 (32 bytes
hex), consistent with the hex-only validator and sibling screens.

Fixes finding from CodeRabbit review of PR #839.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs(context): correct SPV UTXO delivery path in ensure_address_imported doc

The doc comment incorrectly claimed that in SPV mode incoming UTXOs are
delivered via `received_transaction_finality()` (which is ZMQ-driven and
only available in Core/RPC mode) and that `Wallet::register_address` feeds
the SPV bloom filter (it only updates `known_addresses` in wallet state).

Replace both inaccuracies: SPV UTXOs are populated by `reconcile_spv_wallets()`
and HD-derived addresses are tracked by the SPV wallet manager watching the
BIP44 account xprv, not via per-address bloom-filter registration.

Fixes CodeRabbit nitpick (thread r3136009337) and Copilot finding
(thread r3136166317) from PR #839 review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(spv): no-op try_import_address when backend is SPV

Mirror the SPV early-return from ensure_address_imported. Previously,
every BIP44 address derivation called try_import_address which opened
an RPC client and failed silently in SPV mode — wasting resources on
every unused_bip_44_public_key call.

Fixes Copilot thread r3136077639 from PR #839 review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* perf(ui): avoid double-alloc in PasswordInput width measurement

Typography::measure_text_width cloned the sample string via to_owned()
even though the caller had already allocated it with repeat(limit).
Change the signature to consume the sample (impl Into<String>) so the
single allocation flows straight into egui's layout_no_wrap.

Fixes Copilot thread r3136166368 from PR #839 review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(ui): use PASSWORD_INPUT_REVEAL_ICON_WIDTH as margin source of truth

Previously the reveal-icon width appeared as both a typed constant
(f32 = 28.0) used for TextEdit desired_width and as a hardcoded
literal (right: 28) in the egui::Margin. Bind the latter to the
constant so the two cannot drift out of sync.

Fixes Copilot thread r3136235773 from PR #839 review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(ui): resolve FontId from active TextStyle in PasswordInput width measurement

Previously the width measurement used hardcoded Typography::monospace()/body()
while the TextEdit font was driven by TextStyle::Monospace. Any theme or
per-screen override of those TextStyles would cause the computed desired_width
to drift from the actual rendered width. Read the FontId from the active
style so measurement and rendering stay in lockstep.

Fixes Copilot thread r3136166359 and the related CodeRabbit nitpick on
src/ui/components/password_input.rs from PR #839 review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(ui): drop redundant monospace/proportional sample branch

In a monospace font every glyph has the same advance width, so
"0".repeat(limit) and "W".repeat(limit) measure identically. The
branch was doing defensive work that isn't needed. Collapse to the
proportional-safe "W" sample for both modes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants