rpc: Add feeRate argument to bumpFee RPC

ezegom commented at 0:34 am on July 30, 2019: contributor

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.

ezegom closed this on Jul 30, 2019

ezegom reopened this on Jul 30, 2019

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

fanquake renamed this:
~~<WIP> Add feeRate argument to bumpFee RPC~~
WIP: Add feeRate argument to bumpFee RPC
on Jul 30, 2019

ezegom force-pushed on Jul 30, 2019

instagibbs commented at 3:02 pm on July 30, 2019: member

concept ACK

in src/wallet/feebumper.cpp:65 in b89249e0de outdated

56@@ -57,6 +57,58 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
57     return feebumper::Result::OK;
58 }
59 
60+//! Check if the user provided a valid feeRate 
61+static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, std::vector<std::string>& errors) {
62+    // check that fee rate is higher than mempool's minimum fee

instagibbs commented at 3:05 pm on July 30, 2019:

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

ezegom commented at 3:35 pm on July 30, 2019:

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

in src/wallet/feebumper.cpp:89 in b89249e0de outdated

84+    CFeeRate nOldFeeRate(old_fee, txSize);
85+    // Min total fee is old fee + relay fee
86+    CAmount minTotalFee = nOldFeeRate.GetFee(maxTxSize) + incrementalRelayFee.GetFee(maxTxSize);
87+
88+    if (new_total_fee < minTotalFee) {
89+        errors.push_back(strprintf("Insufficient totalFee %s, must be at least %s (oldFee %s + incrementalFee %s)",

instagibbs commented at 3:06 pm on July 30, 2019:

totalFee isn’t a thing

ezegom commented at 3:36 pm on July 30, 2019:

I’ll rewrite these error messages.

promag commented at 1:03 am on July 31, 2019: member

Concept ACK. Some notes:

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

ezegom commented at 1:14 am on July 31, 2019: contributor

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?

MarcoFalke commented at 11:46 am on July 31, 2019: member

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

ezegom force-pushed on Jul 31, 2019

ezegom force-pushed on Aug 1, 2019

in test/functional/wallet_bumpfee.py:179 in 9bc7a55111 outdated

187@@ -176,7 +188,6 @@ def test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address)
188     rbf_node.sendrawtransaction(tx["hex"])
189     assert_raises_rpc_error(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
190 
191-

Sjors commented at 8:29 am on August 2, 2019:

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

ezegom commented at 1:27 pm on August 2, 2019:

fixed

Sjors changes_requested

Sjors commented at 8:35 am on August 2, 2019: member

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

rpc: bumpfee move feeRate estimation logic into sepparate method f3027d1645

ezegom force-pushed on Aug 2, 2019

ezegom commented at 1:28 pm on August 2, 2019: contributor

Thanks @Sjors, I cleaned up the commits accordingly.

ezegom force-pushed on Aug 2, 2019

in doc/release-notes.md:96 in 002dbdec9d outdated

91@@ -92,7 +92,9 @@ Deprecated or removed RPCs
92 --------------------------
93 
94 - The `totalFee` option of the `bumpfee` RPC has been deprecated and will be
95-  removed in 0.20.  To continue using this option start with
96+  removed in 0.20. The `feeRate` option is now the preferred way to specify
97+  the fee for the bump transaction.

Sjors commented at 3:10 pm on August 2, 2019:

Nit: bumped

ezegom commented at 8:13 pm on August 2, 2019:

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?

promag commented at 8:16 pm on August 2, 2019:

the fee for the new transaction.

See https://github.com/bitcoin/bitcoin/blob/3a3d8b83571205b8329d4ee25537e3cc4397c3b8/src/wallet/rpcwallet.cpp#L3274

ezegom commented at 8:34 pm on August 2, 2019:

thats better, fixed.

in src/wallet/rpcwallet.cpp:3363 in 0294fa7ed1 outdated

3360@@ -3361,7 +3361,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3361             if (fee_rate <= 0) {
3362                 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid feeRate %s (must be greater than 0)", fee_rate.ToString()));
3363             }
3364-            coin_control.m_feerate = fee_rate; 
3365+            coin_control.m_feerate = fee_rate;

Sjors commented at 3:11 pm on August 2, 2019:

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

ezegom commented at 8:46 pm on August 2, 2019:

fixed

in src/wallet/rpcwallet.cpp:3281 in a6bd4c1c8c outdated

3277@@ -3278,7 +3278,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3278                 "All inputs in the original transaction will be included in the replacement transaction.\n"
3279                 "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
3280                 "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
3281-                "The user can specify a confirmation target for estimatesmartfee.\n"

Sjors commented at 3:15 pm on August 2, 2019:

Nit: don’t remove whitespace here

ezegom commented at 8:47 pm on August 2, 2019:

fixed

in src/wallet/feebumper.cpp:298 in a6bd4c1c8c outdated

360@@ -304,7 +361,8 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
361     return Result::OK;
362 }
363 
364-bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {

Sjors commented at 3:15 pm on August 2, 2019:

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

ezegom commented at 8:47 pm on August 2, 2019:

fixed :)

in src/wallet/feebumper.cpp:7 in 002dbdec9d outdated

3@@ -4,16 +4,16 @@
4 
5 #include <consensus/validation.h>
6 #include <interfaces/chain.h>
7-#include <wallet/coincontrol.h>

Sjors commented at 3:28 pm on August 2, 2019:

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

ezegom commented at 8:47 pm on August 2, 2019:

fixed

in src/wallet/feebumper.cpp:155 in 002dbdec9d outdated

151@@ -71,8 +152,7 @@ bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
152     return res == feebumper::Result::OK;
153 }
154 
155-Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
156-                                  CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
157+Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors, CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)

Sjors commented at 3:30 pm on August 2, 2019:

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

ezegom commented at 8:47 pm on August 2, 2019:

fixed this too

in src/wallet/feebumper.cpp:225 in 002dbdec9d outdated

226-     if (new_fee > max_tx_fee) {
227-         errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",
228-                               FormatMoney(new_fee), FormatMoney(max_tx_fee)));
229-         return Result::WALLET_ERROR;
230-     }
231+    const CAmount max_tx_fee = wallet->m_default_max_tx_fee;

Sjors commented at 3:30 pm on August 2, 2019:

nit: don’t fix this

ezegom commented at 8:47 pm on August 2, 2019:

done

Sjors commented at 3:39 pm on August 2, 2019: member

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

ezegom force-pushed on Aug 2, 2019

ezegom commented at 8:50 pm on August 2, 2019: contributor

@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.

ezegom force-pushed on Aug 2, 2019

rpc: Add feeRate argument to bumpFee method c30fe97196

rpc: bumpfee check feeRate argument be1997e073

rpc: test bumpfee feeRate argument f9e6d1b3a4

ezegom force-pushed on Aug 2, 2019

DrahtBot commented at 10:06 pm on August 2, 2019: member

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.

ezegom renamed this:
~~WIP: Add feeRate argument to bumpFee RPC~~
rpc: Add feeRate argument to bumpFee RPC
on Aug 2, 2019

ezegom marked this as ready for review on Aug 2, 2019

doc: Update release-notes 3e7d71b6af

ezegom force-pushed on Aug 2, 2019

in src/wallet/rpcwallet.cpp:3346 in c30fe97196 outdated

3344-        if (options.exists("confTarget") && options.exists("totalFee")) {
3345-            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget and totalFee options should not both be set. Please provide either a confirmation target for fee estimation or an explicit total fee for the transaction.");
3346+        if (options.exists("confTarget") && (options.exists("totalFee") || options.exists("feeRate"))) {
3347+            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.");
3348+        } else if (options.exists("feeRate") && options.exists("totalFee")) {
3349+            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.");

instagibbs commented at 8:27 pm on August 6, 2019:

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

ezegom commented at 2:30 am on August 7, 2019:

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 commented at 2:49 pm on August 19, 2019: member

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

DrahtBot added the label Needs rebase on Aug 24, 2019

DrahtBot commented at 3:17 am on August 24, 2019: member

fanquake added the label Up for grabs on Aug 26, 2019

fanquake commented at 1:08 am on August 27, 2019: member

Taken over in #16727.

fanquake closed this on Aug 27, 2019

fanquake removed the label Up for grabs on Aug 27, 2019

laanwj referenced this in commit 8afa602f30 on Oct 2, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

rpc: Add feeRate argument to bumpFee RPC #16492

Conflicts