From 1a4c791cf49ff15aa9deba4388c0180b8f47f15b Mon Sep 17 00:00:00 2001 From: ezegom Date: Mon, 29 Jul 2019 17:43:43 -0400 Subject: rpc bumpfee: move feerate estimation logic into separate method --- src/wallet/feebumper.cpp | 54 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 619197a57a..1e25d4ee28 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -57,6 +57,34 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai 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 +258,7 @@ 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); + 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 -- cgit v1.2.3 From 88e5f997dfab3f03bb1ec3f149eaff8dcc2981fe Mon Sep 17 00:00:00 2001 From: ezegom Date: Mon, 29 Jul 2019 18:02:02 -0400 Subject: rpc bumpfee: add fee_rate argument --- src/wallet/feebumper.cpp | 7 ++++++- src/wallet/rpcwallet.cpp | 19 +++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 1e25d4ee28..ce76f4e512 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -258,7 +258,12 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo } } - new_coin_control.m_feerate = EstimateFeeRate(wallet, wtx, new_coin_control, old_fee); + if (coin_control.m_feerate) { + // The user provided a feeRate argument. + } 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 diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 30e767e489..098778d878 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3290,7 +3290,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", { @@ -3302,6 +3302,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" @@ -3343,13 +3346,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")) { @@ -3360,6 +3365,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")) { -- cgit v1.2.3 From 9f25de3d9eb8d012ca1a98cbcd28021e3e1c85ee Mon Sep 17 00:00:00 2001 From: ezegom Date: Mon, 29 Jul 2019 19:01:42 -0400 Subject: rpc bumpfee check fee_rate argument --- src/wallet/feebumper.cpp | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) (limited to 'src') diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index ce76f4e512..b87231293f 100644 --- a/src/wallet/feebumper.cpp +++ b/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& 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 @@ -260,6 +312,12 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo 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); -- cgit v1.2.3