QT: bump fee returns PSBT on clipboard for watchonly-only wallets #17492

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:gui_bump_psbt changing 1 files +21 −1

instagibbs commented at 9:09 PM on November 15, 2019: member

Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT.

quasi-companion to #16373
fanquake added the label GUI on Nov 15, 2019
instagibbs commented at 9:11 PM on November 15, 2019: member

Need to change the "Send" button text to whatever the parent PR is doing
jonasschnelli commented at 10:51 PM on November 15, 2019: contributor

Concept ACK
DrahtBot commented at 12:32 AM on November 16, 2019: member



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



Conflicts

No conflicts as of last run.
Sjors commented at 8:42 AM on November 16, 2019: member

Concept ACK. Linter complains about circular dependency.
instagibbs commented at 7:57 PM on November 16, 2019: member

@Sjors appears to be due to the forward declaration of SendCoinsRecipient in qt/guiutil.h, and is already somewhat covered in an exception: "qt/guiutil -> qt/walletmodel -> qt/optionsmodel -> qt/guiutil"

If my reading is correct I propose to add this particular path to the exception list in lieu of a larger refactor which should get rid of both the exceptions: "qt/guiutil -> qt/walletmodel -> qt/guiutil
instagibbs force-pushed on Nov 18, 2019
instagibbs commented at 2:21 PM on November 18, 2019: member

Removed the circular dependency(and removed a whitelisted one).
instagibbs renamed this:
~~[WIP] QT: bump fee returns PSBT on clipboard for watchonly-only wallets~~
QT: bump fee returns PSBT on clipboard for watchonly-only wallets
on Nov 18, 2019
instagibbs commented at 7:37 PM on November 19, 2019: member

I'll rebase onto master once #17513 (comment) is merged
DrahtBot added the label Needs rebase on Nov 21, 2019
instagibbs force-pushed on Nov 22, 2019
instagibbs commented at 8:33 PM on November 22, 2019: member

rebased onto master, split up into two commits for less conflict with https://github.com/bitcoin/bitcoin/pull/16373
DrahtBot removed the label Needs rebase on Nov 22, 2019
QT: bump fee returns PSBT on clipboard for watchonly-only wallets e3b19d8696
instagibbs force-pushed on Jan 7, 2020
instagibbs commented at 9:50 PM on January 7, 2020: member

rebased and ready for review now that RPC-version of this is merged
fanquake requested review from jonasschnelli on Jan 7, 2020
fanquake requested review from Sjors on Jan 7, 2020
in src/qt/walletmodel.cpp:531 in e3b19d8696 outdated
```
 526 | +    if (create_psbt) {
 527 | +        PartiallySignedTransaction psbtx(mtx);
 528 | +        bool complete = false;
 529 | +        const TransactionError err = wallet().fillPSBT(psbtx, complete, SIGHASH_ALL, false /* sign */, true /* bip32derivs */);
 530 | +        assert(!complete);
 531 | +        assert(err == TransactionError::OK);
```
Sjors commented at 6:53 AM on January 8, 2020:

Nit: check this first

instagibbs commented at 7:34 PM on January 13, 2020:

for the future or in case I touch this again: why?

gwillen commented at 11:31 PM on January 13, 2020:

The value of complete isn't necessarily meaningful if err is set, I would say, so in that case it's probably less misleading to show a failure message referencing err, where one referencing complete seems potentially very misleading.

gwillen commented at 11:40 PM on January 13, 2020:

... actually, for the record, assert()s here kind of scare me -- is that a practice we generally use? Would it be better to do as we do above, and put up a critical messagebox and return failure? Blowing up the process is drastic here; the failure seems well-contained even if surprising.

instagibbs commented at 2:27 PM on January 14, 2020:

pushed a commit to address the assert concern

Sjors commented at 2:36 AM on January 15, 2020:

Also because you changed that in the earlier PR too :-)
Sjors approved
Sjors commented at 6:55 AM on January 8, 2020: member

Tested ACK e3b19d869612b637f8bb702add0c363afe8adb8f

It would be nice if we can get the PSBT generation stuff in one place, so #17509 can build on that.
achow101 commented at 8:10 PM on January 13, 2020: member

Code review ACK e3b19d869612b637f8bb702add0c363afe8adb8f
gwillen commented at 11:46 PM on January 13, 2020: contributor
Code review ACK e3b19d8, nits/nice-to-haves:
- I kind of agree with Sjors that it would be nice to end up with all the PSBT-generation stuff in one place for further development on it.
- See my comment above about the asserts.
instagibbs commented at 2:28 PM on January 14, 2020: member

At the risk of invalidating 3 ACKs, I pushed a commit that converts the asserts to error message box alert instead.
gwillen commented at 4:20 PM on January 14, 2020: contributor

Thanks, code review ACK 267c77f.
Sjors commented at 2:43 AM on January 15, 2020: member

Concept ACK on using QMessageBox::critical instead of assert. Are you sure it's always safe to continue putting the PSBT on the clipboard after a failure?
gwillen commented at 2:45 AM on January 15, 2020: contributor

Concept ACK on using QMessageBox::critical instead of assert. Are you sure it's always safe to continue putting the PSBT on the clipboard after a failure?

Oh, indeed, there should be a "return false;" after the messagebox. This is what you get for making reasonable changes in response to review, Greg. :-(
QT: Change bumpFee asserts to simple error message 3c30d7118a
instagibbs force-pushed on Jan 15, 2020
instagibbs commented at 3:52 PM on January 15, 2020: member

fixed :)
Sjors commented at 4:52 PM on January 15, 2020: member

utACK 3c30d71
gwillen commented at 5:41 PM on January 15, 2020: contributor

code review ACK 3c30d71
achow101 commented at 7:49 PM on January 15, 2020: member

ACK 3c30d7118a5d5cb40c3686e0da884d3928caaeba

nit: squash, but 3 ACKs, so meh.
meshcollider commented at 1:43 AM on January 17, 2020: contributor

We now have essentially the same code in the bumpfee RPC, in the GUI when drafting a transaction, and now here when bumping in the GUI too. It would be nice to consolidate it a bit. If you want to leave it to a follow-up though I am happy to merge this with the current ACKs.
promag commented at 2:00 AM on January 17, 2020: member
Correct me if I'm wrong but I see 3 things that should be addressed later:
1. bumpFee should not be in the wallet model - could follow same approach as WalletControllerActivity - because IMO it's wrong to open message boxes (any GUI change) or copy to clipboard from a model class;
2. bumpFee should be asynchronous - 1. above would help here - if from the GUI thread we hit cs_main or cs_wallet then that's bad;
3. we keep adding nested event loops with QMessageBox helper functions - again 1. would help here.
instagibbs commented at 1:56 PM on January 17, 2020: member

@meshcollider I'm going to claim ignorance on how to best structure QT code and ask you merge this as-is. Sorry!
promag commented at 3:11 PM on January 17, 2020: member

Code review ACK 3c30d7118a5d5cb40c3686e0da884d3928caaeba.

I'll address my concerns in a follow up.
fanquake commented at 12:15 AM on January 22, 2020: member

Given the 4 ACKs and self-confessed Qt ignorance, I'm going to merge this. Will open an issue to capture all the items that need addressing as followup.
fanquake referenced this in commit 742f84d0de on Jan 22, 2020
fanquake merged this on Jan 22, 2020
fanquake closed this on Jan 22, 2020
sidhujag referenced this in commit 8a43f5f9df on Jan 24, 2020
sidhujag referenced this in commit 0cd2c36b07 on Nov 10, 2020
DrahtBot locked this on Feb 15, 2022