psbt: handle unspendable psbts #17524

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:analyzepsbt-invalid changing 6 files +29 −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.

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

Mark PSBTs spending unspendable outputs as invalid in analysis 773d4572a4
achow101 cross-referenced this on Nov 19, 2019 from issue Potential PSBT issues found by the PSBT fuzzer by practicalswift
fanquake added the label PSBT on Nov 19, 2019
practicalswift commented at 8:23 PM on November 19, 2019: contributor

Concept ACK

Thanks for fixing this!
Sjors commented at 8:52 PM on November 19, 2019: member

Concept ACK

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:

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

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:

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

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 :)

instagibbs commented at 9:26 PM on November 19, 2019: member

@practicalswift 2 minutes too late bro :joy:
DrahtBot commented at 10:03 PM on November 19, 2019: contributor


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.
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.
Sjors cross-referenced this on Nov 23, 2019 from issue gui: save and load PSBT by Sjors
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 ?
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.
instagibbs commented at 2:15 PM on November 25, 2019: member

After some thought ACK https://github.com/bitcoin/bitcoin/commit/773d4572a4864ab7b6380858d07d9579ff6dd9a2
MarcoFalke referenced this in commit fae94785d9 on Dec 10, 2019
MarcoFalke merged this on Dec 10, 2019
MarcoFalke closed this on Dec 10, 2019
sidhujag referenced this in commit f253162790 on Dec 10, 2019
jonatack cross-referenced this on Dec 10, 2019 from issue rpc: add missing newline in analyzepsbt RPCResult by jonatack
MarcoFalke referenced this in commit f1d3d3430e on Dec 11, 2019
luke-jr referenced this in commit 551583398b on Jan 3, 2020
luke-jr referenced this in commit ca5f8deefd on Jan 3, 2020
luke-jr cross-referenced this on Jan 3, 2020 from issue [0.19] psbt: handle unspendable psbts by luke-jr
fanquake referenced this in commit febf04841f on Jan 4, 2020
MarkLTZ referenced this in commit d99bc96496 on Feb 13, 2020
barrystyle referenced this in commit 11c68f79bc on Feb 29, 2020
barrystyle referenced this in commit c9728de7f4 on Feb 29, 2020
MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
deadalnix referenced this in commit 5a49f316c1 on Oct 23, 2020
sidhujag referenced this in commit 030f3da4f4 on Nov 10, 2020
bitcoin locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#17136 tests: Add fuzzing harness for various PSBT related functions #17149 Potential PSBT issues found by the PSBT fuzzer #17509 gui: save and load PSBT #17714 rpc: add missing newline in analyzepsbt RPCResult #17853 [0.19] psbt: handle unspendable psbts

psbt: handle unspendable psbts #17524

Conflicts