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

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, hebasto
Approach ACK	john-moffett, w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

"Choose which output is change." -> "Choose which output is the change." [The phrase "is change" is ungrammatical; "is the change" clarifies meaning.]
"/** Dialog for choosing the change output when bumping fee" -> "/** Dialog for choosing the change output when bumping the fee" [Missing article "the" makes the phrase awkward and less clear.]

2025-12-11

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:

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

DrahtBot added the label Needs rebase on Apr 23, 2024

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.

Aminzarbani approved

DrahtBot added the label CI failed on Jan 26, 2025

DrahtBot removed the label CI failed on Jan 26, 2025

w0xlt commented at 10:18 PM on April 3, 2025: contributor

Approach ACK

DrahtBot added the label CI failed on May 17, 2025

DrahtBot commented at 4:26 PM on May 17, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin-core/gui/runs/31101606803 LLM reason (✨ experimental): The CI failure is due to a build error in bumpfeechoosechangedialog.cpp.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

maflcko removed the label CI failed on Sep 4, 2025

achow101 commented at 2:34 PM on October 22, 2025: member

Are you still working on this?

hebasto closed this on Nov 18, 2025

hebasto reopened this on Nov 18, 2025

hebasto added the label CI failed on Nov 18, 2025

interfaces: Expose CreateRateBumpTransaction's orig_change_pos bbc2920d21

interfaces: Add isChange to wallet interface 529a3df785

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.

a868bde35f

achow101 force-pushed on Dec 11, 2025

hebasto commented at 3:47 PM on December 15, 2025: member

Concept ACK.

DrahtBot commented at 1:14 AM on March 15, 2026: contributor

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.