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

Switch dependencies to accelerated versions #1

Open
wants to merge 4 commits into
base: risczero
Choose a base branch
from

Conversation

austinabell
Copy link

risc0 branch created at the 2.1.0 tag

@austinabell austinabell requested review from nategraf and hmrtn June 10, 2024 17:10
Copy link

@nategraf nategraf left a comment

Choose a reason for hiding this comment

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

Left a few suggestions. Overall, this is small patch and we can definitely do this. Is there a reason to use sha2 at v0.10.8 instead of v0.9.9?

Another note. I've been calling the branch risczero instead of risc0. I'd re-target this just to be consistent.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
Comment on lines 29 to 31
[patch.crates-io]
sha2 = { git = "https://github.com/risc0/RustCrypto-hashes", tag = "sha2-v0.10.8-risczero.0" }

Choose a reason for hiding this comment

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

Looking at cargo tree -i -p sha2 it looks like ed25519-zebra uses v0.9.9. As a result, I don't think this patch would effect it. We should also try to avoid having two versions of sha2 from being used in the tree. Using v0.9.9 above would help with that.

Choose a reason for hiding this comment

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

Sidenote: we should really have a tool that can tell devs whether or not the patches are being used. We could probably base it on cargo tree, to check that no unaccelerated versions of sha2, k256, or curve25519-dalek appear in the tree 🤔

Copy link
Author

Choose a reason for hiding this comment

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

So that dep is just used for tests, which is why I figured it was fine to leave as is. Let me know if you think otherwise!

Choose a reason for hiding this comment

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

Ah, indeed! I'd go ahead and leave out the patch entirely then and let the tests use the software implementation.

Suggested change
[patch.crates-io]
sha2 = { git = "https://github.com/risc0/RustCrypto-hashes", tag = "sha2-v0.10.8-risczero.0" }

Copy link
Author

Choose a reason for hiding this comment

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

So it works fine with this, but should be fine to remove this patch (I think it gets ignored anyway in upstream usages)

src/batch.rs Outdated Show resolved Hide resolved
src/batch.rs Outdated Show resolved Hide resolved
tests/small_order.rs Outdated Show resolved Hide resolved
@nategraf
Copy link

I also pushed the version tags from the upstream repo so that we have them here as well.

@austinabell
Copy link
Author

Left a few suggestions. Overall, this is small patch and we can definitely do this. Is there a reason to use sha2 at v0.10.8 instead of v0.9.9

So the reason was is that the non ng curve-dalek crate uses 0.10, so there are some incompatibilities and issues with it.

Another note. I've been calling the branch risczero instead of risc0. I'd re-target this just to be consistent.

I'll do that now!

@austinabell austinabell changed the base branch from risc0 to risczero June 10, 2024 22:59
@austinabell austinabell requested a review from nategraf June 10, 2024 23:01
@nategraf
Copy link

Left a few suggestions. Overall, this is small patch and we can definitely do this. Is there a reason to use sha2 at v0.10.8 instead of v0.9.9

So the reason was is that the non ng curve-dalek crate uses 0.10, so there are some incompatibilities and issues with it.

It looks like ed25519-dalek uses sha2, but curve25519-dalek does not. So the only version pulled in for a release build is the one directly referenced here. Using our v0.9.9 patch would result in a slightly smaller diff here (which is kind of the name of the game with these patches, haha)

@austinabell
Copy link
Author

It looks like ed25519-dalek uses sha2, but curve25519-dalek does not. So the only version pulled in for a release build is the one directly referenced here. Using our v0.9.9 patch would result in a slightly smaller diff here (which is kind of the name of the game with these patches, haha)

Addr in dms, the downstream deps of the non ng variant are mismatched using 0.9, and also reduces duplicate dependencies being pulled in from upstream uses (namely tendermint-light-client-verifier)

Co-authored-by: Victor Graf <victor@subgraf.dev>
austinabell added a commit to risc0/risc0 that referenced this pull request Dec 11, 2024
Some additions I think might be helpful, notably:
- Update `k256` to use tagged release
- Puts acceleration in table with example usages of each
- noting this could include
risc0/ed25519-consensus#1 if we decide to tag
those changes (currently used in blobstream0)
- Documentation around the nuances of updating to patch (as there are
many footguns)
- Updates old patch tags to use the consistent `-risczero.X` format

Pointing at #2639
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.

2 participants