Skip to content

Fee selection#546

Merged
hebasto merged 14 commits into
bitcoin-core:qt6from
johnny9:standard-fee-selection
May 8, 2026
Merged

Fee selection#546
hebasto merged 14 commits into
bitcoin-core:qt6from
johnny9:standard-fee-selection

Conversation

@johnny9
Copy link
Copy Markdown
Collaborator

@johnny9 johnny9 commented Apr 2, 2026

image image image

Wires up fee selection in the Send page. Overrides are in place for Regtest where CoinControl fee estimations aren't supported.

@johnny9 johnny9 force-pushed the standard-fee-selection branch 2 times, most recently from 88739f0 to 7824720 Compare April 2, 2026 13:59
@johnny9 johnny9 changed the title Standard fee selection Fee selection Apr 2, 2026
@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Apr 3, 2026

Related to #520

Copy link
Copy Markdown
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build fails 5c7cd51

In file included from /usr/include/x86_64-linux-gnu/qt6/QtTest/qtesteventloop.h:8,
                 from /usr/include/x86_64-linux-gnu/qt6/QtTest/qsignalspy.h:11,
                 from /usr/include/x86_64-linux-gnu/qt6/QtTest/QtTest:10,
                 from /home/marnix/projects/gui-qml/test/test_walletqmlmodel.cpp:5:
/home/marnix/projects/gui-qml/test/test_walletqmlmodel.cpp: In lambda function:
/home/marnix/projects/gui-qml/test/test_walletqmlmodel.cpp:409:9: error: return-statement with no value, in function returning ‘util::Result<std::shared_ptr<const CTransaction> >’ [-fpermissive]
  409 |         QCOMPARE(recipients.size(), 1U);
      |         ^~~~~~~~
gmake[2]: *** [test/CMakeFiles/bitcoinqml_unit_tests.dir/build.make:216: test/CMakeFiles/bitcoinqml_unit_tests.dir/test_walletqmlmodel.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:2075: test/CMakeFiles/bitcoinqml_unit_tests.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2

@johnny9 johnny9 force-pushed the standard-fee-selection branch from 5c7cd51 to a4de80b Compare April 6, 2026 17:40
@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Apr 6, 2026

fix the compile issue in the unittest

Copy link
Copy Markdown
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a4de80b

some comments from first test (some maybe for follow up):

  • Fee amount is not displayed in sats in case this is active
  • The Fee Rate entry is not limited when no decimal is used (i.e. it accepts an unlimited amount of characters as long as no comma is used)
  • When entering full balance, and not Include fee in amount the Review button doesn't work. Which makes sense, but there is no clear user feedback on why.

@pseudoramdom
Copy link
Copy Markdown
Contributor

a4de80b

Works well.
Would it be good to include fee rate in each of the option - high, default, low? (doesn't have to be in this PR, we can track this separately)

Screenshot 2026-04-22 at 11 40 26 AM

@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Apr 22, 2026

a4de80b

Works well. Would it be good to include fee rate in each of the option - high, default, low? (doesn't have to be in this PR, we can track this separately)
Screenshot 2026-04-22 at 11 40 26 AM

The fees are based on number of blocks. I think the fee rate might clutter up the information in a dropdown a bit too much but there might be an argument for the fee rate being more important than the fee itself but that migth just be for people that know bitcoin too well.

quint64 m_fee_estimate_request_id{0};
int m_fee_estimate_revision{0};
bool m_custom_fee_enabled{false};
bool m_fee_estimate_pending{false};
Copy link
Copy Markdown
Contributor

@pseudoramdom pseudoramdom Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it'll be better to extract the fee estimation related code to its own model, say feeselectionmodel.
This way both send and RBF flows can use it

Copy link
Copy Markdown
Collaborator Author

@johnny9 johnny9 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think that makes sense. Probably some association per transaction with the current_transaction being the one in the send dialog and maybe another for a chosen RBF transaction? I'm not totally sure where it would land but just at a glance it looks like it might be a needed refactor.

johnny9 added a commit to johnny9/BitcoinCoreAppDevelopment that referenced this pull request Apr 27, 2026
# Conflicts:
#	qml/models/walletqmlmodel.cpp
#	qml/pages/wallet/DesktopWallets.qml
#	qml/pages/wallet/Send.qml
#	test/CMakeLists.txt
#	test/qml/qml_tests_main.cpp
#	test/test_unit_tests_main.cpp
@johnny9 johnny9 force-pushed the standard-fee-selection branch from a4de80b to e5fab68 Compare May 3, 2026 03:39
Comment thread qml/pages/wallet/Send.qml Outdated
Copy link
Copy Markdown
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f3ca7a0


minor issues can be in follow-up
For example, below dust threshold handling

Image

@hebasto hebasto merged commit 98954ad into bitcoin-core:qt6 May 8, 2026
4 checks passed
hebasto added a commit that referenced this pull request May 9, 2026
3268d3e qml: Dim conflicted transactions in activity list (pseudoramdom)
2cf482b test: Add functional tests for RBF flow (pseudoramdom)
5999e80 test: Add bump transaction model unit tests & qml tests (pseudoramdom)
f4ff2b8 qml: Implement Replace-by-fee (pseudoramdom)

Pull request description:

  Bring support to "Speed up" unconfirmed transactions by bumping fees (#521)

  - Add `BumpTransactionModel` to manage the fee bump flow
  - Add `SpeedUpOverlay` popup to review and confirm the higher-fee replacement transaction
  - Add a reusable` InfoBanner` component for contextual actions/status
    -  Show a speed up banner in transaction details for bumpable transactions
    - Show a replacement-status banner in transaction details when a transaction has been replaced

  #### Review note
  - ~~This PR is stacked on top of #546 (a4de80b)~~ Now merged and this PR is rebased

  ### Screenshots

  <img width="1181" height="906" alt="Screenshot 2026-04-24 at 2 51 49 PM" src="https://github.com/user-attachments/assets/93f3a27d-a540-4adb-95ff-0ff339ae3e0d" />
  <img width="1428" height="1127" alt="Screenshot 2026-04-25 at 1 18 36 PM" src="https://github.com/user-attachments/assets/d91dd115-4f94-473a-8019-21073555bab3" />
  <img width="1428" height="1127" alt="Screenshot 2026-04-25 at 1 19 16 PM" src="https://github.com/user-attachments/assets/73e40893-4f9c-493c-bf6e-6eefe3692f6a" />
  <img width="1428" height="1127" alt="Screenshot 2026-04-25 at 1 19 32 PM" src="https://github.com/user-attachments/assets/189a4163-5565-452a-ba5d-ae9c17a71868" />

ACKs for top commit:
  johnny9:
    ACK 3268d3e

Tree-SHA512: 2d6e887815484d10883adfa84b0f0abcdfc953bf836c683e3c87a186a36a5de03524bf324ed4bd36fccb2e9b747f228a7f6c3eed1bb2c3192e3f1ac8bed2df40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants