psbt: handle unspendable psbts #17524

1. 
   achow101 commented at 8:19 pm on November 19, 2019: member

   When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.
2. Have a PSBTAnalysis state that indicates invalid PSBT

   Invalid PSBTs need to be re-created, so the next role is the
   Creator (new PSBTRole). Additionally, we need to know what went
   wrong so an error field was added to PSBTAnalysis.
   
   A PSBTAnalysis indicating invalid will have empty everything,
   next will be set to PSBTRole::CREATOR, and an error message.

   638e40cb60
3. Mark PSBTs spending unspendable outputs as invalid in analysis 773d4572a4
4. fanquake added the label PSBT on Nov 19, 2019
5. 
   practicalswift commented at 8:23 pm on November 19, 2019: contributor

   Concept ACK

   Thanks for fixing this!

6. 
   Sjors commented at 8:52 pm on November 19, 2019: member

   Concept ACK
7. 
   practicalswift commented at 9:24 pm on November 19, 2019: contributor

   Note to reviewers:

   This is the issue (https://github.com/bitcoin/bitcoin/issues/17149#issuecomment-552202472) fixed by this PR:

   0$ bitcoind &
   1$ bitcoin-cli analyzepsbt "cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWAEHYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFv8/wADXYP/7//////8JxOh0LR2HAI8AAAAAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHEAABAACAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHENkMak8AAAAA"
   2bitcoind: consensus/tx_verify.cpp:130: unsigned int 
   3    GetP2SHSigOpCount(const CTransaction &, const CCoinsViewCache &): 
   4    Assertion `!coin.IsSpent()' failed.
   

8. in test/functional/rpc_psbt.py:420 in 773d4572a4

   415@@ -416,5 +416,10 @@ def test_psbt_input_keys(psbt_input, keys):
   416         analyzed = self.nodes[0].analyzepsbt(signed)
   417         assert analyzed['inputs'][0]['has_utxo'] and analyzed['inputs'][0]['is_final'] and analyzed['next'] == 'extractor'
   418 
   419+        self.log.info("PSBT spending unspendable outputs should have error message and Creator as next")
   420+        analysis = self.nodes[0].analyzepsbt('cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWAEHYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFv8/wADXYP/7//////8JxOh0LR2HAI8AAAAAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHEAABAACAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHENkMak8AAAAA')
   

   ---

   ---

   

   instagibbs commented at 9:25 pm on November 19, 2019:

   damnit you should have warned me this crashes Core, lol

   ---

   

   instagibbs commented at 9:31 pm on November 19, 2019:

   0      "final_scriptSig": {
   1        "asm": "1050369 0 0 0 OP_LEFT",
   2        "hex": "030107100001000080"
   3      }
   

   why does this PSBT have scriptsigs for OP_RETURN inputs? Can’t this PSBT be generated dynamically for better readability? Even if not, we should assert some things using decodepsbt to make sure we know the scriptpubkeys.

   ---

   

   practicalswift commented at 9:38 pm on November 19, 2019:

   The PSBT was created by a coverage-guided fuzzer using the fuzzing harness that I added in PR #17136 (“tests: Add fuzzing harness for various PSBT related functions”). The only post-processing done was a test-case minimisation run :)
9. 
   instagibbs commented at 9:26 pm on November 19, 2019: member

   @practicalswift 2 minutes too late bro :joy:
10. 
    DrahtBot commented at 10:03 pm on November 19, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
    • #17156 (psbt: check that various indexes and amounts are within bounds 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.

11. 
    Sjors commented at 10:05 am on November 20, 2019: member

    ACK 773d457

    The new creator role and error field are release note worthy.

    This should be back-ported.

12. 
    darosior commented at 1:30 pm on November 23, 2019: member

    The removal of the input field in case of error would break the API, wouldn’t be better to push an empty array instead ?
13. 
    achow101 commented at 4:58 pm on November 23, 2019: member

    I don’t think this really breaks the api since this error condition would cause a crash rather than return any sort of result previously.
14. 
    instagibbs commented at 2:15 pm on November 25, 2019: member

    After some thought ACK https://github.com/bitcoin/bitcoin/commit/773d4572a4864ab7b6380858d07d9579ff6dd9a2
15. MarcoFalke referenced this in commit fae94785d9 on Dec 10, 2019
16. MarcoFalke merged this on Dec 10, 2019
17. MarcoFalke closed this on Dec 10, 2019

    ---

18. sidhujag referenced this in commit f253162790 on Dec 10, 2019
19. MarcoFalke referenced this in commit f1d3d3430e on Dec 11, 2019
20. luke-jr referenced this in commit 551583398b on Jan 3, 2020
21. luke-jr referenced this in commit ca5f8deefd on Jan 3, 2020
22. fanquake referenced this in commit febf04841f on Jan 4, 2020
23. MarkLTZ referenced this in commit d99bc96496 on Feb 13, 2020
24. barrystyle referenced this in commit 11c68f79bc on Feb 29, 2020
25. barrystyle referenced this in commit c9728de7f4 on Feb 29, 2020
26. deadalnix referenced this in commit 5a49f316c1 on Oct 23, 2020
27. sidhujag referenced this in commit 030f3da4f4 on Nov 10, 2020
28. MarcoFalke locked this on Dec 16, 2021

Contributors
achow101 practicalswift Sjors instagibbs DrahtBot darosior

Labels
PSBT