wallet: Explicit feerate for bumpfee

instagibbs commented at 3:25 pm on August 26, 2019: member

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.

rpc bumpfee: move feerate estimation logic into separate method 1a4c791cf4

in test/functional/wallet_bumpfee.py:98 in 664f08b53f outdated

94     rbfid = spend_one_input(rbf_node, dest_address)
95     rbftx = rbf_node.gettransaction(rbfid)
96     self.sync_mempools((rbf_node, peer_node))
97     assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
98-    bumped_tx = rbf_node.bumpfee(rbfid)
99+    if(mode == "fee_rate"):

promag commented at 3:41 pm on August 26, 2019:

Drop ().

instagibbs commented at 6:43 pm on September 13, 2019:

fixed

instagibbs force-pushed on Aug 26, 2019

instagibbs commented at 4:26 pm on August 26, 2019: member

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

instagibbs force-pushed on Aug 26, 2019

DrahtBot added the label RPC/REST/ZMQ on Aug 26, 2019

DrahtBot added the label Tests on Aug 26, 2019

DrahtBot added the label Wallet on Aug 26, 2019

DrahtBot commented at 7:13 pm on August 26, 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:

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

fanquake renamed this:
~~Explicit feerate for bumpfee~~
wallet: Explicit feerate for bumpfee
on Aug 27, 2019

fanquake removed the label Tests on Aug 27, 2019

Sjors commented at 10:01 am on August 27, 2019: member

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 commented at 10:41 am on August 27, 2019: none

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

Sjors commented at 11:41 am on August 27, 2019: member

@Relaxo143 you’ll be pleased to see #11413 then…

instagibbs commented at 11:45 am on August 27, 2019: member

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

On Tue, Aug 27, 2019, 6:44 AM Relaxo143 notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/16727?email_source=notifications&email_token=ABMAFUYWXNZF556SIQ4CPFTQGUAQXA5CNFSM4IPQ24D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5HJXSY#issuecomment-525245387, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMAFUZSS2H575PNQTKBVL3QGUAQXANCNFSM4IPQ24DQ .

promag commented at 3:43 pm on August 27, 2019: member

Concept ACK.

luke-jr commented at 11:12 pm on September 2, 2019: member

Renamed feeRate to fee_rate to reflect updated guidelines.

Every other RPC uses “feeRate”…

instagibbs commented at 1:28 pm on September 3, 2019: member

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 force-pushed on Sep 13, 2019

instagibbs commented at 6:45 pm on September 13, 2019: member

going to leave #11413 type ideas alone for now.

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

in src/wallet/rpcwallet.cpp:3354 in 33911b9d20 outdated

3352             },
3353             true, true);
3354-
3355-        if (options.exists("confTarget") && options.exists("totalFee")) {
3356-            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.");
3357+        // EZTODO: If FeeRate is passed in BUT its 0, give JSONRPCError.

MarcoFalke commented at 6:37 pm on September 17, 2019:

:eyes:

instagibbs commented at 6:42 pm on September 17, 2019:

lol, will change

instagibbs commented at 1:46 pm on September 23, 2019:

fixed

instagibbs force-pushed on Sep 23, 2019

MarcoFalke added this to the milestone 0.19.0 on Sep 23, 2019

in src/wallet/rpcwallet.cpp:3293 in ad4eb894b6 outdated

3289@@ -3290,7 +3290,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3290                 "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
3291                 "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
3292                 "The user can specify a confirmation target for estimatesmartfee.\n"
3293-                "Alternatively, the user can specify totalFee (DEPRECATED), or use RPC settxfee to set a higher fee rate.\n"
3294+                "Alternatively, the user can specify totalFee (DEPRECATED), or fee_rate (in satoshis per K) for the new transaction .\n"

Sjors commented at 5:15 pm on September 27, 2019:

“BTC per kB”

in src/wallet/rpcwallet.cpp:3305 in ad4eb894b6 outdated

3301@@ -3302,6 +3302,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3302             "                         In rare cases, the actual fee paid might be slightly higher than the specified\n"
3303             "                         totalFee if the tx change output has to be removed because it is too close to\n"
3304             "                         the dust threshold."},
3305+                            {"fee_rate", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "FeeRate (NOT total fee) to pay, in " + CURRENCY_UNIT + "\n"

Sjors commented at 5:22 pm on September 27, 2019:

CURRENCY_UNIT + "per kB\n"

in src/wallet/rpcwallet.cpp:3306 in ad4eb894b6 outdated

3301@@ -3302,6 +3302,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3302             "                         In rare cases, the actual fee paid might be slightly higher than the specified\n"
3303             "                         totalFee if the tx change output has to be removed because it is too close to\n"
3304             "                         the dust threshold."},
3305+                            {"fee_rate", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "FeeRate (NOT total fee) to pay, in " + CURRENCY_UNIT + "\n"
3306+            "                         The fee_rate argument allows the users to use their own fee estimator. The resulting bump transaction will "

Sjors commented at 5:25 pm on September 27, 2019:

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

in src/wallet/rpcwallet.cpp:3355 in ad4eb894b6 outdated

3353             true, true);
3354-
3355-        if (options.exists("confTarget") && options.exists("totalFee")) {
3356-            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.");
3357+        if (options.exists("confTarget") && (options.exists("totalFee") || options.exists("fee_rate"))) {
3358+            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget can't be set with totalFee or fee_rate. Please provide either a conftarget for fee estimation, or an explicit total fee or fee rate for the transaction.");

Sjors commented at 5:37 pm on September 27, 2019:

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

instagibbs commented at 11:34 am on September 28, 2019:

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

in src/wallet/rpcwallet.cpp:3368 in ad4eb894b6 outdated

3364@@ -3360,6 +3365,12 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3365             if (totalFee <= 0) {
3366                 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee %s (must be greater than 0)", FormatMoney(totalFee)));
3367             }
3368+        } else if (options.exists("fee_rate")) {

Sjors commented at 5:38 pm on September 27, 2019:

May want to check that it’s not null.

Sjors changes_requested

Sjors commented at 5:46 pm on September 27, 2019: member

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.

rpc bumpfee: add fee_rate argument 88e5f997df

rpc bumpfee check fee_rate argument 9f25de3d9e

instagibbs force-pushed on Sep 28, 2019

instagibbs commented at 11:39 am on September 28, 2019: member

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 commented at 9:23 am on September 30, 2019: member

The bumpfee test is failing in the CIs.

laanwj added the label Waiting for author on Sep 30, 2019

test bumpfee fee_rate argument c812aba394

instagibbs force-pushed on Sep 30, 2019

instagibbs commented at 1:40 pm on September 30, 2019: member

pushed fixed test

Sjors commented at 3:31 pm on September 30, 2019: member

Code review ACK c812aba

MarcoFalke added the label Needs release note on Oct 1, 2019

MarcoFalke removed the label Waiting for author on Oct 1, 2019

in src/wallet/feebumper.cpp:126 in c812aba394

121+
122+    // The node has a configurable incremental relay fee. Increment the fee by
123+    // the minimum of that and the wallet's conservative
124+    // WALLET_INCREMENTAL_RELAY_FEE value to future proof against changes to
125+    // network wide policy for incremental relay fee that our node may not be
126+    // aware of. This ensures we're over the over the required relay fee rate

laanwj commented at 1:26 pm on October 2, 2019:

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

in src/wallet/feebumper.cpp:317 in c812aba394

337-    new_coin_control.m_feerate = std::max(feerate, min_feerate);
338+    if (coin_control.m_feerate) {
339+        // The user provided a feeRate argument.
340+        // We calculate this here to avoid compiler warning on the cs_wallet lock
341+        const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
342+        Result res = CheckFeeRate(wallet, wtx, *(new_coin_control.m_feerate), maxTxSize, errors);

laanwj commented at 1:31 pm on October 2, 2019:

Why the set of parentheses around new_coin_control.m_feerate?

in src/wallet/feebumper.cpp:71 in c812aba394

66+    // moment earlier. In this case, we report an error to the user, who may adjust the fee.
67+    CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee();
68+
69+    if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
70+        errors.push_back(strprintf(
71+            "New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- ",

laanwj commented at 1:32 pm on October 2, 2019:

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

laanwj approved

laanwj commented at 1:41 pm on October 2, 2019: member

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

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

laanwj merged this on Oct 2, 2019

laanwj closed this on Oct 2, 2019

in src/wallet/feebumper.cpp:112 in c812aba394

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

MarcoFalke commented at 8:11 pm on October 9, 2019:

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

MarcoFalke commented at 8:13 pm on October 9, 2019: member

post merge ACK c812aba3949b6ab81030dc708cda7c8821be2f70

MarcoFalke removed the label Needs release note on Oct 9, 2019

MarcoFalke commented at 8:20 pm on October 9, 2019: member

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

MarcoFalke referenced this in commit e180be49d7 on Oct 15, 2019

sidhujag referenced this in commit 5b52c182ea on Oct 16, 2019

laanwj referenced this in commit 694f4cbd78 on Mar 26, 2020

DrahtBot locked this on Dec 16, 2021

wallet: Explicit feerate for bumpfee #16727

Conflicts