Make AnalyzePSBT next role calculation simple, correct #18224
Merged
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
fef9c78 to
1ef28b4
Compare
|
ACK 1ef28b4, much nicer. Don't forget to document the bug fix. |
|
@achow101 poing |
|
ACK 1ef28b4 |
|
ACK 1ef28b4 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
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
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Mar 5, 2020
Github-Pull: bitcoin#18224 Rebased-From: 1ef28b4
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.
Sniped test and alternative to #18220
Sjors documenting the issue:
It makes it much clearer that the role has been decided before hitting the
calc_feeblock, 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.