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(decaf377): bump dep #3806

Merged
merged 24 commits into from
Jul 10, 2024
Merged

refactor(decaf377): bump dep #3806

merged 24 commits into from
Jul 10, 2024

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Feb 12, 2024

Describe your changes

Updates the decaf377, decaf377-rdsa, and poseidon377 dep versions

Issue ticket number and link

References #3676 and consumes changes in #3678. unblocked by penumbra-zone/decaf377#101

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

@TalDerei
Copy link
Collaborator Author

While the circuits are passing in bin/pcli/tests/proof.rs we're still getting some failures in the penumbra-app package I'll spend today debugging.

@TalDerei TalDerei added A-shielded-crypto Area: Cryptographic design for Penumbra's shielded transaction model C-enhancement Category: an enhancement to the codebase labels Mar 1, 2024
@TalDerei
Copy link
Collaborator Author

TalDerei commented Mar 4, 2024

There's still smoke test failures that need to be resolved.

@TalDerei
Copy link
Collaborator Author

TalDerei commented Mar 4, 2024

currently soft blocked by penumbra-zone/decaf377#91. Once that's merged, I can modify the decaf377, decaf377-rdsa, and poseidon377 deps in this branch to point to main.

@TalDerei
Copy link
Collaborator Author

TalDerei commented Mar 8, 2024

There's an arkworks workstream to support 32-bit limbs (64-bit multiplier):

This PR will (maybe?) remain blocked until that work is merged. This is based on the observation that our fiat-crypto generated field impl (benchmarked in #3976 and recorded in https://docs.google.com/spreadsheets/d/1DonNNCN6-Nm3SxSpOymkGYOQi2wAK0l3_CfuDPq4vQc/edit#gid=0) is currently slower than the arkworks backends

@TalDerei
Copy link
Collaborator Author

TalDerei commented May 4, 2024

@redshiftzero what's the current status of this?

@cratelyn cratelyn removed the on hold label May 6, 2024
@TalDerei TalDerei added the stale Issue may no longer be relevant, needs triage label May 9, 2024
@TalDerei
Copy link
Collaborator Author

TalDerei commented May 16, 2024

will be unblocked by work-stream push in penumbra-zone/decaf377#101

@TalDerei
Copy link
Collaborator Author

TalDerei commented Jul 1, 2024

@redshiftzero @cronokirby rebased PR, and crate deps have been updated for decaf377 (0.5.0 to 0.10.0), decaf377-rdsa, and poseidon377. We have smoke test coverage, but still need to resolve the remaining CI failures. We should cut any necessary releases for decaf377-rdsa and poseidon377 now that their cargo files have been updated to use version 0.10.0 of decaf. I think we're close to finally closing the tracking issue.

@TalDerei TalDerei added C-refactor Category: refactors and other related improvements and removed stale Issue may no longer be relevant, needs triage labels Jul 2, 2024
@TalDerei TalDerei requested a review from erwanor July 5, 2024 14:02
Copy link
Member

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

thanks!

@redshiftzero
Copy link
Member

one last thing to figure out here actually is why we need the getrandom dependency added everywhere, since it's a dev dependency only of decaf377

@redshiftzero
Copy link
Member

cargo tree --package penumbra-community-pool (the failing package here) on a527092 indicates that getrandom is being pulled in due to ark-ff:

penumbra-community-pool v0.79.0-alpha.1 
├── anyhow v1.0.86
├── ark-ff v0.4.2
│   ├── ark-ff-asm v0.4.2 (proc-macro)
│   │   ├── quote v1.0.36
│   │   │   └── proc-macro2 v1.0.86
│   │   │       └── unicode-ident v1.0.12
│   │   └── syn v1.0.109
│   │       ├── proc-macro2 v1.0.86 (*)
│   │       ├── quote v1.0.36 (*)
│   │       └── unicode-ident v1.0.12
│   ├── ark-ff-macros v0.4.2 (proc-macro)
│   │   ├── num-bigint v0.4.6
│   │   │   ├── num-integer v0.1.46
│   │   │   │   └── num-traits v0.2.19
│   │   │   │       [build-dependencies]
│   │   │   │       └── autocfg v1.3.0
│   │   │   └── num-traits v0.2.19 (*)
│   │   ├── num-traits v0.2.19 (*)
│   │   ├── proc-macro2 v1.0.86 (*)
│   │   ├── quote v1.0.36 (*)
│   │   └── syn v1.0.109 (*)
│   ├── ark-serialize v0.4.2
│   │   ├── ark-serialize-derive v0.4.2 (proc-macro)
│   │   │   ├── proc-macro2 v1.0.86 (*)
│   │   │   ├── quote v1.0.36 (*)
│   │   │   └── syn v1.0.109 (*)
│   │   ├── ark-std v0.4.0
│   │   │   ├── num-traits v0.2.19 (*)
│   │   │   ├── rand v0.8.5
│   │   │   │   ├── libc v0.2.155
│   │   │   │   ├── rand_chacha v0.3.1
│   │   │   │   │   ├── ppv-lite86 v0.2.17
│   │   │   │   │   └── rand_core v0.6.4
│   │   │   │   │       └── getrandom v0.2.15
│   │   │   │   │           ├── cfg-if v1.0.0
│   │   │   │   │           └── libc v0.2.155
│   │   │   │   └── rand_core v0.6.4 (*)
│   │   │   └── rayon v1.10.0
[snip]

i.e., ark-ff is pulling in ark-serialize which is pulling in ark-std which eventually pulls in rand_core and getrandom. rand_core pulling in getrandom here makes sense given feature unification across the workspace since rand_core elsewhere in the workspace uses the getrandom feature.

@redshiftzero redshiftzero merged commit c1d7a84 into main Jul 10, 2024
13 checks passed
@redshiftzero redshiftzero deleted the upgrade-decaf377 branch July 10, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shielded-crypto Area: Cryptographic design for Penumbra's shielded transaction model C-enhancement Category: an enhancement to the codebase C-refactor Category: refactors and other related improvements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants