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

Release 2.1.0 #54

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Release 2.1.0 #54

merged 2 commits into from
Apr 3, 2024

Conversation

stevenroose
Copy link
Collaborator

I added in one change that I had pending, I can split that up if people don't like it.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Looks good to me, ACK 7416986

@stevenroose
Copy link
Collaborator Author

Gonna await another ack. @jeandudey maybe?

@jeandudey
Copy link
Contributor

@stevenroose LGTM, ACK 7416986

@benma
Copy link
Contributor

benma commented Oct 26, 2023

Please also bump the bitcoin-hashes version to align with the one published by rust-bitcoin, which is currently bitcoin-hashes=0.12.0. This way if one uses both crates, the hashes dependency is not duplicated.

@jeandudey
Copy link
Contributor

@benma Maybe three versions could be released to keep everyone happy as I need bitcoin_hashes to be 0.13 as bitcoin_hashes takes a lot of flash space due to unrolling of the implementations.

E.g.:

2.1.0 with bitcoin_hashes@0.11
2.2.0 with bitcoin_hashes@0.12
2.3.0 with bitcoin_hashes@0.13

@benma
Copy link
Contributor

benma commented Oct 26, 2023

@jeandudey can you elaborate? Which version of bitcoin_hashes does the unrolling and which doesn't? Do you have links to the relevant changes/discussions? It is relevant to me as we use the bitocoin-hashes crate in the BitBox02 firmware.

Could also go to bitcoin_hashes 0.13 and ask rust-bitcoin to also bump to 0.13.

@tcharding
Copy link
Member

If no one needs 0.11 then we can use a range dependency over 12 and 13

diff --git a/Cargo.toml b/Cargo.toml
index 2970cc5..2e57a82 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -48,13 +48,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.11.0", default-features = false }
+bitcoin_hashes = { version = ">=0.12.0, <0.13", default-features = false }
 unicode-normalization = { version = "=0.1.22", optional = true }
 
 [dev-dependencies]
 # Enabling the "rand" feature by default to run the benches
 bip39 = { path = ".", features = ["rand"] }
-bitcoin_hashes = "0.11.0" # enable default features for test
+bitcoin_hashes = "0.13" # enable default features for test
 
 
 [package.metadata.docs.rs]
diff --git a/src/pbkdf2.rs b/src/pbkdf2.rs
index e7d3375..326ca55 100644
--- a/src/pbkdf2.rs
+++ b/src/pbkdf2.rs
@@ -112,7 +112,7 @@ pub(crate) fn pbkdf2<M>(mnemonic: M, unprefixed_salt: &[u8], c: usize, res: &mut
                        prfc.input(unprefixed_salt);
                        prfc.input(&u32_to_array_be((i + 1) as u32));
 
-                       let salt = hmac::Hmac::from_engine(prfc).into_inner();
+                       let salt = hmac::Hmac::from_engine(prfc).to_byte_array();
                        xor(chunk, &salt);
                        salt
                };
@@ -120,7 +120,7 @@ pub(crate) fn pbkdf2<M>(mnemonic: M, unprefixed_salt: &[u8], c: usize, res: &mut
                for _ in 1..c {
                        let mut prfc = prf.clone();
                        prfc.input(&salt);
-                       salt = hmac::Hmac::from_engine(prfc).into_inner();
+                       salt = hmac::Hmac::from_engine(prfc).to_byte_array();
 
                        xor(chunk, &salt);

@tcharding
Copy link
Member

and ask rust-bitcoin to also bump to 0.13.

Upcoming rust-bitcoin v0.31.0 release depends on hashes v0.13. Latest rust-secp256k1 uses a range dependency as shown above.

@benma
Copy link
Contributor

benma commented Oct 27, 2023

@tcharding that seems like a great option.

+bitcoin_hashes = { version = ">=0.12.0, <0.13", default-features = false }

Shouldn't that be <0.14?

@jeandudey
Copy link
Contributor

@benma All of the versions unroll the rounds of sha256, upgrading the crate in a separate minor version is just a convenience for people that want to stick with one version of bitcoin_hashes.

The code is manually unrolled as seen here:

https://docs.rs/bitcoin_hashes/0.13.0/src/bitcoin_hashes/sha256.rs.html#340

We do also use bitcoin_hashes in Passport (specifically for sha256) so it takes a little bit of .text space.

That can be disabled by setting the small_hash feature but it's slower.

@tcharding
Copy link
Member

+bitcoin_hashes = { version = ">=0.12.0, <0.13", default-features = false }

Shouldn't that be <0.14?

My bad, my intention was <=0.13, that's what we have in secp. I've not tested with a non-existent version (0.14).

@kayabaNerve
Copy link
Contributor

I was about to submit a PR bumping to 0.13, yet there seems to be a bit of discussion above. Should I submit a PR with >=0.12, <=0.13 or was that not the best solution?

@tcharding
Copy link
Member

Range dependency works well in secp, AFAIU it should be favoured anytime it works because its more flexible for users. But that is just my understanding, I don't speak for this project.

@kayabaNerve
Copy link
Contributor

I opened #60.

@benma
Copy link
Contributor

benma commented Nov 29, 2023

In the meantime I updated my project to bitcoin_hashes 0.13, so I am fine with either 0.13 or the range to include 0.12 as well. The other rust-bitcoin crates also seem to be at 0.13.

@benma
Copy link
Contributor

benma commented Dec 11, 2023

@stevenroose wen release 😄

@stevenroose
Copy link
Collaborator Author

Waiting on #57 and #64. Then will replace this release MR with another one and wait for feedback.

@stevenroose
Copy link
Collaborator Author

Sorry for the delay. Merged the other MRs and updated the changelog. Will release when this gets merged.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 7416986

@tcharding
Copy link
Member

CI fixes in #68 if you want them.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 125abeb

@stevenroose stevenroose merged commit b100bf3 into master Apr 3, 2024
12 checks passed
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.

None yet

5 participants