psbt: AnalyzePSBT set next to"finalizer" when all inputs need finalizing #18220
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you explain the code path master is hitting? Mind sharing the PSBT? :) |
|
Just try with a ColdCard simulator, it's pretty reproducible. On master The reason master doesn't check |
|
If it's really easy to set up I think a checked in test is in order |
…zing Previously the next step would be SIGNER if all of the inputs had FINALIZER as their next step.
a4cdad9 to
64a44dc
Compare
|
I added a test. |
|
I had such trouble understanding the function I decided a proper refactor of logic was in order: #18224 |
meshcollider
added a commit
that referenced
this pull request
Mar 2, 2020
1ef28b4 Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders) Pull request description: Sniped test and alternative to #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 1ef28b4 Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Mar 2, 2020
…orrect 1ef28b4 Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders) Pull request description: Sniped test and alternative to bitcoin#18220 Sjors documenting the issue: ``` A PSBT signed by ColdCard was analyzed as follows (see bitcoin#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@1ef28b4 Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
…orrect 1ef28b4 Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders) Pull request description: Sniped test and alternative to bitcoin#18220 Sjors documenting the issue: ``` A PSBT signed by ColdCard was analyzed as follows (see bitcoin#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@1ef28b4 Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AnalyzePSBTso that it returns"next": "finalizer"instead.