wallet: Explicit feerate for bumpfee #16727

instagibbs · 2019-08-26T15:25:10Z

Taking over for #16492 which seems to have gone inactive.

Only minor commit cleanups, rebase, and some help text fixes on top of previous PR. Renamed feeRate to fee_rate to reflect updated guidelines.

instagibbs · 2019-08-26T16:26:17Z

Added some additional basic argument testing, changed the logic to use BTC/kB like everywhere else.

DrahtBot · 2019-08-26T19:13:50Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

Sjors · 2019-08-27T10:01:37Z

Concept ACK.

Alternatively you could rebase on #11413 and use that syntax instead, which is easier to use on the command line than a JSON options object.

Relaxo143 · 2019-08-27T10:41:15Z

Why does Bitcoin Core use btc/kB? Every other service/wallet seems to use sats/byte.

Sjors · 2019-08-27T11:41:43Z

@Relaxo143 you'll be pleased to see #11413 then...

instagibbs · 2019-08-27T11:45:10Z

@Relaxo143 Unless you allow decimal points it will have less precision.

…

On Tue, Aug 27, 2019, 6:44 AM Relaxo143 ***@***.***> wrote: Why does Bitcoin Core use btc/kB? Every other service/wallet seems to use sats/byte. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16727?email_source=notifications&email_token=ABMAFUYWXNZF556SIQ4CPFTQGUAQXA5CNFSM4IPQ24D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5HJXSY#issuecomment-525245387>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMAFUZSS2H575PNQTKBVL3QGUAQXANCNFSM4IPQ24DQ> .

promag

Concept ACK.

promag · 2019-08-26T15:41:39Z

test/functional/wallet_bumpfee.py

luke-jr · 2019-09-02T23:12:44Z

Renamed feeRate to fee_rate to reflect updated guidelines.

Every other RPC uses "feeRate"...

instagibbs · 2019-09-03T13:28:18Z

Every other RPC uses "feeRate"...

We used to use confTarget, but now have conf_target in use in various newer args. I see no reason to introduce old styles for old style sake.

instagibbs · 2019-09-13T18:45:11Z

going to leave #11413 type ideas alone for now.

I think this is a pretty simple feature and I'd like it in.

src/wallet/rpcwallet.cpp

Sjors

Quite a bit of code duplication in 74b271616016a5efd03d96c3077bdac79c38b134 rpc bumpfee check fee_rate argument. Can you add a helper?

Also left some documentation nits as of d086d25.

Sjors · 2019-09-27T17:15:06Z

src/wallet/rpcwallet.cpp

"BTC per kB"

Sjors · 2019-09-27T17:22:55Z

src/wallet/rpcwallet.cpp

CURRENCY_UNIT + "per kB\n"

Sjors · 2019-09-27T17:25:04Z

src/wallet/rpcwallet.cpp

\n missing

Sjors doesn't like being addressed in third person. Also there's no need to explain that a fee rate determines the total fee; we don't do that in other places either. More importantly, this doesn't cover the actual RBF rules.

What about: "Specify a specific fee rate instead of relying on the built-in fee estimator. Must be at least 0.0001 BTC per kB higher than the current rate."

Fun fact: there's no convenient way to find the current fee rate.

Sjors · 2019-09-27T17:37:52Z

src/wallet/rpcwallet.cpp

"either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate (or an absolute fee amount - deprecated)."

not going to tell users about a deprecated arg here, otherwise taking suggestion

Sjors · 2019-09-27T17:38:38Z

src/wallet/rpcwallet.cpp

May want to check that it's not null.

instagibbs · 2019-09-28T11:39:07Z

Fixed most nits, @Sjors if you're referring to code duplication with respect to TotalBump, I don't really feel like optimizing a piece of code we're going to rip out in 0.20.

laanwj · 2019-09-30T09:23:09Z

The bumpfee test is failing in the CIs.

instagibbs · 2019-09-30T13:40:22Z

pushed fixed test

Sjors · 2019-09-30T15:31:17Z

Code review ACK c812aba

laanwj

ACK c812aba
Some small remarks but nothing that needs to hold up the merge.

laanwj · 2019-10-02T13:26:45Z

src/wallet/feebumper.cpp

+    // the minimum of that and the wallet's conservative
+    // WALLET_INCREMENTAL_RELAY_FEE value to future proof against changes to
+    // network wide policy for incremental relay fee that our node may not be
+    // aware of. This ensures we're over the over the required relay fee rate


"over the over the"
Though I see this typo exists in the old code as well so there's no need to fix that in this PR.

laanwj · 2019-10-02T13:31:25Z

src/wallet/feebumper.cpp

+        // The user provided a feeRate argument.
+        // We calculate this here to avoid compiler warning on the cs_wallet lock
+        const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
+        Result res = CheckFeeRate(wallet, wtx, *(new_coin_control.m_feerate), maxTxSize, errors);


Why the set of parentheses around new_coin_control.m_feerate?

laanwj · 2019-10-02T13:32:58Z

src/wallet/feebumper.cpp

+
+    if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
+        errors.push_back(strprintf(
+            "New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- ",


Does this error message need to specify units for the numbers?

c812aba test bumpfee fee_rate argument (ezegom) 9f25de3 rpc bumpfee check fee_rate argument (ezegom) 88e5f99 rpc bumpfee: add fee_rate argument (ezegom) 1a4c791 rpc bumpfee: move feerate estimation logic into separate method (ezegom) Pull request description: Taking over for #16492 which seems to have gone inactive. Only minor commit cleanups, rebase, and some help text fixes on top of previous PR. Renamed `feeRate` to `fee_rate` to reflect updated guidelines. ACKs for top commit: Sjors: Code review ACK c812aba laanwj: ACK c812aba Tree-SHA512: 5f7f51bd780a573ccef1ccd72b0faf3e5d143f6551060a667560c5163f7d9480e17e73775d1d7bcac0463f3b6b4328f0cff7b27e39483bddc42a530f4583ce30

maflcko · 2019-10-09T20:11:09Z

src/wallet/feebumper.cpp

+    return feebumper::Result::OK;
+}
+
+static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee)


This can be const CWallet& wallet, as this shouldn't be mutable, nor nullable.

maflcko · 2019-10-09T20:13:12Z

post merge ACK c812aba

maflcko · 2019-10-09T20:20:22Z

Added release notes: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.19.0-Release-Notes-Draft#deprecated-or-removed-rpcs

66b2984 change wallet pointers to references in feebumper (Adam Jonas) 9be6666 typo and unneccessary parentheses (Adam Jonas) Pull request description: Picking up some of the suggestions in the comments of #16727 including: #16727 (comment) #16727 (comment) #16727 (comment) ACKs for top commit: promag: Code review ACK 66b2984. MarcoFalke: ACK 66b2984 (looked at the diff on GitHub) fjahr: ACK 66b2984 reviewed code Tree-SHA512: d118f7689970fe39d9f5318dc818f13283cce9194370b3ce4758f298172e4681ae119ddc809f5c0b7602677137ac0d38147b915422ff616531a76a570b766fa2

66b2984 change wallet pointers to references in feebumper (Adam Jonas) 9be6666 typo and unneccessary parentheses (Adam Jonas) Pull request description: Picking up some of the suggestions in the comments of bitcoin#16727 including: bitcoin#16727 (comment) bitcoin#16727 (comment) bitcoin#16727 (comment) ACKs for top commit: promag: Code review ACK 66b2984. MarcoFalke: ACK 66b2984 (looked at the diff on GitHub) fjahr: ACK 66b2984 reviewed code Tree-SHA512: d118f7689970fe39d9f5318dc818f13283cce9194370b3ce4758f298172e4681ae119ddc809f5c0b7602677137ac0d38147b915422ff616531a76a570b766fa2

c3857c5 wallet: remove CreateTotalBumpTransaction() (Jon Atack) 4a0b27b wallet: remove totalfee from createBumpTransaction() (Jon Atack) e347cfa rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack) bd05f96 test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack) a6d1ab8 test: update bumpfee testing from totalFee to fee_rate (Jon Atack) Pull request description: Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it. ACKs for top commit: laanwj: ACK c3857c5 Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00

rpc bumpfee: move feerate estimation logic into separate method

1a4c791

instagibbs force-pushed the feerate_bumpfee branch from 664f08b to ae700f8 Compare August 26, 2019 16:25

instagibbs force-pushed the feerate_bumpfee branch from ae700f8 to b72d3ff Compare August 26, 2019 16:46

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Aug 26, 2019

fanquake mentioned this pull request Aug 27, 2019

rpc: Add feeRate argument to bumpFee RPC #16492

Closed

fanquake changed the title ~~Explicit feerate for bumpfee~~ wallet: Explicit feerate for bumpfee Aug 27, 2019

fanquake removed the Tests label Aug 27, 2019

promag reviewed Aug 27, 2019

View reviewed changes

instagibbs force-pushed the feerate_bumpfee branch from b72d3ff to 33911b9 Compare September 13, 2019 18:43

instagibbs mentioned this pull request Sep 13, 2019

bumpfee.TotalFee removed without replacement #16485

Closed

maflcko reviewed Sep 17, 2019

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the feerate_bumpfee branch from 33911b9 to d086d25 Compare September 23, 2019 13:46

maflcko added this to the 0.19.0 milestone Sep 23, 2019

Sjors suggested changes Sep 27, 2019

View reviewed changes

ezegom added 2 commits September 28, 2019 07:34

rpc bumpfee: add fee_rate argument

88e5f99

rpc bumpfee check fee_rate argument

9f25de3

instagibbs force-pushed the feerate_bumpfee branch from d086d25 to 9c92d11 Compare September 28, 2019 11:34

laanwj added the Waiting for author label Sep 30, 2019

test bumpfee fee_rate argument

c812aba

instagibbs force-pushed the feerate_bumpfee branch from 9c92d11 to c812aba Compare September 30, 2019 13:32

maflcko added Needs release note and removed Waiting for author labels Oct 1, 2019

laanwj mentioned this pull request Oct 2, 2019

Release process for 0.19.0 #16996

Closed

19 tasks

laanwj approved these changes Oct 2, 2019

View reviewed changes

laanwj merged commit c812aba into bitcoin:master Oct 2, 2019

ajtowns mentioned this pull request Oct 3, 2019

feefilter: Compute the absolute fee rather than stored rate #16507

Merged

maflcko reviewed Oct 9, 2019

View reviewed changes

maflcko removed the Needs release note label Oct 9, 2019

adamjonas mentioned this pull request Oct 10, 2019

refactor: Feebumper EstimateFeeRate follow-up #17098

Merged

jonatack mentioned this pull request Mar 10, 2020

wallet: remove deprecated fee bumping by totalFee #18312

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

wallet: Explicit feerate for bumpfee #16727

wallet: Explicit feerate for bumpfee #16727

Uh oh!

Conversation

instagibbs commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Aug 26, 2019

Uh oh!

DrahtBot commented Aug 26, 2019

Conflicts

Uh oh!

Sjors commented Aug 27, 2019

Uh oh!

Relaxo143 commented Aug 27, 2019

Uh oh!

Sjors commented Aug 27, 2019

Uh oh!

instagibbs commented Aug 27, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Sep 2, 2019

Uh oh!

instagibbs commented Sep 3, 2019

Uh oh!

instagibbs commented Sep 13, 2019

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Sep 28, 2019

Uh oh!

laanwj commented Sep 30, 2019

Uh oh!

instagibbs commented Sep 30, 2019

Uh oh!

Sjors commented Sep 30, 2019

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 9, 2019

Uh oh!

maflcko commented Oct 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

instagibbs commented Aug 26, 2019 •

edited

Loading

instagibbs commented Aug 27, 2019 via email •

edited

Loading