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

instagibbs · 2019-11-15T21:09:26Z

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

quasi-companion to #16373

instagibbs · 2019-11-15T21:11:51Z

Need to change the "Send" button text to whatever the parent PR is doing

jonasschnelli · 2019-11-15T22:51:05Z

Concept ACK

DrahtBot · 2019-11-16T00:32:12Z

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

Conflicts

No conflicts as of last run.

Sjors · 2019-11-16T08:42:28Z

Concept ACK. Linter complains about circular dependency.

instagibbs · 2019-11-16T19:57:29Z

@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 · 2019-11-18T14:21:46Z

Removed the circular dependency(and removed a whitelisted one).

instagibbs · 2019-11-19T19:37:06Z

I'll rebase onto master once #17513 (comment) is merged

instagibbs · 2019-11-22T20:33:27Z

rebased onto master, split up into two commits for less conflict with #16373

instagibbs · 2020-01-07T21:50:21Z

rebased and ready for review now that RPC-version of this is merged

Sjors

Tested ACK e3b19d8

It would be nice if we can get the PSBT generation stuff in one place, so #17509 can build on that.

src/qt/walletmodel.cpp

achow101 · 2020-01-13T20:10:04Z

Code review ACK e3b19d8

gwillen · 2020-01-13T23:46:47Z

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 · 2020-01-14T14:28:05Z

At the risk of invalidating 3 ACKs, I pushed a commit that converts the asserts to error message box alert instead.

gwillen · 2020-01-14T16:20:24Z

Thanks, code review ACK 267c77f.

Sjors · 2020-01-15T02:43:36Z

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 · 2020-01-15T02:45:34Z

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. :-(

instagibbs · 2020-01-15T15:52:08Z

fixed :)

Sjors · 2020-01-15T16:52:54Z

utACK 3c30d71

gwillen · 2020-01-15T17:41:50Z

code review ACK 3c30d71

achow101 · 2020-01-15T19:49:20Z

ACK 3c30d71

nit: squash, but 3 ACKs, so meh.

meshcollider

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

Correct me if I'm wrong but I see 3 things that should be addressed later:

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;
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;
we keep adding nested event loops with QMessageBox helper functions - again 1. would help here.

instagibbs · 2020-01-17T13:56:33Z

@meshcollider I'm going to claim ignorance on how to best structure QT code and ask you merge this as-is. Sorry!

promag · 2020-01-17T15:11:52Z

Code review ACK 3c30d71.

I'll address my concerns in a follow up.

fanquake · 2020-01-22T00:15:06Z

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.

…ly wallets 3c30d71 QT: Change bumpFee asserts to simple error message (Gregory Sanders) e3b19d8 QT: bump fee returns PSBT on clipboard for watchonly-only wallets (Gregory Sanders) Pull request description: Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT. quasi-companion to #16373 ACKs for top commit: gwillen: code review ACK 3c30d71 promag: Code review ACK 3c30d71. Sjors: utACK 3c30d71 achow101: ACK 3c30d71 Tree-SHA512: 7a706141e46d7fd0ad513a08a96c16f2e7e531427a6776b689362f82e32cbd9d4b7eeb98f6936aa3f9347d23ccc94128516fcffa695efacd9cac43606ea916e2

…only-only wallets 3c30d71 QT: Change bumpFee asserts to simple error message (Gregory Sanders) e3b19d8 QT: bump fee returns PSBT on clipboard for watchonly-only wallets (Gregory Sanders) Pull request description: Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT. quasi-companion to bitcoin#16373 ACKs for top commit: gwillen: code review ACK 3c30d71 promag: Code review ACK 3c30d71. Sjors: utACK 3c30d71 achow101: ACK 3c30d71 Tree-SHA512: 7a706141e46d7fd0ad513a08a96c16f2e7e531427a6776b689362f82e32cbd9d4b7eeb98f6936aa3f9347d23ccc94128516fcffa695efacd9cac43606ea916e2

fanquake added the GUI label Nov 15, 2019

instagibbs force-pushed the gui_bump_psbt branch from 1cfd90c to b01be8e Compare November 18, 2019 14:21

instagibbs changed the title ~~[WIP] QT: bump fee returns PSBT on clipboard for watchonly-only wallets~~ QT: bump fee returns PSBT on clipboard for watchonly-only wallets Nov 18, 2019

This was referenced Nov 18, 2019

bumpfee: Return PSBT when wallet has privkeys disabled #16373

Merged

refactor, qt: Nuke some circular dependencies #17513

Merged

DrahtBot added the Needs rebase label Nov 21, 2019

instagibbs force-pushed the gui_bump_psbt branch from b01be8e to 6c81f47 Compare November 22, 2019 20:32

DrahtBot removed the Needs rebase label Nov 22, 2019

Sjors mentioned this pull request Nov 27, 2019

gui: "Increase transaction fee" label should be disabled for watch-only wallets #17263

Closed

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

e3b19d8

instagibbs force-pushed the gui_bump_psbt branch from 6c81f47 to e3b19d8 Compare January 7, 2020 21:49

fanquake requested review from jonasschnelli and Sjors January 7, 2020 22:15

Sjors approved these changes Jan 8, 2020

View reviewed changes

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved

QT: Change bumpFee asserts to simple error message

3c30d71

instagibbs force-pushed the gui_bump_psbt branch from 267c77f to 3c30d71 Compare January 15, 2020 15:51

meshcollider reviewed Jan 17, 2020

View reviewed changes

promag reviewed Jan 17, 2020

View reviewed changes

fanquake merged commit 3c30d71 into bitcoin:master Jan 22, 2020

fanquake mentioned this pull request Jan 22, 2020

gui: followups from #17492 #17979

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

instagibbs commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Nov 15, 2019

Uh oh!

jonasschnelli commented Nov 15, 2019

Uh oh!

DrahtBot commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Nov 16, 2019

Uh oh!

instagibbs commented Nov 16, 2019

Uh oh!

instagibbs commented Nov 18, 2019

Uh oh!

instagibbs commented Nov 19, 2019

Uh oh!

instagibbs commented Nov 22, 2019

Uh oh!

instagibbs commented Jan 7, 2020

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jan 13, 2020

Uh oh!

gwillen commented Jan 13, 2020

Uh oh!

instagibbs commented Jan 14, 2020

Uh oh!

gwillen commented Jan 14, 2020

Uh oh!

Sjors commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gwillen commented Jan 15, 2020

Uh oh!

instagibbs commented Jan 15, 2020

Uh oh!

Sjors commented Jan 15, 2020

Uh oh!

gwillen commented Jan 15, 2020

Uh oh!

achow101 commented Jan 15, 2020

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jan 17, 2020

Uh oh!

promag commented Jan 17, 2020

Uh oh!

fanquake commented Jan 22, 2020

Uh oh!

Uh oh!

instagibbs commented Nov 15, 2019 •

edited

Loading

DrahtBot commented Nov 16, 2019 •

edited

Loading

Sjors commented Jan 15, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading