Tagged hashes for taproot by stevenroose · Pull Request #259 · rust-bitcoin/rust-bitcoin

stevenroose · 2019-05-02T14:23:14Z

Currently blocked on rust-bitcoin/bitcoin_hashes#93.

dr-orlovsky · 2019-08-19T23:07:16Z

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 · 2019-09-05T14:54:49Z

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

codecov-io · 2019-09-05T15:10:42Z

Codecov Report

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

@@            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.

elichai · 2019-09-09T10:01:00Z

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 · 2020-01-21T11:13:09Z

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 · 2020-01-21T21:37:35Z

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

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.

Cargo.toml

src/util/taproot.rs

dr-orlovsky · 2020-09-03T23:21:48Z

src/util/taproot.rs

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.

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

src/util/mod.rs

src/util/taproot.rs

dr-orlovsky · 2020-09-04T00:09:25Z

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 · 2020-10-07T21:12:13Z

@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 · 2020-10-16T17:36:12Z

Taproot got merged: bitcoin/bitcoin#19953

Also bitcoin_hashes with sha256t_hash_newtype are out

stevenroose · 2020-10-25T18:46:10Z

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 · 2020-10-26T10:08:38Z

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 · 2020-10-26T10:09:46Z

src/util/taproot.rs

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

stevenroose · 2020-11-23T21:26:35Z

Rebased.

apoelstra

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

apoelstra

ack a56712b

elichai

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

ACK

bump version to 6.0.0

* segments cache struct and evict method generalized * save segments to disk made generic inside the segments cache struct * save_dirty_segments logic refactorized * tbh I dont know waht I refactored here * removed io module inside storage/disk * you are right rabbit * store in segment moved to SegmentCache * sentinel headers creation moved to the Persistable trait and encapsulated there * unified sentinel item behaviour - no longer compiles bcs the tip_high calculation * renames * new struct to manage the hashmap of segments and tip height moved - doesnt compile yet, wip * get_headers generalized in the SegmentsCache struct - wip, not compiles * store headers logic moved to the SegmentsCache and introduced method to better handle tip_height and storage index - wip, no compiles * store_headers_impl using precomputed_hashes didn't provide real benefics with the current usage - wip, no compiles * removed unused StorageManager::get_headers_batch methos - wip, no compiles * removed warnings * ensure segment loaded moved inside the SegmentsCache with a logic change, we ask for a segment and if it doesn't exist in memory we load it from disk - wip, no compiles * const MAX_ACTIVE_SEGMENTS encapsulated - wip, no compiles * removed one commit as it is fixed * created a SegmentsCache::store_headers_at_height - wip, no compiles * removed inconsistency when loading segments metadata * removed to methods unused bcs now that behaviour is encapsulated * building SegmentCache yip_height when creating the struct * removed unused function * some refactor and removed the notification enum and related behaviour - wip, no compiles * disk storage manager worker no longer needs cloned headers to work * renamed segments stoage fields * removed new unused function * evict logic removed * save dirty segments logic moved into SegmentsCache * clippy warnings fixed * save dirty is now an instance method for the DiskStorageManager * when persisting segments we try to create the parent dirs to ensure they exist * improved logic to ensure different clear behaviour for SegmentsCache * correctly rebuilding the block reverse index * fixed bug found by test test_checkpoint_storage_indexing * fixed bug updating tip_height in SegmentCache thanks spotted by test test_filter_header_segments * fixed bug, we stop persisting segments after we find the first sentinel, to correctly initialize valid_count - bug spotted by test test_filter_header_persistence * refactor: HEADER_PER_SEGMENT encapsulated inside segment and renamed to ITEMS_PER_SEGMENT - wip, no compiles * block index rebuild logic moved into SegmentCache<BlockHeader> and load_segment_metadata renamed in flavor of a better name for its current behaviour being the block index contructor * added some cool inlines * fixed test that was creating a centinel filter header at height 0 making the segment not persist entirely * renamed header reference to item in segments.rs so its clear that the new struct can work with any struct * clippy warning fixed * logging when storing multiple items simplified * removed sentinel headers from the segments logic * unit tests for the segment and segment_cache structs after the refactor (rust-bitcoin#259) * removed unused methods after rebase * renamed and removed old documentation for store_headers_impl * refactorized and adjusted docs for conversion methods between stoage index, height and offset * removed old comments and forcing to give sync_base_height when creating the SegmentCache * quick fix to load sync_base_height if persisted before * review comments addressed * read block index operation made async * using atomic write where we write to disk

stevenroose force-pushed the taproot branch from 9571f1d to 77ff6b7 Compare September 5, 2019 14:48

stevenroose mentioned this pull request Sep 5, 2019

Tagged SHA256 hashes rust-bitcoin/bitcoin_hashes#42

Merged

stevenroose changed the title ~~WIP: [experiment] Tagged hashes for taproot~~ [BLOCKED] Tagged hashes for taproot Sep 5, 2019

stevenroose force-pushed the taproot branch 2 times, most recently from 7f72d53 to 633ce66 Compare September 9, 2019 09:47

dr-orlovsky reviewed Sep 3, 2020

View reviewed changes

src/util/mod.rs Outdated Show resolved Hide resolved

dr-orlovsky reviewed Sep 3, 2020

View reviewed changes

src/util/taproot.rs Outdated Show resolved Hide resolved

dr-orlovsky mentioned this pull request Sep 3, 2020

Improving tagged hashes implementation in taproot mod stevenroose/rust-bitcoin#1

Closed

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

stevenroose force-pushed the taproot branch from e7b7574 to 89527cc Compare October 7, 2020 21:41

jrawsthorne mentioned this pull request Oct 20, 2020

Tracking issue for Taproot #503

Closed

20 tasks

stevenroose force-pushed the taproot branch from 89527cc to 40c15c4 Compare October 25, 2020 18:44

stevenroose changed the title ~~[BLOCKED] Tagged hashes for taproot~~ Tagged hashes for taproot Oct 25, 2020

dr-orlovsky previously approved these changes Oct 26, 2020

View reviewed changes

src/util/taproot.rs Outdated

Copy link
Copy Markdown

Collaborator

dr-orlovsky Oct 26, 2020

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 dismissed dr-orlovsky’s stale review via a0fb4f1 November 23, 2020 21:24

stevenroose force-pushed the taproot branch from 40c15c4 to a0fb4f1 Compare November 23, 2020 21:24

apoelstra previously approved these changes Nov 25, 2020

View reviewed changes

stevenroose dismissed apoelstra’s stale review via a56712b November 30, 2020 20:07

stevenroose force-pushed the taproot branch from a0fb4f1 to a56712b Compare November 30, 2020 20:07

Create tagged taproot hashes

a56712b

apoelstra approved these changes Dec 3, 2020

View reviewed changes

elichai approved these changes Dec 7, 2020

View reviewed changes

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

Merge pull request rust-bitcoin#259 from apoelstra/2021-07--600

5193fa4

bump version to 6.0.0

Conversation

stevenroose commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Aug 19, 2019

Uh oh!

stevenroose commented Sep 5, 2019

Uh oh!

codecov-io commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

elichai commented Sep 9, 2019

Uh oh!

elichai commented Jan 21, 2020

Uh oh!

stevenroose commented Jan 21, 2020

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky commented Sep 4, 2020

Uh oh!

stevenroose commented Oct 7, 2020

Uh oh!

dr-orlovsky commented Oct 16, 2020

Uh oh!

stevenroose commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Oct 26, 2020

Uh oh!

dr-orlovsky Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Nov 23, 2020

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

elichai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stevenroose commented May 2, 2019 •

edited

Loading

codecov-io commented Sep 5, 2019 •

edited

Loading

stevenroose commented Oct 25, 2020 •

edited

Loading