Merge #18312: wallet: remove deprecated fee bumping by totalFee

c3857c5fcb21836ddc1b79a6b19cffe562cade10 wallet: remove CreateTotalBumpTransaction() (Jon Atack)
4a0b27bb01738e6917e27b2cf47f9a8536249693 wallet: remove totalfee from createBumpTransaction() (Jon Atack)
e347cfa9a7244277f9d220a4dc3537182f18441e rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack)
bd05f96d79df1a1561f84850d777808f8575fb8b test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack)
a6d1ab8caa63bd343207baa60edb705209f16fb4 test: update bumpfee testing from totalFee to fee_rate (Jon Atack)

Pull request description:

  Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it.

ACKs for top commit:
  laanwj:
    ACK c3857c5fcb21836ddc1b79a6b19cffe562cade10

Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00

6 files changed, 10 insertions, 176 deletions

diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
index c3a62cf73d..bb6bb4923f 100644
--- a/src/interfaces/wallet.cpp
+++ b/src/interfaces/wallet.cpp
@@ -253,19 +253,12 @@ public:
     }
     bool createBumpTransaction(const uint256& txid,
         const CCoinControl& coin_control,
-        CAmount total_fee,
         std::vector<std::string>& errors,
         CAmount& old_fee,
         CAmount& new_fee,
         CMutableTransaction& mtx) override
     {
-        if (total_fee > 0) {
-            return feebumper::CreateTotalBumpTransaction(m_wallet.get(), txid, coin_control, total_fee, errors, old_fee, new_fee, mtx) ==
-                feebumper::Result::OK;
-        } else {
-            return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx) ==
-                feebumper::Result::OK;
-        }
+        return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx) == feebumper::Result::OK;
     }
     bool signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(*m_wallet.get(), mtx); }
     bool commitBumpTransaction(const uint256& txid,
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 487a7c3a5a..0e551b0a96 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -155,7 +155,6 @@ public:
     //! Create bump transaction.
     virtual bool createBumpTransaction(const uint256& txid,
         const CCoinControl& coin_control,
-        CAmount total_fee,
         std::vector<std::string>& errors,
         CAmount& old_fee,
         CAmount& new_fee,
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 94e7a05e40..41876f7119 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -482,7 +482,7 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
     CAmount old_fee;
     CAmount new_fee;
     CMutableTransaction mtx;
-    if (!m_wallet->createBumpTransaction(hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx)) {
+    if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx)) {
         QMessageBox::critical(nullptr, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
             (errors.size() ? QString::fromStdString(errors[0]) : "") +")");
          return false;
diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
index 486a6b8bf1..1623ab9074 100644
--- a/src/wallet/feebumper.cpp
+++ b/src/wallet/feebumper.cpp
@@ -60,7 +60,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
 static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, std::vector<std::string>& 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,
+    // This may occur if the user set fee_rate 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();
@@ -150,132 +150,6 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
     return res == feebumper::Result::OK;
 }
 
-Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
-                                  CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
-{
-    new_fee = total_fee;
-
-    auto locked_chain = wallet->chain().lock();
-    LOCK(wallet->cs_wallet);
-    errors.clear();
-    auto it = wallet->mapWallet.find(txid);
-    if (it == wallet->mapWallet.end()) {
-        errors.push_back("Invalid or non-wallet transaction id");
-        return Result::INVALID_ADDRESS_OR_KEY;
-    }
-    const CWalletTx& wtx = it->second;
-
-    Result result = PreconditionChecks(*wallet, wtx, errors);
-    if (result != Result::OK) {
-        return result;
-    }
-
-    // figure out which output was change
-    // if there was no change output or multiple change outputs, fail
-    int nOutput = -1;
-    for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
-        if (wallet->IsChange(wtx.tx->vout[i])) {
-            if (nOutput != -1) {
-                errors.push_back("Transaction has multiple change outputs");
-                return Result::WALLET_ERROR;
-            }
-            nOutput = i;
-        }
-    }
-    if (nOutput == -1) {
-        errors.push_back("Transaction does not have a change output");
-        return Result::WALLET_ERROR;
-    }
-
-    // Calculate the expected size of the new transaction.
-    int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
-    const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
-    if (maxNewTxSize < 0) {
-        errors.push_back("Transaction contains inputs that cannot be signed");
-        return Result::INVALID_ADDRESS_OR_KEY;
-    }
-
-    // calculate the old fee and fee-rate
-    isminefilter filter = wallet->GetLegacyScriptPubKeyMan() && wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
-    old_fee = wtx.GetDebit(filter) - wtx.tx->GetValueOut();
-    CFeeRate nOldFeeRate(old_fee, txSize);
-    // The wallet uses a 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.
-    CFeeRate nodeIncrementalRelayFee = wallet->chain().relayIncrementalFee();
-    CFeeRate walletIncrementalRelayFee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE);
-    if (nodeIncrementalRelayFee > walletIncrementalRelayFee) {
-        walletIncrementalRelayFee = nodeIncrementalRelayFee;
-    }
-
-    CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + nodeIncrementalRelayFee.GetFee(maxNewTxSize);
-    if (total_fee < minTotalFee) {
-        errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
-            FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(nodeIncrementalRelayFee.GetFee(maxNewTxSize))));
-        return Result::INVALID_PARAMETER;
-    }
-    CAmount requiredFee = GetRequiredFee(*wallet, maxNewTxSize);
-    if (total_fee < requiredFee) {
-        errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
-            FormatMoney(requiredFee)));
-        return 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_fee > max_tx_fee) {
-         errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",
-                               FormatMoney(new_fee), FormatMoney(max_tx_fee)));
-         return Result::WALLET_ERROR;
-     }
-
-    // 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 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 use total_fee to make an adjustment.
-    CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee();
-    CFeeRate nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
-    if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
-        errors.push_back(strprintf(
-            "New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "
-            "the totalFee value should be at least %s to add transaction",
-            FormatMoney(nNewFeeRate.GetFeePerK()),
-            FormatMoney(minMempoolFeeRate.GetFeePerK()),
-            FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize))));
-        return Result::WALLET_ERROR;
-    }
-
-    // Now modify the output to increase the fee.
-    // If the output is not large enough to pay the fee, fail.
-    CAmount nDelta = new_fee - old_fee;
-    assert(nDelta > 0);
-    mtx = CMutableTransaction{*wtx.tx};
-    CTxOut* poutput = &(mtx.vout[nOutput]);
-    if (poutput->nValue < nDelta) {
-        errors.push_back("Change output is too small to bump the fee");
-        return Result::WALLET_ERROR;
-    }
-
-    // If the output would become dust, discard it (converting the dust to fee)
-    poutput->nValue -= nDelta;
-    if (poutput->nValue <= GetDustThreshold(*poutput, GetDiscardRate(*wallet))) {
-        wallet->WalletLogPrintf("Bumping fee and discarding dust output\n");
-        new_fee += poutput->nValue;
-        mtx.vout.erase(mtx.vout.begin() + nOutput);
-    }
-
-    // Mark new tx not replaceable, if requested.
-    if (!coin_control.m_signal_bip125_rbf.get_value_or(wallet->m_signal_rbf)) {
-        for (auto& input : mtx.vin) {
-            if (input.nSequence < 0xfffffffe) input.nSequence = 0xfffffffe;
-        }
-    }
-
-    return Result::OK;
-}
-
-
 Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors,
                                  CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
 {
diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h
index 9357397606..859f754761 100644
--- a/src/wallet/feebumper.h
+++ b/src/wallet/feebumper.h
@@ -28,16 +28,6 @@ enum class Result
 //! Return whether transaction can be bumped.
 bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
 
-//! Create bumpfee transaction based on total amount.
-Result CreateTotalBumpTransaction(const CWallet* wallet,
-                         const uint256& txid,
-                         const CCoinControl& coin_control,
-                         CAmount total_fee,
-                         std::vector<std::string>& errors,
-                         CAmount& old_fee,
-                         CAmount& new_fee,
-                         CMutableTransaction& mtx);
-
 //! Create bumpfee transaction based on feerate estimates.
 Result CreateRateBumpTransaction(CWallet& wallet,
                          const uint256& txid,
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index a913aa3024..0eb7ed2b71 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3342,12 +3342,11 @@ static UniValue bumpfee(const JSONRPCRequest& request)
                 "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
                 "An opt-in RBF transaction with the given txid must be in the wallet.\n"
                 "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"
-                "If `totalFee` (DEPRECATED) is given, adding inputs is not supported, so there must be a single change output that is big enough or it will fail.\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"
                 "The user can specify a confirmation target for estimatesmartfee.\n"
-                "Alternatively, the user can specify totalFee (DEPRECATED), or fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction .\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"
                 "returned by getnetworkinfo) to enter the node's mempool.\n",
                 {
@@ -3355,11 +3354,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
                     {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
                         {
                             {"confTarget", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"},
-                            {"totalFee", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis. (DEPRECATED)\n"
-            "                         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"
+                            {"fee_rate", RPCArg::Type::NUM, /* default */ "fall back to 'confTarget'", "fee rate (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 " + CURRENCY_UNIT + " per kB higher than the current transaction fee rate.\n"},
                             {"replaceable", RPCArg::Type::BOOL, /* default */ "true", "Whether the new transaction should still be\n"
@@ -3400,7 +3395,6 @@ static UniValue bumpfee(const JSONRPCRequest& request)
     CCoinControl coin_control;
     coin_control.fAllowWatchOnly = pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
     // optional parameters
-    CAmount totalFee = 0;
     coin_control.m_signal_bip125_rbf = true;
 
     if (!request.params[1].isNull()) {
@@ -3408,26 +3402,15 @@ static UniValue bumpfee(const JSONRPCRequest& request)
         RPCTypeCheckObj(options,
             {
                 {"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") || 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.");
+        if (options.exists("confTarget") && options.exists("fee_rate")) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
         } 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")) {
-            if (!pwallet->chain().rpcEnableDeprecated("totalFee")) {
-                throw JSONRPCError(RPC_INVALID_PARAMETER, "totalFee argument has been deprecated and will be removed in 0.20. Please use -deprecatedrpc=totalFee to continue using this argument until removal.");
-            }
-            totalFee = options["totalFee"].get_int64();
-            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 <= CFeeRate(0)) {
@@ -3460,13 +3443,8 @@ static UniValue bumpfee(const JSONRPCRequest& request)
     CAmount new_fee;
     CMutableTransaction mtx;
     feebumper::Result res;
-    if (totalFee > 0) {
-        // Targeting total fee bump. Requires a change output of sufficient size.
-        res = feebumper::CreateTotalBumpTransaction(pwallet, hash, coin_control, totalFee, errors, old_fee, new_fee, mtx);
-    } else {
-        // Targeting feerate bump.
-        res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx);
-    }
+    // Targeting feerate bump.
+    res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx);
     if (res != feebumper::Result::OK) {
         switch(res) {
             case feebumper::Result::INVALID_ADDRESS_OR_KEY:
@@ -4196,7 +4174,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
                             },
                             {"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 */ "Fallback to wallet's confirmation target", "Confirmation target (in blocks)"},
+                            {"conf_target", RPCArg::Type::NUM, /* default */ "fall back to wallet's confirmation target (txconfirmtarget)", "Confirmation target (in blocks)"},
                             {"estimate_mode", RPCArg::Type::STR, /* default */ "UNSET", "The fee estimate mode, must be one of:\n"
                             "         \"UNSET\"\n"
                             "         \"ECONOMICAL\"\n"