-
-
Notifications
You must be signed in to change notification settings - Fork 471
Adapt to chacha20 #1642
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
Open
hpenne
wants to merge
8
commits into
rust-random:master
Choose a base branch
from
hpenne:adapt_to_chacha20
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Adapt to chacha20 #1642
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
20e7ec9
Added test vectors for StdRng, verified using rand_chacha.
hpenne 92e094d
Replaced rand_chacha with chacha20.
hpenne 3adf5e6
Test refactoring
hpenne e3c1701
Added entry to CHANGELOG.md
hpenne 8dc528a
Changed MSRV to 1.85 to accomodate chacha20.
hpenne 09befe3
Put back clippy suppression that failed locally.
hpenne 296ae2b
Added fix for multiple versions of rand_core to benches.
hpenne 0fbb567
Attempted fix for benches compile error (not verified, benches does n…
hpenne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make it impossible to make releases.
Ugh. I think there are only three options here:
chacha20
source into this repo (likely undesirable; CC @tarcieri)rand_core
from the workspace. This will require using a patch similar to this for development wheneverrand_core
has breaking changes, but at least allows clean compiling otherwise.This is why
rand_chacha
is co-located in this repo with external dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think crate releases ignore these directives and try to build directly against crates.io?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be a 4th option: Depend on chacha20 but without the "rng" feature, then implement the required traits for chacha inside the rand crate instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it? I wish their documentation would clarify.
cargo publish --dry-run
looks okay, so this might work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But even assuming this does work, how are we supposed to handle breaking changes to
rand_core
in the future? If we ever make any changes incompatible withchacha20
our local builds will simply not work until upstreamchacha20
gets fixed.I don't think that's viable. (And the only reason it's not definitely unviable is that I don't expect much in the way of breaking changes to
rand_core
in the future.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tarcieri @newpavlov do you have a policy on breaking changes to affected crates (e.g.
digest
,signature
)? Do you just simultaneously commit a fix to other affected repos and pin the version? Or do you never really get breaking changes which affect the build in practice?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, often we'll have a PR to the other repo ready to go.
That said we'll hopefully be wrapping up breaking changes soon and be stable for awhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhardy Isn't that a deadlock situation? With the "patch"-solution, if you make changes to rand_core so that local builds fail (due to chacha being forced to use the newer version that it is not compatible with), then you cannot publish a new version of rand_core, which means that chacha20 cannot be updated to support the new rand_core version either, because there is no such version published. Can that deadlock be broken in any acceptable way? If I implement the newtype/adapter workaround above, then we can use the patch solution 99% of the time to ensure that rand_core is only built once, but if you get into this potential deadlock situation then you can remove the patch temporarily to resolve it (everything will work, but you get rand_core built twice) and put it back again when both rand_core and chacha20 has been updated. You will get to have your cake and eat it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hpenne publishing a new version of
rand_core
only requires that therand_core
build succeeds, not the whole workspace, so breaking changes don't cause a deadlock, no.I guess the
patch
solution is tolerable. If you do try implementing the newtype/adapter workaround, please push as a new PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clearing that up for me. I suppose that the problem of rand itself potentially not building in that situation due to breaking changes in rand_core can be resolved by modifying rand to fetch rand_core from crates.io for a while (using the previous release of rand_core), until chacha20 is updated to use the new rand_core and everything is in sync again so that it all builds with the local current rand_core from the rand git repo. Or perhaps there are even better ways, these kinds of sort-of-circular dependencies are a little new to me.
Noted. It probably won't be needed as long as the situation can be resolved by publishing any new breaking version rand_core separately.