zksecurity / noname

Noname: a programming language to write zkapps
https://zksecurity.github.io/noname/
178 stars 44 forks source link

Add test to check incorrect output for kimchi backend #59

Closed katat closed 4 months ago

katat commented 4 months ago

https://github.com/zksecurity/noname/issues/58

The kimchi prover/ verifier already cover the public output constraints. Here we add a test to ensure this is the case.

katat commented 4 months ago

I had a check, the verify function from the ProverIndex seems also including the permutation check?

for col in 0..PERMUTS {
  let wire = gate.wires[col];

  if wire.col >= PERMUTS {
      return Err(GateError::Custom {
          row,
          err: format!("a wire can only be connected to the first {PERMUTS} columns"),
      });
  }

  if witness[col][row] != witness[wire.col][wire.row] {
      return Err(GateError::DisconnectedWires(
          Wire { col, row },
          Wire {
              col: wire.col,
              row: wire.row,
          },
      ));
  }
}

Once the assert_eq constraint for the output is added, this check ^ seems already covering without the need to generate proof. (The refactored test demonstrates this fact)

katat commented 4 months ago

best practice is to include a screenshot of the test failing before you fix the test by adding the constraints, can you do that :D?

I rebased the commits to showcase the failed test at the commit https://github.com/zksecurity/noname/pull/59/commits/8ff921e966f743b7464199d3252d6daff0d88ff7

screenshot:

Screenshot 2024-05-06 at 10 22 34

Then it is fixed by the subsequent commit, which details the test on different types of verification errors.

also I didn't review some code that seems unrelated to this PR, so wondering if I need to?

I think they are related to the fix. Please refer to my comments above :D