Skip to content

Commit

Permalink
Merge #20220: wallet, rpc: explicit fee rate follow-ups/fixes for 0.21
Browse files Browse the repository at this point in the history 
0be2900 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c005 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e6 wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea42 test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40 wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc57217 wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427e wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)

Pull request description:

  Follow-up to #11413 providing a base to build on for #19543:

  - bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
  - adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
  - improves a few related RPC error messages and `ParseConfirmTarget()` / error message
  - fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units

  This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.

ACKs for top commit:
  kallewoof:
    Concept/Tested ACK 0be2900
  meshcollider:
    Code review + functional test run ACK 0be2900

Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
  • Loading branch information
@meshcollider
meshcollider committed Nov 4, 2020
2 parents 17c6fb1 + 0be2900 commit 5d32009
Show file tree
Hide file tree
Showing 7 changed files with 282 additions and 95 deletions.
9 changes: 5 additions & 4 deletions src/rpc/util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,12 @@ UniValue DescribeAddress(const CTxDestination& dest)

unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target)
{
int target = value.get_int();
if (target < 1 || (unsigned int)target > max_target) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u - %u", 1, max_target));
const int target{value.get_int()};
const unsigned int unsigned_target{static_cast<unsigned int>(target)};
if (target < 1 || unsigned_target > max_target) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u and %u", 1, max_target));
}
return (unsigned int)target;
return unsigned_target;
}

RPCErrorCode RPCErrorFromTransactionError(TransactionError terr)
Expand Down
38 changes: 22 additions & 16 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U

if (cc.m_fee_mode == FeeEstimateMode::BTC_KB || cc.m_fee_mode == FeeEstimateMode::SAT_B) {
if (estimate_param.isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected estimate_mode requires a fee rate");
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Selected estimate_mode %s requires a fee rate to be specified in conf_target", estimate_mode.get_str()));
}

CAmount fee_rate = AmountFromValue(estimate_param);
Expand Down Expand Up @@ -440,7 +440,8 @@ static RPCHelpMan sendtoaddress()
{"subtractfeefromamount", RPCArg::Type::BOOL, /* default */ "false", "The fee will be deducted from the amount being sent.\n"
"The recipient will receive less bitcoins than you enter in the amount field."},
{"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
{"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n"
Expand Down Expand Up @@ -868,7 +869,8 @@ static RPCHelpMan sendmany()
},
},
{"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
{"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."},
Expand Down Expand Up @@ -3044,7 +3046,7 @@ static RPCHelpMan listunspent()
};
}

void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl)
void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl)
{
// Make sure the results are valid at least up to the most recent block
// the user could have gotten from another RPC command prior to now
Expand Down Expand Up @@ -3210,7 +3212,8 @@ static RPCHelpMan fundrawtransaction()
},
{"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Marks this transaction as BIP125 replaceable.\n"
"Allows this transaction to be replaced by a transaction with higher fees"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
},
Expand Down Expand Up @@ -3378,7 +3381,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
"The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist.\n"
"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"
"By default, the new fee will be calculated automatically using the estimatesmartfee RPC.\n"
"The user can specify a confirmation target for estimatesmartfee.\n"
"Alternatively, the user can specify a 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"
Expand All @@ -3387,10 +3390,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
{"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The txid to be bumped"},
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
{
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"},
{"fee_rate", RPCArg::Type::NUM, /* default */ "fall back to 'conf_target'", "fee rate (NOT total fee) to pay, in " + CURRENCY_UNIT + " per kB\n"
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"fee_rate", RPCArg::Type::NUM, /* default */ "fall back to 'conf_target'", "fee rate (NOT total fee) to pay, in " + CURRENCY_UNIT + "/kB.\n"
"Specify a fee rate instead of relying on the built-in fee estimator.\n"
"Must be at least 0.0001 " + CURRENCY_UNIT + " per kB higher than the current transaction fee rate.\n"},
"Must be at least 0.0001 " + CURRENCY_UNIT + "/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"
Expand Down Expand Up @@ -3464,9 +3468,8 @@ static RPCHelpMan bumpfee_helper(std::string method_name)

if (!conf_target.isNull()) {
if (options.exists("fee_rate")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "conf_target can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
}
coin_control.m_confirm_target = ParseConfirmTarget(conf_target, pwallet->chain().estimateMaxBlocks());
} else if (options.exists("fee_rate")) {
CFeeRate fee_rate(AmountFromValue(options["fee_rate"]));
if (fee_rate <= CFeeRate(0)) {
Expand Down Expand Up @@ -4014,7 +4017,8 @@ static RPCHelpMan send()
},
},
},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
Expand All @@ -4024,7 +4028,8 @@ static RPCHelpMan send()
{"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The bitcoin address to receive the change"},
{"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
{"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if change_address is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
{"include_watching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n"
Expand All @@ -4040,7 +4045,7 @@ static RPCHelpMan send()
{"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
{"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
{"psbt", RPCArg::Type::BOOL, /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."},
{"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A JSON array of integers.\n"
{"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "Outputs to subtract the fee from, specified as integer indices.\n"
"The fee will be equally deducted from the amount of each specified output.\n"
"Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n"
"If no outputs are specified here, the sender pays the fee.",
Expand Down Expand Up @@ -4121,7 +4126,7 @@ static RPCHelpMan send()
CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
CCoinControl coin_control;
// Automatically select coins, unless at least one is manually selected. Can
// be overriden by options.add_inputs.
// be overridden by options.add_inputs.
coin_control.m_add_inputs = rawTx.vin.size() == 0;
FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control);

Expand Down Expand Up @@ -4362,7 +4367,8 @@ static RPCHelpMan walletcreatefundedpsbt()
},
{"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Marks this transaction as BIP125 replaceable.\n"
"Allows this transaction to be replaced by a transaction with higher fees"},
{"conf_target", RPCArg::Type::NUM, /* default */ "fall back to wallet's confirmation target (txconfirmtarget)", "Confirmation target (in blocks)"},
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
"or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
},
Expand Down
55 changes: 55 additions & 0 deletions test/functional/rpc_fundrawtransaction.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from test_framework.descriptors import descsum_create
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_approx,
assert_equal,
assert_fee_amount,
assert_greater_than,
Expand Down Expand Up @@ -89,6 +90,7 @@ def run_test(self):
self.test_op_return()
self.test_watchonly()
self.test_all_watched_funds()
self.test_feerate_with_conf_target_and_estimate_mode()
self.test_option_feerate()
self.test_address_reuse()
self.test_option_subtract_fee_from_outputs()
Expand Down Expand Up @@ -722,6 +724,59 @@ def test_option_feerate(self):
assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate)
assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate)

def test_feerate_with_conf_target_and_estimate_mode(self):
self.log.info("Test fundrawtxn passing an explicit fee rate using conf_target and estimate_mode")
node = self.nodes[3]
# Make sure there is exactly one input so coin selection can't skew the result.
assert_equal(len(node.listunspent(1)), 1)
inputs = []
outputs = {node.getnewaddress() : 1}
rawtx = node.createrawtransaction(inputs, outputs)

for unit, fee_rate in {"btc/kb": 0.1, "sat/b": 10000}.items():
self.log.info("Test fundrawtxn with conf_target {} estimate_mode {} produces expected fee".format(fee_rate, unit))
# With no arguments passed, expect fee of 141 sats/b.
assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000141, vspan=0.00000001)
# Expect fee to be 10,000x higher when explicit fee 10,000x greater is specified.
result = node.fundrawtransaction(rawtx, {"conf_target": fee_rate, "estimate_mode": unit})
assert_approx(result["fee"], vexp=0.0141, vspan=0.0001)

for field, fee_rate in {"conf_target": 0.1, "estimate_mode": "sat/b"}.items():
self.log.info("Test fundrawtxn raises RPC error if both feeRate and {} are passed".format(field))
assert_raises_rpc_error(
-8, "Cannot specify both {} and feeRate".format(field),
lambda: node.fundrawtransaction(rawtx, {"feeRate": 0.1, field: fee_rate}))

self.log.info("Test fundrawtxn with invalid estimate_mode settings")
for k, v in {"number": 42, "object": {"foo": "bar"}}.items():
assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k),
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": v, "conf_target": 0.1}))
for mode in ["foo", Decimal("3.141592")]:
assert_raises_rpc_error(-8, "Invalid estimate_mode parameter",
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": 0.1}))

self.log.info("Test fundrawtxn with invalid conf_target settings")
for mode in ["unset", "economical", "conservative", "btc/kb", "sat/b"]:
self.log.debug("{}".format(mode))
for k, v in {"string": "", "object": {"foo": "bar"}}.items():
assert_raises_rpc_error(-3, "Expected type number for conf_target, got {}".format(k),
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": v}))
if mode in ["btc/kb", "sat/b"]:
assert_raises_rpc_error(-3, "Amount out of range",
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": -1}))
assert_raises_rpc_error(-4, "Fee rate (0.00000000 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": 0}))
else:
for n in [-1, 0, 1009]:
assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008",
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": n}))

for unit, fee_rate in {"sat/B": 0.99999999, "BTC/kB": 0.00000999}.items():
self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit))
assert_raises_rpc_error(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True}))


def test_address_reuse(self):
"""Test no address reuse occurs."""
self.log.info("Test fundrawtxn does not reuse addresses")
Expand Down
Loading

0 comments on commit 5d32009

Please sign in to comment.