diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 37a704bfa44..d127c41c43d 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -63,7 +63,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet } //! 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, CAmount old_fee, std::vector& errors) +static feebumper::Result CheckFeeRate(const CWallet& wallet, const CFeeRate& newFeerate, const int64_t maxTxSize, CAmount old_fee, 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) @@ -84,15 +84,12 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wt CFeeRate incrementalRelayFee = std::max(wallet.chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE)); - // Given old total fee and transaction size, calculate the old feeRate - 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); + CAmount minTotalFee = old_fee + incrementalRelayFee.GetFee(maxTxSize); if (new_total_fee < minTotalFee) { errors.push_back(strprintf(Untranslated("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)))); + FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(old_fee), FormatMoney(incrementalRelayFee.GetFee(maxTxSize)))); return feebumper::Result::INVALID_PARAMETER; } @@ -234,7 +231,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // is one). If outputs vector is non-empty, replace original // outputs with its contents, otherwise use original outputs. std::vector recipients; - for (const auto& output : outputs.empty() ? wtx.tx->vout : outputs) { + const auto& txouts = outputs.empty() ? wtx.tx->vout : outputs; + for (const auto& output : txouts) { if (!OutputIsChange(wallet, output)) { CRecipient recipient = {output.scriptPubKey, output.nValue, false}; recipients.push_back(recipient); @@ -249,13 +247,14 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // The user provided a feeRate argument. // We calculate this here to avoid compiler warning on the cs_wallet lock // We need to make a temporary transaction with no input witnesses as the dummy signer expects them to be empty for external inputs - CMutableTransaction mtx{*wtx.tx}; - for (auto& txin : mtx.vin) { + CMutableTransaction temp_mtx{*wtx.tx}; + for (auto& txin : temp_mtx.vin) { txin.scriptSig.clear(); txin.scriptWitness.SetNull(); } - const int64_t maxTxSize{CalculateMaximumSignedTxSize(CTransaction(mtx), &wallet, &new_coin_control).vsize}; - Result res = CheckFeeRate(wallet, wtx, *new_coin_control.m_feerate, maxTxSize, old_fee, errors); + temp_mtx.vout = txouts; + const int64_t maxTxSize{CalculateMaximumSignedTxSize(CTransaction(temp_mtx), &wallet, &new_coin_control).vsize}; + Result res = CheckFeeRate(wallet, *new_coin_control.m_feerate, maxTxSize, old_fee, errors); if (res != Result::OK) { return res; } diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index ad79e0288ca..4f7af328c1b 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -26,6 +26,7 @@ from test_framework.util import ( assert_equal, assert_greater_than, assert_raises_rpc_error, + get_fee, ) from test_framework.wallet import MiniWallet @@ -100,6 +101,7 @@ class BumpFeeTest(BitcoinTestFramework): test_change_script_match(self, rbf_node, dest_address) test_settxfee(self, rbf_node, dest_address) test_maxtxfee_fails(self, rbf_node, dest_address) + test_feerate_checks_replaced_outputs(self, rbf_node) # These tests wipe out a number of utxos that are expected in other tests test_small_output_with_feerate_succeeds(self, rbf_node, dest_address) test_no_more_inputs_fails(self, rbf_node, dest_address) @@ -668,5 +670,30 @@ def test_no_more_inputs_fails(self, rbf_node, dest_address): self.clear_mempool() +def test_feerate_checks_replaced_outputs(self, rbf_node): + self.log.info("Test that feerate checks use replaced outputs") + outputs = [] + for i in range(50): + outputs.append({rbf_node.getnewaddress(address_type="bech32"): 1}) + tx_res = rbf_node.send(outputs=outputs, fee_rate=5) + tx_details = rbf_node.gettransaction(txid=tx_res["txid"], verbose=True) + + # Calculate the minimum feerate required for the bump to work. + # Since the bumped tx will replace all of the outputs with a single output, we can estimate that its size will 31 * (len(outputs) - 1) bytes smaller + tx_size = tx_details["decoded"]["vsize"] + est_bumped_size = tx_size - (len(tx_details["decoded"]["vout"]) - 1) * 31 + inc_fee_rate = max(rbf_node.getmempoolinfo()["incrementalrelayfee"], Decimal(0.00005000)) # Wallet has a fixed incremental relay fee of 5 sat/vb + # RPC gives us fee as negative + min_fee = (-tx_details["fee"] + get_fee(est_bumped_size, inc_fee_rate)) * Decimal(1e8) + min_fee_rate = (min_fee / est_bumped_size).quantize(Decimal("1.000")) + + # Attempt to bumpfee and replace all outputs with a single one using a feerate slightly less than the minimum + new_outputs = [{rbf_node.getnewaddress(address_type="bech32"): 49}] + assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, tx_res["txid"], {"fee_rate": min_fee_rate - 1, "outputs": new_outputs}) + + # Bumpfee and replace all outputs with a single one using the minimum feerate + rbf_node.bumpfee(tx_res["txid"], {"fee_rate": min_fee_rate, "outputs": new_outputs}) + + if __name__ == "__main__": BumpFeeTest().main()