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

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

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

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

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

Branch

pr10706-2

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5185
Build 8433: Bitcoin ABC Buildbot (legacy)
Build 8432: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Mar 8 2019, 00:32

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 8 2019, 00:32

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5185: Diff 7649.Mar 8 2019, 00:47

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	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	This can likely be removed
38	dito
src/wallet/test/wallet_tests.cpp
733	Why changing the name from the original PR ?

This revision now requires changes to proceed.Mar 8 2019, 17:23

jasonbcox added inline comments.Mar 8 2019, 18:08

src/qt/sendcoinsdialog.cpp
729	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	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	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

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.