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.

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

crypto-common: migrate to hybrid-array; MSRV 1.65 #1319

Merged
merged 3 commits into from
Oct 26, 2023
Merged

crypto-common: migrate to hybrid-array; MSRV 1.65 #1319

merged 3 commits into from
Oct 26, 2023

Conversation

tarcieri
Copy link
Member

Replaces generic-array with hybrid-array, which is built on a combination of typenum and const generics, providing a degree of interoperability between the two systems.

hybrid-array is designed to be a largely drop-in replacement, and the number of changes required to switch are relatively minimal aside from some idiosyncracies.

@tarcieri tarcieri requested a review from newpavlov May 30, 2023 23:35
tarcieri
tarcieri commented May 30, 2023
View reviewed changes
crypto-common/src/lib.rs Outdated Show resolved Hide resolved
tarcieri
tarcieri commented May 30, 2023
View reviewed changes
crypto-common/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch 3 times, most recently from 5e808f0 to 0856ad0 Compare May 31, 2023 02:57
newpavlov
newpavlov reviewed May 31, 2023
View reviewed changes
Copy link
Member

@newpavlov newpavlov 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, but it would be nice to see migration of the other trait crates as well to be safe against potential unforeseen issues.

@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch 3 times, most recently from 5a6327f to 0c9a9bb Compare September 5, 2023 01:34
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
28eeaf7 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
1491f8b 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@tarcieri tarcieri mentioned this pull request Sep 5, 2023
Merged
@tarcieri
Copy link
Member Author

tarcieri commented Sep 5, 2023

Since block-buffer depends on crypto-common, I've opened the next step of this migration in the utils repo: RustCrypto/utils#944

tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
0611823 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch from 0c9a9bb to 9a65db3 Compare September 5, 2023 19:13
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
0f34165 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
da7395d 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@newpavlov
Copy link
Member

generic-array v1.0 has implemented Try/From for arrays using const generics and simplified the ArrayLength trait. Is this PR still relevant in the light of it?

@tarcieri
Copy link
Member Author

tarcieri commented Sep 13, 2023 •
edited
Loading

Yes, hybrid-array is natively built on const generic arrays at its core. Array is a newtype of the inner array type, which is pub.

generic-array instead has an opaque inner type and uses large amounts of unsafe code to achieve its goals. It has wrappers to use const generics which use a similar technique to hybrid-array (and thus have similar limitations), but if you use that approach like hybrid-array has done you can go all-in and get rid of (most of) the unsafe code.

@newpavlov
Copy link
Member

Ok. Should we migrate to generic-array v1.0 in the next release cycle first or would you prefer to try hybrid-array migration without it?

@tarcieri
Copy link
Member Author

Personally I'd prefer to go straight to hybrid-array and have already done some of the work, see also RustCrypto/utils#944

I can do cipher and digest next.

@sinui0 sinui0 mentioned this pull request Sep 13, 2023
Open
@tarcieri tarcieri mentioned this pull request Sep 22, 2023
Closed
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
c552434 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
f887f5e 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@tarcieri tarcieri mentioned this pull request Oct 8, 2023
Closed
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
2ca9557 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
@tarcieri
[WIP] Migrate to hybrid-array; MSRV 1.65
7362ec1 
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@tarcieri tarcieri mentioned this pull request Oct 8, 2023
Merged
@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch 2 times, most recently from 2854b76 to 11330b0 Compare October 26, 2023 01:30
@tarcieri tarcieri changed the title [WIP] crypto-common: migrate to hybrid-array; MSRV 1.65 crypto-common: migrate to hybrid-array; MSRV 1.65 Oct 26, 2023
@tarcieri
Copy link
Member Author

The minimal-versions failure seems to be a nightly regression and probably impacts master as well

@tarcieri
crypto-common: migrate to hybrid-array; MSRV 1.65
3ada46b 
Replaces `generic-array` with `hybrid-array`, which is built on a
combination of `typenum` and const generics, providing a degree of
interoperability between the two systems.

`hybrid-array` is designed to be a largely drop-in replacement, and the
number of changes required to switch are relatively minimal aside from
some idiosyncracies.
@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch from 11330b0 to 3ada46b Compare October 26, 2023 15:02
@tarcieri tarcieri marked this pull request as ready for review October 26, 2023 15:22
@tarcieri tarcieri requested a review from newpavlov October 26, 2023 15:23
@tarcieri
Copy link
Member Author

@newpavlov this is ready now

newpavlov
newpavlov approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@newpavlov newpavlov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you also change bound for the BlockSize sealed impl from Self: IsLess<U256> + IsGreater<U1> to Self: IsLess<U256> + IsGreater<U0>? I intended to do it in #1174.

crypto-common/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri requested a review from newpavlov October 26, 2023 16:37
@tarcieri tarcieri merged commit 766c138 into master Oct 26, 2023
86 checks passed
@tarcieri tarcieri deleted the crypto-common/hybrid-array branch October 26, 2023 17:19
@akoshelev akoshelev mentioned this pull request Mar 27, 2024
Merged
This was referenced Sep 18, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newpavlov newpavlov newpavlov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tarcieri @newpavlov