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.

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

Enable bitcoin_hashes v0.14.0 #76

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tcharding
Copy link
Member

Hardware devices like the smallest binary possible, currently if one builds with latest rust-bitcoin and rust-bip39 they get two versions of bitcoin_hashes in the dependency graph because we don't support the latest bitcoin_hashes.

We use a range dependency for bitcoin_hashes. We can support the latest version with no code changes. However using the latest version causes the MSRV to increase.

Add the latest version of bitcoin_hashes to the range dependency.

@tcharding
Copy link
Member Author

cc @afilini

@afilini
Copy link

afilini commented Jul 31, 2024

ACK, that would be helpful for me, all the other libs like rust-bitcoin and bdk have already upgraded to hashes 0.14

Copy link

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

This would be nice for me even just for cleanness sake.

Cargo.toml Outdated
unicode-normalization = { version = "=0.1.22", default-features = false, optional = true }

[dev-dependencies]
# Enabling the "rand" feature by default to run the benches
bip39 = { path = ".", features = ["rand"] }
bitcoin_hashes = ">=0.12,<0.14" # enable default features for test
bitcoin_hashes = ">=0.12,<=0.14" # enable default features for test
Copy link

Choose a reason for hiding this comment

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

This must be >= 0.12, <0.15, otherwise 0.14.1 will be unusable even though it'd be compatible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, TIL. Will fix, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to go right ahead and 'trust don't verify'.

Copy link

Choose a reason for hiding this comment

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

The verification is easy, just evaluate 0 < 1 expression in your head. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I thought we were talking about 0.14 working for 0.14.0 but not for 0.14.1.

Copy link

Choose a reason for hiding this comment

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

0.14 implies 0.14.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I have no idea what you were talking about in your original comment then. <= 14 is equivalent to < 15

Copy link

Choose a reason for hiding this comment

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

It's not. 0.14 == 0.14.0
0.14.1 > 0.14.0
0.14.1 < 0.15.0
And the above implies 0.14.1 > 0.14 which is a negation of 0.14.1 <= 0.14
So for <= 14 to be equivalent to < 15 we require all x to satisfy x <= 0.14 <-> x <0.15 and we have a counter example for x = 0.14.1. QED

Copy link
Member Author

Choose a reason for hiding this comment

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

I get it now. Seems the confusion started when I wrote "I'm going to go right ahead and 'trust don't verify'." and I have no clue what I was thinking then. Your original review comment is actually perfectly clean.

Just call me retarded :)

Cargo.toml Outdated
@@ -49,13 +49,13 @@ serde = { version = "1.0", default-features = false, features = [ "alloc" ], opt
zeroize = { version = "1.5", features = ["zeroize_derive"], optional = true }

# Unexported dependnecies
bitcoin_hashes = { version = ">=0.12, <=0.13", default-features = false }
bitcoin_hashes = { version = ">=0.12, <=0.14", default-features = false }
Copy link

Choose a reason for hiding this comment

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

This also needs to be <0.15, it was already broken.

Hardware devices like the smallest binary possible, currently if one
builds with latest `rust-bitcoin` and `rust-bip39` they get two versions
of `bitcoin_hashes` in the dependency graph because we don't support the
latest `bitcoin_hashes`.

We use a range dependency for `bitcoin_hashes`. We can support the
latest version with no code changes. However using the latest version
causes the MSRV to increase.

Add the latest version of `bitcoin_hashes` to the range dependency.
Copy link

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

ACK 2b47d40

@tcharding
Copy link
Member Author

Oh this PR is no good anyway, bitcoin_hashes v14 has an incompatible MSRV.

@tcharding
Copy link
Member Author

Raised #79 to discuss the MSRV bump. Converting to draft until that resolves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants