Skip to content

Commit

Permalink
wallet: override minfee checks (fOverrideFeeRate) for fee_rate
Browse files Browse the repository at this point in the history 
in RPCs fundrawtransaction and walletcreatefundedpsbt only.

This maintains the feeRate behavior in these two RPCs.

See this GitHub discussion for more:
bitcoin#10706
  • Loading branch information
@jonatack
jonatack committed Nov 12, 2020
1 parent 702b4a7 commit afe9471
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 33 deletions.
45 changes: 27 additions & 18 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,19 @@ static std::string LabelFromValue(const UniValue& value)
/**
* Update coin control with fee estimation based on the given parameters
*
* @param[in] pwallet Wallet pointer
* @param[in,out] cc Coin control which is to be updated
* @param[in] conf_target UniValue integer, confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
* if a fee_rate is present, 0 is allowed here as a no-op positional placeholder
* @param[in] estimate_mode UniValue string, fee estimation mode, valid values are "unset", "economical" or "conservative";
* if a fee_rate is present, "" is allowed here as a no-op positional placeholder
* @param[in] fee_rate UniValue real, fee rate in sat/vB;
* if a fee_rate is present, both conf_target and estimate_mode must either be null, or no-op values
* @param[in] pwallet Wallet pointer
* @param[in,out] cc Coin control to be updated
* @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
* if a fee_rate is present, 0 is allowed here as a no-op positional placeholder
* @param[in] estimate_mode UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative";
* if a fee_rate is present, "" is allowed here as a no-op positional placeholder
* @param[in] fee_rate UniValue real; fee rate in sat/vB;
* if a fee_rate is present, both conf_target and estimate_mode must either be null, or no-op
* @param[in] override_min_fee bool; whether to set fOverrideFeeRate to true to disable minimum fee rate checks and instead
* verify only that fee_rate is greater than 0
* @throws a JSONRPCError if conf_target, estimate_mode, or fee_rate contain invalid values or are in conflict
*/
static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate)
static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, bool override_min_fee)
{
if (!fee_rate.isNull()) {
if (!conf_target.isNull() && conf_target.get_int() > 0) {
Expand All @@ -216,7 +218,14 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U
if (!estimate_mode.isNull() && !estimate_mode.get_str().empty()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate");
}
cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);
CFeeRate fee_rate_in_sat_vb{CFeeRate(AmountFromValue(fee_rate), COIN)};
if (override_min_fee) {
if (fee_rate_in_sat_vb <= CFeeRate(0)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate_in_sat_vb.ToString(FeeEstimateMode::SAT_VB)));
}
cc.fOverrideFeeRate = true;
}
cc.m_feerate = fee_rate_in_sat_vb;
// Default RBF to true for explicit fee_rate, if unset.
if (cc.m_signal_bip125_rbf == nullopt) cc.m_signal_bip125_rbf = true;
return;
Expand Down Expand Up @@ -504,7 +513,7 @@ static RPCHelpMan sendtoaddress()
// We also enable partial spend avoidance if reuse avoidance is set.
coin_control.m_avoid_partial_spends |= coin_control.m_avoid_address_reuse;

SetFeeEstimateMode(pwallet, coin_control, /* conf_target */ request.params[6], /* estimate_mode */ request.params[7], /* fee_rate */ request.params[9]);
SetFeeEstimateMode(pwallet, coin_control, /* conf_target */ request.params[6], /* estimate_mode */ request.params[7], /* fee_rate */ request.params[9], /* override_min_fee */ false);

EnsureWalletIsUnlocked(pwallet);

Expand Down Expand Up @@ -932,7 +941,7 @@ static RPCHelpMan sendmany()
coin_control.m_signal_bip125_rbf = request.params[5].get_bool();
}

SetFeeEstimateMode(pwallet, coin_control, /* conf_target */ request.params[6], /* estimate_mode */ request.params[7], /* fee_rate */ request.params[8]);
SetFeeEstimateMode(pwallet, coin_control, /* conf_target */ request.params[6], /* estimate_mode */ request.params[7], /* fee_rate */ request.params[8], /* override_min_fee */ false);

std::vector<CRecipient> recipients;
ParseRecipients(sendTo, subtractFeeFromAmount, recipients);
Expand Down Expand Up @@ -3049,7 +3058,7 @@ static RPCHelpMan listunspent()
};
}

void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl)
void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee)
{
// 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 @@ -3154,7 +3163,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
if (options.exists("replaceable")) {
coinControl.m_signal_bip125_rbf = options["replaceable"].get_bool();
}
SetFeeEstimateMode(pwallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"]);
SetFeeEstimateMode(pwallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"], override_min_fee);
}
} else {
// if options is null and not a bool
Expand Down Expand Up @@ -3275,7 +3284,7 @@ static RPCHelpMan fundrawtransaction()
CCoinControl coin_control;
// Automatically select (additional) coins. Can be overridden by options.add_inputs.
coin_control.m_add_inputs = true;
FundTransaction(pwallet, tx, fee, change_position, request.params[1], coin_control);
FundTransaction(pwallet, tx, fee, change_position, request.params[1], coin_control, /* override_min_fee */ true);

UniValue result(UniValue::VOBJ);
result.pushKV("hex", EncodeHexTx(CTransaction(tx)));
Expand Down Expand Up @@ -3482,7 +3491,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
if (options.exists("replaceable")) {
coin_control.m_signal_bip125_rbf = options["replaceable"].get_bool();
}
SetFeeEstimateMode(pwallet, coin_control, conf_target, options["estimate_mode"], options["fee_rate"]);
SetFeeEstimateMode(pwallet, coin_control, conf_target, options["estimate_mode"], options["fee_rate"], /* override_min_fee */ false);
}

// Make sure the results are valid at least up to the most recent block
Expand Down Expand Up @@ -4146,7 +4155,7 @@ static RPCHelpMan send()
// Automatically select coins, unless at least one is manually selected. Can
// be overridden by options.add_inputs.
coin_control.m_add_inputs = rawTx.vin.size() == 0;
FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control);
FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control, /* override_min_fee */ false);

bool add_to_wallet = true;
if (options.exists("add_to_wallet")) {
Expand Down Expand Up @@ -4433,7 +4442,7 @@ static RPCHelpMan walletcreatefundedpsbt()
// Automatically select coins, unless at least one is manually selected. Can
// be overridden by options.add_inputs.
coin_control.m_add_inputs = rawTx.vin.size() == 0;
FundTransaction(pwallet, rawTx, fee, change_position, request.params[3], coin_control);
FundTransaction(pwallet, rawTx, fee, change_position, request.params[3], coin_control, /* override_min_fee */ true);

// Make a blank psbt
PartiallySignedTransaction psbtx(rawTx);
Expand Down
13 changes: 5 additions & 8 deletions test/functional/rpc_fundrawtransaction.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,9 +752,9 @@ def test_option_feerate(self):
node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": n, "add_inputs": True})

self.log.info("Test invalid fee rate settings")
assert_raises_rpc_error(-4, "Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)",
assert_raises_rpc_error(-8, "Invalid fee_rate 0.000 sat/vB (must be greater than 0)",
node.fundrawtransaction, rawtx, {"fee_rate": 0, "add_inputs": True})
assert_raises_rpc_error(-8, "Invalid feeRate 0.000 sat/vB (must be greater than 0)",
assert_raises_rpc_error(-8, "Invalid feeRate 0.00000000 BTC/kvB (must be greater than 0)",
node.fundrawtransaction, rawtx, {"feeRate": 0, "add_inputs": True})
for param, value in {("fee_rate", 100000), ("feeRate", 1.000)}:
assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)",
Expand All @@ -766,12 +766,9 @@ def test_option_feerate(self):
assert_raises_rpc_error(-3, "Invalid amount",
node.fundrawtransaction, rawtx, {"fee_rate": "", "add_inputs": True})

# Test setting explicit fee rate just below the minimum.
self.log.info("- raises RPC error 'fee rate too low' if fee_rate of 0.99999999 sat/vB is passed")
msg = "Fee rate (0.999 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)"
assert_raises_rpc_error(-4, msg, node.fundrawtransaction, rawtx, {"fee_rate": 0.99999999, "add_inputs": True})
# This feeRate test only passes if `coinControl.fOverrideFeeRate = true` in wallet/rpcwallet.cpp::FundTransaction is removed.
# assert_raises_rpc_error(-4, msg, node.fundrawtransaction, rawtx, {"feeRate": 0.00000999, "add_inputs": True})
self.log.info("Test min fee rate checks are bypassed with fundrawtxn, e.g. a fee_rate under 1 sat/vB is allowed")
node.fundrawtransaction(rawtx, {"fee_rate": 0.99999999, "add_inputs": True})
node.fundrawtransaction(rawtx, {"feeRate": 0.00000999, "add_inputs": True})

self.log.info("- raises RPC error if both feeRate and fee_rate are passed")
assert_raises_rpc_error(-8, "Cannot specify both fee_rate (sat/vB) and feeRate (BTC/kvB)",
Expand Down
14 changes: 7 additions & 7 deletions test/functional/rpc_psbt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,16 @@ def run_test(self):
assert_approx(res1["fee"], 0.055, 0.005)
res2 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True})
assert_approx(res2["fee"], 0.055, 0.005)
self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed, e.g. a fee_rate under 1 sat/vB is allowed")
res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 0.99999999, "add_inputs": True})
assert_approx(res3["fee"], 0.00000381, 0.0000001)
res4 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.00000999, "add_inputs": True})
assert_approx(res4["fee"], 0.00000381, 0.0000001)

self.log.info("Test invalid fee rate settings")
assert_raises_rpc_error(-4, "Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)",
assert_raises_rpc_error(-8, "Invalid fee_rate 0.000 sat/vB (must be greater than 0)",
self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": 0, "add_inputs": True})
assert_raises_rpc_error(-8, "Invalid feeRate 0.000 sat/vB (must be greater than 0)",
assert_raises_rpc_error(-8, "Invalid feeRate 0.00000000 BTC/kvB (must be greater than 0)",
self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"feeRate": 0, "add_inputs": True})
for param, value in {("fee_rate", 100000), ("feeRate", 1)}:
assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)",
Expand Down Expand Up @@ -244,11 +249,6 @@ def run_test(self):
assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"estimate_mode": mode, "conf_target": n, "add_inputs": True})

# Test setting explicit fee rate just below the minimum.
self.log.info("- raises RPC error 'fee rate too low' if feerate_sat_vb of 0.99999999 is passed")
assert_raises_rpc_error(-4, "Fee rate (0.999 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)",
self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": 0.99999999, "add_inputs": True})

self.log.info("Test walletcreatefundedpsbt with too-high fee rate produces total fee well above -maxtxfee and raises RPC error")
# previously this was silently capped at -maxtxfee
for bool_add, outputs_array in {True: outputs, False: [{self.nodes[1].getnewaddress(): 1}]}.items():
Expand Down

0 comments on commit afe9471

Please sign in to comment.