fix(spv): no-op ensure_address_imported when backend is SPV#839
Conversation
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>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request introduces input validation improvements and backend mode handling refinements. A character limit is added to private key input, password input width calculation now supports char-limit-driven measurement with a new text width utility, the address import logic early-exits in non-RPC modes, and Claude workspace directories are added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit c777f59) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/context/mod.rs (1)
709-716: Minor: clarify what actually feeds the SPV bloom filter.The phrase "
Wallet::register_address(which populatesknown_addressesand feeds the SPV bloom filter)" is a bit misleading. Looking atsrc/model/wallet/mod.rs:1162-1210,register_addressonly updatesknown_addresses/watched_addressesand persists to the DB; it does not callSpvManager::register_addresses(comparesrc/backend_task/dashpay/incoming_payments.rs:130-190, where bloom-filter registration is an explicit, separately-tracked step). SPV coverage for HD receive addresses comes from the SPV wallet manager watching the BIP44 account derived from the same xprv, as you note in the PR description — not fromregister_addressitself. Rewording to reflect that would prevent future readers from assumingregister_addressis the bloom-filter hook.📝 Suggested wording
/// In SPV mode this is a no-op: there is no Dash Core node to import into, /// and HD-derived addresses are already tracked by the SPV subsystem via - /// `Wallet::register_address` (which populates `known_addresses` and feeds - /// the SPV bloom filter). Incoming UTXOs on these addresses are delivered - /// through `received_transaction_finality()` from InstantLock/ChainLock - /// events — the same path the QR funding flows poll via - /// `capture_qr_funding_utxo_if_available`. + /// `Wallet::register_address` (which populates `known_addresses`). The SPV + /// wallet manager watches the BIP44 account derived from the same xprv, + /// so every address derived from that account is covered without a + /// per-address bloom-filter registration. Incoming UTXOs are delivered + /// through `received_transaction_finality()` from InstantLock/ChainLock + /// events — the same path the QR funding flows poll via + /// `capture_qr_funding_utxo_if_available`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/mod.rs` around lines 709 - 716, The doc comment incorrectly implies Wallet::register_address populates the SPV bloom filter; update the wording in the comment around Wallet::register_address to state it only updates known_addresses/watched_addresses and persists them, and explicitly note that SPV coverage is provided by the SPV wallet manager watching the BIP44 account (not by Wallet::register_address), while keeping the rest of the sentence about received_transaction_finality and capture_qr_funding_utxo_if_available intact; mention SpvManager::register_addresses as the separate code path that actually registers addresses with the bloom filter.src/ui/components/password_input.rs (1)
161-177: Font resolution for measurement should match TextEdit's actual font.
text_editis styled viaegui::TextStyle::Monospace(line 152), whose concreteFontIdis resolved fromui.style().text_styleswhen rendering. However, the measurement here uses hardcodedTypography::monospace()(SCALE_BASE = 16.0). If the active style or theme ever customizes theMonospaceorBodytext style sizes, the computeddesired_widthwill diverge proportionally from the actual rendered width.Low-impact today since the theme doesn't override those sizes and relies on egui's defaults, but resolving the font from the style would make this self-consistent and future-proof:
♻️ Optional: resolve the font from the active style
- let font_id = if self.monospace { - Typography::monospace() - } else { - Typography::body() - }; + let font_id = ui + .style() + .text_styles + .get(if self.monospace { + &egui::TextStyle::Monospace + } else { + &egui::TextStyle::Body + }) + .cloned() + .unwrap_or_else(|| { + if self.monospace { + Typography::monospace() + } else { + Typography::body() + } + });Also:
"W".repeat(limit)overestimates width for typical content (addresses, keys, passwords). Using the widest glyph as a safe upper bound is fine, but be aware the field will tend to be wider than strictly needed when sizingPASSWORD_INPUT_HORIZONTAL_PADDINGandPASSWORD_INPUT_REVEAL_ICON_WIDTH.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/password_input.rs` around lines 161 - 177, The width measurement uses hardcoded Typography::monospace()/Typography::body() instead of the actual FontId resolved from the current UI style, so change the measurement to resolve the FontId from ui.style().text_styles (e.g. ui.style().text_styles.get(&egui::TextStyle::Monospace).cloned().or_else(|| ui.style().text_styles.get(&egui::TextStyle::Body).cloned()).unwrap_or_else(|| Typography::monospace())) and pass that resolved FontId into Typography::measure_text_width when computing measured_width for the text_edit.desired_width; keep the existing sample generation logic (but you can keep "0".repeat(limit) for monospace and "W".repeat(limit) for proportional) so measurement matches the actual TextEdit font style.
🤖 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/ui/identities/keys/add_key_screen.rs`:
- Line 79: The char_limit on the private key input is incorrectly set to 66
while validate_and_add_key expects a 32-byte hex (64 hex chars); update the
three constructors (new, new_for_dashpay_encryption, new_for_dashpay_decryption)
to use .with_char_limit(64) and ensure the input hint remains "Private key
(hex)"; alternatively if WIF should be supported, update the validator in
validate_and_add_key and the hint text to reflect "WIF or 64-char hex"
instead—prefer tightening to 64 across the three constructors to match existing
codebase conventions.
---
Nitpick comments:
In `@src/context/mod.rs`:
- Around line 709-716: The doc comment incorrectly implies
Wallet::register_address populates the SPV bloom filter; update the wording in
the comment around Wallet::register_address to state it only updates
known_addresses/watched_addresses and persists them, and explicitly note that
SPV coverage is provided by the SPV wallet manager watching the BIP44 account
(not by Wallet::register_address), while keeping the rest of the sentence about
received_transaction_finality and capture_qr_funding_utxo_if_available intact;
mention SpvManager::register_addresses as the separate code path that actually
registers addresses with the bloom filter.
In `@src/ui/components/password_input.rs`:
- Around line 161-177: The width measurement uses hardcoded
Typography::monospace()/Typography::body() instead of the actual FontId resolved
from the current UI style, so change the measurement to resolve the FontId from
ui.style().text_styles (e.g.
ui.style().text_styles.get(&egui::TextStyle::Monospace).cloned().or_else(||
ui.style().text_styles.get(&egui::TextStyle::Body).cloned()).unwrap_or_else(||
Typography::monospace())) and pass that resolved FontId into
Typography::measure_text_width when computing measured_width for the
text_edit.desired_width; keep the existing sample generation logic (but you can
keep "0".repeat(limit) for monospace and "W".repeat(limit) for proportional) so
measurement matches the actual TextEdit font style.
🪄 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: a6b96f37-df56-4995-b3fe-7ab282efc245
📒 Files selected for processing (5)
.gitignoresrc/context/mod.rssrc/ui/components/password_input.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/theme.rs
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>
There was a problem hiding this comment.
Pull request overview
Fixes SPV-mode QR-based funding flows by preventing Core-RPC address import attempts when running with the SPV backend, and includes small UI/input ergonomics updates.
Changes:
- Make
AppContext::ensure_address_importeda no-op whencore_backend_mode != Rpcto unblock SPV-mode QR flows. - Add text-width measurement helper and use it to size
PasswordInputbased on character limits. - Add a 66-character limit to identity key “Private key (hex)” inputs; update
.gitignorefor Claude worktrees.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/context/mod.rs |
Early-return in SPV mode for ensure_address_imported, plus explanatory doc comment. |
src/ui/theme.rs |
Adds Typography::measure_text_width helper for egui font-metric sizing. |
src/ui/components/password_input.rs |
Uses char-limit-based text measurement to set TextEdit desired width; adds sizing constants. |
src/ui/identities/keys/add_key_screen.rs |
Applies a character limit to private key hex input fields. |
.gitignore |
Ignores .claude/worktrees. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ted 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>
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>
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…f 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>
…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>
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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
@/tmp/review_839_body.txt
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
This rerun reviewed commit 764977c0 specifically.
Overall: the PR fixes the user-visible SPV QR funding failure in ensure_address_imported(), but the same SPV receive-address path still contains one unconditional Core import helper. I also confirmed that the add-key UI still allows impossible 66-character "hex" private-key input even though validation only accepts 32 decoded bytes.
Findings:
-
src/context/mod.rs:739try_import_address()still performs Core RPC work in SPV mode.ensure_address_imported()now short-circuits outside RPC mode, butWallet::unused_bip_44_public_key()still reachestry_import_address()when deriving a fresh receive address. In SPV mode that means we still do a pointless synchronous Core RPC attempt whose error is silently discarded.register_address()already guards its own import path by backend mode, sotry_import_address()should do the same.
-
src/ui/identities/keys/add_key_screen.rs:79- The add-key forms allow 65-66 character hex private keys that can never validate.
- All three constructors use
with_char_limit(66), but submission only acceptshex::decode(...)output with decoded length exactly 32 bytes, i.e. 64 hex characters. The UI currently permits two extra characters that are guaranteed to fail later at submit time.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents independently reviewed the four-file delta from 764977c to c777f59 and found no new issues. The two prior findings (SPV guard in try_import_address, char limit in add-key forms) are both fixed on the current head. The follow-up password-input cleanup is a benign refactor with no correctness issues.
Reviewed commit: c777f59
Summary
Could not connect to Dash Core at 127.0.0.1:19998. The same failure path affects identity top-up via QR and the Create Asset Lock wallet tool.AppContextunconditionally called Dash Core RPC to watch newly-derived receive addresses. In SPV mode there is no Dash Core node to reach, so those calls failed withCoreRpcConnectionFailedand the user never saw the QR code:ensure_address_imported(src/context/mod.rs) — called viaget_address_info+import_addresstry_import_address(src/context/wallet_lifecycle.rs) — the async companion used in the wallet lifecycle pathOk(())whencore_backend_mode() != CoreBackendMode::Rpc. The import is unnecessary in SPV mode becauseWallet::receive_address -> unused_bip_44_public_key -> register_addressalready inserts the address intoknown_addresses; the SPV wallet manager (loaded from the same xprv viaWalletAccountCreationOptions::Default) watches the entire standard BIP44 account. Incoming UTXOs reachwallet.utxosthroughreceived_transaction_finality, which is the exact map the QR flow polls viacapture_qr_funding_utxo_if_available. Matches the precedent already inWallet::register_addressand the broadcast dispatcher shipped in fix(spv): disable features not available in spv mode #837.ensure_address_importeddoc comment to accurately describe the SPV UTXO delivery path (previously the comment described only the RPC path).Diff scope: 4 source files + 1
.gitignoreentry — SPV no-op guards, doc correction, and the PasswordInput secondary changes below.Secondary changes — PasswordInput improvements
While working in the password input area, several small correctness and quality issues were addressed:
8ba789db9a0e002915a069c0Typography::measure_text_widthnow acceptsimpl Into<String>instead ofString, eliminating one allocation per frame during rendering6e6cf2c4PASSWORD_INPUT_REVEAL_ICON_WIDTHconstant now drives theTextEditright-margin directly, removing a duplicated magic numberVerification performed
AddNewIdentityScreen,TopUpIdentityScreen,CreateAssetLockScreen) holdOption<Arc<RwLock<Wallet>>>—Walletis structurally HD-only (hasmaster_bip44_ecdsa_extended_public_key).SingleKeyWalletis a distinct type kept in a separateAppContext.single_key_walletsmap, so single-key wallets cannot reach these flows. No risk of silently hiding a single-key-in-SPV bug.m/9'/coinType'/5'/subfeature'/index. This PR only affects BIP44 funding addresses atm/44'/coinType'/0'/0/index, already covered byWalletAccountCreationOptions::Default.list_core_wallets/try_detect_core_wallet_for_addresssiblings. Audited all callers:add_new_wallet_screenandimport_mnemonic_screenhave explicit SPV short-circuits;wallets_screen/mod.rsandwith_wallet_recoveryonly fire onTaskError::CoreWalletNotConfigured, which in SPV mode is preempted byCoreRpcConnectionFailed. No sibling fixes required.Test plan
SPV QR funding (primary fix)
ensure_address_importedstill imports the address into Core's wallet (existing behaviour unchanged).PasswordInput secondary changes
cargo +nightly fmt --all -- --check→ clean.cargo clippy --all-features --all-targets -- -D warnings→ clean.🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements