Skip to content

Conversation

@achow101
Copy link
Member

If we are given a PSBT that is missing one or more input UTXOs, our
PrecomputedTransactionData will be incorrect and missing information
that it should otherwise have, and therefore we may not produce a
signature when we should. To avoid this problem, we can do the
precomputation after we have set the UTXOs the wallet is able to set for
the PSBT.

Also adds a test for this behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reviewers: without the fix, this would have to be called twice in a row

Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 12f5f8e0b726373109fae9356be2fcb5ea3c608e

fixes the issue I was having

If we are given a PSBT that is missing one or more input UTXOs, our
PrecomputedTransactionData will be incorrect and missing information
that it should otherwise have, and therefore we may not produce a
signature when we should. To avoid this problem, we can do the
precomputation after we have set the UTXOs the wallet is able to set for
the PSBT.

Also adds a test for this behavior.
@achow101 achow101 force-pushed the sign-psbt-tr-wo-utxos branch from 12f5f8e to d2ed976 Compare July 11, 2022 22:08
@instagibbs
Copy link
Member

reACK d2ed976

@Sjors
Copy link
Member

Sjors commented Jul 14, 2022

ACK d2ed976

@DrahtBot
Copy link
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:

  • #25625 (test: add test for decoding PSBT with per-input preimage types by theStack)

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.

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test does not fail without the fix, is this normal?

@Sjors
Copy link
Member

Sjors commented Jul 19, 2022

@aureleoules I'm fairly sure I tested that the test did fail without the fix (don't forget to recompile after removing the fix). Can you try again?

@aureleoules
Copy link
Contributor

My bad @Sjors, I didn't run the test with --descriptors.

@aureleoules
Copy link
Contributor

ACK d2ed976.
This change does fix the behavior of the test.

@maflcko maflcko merged commit 1b285b7 into bitcoin:master Jul 19, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 19, 2022
…uts' UTXOs

d2ed976 wallet: Precompute Txdata after setting PSBT inputs' UTXOs (Andrew Chow)

Pull request description:

  If we are given a PSBT that is missing one or more input UTXOs, our
  PrecomputedTransactionData will be incorrect and missing information
  that it should otherwise have, and therefore we may not produce a
  signature when we should. To avoid this problem, we can do the
  precomputation after we have set the UTXOs the wallet is able to set for
  the PSBT.

  Also adds a test for this behavior.

ACKs for top commit:
  instagibbs:
    reACK bitcoin@d2ed976
  Sjors:
    ACK d2ed976
  aureleoules:
    ACK d2ed976.

Tree-SHA512: 71beb6c7946096e82cfca83f36277302aa9e69d27b4f6d73d7d8f2f9f0ea1c0d653e846fa6aebee5e4763f56f950b4481240e953f6a2412caa84908d519171e1
@bitcoin bitcoin locked and limited conversation to collaborators Jul 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants