gui: create PSBT with watch-only wallet

[Sjors]

For wallets with WALLET_FLAG_DISABLE_PRIVATE_KEYS this makes the watch-only balance available on the send screen (including coin selection). Instead of sending a transaction it generates a PSBT.

The user can take this PSBT and process it with HWI or put it an SD card for hardware wallets that support that.

The PSBT is copied to the clipboard. This was the easiest approach; we can add a dialog later to display it, as well as an option to save to disk.

[jb55]

Concept ACK

[nvk]

Would love to see this happen +1

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17518 (refactor, wallet: Nuke coincontrol circular dependency by hebasto)
• #17463 (Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr)
• #17457 (gui: Fix manual coin control with multiple wallets loaded by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[instagibbs]

concept ACK.

I do however think a proper dialogue will be required to let the user know what's going on at a minimum rather than magic resulting in a clipboard result.

very light testing also shows it appears it does what's on the tin. I'll do lots more testing later once approach is settled.

[gwillen]

So as some of you saw, I did some related work quite awhile ago. I have a dialog with support for creating/saving/loading/signing/sending these. It's somewhat modeled after the offline signing UI in Armory. I was never happy with the UI, and I think designing this to be intuitive is really hard, but I think it may already be an improvement over clipboard-only.

Let me spend a few minutes figuring out what the most recent version is that builds cleanly and works, and I will link it here so people can try it.

[promag]

Concept ACK.

[gwillen]

Ok, if you'd like to see my work on this, please have a look at https://github.com/gwillen/bitcoin/tree/feature-offline-v2 .

You will need to check preferences->wallet->enable offline transaction features. Then you'll get an 'Offline' menu, a new offline transactions dialog box, and a new pair of checkboxes in the 'send' dialog: "Include watchonly coins" and "create offline transaction" (which is forced on if you include watchonly). If you select the latter, the 'send' button becomes "Create unsigned" and will take you to the new dialog.

There is still some amount of jank to be ironed out, there are some XXX comments in the source and some missing error handling.

I've had this vaguely shelved for awhile, but I'd be happy to bring it out and work aggressively on it now if desired; I'd also be happy to bring it out less aggressively and start prepping it as a possible sequel to the PR under discussion here.

[Sjors]

@gwillen can you make a (draft) PR with that branch? If you rebase on top of this, you can probably drop the "Enable offline transaction features" checkbox as well as the "include watchonly coins" and "create unsigned "checkboxes, in favor of only allowing the functionality with watch-only wallets.

I like how the send button text changes, so I'll steal that for this PR :-)

Once we have descriptor wallets and better HWI integration, the simple distinction between watch-only and wallets with private keys will no longer work. For example you might be part of a 2 of 3 multisig, with the keys in the wallet. Or maybe you don't have keys in the Core wallet, but your hardware wallet can sign its part via USB.

So in terms of your wizard, I would split into two wizards: one for composing & exporting, and one for loading.

There's probably a step 0 where you attempt to (partially) sign with your own keys and with any connected devices. If that's not enough, then you go to step 1 where you can export the PSBT via clipboard or file system.

I would introduce a fresh dialog for loading, maybe co-signing and then broadcasting a transaction.

[Sjors]

Let's continue the UX brainstom here: #16954

[practicalswift]

I'm afraid the current version of this PR introduces a bug in CWallet::ListCoins.

First, note that == has higher precedence than the ternary conditional:

[cling]$ 1 == 2 ? 3 : 4
(int) 4
[cling]$ (1 == 2) ? 3 : 4
(int) 4
[cling]$ 1 == (2 ? 3 : 4)
(bool) false

Second, some context:

unsigned int IsMine(...);
bool IsWalletFlagSet(...);
int ISMINE_WATCH_ONLY = 1;
int ISMINE_SPENDABLE = 2;

This is being suggested in this PR:

bitcoin/src/wallet/wallet.cpp

Lines 2592 to 2593 in 41c5165

	if (depth >= 0 && output.n < it->second.tx->vout.size() &&
	IsMine(it->second.tx->vout[output.n]) == IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE) {

Shortened to highlight the issue:

if (... &&
    IsMine(it->second.tx->vout[output.n]) ==
      IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ?
      ISMINE_WATCH_ONLY : ISMINE_SPENDABLE) {

Which -- given the precedence rules -- is equivalent to:

if (... &&
    (IsMine(it->second.tx->vout[output.n]) ==
      IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) ?
      ISMINE_WATCH_ONLY : ISMINE_SPENDABLE) {

Which -- given that ISMINE_WATCH_ONLY and ISMINE_SPENDABLE are both non-zero integer constants (1 and 2) that will be evaluated in a boolean context -- is equivalent to:

if (... && true) {

Or simply:

if (...) {

:)

I think the intention was:

if (... &&
    IsMine(it->second.tx->vout[output.n]) ==
      (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ?
      ISMINE_WATCH_ONLY : ISMINE_SPENDABLE)) {

Could that be the case? :)

[instagibbs]

The final dialogue should have the "Yes" text replace with "Copy unsigned tx to clipboard" or something self-explanatory.

Really "Yes" should be "Send" in the average case anyways(think I might PR that change separately)

[Sjors]

Fixed the precedence bug @practicalswift found.

I added some additional copy:

With that I don't think it's necessary to show a dialog with the PSBT text. See also #16966 for a more rigorous approach (I'm trying to keep that an easy rebase), where it will be easier to integrate @gwillen's PSBT management changes (save to disk, check progress, etc).

[instagibbs]

"e.g., and offline Bitcoin Core wallet, or a PSBT-compatible hardware wallet"

I think that change would make me happy.

[Sjors]

@instagibbs done (I didn't update the screenshots)

[instagibbs]

tACK da86959

Ran both watch-only and non-watch-only modes, tested coin control in watch-only mode, tested a Core -> HWI -> Core round-trip and successfully finalized a PSBT with the desired properties.

[achow101]

Conecept ACK

It would be preferable if clicking "Copy to Clipboard" wouldn't close the dialog. Also, maybe show the psbt in a text box in the dialog too?

[achow101]

This doesn't need to be outside of the else block since it isn't used in the no private keys case.

[Sjors]

In the no private keys case we also clear the form. We just assume (with an assert) that PSBT creation can never fail, which is why send_failure is always false.

[Sjors]

The confirmation dialog is a QMessageBox, which isn't very flexible. #16966 is a better place to tweak things (and I want to avoid rebase hell there).