8000 Tagged hashes for taproot by stevenroose · Pull Request #259 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Tagged hashes for taproot #259

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
Merged
merged 1 commit into from
Dec 7, 2020
Merged

Tagged hashes for taproot #259

merged 1 commit into from
Dec 7, 2020

Conversation

stevenroose
Copy link
Collaborator
@stevenroose stevenroose commented May 2, 2019 •
edited
Loading

Currently blocked on rust-bitcoin/bitcoin_hashes#93.

@dr-orlovsky
Copy link
Collaborator

Yes, really need that option for implementing both single-use seals and RGB assets: https://github.com/rgb-org/spec/blob/v1.0/01-OpenSeals.md#pay-to-contract

@stevenroose stevenroose mentioned this pull request Sep 5, 2019
Merged
@stevenroose stevenroose changed the title WIP: [experiment] Tagged hashes for taproot [BLOCKED] Tagged hashes for taproot Sep 5, 2019
@stevenroose
Copy link
Collaborator Author

The implementation works now, but is still blocked by the upstream hashes PR. Very interesting, though!

@codecov-io
Copy link
codecov-io commented Sep 5, 2019 •
edited
Loading

Codecov Report

Merging #259 (77ff6b7) into master (f4e26ca) will decrease coverage by 4.36%.
The diff coverage is 70.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
- Coverage   86.19%   81.82%   -4.37%     
==========================================
  Files          39       39              
  Lines        7394     7026     -368     
==========================================
- Hits         6373     5749     -624     
- Misses       1021     1277     +256
Impacted Files Coverage Δ
src/util/mod.rs 0.00% <ø> (ø)
src/util/taproot.rs 70.58% <70.58%> (ø)
src/network/message_filter.rs 0.00% <0.00%> (-100.00%) ⬇️
src/network/message_blockdata.rs 36.36% <0.00%> (-52.87%) ⬇️
src/network/message_network.rs 41.17% <0.00%> (-33.83%) ⬇️
src/network/message.rs 61.88% <0.00%> (-32.90%) ⬇️
src/network/address.rs 80.00% <0.00%> (-18.83%) ⬇️
src/consensus/encode.rs 76.77% <0.00%> (-12.45%) ⬇️
src/util/base58.rs 80.68% <0.00%> (-7.89%) ⬇️
src/blockdata/block.rs 73.82% <0.00%> (-7.88%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee192eb...a56712b. Read the comment docs.

@stevenroose stevenroose force-pushed the taproot branch 2 times, most recently from 7f72d53 to 633ce66 Compare September 9, 2019 09:47
@elichai
Copy link
Member
elichai commented Sep 9, 2019

This is where I wish const fn's were more mature.
curious if we could use proc_macros to do this is compile time. (not sure what's the MRSV for proc-macro stuff)

@elichai
Copy link
Member
elichai commented Jan 21, 2020

Now that taproot is getting closer and we're not any closer to be able to do something like that in const fn I wonder if I can do it as a function like proc macro. I think it's possible just not sure the in those (what I mean is compile time doing the hash calcs for the taggs)

@stevenroose
Copy link
Collaborator Author

This should all work, though. The only issue I was trying to solve last week is the serde serializer and deserializer of the hex value. I can't get it to work over the generic type. I'm getting some errors.

I just pushed my update: rust-bitcoin/bitcoin_hashes#42 Feel free to take a look.

dr-orlovsky
dr-orlovsky reviewed Sep 3, 2020
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.

Pls see my comments: looks like this PR should belong to bitcoin_hashes and not here. Also I ask for exporting the macro, which I use in other project.

src/util/taproot.rs

use hashes::{sha256, sha256t};

/// The SHA-256 midstate value for the TapLeaf hash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I'm starting thinking that the whole taproot support in terms of tagged hashes should be part of bitcoin_hashes alike other bitcoin-specific Hash160 etc. While this things are taproot-related, they have no other functionality then pure hash computation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed my mind after the second thought: #259 (comment)

dr-orlovsky
dr-orlovsky reviewed Sep 3, 2020
View reviewed changes
dr-orlovsky
dr-orlovsky reviewed Sep 3, 2020
View reviewed changes
@dr-orlovsky dr-orlovsky mentioned this pull request Sep 3, 2020
Closed
@dr-orlovsky
Copy link
Collaborator

Created my version on top of yours (stevenroose#1); b/c it required rebase onto master now it looks like it worth doing it as a separate PR to master...

@stevenroose stevenroose force-pushed the taproot branch 2 times, most recently from a2ee044 to e7b7574 Compare October 7, 2020 21:09
@stevenroose
Copy link
Collaborator Author

@dr-orlovsky I redid this branch a bit with some of your suggestions and also using the hash_newtype macro. About upstreaming the tagged hash creation, we could have a sha256t_hash_newtype macro indeed that kind of supports this API. Let's see when the taproot bip finalizes how the tagged hashes are then. We can always move it over later, for now I don't think we should expose the macro from this crate.

@dr-orlovsky
Copy link
Collaborator

Taproot got merged: bitcoin/bitcoin#19953

Also bitcoin_hashes with sha256t_hash_newtype are out

@jrawsthorne jrawsthorne mentioned this pull request Oct 20, 2020
Closed
20 tasks
@stevenroose stevenroose changed the title [BLOCKED] Tagged hashes for taproot Tagged hashes for taproot Oct 25, 2020
@stevenroose
Copy link
Collaborator Author
stevenroose commented Oct 25, 2020 •
edited
Loading

So I rebased this PR and it's now mergable using the new bitcoin_hashes version. However, we might want to discuss directly putting these hashes in bitcoin_hashes instead. They are hashes after all.. Not sure if there's merit of having them here separately.

I can be convinced of either option, probably.

EDIT: Ok I think we should probably have it here in the bitcoin crate.

@dr-orlovsky
Copy link
Collaborator
dr-orlovsky commented Oct 26, 2020 628C

For me it seems that everything related to transaction- or block-specific data types must reside in bitcoin; while bitcoin_hashes are for non-specific generic types. By this logic it is clear why sha256d & sha256t are in bitcoin_hashes and Txid and PubkeyHash are in bitcoin crate. Thus, specific hash types, used in Script (tapscript) and scriptPubkey IMO should go to bitcoin crate alongside PubkeyHash etc

dr-orlovsky
dr-orlovsky previously approved these changes Oct 26, 2020
View reviewed changes
src/util/taproot.rs

use hashes::{sha256, sha256t};

/// The SHA-256 midstate value for the TapLeaf hash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed my mind after the second thought: #259 (comment)

@stevenroose
Copy link
Collaborator Author

Rebased.

apoelstra
apoelstra previously approved these changes Nov 25, 2020
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.

utack, but we need to fix the macros for rust 1.29 in bitcoin_hashes.

apoelstra
apoelstra approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reaso E1D2 n will be displayed to describe this comment to others. Learn more.

ack a56712b

elichai
elichai approved these changes Dec 7, 2020
View reviewed changes
Copy link
Member
@elichai elichai left a comment

Choose a reason for hiding this comment

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

I'd really love to see this using const fn but for now this is what we have

ACK

@stevenroose stevenroose merged commit b4c8e12 into rust-bitcoin:master Dec 7, 2020
yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024
@sanket1729
Merge pull request rust-bitcoin#259 from apoelstra/2021-07--600
5193fa4 
bump version to 6.0.0
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

@elichai elichai elichai approved these changes

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

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
@stevenroose @dr-orlovsky @codecov-io @elichai @apoelstra
0