Skip to content

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

Merged
fanquake merged 2 commits intobitcoin:masterfrom
instagibbs:gui_bump_psbt
Jan 22, 2020
Merged

QT: bump fee returns PSBT on clipboard for watchonly-only wallets#17492
fanquake merged 2 commits intobitcoin:masterfrom
instagibbs:gui_bump_psbt

Conversation

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Nov 15, 2019 •
edited
Loading

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

quasi-companion to #16373

@fanquake fanquake added the GUI label Nov 15, 2019
@instagibbs
Copy link
Member Author

instagibbs commented Nov 15, 2019

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

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Nov 15, 2019

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 16, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@Sjors
Copy link
Member

Sjors commented Nov 16, 2019

Concept ACK. Linter complains about circular dependency.

@instagibbs
Copy link
Member Author

instagibbs commented Nov 16, 2019

@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
Copy link
Member Author

instagibbs commented Nov 18, 2019

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

@instagibbs 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
Merged
Merged
@instagibbs
Copy link
Member Author

instagibbs commented Nov 19, 2019

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

@instagibbs
Copy link
Member Author

instagibbs commented Nov 22, 2019

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

@Sjors Sjors mentioned this pull request Nov 27, 2019
Closed
@instagibbs
Copy link
Member Author

instagibbs commented Jan 7, 2020

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

Sjors
Sjors approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK e3b19d8

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

@achow101
Copy link
Member

achow101 commented Jan 13, 2020

Code review ACK e3b19d8

@gwillen
Copy link
Contributor

gwillen commented Jan 13, 2020

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
Copy link
Member Author

instagibbs commented Jan 14, 2020

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

@gwillen
Copy link
Contributor

gwillen commented Jan 14, 2020

Thanks, code review ACK 267c77f.

@Sjors
Copy link
Member

Sjors commented Jan 15, 2020 •
edited
Loading

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
Copy link
Contributor

gwillen commented Jan 15, 2020

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
Copy link
Member Author

instagibbs commented Jan 15, 2020

fixed :)

@Sjors
Copy link
Member

Sjors commented Jan 15, 2020

utACK 3c30d71

@gwillen
Copy link
Contributor

gwillen commented Jan 15, 2020

code review ACK 3c30d71

@achow101
Copy link
Member

achow101 commented Jan 15, 2020

ACK 3c30d71

nit: squash, but 3 ACKs, so meh.

meshcollider
meshcollider reviewed Jan 17, 2020
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
promag reviewed Jan 17, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

instagibbs commented Jan 17, 2020

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

@promag
Copy link
Contributor

promag commented Jan 17, 2020

Code review ACK 3c30d71.

I'll address my concerns in a follow up.

@fanquake
Copy link
Member

fanquake commented Jan 22, 2020

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 added a commit that referenced this pull request Jan 22, 2020
@fanquake
Merge #17492: QT: bump fee returns PSBT on clipboard for watchonly-on…
742f84d 
…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
@fanquake fanquake merged commit 3c30d71 into bitcoin:master Jan 22, 2020
@fanquake fanquake mentioned this pull request Jan 22, 2020
Closed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 24, 2020
@fanquake
Merge bitcoin#17492: QT: bump fee returns PSBT on clipboard for watch…
8a43f5f 
…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
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@fanquake
Merge bitcoin#17492: QT: bump fee returns PSBT on clipboard for watch…
0cd2c36 
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jonasschnelli jonasschnelli Awaiting requested review from jonasschnelli

4 more reviewers

@Sjors Sjors Sjors approved these changes

@gwillen gwillen gwillen left review comments

@meshcollider meshcollider meshcollider left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

GUI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@instagibbs @jonasschnelli @DrahtBot @Sjors @achow101 @gwillen @promag @fanquake @meshcollider