psbt: handle unspendable psbts by achow101 · Pull Request #17524 · bitcoin/bitcoin

achow101 · 2019-11-19T20:19:57Z

When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.

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.

practicalswift · 2019-11-19T20:23:44Z

Concept ACK

Thanks for fixing this!

Sjors · 2019-11-19T20:52:32Z

Concept ACK

practicalswift · 2019-11-19T21:24:27Z

Note to reviewers:

This is the issue (#17149 (comment)) 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.

instagibbs · 2019-11-19T21:25:42Z

test/functional/rpc_psbt.py

        assert analyzed['inputs'][0]['has_utxo'] and analyzed['inputs'][0]['is_final'] and analyzed['next'] == 'extractor'

+        self.log.info("PSBT spending unspendable outputs should have error message and Creator as next")
+        analysis = self.nodes[0].analyzepsbt('cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWAEHYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFv8/wADXYP/7//////8JxOh0LR2HAI8AAAAAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHEAABAACAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHENkMak8AAAAA')


damnit you should have warned me this crashes Core, lol

"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.

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 · 2019-11-19T21:26:59Z

@practicalswift 2 minutes too late bro 😂

DrahtBot · 2019-11-19T22:03:40Z

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 · 2019-11-20T10:05:22Z

ACK 773d457

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

This should be back-ported.

darosior · 2019-11-23T13:30:44Z

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 · 2019-11-23T16:58:33Z

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 · 2019-11-25T14:15:22Z

After some thought ACK 773d457

773d457 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow) 638e40c Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow) Pull request description: When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early. ACKs for top commit: Sjors: ACK 773d457 instagibbs: After some thought ACK 773d457 Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5

773d457 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow) 638e40c Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow) Pull request description: When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early. ACKs for top commit: Sjors: ACK 773d457 instagibbs: After some thought ACK bitcoin@773d457 Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5

7e8b4de rpc: add missing newline in analyzepsbt rpcresult (Jon Atack) Pull request description: follow-up to 638e40c in #17524 before ``` "error" : "error" (string) Error message if there is one} ``` after ``` "error" : "error" (string) Error message if there is one } ``` ACKs for top commit: practicalswift: ACK 7e8b4de promag: ACK 7e8b4de. emilengler: ACK 7e8b4de Tree-SHA512: 4cdd365e39d15b7925ea277b7ff3e9bfdc22f5845aa41ca547343b4dabdf319579843a1c7f11fb0edd6abbc31bae2ec96236b83e84f8872bd662848723725e4c

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. Github-Pull: bitcoin#17524 Rebased-From: 638e40c

Github-Pull: bitcoin#17524 Rebased-From: 773d457

ca5f8de Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow) 5515833 Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow) Pull request description: Backport of #17524 ACKs for top commit: achow101: ACK ca5f8de Tree-SHA512: b5f2b951beb9477ac3176a0aade845654d2108ca3a9fbc72097ba4b4797df5419053d6b489bbaa03be08cb8cfdc37a83db8b7642ffa52d42b7aa8ea14aff39cc

- [0.19] wallet: Reset reused transactions cache bitcoin#18083 - 0.19: Backports bitcoin#17792 - psbt: handle unspendable psbts bitcoin#17524 - qt: Fix comparison function signature bitcoin#17634 - psbt: check that various indexes and amounts are within bounds bitcoin#17156 - [0.19] psbt: check that various indexes and amounts are within bounds bitcoin#18079 - [0.19] Final backports for 0.19.1 bitcoin#17988 - Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash bitcoin#17924 - qt: Fix deprecated QCharRef usage bitcoin#18101 - gui: Throttle GUI update pace when -reindex bitcoin#18121 - gui: Fix race in WalletModel::pollBalanceChanged bitcoin#18123 - gui: Fix unintialized WalletView::progressDialog bitcoin#18062 - Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open bitcoin#18007 - bug-fix macos: give free bytes to F_PREALLOCATE bitcoin#17887 - test: add missing #include to fix compiler errors bitcoin#17980 - zmq: Fix due to invalid argument and multiple notifiers bitcoin#17445

Summary: * 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. * Mark PSBTs spending unspendable outputs as invalid in analysis This is a backport of Core [[bitcoin/bitcoin#17524 | PR17524]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8065

773d457 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow) 638e40c Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow) Pull request description: When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early. ACKs for top commit: Sjors: ACK 773d457 instagibbs: After some thought ACK bitcoin@773d457 Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5

achow101 added 2 commits November 19, 2019 14:54

Mark PSBTs spending unspendable outputs as invalid in analysis

773d457

achow101 mentioned this pull request Nov 19, 2019

Potential PSBT issues found by the PSBT fuzzer #17149

Closed

fanquake added the PSBT label Nov 19, 2019

instagibbs reviewed Nov 19, 2019

View reviewed changes

Sjors mentioned this pull request Nov 23, 2019

gui: save and load PSBT #17509

Merged

maflcko merged commit 773d457 into bitcoin:master Dec 10, 2019

jonatack mentioned this pull request Dec 10, 2019

rpc: add missing newline in analyzepsbt RPCResult #17714

Merged

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 3, 2020

Mark PSBTs spending unspendable outputs as invalid in analysis

ca5f8de

Github-Pull: bitcoin#17524 Rebased-From: 773d457

luke-jr mentioned this pull request Jan 3, 2020

[0.19] psbt: handle unspendable psbts #17853

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

psbt: handle unspendable psbts#17524

psbt: handle unspendable psbts#17524
maflcko merged 2 commits intobitcoin:masterfrom
achow101:analyzepsbt-invalid

achow101 commented Nov 19, 2019

Uh oh!

practicalswift commented Nov 19, 2019

Uh oh!

Sjors commented Nov 19, 2019

Uh oh!

practicalswift commented Nov 19, 2019 •

edited

Loading

Uh oh!

instagibbs Nov 19, 2019

Uh oh!

instagibbs Nov 19, 2019

Uh oh!

practicalswift Nov 19, 2019

Uh oh!

instagibbs commented Nov 19, 2019

Uh oh!

DrahtBot commented Nov 19, 2019 •

edited

Loading

Uh oh!

Sjors commented Nov 20, 2019 •

edited

Loading

Uh oh!

darosior commented Nov 23, 2019

Uh oh!

achow101 commented Nov 23, 2019

Uh oh!

instagibbs commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

achow101 commented Nov 19, 2019

Uh oh!

practicalswift commented Nov 19, 2019

Uh oh!

Sjors commented Nov 19, 2019

Uh oh!

practicalswift commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Nov 19, 2019

Uh oh!

DrahtBot commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior commented Nov 23, 2019

Uh oh!

achow101 commented Nov 23, 2019

Uh oh!

instagibbs commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

practicalswift commented Nov 19, 2019 •

edited

Loading

DrahtBot commented Nov 19, 2019 •

edited

Loading

Sjors commented Nov 20, 2019 •

edited

Loading