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

chore: Oink takes directly populates an instance #8170

Merged
merged 4 commits into from
Aug 23, 2024

Conversation

ledwards2225
Copy link
Contributor

@ledwards2225 ledwards2225 commented Aug 23, 2024

Oink can be thought of as an "instance completer", i.e. when it is done running all of the data that comprises an instance has been created. Up until now the model was to pass oink a reference to a proving key. It would "complete" the proving key in place by populating some witness polynomials then explicitly return the rest of the data comprising an instance (relation_parameters etc.) in a custom struct like OinkOutput. The data from this output would then be std::move'd into an instance existing in the external scope.

This PR simplifies this model by simply passing oink an instance (ProverInstance or VerifierInstance) which is "completed" in place throughout oink. IMO this is cleaner and clearer than the half-and-half approach of completing the proving key in place and explicitly returning other data. It also removes a ton of boilerplate for moving data in and out of an instance. I don't love the "input parameter treated as output parameter approach" but unless we refactor Honk/PG to construct proving_key instead of an instance, I think this is preferred. (In that case oink could take a proving_key and return a completed instance).

@ledwards2225 ledwards2225 self-assigned this Aug 23, 2024
@ledwards2225 ledwards2225 marked this pull request as ready for review August 23, 2024 22:33
@ledwards2225 ledwards2225 merged commit 6e46b45 into master Aug 23, 2024
38 checks passed
@ledwards2225 ledwards2225 deleted the lde/oink_takes_instance branch August 23, 2024 22:39
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