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

"PSBT Operations" dialog #18027

Open
wants to merge 11 commits into
base: master
from
Open

Conversation

@gwillen
Copy link
Contributor

gwillen commented Jan 30, 2020

Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.

This is based on @Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)

Some notes:

  • The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
  • I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
  • I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
@fanquake fanquake added the GUI label Jan 30, 2020
Copy link
Member

Sjors left a comment

Concept ACK

Code at b58e6f7 looks mostly alright, but Travis is very sad.

The first time I compiled this on macOS I get a cryptic error when I select the menu item:

2020-01-30 11:09:43.908 bitcoin-qt[619:453974] +[NSXPCSharedListener endpointForReply:withListenerName:]: an error occurred while attempting to obtain endpoint for listener 'com.apple.view-bridge': Connection interrupted

It worked fine the second time, so perhaps just some unrelated issue due to earlier code I compiled.

Schermafbeelding 2020-01-30 om 11 18 56

Ideally disable the sign button if there's nothing we can sign.

display status information is maybe unusual (a status bar, rather than messageboxes.)

I like it.

Currently this only changes the Load PSBT flow. Do you also want to apply this dialog to the send / bump fee flow? I'm fine with doing that later too.

You could also add a menu option to open a PSBT that's currently on the clipboard (e.g. handy when you need to save it as binary).

@@ -317,6 +317,8 @@ void BitcoinGUI::createActions()
signMessageAction->setStatusTip(tr("Sign messages with your Bitcoin addresses to prove you own them"));
verifyMessageAction = new QAction(tr("&Verify message..."), this);
verifyMessageAction->setStatusTip(tr("Verify messages to ensure they were signed with specified Bitcoin addresses"));
m_load_psbt_action = new QAction(tr("Load PSBT..."), this);

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 30, 2020

Member

You should pick a letter to turn into the keyboard shortcut (works on some operating systems, not macOS). E.g. "&Load PSBT..." if L` isn't taken.

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 1, 2020

Author Contributor

Just went with L -- it's hard for me to check what's going on here since I'm on a Mac, but I don't see it as being taken. (I didn't add a shortcut for loading from clipboard, since L is taken, C is taken, I could use P for PSBT but it seems kind of unnatural.

@@ -0,0 +1,52 @@
// Copyright (c) 2011-2014 The Bitcoin Core developers

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 30, 2020

Member

Nit: fix year

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 1, 2020

Author Contributor

Done.

@@ -14,7 +14,7 @@ std::string TransactionErrorString(const TransactionError err)
case TransactionError::OK:
return "No error";
case TransactionError::MISSING_INPUTS:
return "Missing inputs";
return "Inputs missing or spent";

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 30, 2020

Member

Cleaning these error messages could probably be its own commit.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2020

Author Contributor

Yeah, agreed, will fix.


switch (analysis.next) {
case PSBTRole::UPDATER: {
showStatus(tr("Transaction is missing some information about inputs."), StatusLevel::WARNING);

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 30, 2020

Member

I wonder if these strings can be moved to a place where the RPC can also use them.

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2020

Author Contributor

That would make sense actually, yeah. I could move them to wherever AnalyzePSBT is. What would the impact of this be on translation issues? I know that tr() is only used for GUI strings, and _() is used for non-GUI strings -- is there precedent for shared strings?

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 1, 2020

Member

I'm not sure actually. @laanwj, @hebasto?

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 1, 2020

Member

@gwillen

I know that tr() is only used for GUI strings, and _() is used for non-GUI strings -- is there precedent for shared strings?

Actually, bilingual_str type from util/translation.h supports both original and translated strings. More details are available in #16362 and #16224.

You could use something like this:

#include <util/translation.h>

bilingual_str bs = _("Transaction is missing some information about inputs.");
showStatus(bs.translated, StatusLevel::WARNING);
LogPrintf(bs.original);
return tx_description.toStdString();
}

void PSBTOperationsDialog::showStatus(const QString &msg, StatusLevel level) {

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 30, 2020

Member

Should we create a OperationsDialog class to make this stuff reusable? (can wait of course)

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2020

Author Contributor

I'm happy to go either way, are there other existing dialogs we'd immediately want to adopt it for? I do like it better than messageboxes, I tried to make it as drop-in as possible a replacement for them.


#include <iostream>

size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction &psbt) {

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 30, 2020

Member

Maybe move this to psbt.h?

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 30, 2020

Author Contributor

Can do, was dithering about what to do with it.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 30, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #18246 (wip: gui: Drop connectSlotsByName usage by promag)
  • #17509 (gui: save and load PSBT by Sjors)
  • #17457 (gui: Fix manual coin control with multiple wallets loaded by promag)
  • #17034 (Bip174 extensions by achow101)
  • #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
  • #16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
  • #14485 (Try to use posix_fadvise with CBufferedFile by luke-jr)

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.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Jan 30, 2020

Ideally disable the sign button if there's nothing we can sign.

Can I determine that without actually trying to sign? I was assuming that (1) I cannot, and (2) I shouldn't try to actually sign for real just to see if it works. (Or do you mean just disable the button at the point where we are asked to sign, try, and fail?)

Currently this only changes the Load PSBT flow. Do you also want to apply this dialog to the send / bump fee flow? I'm fine with doing that later too.

I'd rather do that separately, I am trying really hard to cure myself of the disease of biting off way more than I can reasonably do in one go.

You could also add a menu option to open a PSBT that's currently on the clipboard (e.g. handy when you need to save it as binary).

This I'd be happy to add, since it's a very small change. Just another item below "Load PSBT...", perhaps "Load PSBT from clipboard..."?

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 30, 2020

I indeed wouldn't try actually signing. But you e.g. tell if a wallet is watch-only, and the new ScriptPubKeyManager's can tell if they can sign for things.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Jan 30, 2020

Aha, and the Travis failure is the same issue we were talking about yesterday in #bitcoin-core-dev, I got scooped by #17261 and need to rebase again and fix errors, will do. EDIT: Oh, I think actually the Travis failure is just because master was already failing Travis at the moment I made the PR. So I think it will pass as soon as I push again.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Feb 1, 2020

Ok, I'm adding a check so that we do not enable the sign button (and we show a helpful note) if the current wallet is privateKeysDisabled().

I would like to use ScriptPubKeyMan to get a better verdict, but it doesn't seem like the functionality I need is quite there. It looks like what I want is something like CWallet::GetSigningProvider, which calls CanProvide on each ScriptPubKeyMan in the wallet; except that the implementation of LegacyScriptPubKeyMan::CanProvide looks like it can return true in cases where we can't actually sign, but "We can still provide some stuff if we have the script". So I can't tell if there's a call I can make that just answers the question "do we believe that this wallet can sign", without myself looping over all our ScriptPubKeyMans, and trying ProduceSignature with each with a DUMMY_SIGNATURE_CREATOR. Obviously if this is true, a better approach would be to enhance the CWallet/ScriptPubKeyMan interface to provide this, but I think that's best left for another PR. (Or if this is already provided and I'm not seeing how, please fill me in.)

@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Feb 1, 2020

Actually, here's an approach that seems like it should work -- FillPSBT already does basically all this work for us, since it does all the signing using a HidingSigningProvider, and allows us to tell it not to sign. So if I correctly understand the behavior of HidingSigningProvider, I think this should already tell us how many inputs we could sign, it just doesn't expose the result. If that seems correct to you, I can expose that from FillPSBT, and determine that way where there's anything for us to do here. @Sjors do you know whether this sounds correct?

EDIT: No, this does not work. LegacyScriptPubKeyMan::CanProvide is playing some clever games with ProduceSignature in order to answer the "could we sign" question, and I don't want to duplicate those clever games into the PSBT logic.

SON OF EDIT: Ok, a slightly different method does work -- in FillPSBT we can effectively check whether keys are available by whether pwallet->GetSigningProvider returns anything. This ultimately calls LegacyScriptPubKeyMan::CanProvide, which has the apparent issue I described above -- it says it returns true if we don't have keys, but we do have the script -- but I don't understand exactly what that means to try to produce an example that would confuse it.

I do at least have something that works on the obvious cases now.

@gwillen gwillen force-pushed the gwillen:feature-psbt-ops-dialog branch from b58e6f7 to 078e3ca Feb 1, 2020
@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Feb 1, 2020

Updated:

  • Disable sign button unless !privateKeysDisabled() and GetSigningProvider() != nullptr (I'm not sure how perfect a proxy that is for "we can sign" but it worked on my positive and negative test cases.)
  • Added "load from clipboard" option.
  • Added shortcut key in menu.
  • Separated error message fixes into their own commit.
  • Moved CountPSBTUnsignedInputs into psbt.h.

Did not change:

  • Send/bumpfee flow (would rather save for another PR)
  • Factoring out PSBT completeness description strings (not a bad idea, but could someone advise me on how that interacts with translation?)
  • Factoring out "OperationsDialog" (would probably rather save for another PR, and would like to understand where else we might use it first.)

Other notes:

  • Regarding the transient error Sjors got when clicking the menu item, all I can find when googling is that it's some transient issue that shows up for some people on OS X Catalina in multiple apps, with no obvious cause. I have no real insight there.

Please take another look?

@promag

This comment has been minimized.

Copy link
Member

promag commented Feb 3, 2020

Concept ACK.

@gwillen gwillen force-pushed the gwillen:feature-psbt-ops-dialog branch from 078e3ca to 3e7ca1e Feb 8, 2020
@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Feb 8, 2020

Speculatively attempting to fix the Windows-only fails without a Windows machine to test on. It looks like MinGW defines "ERROR" as a macro, so I can't give an enum constant that name. And it looks like the Visual Studio project file for building Bitcoin-QT is not autogenerated from the Makefiles, but has to be manually updated, so I've attempted to update it.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020
In FillPSBT, optionally report the number of inputs we successfully
signed, as an out parameter. If "sign" is false, instead report the
number of inputs for which GetSigningProvider does not return nullptr.
(This is a potentially overbroad estimate of inputs we could sign.)

Github-Pull: bitcoin#18027
Rebased-From: 665c8e6
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu
item, giving options to sign or broadcast the loaded PSBT as
appropriate, as well as copying the result to the clipboard or saving
it to a file.

Github-Pull: bitcoin#18027
Rebased-From: 3a9939c
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020
@Sjors
Sjors approved these changes Feb 10, 2020
Copy link
Member

Sjors left a comment

tACK 3e7ca1e (conditional on original PR being merged)

Would be nice to add change detection in the PSBT summary, but it can wait for a followup.

showStatus(tr("Transaction is missing some information about inputs."), StatusLevel::WARN);
break;
}
case PSBTRole::SIGNER: {

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2020

Member

When all inputs have "next": "finalizer" the transaction itself may still be "next": "signer". This seems to be a bug (@achow101 or feature?) in analyzepsbt. If it's a bug, we can ignore it for this PR, if it's a feature, we should handle it, though imo it can wait for a followup.

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 12, 2020

Author Contributor

I am going to assume this is a bug, so I'm happy to push to a followup. I have been tempted to try to rewrite the flow of AnalyzePSBT, with an eye to simplifying it and making its correctness easier to check.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 27, 2020

Member

Sounds like a bug

This comment has been minimized.

Copy link
@Sjors
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 10, 2020

Concept ACK!

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 12, 2020

Mmm, it's misinterpreting a 2-of-3 multisig with two signed outputs. Probably related to my earlier comment. This is more annoying though, because it means I can't broadcast without the third signature.
Schermafbeelding 2020-02-12 om 19 30 41

@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Feb 12, 2020

@Sjors are you able to supply me with an exact process to reproduce this? (Or an exact description -- is this a 2-of-3 using keys A, B, C, where both A and B have already signed, and you have C in your wallet, so it should be signable?)

I don't think I'm relying on analyzepsbt for this determination -- I will have to look back at the code, but I should be saying that we can sign as long as GetSigningProvider does not return nullptr. So this might just be a simple bug, or I might have misunderstood something fundamental about when we can and can't sign, but either way I'll take a look, thanks!

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 12, 2020

It's a watch-only wallet with a 2-of-3 multisig (three hardware wallets). Two of them signed.

You probably want to call finalize on a copy of the PSBT just to see if it's complete.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Feb 12, 2020

What is it misinterpreting? That is, what do you expect it to say? (If it's a watch-only wallet, then "this wallet does not have the right keys" should really say "this wallet cannot make signatures", because it's supposed to check privateKeysDisabled() first.)

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 12, 2020

The Broadcast Transaction button should not be disabled. I can finalize it with the RPC and testmempoolaccept is happy. The rest of the info is fine I suppose.

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Feb 27, 2020

concept ACK, will start reviewing in depth post-merge/rebase of #17509

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Feb 28, 2020

from a test run:

* Sends 47.99999859 BTC to bcrt1q6yp5np47xjpcn06fxmut4rug5nll4uh4xmefwn
* Sends 2.00000000 BTC to bcrt1qr5nel4sx76q953crm7vq3qjd6umgyzmlufyfaa

Do we really want to display change outputs? I guess it makes sense in coinjoiny contexts.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2020
In FillPSBT, optionally report the number of inputs we successfully
signed, as an out parameter. If "sign" is false, instead report the
number of inputs for which GetSigningProvider does not return nullptr.
(This is a potentially overbroad estimate of inputs we could sign.)

Github-Pull: bitcoin#18027
Rebased-From: 665c8e6
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2020
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu
item, giving options to sign or broadcast the loaded PSBT as
appropriate, as well as copying the result to the clipboard or saving
it to a file.

Github-Pull: bitcoin#18027
Rebased-From: 3a9939c
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2020
@gwillen gwillen force-pushed the gwillen:feature-psbt-ops-dialog branch from 3e7ca1e to c53c68e Mar 12, 2020
Sjors added 2 commits Nov 18, 2019
This commit does not change behavior.
@laanwj laanwj added the Feature label Mar 25, 2020
Sjors and others added 4 commits Mar 26, 2020
So it can be used in the GUI.
co-authored-by: Glenn Willen <gwillen@nerdnet.org>
co-authored-by: Glenn Willen <gwillen@nerdnet.org>
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 26, 2020

now is a good time to rebase since parent PR is likely merged without change in April

gwillen added 4 commits Feb 1, 2020
In FillPSBT, optionally report the number of inputs we successfully
signed, as an out parameter. If "sign" is false, instead report the
number of inputs for which GetSigningProvider does not return nullptr.
(This is a potentially overbroad estimate of inputs we could sign.)
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu
item, giving options to sign or broadcast the loaded PSBT as
appropriate, as well as copying the result to the clipboard or saving
it to a file.
@gwillen gwillen force-pushed the gwillen:feature-psbt-ops-dialog branch from c53c68e to af2df41 Mar 27, 2020
@gwillen

This comment has been minimized.

Copy link
Contributor Author

gwillen commented Mar 27, 2020

Tests segfault on my OS X machine, but it seems that they do that even if I undo my changes, so not going to worry about it too much for the moment...

Needed some fixups to make it build after rebase, which I am at this moment too lazy to rebase into their proper places in the list of commits, but will do that later before this is ready for merge.

(The crash is deep inside test_bitcoin BerkeleyDatabase::CreateMock() at db.h:151, when attempting to create a DbEnv. It seems like some kind of test infrastructure regression, or else my machine is on fire somehow (this is not unlikely). I'd love to know if anybody is seeing this elsewhere. All test cases that use WalletTestingSetup crash in this way when they try to create one.)

@DrahtBot DrahtBot removed the Needs rebase label Mar 27, 2020
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Mar 27, 2020

Travis macOS build is happy, as well as on my own machine (macOS 10.15.3)

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 27, 2020

Try a make clean cycle. I've had really bizarre db-related errors that go away after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.