Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: create PSBT with watch-only wallet #16944

Merged
merged 4 commits into from Nov 22, 2019

Conversation

@Sjors
Copy link
Member

Sjors commented Sep 23, 2019

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

This comment has been minimized.

Copy link
Contributor

jb55 commented Sep 23, 2019

Concept ACK

@nvk

This comment has been minimized.

Copy link

nvk commented Sep 23, 2019

Would love to see this happen +1

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 23, 2019

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

This comment has been minimized.

Copy link
Member

instagibbs commented Sep 23, 2019

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

This comment has been minimized.

Copy link
Contributor

gwillen commented Sep 23, 2019

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

This comment has been minimized.

Copy link
Member

promag commented Sep 23, 2019

Concept ACK.

@gwillen

This comment has been minimized.

Copy link
Contributor

gwillen commented Sep 23, 2019

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.

@fanquake fanquake changed the title UI: create PSBT with watch-only wallet gui: create PSBT with watch-only wallet Sep 24, 2019
@fanquake fanquake requested a review from achow101 Sep 24, 2019
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Sep 24, 2019

@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 :-)

Schermafbeelding 2019-09-24 om 13 49 21

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.

Schermafbeelding 2019-09-24 om 14 09 54

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

This comment has been minimized.

Copy link
Member Author

Sjors commented Sep 24, 2019

Let's continue the UX brainstom here: #16954

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Sep 25, 2019

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? :)

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved
src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Sep 25, 2019

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 Sjors mentioned this pull request Sep 25, 2019
1 of 2 tasks complete
@Sjors Sjors force-pushed the Sjors:2019/08/gui-send-psbt branch from 41c5165 to 0bd3e1c Sep 26, 2019
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Sep 26, 2019

Fixed the precedence bug @practicalswift found.

I added some additional copy:
Schermafbeelding 2019-09-26 om 11 15 42

Schermafbeelding 2019-09-26 om 11 30 25

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

This comment has been minimized.

Copy link
Member

instagibbs commented Sep 26, 2019

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

I think that change would make me happy.

@Sjors Sjors force-pushed the Sjors:2019/08/gui-send-psbt branch from 0bd3e1c to da86959 Sep 26, 2019
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Sep 26, 2019

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

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Sep 26, 2019

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.

Copy link
Member

achow101 left a comment

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?

send_failure = true;
}
}
if (!send_failure) {

This comment has been minimized.

Copy link
@achow101

achow101 Sep 30, 2019

Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Oct 1, 2019

Author Member

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

This comment has been minimized.

Copy link
Member Author

Sjors commented Oct 1, 2019

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).

@Sjors Sjors force-pushed the Sjors:2019/08/gui-send-psbt branch from 33a766f to f80b0b9 Nov 13, 2019
src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved
@Sjors Sjors force-pushed the Sjors:2019/08/gui-send-psbt branch 2 times, most recently from eedcbf7 to e0053c4 Nov 13, 2019
src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved
@Sjors Sjors force-pushed the Sjors:2019/08/gui-send-psbt branch from e0053c4 to c6dd565 Nov 13, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Nov 14, 2019

test and code review ACK c6dd565

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Nov 15, 2019

Concept ACK

@Sjors Sjors mentioned this pull request Nov 18, 2019
Copy link
Member

meshcollider left a comment

re-ACK c6dd565

meshcollider added a commit that referenced this pull request Nov 22, 2019
c6dd565 [gui] watch-only wallet: copy PSBT to clipboard (Sjors Provoost)
39465d5 [wallet] add fillPSBT to interface (Sjors Provoost)
848f889 [gui] send: include watch-only (Sjors Provoost)
40537f0 [wallet] ListCoins: include watch-only for wallets without private keys (Sjors Provoost)

Pull request description:

  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](https://github.com/bitcoin-core/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.

ACKs for top commit:
  instagibbs:
    test and code review ACK c6dd565
  meshcollider:
    re-ACK c6dd565

Tree-SHA512: ebc3da0737e33b255ed926191b84569aedb6097d14868662bd5dce726ce3048e86e9a31eba987b10dffe1482b35c21ae1cd595c2caa4634bc4cf78a826a83852
@meshcollider meshcollider merged commit c6dd565 into bitcoin:master Nov 22, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 22, 2019
c6dd565 [gui] watch-only wallet: copy PSBT to clipboard (Sjors Provoost)
39465d5 [wallet] add fillPSBT to interface (Sjors Provoost)
848f889 [gui] send: include watch-only (Sjors Provoost)
40537f0 [wallet] ListCoins: include watch-only for wallets without private keys (Sjors Provoost)

Pull request description:

  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](https://github.com/bitcoin-core/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.

ACKs for top commit:
  instagibbs:
    test and code review ACK bitcoin@c6dd565
  meshcollider:
    re-ACK c6dd565

Tree-SHA512: ebc3da0737e33b255ed926191b84569aedb6097d14868662bd5dce726ce3048e86e9a31eba987b10dffe1482b35c21ae1cd595c2caa4634bc4cf78a826a83852
@meshcollider meshcollider removed this from Blockers in High-priority for review Nov 23, 2019
@Sjors Sjors deleted the Sjors:2019/08/gui-send-psbt branch Nov 23, 2019
@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 24, 2019

I've used the "Use available balance" which filled the amount, but below balance is still zero.

Screenshot 2019-11-24 at 21 27 50

Something that can be fixed in a follow up - the button could change and/or the balance info on the bottom too.

Tested ACK c6dd565.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 25, 2019

@promag the balance seems unrelated, though obviously a bug

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 25, 2019

@promag fixed in #17587

meshcollider added a commit that referenced this pull request Nov 29, 2019
4a96e45 [gui] send: show watch-only balance in send screen (Sjors Provoost)
2689c8f [test] qt: add send screen balance test (Sjors Provoost)

Pull request description:

  Now that we can create a PSBT from a watch-only wallet (#16944), we should also display the watch-only balance on the send screen.

  Before:
  <img width="1008" alt="before" src="https://user-images.githubusercontent.com/10217/69533384-030e9180-0f78-11ea-9748-c32c957e822e.png">

  After:
  <img width="1009" alt="Schermafbeelding 2019-11-26 om 11 44 17" src="https://user-images.githubusercontent.com/10217/69622879-19811f80-1042-11ea-8279-091012f39b38.png">

  I added a test to check the balance on the send screen, but it only covers regular wallets. A better would add a watch-only only wallet.

ACKs for top commit:
  meshcollider:
    utACK 4a96e45
  jb55:
    utACK 4a96e45
  promag:
    reACK 4a96e45, rebased and label change since last review.
  instagibbs:
    code review and light test ACK 4a96e45

Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 30, 2019
4a96e45 [gui] send: show watch-only balance in send screen (Sjors Provoost)
2689c8f [test] qt: add send screen balance test (Sjors Provoost)

Pull request description:

  Now that we can create a PSBT from a watch-only wallet (bitcoin#16944), we should also display the watch-only balance on the send screen.

  Before:
  <img width="1008" alt="before" src="https://user-images.githubusercontent.com/10217/69533384-030e9180-0f78-11ea-9748-c32c957e822e.png">

  After:
  <img width="1009" alt="Schermafbeelding 2019-11-26 om 11 44 17" src="https://user-images.githubusercontent.com/10217/69622879-19811f80-1042-11ea-8279-091012f39b38.png">

  I added a test to check the balance on the send screen, but it only covers regular wallets. A better would add a watch-only only wallet.

ACKs for top commit:
  meshcollider:
    utACK 4a96e45
  jb55:
    utACK 4a96e45
  promag:
    reACK 4a96e45, rebased and label change since last review.
  instagibbs:
    code review and light test ACK bitcoin@4a96e45

Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.