Merge #18224: Make AnalyzePSBT next role calculation simple, correct

1ef28b4 Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)

Pull request description:

  Sniped test and alternative to bitcoin/bitcoin#18220

  Sjors documenting the issue:
  ```
  A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))

  {
    "inputs": [
      {
        "has_utxo": true,
        "is_final": false,
        "next": "finalizer"
      }
    ],
    "estimated_vsize": 141,
    "estimated_feerate": 1e-05,
    "fee": 1.41e-06,
    "next": "signer"
  }
  I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
  ```

  It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.

  Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.

ACKs for top commit:
  Sjors:
    ACK 1ef28b4, much nicer. Don't forget to document the bug fix.
  achow101:
    ACK 1ef28b4
  Empact:
    ACK bitcoin/bitcoin@1ef28b4

Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934

Author: meshcollider

src/node/psbt.cpp

@@ -18,9 +18,7 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
     PSBTAnalysis result;
 
     bool calc_fee = true;
-    bool all_final = true;
-    bool only_missing_sigs = true;
-    bool only_missing_final = false;
+
     CAmount in_amt = 0;
 
     result.inputs.resize(psbtx.tx->vin.size());
@@ -29,6 +27,9 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
         PSBTInput& input = psbtx.inputs[i];
         PSBTInputAnalysis& input_analysis = result.inputs[i];
 
+        // We set next role here and ratchet backwards as required
+        input_analysis.next = PSBTRole::EXTRACTOR;
+
         // Check for a UTXO
         CTxOut utxo;
         if (psbtx.GetInputUTXO(utxo, i)) {
@@ -57,7 +58,6 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
         // Check if it is final
         if (!utxo.IsNull() && !PSBTInputSigned(input)) {
             input_analysis.is_final = false;
-            all_final = false;
 
             // Figure out what is missing
             SignatureData outdata;
@@ -74,22 +74,24 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
                 if (outdata.missing_pubkeys.empty() && outdata.missing_redeem_script.IsNull() && outdata.missing_witness_script.IsNull() && !outdata.missing_sigs.empty()) {
                     input_analysis.next = PSBTRole::SIGNER;
                 } else {
-                    only_missing_sigs = false;
                     input_analysis.next = PSBTRole::UPDATER;
                 }
             } else {
-                only_missing_final = true;
                 input_analysis.next = PSBTRole::FINALIZER;
             }
         } else if (!utxo.IsNull()){
             input_analysis.is_final = true;
         }
     }
 
-    if (all_final) {
-        only_missing_sigs = false;
-        result.next = PSBTRole::EXTRACTOR;
+    // Calculate next role for PSBT by grabbing "minumum" PSBTInput next role
+    result.next = PSBTRole::EXTRACTOR;
+    for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
+        PSBTInputAnalysis& input_analysis = result.inputs[i];
+        result.next = std::min(result.next, input_analysis.next);
     }
+    assert(result.next > PSBTRole::CREATOR);
+
     if (calc_fee) {
         // Get the output amount
         CAmount out_amt = std::accumulate(psbtx.tx->vout.begin(), psbtx.tx->vout.end(), CAmount(0),
@@ -139,17 +141,6 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
             result.estimated_feerate = feerate;
         }
 
-        if (only_missing_sigs) {
-            result.next = PSBTRole::SIGNER;
-        } else if (only_missing_final) {
-            result.next = PSBTRole::FINALIZER;
-        } else if (all_final) {
-            result.next = PSBTRole::EXTRACTOR;
-        } else {
-            result.next = PSBTRole::UPDATER;
-        }
-    } else {
-        result.next = PSBTRole::UPDATER;
     }
 
     return result;

test/functional/rpc_psbt.py

@@ -437,6 +437,10 @@ def test_psbt_input_keys(psbt_input, keys):
         assert_equal(analysis['next'], 'creator')
         assert_equal(analysis['error'], 'PSBT is not valid. Input 0 has invalid value')
 
+        self.log.info("PSBT with signed, but not finalized, inputs should have Finalizer as next")
+        analysis = self.nodes[0].analyzepsbt('cHNidP8BAHECAAAAAZYezcxdnbXoQCmrD79t/LzDgtUo9ERqixk8wgioAobrAAAAAAD9////AlDDAAAAAAAAFgAUy/UxxZuzZswcmFnN/E9DGSiHLUsuGPUFAAAAABYAFLsH5o0R38wXx+X2cCosTMCZnQ4baAAAAAABAR8A4fUFAAAAABYAFOBI2h5thf3+Lflb2LGCsVSZwsltIgIC/i4dtVARCRWtROG0HHoGcaVklzJUcwo5homgGkSNAnJHMEQCIGx7zKcMIGr7cEES9BR4Kdt/pzPTK3fKWcGyCJXb7MVnAiALOBgqlMH4GbC1HDh/HmylmO54fyEy4lKde7/BT/PWxwEBAwQBAAAAIgYC/i4dtVARCRWtROG0HHoGcaVklzJUcwo5homgGkSNAnIYDwVpQ1QAAIABAACAAAAAgAAAAAAAAAAAAAAiAgL+CIiB59NSCssOJRGiMYQK1chahgAaaJpIXE41Cyir+xgPBWlDVAAAgAEAAIAAAACAAQAAAAAAAAAA')
+        assert_equal(analysis['next'], 'finalizer')
+
         analysis = self.nodes[0].analyzepsbt('cHNidP8BAHECAAAAAfA00BFgAm6tp86RowwH6BMImQNL5zXUcTT97XoLGz0BAAAAAAD/////AgCAgWrj0AcAFgAUKNw0x8HRctAgmvoevm4u1SbN7XL87QKVAAAAABYAFPck4gF7iL4NL4wtfRAKgQbghiTUAAAAAAABAR8A8gUqAQAAABYAFJUDtxf2PHo641HEOBOAIvFMNTr2AAAA')
         assert_equal(analysis['next'], 'creator')
         assert_equal(analysis['error'], 'PSBT is not valid. Output amount invalid')