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

Add support for taproot psbt fields BIP 371 #681

Merged
merged 4 commits into from Dec 30, 2021
Merged

Add support for taproot psbt fields BIP 371 #681

merged 4 commits into from Dec 30, 2021

Conversation

sanket1729
Copy link
Member

@sanket1729 sanket1729 commented Oct 27, 2021 •
edited

Built on top of #677 . Will rebase and mark ready for review after #677 is merged.

@RCasatta
Copy link
Collaborator

Built on top of #666. Will rebase and mark ready for review after #666 is merged.

I think you mean #677

@dr-orlovsky dr-orlovsky added this to In process in Taproot Nov 6, 2021
@dr-orlovsky
Copy link
Collaborator

You can grab SpendingSignature type implementation from my PR #671 after which I will close it. This type IMO simplifies serialization and readibility.

@sanket1729
Copy link
Member Author

I will do so after #677 is merged. I like creating a dedicated type for (signature, sighashtype). I already have a type for it in rust-miniscript and it is indeed handy.

@dr-orlovsky dr-orlovsky mentioned this pull request Nov 6, 2021
Closed
@dr-orlovsky
Copy link
Collaborator

dr-orlovsky commented Nov 12, 2021 •
edited

Now it can be rebased and finalized

@dr-orlovsky dr-orlovsky mentioned this pull request Nov 12, 2021
Closed
20 tasks
@sanket1729
Copy link
Member Author

@dr-orlovsky, I think we should address this along with a resolution for #670. As implemented #671 is incorrect as it uses the wrong sighash type.

@sanket1729 sanket1729 marked this pull request as ready for review November 15, 2021 18:31
dr-orlovsky
dr-orlovsky previously approved these changes Nov 23, 2021
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

tACK eaf1887

Compared against spec descriptions & test vectors and confirm that they do match (actually found a bug in the spec: bitcoin/bips#1241)

src/util/psbt/map/output.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
Taproot automation moved this from In process to In review Nov 24, 2021
dr-orlovsky
dr-orlovsky previously approved these changes Nov 24, 2021
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK 0eb6730

@dr-orlovsky
Copy link
Collaborator

@Kixunil pls if you could review this Taproot release blocker as well

@Kixunil Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 2, 2021
Kixunil
Kixunil reviewed Dec 2, 2021
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Unfortunately, I'm not deeply familiar with byte-level details of Taproot, so I could only check structure of the code and don't feel confident acking. I found various potential improvements and suspicious code.

src/util/psbt/map/output.rs Show resolved Hide resolved
src/util/psbt/map/output.rs Outdated Show resolved Hide resolved
src/util/psbt/map/output.rs Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
src/util/taproot.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
@sanket1729
Copy link
Member Author

Unfortunately, I'm not deeply familiar with byte-level details of Taproot, so I could only check structure of the code and don't feel confident acking. I found various potential improvements and suspicious code.

Thanks, this is super valuable. I will address your comments shortly. As for the correctness of the taproot code, there are exact test vectors from the BIP that we are testing against which should give some confidence.

@sanket1729 sanket1729 force-pushed the taproot_psbt branch 2 times, most recently from 0e0ba8e to 4e79e44 Compare December 12, 2021 07:22
@sanket1729
Copy link
Member Author

Sorry for the delay. I was traveling and could not get to this. I have addressed all your comments.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 12, 2021

First light review looks good. Will try to do deeper later.

dr-orlovsky
dr-orlovsky previously approved these changes Dec 12, 2021
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

tACK 4e79e44

Excellent work! Tested all commits with git rebase -x using full test script.

src/util/psbt/map/output.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Outdated Show resolved Hide resolved
src/util/psbt/serialize.rs Show resolved Hide resolved
@dr-orlovsky
Copy link
Collaborator

Needs a rebase again...

@sanket1729
Copy link
Member Author

Rebased

@sanket1729 sanket1729 force-pushed the taproot_psbt branch 2 times, most recently from 8387423 to 3bbd446 Compare December 24, 2021 10:36
@sanket1729
Copy link
Member Author

Rebased again after #686

apoelstra
apoelstra previously approved these changes Dec 24, 2021
View reviewed changes
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 3bbd446

I also did not check the byte-for-byte compatibility with BIP 371. Hopefully the test vectors save us here. Also I did not check that the test vectors were correct.

@sanket1729 sanket1729 removed the one ack PRs that have one ACK, so one more can progress them label Dec 28, 2021
@sanket1729
Copy link
Member Author

sanket1729 commented Dec 28, 2021 •
edited

Pushed again to use schnorr::SchnorrSig struct from #702

apoelstra
apoelstra approved these changes Dec 28, 2021
View reviewed changes
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 7d982fa

Wow, having the sighashtype inside the signature type really makes things cleaner.

Taproot automation moved this from In review to Ready for merge Dec 30, 2021
dr-orlovsky
dr-orlovsky approved these changes Dec 30, 2021
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

re-tACK 7d982fa basing on git range-diff. The original PR before last re-base was tested commit-by-commit.

@dr-orlovsky dr-orlovsky merged commit 670e808 into rust-bitcoin:master Dec 30, 2021
Taproot automation moved this from Ready for merge to Done Dec 30, 2021
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil left review comments

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

@apoelstra apoelstra apoelstra approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@sanket1729 @RCasatta @dr-orlovsky @Kixunil @apoelstra