Merge #17098: refactor: Feebumper EstimateFeeRate follow-up

66b29848c71c4b3b4dc36ca6d94de829bd533797 change wallet pointers to references in feebumper (Adam Jonas)
9be6666a4e648782b49a6fdc458372ea521b444d typo and unneccessary parentheses (Adam Jonas)

Pull request description:

  Picking up some of the suggestions in the comments of #16727 including:
  https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330547321
  https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330549766
  https://github.com/bitcoin/bitcoin/pull/16727#discussion_r333209674

ACKs for top commit:
  promag:
    Code review ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797.
  MarcoFalke:
    ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797 (looked at the diff on GitHub)
  fjahr:
    ACK 66b2984 reviewed code

Tree-SHA512: d118f7689970fe39d9f5318dc818f13283cce9194370b3ce4758f298172e4681ae119ddc809f5c0b7602677137ac0d38147b915422ff616531a76a570b766fa2

4 files changed, 50 insertions, 50 deletions

diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
index 0c8d92eba5..aff08bfbea 100644
--- a/src/interfaces/wallet.cpp
+++ b/src/interfaces/wallet.cpp
@@ -241,7 +241,7 @@ public:
     }
     bool transactionCanBeBumped(const uint256& txid) override
     {
-        return feebumper::TransactionCanBeBumped(m_wallet.get(), txid);
+        return feebumper::TransactionCanBeBumped(*m_wallet.get(), txid);
     }
     bool createBumpTransaction(const uint256& txid,
         const CCoinControl& coin_control,
@@ -255,17 +255,17 @@ public:
             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) ==
+            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 signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(*m_wallet.get(), mtx); }
     bool commitBumpTransaction(const uint256& txid,
         CMutableTransaction&& mtx,
         std::vector<std::string>& errors,
         uint256& bumped_txid) override
     {
-        return feebumper::CommitTransaction(m_wallet.get(), txid, std::move(mtx), errors, bumped_txid) ==
+        return feebumper::CommitTransaction(*m_wallet.get(), txid, std::move(mtx), errors, bumped_txid) ==
                feebumper::Result::OK;
     }
     CTransactionRef getTx(const uint256& txid) override
diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
index b87231293f..fbcf8d4de7 100644
--- a/src/wallet/feebumper.cpp
+++ b/src/wallet/feebumper.cpp
@@ -17,15 +17,15 @@
 
 //! Check whether transaction has descendant in wallet or mempool, or has been
 //! mined, or conflicts with a mined transaction. Return a feebumper::Result.
-static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chain, const CWallet* wallet, const CWalletTx& wtx, std::vector<std::string>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet)
+static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chain, const CWallet& wallet, const CWalletTx& wtx, std::vector<std::string>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
-    if (wallet->HasWalletSpend(wtx.GetHash())) {
+    if (wallet.HasWalletSpend(wtx.GetHash())) {
         errors.push_back("Transaction has descendants in the wallet");
         return feebumper::Result::INVALID_PARAMETER;
     }
 
     {
-        if (wallet->chain().hasDescendantsInMempool(wtx.GetHash())) {
+        if (wallet.chain().hasDescendantsInMempool(wtx.GetHash())) {
             errors.push_back("Transaction has descendants in the mempool");
             return feebumper::Result::INVALID_PARAMETER;
         }
@@ -48,7 +48,7 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
 
     // check that original tx consists entirely of our inputs
     // if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee)
-    if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
+    if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
         errors.push_back("Transaction contains inputs that don't belong to this wallet");
         return feebumper::Result::WALLET_ERROR;
     }
@@ -58,13 +58,13 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
 }
 
 //! 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<std::string>& errors) {
+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,
     // 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();
+    CFeeRate minMempoolFeeRate = wallet.chain().mempoolMinFee();
 
     if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
         errors.push_back(strprintf(
@@ -76,7 +76,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt
 
     CAmount new_total_fee = newFeerate.GetFee(maxTxSize);
 
-    CFeeRate incrementalRelayFee = std::max(wallet->chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE));
+    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();
@@ -91,7 +91,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt
         return feebumper::Result::INVALID_PARAMETER;
     }
 
-    CAmount requiredFee = GetRequiredFee(*wallet, maxTxSize);
+    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)));
@@ -99,7 +99,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt
     }
 
     // Check that in all cases the new fee doesn't violate maxTxFee
-    const CAmount max_tx_fee = wallet->m_default_max_tx_fee;
+    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)));
@@ -109,7 +109,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt
     return feebumper::Result::OK;
 }
 
-static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee)
+static CFeeRate EstimateFeeRate(const 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
@@ -123,15 +123,15 @@ static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinCont
     // 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
+    // aware of. This ensures we're 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 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));
+    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);
@@ -139,11 +139,11 @@ static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinCont
 
 namespace feebumper {
 
-bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
+bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
 {
-    auto locked_chain = wallet->chain().lock();
-    LOCK(wallet->cs_wallet);
-    const CWalletTx* wtx = wallet->GetWalletTx(txid);
+    auto locked_chain = wallet.chain().lock();
+    LOCK(wallet.cs_wallet);
+    const CWalletTx* wtx = wallet.GetWalletTx(txid);
     if (wtx == nullptr) return false;
 
     std::vector<std::string> errors_dummy;
@@ -166,7 +166,7 @@ Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, co
     }
     const CWalletTx& wtx = it->second;
 
-    Result result = PreconditionChecks(*locked_chain, wallet, wtx, errors);
+    Result result = PreconditionChecks(*locked_chain, *wallet, wtx, errors);
     if (result != Result::OK) {
         return result;
     }
@@ -276,17 +276,17 @@ Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, co
 }
 
 
-Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors,
+Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors,
                                  CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
 {
     // We are going to modify coin control later, copy to re-use
     CCoinControl new_coin_control(coin_control);
 
-    auto locked_chain = wallet->chain().lock();
-    LOCK(wallet->cs_wallet);
+    auto locked_chain = wallet.chain().lock();
+    LOCK(wallet.cs_wallet);
     errors.clear();
-    auto it = wallet->mapWallet.find(txid);
-    if (it == wallet->mapWallet.end()) {
+    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;
     }
@@ -300,7 +300,7 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
     // Fill in recipients(and preserve a single change key if there is one)
     std::vector<CRecipient> recipients;
     for (const auto& output : wtx.tx->vout) {
-        if (!wallet->IsChange(output)) {
+        if (!wallet.IsChange(output)) {
             CRecipient recipient = {output.scriptPubKey, output.nValue, false};
             recipients.push_back(recipient);
         } else {
@@ -313,8 +313,8 @@ 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);
+        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;
         }
@@ -342,7 +342,7 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
     CAmount fee_ret;
     int change_pos_in_out = -1; // No requested location for change
     std::string fail_reason;
-    if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
+    if (!wallet.CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
         errors.push_back("Unable to create transaction: " + fail_reason);
         return Result::WALLET_ERROR;
     }
@@ -353,7 +353,7 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
     // Write back transaction
     mtx = CMutableTransaction(*tx_new);
     // Mark new tx not replaceable, if requested.
-    if (!coin_control.m_signal_bip125_rbf.get_value_or(wallet->m_signal_rbf)) {
+    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;
         }
@@ -362,21 +362,21 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
     return Result::OK;
 }
 
-bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {
-    auto locked_chain = wallet->chain().lock();
-    LOCK(wallet->cs_wallet);
-    return wallet->SignTransaction(mtx);
+bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) {
+    auto locked_chain = wallet.chain().lock();
+    LOCK(wallet.cs_wallet);
+    return wallet.SignTransaction(mtx);
 }
 
-Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
+Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
 {
-    auto locked_chain = wallet->chain().lock();
-    LOCK(wallet->cs_wallet);
+    auto locked_chain = wallet.chain().lock();
+    LOCK(wallet.cs_wallet);
     if (!errors.empty()) {
         return Result::MISC_ERROR;
     }
-    auto it = txid.IsNull() ? wallet->mapWallet.end() : wallet->mapWallet.find(txid);
-    if (it == wallet->mapWallet.end()) {
+    auto it = txid.IsNull() ? wallet.mapWallet.end() : wallet.mapWallet.find(txid);
+    if (it == wallet.mapWallet.end()) {
         errors.push_back("Invalid or non-wallet transaction id");
         return Result::MISC_ERROR;
     }
@@ -394,7 +394,7 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
     mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
 
     CValidationState state;
-    if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state)) {
+    if (!wallet.CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state)) {
         // NOTE: CommitTransaction never returns false, so this should never happen.
         errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state)));
         return Result::WALLET_ERROR;
@@ -408,7 +408,7 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
     }
 
     // mark the original tx as bumped
-    if (!wallet->MarkReplaced(oldWtx.GetHash(), bumped_txid)) {
+    if (!wallet.MarkReplaced(oldWtx.GetHash(), bumped_txid)) {
         // TODO: see if JSON-RPC has a standard way of returning a response
         // along with an exception. It would be good to return information about
         // wtxBumped to the caller even if marking the original transaction
diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h
index 0c4e1cb7dd..9357397606 100644
--- a/src/wallet/feebumper.h
+++ b/src/wallet/feebumper.h
@@ -26,7 +26,7 @@ enum class Result
 };
 
 //! Return whether transaction can be bumped.
-bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid);
+bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
 
 //! Create bumpfee transaction based on total amount.
 Result CreateTotalBumpTransaction(const CWallet* wallet,
@@ -39,7 +39,7 @@ Result CreateTotalBumpTransaction(const CWallet* wallet,
                          CMutableTransaction& mtx);
 
 //! Create bumpfee transaction based on feerate estimates.
-Result CreateRateBumpTransaction(CWallet* wallet,
+Result CreateRateBumpTransaction(CWallet& wallet,
                          const uint256& txid,
                          const CCoinControl& coin_control,
                          std::vector<std::string>& errors,
@@ -50,13 +50,13 @@ Result CreateRateBumpTransaction(CWallet* wallet,
 //! Sign the new transaction,
 //! @return false if the tx couldn't be found or if it was
 //! impossible to create the signature(s)
-bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx);
+bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx);
 
 //! Commit the bumpfee transaction.
 //! @return success in case of CWallet::CommitTransaction was successful,
 //! but sets errors if the tx could not be added to the mempool (will try later)
 //! or if the old transaction could not be marked as replaced.
-Result CommitTransaction(CWallet* wallet,
+Result CommitTransaction(CWallet& wallet,
                          const uint256& txid,
                          CMutableTransaction&& mtx,
                          std::vector<std::string>& errors,
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index dc437e5cd8..debc3fecda 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3417,7 +3417,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
         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);
+        res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx);
     }
     if (res != feebumper::Result::OK) {
         switch(res) {
@@ -3440,12 +3440,12 @@ static UniValue bumpfee(const JSONRPCRequest& request)
     }
 
     // sign bumped transaction
-    if (!feebumper::SignTransaction(pwallet, mtx)) {
+    if (!feebumper::SignTransaction(*pwallet, mtx)) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
     }
     // commit the bumped transaction
     uint256 txid;
-    if (feebumper::CommitTransaction(pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
+    if (feebumper::CommitTransaction(*pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
         throw JSONRPCError(RPC_WALLET_ERROR, errors[0]);
     }
     UniValue result(UniValue::VOBJ);