Dialog for allowing the user to choose the change output when bumping a tx

achow101 commented at 9:13 pm on January 23, 2023: member

Based on https://github.com/bitcoin/bitcoin/pull/26467

Implements a GUI dialog for allowing the user to choose the output to reduce when bumping a transaction. This adds the functionality that was added to the RPC.

DrahtBot commented at 9:13 pm on January 23, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	luke-jr
Approach ACK	john-moffett

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

jarolrod added the label Feature on Jan 24, 2023

jarolrod added the label UX on Jan 24, 2023

Sjors commented at 1:03 pm on January 24, 2023: member

Can you add a screenshot to the PR description (and one for the single-output case)?

john-moffett commented at 4:04 pm on January 24, 2023: contributor

Approach ACK

I think I’d prefer if the user had some more information about the outputs in the dialog. Maybe something like this, but worded better:

Maybe the actual change address isn’t even needed, or could be revealed in a tooltip.

Also, if there’s a single change output with sufficient funds for feebumping, then selecting “none” has basically no effect compared to selecting that output, as the wallet will always(?) deduct from that existing change output. Maybe this common case can be simplified by removing the “None” option?

Sjors commented at 5:39 pm on January 24, 2023: member

Can we be a bit more bold and just assume that a change address is change, and not show the dialog in that case? Ideally we don’t show such a dialog more often than necessary.

john-moffett commented at 5:54 pm on January 24, 2023: contributor

That would prevent the case where you wanted to deduct the extra fee from the recipient, such as when you initially selected “Subtract fee from amount”.

Ideally, you’d only show the dialog if that were selected previously, but I don’t think it’s persisted anywhere. Eg -

https://github.com/bitcoin-core/gui/blob/30f553d45797847166109a161628dba3bf00bc94/src/qt/sendcoinsrecipient.h#L42

I could be wrong, though.

hebasto renamed this:
~~qt: Dialog for allowing the user to choose the change output when bumping a tx~~
Dialog for allowing the user to choose the change output when bumping a tx
on Feb 2, 2023

DrahtBot added the label Needs rebase on Feb 16, 2023

achow101 force-pushed on May 19, 2023

DrahtBot removed the label Needs rebase on May 19, 2023

DrahtBot added the label CI failed on May 20, 2023

luke-jr commented at 1:45 am on June 23, 2023: member

Concept ACK. I’d just show “Change” for the change, though. End users shouldn’t need to know about change addresses.

DrahtBot added the label Needs rebase on Jul 20, 2023

achow101 force-pushed on Jul 20, 2023

DrahtBot removed the label Needs rebase on Jul 20, 2023

DrahtBot removed the label CI failed on Jul 20, 2023

DrahtBot added the label CI failed on Aug 17, 2023

DrahtBot added the label Needs rebase on Aug 22, 2023

achow101 force-pushed on Sep 7, 2023

DrahtBot removed the label Needs rebase on Sep 7, 2023

achow101 marked this as ready for review on Jan 8, 2024

achow101 force-pushed on Jan 8, 2024

in src/qt/bumpfeechoosechangedialog.cpp:46 in c697167741 outdated

41+                address_info = QString::fromStdString(label) + QString(" (") + QString::fromStdString(address) + QString(")");
42+            } else {
43+                address_info = QString::fromStdString(address);
44+            }
45+        }
46+        QString output_info = QString::number(i) + QString(": ") + BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), txout.nValue) + tr(" to ") + address_info;

hebasto commented at 1:54 pm on February 12, 2024:

To make translators’ life easier and to keep the context as large as possible, I suggest to use QString::arg. Also it will fix the lint-qt-translation.py warning.

achow101 commented at 8:24 pm on October 4, 2024:

Done

hebasto commented at 1:55 pm on February 12, 2024: member

The first commit e1ea0ba0f2aa9b36df6b74b664087b236f174990 fails to compile:

 0  CXX      qt/libbitcoinqt_a-walletmodel.o
 1qt/walletmodel.cpp: In member function ‘bool WalletModel::bumpFee(uint256, uint256&)’:
 2qt/walletmodel.cpp:486:41: error: no matching function for call to ‘interfaces::Wallet::createBumpTransaction(uint256&, wallet::CCoinControl&, std::vector<bilingual_str>&, CAmount&, CAmount&, CMutableTransaction&)’
 3  486 |     if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx)) {
 4      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 5In file included from ./qt/walletmodel.h:16,
 6                 from qt/walletmodel.cpp:9:
 7./interfaces/wallet.h:166:18: note: candidate: ‘virtual bool interfaces::Wallet::createBumpTransaction(const uint256&, const wallet::CCoinControl&, std::vector<bilingual_str>&, CAmount&, CAmount&, CMutableTransaction&, std::optional<unsigned int>)’
 8  166 |     virtual bool createBumpTransaction(const uint256& txid,
 9      |                  ^~~~~~~~~~~~~~~~~~~~~
10./interfaces/wallet.h:166:18: note:   candidate expects 7 arguments, 6 provided
11make: *** [Makefile:13550: qt/libbitcoinqt_a-walletmodel.o] Error 1

DrahtBot added the label Needs rebase on Apr 23, 2024

interfaces: Expose CreateRateBumpTransaction's orig_change_pos 15ebc44020

interfaces: Add isChange to wallet interface e1cce622f7

qt: Add a dialog to select the change output when bumping fee

In order to correctly choose the change output when doing fee bumping in
the GUI, we need to ask the user which output is change. We can make a
guess using our ScriptIsChange heuristic, however the user may have
chosen to have a custom change address or have otherwise labeled their
change address which makes our change detection fail. By asking the user
when fee bumping, we can avoid adding additional change outputs that are
unnecessary.

674187caf5

achow101 force-pushed on Oct 4, 2024

achow101 commented at 8:24 pm on October 4, 2024: member

Rebased and fixed the build issue in the first commit.

DrahtBot removed the label Needs rebase on Oct 4, 2024

DrahtBot removed the label CI failed on Oct 10, 2024

hebasto commented at 3:09 pm on October 29, 2024: member

test_bitcoin-qt fails in the CI: https://github.com/bitcoin-core/gui/actions/runs/11186485220.

Dialog for allowing the user to choose the change output when bumping a tx #700

Reviews

Conflicts