rpc: Add feeRate argument to bumpFee RPC #16492

ezegom · 2019-07-30T00:34:56Z

On PR #15996, the totalfee argument in bumpfee gets deprecated. The feeRate argument in bumpfee serves as a replacement to totalFee.

Small changes to CreateRateBumpTransaction

If the coin_control passed in has a feeRate set then we check if the feeRate is OK and if so, create the bump transaction. The logic to verify if the feeRate is good should be identical to the logic that verifies the totalFee value.
If the coin_control does not have a feeRate set then we estimate the fee as we used to.

instagibbs · 2019-07-30T15:02:29Z

concept ACK

instagibbs · 2019-07-30T15:05:46Z

src/wallet/feebumper.cpp

is this block of code cribbed from somewhere to compare with?

Yeah, this code is from the CreateTotalBumpTransaction method https://github.com/bitcoin/bitcoin/blob/b89249e0de8ad1848d487dd3e7a85a0d6d456de8/src/wallet/feebumper.cpp#L199-L247

instagibbs · 2019-07-30T15:06:32Z

src/wallet/feebumper.cpp

totalFee isn't a thing

I'll rewrite these error messages.

promag · 2019-07-31T01:03:32Z

Concept ACK. Some notes:

try to keep commits organized
please see git log and pick one of the commit message format

ezegom · 2019-07-31T01:14:25Z

Concept ACK. Some notes:

try to keep commits organized

please see git log and pick one of the commit message format

@promag Thanks for your feedback.
I'll change the commits from "<rpc>" to "rpc:". My bad, should've taken a look at the git log before assuming this style.
Regarding the organization, what do you feel is not organized?

maflcko · 2019-07-31T11:46:17Z

Could rebase on master and modify the release notes in the section "removed or deprecated rpcs"?

Sjors

Concept ACK.

In the description, if you replace "Draft commit for issue" with "Fixes" then Github will automatically close that issue when this PR is merged, which saves maintainers some effort.

Regarding the organization, what do you feel is not organized?

You should use git interactive rebase to reduce the number of commits, for example:

commit linter: fix whitespace: add this into the commit that added the incorrect whitespace (fixup)
commits rpc: test bumpfee feeRate argument: squash these together
commit doc: Add release note...: drop this commit, because #16504

This makes it easier to review a pull request one commit at a time.

commit Changes the verbosity of msbuild: move to separate pull request

Sjors · 2019-08-02T08:29:04Z

test/functional/wallet_bumpfee.py

Nit: avoid dropping or adding whitespace in code that you're not touching.

ezegom · 2019-08-02T13:28:36Z

Thanks @Sjors,
I cleaned up the commits accordingly.

Sjors

Thanks for condensing the commits.

You can probably remove the WIP and Draft status; those are meant to prevent too detailed review, but it's too late for that :-)

There's still quite a bit of duplicate code between CheckFeeRate and CreateTotalBumpTransaction. Can you add an additional commit, similar to rpc: bumpfee move feeRate estimation logic into sepparate method, where you move this to a helper function? I'll review the actual functionality in more detail after that.

There's a couple of places where you or your editor fixed stuff in code that you're not otherwise touching. This increases review burden, because it's harder to see what changes are relevant, so is best avoided. I left a bunch of inline comments for that.

Couple more nits:

commit rpc: test bumpfee feeRate argument should be named [test] rpc bumpfee feeRate argument

Sjors · 2019-08-02T15:10:24Z

doc/release-notes.md

Nit: bumped

I initially wrote it as 'bumped' but went with 'bump'. My thought was that the bumped transaction is the one we are replacing, and the bump transaction is the new one?

the fee for the new transaction.

See

bitcoin/src/wallet/rpcwallet.cpp

Line 3274 in 3a3d8b8

"\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"

thats better, fixed.

Sjors · 2019-08-02T15:11:33Z

src/wallet/rpcwallet.cpp

Nit: move whitespace fix to commit where the whitespace mistake was made

Sjors · 2019-08-02T15:15:24Z

src/wallet/rpcwallet.cpp

Nit: don't remove whitespace here

Sjors · 2019-08-02T15:15:44Z

src/wallet/feebumper.cpp

Nit: you're not touching this function, so don't fix the brackets please

Sjors · 2019-08-02T15:28:31Z

src/wallet/feebumper.cpp

nit: don't fix the include order; that's best done when you add a new include

Sjors · 2019-08-02T15:30:29Z

src/wallet/feebumper.cpp

nit: don't fix things you're not otherwise touching

fixed this too

Sjors · 2019-08-02T15:30:50Z

src/wallet/feebumper.cpp

nit: don't fix this

ezegom · 2019-08-02T20:50:20Z

@Sjors I should be done with your comments, let me know if there is anything missing. Only thing missing is to get rid of the code re-use on CreateTotalTransaction.

DrahtBot · 2019-08-02T22:06:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16690 (Prepare release notes for 0.19 by harding)
#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.

instagibbs · 2019-08-06T20:27:16Z

src/wallet/rpcwallet.cpp

+        if (options.exists("confTarget") && (options.exists("totalFee") || options.exists("feeRate"))) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget can't be set with totalFee or feeRate. Please provide either a conftarget for fee estimation, or an explicit total fee or fee rate for the transaction.");
+        } else if (options.exists("feeRate") && options.exists("totalFee")) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "feeRate can't be set along with totalFee. Please provide either a total fee or a fee rate (in satoshis per K) for the transaction.");


Where else are we using satoshis per Kb? Seems like we're using BTC/kB everywhere else unless I'm missing a use?

You are correct, I used Satoshis per kB on my first iteration of this feature and forgot to change the error message. nice catch

instagibbs · 2019-08-19T14:49:46Z

(couldn't reach you on IRC) still working on this?

DrahtBot · 2019-08-24T03:17:38Z

Needs rebase

fanquake · 2019-08-27T01:08:34Z

Taken over in #16727.

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

ezegom closed this Jul 30, 2019

ezegom reopened this Jul 30, 2019

fanquake added the RPC/REST/ZMQ label Jul 30, 2019

fanquake changed the title ~~<WIP> Add feeRate argument to bumpFee RPC~~ WIP: Add feeRate argument to bumpFee RPC Jul 30, 2019

ezegom force-pushed the feerate_bumpfee branch from 0d8adc9 to c828209 Compare July 30, 2019 14:05

instagibbs reviewed Jul 30, 2019

View reviewed changes

This was referenced Jul 30, 2019

rpc: Deprecate totalfee argument in bumpfee #15996

Merged

doc: Add release note for the deprecated totalFee option of bumpfee #16504

Merged

ezegom force-pushed the feerate_bumpfee branch 3 times, most recently from 5e564f6 to 9bc7a55 Compare August 1, 2019 13:43

Sjors suggested changes Aug 2, 2019

View reviewed changes

rpc: bumpfee move feeRate estimation logic into sepparate method

f3027d1

ezegom force-pushed the feerate_bumpfee branch 2 times, most recently from 27fb2f1 to e1fd79a Compare August 2, 2019 13:26

ezegom force-pushed the feerate_bumpfee branch 3 times, most recently from 48d3691 to 002dbde Compare August 2, 2019 14:24

Sjors reviewed Aug 2, 2019

View reviewed changes

Sjors mentioned this pull request Aug 2, 2019

[rpc] signer send and fee bump convenience methods #15876

Closed

ezegom force-pushed the feerate_bumpfee branch from 002dbde to 90b34f3 Compare August 2, 2019 20:45

ezegom force-pushed the feerate_bumpfee branch from 90b34f3 to a8905e5 Compare August 2, 2019 21:58

rpc: Add feeRate argument to bumpFee method

c30fe97

ezegom added 2 commits August 2, 2019 18:01

rpc: bumpfee check feeRate argument

be1997e

rpc: test bumpfee feeRate argument

f9e6d1b

ezegom force-pushed the feerate_bumpfee branch from a8905e5 to 5afbeb9 Compare August 2, 2019 22:02

ezegom changed the title ~~WIP: Add feeRate argument to bumpFee RPC~~ rpc: Add feeRate argument to bumpFee RPC Aug 2, 2019

ezegom marked this pull request as ready for review August 2, 2019 22:10

doc: Update release-notes

3e7d71b

ezegom force-pushed the feerate_bumpfee branch from 5afbeb9 to 3e7d71b Compare August 2, 2019 23:21

instagibbs mentioned this pull request Aug 6, 2019

bumpfee.TotalFee removed without replacement #16485

Closed

instagibbs reviewed Aug 6, 2019

View reviewed changes

DrahtBot added the Needs rebase label Aug 24, 2019

instagibbs mentioned this pull request Aug 26, 2019

wallet: Explicit feerate for bumpfee #16727

Merged

fanquake added the Up for grabs label Aug 26, 2019

fanquake closed this Aug 27, 2019

fanquake removed the Up for grabs label Aug 27, 2019

laanwj removed the Needs rebase label Oct 24, 2019

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

rpc: Add feeRate argument to bumpFee RPC #16492

rpc: Add feeRate argument to bumpFee RPC #16492

Uh oh!

Conversation

ezegom commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Jul 30, 2019

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!

promag commented Jul 31, 2019

Uh oh!

ezegom commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 31, 2019

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!

ezegom commented Aug 2, 2019

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

ezegom commented Aug 2, 2019

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezegom commented Jul 30, 2019 •

edited

Loading

ezegom commented Jul 31, 2019 •

edited

Loading

promag Aug 2, 2019 •

edited

Loading

ezegom Aug 2, 2019 •

edited

Loading

DrahtBot commented Aug 2, 2019 •

edited

Loading