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

refactor: simplify k generation following RFC 6979 #570

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

Conversation

tcoratger
Copy link
Collaborator

Summary

This pull request introduces a direct integration of the rfc6979 crate to generate the scalar k according to RFC 6979. The integration is aimed at streamlining the logic within our codebase, ensuring simplicity, and reducing maintenance overheads by relying on established libraries from RustCrypto.

Changes Made

  • Integrated the rfc6979 crate to handle the generation of scalar k.
  • Refactored relevant sections of the codebase to utilize the new integration.
  • Ensured consistency and compliance with RFC 6979 standards.

Motivation

  • Simplifying Codebase: By directly leveraging the rfc6979 crate, we simplify the logic related to k generation, making it more concise and maintainable.
  • Reduced Maintenance Burden: Depending on established libraries from RustCrypto reduces the need for maintaining custom solutions in our repository, ensuring compatibility and reliability.

rfc6979 = { version = "0.4.0", default-features = false }
sha2 = { version = "0.10.6", default-features = false }
zeroize = { version = "1.6.0", default-features = false }
rfc6979 = { version = "0.5.0-pre.3", default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

Do we have to use prerelease versions? I don't think it's the best idea to depend on prereleases. If we depend on upcoming features then we can wait for a release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes in order to simplify the code of generate_k and avoid doing some manual stuffs, we need to depend on this up to date version. But rfc6979 is a crate of RustCrypto which is the official Rust crypto library. It is audited and constantly checked and updated. The changes are minor between versions so I don't think there is any risk in doing this. Precisely, the version previously used was revised and corrected later (if I'm not mistaken it dated 3 years ago, which isn't necessarily great either).

Copy link
Owner

Choose a reason for hiding this comment

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

rfc6979 v0.4.0 was out 2023-03-01 tho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but I checked when I did the manipulations and what is done here was impossible with this version, meaning that there was a fix in the middle. Feel free to wait for an update is needed but I think this is completely fine to do this with beta version.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I understand that this is a new feature not available from v0.4.0. That said, I prefer to wait for the release, as we're actually not getting much out of it beyond "our code now looks nicer", which is a nice-to-have that we don't have to rush for IMO.

@xJonathanLEI xJonathanLEI added the upstream Waiting for an upstream fix/release label Mar 27, 2024
@xJonathanLEI
Copy link
Owner

Rebased to keep branch from becoming staled.

@xJonathanLEI xJonathanLEI changed the title Simplify k generation following RFC 6979 refactor: simplify k generation following RFC 6979 Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream Waiting for an upstream fix/release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants