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: Witness API with MarshalBinary and MarshalJSON #228

Merged
merged 26 commits into from
Jan 12, 2022
Merged

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Jan 8, 2022

Work in progress. Main changes:

Previously:
backend/witness package offered api like witness.WriteFullTo(writer) witness.ReadPublicFrom(reader)...

Now has a struct that implements encoding/BinaryMarshaler and encoding/json/Marshaler. Essentially the witness can be in 3 different "state":

  1. frontend.Circuit --> this is referred to an "assignment"
  2. []fr.Element --> this is represented under witness.Witness struct introduced by this PR
  3. serialized (binary or json)

witness package enables to go from each of these states.

Other changes to validate approach:

backend/groth16 and backend/plonk had ReadAndProve and ReadAndVerify methods. Now Prove and Verify take a witness.Witness as parameter. Puts more work on caller (can't pass a frontend.Circuiit directly) but enables all workflow more cleanly.

This PR might be significantly simplified if we add generics.

Added round trip marshaling test (binary & json) in the test.Assert call.

Another PR should

  • Change APIs of IsSolved (in backend/groth16 & backend/plonk) to take a witness.Witness and to be defined, if possible directly on the frontend.CompiledConstraintSystem interface, since it's not directly dependent on the backend.

@gbotrel gbotrel added cleanup consolidate strengthen an existing feature labels Jan 8, 2022
@gbotrel gbotrel added this to the v0.7.0 milestone Jan 8, 2022
@gbotrel gbotrel marked this pull request as ready for review January 10, 2022 19:49
@gbotrel gbotrel requested a review from ivokub January 10, 2022 19:49
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

From the examples it makes sense to require that witness is instantiated separately from the circuit. I haven't tried it out myself, but from the examples the proposed interface looks good.

There are a few comments, but mostly looks good.

backend/witness/witness.go Outdated Show resolved Hide resolved
backend/witness/witness.go Outdated Show resolved Hide resolved
test/assert.go Show resolved Hide resolved
test/assert.go Outdated Show resolved Hide resolved
@gbotrel gbotrel merged commit d9d6276 into develop Jan 12, 2022
@gbotrel gbotrel deleted the witness-json branch January 12, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup consolidate strengthen an existing feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants