Merge bitcoin#23202: wallet: allow psbtbumpfee to work with txs with external inputs

c3b099a wallet, tests: Test bumpfee's max input weight calculation (Andrew Chow)
116a620 Make DUMMY_CHECKER availble outside of script/sign.cpp (Andrew Chow)
ff63832 test, bumpfee: Check that psbtbumpfee can bump txs with external inputs (Andrew Chow)
1bc8106 bumpfee: be able to bump fee of a tx with external inputs (Andrew Chow)
31dd3dc bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size (Andrew Chow)
a0c3afb bumpfee: extract weights of external inputs when bumping fee (Andrew Chow)
612f1e4 bumpfee: Calculate fee by looking up UTXOs (Andrew Chow)

Pull request description:

  This PR allows `psbtbumpfee` to return a PSBT for transactions that contain external inputs. This does not work for bumping in the GUI nor `bumpfee` because these need private keys available to sign and send the transaction. But `psbtbumpfee` returns a psbt, so it is fine to not be able to sign.

  In order to correctly estimate the size of the inputs for coin selection, the fee bumper will use the size of the inputs of the transaction being bumped. Because the sizes of signatures are not guaranteed, for external inputs, the fee bumper will verify the scripts with a special SignatureChecker which will compute the weight of all of the signatures in that input, and compute their weights if those signatures were maximally sized. This allows the fee bumper to obtain a max size estimate for each external input.

  Builds on bitcoin#23201 as it relies on the ability to pass weights in to coin selection.

  Closes bitcoin#23189

ACKs for top commit:
  ishaanam:
    reACK c3b099a
  t-bast:
    Re-ran my tests agains bitcoin@c3b099a, ACK

Tree-SHA512: 40016ec52d351430977579cfa2694c7e6764f42c9ce09d3a6f1753b767f86053f296d9de988248df033be6d725d67badbf2a5ef82c8ace23c61487729b7691e5

Author: fanquake

src/Makefile.test.include

@@ -162,6 +162,7 @@ BITCOIN_TESTS =\
 
 if ENABLE_WALLET
 BITCOIN_TESTS += \
+  wallet/test/feebumper_tests.cpp \
   wallet/test/psbt_wallet_tests.cpp \
   wallet/test/spend_tests.cpp \
   wallet/test/wallet_tests.cpp \

src/script/interpreter.h

@@ -307,10 +307,10 @@ using MutableTransactionSignatureChecker = GenericTransactionSignatureChecker<CM
 class DeferringSignatureChecker : public BaseSignatureChecker
 {
 protected:
-    BaseSignatureChecker& m_checker;
+    const BaseSignatureChecker& m_checker;
 
 public:
-    DeferringSignatureChecker(BaseSignatureChecker& checker) : m_checker(checker) {}
+    DeferringSignatureChecker(const BaseSignatureChecker& checker) : m_checker(checker) {}
 
     bool CheckECDSASignature(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override
     {

src/script/sign.cpp

@@ -596,8 +596,11 @@ class DummySignatureChecker final : public BaseSignatureChecker
     bool CheckECDSASignature(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override { return true; }
     bool CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey, SigVersion sigversion, ScriptExecutionData& execdata, ScriptError* serror) const override { return true; }
 };
-const DummySignatureChecker DUMMY_CHECKER;
+}
+
+const BaseSignatureChecker& DUMMY_CHECKER = DummySignatureChecker();
 
+namespace {
 class DummySignatureCreator final : public BaseSignatureCreator {
 private:
     char m_r_len = 32;

src/script/sign.h

@@ -52,6 +52,8 @@ class MutableTransactionSignatureCreator : public BaseSignatureCreator
     bool CreateSchnorrSig(const SigningProvider& provider, std::vector<unsigned char>& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const override;
 };
 
+/** A signature checker that accepts all signatures */
+extern const BaseSignatureChecker& DUMMY_CHECKER;
 /** A signature creator that just produces 71-byte empty signatures. */
 extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;
 /** A signature creator that just produces 72-byte empty signatures. */

src/wallet/feebumper.cpp

@@ -2,6 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <consensus/validation.h>
 #include <interfaces/chain.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
@@ -19,7 +20,7 @@
 namespace wallet {
 //! 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(const CWallet& wallet, const CWalletTx& wtx, std::vector<bilingual_str>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, bool require_mine, std::vector<bilingual_str>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     if (wallet.HasWalletSpend(wtx.tx)) {
         errors.push_back(Untranslated("Transaction has descendants in the wallet"));
@@ -48,20 +49,21 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
         return feebumper::Result::WALLET_ERROR;
     }
 
-    // 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)
-    isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
-    if (!AllInputsMine(wallet, *wtx.tx, filter)) {
-        errors.push_back(Untranslated("Transaction contains inputs that don't belong to this wallet"));
-        return feebumper::Result::WALLET_ERROR;
+    if (require_mine) {
+        // 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)
+        isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
+        if (!AllInputsMine(wallet, *wtx.tx, filter)) {
+            errors.push_back(Untranslated("Transaction contains inputs that don't belong to this wallet"));
+            return feebumper::Result::WALLET_ERROR;
+        }
     }
 
-
     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<bilingual_str>& errors)
+static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, CAmount old_fee, std::vector<bilingual_str>& 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)
@@ -83,8 +85,6 @@ 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
-    isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
-    CAmount old_fee = CachedTxGetDebit(wallet, wtx, filter) - wtx.tx->GetValueOut();
     const int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
     CFeeRate nOldFeeRate(old_fee, txSize);
     // Min total fee is old fee + relay fee
@@ -150,12 +150,12 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
     if (wtx == nullptr) return false;
 
     std::vector<bilingual_str> errors_dummy;
-    feebumper::Result res = PreconditionChecks(wallet, *wtx, errors_dummy);
+    feebumper::Result res = PreconditionChecks(wallet, *wtx, /* require_mine=*/ true, errors_dummy);
     return res == feebumper::Result::OK;
 }
 
 Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<bilingual_str>& errors,
-                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
+                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine)
 {
     // We are going to modify coin control later, copy to re-use
     CCoinControl new_coin_control(coin_control);
@@ -169,13 +169,63 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     }
     const CWalletTx& wtx = it->second;
 
-    Result result = PreconditionChecks(wallet, wtx, errors);
+    // Retrieve all of the UTXOs and add them to coin control
+    // While we're here, calculate the input amount
+    std::map<COutPoint, Coin> coins;
+    CAmount input_value = 0;
+    std::vector<CTxOut> spent_outputs;
+    for (const CTxIn& txin : wtx.tx->vin) {
+        coins[txin.prevout]; // Create empty map entry keyed by prevout.
+    }
+    wallet.chain().findCoins(coins);
+    for (const CTxIn& txin : wtx.tx->vin) {
+        const Coin& coin = coins.at(txin.prevout);
+        if (coin.out.IsNull()) {
+            errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n)));
+            return Result::MISC_ERROR;
+        }
+        if (wallet.IsMine(txin.prevout)) {
+            new_coin_control.Select(txin.prevout);
+        } else {
+            new_coin_control.SelectExternal(txin.prevout, coin.out);
+        }
+        input_value += coin.out.nValue;
+        spent_outputs.push_back(coin.out);
+    }
+
+    // Figure out if we need to compute the input weight, and do so if necessary
+    PrecomputedTransactionData txdata;
+    txdata.Init(*wtx.tx, std::move(spent_outputs), /* force=*/ true);
+    for (unsigned int i = 0; i < wtx.tx->vin.size(); ++i) {
+        const CTxIn& txin = wtx.tx->vin.at(i);
+        const Coin& coin = coins.at(txin.prevout);
+
+        if (new_coin_control.IsExternalSelected(txin.prevout)) {
+            // For external inputs, we estimate the size using the size of this input
+            int64_t input_weight = GetTransactionInputWeight(txin);
+            // Because signatures can have different sizes, we need to figure out all of the
+            // signature sizes and replace them with the max sized signature.
+            // In order to do this, we verify the script with a special SignatureChecker which
+            // will observe the signatures verified and record their sizes.
+            SignatureWeights weights;
+            TransactionSignatureChecker tx_checker(wtx.tx.get(), i, coin.out.nValue, txdata, MissingDataBehavior::FAIL);
+            SignatureWeightChecker size_checker(weights, tx_checker);
+            VerifyScript(txin.scriptSig, coin.out.scriptPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, size_checker);
+            // Add the difference between max and current to input_weight so that it represents the largest the input could be
+            input_weight += weights.GetWeightDiffToMax();
+            new_coin_control.SetInputWeight(txin.prevout, input_weight);
+        }
+    }
+
+    Result result = PreconditionChecks(wallet, wtx, require_mine, errors);
     if (result != Result::OK) {
         return result;
     }
 
     // Fill in recipients(and preserve a single change key if there is one)
+    // While we're here, calculate the output amount
     std::vector<CRecipient> recipients;
+    CAmount output_value = 0;
     for (const auto& output : wtx.tx->vout) {
         if (!OutputIsChange(wallet, output)) {
             CRecipient recipient = {output.scriptPubKey, output.nValue, false};
@@ -185,16 +235,22 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
             ExtractDestination(output.scriptPubKey, change_dest);
             new_coin_control.destChange = change_dest;
         }
+        output_value += output.nValue;
     }
 
-    isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
-    old_fee = CachedTxGetDebit(wallet, wtx, filter) - wtx.tx->GetValueOut();
+    old_fee = input_value - output_value;
 
     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).vsize};
-        Result res = CheckFeeRate(wallet, wtx, *new_coin_control.m_feerate, maxTxSize, errors);
+        // 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) {
+            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);
         if (res != Result::OK) {
             return res;
         }
@@ -254,7 +310,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti
     const CWalletTx& oldWtx = it->second;
 
     // make sure the transaction still has no descendants and hasn't been mined in the meantime
-    Result result = PreconditionChecks(wallet, oldWtx, errors);
+    Result result = PreconditionChecks(wallet, oldWtx, /* require_mine=*/ false, errors);
     if (result != Result::OK) {
         return result;
     }

src/wallet/feebumper.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_WALLET_FEEBUMPER_H
 #define BITCOIN_WALLET_FEEBUMPER_H
 
+#include <consensus/consensus.h>
+#include <script/interpreter.h>
 #include <primitives/transaction.h>
 
 class uint256;
@@ -31,14 +33,25 @@ enum class Result
 //! Return whether transaction can be bumped.
 bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
 
-//! Create bumpfee transaction based on feerate estimates.
+/** Create bumpfee transaction based on feerate estimates.
+ *
+ * @param[in] wallet The wallet to use for this bumping
+ * @param[in] txid The txid of the transaction to bump
+ * @param[in] coin_control A CCoinControl object which provides feerates and other information used for coin selection
+ * @param[out] errors Errors
+ * @param[out] old_fee The fee the original transaction pays
+ * @param[out] new_fee the fee that the bump transaction pays
+ * @param[out] mtx The bump transaction itself
+ * @param[in] require_mine Whether the original transaction must consist of inputs that can be spent by the wallet
+ */
 Result CreateRateBumpTransaction(CWallet& wallet,
     const uint256& txid,
     const CCoinControl& coin_control,
     std::vector<bilingual_str>& errors,
     CAmount& old_fee,
     CAmount& new_fee,
-    CMutableTransaction& mtx);
+    CMutableTransaction& mtx,
+    bool require_mine);
 
 //! Sign the new transaction,
 //! @return false if the tx couldn't be found or if it was
@@ -55,6 +68,55 @@ Result CommitTransaction(CWallet& wallet,
     std::vector<bilingual_str>& errors,
     uint256& bumped_txid);
 
+struct SignatureWeights
+{
+private:
+    int m_sigs_count{0};
+    int64_t m_sigs_weight{0};
+
+public:
+    void AddSigWeight(const size_t weight, const SigVersion sigversion)
+    {
+        switch (sigversion) {
+        case SigVersion::BASE:
+            m_sigs_weight += weight * WITNESS_SCALE_FACTOR;
+            m_sigs_count += 1 * WITNESS_SCALE_FACTOR;
+            break;
+        case SigVersion::WITNESS_V0:
+            m_sigs_weight += weight;
+            m_sigs_count++;
+            break;
+        case SigVersion::TAPROOT:
+        case SigVersion::TAPSCRIPT:
+            assert(false);
+        }
+    }
+
+    int64_t GetWeightDiffToMax() const
+    {
+        // Note: the witness scaling factor is already accounted for because the count is multiplied by it.
+        return (/* max signature size=*/ 72 * m_sigs_count) - m_sigs_weight;
+    }
+};
+
+class SignatureWeightChecker : public DeferringSignatureChecker
+{
+private:
+    SignatureWeights& m_weights;
+
+public:
+    SignatureWeightChecker(SignatureWeights& weights, const BaseSignatureChecker& checker) : DeferringSignatureChecker(checker), m_weights(weights) {}
+
+    bool CheckECDSASignature(const std::vector<unsigned char>& sig, const std::vector<unsigned char>& pubkey, const CScript& script, SigVersion sigversion) const override
+    {
+        if (m_checker.CheckECDSASignature(sig, pubkey, script, sigversion)) {
+            m_weights.AddSigWeight(sig.size(), sigversion);
+            return true;
+        }
+        return false;
+    }
+};
+
 } // namespace feebumper
 } // namespace wallet
 

src/wallet/interfaces.cpp

@@ -291,7 +291,7 @@ class WalletImpl : public Wallet
         CAmount& new_fee,
         CMutableTransaction& mtx) override
     {
-        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, /* require_mine= */ true) == feebumper::Result::OK;
     }
     bool signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(*m_wallet.get(), mtx); }
     bool commitBumpTransaction(const uint256& txid,

src/wallet/rpc/spend.cpp

@@ -1045,7 +1045,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
     CMutableTransaction mtx;
     feebumper::Result res;
     // 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, /*require_mine=*/ !want_psbt);
     if (res != feebumper::Result::OK) {
         switch(res) {
             case feebumper::Result::INVALID_ADDRESS_OR_KEY:

src/wallet/test/feebumper_tests.cpp

@@ -0,0 +1,54 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <primitives/transaction.h>
+#include <script/script.h>
+#include <util/strencodings.h>
+#include <wallet/feebumper.h>
+#include <wallet/test/util.h>
+#include <wallet/test/wallet_test_fixture.h>
+
+#include <boost/test/unit_test.hpp>
+
+namespace wallet {
+namespace feebumper {
+BOOST_FIXTURE_TEST_SUITE(feebumper_tests, WalletTestingSetup)
+
+static void CheckMaxWeightComputation(const std::string& script_str, const std::vector<std::string>& witness_str_stack, const std::string& prevout_script_str, int64_t expected_max_weight)
+{
+    std::vector script_data(ParseHex(script_str));
+    CScript script(script_data.begin(), script_data.end());
+    CTxIn input(uint256(), 0, script);
+
+    for (const auto& s : witness_str_stack) {
+        input.scriptWitness.stack.push_back(ParseHex(s));
+    }
+
+    std::vector prevout_script_data(ParseHex(prevout_script_str));
+    CScript prevout_script(prevout_script_data.begin(), prevout_script_data.end());
+
+    int64_t weight = GetTransactionInputWeight(input);
+    SignatureWeights weights;
+    SignatureWeightChecker size_checker(weights, DUMMY_CHECKER);
+    bool script_ok = VerifyScript(input.scriptSig, prevout_script, &input.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, size_checker);
+    BOOST_CHECK(script_ok);
+    weight += weights.GetWeightDiffToMax();
+    BOOST_CHECK_EQUAL(weight, expected_max_weight);
+}
+
+BOOST_AUTO_TEST_CASE(external_max_weight_test)
+{
+    // P2PKH
+    CheckMaxWeightComputation("453042021f03c8957c5ce12940ee6e3333ecc3f633d9a1ac53a55b3ce0351c617fa96abe021f0dccdcce3ef45a63998be9ec748b561baf077b8e862941d0cd5ec08f5afe68012102fccfeb395f0ecd3a77e7bc31c3bc61dc987418b18e395d441057b42ca043f22c", {}, "76a914f60dcfd3392b28adc7662669603641f578eed72d88ac", 593);
+    // P2SH-P2WPKH
+    CheckMaxWeightComputation("160014001dca1b22c599b5a56a87c78417ad2ff39552f1", {"3042021f5443c58eaf45f3e5ef46f8516f966b334a7d497cedda4edb2b9fad57c90c3b021f63a77cb56cde848e2e2dd20b487eec2f53101f634193786083f60b4d23a82301", "026cfe86116f161057deb240201d6b82ebd4f161e0200d63dc9aca65a1d6b38bb7"}, "a9147c8ab5ad7708b97ccb6b483d57aba48ee85214df87", 364);
+    // P2WPKH
+    CheckMaxWeightComputation("", {"3042021f0f8906f0394979d5b737134773e5b88bf036c7d63542301d600ab677ba5a59021f0e9fe07e62c113045fa1c1532e2914720e8854d189c4f5b8c88f57956b704401", "0359edba11ed1a0568094a6296a16c4d5ee4c8cfe2f5e2e6826871b5ecf8188f79"}, "00149961a78658030cc824af4c54fbf5294bec0cabdd", 272);
+    // P2WSH HTLC
+    CheckMaxWeightComputation("", {"3042021f5c4c29e6b686aae5b6d0751e90208592ea96d26bc81d78b0d3871a94a21fa8021f74dc2f971e438ccece8699c8fd15704c41df219ab37b63264f2147d15c34d801", "01", "6321024cf55e52ec8af7866617dc4e7ff8433758e98799906d80e066c6f32033f685f967029000b275210214827893e2dcbe4ad6c20bd743288edad21100404eb7f52ccd6062fd0e7808f268ac"}, "002089e84892873c679b1129edea246e484fd914c2601f776d4f2f4a001eb8059703", 318);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+} // namespace feebumper
+} // namespace wallet