Verify PSBT inputs rather than check for fields being empty #25595

1. 
   instagibbs commented at 3:01 PM on July 12, 2022: member

   In a few keys spots, PSBT finality is checked by looking for non-empty witness data.

   This complicates a couple things:

   1. Empty data can be valid in certain cases
   2. User may be passed bogus final data by a counterparty during PSBT work happening, and end up with incorrect signatures that they may not be able to check in other contexts if the UTXO doesn't exist yet in chain/mempool, timelocks, etc.

   On the whole I think these heavier checks are worth it in case someone is actually assuming the signatures are correct if our API is saying so.

2. fanquake requested review from achow101 on Jul 12, 2022
3. 
   DrahtBot commented at 6:09 PM on July 12, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21283 (Implement BIP 370 PSBTv2 by achow101)
   • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. in src/psbt.cpp:272 in 862236cf70 outdated

    267 | @@ -267,6 +268,12 @@ void PSBTOutput::Merge(const PSBTOutput& output)
    268 |      if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
    269 |      if (m_tap_tree.has_value() && !output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree;
    270 |  }
    271 | +
    272 | +bool PSBTInputSigned(const PSBTInput& input, const CScript& script_pubkey)
   

   ---

   ---

   

   achow101 commented at 11:01 PM on July 12, 2022:

   Considering that a PSBTInput already has the utxo, it's unnecessary to have the spk passed in by the caller. It could just be looked up.

   ---

   

   instagibbs commented at 4:40 PM on August 8, 2022:

   for non-witness utxos you have the whole txn, so you kind of need the prevout index of that tx? Pushed a change that splits concepts but requires the PSBT/index tuple to be passed in instead.

   ---

   

   achow101 commented at 8:52 PM on August 19, 2022:

   The caller, SignPSBTInput fetches the UTXO after it does this check. So that could be reordered to fetch the UTXO first, then pass that into this function. Thins would remove the need for the lookup to be done twice.

   This would also be a lot easier with PSBTv2 implemented as the prevout info will be in the PSBTInput, regardless of version.

5. instagibbs force-pushed on Aug 8, 2022
6. instagibbs force-pushed on Oct 11, 2022
7. 
   achow101 commented at 6:09 PM on October 11, 2022: member

   ACK 862236cf707e49174b0fa3413d42b3205d2ea3eb

8. in src/psbt.cpp:274 in 862236cf70 outdated

    267 | @@ -267,6 +268,12 @@ void PSBTOutput::Merge(const PSBTOutput& output)
    268 |      if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
    269 |      if (m_tap_tree.has_value() && !output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree;
    270 |  }
    271 | +
    272 | +bool PSBTInputSigned(const PSBTInput& input, const CScript& script_pubkey)
    273 | +{
    274 | +    return VerifyScript(input.final_script_sig, script_pubkey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, DUMMY_SIGNATURE_CREATOR.Checker());
   

   ---

   ---

   

   achow101 commented at 6:13 PM on October 11, 2022:

   Is having the dummy checker correct? While this will check the script, it will also pass any signature as valid.

   ---

   

   instagibbs commented at 6:16 PM on October 11, 2022:

   sounds like I need tests eh

   ---

   

   instagibbs commented at 6:17 PM on October 11, 2022:

   suggested alternative?

   ---

   

   achow101 commented at 6:28 PM on October 11, 2022:

   To actually verify the signatures you need to use a TransactionSignatureChecker. The data that needs to be passed into that are all available in the callers to this function.

9. instagibbs force-pushed on Oct 12, 2022
10. instagibbs force-pushed on Oct 12, 2022
11. 
    instagibbs commented at 5:14 PM on October 12, 2022: member

    Fixed the tests, it now fails if signatures fail and passes when they're complete.

12. in src/psbt.cpp:303 in 5cbd014f92 outdated

     292 | +        utxo = input.non_witness_utxo->vout[prevout.n];
     293 | +    } else if (!input.witness_utxo.IsNull()) {
     294 | +        utxo = input.witness_utxo;
     295 | +    } else {
     296 | +        return false;
     297 | +    }
    

    ---

    ---

    

    achow101 commented at 3:25 PM on October 13, 2022:

    The UTXO fetching has snuck back in.

    ---

    

    instagibbs commented at 5:44 PM on October 13, 2022:

    I went back and forth, the "extra code" to me seems to make the interface cleaner? Leaving as is if that's ok.

13. in src/psbt.cpp:300 in 5cbd014f92 outdated

     295 | +    } else {
     296 | +        return false;
     297 | +    }
     298 | +
     299 | +    if (txdata) {
     300 | +        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, GenericTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL});
    

    ---

    ---

    

    achow101 commented at 3:27 PM on October 13, 2022:

    Should probably be specific and use MutableTransactionSignatureChecker

    ---

    

    instagibbs commented at 5:44 PM on October 13, 2022:

    done

14. instagibbs force-pushed on Oct 13, 2022
15. 
    instagibbs commented at 5:57 PM on October 13, 2022: member

    @achow101 maybe a silent merge conflict on master?

16. 
    achow101 commented at 6:14 PM on October 13, 2022: member

    @achow101 maybe a silent merge conflict on master?

    Yes: #24407 (comment)

17. 
    achow101 commented at 6:57 PM on October 13, 2022: member

    ACK 2e8798389aacdfcd42ca818459fb9c824af18da1

    I'd like to see a test, but this could go in as is.

18. instagibbs force-pushed on Oct 17, 2022
19. Verify PSBT inputs rather than check for fields being empty d25699280a
20. Add test for PSBT input verification e133264c5b
21. instagibbs force-pushed on Oct 17, 2022
22. 
    instagibbs commented at 3:14 PM on October 17, 2022: member

    Added a basic test that fails if parent commit is reverted

23. fanquake requested review from achow101 on Oct 19, 2022
24. 
    achow101 commented at 3:05 PM on October 19, 2022: member

    ACK e133264c5b1f72e94dcb9cebd85cdb523fcf8070

25. fanquake merged this on Oct 20, 2022
26. fanquake closed this on Oct 20, 2022

    ---

27. sidhujag referenced this in commit 0ab49b3878 on Oct 23, 2022
28. bitcoin locked this on Oct 20, 2023

Contributors

instagibbsDrahtBotachow101

Review Requested
achow101