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

@@ -57,6 +57,58 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
    return feebumper::Result::OK;
 }

+//! Check if the user provided a valid feeRate 
+static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, std::vector<std::string>& errors) {
+    // check that fee rate is higher than mempool's minimum fee


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

Yeah, this code is from the CreateTotalBumpTransaction method

bitcoin/src/wallet/feebumper.cpp

Lines 199 to 247 in b89249e

// calculate the old fee and fee-rate

old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();

CFeeRate nOldFeeRate(old_fee, txSize);

// The wallet uses a 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.

CFeeRate nodeIncrementalRelayFee = wallet->chain().relayIncrementalFee();

CFeeRate walletIncrementalRelayFee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE);

if (nodeIncrementalRelayFee > walletIncrementalRelayFee) {

walletIncrementalRelayFee = nodeIncrementalRelayFee;

}

CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + nodeIncrementalRelayFee.GetFee(maxNewTxSize);

if (total_fee < minTotalFee) {

errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",

FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(nodeIncrementalRelayFee.GetFee(maxNewTxSize))));

return Result::INVALID_PARAMETER;

}

CAmount requiredFee = GetRequiredFee(*wallet, maxNewTxSize);

if (total_fee < requiredFee) {

errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",

FormatMoney(requiredFee)));

return Result::INVALID_PARAMETER;

}

// Check that in all cases the new fee doesn't violate maxTxFee

const CAmount max_tx_fee = wallet->m_default_max_tx_fee;

if (new_fee > max_tx_fee) {

errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",

FormatMoney(new_fee), FormatMoney(max_tx_fee)));

return Result::WALLET_ERROR;

}

// check that fee rate is higher than mempool's minimum fee

// (no point in bumping fee if we know that the new tx won't be accepted to the mempool)

// This may occur if the user set TotalFee or paytxfee too low, if fallbackfee is too low, or, perhaps,

// in a rare situation where the mempool minimum fee increased significantly since the fee estimation just a

// moment earlier. In this case, we report an error to the user, who may use total_fee to make an adjustment.

CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee();

CFeeRate nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);

if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {

errors.push_back(strprintf(

"New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "

"the totalFee value should be at least %s to add transaction",

FormatMoney(nNewFeeRate.GetFeePerK()),

FormatMoney(minMempoolFeeRate.GetFeePerK()),

FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize))));

return Result::WALLET_ERROR;

}

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

src/wallet/feebumper.cpp

+    CAmount minTotalFee = nOldFeeRate.GetFee(maxTxSize) + incrementalRelayFee.GetFee(maxTxSize);
+
+    if (new_total_fee < minTotalFee) {
+        errors.push_back(strprintf("Insufficient totalFee %s, must be at least %s (oldFee %s + incrementalFee %s)",


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 doc: Add release note for the deprecated totalFee option of bumpfee #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

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

-


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

@@ -92,7 +92,9 @@ Deprecated or removed RPCs
 --------------------------

 - The `totalFee` option of the `bumpfee` RPC has been deprecated and will be
-  removed in 0.20.  To continue using this option start with
+  removed in 0.20. The `feeRate` option is now the preferred way to specify
+  the fee for the bump transaction.


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

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


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

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

src/wallet/rpcwallet.cpp

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


Nit: don't remove whitespace here

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

src/wallet/feebumper.cpp

@@ -304,7 +361,8 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
    return Result::OK;
 }

-bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {


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

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

src/wallet/feebumper.cpp

@@ -4,16 +4,16 @@

 #include <consensus/validation.h>
 #include <interfaces/chain.h>
-#include <wallet/coincontrol.h>


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

@@ -71,8 +152,7 @@ bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
    return res == feebumper::Result::OK;
 }

-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)
+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)


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

fixed this too

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

src/wallet/feebumper.cpp

-                               FormatMoney(new_fee), FormatMoney(max_tx_fee)));
-         return Result::WALLET_ERROR;
-     }
+    const CAmount max_tx_fee = wallet->m_default_max_tx_fee;


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

	// calculate the old fee and fee-rate
	old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
	CFeeRate nOldFeeRate(old_fee, txSize);
	// The wallet uses a 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.
	CFeeRate nodeIncrementalRelayFee = wallet->chain().relayIncrementalFee();
	CFeeRate walletIncrementalRelayFee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE);
	if (nodeIncrementalRelayFee > walletIncrementalRelayFee) {
	walletIncrementalRelayFee = nodeIncrementalRelayFee;
	}

	CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + nodeIncrementalRelayFee.GetFee(maxNewTxSize);
	if (total_fee < minTotalFee) {
	errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
	FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(nodeIncrementalRelayFee.GetFee(maxNewTxSize))));
	return Result::INVALID_PARAMETER;
	}
	CAmount requiredFee = GetRequiredFee(*wallet, maxNewTxSize);
	if (total_fee < requiredFee) {
	errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
	FormatMoney(requiredFee)));
	return Result::INVALID_PARAMETER;
	}

	// Check that in all cases the new fee doesn't violate maxTxFee
	const CAmount max_tx_fee = wallet->m_default_max_tx_fee;
	if (new_fee > max_tx_fee) {
	errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",
	FormatMoney(new_fee), FormatMoney(max_tx_fee)));
	return Result::WALLET_ERROR;
	}

	// check that fee rate is higher than mempool's minimum fee
	// (no point in bumping fee if we know that the new tx won't be accepted to the mempool)
	// This may occur if the user set TotalFee or paytxfee too low, if fallbackfee is too low, or, perhaps,
	// in a rare situation where the mempool minimum fee increased significantly since the fee estimation just a
	// moment earlier. In this case, we report an error to the user, who may use total_fee to make an adjustment.
	CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee();
	CFeeRate nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
	if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
	errors.push_back(strprintf(
	"New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "
	"the totalFee value should be at least %s to add transaction",
	FormatMoney(nNewFeeRate.GetFeePerK()),
	FormatMoney(minMempoolFeeRate.GetFeePerK()),
	FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize))));
	return Result::WALLET_ERROR;
	}

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

rpc: Add feeRate argument to bumpFee RPC #16492

rpc: Add feeRate argument to bumpFee RPC #16492

Conversation

ezegom commented Jul 30, 2019 • edited Loading

instagibbs commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag commented Jul 31, 2019

ezegom commented Jul 31, 2019 • edited Loading

maflcko commented Jul 31, 2019

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezegom commented Aug 2, 2019

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

ezegom Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezegom commented Aug 2, 2019

DrahtBot commented Aug 2, 2019 • edited Loading

Conflicts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Aug 19, 2019

DrahtBot commented Aug 24, 2019

fanquake commented Aug 27, 2019

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