"PSBT Operations" dialog

gwillen commented at 7:16 am on January 30, 2020: contributor

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 added the label GUI on Jan 30, 2020

in src/qt/bitcoingui.cpp:320 in b58e6f70f9 outdated

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

Sjors commented at 10:08 am on January 30, 2020:

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.

gwillen commented at 0:18 am on February 1, 2020:

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.

in src/qt/psbtoperationsdialog.h:1 in b58e6f70f9 outdated

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

Sjors commented at 10:09 am on January 30, 2020:

Nit: fix year

gwillen commented at 0:17 am on February 1, 2020:

Done.

in src/util/error.cpp:17 in b58e6f70f9 outdated

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

Sjors commented at 10:15 am on January 30, 2020:

Cleaning these error messages could probably be its own commit.

gwillen commented at 7:11 pm on January 30, 2020:

Yeah, agreed, will fix.

in src/qt/psbtoperationsdialog.cpp:239 in b58e6f70f9 outdated

233+void PSBTOperationsDialog::showTransactionStatus(const PartiallySignedTransaction &psbtx) {
234+    PSBTAnalysis analysis = AnalyzePSBT(psbtx);
235+
236+    switch (analysis.next) {
237+        case PSBTRole::UPDATER: {
238+            showStatus(tr("Transaction is missing some information about inputs."), StatusLevel::WARNING);

Sjors commented at 10:24 am on January 30, 2020:

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

gwillen commented at 7:11 pm on January 30, 2020:

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?

Sjors commented at 3:45 pm on February 1, 2020:

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

hebasto commented at 5:10 pm on February 1, 2020:

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

0#include <util/translation.h>
1
2bilingual_str bs = _("Transaction is missing some information about inputs.");
3showStatus(bs.translated, StatusLevel::WARNING);
4LogPrintf(bs.original);

in src/qt/psbtoperationsdialog.cpp:206 in b58e6f70f9 outdated

209+    }
210+
211+    return tx_description.toStdString();
212+}
213+
214+void PSBTOperationsDialog::showStatus(const QString &msg, StatusLevel level) {

Sjors commented at 10:26 am on January 30, 2020:

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

gwillen commented at 7:09 pm on January 30, 2020:

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.

in src/qt/psbtoperationsdialog.cpp:19 in b58e6f70f9 outdated

14+#include <qt/optionsmodel.h>
15+#include <util/strencodings.h>
16+
17+#include <iostream>
18+
19+size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction &psbt) {

Sjors commented at 10:33 am on January 30, 2020:

Maybe move this to psbt.h?

gwillen commented at 7:08 pm on January 30, 2020:

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

Sjors commented at 10:34 am on January 30, 2020: member

Concept ACK

Code at b58e6f70f99d650a8e6ef4a081641a332cf8748e 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:

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

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

DrahtBot commented at 11:52 am on January 30, 2020: member

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

Conflicts

No conflicts as of last run.

gwillen commented at 7:14 pm on January 30, 2020: contributor

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 commented at 7:17 pm on January 30, 2020: member

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 commented at 7:17 pm on January 30, 2020: contributor

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 commented at 0:36 am on February 1, 2020: contributor

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 commented at 0:48 am on February 1, 2020: contributor

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 force-pushed on Feb 1, 2020

gwillen commented at 2:58 am on February 1, 2020: contributor

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 commented at 0:51 am on February 3, 2020: member

Concept ACK.

gwillen force-pushed on Feb 8, 2020

gwillen commented at 0:27 am on February 8, 2020: contributor

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 referenced this in commit 9a135e775b on Feb 9, 2020

luke-jr referenced this in commit 95b10a8484 on Feb 9, 2020

luke-jr referenced this in commit 199b587b40 on Feb 9, 2020

in src/qt/psbtoperationsdialog.cpp:245 in 3a9939cf83 outdated

237+    switch (analysis.next) {
238+        case PSBTRole::UPDATER: {
239+            showStatus(tr("Transaction is missing some information about inputs."), StatusLevel::WARN);
240+            break;
241+        }
242+        case PSBTRole::SIGNER: {

Sjors commented at 3:20 pm on February 10, 2020:

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.

gwillen commented at 7:35 pm on February 12, 2020:

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.

achow101 commented at 9:36 pm on February 27, 2020:

Sounds like a bug

Sjors commented at 2:33 pm on February 28, 2020:

https://github.com/bitcoin/bitcoin/pull/18220

Sjors approved

Sjors commented at 3:22 pm on February 10, 2020: member

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.

laanwj commented at 4:18 pm on February 10, 2020: member

Concept ACK!

Sjors commented at 6:34 pm on February 12, 2020: member

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.

gwillen commented at 6:43 pm on February 12, 2020: contributor

@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 commented at 7:01 pm on February 12, 2020: member

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 commented at 7:04 pm on February 12, 2020: contributor

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 commented at 7:05 pm on February 12, 2020: member

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.

gwillen commented at 7:20 pm on February 12, 2020: contributor

Oh!! Sorry, of course. You’re right, I will look into why it’s doing that. For testing broadcastability, I am relying on FillPSBT’s “complete” output, which I thought was the right thing and should just work in this case. The actual text does rely on AnalyzePSBT, but the button should in theory still be enabled. I’ll figure that out.

Sjors commented at 7:32 pm on February 12, 2020: member

Mmm, actually it looks like I was testing on an outdated descriptor wallet branch (with your commits cherry-picked). I just tried on slightly more recent version and it now it recognized it as finished. So I think you’re right that FillPSBT did the job (it’s also a finalizer, as I learned the hard way in #18039).

gwillen commented at 7:33 pm on February 12, 2020: contributor

Oh, fantastic. Yeah, I have essentially ended up assuming that finalization is trivial and will always succeed given sufficient signatures, and I automatically attempt it before doing anything else.

jonasschnelli commented at 7:56 am on February 13, 2020: contributor

Concept ACK

DrahtBot added the label Needs rebase on Feb 25, 2020

instagibbs commented at 7:14 pm on February 27, 2020: member

concept ACK, will start reviewing in depth post-merge/rebase of https://github.com/bitcoin/bitcoin/pull/17509

instagibbs commented at 2:23 pm on February 28, 2020: member

from a test run:

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

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

luke-jr referenced this in commit 2b2355b541 on Mar 5, 2020

luke-jr referenced this in commit 505c398024 on Mar 5, 2020

luke-jr referenced this in commit 97b9e54591 on Mar 5, 2020

gwillen force-pushed on Mar 12, 2020

laanwj added the label Feature on Mar 25, 2020

instagibbs commented at 2:46 pm on March 26, 2020: member

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

gwillen force-pushed on Mar 27, 2020

gwillen commented at 8:00 am on March 27, 2020: contributor

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 removed the label Needs rebase on Mar 27, 2020

Sjors commented at 10:55 am on March 27, 2020: member

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

instagibbs commented at 1:35 pm on March 27, 2020: member

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

jb55 commented at 9:19 pm on April 14, 2020: member

Concept ACK. With this branch I no longer need to hit the command line to finalize a PSBT! Although I still need HWI to sign :(

It might be useful to mention if the transaction is already in the mempool or a block? I noticed it always says broadcast even if it already was broadcasted. Happy to defer these improvements to future PRs, along with change detection :)

jb55 commented at 9:25 pm on April 14, 2020: member

Another thing I noticed is that when I have the wrong wallet selected I get conflicting messages:

Apr14-142210

“is fully signed and ready for broadcast”

and

“Transaction as 1 unsigned inputs”

but this goes away with the right wallet selected:

Apr14-142431

instagibbs commented at 9:27 pm on April 14, 2020: member

Although I still need HWI to sign :(

There’s HWI-QT! ;)

meshcollider commented at 1:42 am on April 23, 2020: contributor

This can be rebased now #17509 is merged :)

DrahtBot added the label Needs rebase on Apr 23, 2020

instagibbs commented at 7:11 pm on April 24, 2020: member

I’d wait for rebase until #16528 is merged since it appears close

instagibbs commented at 2:32 pm on April 27, 2020: member

and now is the actual time to rebase

instagibbs commented at 7:06 pm on May 21, 2020: member

I could take on this PR to champion it if you’re too busy?

gwillen force-pushed on May 29, 2020

gwillen commented at 8:33 am on May 29, 2020: contributor

Rebased!

I think I still need to address comments from @jb55. I should probably also consider improving handling of change addresses (but I’d like to defer that to another PR if nobody objects.)

DrahtBot removed the label Needs rebase on May 29, 2020

instagibbs commented at 2:03 pm on May 29, 2020: member

Change address handling is something we can improve later

instagibbs commented at 2:05 pm on May 29, 2020: member

restarted an unrelated travis failure

in src/qt/psbtoperationsdialog.cpp:53 in 7169451646 outdated

47+{
48+    m_transaction_data = psbtx;
49+
50+    bool complete;
51+    size_t n_could_sign;
52+    TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */,  m_transaction_data, complete, &n_could_sign);

luke-jr commented at 10:45 pm on May 29, 2020:

Nit: Extra space before m_transaction_data

gwillen commented at 6:23 am on June 1, 2020: contributor

@jb55 I’d be up for holding back ‘in mempool / in block’ detection to a future PR (but I do think it should be in there for better UI, yeah.) Can you elaborate on what you mean by “right wallet” / “wrong wallet”? In theory once the tx is fully signed, it shouldn’t matter what wallet you’re using. So I’m confused about what’s happening there. Can you give me a more precise sequence of steps to reproduce?

This is fairly surprising to me because the ‘unsigned inputs’ text is based on PSBTInputSigned, which is a function of the PSBT itself and should not depend on the wallet at all. However loading a PSBT does call fillPSBT automatically, so it could mutate the PSBT in a wallet-dependent way somehow. But it’s called with sign=false, so that would seem like a bug, if it converted a PSBT that was “unsigned” to a PSBT that was “signed”.

If you are changing the wallet that’s open while the dialog is open – which I wouldn’t expect to be possible – that could definitely confuse the heck out of things. I assume you are closing and reopening it?

DrahtBot added the label Needs rebase on Jun 1, 2020

luke-jr referenced this in commit f998c5d5dd on Jun 9, 2020

luke-jr referenced this in commit 72dcd1bba3 on Jun 9, 2020

luke-jr referenced this in commit 8cbdc38578 on Jun 9, 2020

luke-jr referenced this in commit 8796c42fa0 on Jun 10, 2020

luke-jr referenced this in commit f70de9140b on Jun 10, 2020

luke-jr referenced this in commit 9df0a1b88a on Jun 10, 2020

Sjors commented at 4:56 pm on June 10, 2020: member

Needs rebase, mostly due to #19176

luke-jr referenced this in commit 1e048f32e7 on Jun 12, 2020

luke-jr referenced this in commit 2dc7e472c3 on Jun 12, 2020

luke-jr referenced this in commit b854c40f4a on Jun 12, 2020

in src/qt/walletview.cpp:233 in efde552570 outdated

236+            Q_EMIT message(tr("Error"), tr("PSBT file must be smaller than 100 MiB"), CClientUIInterface::MSG_ERROR);
237+            return;
238+        }
239+        std::ifstream in(filename.toLocal8Bit().data(), std::ios::binary);
240+        std::stringstream sstr;
241+        sstr << in.rdbuf();

Sjors commented at 12:31 pm on June 15, 2020:

nit: this refactor in how the file is read might deserve its own commit.

gwillen commented at 6:50 am on June 19, 2020:

I’m not sure how much sense that change makes in isolation – it was forced by the clipboard change, in which it is included, because when I lifted the definition of ‘data’ to an outer scope, I couldn’t figure out how to avoid changing the approach at the same time (since I was no longer able to use a non-default constructor of std::string.)

gwillen commented at 6:57 am on June 19, 2020:

… And actually the refactor isn’t necessary, I can do data = std::string(std::istreambuf_iterator<char>{in}, {}); and move-assignment should cause the copy to be elided so it’s free. So I’ll just change it to that.

in src/qt/psbtoperationsdialog.cpp:186 in 355e33f85a outdated

181+        // add total amount in all subdivision units
182+        tx_description.append("<hr />");
183+        QStringList alternativeUnits;
184+        for (const BitcoinUnits::Unit u : BitcoinUnits::availableUnits())
185+        {
186+            if(u != m_client_model->getOptionsModel()->getDisplayUnit())

Sjors commented at 12:38 pm on June 15, 2020:

nit: brackets

gwillen commented at 7:04 am on June 19, 2020:

good call, fixed

Sjors approved

Sjors commented at 12:44 pm on June 15, 2020: member

tACK 7169451646511964f5356a8859f696b827dabe50 modulo rebase

I tested with a fairly simple setup (single signature Trezor wallet and HWI). I might take some followups to make it completely idiot proof and handle multi-sig with perfect messaging.

Improve TransactionErrorString messages. 9e7b23b733

FillPSBT: report number of inputs signed (or would sign)

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

5dd0c03ffa

gwillen force-pushed on Jun 19, 2020

gwillen commented at 7:08 am on June 19, 2020: contributor

Rebasing across #19176, I turned a few instances of QString::fromStdString(TransactionErrorString(err)) into QString::fromStdString(TransactionErrorString(err).translated) to get things to build – I think that’s the right approach but let me know if it’s not.

gwillen force-pushed on Jun 19, 2020

DrahtBot removed the label Needs rebase on Jun 19, 2020

[gui] PSBT Operations Dialog (sign & broadcast)

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.

a6cb0b0c29

[gui] Load PSBT from clipboard 11a0ffb29d

Make lint-spelling.py happy 931dd47608

gwillen force-pushed on Jun 19, 2020

gwillen commented at 9:24 am on June 19, 2020: contributor

It seems like @jb55’s issue is consistent with FillPSBT not finalizing the transaction like I expected that it would, variably depending on which wallet is loaded. (@jb55, is one or both of the wallets a descriptor wallet by any chance?) FinalizePSBT calls SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, SIGHASH_ALL); on every input, whereas FillPSBT seems to bottom out in calls to SignPSBTInput(HidingSigningProvider([some signing provider], !sign, !bip32derivs), psbtx, i, sighash_type);. I would have hoped those would ultimately do the same thing, but this code is twisty, dark and deep… (And I’m nervous about the way that the ScriptPubKeyMan interface results in a somewhat complex web of signing logic getting semi-duplicated across each SPKM implementation. The wallet signing logic was just about beyond my comprehension already, before the addition of another layer.)

There are really two issues: (1) we’re trying to finalize the transaction when we load it, so that we know whether it’s ready or not, but failing. (2) our two information sources are not in sync (one of them we are calculating directly, the other we are getting from AnalyzePSBT, which appears to do its own finalization when checking if all signatures are present.)

I think I called FillPSBT because I wanted to know whether it thinks the PSBT is complete, and I assumed it would finalize in the process (and I think this might have been true before the SPKM refactor, though I’m not really very sure.)

So I think the fix to (1) is for me to add a call to FinalizePSBT in openWithPSBT before I call fillPSBT, which I have just done. And as for (2) I think it’s okay to ignore it for now (it’s not ideal that we can theoretically display conflicting information like this, but it’s purely a display issue and shouldn’t happen unless something else is buggy.)

The fix is harmless even if I’m wrong, so I think it’s okay to call it good at this point. But I would still really really like a repro test case to play with, @jb55. (If I have correctly deduced fillPSBT’s behavior, I find it surprising.)

Can we officially declare this PR done and ready (and pray that this is the last time it will need a rebase?) :-)

instagibbs commented at 1:31 pm on June 19, 2020: member

concept ACK fix, going to test again

instagibbs commented at 1:54 pm on June 19, 2020: member

tested ACK https://github.com/bitcoin/bitcoin/pull/18027/commits/931dd4760855e036c176a23ec2de367c460e4243

Tested across multiple Core wallets and HWI

Sjors commented at 3:14 pm on June 19, 2020: member

re-tACK 931dd4760855e036c176a23ec2de367c460e4243

in src/wallet/scriptpubkeyman.cpp:625 in 5dd0c03ffa outdated

619@@ -617,6 +620,14 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb
620         SignatureData sigdata;
621         input.FillSignatureData(sigdata);
622         SignPSBTInput(HidingSigningProvider(this, !sign, !bip32derivs), psbtx, i, sighash_type);
623+
624+        bool signed_one = PSBTInputSigned(input);
625+        if (n_signed && (signed_one || !sign)) {

achow101 commented at 6:43 pm on June 19, 2020:

The intent is to indicate whether signing happened, or whether we could sign. But this is really checking whether we could finalize, which may include signing. For instance, this wouldn’t work if the input were multisig.

But that might be ok for this PR.

To reduce code duplication, maybe this should go inside SignPSBTInput?

gwillen commented at 7:56 pm on June 19, 2020:

I’m very interested in a conversation about refactoring some of this stuff. Ok to save for another PR?

gwillen commented at 7:59 pm on June 19, 2020:

(I am interested both in (1) reducing code duplication and (2) working out more precisely whether we can sign something, which I found to be very difficult to actually compute, and I definitely recognize this as a conservative approximation.)

achow101 commented at 8:00 pm on June 19, 2020:

Ok to save for another PR?

Yep

jonatack commented at 7:13 pm on June 19, 2020: member

Concept ACK

achow101 commented at 7:40 pm on June 19, 2020: member

ACK 931dd4760855e036c176a23ec2de367c460e4243

jb55 commented at 4:45 am on June 20, 2020: member

ACK 931dd4760855e036c176a23ec2de367c460e4243

meshcollider commented at 10:31 am on June 21, 2020: contributor

Concept ACK, glanced at the code, looks RTM

meshcollider merged this on Jun 21, 2020

meshcollider closed this on Jun 21, 2020

in src/wallet/scriptpubkeyman.cpp:627 in 931dd47608

619@@ -617,6 +620,14 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb
620         SignatureData sigdata;
621         input.FillSignatureData(sigdata);
622         SignPSBTInput(HidingSigningProvider(this, !sign, !bip32derivs), psbtx, i, sighash_type);
623+
624+        bool signed_one = PSBTInputSigned(input);
625+        if (n_signed && (signed_one || !sign)) {
626+            // If sign is false, we assume that we _could_ sign if we get here. This
627+            // will never have false negatives; it is hard to tell under what i

jonatack commented at 9:25 am on June 22, 2020:

nit: s/what i/what/ here and at line 2136 below

in src/qt/walletview.cpp:14 in 931dd47608

10@@ -11,6 +11,7 @@
11 #include <qt/askpassphrasedialog.h>
12 #include <qt/clientmodel.h>
13 #include <qt/guiutil.h>
14+#include <qt/psbtoperationsdialog.h>

jonatack commented at 9:34 am on June 22, 2020:

nit: sort

jonatack commented at 9:47 am on June 22, 2020: member

Light code review and partially tested ACK 931dd4760855e036c176a23ec2de367c460e4243

Good to see this move forward and looking forward to continued progress as described in #18027 (comment). A couple of nits for the next PR follow, no need to respond to them.

MarcoFalke referenced this in commit dae1bd61b2 on Jun 24, 2020

luke-jr referenced this in commit 165a270dfb on Aug 15, 2020

PastaPastaPasta referenced this in commit 610b63c3bd on Jun 27, 2021

PastaPastaPasta referenced this in commit b9f60a69b6 on Jun 28, 2021

PastaPastaPasta referenced this in commit 0395c3b6fc on Jun 29, 2021

PastaPastaPasta referenced this in commit 6893b0c941 on Sep 17, 2021

DrahtBot locked this on Feb 15, 2022

“PSBT Operations” dialog #18027

Conflicts