gui: Fix manual coin control with multiple wallets loaded #18894

promag · 2020-05-05T23:27:40Z

This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog.

This is an alternative to #17457. Two main differences are:

scope reduced - no unnecessary changes unrelated to the fix;
approach taken - coin control instance now belongs to the send view.

All problems raised in #17457 reviews no longer apply due to the approach taken - #17457 (review) and #17457 (comment))

No change in behavior if only one wallet is loaded.

Closes #15725.

promag · 2020-05-05T23:33:27Z

If possible tag 0.20. @Sjors @hebasto @gwillen would ❤️ some reviews.

ryanofsky

Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected

promag · 2020-05-06T01:05:05Z

Not sure if this means coin control settings are reset between payments.

Yes, and new change shows if coin control dialog is opened again.

It would be good to note in the PR description or release notes if single wallet behavior is affected

Yes, at least that's what I expect - I'll note it.

DrahtBot · 2020-05-06T02:54:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

gui: Make send and receive widgets extend QWidget #17611 (gui: Make send and receive widgets extend QWidget by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto

Approach ACK a8b5f1b, tested on Linux Mint 19.3: #15725 is fixed.

src/qt/coincontroldialog.h

hebasto

ACK a8b5f1b

jonasschnelli · 2020-05-07T07:48:20Z

Concept ACK.
Will detail review and test soon.
Could be a 0.20 bugfix.

Sjors

tACK a8b5f1b

Sjors · 2020-05-08T13:04:09Z

src/qt/coincontroldialog.cpp


    GUIUtil::handleCloseWindowShortcut(this);
+
+    if(_model->getOptionsModel() && _model->getAddressTableModel())


This needs an assert that _model isn't nullptr.

This will crash anyway.

assert crashes are marginally easier to debug afaik

Yes. But note that the model is received in the constructor and this dialog is instanced in SendCoinsDialog which already has the model set.

I'll add the assertion if I had to push again for some reason or if others think the same.

src/qt/coincontroldialog.h

jonasschnelli · 2020-05-13T08:15:11Z

utACK a8b5f1b

laanwj · 2020-05-13T17:50:40Z

src/qt/coincontroldialog.cpp

        label->setVisible(nChange < 0);
 }

-CCoinControl* CoinControlDialog::coinControl()


Thanks for getting rid of this.

…ts loaded a8b5f1b gui: Fix manual coin control with multiple wallets loaded (João Barbosa) Pull request description: This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog. This is an alternative to bitcoin#17457. Two main differences are: - scope reduced - no unnecessary changes unrelated to the fix; - approach taken - coin control instance now belongs to the send view. All problems raised in bitcoin#17457 reviews no longer apply due to the approach taken - bitcoin#17457 (review) and bitcoin#17457 (comment)) No change in behavior if only one wallet is loaded. Closes bitcoin#15725. ACKs for top commit: jonasschnelli: utACK a8b5f1b ryanofsky: Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected hebasto: ACK a8b5f1b Sjors: tACK a8b5f1b Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65

Github-Pull: bitcoin#18894 Rebased-From: a8b5f1b

245c862 test: disable script fuzz tests (fanquake) 9a8fb4c fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer (practicalswift) 6161c94 [net processing] Only send a getheaders for one block in an INV (John Newbery) cf2a6e2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan) cc7d344 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke) 37a6207 test: Add unregister_validation_interface_race test (MarcoFalke) ff4dc20 gui: Fix manual coin control with multiple wallets loaded (João Barbosa) ed0afe8 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) 251e321 rpc: Relock wallet only if most recent callback (João Barbosa) ca4dac4 rpc: Add mutex to guard deadlineTimers (João Barbosa) a3fe458 [docs] Improve commenting in ProcessGetData() (John Newbery) 011532e [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar) 1e73d72 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar) fb82173 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar) 315ae14 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Backports the following PRs to the 0.20 branch: * #18578: gui: Fix leak in CoinControlDialog::updateView * #18808: [net processing] Drop unknown types in getdata * #18814: rpc: Relock wallet only if most recent callback * #18878: test: Add test for conflicted wallet tx notifications * #18894: gui: Fix manual coin control with multiple wallets loaded * #18742: miner: Avoid stack-use-after-return in validationinterface * #18962: net processing: Only send a getheaders for one block in an INV * #18975: test: Remove const to work around compiler error on xenial ACKs for top commit: promag: Tested ACK 245c862 coin control with multiple wallets. laanwj: ACK 245c862 MarcoFalke: ACK 245c862 solved the conflicts myself as a sanity check. Did not re-review 🍷 Tree-SHA512: 285e5a5fad5bbeba6032742c65dc68836e8eccfcceda9e69fec4ddd162a3f61679a96f9bbe3d434267835af67c21ac4c05accf6f63e827c2eb47203c6daafe31

…ts loaded a8b5f1b gui: Fix manual coin control with multiple wallets loaded (João Barbosa) Pull request description: This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog. This is an alternative to bitcoin#17457. Two main differences are: - scope reduced - no unnecessary changes unrelated to the fix; - approach taken - coin control instance now belongs to the send view. All problems raised in bitcoin#17457 reviews no longer apply due to the approach taken - bitcoin#17457 (review) and bitcoin#17457 (comment)) No change in behavior if only one wallet is loaded. Closes bitcoin#15725. ACKs for top commit: jonasschnelli: utACK a8b5f1b ryanofsky: Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected hebasto: ACK a8b5f1b Sjors: tACK a8b5f1b Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65

…aded (#3777) * Merge bitcoin#18894: gui: Fix manual coin control with multiple wallets loaded a8b5f1b gui: Fix manual coin control with multiple wallets loaded (João Barbosa) Pull request description: This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog. This is an alternative to bitcoin#17457. Two main differences are: - scope reduced - no unnecessary changes unrelated to the fix; - approach taken - coin control instance now belongs to the send view. All problems raised in bitcoin#17457 reviews no longer apply due to the approach taken - bitcoin#17457 (review) and bitcoin#17457 (comment)) No change in behavior if only one wallet is loaded. Closes bitcoin#15725. ACKs for top commit: jonasschnelli: utACK a8b5f1b ryanofsky: Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected hebasto: ACK a8b5f1b Sjors: tACK a8b5f1b Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65 * Update src/qt/coincontroldialog.cpp Co-authored-by: PastaPastaPasta <[email protected]> Co-authored-by: Jonas Schnelli <[email protected]> Co-authored-by: PastaPastaPasta <[email protected]>

Summary: This is a backport of Core [[bitcoin/bitcoin#18894 | PR18894]] Test Plan: `ninja && src/qt/bitcoin-qt` Open 2 wallets, open the coin control dialog and select coins, select the second wallet and open the coin control dialog again, verify that it has been updated for the new wallet (old coins and selection cleared) Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9098

Github-Pull: #18894 Rebased-From: a8b5f1b

gui: Fix manual coin control with multiple wallets loaded

a8b5f1b

fanquake added the GUI label May 5, 2020

promag mentioned this pull request May 5, 2020

gui: Fix manual coin control with multiple wallets loaded #17457

Closed

ryanofsky approved these changes May 6, 2020

View reviewed changes

hebasto requested changes May 6, 2020

View reviewed changes

src/qt/coincontroldialog.h Show resolved Hide resolved

DrahtBot mentioned this pull request May 6, 2020

gui: Make send and receive widgets extend QWidget #17611

Closed

hebasto approved these changes May 7, 2020

View reviewed changes

DrahtBot mentioned this pull request May 8, 2020

UI external signer support (e.g. hardware wallet) #16549

Closed

Sjors approved these changes May 8, 2020

View reviewed changes

jonasschnelli added the Needs backport (0.20) label May 13, 2020

jonasschnelli added this to the 0.20.0 milestone May 13, 2020

jonasschnelli merged commit 246e878 into bitcoin:master May 13, 2020

promag deleted the 2019-11-fix-15725.2 branch May 13, 2020 08:29

laanwj reviewed May 13, 2020

View reviewed changes

fanquake removed the Needs backport (0.20) label May 14, 2020

fanquake mentioned this pull request May 14, 2020

[0.20] Final backports for rc2 #18973

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

gui: Fix manual coin control with multiple wallets loaded

ff4dc20

Github-Pull: bitcoin#18894 Rebased-From: a8b5f1b

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

gui: Fix manual coin control with multiple wallets loaded

20b8f93

Github-Pull: #18894 Rebased-From: a8b5f1b

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


		GUIUtil::handleCloseWindowShortcut(this);

		if(_model->getOptionsModel() && _model->getAddressTableModel())

gui: Fix manual coin control with multiple wallets loaded #18894

gui: Fix manual coin control with multiple wallets loaded #18894

Uh oh!

Conversation

promag commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 5, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented May 6, 2020

Uh oh!

DrahtBot commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 7, 2020

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors May 8, 2020

Choose a reason for hiding this comment

Uh oh!

promag May 8, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors May 8, 2020

Choose a reason for hiding this comment

Uh oh!

promag May 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonasschnelli commented May 13, 2020

Uh oh!

laanwj May 13, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

promag commented May 5, 2020 •

edited

Loading

DrahtBot commented May 6, 2020 •

edited

Loading