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

Feat/g16 enormous fs hint #405

Merged
merged 24 commits into from Dec 5, 2022
Merged

Feat/g16 enormous fs hint #405

merged 24 commits into from Dec 5, 2022

Conversation

Tabaie
Copy link
Contributor

@Tabaie Tabaie commented Dec 1, 2022

No description provided.

@Tabaie Tabaie requested a review from gbotrel December 1, 2022 18:28
@Tabaie Tabaie linked an issue Dec 1, 2022 that may be closed by this pull request
@gbotrel
Copy link
Collaborator

gbotrel commented Dec 1, 2022

quick remark; maybe add a panic for now in ExportSolidity in groth16.Verify when Commitment is set. (+ create github issue to tackle this and the same thing you did foir Groth16 in PlonK 👍 )

@@ -576,3 +576,19 @@ func (system *r1cs) negateLinExp(l compiled.LinearExpression) compiled.LinearExp
func (system *r1cs) Compiler() frontend.Compiler {
return system
}

func (system *r1cs) Commit(v ...frontend.Variable) (frontend.Variable, error) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

need to ensure we do it only once? since it uses a singleton (system.CommitmentInfo). But ... technically we could have (if needed) a list of []CommitmentInfo right?

Copy link
Contributor Author

@Tabaie Tabaie Dec 2, 2022

Choose a reason for hiding this comment

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

It's definitely ideal for different subcircuits to be able to get different commitments. Maybe something like

func Commit(uid []byte, v ...frontend.Variable) (frontend.Variable, error)

that could be called multiple times.
I think it's worth trying but would involve even more modifying-a-hint-after-it's-created type shenanigans. I'll create an issue and once I'm a bit more secure that my current hint hacks are okay, I'll take a stab at it.

But in the meantime I agree there should be an error if Commit is called more than once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course if the hint hacks started to get out of hand, we could open the can of worms that is giving the commitments their own solving and scheduling mechanisms.
I do remember though that you said @ThomasPiellard had an idea regarding generalizing hints. Perhaps the cleanest way is to implement that first ( "that" = an abstraction that both hints and commitments embody that takes care of scheduling and solving), then build the commitments on top of that cleanly; no hints, no hacks, no duplication of logic.

}

func bsb22CommitmentComputePlaceholder(*big.Int, []*big.Int, []*big.Int) error {
return fmt.Errorf("placeholder function: to be replaced by commitment computation")
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this just a placeholder? Would be nice to have a default impl, so that calling R1CS.Solve on a R1CS with commitment info actually works?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(that is, without depending on Groth16)

Copy link
Contributor Author

@Tabaie Tabaie Dec 2, 2022

Choose a reason for hiding this comment

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

The problem is that since the hint computes the commitment, it requires the Commitment Key, created during setup. But the hint is created in compilation time, when the Commitment Key is not yet available.

The only way out of this that I can think of is to instead randomize the Commitment Key during compilation. But that would break modularity by making setup-type tasks bleed into compilation, and also bring a big code-generated curve switch-case into the compilation codebase, which as I understand currently doesn't have any.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, what I'm saying is that, technically, the frontend should build a constraint system, that's solvable as is, without going to the next step (proof systems).
So why not just put a hint that do a sha256 or something deterministic with the inputs, without a "key"?

Copy link
Contributor Author

@Tabaie Tabaie Dec 2, 2022

Choose a reason for hiding this comment

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

Sha256 is only possible for public variables to which the verifier has direct answer. For private variables we will need that Pedersen commitment whose basis comes from the verifying key. Unfortunately this spaghetti quality, that weakens the modular frontend/backend separation is inherent to the scheme, and actually the reason why it works (i.e. soundness depends on it.)

for _, inI := range publicCommitted {
inIBytes := inI.Bytes()
slack := fieldByteLen - len(inIBytes)
buf.Write(make([]byte, slack))
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the case of many public commited values, would probably be beneficial to allocate an empty []byte of len fieldByteLen before the loop, and write slack[:slackIdx] here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed something in that spirit!

@gbotrel gbotrel merged commit 2e88f72 into develop Dec 5, 2022
@gbotrel gbotrel deleted the feat/g16-enormous-fs-hint branch December 5, 2022 16:42
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.

Compiler.Commit(...)
2 participants