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

Add --disable-safety-check option #235

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

StefanosChaliasos
Copy link

@StefanosChaliasos StefanosChaliasos commented Nov 14, 2024

This option allows variables that are not constrained.

Right now, it's a bit ugly how this parameter has to be passed. I will explain the workflow below, but I guess in the future, we can have a struct to capture parameters and pass them around so we can have access to user-defined configs.

We need to add some ifs for this particular feature when we validate that variables have been used. This is happening in the finalize_circuit function inside circuit_writer instead of in the type checking and semantic checks. To pass the parameter from the UI based on which we are going to return an error or not, we do the following:

  1. Add the option to cli/cmd_build_and_check.rs and cli/cmd_prove_and_verify.rs
  2. Add bool parameter (disable_safety_check) in compiler.rs::compile function
  3. Pass that parameter to CircuitWriter::generate_circuit function
  4. Pass that parameter to finalize_circuit function (In all backends)
  5. Add the if statements (In all backends)

Furthermore, in the tests, we always set that option to false.

This option allows variables that are not constrained.
Copy link
Collaborator

@katat katat left a comment

Choose a reason for hiding this comment

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

Cool, lgtm. @mimoo may want to have a pass.

ErrorKind::PrivateInputNotUsed,
private_cell_var.span,
))?
if !disable_safety_check {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

private_cell_var.span,
);
Err(err)?;
if !disable_safety_check {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe checking this flag at the root block is cleaner :)

    if !written_vars.contains(&var) &&  !disable_safety_check {

@@ -71,6 +71,10 @@ pub struct CmdBuild {
/// Run in server mode to help debug and understand the compiler passes.
#[clap(long)]
server_mode: bool,

/// Do not check that every variable is in a constraint
#[arg(long = "disable-safety-check", global = true)]
Copy link
Collaborator

@katat katat Nov 18, 2024

Choose a reason for hiding this comment

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

Maybe adding a doc on why this is necessary in certain cases, and it should not be recommended by default.

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