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

wallet: Precompute Txdata after setting PSBT inputs' UTXOs #25590

Merged
merged 1 commit into from Jul 19, 2022

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.

txid = self.nodes[0].sendtoaddress(addr, 1)
vout = find_vout_for_address(self.nodes[0], txid, addr)
psbt = self.nodes[0].createpsbt([{"txid": txid, "vout": vout}], [{self.nodes[0].getnewaddress(): 0.9999}])
signed = self.nodes[0].walletprocesspsbt(psbt)
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 12f5f8e

fixes the issue I was having

test/functional/wallet_taproot.py Outdated Show resolved Hide resolved
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.
@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
Member

@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
Member

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

@aureleoules
Copy link
Member

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.

None yet

6 participants