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 and AI policy 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

Reviewers, this pull request conflicts with the following ones:

#bitcoin/bitcoin/35302 (Silent Payments: Sending (take 2) by Eunovo)
#bitcoin/bitcoin/34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#bitcoin/bitcoin/34400 (wallet: parallel fast rescan (approx 8x speed up with 8 threads) by Eunovo)

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.

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. <sub>Task previous releases, depends DEBUG: https://github.com/bitcoin-core/gui/runs/31101606803</sub> <sub>LLM reason (✨ experimental): The CI failure is due to a build error in bumpfeechoosechangedialog.cpp. </sub>

<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

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.

hebasto closed this on Apr 28, 2026

hebasto reopened this on Apr 28, 2026

hebasto commented at 3:00 PM on April 28, 2026: member

@achow101

test_bitcoin-qt still fails in the CI on Windows.

achow101 force-pushed on Apr 28, 2026

achow101 commented at 11:34 PM on April 28, 2026: member

test_bitcoin-qt still fails in the CI on Windows.

Huh, it works when run directly but not in ctest.

achow101 force-pushed on Apr 29, 2026

interfaces: Expose CreateRateBumpTransaction's orig_change_pos c52c3ca2d2

interfaces: Add isChange to wallet interface 3c16054bca

gui, test: Ensure buttons are actually pressed before returning

ConfirmSend and ConfirmMessage assumed that the widgets with the buttons
to click would be ready by the time the functions fired. This may not
necessarily be the case, so instead they should check for the right
widgets in a loop and only exit when a button has been clicked.

2de2410e5e

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.

bc93de8a19

achow101 force-pushed on Jul 27, 2026

achow101 commented at 12:35 AM on July 29, 2026: member

The CI failure has something to do with using QDialog::exec and trying to send button clicks from the test within a single thread. I cannot figure out how to resolve that problem. Given that this has not had any major review in the past 3 years, I'm going to close this as up for grabs. Hopefully someone who better understands Qt can figure out the problem, or maybe this can be implemented in the new QML GUI.

achow101 closed this on Jul 29, 2026

hebasto commented at 5:47 PM on July 29, 2026: member

... or maybe this can be implemented in the new QML GUI.

Requested in https://github.com/bitcoin-core/gui-qml/issues/813.

pablomartin4btc commented at 3:07 AM on July 31, 2026: contributor

Tracked down the CI issue. The deadlock is in the test helpers: ConfirmSend and ChooseBumpfeeOutput used while(true) spin loops inside QTimer::singleShot callbacks to wait for dialogs to appear. Spinning inside the callback blocks the Qt event loop, so QDialog::exec() can never make the dialog visible.

The fix replaces the spin loops with a non-blocking retry: if the dialog is not yet visible, reschedule with a 10 ms delay so the next check fires in a later event loop iteration.

Branch with fix on top: pablomartin4btc:pr700-rebased and CI green: run 30593340908.

I'll be opening a rebased PR with this fix integrated.