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

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

Merged
merged 2 commits into from Jan 22, 2020

Conversation

@instagibbs
Copy link
Member

instagibbs commented Nov 15, 2019

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

This comment has been minimized.

Copy link
Member Author

instagibbs commented Nov 15, 2019

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Nov 15, 2019

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 16, 2019

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

Conflicts

No conflicts as of last run.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 16, 2019

Concept ACK. Linter complains about circular dependency.

@instagibbs

This comment has been minimized.

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 instagibbs force-pushed the instagibbs:gui_bump_psbt branch from 1cfd90c to b01be8e Nov 18, 2019
@instagibbs

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

instagibbs commented Nov 19, 2019

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

@instagibbs instagibbs force-pushed the instagibbs:gui_bump_psbt branch from b01be8e to 6c81f47 Nov 22, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Nov 22, 2019

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

@instagibbs instagibbs force-pushed the instagibbs:gui_bump_psbt branch from 6c81f47 to e3b19d8 Jan 7, 2020
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jan 7, 2020

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

@fanquake fanquake requested review from jonasschnelli and Sjors Jan 7, 2020
@Sjors
Sjors approved these changes Jan 8, 2020
Copy link
Member

Sjors left a comment

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 Outdated Show resolved Hide resolved
@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jan 13, 2020

Code review ACK e3b19d8

@gwillen

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

gwillen commented Jan 14, 2020

Thanks, code review ACK 267c77f.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors 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?

@gwillen

This comment has been minimized.

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 instagibbs force-pushed the instagibbs:gui_bump_psbt branch from 267c77f to 3c30d71 Jan 15, 2020
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jan 15, 2020

fixed :)

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 15, 2020

utACK 3c30d71

@gwillen

This comment has been minimized.

Copy link
Contributor

gwillen commented Jan 15, 2020

code review ACK 3c30d71

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jan 15, 2020

ACK 3c30d71

nit: squash, but 3 ACKs, so meh.

Copy link
Member

meshcollider left a comment

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.

Copy link
Member

promag left a comment

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

promag commented Jan 17, 2020

Code review ACK 3c30d71.

I'll address my concerns in a follow up.

@fanquake

This comment has been minimized.

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
…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
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Jan 24, 2020
…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
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

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