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

fix: circular dependency #3392

Merged
merged 1 commit into from
Nov 22, 2023
Merged

fix: circular dependency #3392

merged 1 commit into from
Nov 22, 2023

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Nov 16, 2023

References #3389. This PR migrates the benches from proof-params to a dedicated bench package and removes any unnecessary dev-dependencies to bypass the circular dependency bug that breaks rust-analyzer.

@conorsch
Copy link
Contributor

conorsch commented Nov 16, 2023

CI looks happy, which is a great sign. If I run cargo bench locally, however, I see a bunch of warnings:

❯ cargo bench
warning: invalid feature `proving-keys` in required-features of target `nullifier_derivation`: `proving-keys` is not present in [features] section
warning: invalid feature `proving-keys` in required-features of target `output`: `proving-keys` is not present in [features] section
warning: invalid feature `proving-keys` in required-features of target `delegator_vote`: `proving-keys` is not present in [features] section
warning: invalid feature `proving-keys` in required-features of target `spend`: `proving-keys` is not present in [features] section
warning: invalid feature `proving-keys` in required-features of target `swap`: `proving-keys` is not present in [features] section
warning: invalid feature `proving-keys` in required-features of target `swap_claim`: `proving-keys` is not present in [features] section
warning: invalid feature `proving-keys` in required-features of target `undelegate_claim`: `proving-keys` is not present in [features] section
   Compiling penumbra-custody v0.63.1 (/home/conor/src/penumbra/crates/custody)
   Compiling penumbra-view v0.63.1 (/home/conor/src/penumbra/crates/view)
   Compiling narsil v0.63.1 (/home/conor/src/penumbra/crates/narsil/narsil)
   Compiling pd v0.63.1 (/home/conor/src/penumbra/crates/bin/pd)
[...snip...]

I made sure to confirm that these warnings do not appear on main. Anything we can do to clean them up? Once we're happy with the substantive changes, please make sure to edit the docs to make sure the correct directories and commands are specified, e.g. at https://guide.penumbra.zone/main/dev/parameter_setup.html, the source for which you can find at docs/guide/src/dev/parameter_setup.md.

@cronokirby
Copy link
Contributor

I don't want to bikeshed too hard either, but conceptually it is sort of weird to have these things in proof-setup, which is basically just the internal math necessary for the summoning ceremony. The reason that crate has a dependency on the circuits is just to calculate their size.

@cronokirby
Copy link
Contributor

I think the ideal thing would be to have benches at the monorepo level, which can then take in any crate they want for whatever reason.

@TalDerei
Copy link
Collaborator Author

TalDerei commented Nov 16, 2023

@conorsch I just noticed this as well. Per #3337, it'll require passing in the

penumbra-proof-params = { path = "../../crypto/proof-params", features = [
    "bundled-proving-keys",
    "download-proving-keys",
] }

dependency to the Cargo file. Right now, it requires a slightly different feature proving-keys to be present. I'll incorporate the change.

@cronokirby I agree. I sort of put the benches there as a placeholder to satisfy the CI, but moving it should be simple. I'll make that change as well.

@TalDerei TalDerei changed the title Fix Circular dependency fix: circular dependency Nov 16, 2023
@TalDerei TalDerei requested a review from cronokirby November 17, 2023 01:51
@TalDerei TalDerei changed the base branch from main to lazy-proving-key November 17, 2023 01:52
@TalDerei TalDerei changed the base branch from lazy-proving-key to main November 17, 2023 01:52
@TalDerei
Copy link
Collaborator Author

TalDerei commented Nov 17, 2023

I had to merge the commits from #3389 to pass the CI and prevent the changes in this branch from breaking once #3389 is merged into master. Keeping this PR as a draft until the other PR is approved and merged.

@TalDerei TalDerei marked this pull request as ready for review November 17, 2023 08:42
@TalDerei
Copy link
Collaborator Author

Ready for review!

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

I like the approach here; let's rebase and then get this merged

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

One more thing: when I run cargo bench on this branch, it updates the workspace's Cargo.lock. The changes to that file should be committed and added to this PR. Other than that, LGTM!

docs/guide/src/dev/parameter_setup.md Outdated Show resolved Hide resolved
@conorsch
Copy link
Contributor

I'm going to do a bit of housekeeping on the commit history here, then merge.

References #3389. This PR migrates the benches from proof-params
to a dedicated bench package and removes any unnecessary
dev-dependencies to bypass the circular dependency bug
that breaks rust-analyzer.
@conorsch conorsch force-pushed the circular-dependency-fix branch from 1cddc43 to cd1d63e Compare November 22, 2023 17:11
@conorsch conorsch merged commit f9fdf40 into main Nov 22, 2023
6 checks passed
@conorsch conorsch deleted the circular-dependency-fix branch November 22, 2023 17:38
conorsch added a commit that referenced this pull request Nov 29, 2023
conorsch added a commit that referenced this pull request Nov 29, 2023
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.

4 participants