fix(wallet): report public-only descriptors in signing wallets as watch-only - #7510
fix(wallet): report public-only descriptors in signing wallets as watch-only#7510PastaPastaPasta wants to merge 1 commit into
Conversation
…ch-only DescriptorScriptPubKeyMan::IsMine returned ISMINE_SPENDABLE for any script tracked by one of the wallet's descriptors, including descriptors that contain only public keys. In a wallet that otherwise has private keys, outputs on such a descriptor were treated as spendable balance and could be selected by coin selection, producing transactions the wallet cannot sign. A public-only descriptor can solve a script for fee estimation, but it cannot sign it. Report such scripts as ISMINE_WATCH_ONLY instead. Descriptor watch-only wallets and external-signer wallets (WALLET_FLAG_DISABLE_PRIVATE_KEYS) keep their existing ISMINE_SPENDABLE semantics; those wallets deliberately operate without local private keys. The dangerous case is a public descriptor mixed into a signing wallet, e.g. a contact's receiving chain imported for address discovery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 2 ahead in queue (commit 5004546) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5004546fc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // dangerous case is a public descriptor mixed into a signing wallet. | ||
| return HavePrivateKeys() || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) | ||
| ? ISMINE_SPENDABLE | ||
| : ISMINE_WATCH_ONLY; |
There was a problem hiding this comment.
When a private-key-enabled descriptor wallet receives an output matching one of these public-only descriptors, this branch removes the amount from the spendable balance but Dash Qt cannot display it as watch-only: interfaces::Wallet::haveWatchOnly() only queries LegacyScriptPubKeyMan (src/wallet/interfaces.cpp:248-254), and getBalances() consequently leaves all watch-only fields unset (src/wallet/interfaces.cpp:492-497); the descriptor branch of OverviewPage::setBalance also renders only the spendable fields. Thus the amount disappears entirely from the overview instead of being reported as watch-only. The descriptor-aware watch-only state and corresponding Qt balance presentation need to be updated with this classification change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding. I implemented the descriptor-aware HaveWatchOnly() path, getbalances/interfaces propagation, Qt watch-only columns, import notification, and regression coverage in amended commit c8576edb511. wallet_tests and the focused regression test pass. I could not update this PR head because it lives on PastaPastaPasta/dash and the available thepastaclaw credential has read-only access there (force-push rejected with 403); the PR owner needs to force-push/cherry-pick this amended commit.
|
Closing after more thought about sequencing: nothing in the current DashPay/Platform series actually depends on this — the friendship receiving chain imports as a private descriptor, and the contact's chain is never imported at all. The state this guards (a public-only descriptor inside a signing wallet) is unreachable via RPC/GUI today, so this is a guard rail without an in-tree consumer. If a future iteration imports xpub-only chains (e.g. locked-wallet contact acceptance), this will return alongside that change, where its motivation is adjacent and reviewable in context. |
2 participants
Issue being fixed or feature implemented
DescriptorScriptPubKeyMan::IsMinereturnsISMINE_SPENDABLEfor any script tracked by one of the wallet's descriptors — including descriptors that contain only public keys. In a wallet that otherwise has private keys, outputs on such a descriptor are treated as spendable balance and can be selected by coin selection, producing transactions the wallet cannot sign. A public-only descriptor can solve a script (enough for fee estimation), but it cannot sign it.Today this is hard to hit from the GUI/RPC because
importdescriptorsinto a private-key-enabled wallet normally imports descriptors with private keys, but any current or future path that tracks a public-only descriptor in a signing wallet (e.g. address discovery for an external party's chain) silently corrupts the wallet's view of its spendable balance. This is a prerequisite correctness fix extracted from the Platform GUI work in PastaPastaPasta#49, where a DashPay contact's receiving chain is exactly such a descriptor.What was done?
Report scripts from public-only descriptors as
ISMINE_WATCH_ONLYwhen the wallet is a signing wallet. Descriptor watch-only wallets and external-signer wallets (WALLET_FLAG_DISABLE_PRIVATE_KEYS) keep their existingISMINE_SPENDABLEsemantics — those wallets deliberately operate without local private keys and their spend paths handle signing elsewhere. The dangerous case is only a public descriptor mixed into a signing wallet.How Has This Been Tested?
New unit test
wallet_tests/public_descriptor_outputs_are_watch_only: imports a public-onlypkh(xpub/*)descriptor into a descriptor wallet with private keys enabled, and checks that a payment to a derived address is reportedISMINE_WATCH_ONLY, is excluded fromAvailableCoinsby default, and appears only whenfAllowWatchOnlyis set.wallet_tests,scriptpubkeyman_tests,ismine_tests, andcoinselector_testssuites pass locally (macOS, clang).Breaking Changes
None expected. Balances previously (mis)reported as spendable on such descriptors are now reported as watch-only.
Checklist:
🤖 Generated with Claude Code