Fix error checking and bytecode circuit

[adria0]

With ok() errors are turned into an Option so the compiler do not check the result and also do not warns about unchecked result.

It seems that this woke up test errors due to changes in the underlying halo2 in some moment ( see #407 (comment) ), so also applied @Brechtpd workaround.

[ed255]

Good catch! And it seems there were indeed silent errors on the bytecode circuit tests! Probably the k values in the tests in zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs need to be bumped.

[CPerezz]

The change looks good! It's a nice catch!

Before approving, I would like that the tests that are now failing are fixed.

The NotEnoughRowsAvaliable err suggests that the k param passed to the MockProver in the run fn is lower than what's actually required. My suggestion is to make these values bigger so that they fit the size requirements.

[adria0]

@ed255 @CPerezz

Wondering if anything else wrong there, I'm running the test, with only one byte of code and one million rows k=20 and still needs more

/// Tests a circuit with incomplete bytecode
#[test]
fn bytecode_incomplete() {
   let k = 20;
   let r = MyCircuit::r();
   verify::<Fr>(k, vec![unroll(vec![7u8; 1], r)], false);
}

fails with

[zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs:618] config.assign(layouter, self.size, &self.bytecodes) = Err(
    NotEnoughRowsAvailable {
        current_k: 20,
    },
)
thread 'bytecode_circuit::bytecode_unroller::tests::bytecode_incomplete' panicked at 'called `Result::unwrap()` on an `Err` value: NotEnoughRowsAvailable { current_k: 20 }', zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs:629:64
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[CPerezz]

The error definition in Halo2 is quite clear:

/// `k` is too small for the given circuit.
    NotEnoughRowsAvailable {
        /// The current value of `k` being used.
        current_k: u32,
    },

Is it possible that you're passing more instances than required?

[therealyingtong]

I think this fixes the MockProver error: #407 (comment)

[CPerezz]

I think this fixes the MockProver error: #407 (comment)

Sorry we didn't update earlier here @therealyingtong.
In fact, this solved the error. But the circuit is still not passing due to other things (constraints not satisfied etc...).

[therealyingtong]

While reviewing the bytecode_unroller circuit I had a few questions / comments:

• is_code should be boolean-constrained (like is_final and padding are);
• do we expect the bytecode_invalid_index test to fail?

[adria0]

While reviewing the bytecode_unroller circuit I had a few questions / comments:

thanks for reviewing. :)

• is_code should be boolean-constrained (like is_final and padding are);

Yes, you're right!

• do we expect the bytecode_invalid_index test to fail?

As I understood well the test code, the idea was to deliver more unrolled code to the circuit that can handle (and this fails with a low k panic!). Maybe in a previous halo2 version used the library does not paniked and returned a Result::Err, but for now I have to catch this panic!. Or maybe I misunderstood the reason for this test?

BTW, I saw your previous comment about witnessing unqueried values, and I was checking if it works in a way do you said. Just for curiosity why do removed this comment?

[therealyingtong]

I had previously asked why we witnessed hash, hash_length on every row, when we're only looking it up on rows where is_final == 1.

I deleted it because I was going to try and figure it out from privacy-scaling-explorations/zkevm-specs#151, but I'm still in the process of doing that.

[therealyingtong]

I think I see why we're currently catching a panic for the bytecode_incomplete test; but why are we ignoring the bytecode_invalid_index test?

[Brechtpd]

is_code should be boolean-constrained (like is_final and padding are);

It's already being constrained in both the start/continue cases so should be fine I think?

but why are we ignoring the bytecode_invalid_index test?

Oh right I forgot about that, I temporarily disabled that test because halo2 gave this internal error only when providing incorrect witness values in the test: thread 'bytecode_circuit::bytecode_unroller::tests::bytecode_invalid_index' panicked at 'internal error: entered unreachable code', /halo2/halo2_proofs/src/dev/util.rs:48:42:. This happened after some update was done to halo2 because while I was writing these tests it worked correctly (as in the circuit correctly returned an error for the incorrect witness).

I had previously asked why we witnessed hash, hash_length on every row, when we're only looking it up on rows where is_final == 1.

We'll need it on each row for external circuits using the data in lookups.

[CPerezz]

So just changing q_enable from Selector to a Fixed column made it?
Could you explain why is that specifically? I'm really curious!

[CPerezz]

If it's incomplete how can it panic due to NotEnoughRowsAvailable??

[adria0]

AFAIU incomplete here means that not all code can be processed, but probally there's another definition that I'm missing.

[Brechtpd]

The goal of the test is to make sure that the circuit cannot contain partial bytecode because the bytecode hash is only verified at the last byte. So it's crucial the circuit contains all bytes. Modifying the test to catch the NotEnoughRowsAvailable rows error doesn't really achieve this goal because that does not test the actual circuit, just the witness assignment. The better way to update the test would be to allow the witness to be successfully assigned, but let the circuit verification fail because the circuit should reject incomplete bytecodes. I think that was the behaviour before because the witness assignment always returned successful and assigned as many rows as possible (but there were not enough rows to assign the full bytecode).

[adria0]

@Brechtpd, I see, thanks!

Yes, I can make the circuit failing as you said by not adding rows when assigning witness if they are going to overflow the table, and works,

341                        if offset <= last_row_offset {
342                             // Set the data for this row
343                             self.set_row(

Nonetheless, I'm asking myself if creating witness that makes a circuit failing a good strategy for tests. Is not just better to fail with NotEnoughRowsAvailable?

Anyway we are going to change how bytecode circuits works, so maybe we do not have to spend more time on this issue :)

[Brechtpd]

Nonetheless, I'm asking myself if creating witness that makes a circuit failing a good strategy for tests. Is not just better to fail with NotEnoughRowsAvailable?

The main goal of the circuits is to reject incorrect data, and so the only way to really test that is to check if the circuit indeed cannot be verified with that data. The fact that halo2 library throws an error assigning the witness is actually not useful from a security pov because that is something a malicious prover can just work around by changing a bit of code when he generates the proof.

Anyway we are going to change how bytecode circuits works, so maybe we do not have to spend more time on this issue :)

Yeah true :), but I think the tests can remain the same because the change is just how the circuit does some of the constraints, and so the tests still need to test the same things.

[adria0]

The main goal of the circuits is to reject incorrect data, and so the only way to really test that is to check if the circuit indeed cannot be verified with that data. The fact that halo2 library throws an error assigning the witness is actually not useful from a security pov because that is something a malicious prover can just work around by changing a bit of code when he generates the proof.

Super good information to keep in mind! Thanks!

Yeah true :), but I think the tests can remain the same because the change is just how the circuit does some of the constraints, and so the tests still need to test the same things.

Fixed in f33f782 (I hope!)

[Brechtpd]

So just changing q_enable from Selector to a Fixed column made it? Could you explain why is that specifically? I'm really curious!

The MockProver requires any gate with a selector enabled to have all its referenced cells to be assigned. Fixed columns do not have this extra check. I also find this difference a bit strange (and impossible to achieve in certain scenario's) and made this issue: zcash/halo2#533.

[adria0]

@miha-stopar I did not manually required your review, just the new automatic system assigned it to you!

[miha-stopar]

LGTM!

[CPerezz]

I think we should follow @ed255 's approach mentioned here: #403 (comment)

[ed255]

I'm approving this PR following the conclusion at #403 (comment) and moving the discussion of negative tests to #400

@CPerezz does this seem OK to you? (as currently your review status is "requested changes")

[CPerezz]

LGTM.

Sorry for blocking on the merge of this.

Let's try to not forget the work left or tech-debt generated here by tracking this in #400