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

Add PsbtOutputExt::update_with_descriptor #465

Merged
merged 6 commits into from
Oct 1, 2022

Conversation

afilini
Copy link
Contributor

@afilini afilini commented Sep 29, 2022

This essentially mirrors PsbtInputExt::update_with_descriptor but it works on PSBT outputs.

To avoid code duplication, the current logic that deals with PSBT inputs has been generalized to handle both input and outputs with a new internal PsbtFields trait.


I hope it's not too late to get this PR in the new 8.0 release (cc #462)

This essentially mirrors `PsbtInputExt::update_with_descriptor` but it
works on PSBT outputs.

To avoid code duplication, the current logic that deals with PSBT inputs
has been generalized to handle both input and outputs with a new
internal `PsbtFields` trait.
I guess this is a bit of an edge case, but in general I think it's
always better to append metadata to a PSBT without removing what's
already in there.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK f46830c

apoelstra
apoelstra previously approved these changes Sep 29, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 9f63165

Some PSBT may only contain the `non_witness_utxo` to save space, since
the `witness_utxo` would be strictly redundant. This should not be
considered an error.
@afilini
Copy link
Contributor Author

afilini commented Sep 30, 2022

Sorry if I keep pushing commits here, let me know if you'd like me to open separate PRs.

The last commit allows setting just the non_witness_utxo of a psbt input even if it is non-legacy. This was reported to us recently and I think it make sense to support this (but to be fair I haven't checked what the spec says).

I may have to push another fix because I'm having a failure in a bdk test since I switched to miniscript's update_input_with_descriptor, I need to figure out if my previous implementation was broken or if this one is.

@afilini
Copy link
Contributor Author

afilini commented Sep 30, 2022

Turns out nothing was really broken, but calling update_input_with_descriptor multiple times on the same psbt would lead to duplicated leaf hashes in tap_key_origins since it's just a vec, not a map.

I fixed it by sorting and the end and then running dedup on the vec

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 9a57e00

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 9a57e00. Not completely happy with internal trait abstraction. I may work on a future PR to abstract that into a function that takes 1) script pubkey and 2) descriptor and outputs the relevant fields that are added to the psbt.

We can then call this function on the input script pubkey and output script pubkey respectively.

@sanket1729 sanket1729 merged commit 2f1535e into rust-bitcoin:master Oct 1, 2022
@afilini afilini deleted the feat/psbt-output-metadata branch October 3, 2022 09:01
sanket1729 added a commit to sanket1729/elements-miniscript that referenced this pull request Oct 21, 2022
…022_10

2f1535e Merge rust-bitcoin/rust-miniscript#465: Add `PsbtOutputExt::update_with_descriptor`

Had to manually change the test vectors because elements has different
tagged hashes
notmandatory added a commit to bitcoindevkit/bdk that referenced this pull request Oct 27, 2022
c7a43d9 Remove unused code (Alekos Filini)
1ffd59d Upgrade to rust-bitcoin 0.29 (Alekos Filini)
ae4f4e5 Upgrade `rand` to `0.8` (Alekos Filini)
9854fd3 Remove deprecated address validators (Alekos Filini)

Pull request description:

  ### Description

  Upgrade BDK to rust-bitcoin 0.29

  Missing pieces:

  - [x] rust-miniscript `update_output_with_descriptor` - rust-bitcoin/rust-miniscript#465
  - [x] rust-miniscript 8.0.0 release - rust-bitcoin/rust-miniscript#462
  - [x] Upgrade rust-hwi to bitcoin 0.29 bitcoindevkit/rust-hwi#50
  - [x] Upgrade esplora-client to bitcoin 0.29 bitcoindevkit/rust-esplora-client#20
  - [x] Upgrade rand to 0.8 like secp256k1 did

  ### Notes to the reviewers

  The commits still need to be reordered and cleaned up

  ### Changelog notice

  - Upgrade rust-bitcoin to 0.29
  - Remove deprecated "address validators"

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  notmandatory:
    ACK c7a43d9

Tree-SHA512: 718a1baf3613b31ec1de39fe63467ebee38617963a4ce0670a617e20fe4f46a57c5786933cdde6cfad9fc76ce0af08843f58844fb4a89f5948cb42c697f802ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants