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.

Sign up for GitHub

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

Jump to bottom

Change Signer::sign_tx_input to Signer::sign_psbt_input #172

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Change Signer::sign_tx_input to Signer::sign_psbt_input #172

merged 1 commit into from
Dec 21, 2023

Conversation

benthecarman
Copy link
Contributor

With the previous API, signing taproot inputs comes with a lot of problems. Taproot signatures commit to the previous txo of all the inputs so to be able to construct a signature, you need to have all of them available. The previous API would require the user to look up the outputs themself which does not really work for mobile clients and it is a bad practice. This changes it to a PSBT so we can give all the necessary info to the user directly.

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Nov 15, 2023

Mmm I'll have to sit on this one for a bit. I'm actually experimenting with taproot DLC now I'll let you know once I've got a better understanding of the issue.

@benthecarman
Copy link
Contributor Author

benthecarman commented Nov 15, 2023 •
edited
Loading

Mmm I'll have to sit on this one for a bit. I'm actually experimenting with taproot DLC now I'll let you know once I've got a better understanding of the issue.

Okay, FWIW, I couldn't sign a funding tx with taproot inputs without this but now was able to.

https://mutinynet.com/tx/58688201c22b27fb390b89656bad670fddad8322a2d3a6bec1d485a33c1cfd9e

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Nov 24, 2023

I'm not sure I see the advantage of using a PSBT compared to just passing say an array of (TxOut, Script)? E.g., in the bitcoin-rpr-provider code it looks like you're just de-constructing it and then re-adding things after. I can see that in can be useful in some cases, but I feel it'd be better to keep it outside of the interface and let users decided to use it or not, unless there is some advantage that I'm not seeing?

@benthecarman
Copy link
Contributor Author

You could just pass the txouts and scripts but i imagine most people will just make it into a PSBT, most wallets and libraries just use PSBTs for negotiating all this information.

@bennyhodl
Copy link
Contributor

Would like to see this make it's way through.

If most users, like myself, are using BDK it makes the API a lot easier.

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Dec 4, 2023

I'm not convinced that the PSBT should be created within the library. It's easy to create it if people want to use it, but it adds many lines of codes to the library (creating and unpacking it after it gets signed).

@benthecarman benthecarman mentioned this pull request Dec 4, 2023
Closed
@benthecarman
Copy link
Contributor Author

Created #184 but tbh this still feels like a better approach

Tibo-lg
Tibo-lg reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

I personally much prefer #184. @luckysori any opinion on this?

dlc-manager/src/contract_updater.rs Outdated Show resolved Hide resolved
@luckysori
Copy link
Collaborator

I personally much prefer #184. @luckysori any opinion on this?

Sorry I took so long to get to this.

#184 is nice because the change set is smaller and clearer. But I can see why this PR might make things easier for others. Also, I would assume most people using rust-dlc use bdk (we certainly do).

We're not currently affected by this, so I don't feel strongly about this.

@benthecarman benthecarman mentioned this pull request Dec 12, 2023
Merged
@bonomat
Copy link
Collaborator

bonomat commented Dec 12, 2023

I can't speak to how much this change will make life easier for signing taproot transactions but in general I'm in favor of using PSBTs on the API level. It's very likely that developers using rust-dlc will also be using bdk which already uses PSBTs.

As @luckysori already said, we're currently not affected by this, so I neithert feel strongly about this (:

Tibo-lg
Tibo-lg requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Alright I'll try to put my PSBTphobia aside, let's go with this. Couple of things to fix and then LGTM.

dlc-manager/src/contract_updater.rs Outdated Show resolved Hide resolved
dlc-manager/src/contract_updater.rs Outdated Show resolved Hide resolved
simple-wallet/src/lib.rs Show resolved Hide resolved
@benthecarman benthecarman force-pushed the sign-psbt branch 2 times, most recently from 0e71ed8 to d841f2a Compare December 13, 2023 01:07
@benthecarman benthecarman requested a review from Tibo-lg December 13, 2023 01:08
@benthecarman benthecarman force-pushed the sign-psbt branch 2 times, most recently from 5f98784 to a1ec1c5 Compare December 13, 2023 03:32
bonomat
bonomat reviewed Dec 13, 2023
View reviewed changes
Copy link
Collaborator

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

Two minor comments.

bitcoin-rpc-provider/src/lib.rs Outdated Show resolved Hide resolved
bitcoin-rpc-provider/src/lib.rs Outdated Show resolved Hide resolved
@benthecarman benthecarman force-pushed the sign-psbt branch 2 times, most recently from 5b34da2 to 07a4018 Compare December 18, 2023 23:15
@benthecarman
Copy link
Contributor Author

Fixed

@benthecarman benthecarman requested a review from Tibo-lg December 19, 2023 19:27
Tibo-lg
Tibo-lg requested changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Some small stuffs left

dlc-manager/src/contract_updater.rs Outdated Show resolved Hide resolved
dlc-manager/src/channel_updater.rs Outdated Show resolved Hide resolved
dlc-manager/src/contract_updater.rs Outdated Show resolved Hide resolved
dlc-manager/src/manager.rs Outdated Show resolved Hide resolved
dlc-manager/src/manager.rs Outdated Show resolved Hide resolved
@benthecarman
Change Signer::sign_tx_input to Signer::sign_psbt_input
9b4b450 
With the previous API, signing taproot inputs comes with a lot of
problems. Taproot signatures commit to the previous txo of all the
inputs so to be able to construct a signature, you need to have all of
them available. The previous API would require the user to look up the
outputs themself which does not really work for mobile clients and it is
a bad practice. This changes it to a PSBT so we can give all the
necessary info to the user directly.
Tibo-lg
Tibo-lg approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@Tibo-lg Tibo-lg merged commit f303972 into p2pderivatives:master Dec 21, 2023
69 checks passed
@benthecarman benthecarman deleted the sign-psbt branch December 21, 2023 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonomat bonomat bonomat left review comments

@Tibo-lg Tibo-lg Tibo-lg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@benthecarman @Tibo-lg @bennyhodl @luckysori @bonomat