Merge #16727: wallet: Explicit feerate for bumpfee
c812aba394
test bumpfee fee_rate argument (ezegom)9f25de3d9e
rpc bumpfee check fee_rate argument (ezegom)88e5f997df
rpc bumpfee: add fee_rate argument (ezegom)1a4c791cf4
rpc bumpfee: move feerate estimation logic into separate method (ezegom) Pull request description: Taking over for https://github.com/bitcoin/bitcoin/pull/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 ACKc812aba
laanwj: ACKc812aba394
Tree-SHA512: 5f7f51bd780a573ccef1ccd72b0faf3e5d143f6551060a667560c5163f7d9480e17e73775d1d7bcac0463f3b6b4328f0cff7b27e39483bddc42a530f4583ce30
This commit is contained in:
commit
8afa602f30
3 changed files with 131 additions and 33 deletions
|
@ -57,6 +57,86 @@ 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
|
||||
// (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 FeeRate, 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 adjust the fee.
|
||||
CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee();
|
||||
|
||||
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 -- ",
|
||||
FormatMoney(newFeerate.GetFeePerK()),
|
||||
FormatMoney(minMempoolFeeRate.GetFeePerK())));
|
||||
return feebumper::Result::WALLET_ERROR;
|
||||
}
|
||||
|
||||
CAmount new_total_fee = newFeerate.GetFee(maxTxSize);
|
||||
|
||||
CFeeRate incrementalRelayFee = std::max(wallet->chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE));
|
||||
|
||||
// Given old total fee and transaction size, calculate the old feeRate
|
||||
CAmount old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
|
||||
const int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
|
||||
CFeeRate nOldFeeRate(old_fee, txSize);
|
||||
// Min total fee is old fee + relay fee
|
||||
CAmount minTotalFee = nOldFeeRate.GetFee(maxTxSize) + incrementalRelayFee.GetFee(maxTxSize);
|
||||
|
||||
if (new_total_fee < minTotalFee) {
|
||||
errors.push_back(strprintf("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)",
|
||||
FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxTxSize)), FormatMoney(incrementalRelayFee.GetFee(maxTxSize))));
|
||||
return feebumper::Result::INVALID_PARAMETER;
|
||||
}
|
||||
|
||||
CAmount requiredFee = GetRequiredFee(*wallet, maxTxSize);
|
||||
if (new_total_fee < requiredFee) {
|
||||
errors.push_back(strprintf("Insufficient total fee (cannot be less than required fee %s)",
|
||||
FormatMoney(requiredFee)));
|
||||
return feebumper::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_total_fee > max_tx_fee) {
|
||||
errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",
|
||||
FormatMoney(new_total_fee), FormatMoney(max_tx_fee)));
|
||||
return feebumper::Result::WALLET_ERROR;
|
||||
}
|
||||
|
||||
return feebumper::Result::OK;
|
||||
}
|
||||
|
||||
static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee)
|
||||
{
|
||||
// Get the fee rate of the original transaction. This is calculated from
|
||||
// the tx fee/vsize, so it may have been rounded down. Add 1 satoshi to the
|
||||
// result.
|
||||
old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
|
||||
int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
|
||||
CFeeRate feerate(old_fee, txSize);
|
||||
feerate += CFeeRate(1);
|
||||
|
||||
// The node has a configurable incremental relay fee. Increment the fee by
|
||||
// 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
|
||||
// (BIP 125 rule 4). The replacement tx will be at least as large as the
|
||||
// original tx, so the total fee will be greater (BIP 125 rule 3)
|
||||
CFeeRate node_incremental_relay_fee = wallet->chain().relayIncrementalFee();
|
||||
CFeeRate wallet_incremental_relay_fee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE);
|
||||
feerate += std::max(node_incremental_relay_fee, wallet_incremental_relay_fee);
|
||||
|
||||
// Fee rate must also be at least the wallet's GetMinimumFeeRate
|
||||
CFeeRate min_feerate(GetMinimumFeeRate(*wallet, coin_control, /* feeCalc */ nullptr));
|
||||
|
||||
// Set the required fee rate for the replacement transaction in coin control.
|
||||
return std::max(feerate, min_feerate);
|
||||
}
|
||||
|
||||
namespace feebumper {
|
||||
|
||||
bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
|
||||
|
@ -230,31 +310,18 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
|
|||
}
|
||||
}
|
||||
|
||||
// Get the fee rate of the original transaction. This is calculated from
|
||||
// the tx fee/vsize, so it may have been rounded down. Add 1 satoshi to the
|
||||
// result.
|
||||
old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
|
||||
int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
|
||||
// Feerate of thing we are bumping
|
||||
CFeeRate feerate(old_fee, txSize);
|
||||
feerate += CFeeRate(1);
|
||||
|
||||
// The node has a configurable incremental relay fee. Increment the fee by
|
||||
// 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
|
||||
// (BIP 125 rule 4). The replacement tx will be at least as large as the
|
||||
// original tx, so the total fee will be greater (BIP 125 rule 3)
|
||||
CFeeRate node_incremental_relay_fee = wallet->chain().relayIncrementalFee();
|
||||
CFeeRate wallet_incremental_relay_fee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE);
|
||||
feerate += std::max(node_incremental_relay_fee, wallet_incremental_relay_fee);
|
||||
|
||||
// Fee rate must also be at least the wallet's GetMinimumFeeRate
|
||||
CFeeRate min_feerate(GetMinimumFeeRate(*wallet, new_coin_control, /* feeCalc */ nullptr));
|
||||
|
||||
// Set the required fee rate for the replacement transaction in coin control.
|
||||
new_coin_control.m_feerate = std::max(feerate, min_feerate);
|
||||
if (coin_control.m_feerate) {
|
||||
// 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);
|
||||
if (res != Result::OK) {
|
||||
return res;
|
||||
}
|
||||
} else {
|
||||
// The user did not provide a feeRate argument
|
||||
new_coin_control.m_feerate = EstimateFeeRate(wallet, wtx, new_coin_control, old_fee);
|
||||
}
|
||||
|
||||
// Fill in required inputs we are double-spending(all of them)
|
||||
// N.B.: bip125 doesn't require all the inputs in the replaced transaction to be
|
||||
|
|
|
@ -3309,7 +3309,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
|
|||
"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"
|
||||
"Alternatively, the user can specify totalFee (DEPRECATED), or use RPC settxfee to set a higher fee rate.\n"
|
||||
"Alternatively, the user can specify totalFee (DEPRECATED), or fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction .\n"
|
||||
"At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
|
||||
"returned by getnetworkinfo) to enter the node's mempool.\n",
|
||||
{
|
||||
|
@ -3321,6 +3321,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
|
|||
" In rare cases, the actual fee paid might be slightly higher than the specified\n"
|
||||
" totalFee if the tx change output has to be removed because it is too close to\n"
|
||||
" the dust threshold."},
|
||||
{"fee_rate", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "FeeRate (NOT total fee) to pay, in " + CURRENCY_UNIT + " per kB\n"
|
||||
" Specify a fee rate instead of relying on the built-in fee estimator.\n"
|
||||
" Must be at least 0.0001 BTC per kB higher than the current transaction fee rate.\n"},
|
||||
{"replaceable", RPCArg::Type::BOOL, /* default */ "true", "Whether the new transaction should still be\n"
|
||||
" marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
|
||||
" be left unchanged from the original. If false, any input sequence numbers in the\n"
|
||||
|
@ -3362,13 +3365,15 @@ static UniValue bumpfee(const JSONRPCRequest& request)
|
|||
{
|
||||
{"confTarget", UniValueType(UniValue::VNUM)},
|
||||
{"totalFee", UniValueType(UniValue::VNUM)},
|
||||
{"fee_rate", UniValueType(UniValue::VNUM)},
|
||||
{"replaceable", UniValueType(UniValue::VBOOL)},
|
||||
{"estimate_mode", UniValueType(UniValue::VSTR)},
|
||||
},
|
||||
true, true);
|
||||
|
||||
if (options.exists("confTarget") && options.exists("totalFee")) {
|
||||
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.");
|
||||
if (options.exists("confTarget") && (options.exists("totalFee") || options.exists("fee_rate"))) {
|
||||
throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget can't be set with totalFee or fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
|
||||
} else if (options.exists("fee_rate") && options.exists("totalFee")) {
|
||||
throw JSONRPCError(RPC_INVALID_PARAMETER, "fee_rate can't be set along with totalFee.");
|
||||
} else if (options.exists("confTarget")) { // TODO: alias this to conf_target
|
||||
coin_control.m_confirm_target = ParseConfirmTarget(options["confTarget"], pwallet->chain().estimateMaxBlocks());
|
||||
} else if (options.exists("totalFee")) {
|
||||
|
@ -3379,6 +3384,12 @@ static UniValue bumpfee(const JSONRPCRequest& request)
|
|||
if (totalFee <= 0) {
|
||||
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee %s (must be greater than 0)", FormatMoney(totalFee)));
|
||||
}
|
||||
} else if (options.exists("fee_rate")) {
|
||||
CFeeRate fee_rate(AmountFromValue(options["fee_rate"]));
|
||||
if (fee_rate <= 0) {
|
||||
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate.ToString()));
|
||||
}
|
||||
coin_control.m_feerate = fee_rate;
|
||||
}
|
||||
|
||||
if (options.exists("replaceable")) {
|
||||
|
|
|
@ -67,7 +67,9 @@ class BumpFeeTest(BitcoinTestFramework):
|
|||
|
||||
self.log.info("Running tests")
|
||||
dest_address = peer_node.getnewaddress()
|
||||
test_simple_bumpfee_succeeds(self, rbf_node, peer_node, dest_address)
|
||||
test_simple_bumpfee_succeeds(self, "default", rbf_node, peer_node, dest_address)
|
||||
test_simple_bumpfee_succeeds(self, "fee_rate", rbf_node, peer_node, dest_address)
|
||||
test_feerate_args(self, rbf_node, peer_node, dest_address)
|
||||
test_segwit_bumpfee_succeeds(rbf_node, dest_address)
|
||||
test_nonrbf_bumpfee_fails(peer_node, dest_address)
|
||||
test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address)
|
||||
|
@ -88,12 +90,15 @@ class BumpFeeTest(BitcoinTestFramework):
|
|||
self.log.info("Success")
|
||||
|
||||
|
||||
def test_simple_bumpfee_succeeds(self, rbf_node, peer_node, dest_address):
|
||||
def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
|
||||
rbfid = spend_one_input(rbf_node, dest_address)
|
||||
rbftx = rbf_node.gettransaction(rbfid)
|
||||
self.sync_mempools((rbf_node, peer_node))
|
||||
assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
|
||||
bumped_tx = rbf_node.bumpfee(rbfid)
|
||||
if mode == "fee_rate":
|
||||
bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate":0.0015})
|
||||
else:
|
||||
bumped_tx = rbf_node.bumpfee(rbfid)
|
||||
assert_equal(bumped_tx["errors"], [])
|
||||
assert bumped_tx["fee"] - abs(rbftx["fee"]) > 0
|
||||
# check that bumped_tx propagates, original tx was evicted and has a wallet conflict
|
||||
|
@ -109,6 +114,22 @@ def test_simple_bumpfee_succeeds(self, rbf_node, peer_node, dest_address):
|
|||
assert_equal(oldwtx["replaced_by_txid"], bumped_tx["txid"])
|
||||
assert_equal(bumpedwtx["replaces_txid"], rbfid)
|
||||
|
||||
def test_feerate_args(self, rbf_node, peer_node, dest_address):
|
||||
rbfid = spend_one_input(rbf_node, dest_address)
|
||||
self.sync_mempools((rbf_node, peer_node))
|
||||
assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
|
||||
|
||||
assert_raises_rpc_error(-8, "confTarget can't be set with totalFee or fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.", rbf_node.bumpfee, rbfid, {"fee_rate":0.00001, "confTarget":1})
|
||||
assert_raises_rpc_error(-8, "confTarget can't be set with totalFee or fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.", rbf_node.bumpfee, rbfid, {"totalFee":0.00001, "confTarget":1})
|
||||
assert_raises_rpc_error(-8, "fee_rate can't be set along with totalFee.", rbf_node.bumpfee, rbfid, {"fee_rate":0.00001, "totalFee":0.001})
|
||||
|
||||
# Bumping to just above minrelay should fail to increase total fee enough, at least
|
||||
assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, {"fee_rate":0.00001000})
|
||||
|
||||
assert_raises_rpc_error(-3, "Amount out of range", rbf_node.bumpfee, rbfid, {"fee_rate":-1})
|
||||
|
||||
assert_raises_rpc_error(-4, "is too high (cannot be higher than", rbf_node.bumpfee, rbfid, {"fee_rate":1})
|
||||
|
||||
|
||||
def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
|
||||
# Create a transaction with segwit output, then create an RBF transaction
|
||||
|
@ -176,7 +197,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)
|
||||
|
||||
|
||||
def test_small_output_fails(rbf_node, dest_address):
|
||||
# cannot bump fee with a too-small output
|
||||
rbfid = spend_one_input(rbf_node, dest_address)
|
||||
|
|
Loading…
Add table
Reference in a new issue