From be177c15a40199fac79d8ab96bb4b4d5a9b4fe22 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 22 Mar 2023 17:49:09 -0400 Subject: [PATCH 1/2] bumpfee: Check the correct feerate when replacing outputs When doing the feerate check for bumped transactions that replace the outputs, we need to consider that the size of the new outputs may be different from the old outputs and calculate the minimum feerate accordingly. --- src/wallet/feebumper.cpp | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) 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; } From d52fa1b0a5a8eecbe1e296a44b72965717e9235b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 22 Mar 2023 18:47:54 -0400 Subject: [PATCH 2/2] tests: Make sure that bumpfee feerate checks work when replacing outputs When replacing the outputs of a transaction, we can end up with fees that are drastically different from the original. This tests that the feerate checks we perform will properly detect when the bumping tx will have an insufficient feerate. --- test/functional/wallet_bumpfee.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) 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()