Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AnalyzePSBT next role calculation simple, correct #18224

Merged
merged 1 commit into from Mar 2, 2020

Conversation

@instagibbs
Copy link
Member

instagibbs commented Feb 28, 2020

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.

@instagibbs instagibbs force-pushed the instagibbs:analyze_psbt_role_simple branch from fef9c78 to 1ef28b4 Feb 28, 2020
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 28, 2020

ACK 1ef28b4, much nicer. Don't forget to document the bug fix.

@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Feb 28, 2020

@achow101 poing

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Feb 28, 2020

ACK 1ef28b4

@DrahtBot DrahtBot added the Tests label Feb 28, 2020
@Empact

This comment has been minimized.

Copy link
Member

Empact commented Feb 28, 2020

ACK 1ef28b4

@fanquake fanquake added the PSBT label Feb 28, 2020
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 28, 2020

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

Conflicts

No conflicts as of last run.

@meshcollider meshcollider merged commit 1f88624 into bitcoin:master Mar 2, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added 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 added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.