Merge #19740: [0.20] wallet: Simplify and fix CWallet::SignTransaction

6a326cf66f04948ffe729e5540a69e159a7840ad tests: Test that a fully signed tx given to signrawtx is unchanged (Andrew Chow)
2d48d7dcfb93eeec36dd6f5cbad289b89ec69324 Simplify and fix CWallet::SignTransaction (Andrew Chow)

Pull request description:

  Backport `CWallet::SignTransaction` from master which is simpler and not broken.

  Previously `CWallet::SignTransaction` would return false for a fully signed transaction. This is a regression and obviously incorrect - a fully signed transaction is always complete. This occurs because `CWallet::SignTransaction` would iterate through each input and skip any further checks if the input was already signed. It would then end up falling through to the `return false` catch-all thus erroneously saying a fully signed transaction is incomplete. The change to attempting to use all `ScriptPubKeyMan`s fixes this problem since the `LegacyScriptPubKeyMan` (the only spkm implemented in 0.20) will verify inputs during its signing attempt and correctly return that it is complete when the inputs verify. Thus a fully signed transaction will be correctly identified as complete, `LegacyScriptPubKeyMan::SignTranaction` will return true, and so `CWallet::Transaction` will return true too.

  Note that this is not a backport of any specific commit. Rather it is the end result of the changes we have made to this function in master.

  Fixes #19737

ACKs for top commit:
  fjahr:
    Code review ACK 6a326cf66f04948ffe729e5540a69e159a7840ad
  MarcoFalke:
    re-ACK 6a326cf66f 👓

Tree-SHA512: 7b8e04cfc264de01a2de08a99477c1a110fa0965dcbd4305bf4632a165f28b090746789195f5cb584eb6d85e27d4801a09d2dad28e508d7898c7c088e771812b

1 files changed, 8 insertions, 42 deletions

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index af3f958349..bd2b0145b8 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2474,50 +2474,16 @@ bool CWallet::SignTransaction(CMutableTransaction& tx) const
 
 bool CWallet::SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const
 {
-    // Sign the tx with ScriptPubKeyMans
-    // Because each ScriptPubKeyMan can sign more than one input, we need to keep track of each ScriptPubKeyMan that has signed this transaction.
-    // Each iteration, we may sign more txins than the txin that is specified in that iteration.
-    // We assume that each input is signed by only one ScriptPubKeyMan.
-    std::set<uint256> visited_spk_mans;
-    for (unsigned int i = 0; i < tx.vin.size(); i++) {
-        // Get the prevout
-        CTxIn& txin = tx.vin[i];
-        auto coin = coins.find(txin.prevout);
-        if (coin == coins.end() || coin->second.IsSpent()) {
-            input_errors[i] = "Input not found or already spent";
-            continue;
-        }
-
-        // Check if this input is complete
-        SignatureData sigdata = DataFromTransaction(tx, i, coin->second.out);
-        if (sigdata.complete) {
-            continue;
-        }
-
-        // Input needs to be signed, find the right ScriptPubKeyMan
-        std::set<ScriptPubKeyMan*> spk_mans = GetScriptPubKeyMans(coin->second.out.scriptPubKey, sigdata);
-        if (spk_mans.size() == 0) {
-            input_errors[i] = "Unable to sign input, missing keys";
-            continue;
-        }
-
-        for (auto& spk_man : spk_mans) {
-            // If we've already been signed by this spk_man, skip it
-            if (visited_spk_mans.count(spk_man->GetID()) > 0) {
-                continue;
-            }
-
-            // Sign the tx.
-            // spk_man->SignTransaction will return true if the transaction is complete,
-            // so we can exit early and return true if that happens.
-            if (spk_man->SignTransaction(tx, coins, sighash, input_errors)) {
-                return true;
-            }
-
-            // Add this spk_man to visited_spk_mans so we can skip it later
-            visited_spk_mans.insert(spk_man->GetID());
+    // Try to sign with all ScriptPubKeyMans
+    for (ScriptPubKeyMan* spk_man : GetAllScriptPubKeyMans()) {
+        // spk_man->SignTransaction will return true if the transaction is complete,
+        // so we can exit early and return true if that happens
+        if (spk_man->SignTransaction(tx, coins, sighash, input_errors)) {
+            return true;
         }
     }
+
+    // At this point, one input was not fully signed otherwise we would have exited already
     return false;
 }