Page MenuHomePhabricator

Merge #10706: Improve wallet fee logic and fix GUI bugs
ClosedPublic

Authored by jasonbcox on Fri, Mar 8, 00:32.

Details

Summary

11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos)
fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos)
2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos)
1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos)
03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos)
ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos)

Pull request description:

This builds on #10589  (first 5 commits from that PR, last 5 commits are new)

The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around.

This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings.  Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI.   After these changes, using the GUI does not involve the global transaction confirm target or payTxFee.

The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases.

Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee.

This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release.

Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1

Backport of Core PR10706 minus RBF and other unused codepaths.
https://github.com/bitcoin/bitcoin/pull/10706/files
Completes T521

Co-authored-by: Jason B. Cox <contact@jasonbcox.com>

Test Plan
make check
ninja check
test_runner.py
bitcoin-qt # for sanity

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Fri, Mar 8, 00:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Mar 8, 00:32
Herald added a subscriber: schancel. · View Herald Transcript
Fabien requested changes to this revision.Fri, Mar 8, 17:23

I think there is a backport missing in rpcwallet.cpp for the fundrawtransaction RPC: https://github.com/bitcoin/bitcoin/pull/9672/commits/891c5eeec2dbf295d14e8f888de02637367dd930

src/qt/sendcoinsdialog.cpp
729 ↗(On Diff #7649)

I don't think this is actually possible to get estimateFee() to return a negative fee rate ?
If so this branch can be removed, as well as labelSmartfee2.
Anyway this is out of scope for this backport.

src/wallet/coincontrol.h
27 ↗(On Diff #7649)

This can likely be removed

38 ↗(On Diff #7649)

dito

src/wallet/test/wallet_tests.cpp
733 ↗(On Diff #7649)

Why changing the name from the original PR ?

This revision now requires changes to proceed.Fri, Mar 8, 17:23
jasonbcox added inline comments.Fri, Mar 8, 18:08
src/qt/sendcoinsdialog.cpp
729 ↗(On Diff #7649)

That is correct, txmempool::estimateFee can't return a negative value. Once these backports are more or less up-to-date, I can go in and mop stuff like this up.

src/wallet/coincontrol.h
27 ↗(On Diff #7649)

I'm leaving it in for now to reduce merge conflicts, but yes it can be removed as it's effectively unused (similar to labelSmartFee).

src/wallet/test/wallet_tests.cpp
733 ↗(On Diff #7649)

I don't recall doing this. Will fix.

In D2673#62455, @Fabien wrote:

I think there is a backport missing in rpcwallet.cpp for the fundrawtransaction RPC: https://github.com/bitcoin/bitcoin/pull/9672/commits/891c5eeec2dbf295d14e8f888de02637367dd930

I don't think this has a direct impact on this diff, but I will go ahead and backport it in order: T546

Fabien added a comment.Sat, Mar 9, 07:43

I think this will allow to backport some more change in the fundrawtransaction RPC. In the original PR these changes are in located rpcwallet.cpp starting from line 2732.

src/wallet/wallet.cpp
2754 ↗(On Diff #7649)

Also this is not part of the original PR and shouldn't be needed after PR9672

jasonbcox updated this revision to Diff 7675.Mon, Mar 11, 16:24

Rebased

jasonbcox updated this revision to Diff 7676.Mon, Mar 11, 17:03

Fixed missing lines from the original backport in rpcwallet.cpp

Fabien accepted this revision.Mon, Mar 11, 17:27
This revision is now accepted and ready to land.Mon, Mar 11, 17:27
Closed by commit rABCfa270cac02ed: Merge #10706: Improve wallet fee logic and fix GUI bugs (authored by Wladimir J. van der Laan <laanwj@gmail.com>, committed by jasonbcox). · Explain WhyMon, Mar 11, 17:33
This revision was automatically updated to reflect the committed changes.