Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Upgrade all crates to the 2024 edition and bump MSRV to 1.85 #913

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Upgrade all crates to the 2024 edition and bump MSRV to 1.85 #913

wants to merge 33 commits into from

Conversation

baloo
Copy link
Member

@baloo baloo commented Feb 23, 2025 •
edited
Loading

This was referenced Feb 23, 2025
Closed
Closed
Closed
Closed
@baloo baloo force-pushed the baloo/edition-2024 branch 3 times, most recently from 436380a to e6cdcfb Compare February 23, 2025 22:15
baloo
baloo commented Feb 23, 2025
View reviewed changes
Copy link
Member Author

@baloo baloo left a comment

Choose a reason for hiding this comment

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

Opinion about RandomizedSignerMut signature?

lms/src/ots/private.rs
@@ -107,7 +107,7 @@ impl<Mode: LmsOtsMode> RandomizedSignerMut<Signature<Mode>> for SigningKey<Mode>

// Generate the message randomizer C
let mut c = <Output<Mode::Hasher>>::default();
rng.fill_bytes(&mut c);
rng.try_fill_bytes(&mut c).map_err(|_| Error::new())?;
Copy link
Member Author

Choose a reason for hiding this comment

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

We kind of eat the error here. This might be a bad idea.

I wish we could do Error::from_source instead, but that would require the RandomizedSignerMut to change its signature to something like:

fn try_sign_with_rng<E: core:error::Error, R: TryCryptoRng<Error = E>>(
        &mut self,
        rng: &mut R,
        msg: &[u8],
    ) -> Result<Signature<Mode>, Error> {
``
Not sure.

Copy link
Member

Choose a reason for hiding this comment

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

We can potentially consider something like that

ecdsa/src/signing.rs
prehash: &[u8],
) -> Result<Signature<C>> {
let z = bits2field::<C>(prehash)?;
let mut ad = FieldBytes::<C>::default();
rng.fill_bytes(&mut ad);
rng.try_fill_bytes(&mut ad).map_err(|_| Error::new())?;
Copy link
Member Author

Choose a reason for hiding this comment

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

similar situation here.

@baloo baloo force-pushed the baloo/edition-2024 branch 2 times, most recently from 2e419ca to 733b105 Compare February 24, 2025 06:37
@tarcieri
Copy link
Member

@baloo can you rebase?

@baloo baloo force-pushed the baloo/edition-2024 branch from 309e627 to 9d6fe57 Compare February 24, 2025 16:03
@baloo baloo mentioned this pull request Feb 24, 2025
Closed
@baloo baloo force-pushed the baloo/edition-2024 branch from 9d6fe57 to 9a7625f Compare February 24, 2025 16:06
@baloo baloo force-pushed the baloo/edition-2024 branch from 9a7625f to 6ad65c7 Compare February 24, 2025 16:43
@erik-3milabs
Copy link

Would it be possible to include new elliptic-curve and ecdsa pre-releases with this PR? 😄

Currently, ecdsa@0.17.0-pre.9 and crypto-bigint@0.6.0 do not work together, because:

  • elliptic-curve@0.14.0-rc.1 (on which ecdsa depends) pins serdect=0.3.0-rc.0, while
  • crypto-bigint@0.6.0 requires ^0.3.

elliptic-curve has already been updated to use 0.3.0; it just needs a new release for these packages to work together again.

I might be mistaken, but all I think needs to happen is:

  • release elliptic-curve@0.14.0-rc.2
  • have ecdsa depend on this new release, and
  • release ecdsa@0.17.0-pre.9.

If this is not the right place and/or time, I'm happy to create a separate issue for it :)

@tarcieri
Copy link
Member

@erik-3milabs we generally do releases in separate PRs to keep ones like this focused

@baloo
Copy link
Member Author

baloo commented Feb 24, 2025 •
edited
Loading

Just to mitigate expectations, I don't know if we'll be able to release elliptic-curve until the situation of group and ff support of rand_core 0.9 is sorted out. (RustCrypto/traits#1751 (comment))

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@tarcieri tarcieri tarcieri 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.
3 participants
@baloo @tarcieri @erik-3milabs