Skip to content

BIP341 test vectors#695

Merged
RCasatta merged 2 commits intorust-bitcoin:masterfrom
sanket1729:taptree_tests
Dec 23, 2021
Merged

BIP341 test vectors#695
RCasatta merged 2 commits intorust-bitcoin:masterfrom
sanket1729:taptree_tests

Conversation

@sanket1729
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 commented Nov 12, 2021 •
edited
Loading

Add tests for taproot.

  • Also fixes one bug in Add taproot data structures #677, namely, I was returning LeafVersion::default() instead of given version
  • ~ Fixes a bug in P2TR address from untweaked key #691 about taking secp context as a reference instead of consuming it. This should have not passed my review, but this is easy to miss. ~
  • Makes the display on taproot hashes forward instead of the reverse (because the BIP prints in a forward way, I think we should too and it is more natural. )

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 12, 2021

I am not sure but it seems we have another bug to fix: if we have just a single script spending leaf, it TaprootSpendInfo::with_huffman_tree will create a "merkle tree root" with improper tweaked hash (leaf instead of root)

@dr-orlovsky dr-orlovsky added the API break This PR requires a version bump for the next release label Nov 12, 2021
@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Nov 12, 2021
@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Nov 12, 2021

merkle tree root with improper tweaked hash (leaf instead of root)

I think this is the expected behavior. This is the reason why TaprootMerkleProof(Vecsha256::Hash), because it contains both leafhash and branchhash

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 12, 2021

Yep, I just checked the spec and it is the way python script in BIP-341 will also compute

@sanket1729 sanket1729 changed the title Taptree tests BIP341 test vectors Nov 12, 2021
@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 12, 2021 •
edited
Loading

Sorry, accedently pressed wrong button

Related: #696

@dr-orlovsky dr-orlovsky reopened this Nov 12, 2021
@dr-orlovsky dr-orlovsky mentioned this pull request Nov 12, 2021
Closed
20 tasks
dr-orlovsky
dr-orlovsky previously approved these changes Nov 15, 2021
View reviewed changes
Copy link
Copy Markdown
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 a47df1b30b7bae1dbf253fa6b7c5ef5c8559cbe3

@sanket1729 sanket1729 mentioned this pull request Nov 17, 2021
Merged
@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Nov 19, 2021

Reabsed after #703

RCasatta
RCasatta reviewed Nov 24, 2021
View reviewed changes
src/util/taproot.rs Outdated
Copy link
Copy Markdown
Collaborator

@RCasatta RCasatta Nov 24, 2021

Choose a reason for hiding this comment

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

did you consider to use the json file directly and use include_str! here?

Copy link
Copy Markdown
Member Author

@sanket1729 sanket1729 Nov 24, 2021

Choose a reason for hiding this comment

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

Yes, I did consider. However, I was not sure about conventions about where to put test artifacts like the JSON file in the current directory. Any suggestions are appreciated.

Copy link
Copy Markdown
Member Author

@sanket1729 sanket1729 Nov 24, 2021

Choose a reason for hiding this comment

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

I like your suggestion, just was not sure about the rust idiomatic way to do things here.

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 24, 2021

needs rebase again :(

@sanket1729 sanket1729 force-pushed the taptree_tests branch 2 times, most recently from fc1507f to 1fb712a Compare November 24, 2021 19:21
@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Nov 24, 2021

Rebased

dr-orlovsky
dr-orlovsky previously approved these changes Nov 24, 2021
View reviewed changes
Copy link
Copy Markdown
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 1fb712a51bd22b58fb4a8244676d692d9be31645

src/util/taproot.rs Outdated
Comment on lines 1149 to 1150
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky Nov 24, 2021

Choose a reason for hiding this comment

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

This as_ref()s are the reason I think that the switch from Option<TapBranchHash> to Option<&TapBranchHash> was a bad idea.

I've fixed this in #696 anyway

Copy link
Copy Markdown
Member Author

@sanket1729 sanket1729 Nov 24, 2021

Choose a reason for hiding this comment

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

Agreed, I am just confused about the order in which things are changed in master. But agree that the final result should be Option<TapBranchHash>

@dr-orlovsky dr-orlovsky mentioned this pull request Nov 24, 2021
Merged
@Kixunil Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 2, 2021
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Dec 3, 2021 •
edited
Loading

Hey @sanket1729, these tests can be put in the integration tests directory (rust-bitcoin/tests). I did so and also put the json in a separate file. I pushed a branch to my tree with the same name as this PR branch if you would like to take a look. Tested with contrib/test.sh.

https://github.com/tcharding/rust-bitcoin/tree/taptree_tests

@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Dec 3, 2021

I was thinking about this, but these conceptually are not integration tests. I could not find any clear guidelines on where to put large tests on rust IRC or discord or reddit.

In any case, I think what you have done is a better way than pasting the large json file here.

@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Dec 3, 2021

How about creating a test_data directory at root and placing all test artifacts there? I think @RCasatta mentioned this previously. I like this better than treating this as an integration test

@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Dec 12, 2021

Added a test_data directory at the project root

@RCasatta
Copy link
Copy Markdown
Collaborator

RCasatta commented Dec 16, 2021

ACK 7aacc37

I checked the inlcuded json file matches https://raw.githubusercontent.com/bitcoin/bips/master/bip-0341/wallet-test-vectors.json

apoelstra
apoelstra approved these changes Dec 22, 2021
View reviewed changes
Copy link
Copy Markdown
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 7aacc37

I did not check the test vector matches the BIP :) but I did read the code. Thanks for creating the test_data directory!

@RCasatta RCasatta merged commit 6fa8a82 into rust-bitcoin:master Dec 23, 2021
@sanket1729 sanket1729 mentioned this pull request Dec 24, 2021
Merged
RCasatta added a commit that referenced this pull request Jan 3, 2022
@RCasatta
Merge #705: Tapsighash test vectors
4fa477c 
92ee5a7 Test BIP341 sighash code (sanket1729)

Pull request description:

  Based on #695 . Adds the remaining test vectors to test sighash paths. Will rebase after #695

ACKs for top commit:
  apoelstra:
    ACK 92ee5a7
  RCasatta:
    ACK 92ee5a7

Tree-SHA512: 4bd66632dd9ffeab5a70f44763d87ddd1fccffe22df66385b79835f9033408a67e0b157e855ea2394797ac2572cc08db1d66a5a71bfb58e2adbcb9f2ff7d1f0b
@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

@apoelstra apoelstra apoelstra approved these changes

@RCasatta RCasatta RCasatta approved these changes

+1 more reviewer

@dr-orlovsky dr-orlovsky dr-orlovsky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

API break This PR requires a version bump for the next release one ack PRs that have one ACK, so one more can progress them

Projects

None yet

Milestone

0.28.0

Development

Successfully merging this pull request may close these issues.

6 participants

@sanket1729 @dr-orlovsky @tcharding @RCasatta @apoelstra @Kixunil