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

Lift the Default bound for the Circuit trait #716

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Dec 5, 2022

The Compiler::compile function keeps this bound since it actually requires the circuit to have a Default implementation, but all other functionality now works without said bound.

Resolves #715

@ureeves ureeves requested review from ZER0, vlopes11 and moCello December 5, 2022 13:00
The `Compiler::compile` function keeps this bound since it actually
requires the circuit to have a `Default` implementation, but all other
functionality now works without said bound.

Resolves #715
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@b765f44). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #716   +/-   ##
=========================================
  Coverage          ?   85.14%           
=========================================
  Files             ?       47           
  Lines             ?     3830           
  Branches          ?        0           
=========================================
  Hits              ?     3261           
  Misses            ?      569           
  Partials          ?        0           
Impacted Files Coverage Δ
src/composer/compiler.rs 98.57% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b765f44...8ed3308. Read the comment docs.

Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

LGTM

@vlopes11
Copy link
Contributor

vlopes11 commented Dec 5, 2022

The Default trait is meant to be the definition in which the circuit will compile to. The existence of the execute circuit should not be taken as reference because it was created several plonk/rust versions ago, when the language limited for instance associated constants for traits (and a couple of other things).

The general idea behind this concept is that a type should have a single circuit (hence, a keys set). This will provide type-safety and will make it easier to store the keys statically.

@ureeves
Copy link
Member Author

ureeves commented Dec 5, 2022

If it is the case that we shouldn't rely on downstream to inform us on what the API should look like, and we really want to enforce type safety - which is already questionable in the current implementation (see #713) - the we should remove Compiler::compile_with_circuit instead.
I'm not sure how to proceed here. As I mention in the linked issue, the exact problem you mention this is intended to address happens anyway.

@vlopes11
Copy link
Contributor

vlopes11 commented Dec 5, 2022

You mean that the user can serialize a PLONK key using a type A, and if he deserializes with type B this is a violation of type-safety?

If we feel this is indeed a violation to the principle, then my suggestion would be to fix that issue and improve the security instead of removing the concept. One alternative is to put additional bytes in the key to identify the type, and assert upon deserialization. But my feeling is this is overkill as it will consume more bytes.

The fact that we can't have this implementation for the execute circuit just highlights it should be refactored to comply with the new rules.

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.

Circuit shouldn't be required to be Default
3 participants