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

Move Towards Generic API #170

Merged
merged 48 commits into from
Jun 21, 2024
Merged

Move Towards Generic API #170

merged 48 commits into from
Jun 21, 2024

Conversation

sjudson
Copy link
Contributor

@sjudson sjudson commented Jun 7, 2024

At present, we have a variety of pipelines for each frontend -> vm -> proving scheme implementation.

  • tools (minus src/command/jolt.rs) -> prover -> nova
  • prover -> nova
  • tools/src/command/jolt.rs -> jolt
  • api -> prover -> nova

where the last has to take care to avoid a lot of coupling between the CLI and the prover crate. These pipelines are often independent from each other when they should be coherent, or intertwined with each other when they should be separated. This is not so big a deal at present, but in the short-term make it difficult to cleanly implement a future-proofed (as much as possible) SDK, and in the long-term will get unwieldy over time as various frontends and backends proliferate.

This PR is an initial move to a cleaner approach, where the api crate concentrates various backends that can be consumed by the various frontends (for now the CLI, Rust SDK, and network crate, in the future more network functionality as well as SDKs for other languages and environments).

In practice, this PR does three major things:

  1. It moves the config and prover crates under the api crate, developing a single interface for frontends to access.
  2. It slims down the prover crate, moving all CLI-dependent functionality into the tools crate and removing the vestigal SDK that was implemented within it.
  3. It passes the Jolt implementation through the api/prover crate as well, again with the goal of having a single interface for frontends to access.

Also relevant is #169. At present, the API crate is still "opinionated" about configuration of generic parameters, e.g., choices of curve pairings for Nova, random oracles, and commitment schemes. In the future we may want to let the various frontends to configure these generic parameters, which would require making the code in api suitably generic for the frontends to consume as needed.

@sjudson sjudson requested review from slumber and mx00s June 7, 2024 18:54
Copy link

vercel bot commented Jun 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nexus-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 21, 2024 6:48pm

config/Cargo.toml Outdated Show resolved Hide resolved
api/examples/prover_run.rs Outdated Show resolved Hide resolved
@sjudson
Copy link
Contributor Author

sjudson commented Jun 10, 2024

Restored the original config crate at @slumber's request.

Copy link
Contributor

@slumber slumber left a comment

Choose a reason for hiding this comment

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

I won't block the PR, but I still don't agree with general approach on introducing "programmatic interface"

  1. "api" usually describes something network-related, I bet a lot of people would find crate name confusing
  2. we have a bulky nexus-prover crate which does sort of everything. With almost no changes this crate migrates to nexus-api, which I don't find any better. I outlined it here.
  3. going back to review comment, I don't understand the approach of rewriting anything before the actual sdk is implemented. Meaning: come up with generic trait interface (Prover SDK tracking issue #148 original description seems to be ignored), introduce it as a crate into the workspace, only then start migrating code like tools or vm to use these interfaces. Finally, remove api and prover altogether.
  4. The approach taken right now doesn't facilitate the usage of the api crate, because it's just huge (just like the prover crate), while also gets in the way of developing other parts of the system, like RPC. I had to introduce ProverT trait to RPC myself because it still doesn't exist. And the PR breaks the code.

@@ -10,6 +10,53 @@ keywords = { workspace = true }
categories = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs a cleanup, e.g. I don't think it should depend on tui and have verbose feature anymore

@slumber
Copy link
Contributor

slumber commented Jun 11, 2024

As for the sdk shape: I imagined we could start with very minimal

// call it nexus-prover-core crate or smth
trait Prover {}

which should be as lightweight as possible

Then, all prover crates would depend on it.

// nexus-nova
impl Prover for NovaProver {}
// nexus-jolt
impl Prover for JoltProver {}

Finally,

// nexus-sdk
pub use Prover;
#[cfg(feature = "nova")]
pub use nexus_nova::NovaProver;
#[cfg(feature = "jolt")]
pub use nexus_jolt::JoltProver;

Maybe not so much configurable in terms of curves/fields/k/etc., perhaps snips some prover-specific features, but clean and easy to use.

@sjudson
Copy link
Contributor Author

sjudson commented Jun 11, 2024

@slumber

"api" usually describes something network-related, I bet a lot of people would find crate name confusing

I'd disagree on this and think api is exactly the right name, but also have no special attachment to it: would be to rename it, maybe something like interface (or even, cheekily, nexus)?

we have a bulky nexus-prover crate which does sort of everything. With almost no changes this crate migrates to nexus-api, which I don't find any better. I outlined it #146 (review).

There are significant changes: stripping out CLI interfaces, removing redundant code, removing quasi-sdk interfaces in favor of more concise ones. Moreover, the prover crate is really a nova prover crate -- as we implement other proving schemes and a proper sdk the name becomes more and more misleading.

going back to review #170 (comment), I don't understand the approach of rewriting anything before the actual sdk is implemented. Meaning: come up with generic trait interface (#148 original description seems to be ignored), introduce it as a crate into the workspace, only then start migrating code like tools or vm to use these interfaces. Finally, remove api and prover altogether.

I've worked in a context before where you have a proliferation of frontend SDKs/CLI tooling and backends, and the only reason it was manageable from a cross-team coordination standpoint was because the web API that linked everything together provided a single, concrete interface that enabled clear interoperability. For a non-web project the roadmap of the zkVM development is I think unusually many-to-many, and I think there is a clear benefit to using this api approach to replicate the same structure here. I like the idea of having a generic trait interface that disparate code can implement in principle, but it will still make things difficult to navigate when it has so many different producers and consumers.

Thoughts on this @mx00s?

The approach taken right now doesn't facilitate the usage of the api crate, because it's just huge (just like the prover crate), while also gets in the way of developing other parts of the system, like RPC. I had to introduce ProverT trait to RPC myself because it still doesn't exist. And the PR breaks the code.

If you are already implementing a generic ProverT trait for the RPC work then I'll put this on draft, we can get that PR merged, and then I'll adapt this PR to your work instead of the other way around.

@sjudson sjudson marked this pull request as draft June 11, 2024 15:29
@sjudson
Copy link
Contributor Author

sjudson commented Jun 11, 2024

Converted back to draft to enable some other relevant code to come through first, that this will be adapted to.

@mx00s
Copy link
Contributor

mx00s commented Jun 15, 2024

Thoughts on this @mx00s?

To start, I'm going to avoid using the terms API and SDK.

I agree with the idea of leveraging clearly defined interfaces to improve cross-team coordination.

I've worked in a context before where you have a proliferation of frontend SDKs/CLI tooling and backends, and the only reason it was manageable from a cross-team coordination standpoint was because the web API that linked everything together provided a single, concrete interface that enabled clear interoperability.

That will be necessary as

  1. we iteratively improve implementations of those interfaces,
  2. create new implementations, and
  3. support a growing variety of consumers.

In line with what @slumber seems to suggest, I also appreciate the benefits of designing simple interfaces starting top-down as opposed to bottom-up:

going back to review #170 (comment), I don't understand the approach of rewriting anything before the actual sdk is implemented. Meaning: come up with generic trait interface (#148 original description seems to be ignored), introduce it as a crate into the workspace, only then start migrating code like tools or vm to use these interfaces.

In terms of API and SDK dichotomy as it's been presented so far, my understanding is the API is meant to be a lower-level abstraction for IVC and the SDK will eventually depend on it, providing higher-level interfaces that encapsulate more of the details. I can imagine this working well, but admittedly am hazy on who the intended consumers of the SDK would be and what details relevant to the API they'd benefit from having encapsulated.

It's abundantly clear we need clean, simple interfaces to support the aforementioned coordination points over time. Whether to design such interfaces working bottom-up or top-down is somewhat a matter of taste, but I am admittedly biased toward the top-down approach to optimize for ergonomic UX. Admittedly, sometimes the top-down approach means discovering while wiring up the internals that the high-level interfaces must be adjusted, but that's okay.

I've mentioned it elsewhere, but if we align on the top-down approach I think we'd benefit from:

  1. implementing end-to-end tests Add end-to-end zkVM regression test #165
  2. defining high-level interfaces that make those tests clean
  3. promoting those high-level interfaces to a public crate that serves as a coordination point, whatever we choose to call it

By the way, like @sjudson I don't see "api" as usually pertaining to networking. I suppose OpenAPI/Swagger may have shifted norms in that direction a bit, though.

@sjudson
Copy link
Contributor Author

sjudson commented Jun 21, 2024

After some further discussion, added feature gating @mx00s @slumber: fde39c0

@sjudson
Copy link
Contributor Author

sjudson commented Jun 21, 2024

Updated dependency graph:

image

replacing

image

@mx00s
Copy link
Contributor

mx00s commented Jun 21, 2024

Nice. For reference, the following command can be used to produce those dependency graphs:

cargo install cargo-depgraph
cargo depgraph --workspace-only | dot -Tpng > graph.png

Copy link
Contributor

@mx00s mx00s left a comment

Choose a reason for hiding this comment

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

With the dependency graph reconfigured, we're better positioned to iterate on nexus-api and support the many-to-many dependence between prover frontends and backends.

Thanks for adding the prover feature flags too. This should help as continue implementing/improving provers and even vary defaults per deployment environment, for instance.

LGTM!

@sjudson sjudson merged commit a6b3e24 into main Jun 21, 2024
6 checks passed
@sjudson sjudson deleted the generic-api branch June 21, 2024 19:13
sjudson pushed a commit that referenced this pull request Feb 5, 2025
to ba90cee80df22aa2c54d5b90a14b2e7ad1ccea52

This involved moving the preprocessed trace to interaction index 0.
sjudson pushed a commit that referenced this pull request Feb 12, 2025
to ba90cee80df22aa2c54d5b90a14b2e7ad1ccea52

This involved moving the preprocessed trace to interaction index 0.
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.

3 participants