8000 Remove usage of ThirtyTwoByteHash by tcharding · Pull Request #1998 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove usage of ThirtyTwoByteHash #1998

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
Aug 23, 2023
Merged

Remove usage of ThirtyTwoByteHash #1998

merged 1 commit into from
Aug 23, 2023

Conversation

tcharding
Copy link
Member

The ThirtyTwoByteHash trait is defined in secp256k1 and used in hashes as well as bitcoin. This means that we must use the same version of hashes in both bitcoin and secp256k1. This makes doing release difficult.

Remove usage of ThirtyTwoByteHash and use Message::from_slice. Include TODO above each usage because as soon as we release the new version of secp we can use the new Message::from_digest.

This is step backwards as far as type safety goes and it makes the code more ugly as well because it uses expect but thems the breaks.

For context see #1985

@apoelstra
Copy link
Member

@tcharding I don't think we can do the removal of impl_thirty_two_byte_hash :(. This is a (bad) breaking API change that we definitely don't want to slip into this release.

@tcharding tcharding force-pushed the 08-16-thirty-two-byte-hash branch from 026faf2 to ac2f092 Compare August 17, 2023 00:22
@tcharding
Copy link
Member Author

Oh interesting, even the commit log never mentions removing the trait - fail. For anyone else following, by the time we release bitcoin this macro is useful because all the versions of hashes match up again. Its just not usable to us in rust-bitcoin. My bad.

@tcharding tcharding force-pushed the 08-16-thirty-two-byte-hash branch 2 times, most recently from de0b59f to b3119fc Compare August 17, 2023 01:11
@tcharding tcharding mentioned this pull request Aug 17, 2023
Merged
@tcharding
Copy link
Member Author
tcharding commented Aug 21, 2023 •
edited
Loading

This one should be right to go @apoelstra, only changes our usage now, does not touch the trait. My proposed plan is

  1. merge this
  2. release hashes
  3. release scep
  4. update hashes/secp deps in bitcoin
  5. fix all the todos introduced in this PR
  6. release bitcoin
  7. profit

@tcharding tcharding added the P-high High priority label Aug 21, 2023
apoelstra
apoelstra previously approved these changes Aug 21, 2023
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 b3119fc419abcfc4131a9c38bbbbe92a4c8c8e12

@apoelstra
Copy link
Member

But needs rebase.

@tcharding tcharding force-pushed the 08-16-thirty-two-byte-hash branch from b3119fc to da2cf17 Compare August 22, 2023 01:41
@tcharding
Copy link
Member Author

Rebase only, no other changes.

@apoelstra
Copy link
Member

The rebased version seems to leave a bunch of Message::from calls in place. Is that intentional?

@tcharding
Copy link
Member Author

Bother, I must have botched up the rebase. Will fix, thanks.

@tcharding
Remove usage of ThirtyTwoByteHash
d953352 
The `ThirtyTwoByteHash` trait is defined in `secp256k1` and used in
`hashes` as well as `bitcoin`. This means that we must use the same
version of `hashes` in both `bitcoin` and `secp256k1`. This makes doing
release difficult.

Remove usage of `ThirtyTwoByteHash` and use `Message::from_slice`.
Include TODO above each usage because as soon as we release the new
version of secp we can use the new `Message::from_digest`.

This is step backwards as far as type safety goes and it makes the code
more ugly as well because it uses `expect` but thems the breaks.
@tcharding tcharding force-pushed the 08-16-thirty-two-byte-hash branch from da2cf17 to d953352 Compare August 23, 2023 02:22
@tcharding
Copy link
Member Author

git grep 'Message::from\(' now returns nothing.

sanket1729
sanket1729 approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utACK d953352

apoelstra
apoelstra approved these changes Aug 23, 2023
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 d953352

@apoelstra apoelstra merged commit 1991b7a into rust-bitcoin:master Aug 23, 2023
@tcharding tcharding deleted the 08-16-thirty-two-byte-hash branch August 24, 2023 02:12
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

@sanket1729 sanket1729 sanket1729 approved these changes

Assignees
No one assigned
Labels
P-high High priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tcharding @apoelstra @sanket1729
0