fix: constraint system fix | related fixes in copy circuit

[roynalnaruto]

The following fixes have been made in this PR:

• Exceptions (eg. assertion) from within a cs.condition were not raised since the __exit__ did not raise them. This meant, we were never warned about nested conditions not allowed, and any constraint failure within a condition was also completely ignored.
• CALLDATACOPY memory read was added even for is_pad = True
• Remove nested conditions
• SHA3 gadget should do a copy table lookup only if length > 0. Otherwise copy_rwc_inc = rlc_acc = 0

[ChihChengLiang]

1. I don't understand the first point. If lhs != rhs, it makes sense the subtraction (self._eval(lhs.expr() - rhs.expr())) gave a non-zero output. Do you mean even if the condition is 0, the constrain_equal still raises ConstraintUnsatFailure for lhs != rhs?
2. Do you think it makes sense we make ConstraintSystem support nested conditions in a future PR?

[roynalnaruto]

@ChihChengLiang

I don't understand the first point. If lhs != rhs

Sorry for the confusion. This change was actually not meant to be made. The current block of code in constrain_equal is OK. I included the change in a previous commit while testing some other constraints, but missed to realise later (after finishing the debugging) that this part was not an issue. I have reverted that change :)

Do you think it makes sense we make ConstraintSystem support nested conditions in a future PR?

I don't think it's necessary, since we are already able to concisely define our circuit (in the copy-circuit) without really needing nested conditions support. It's also in line with the zkevm-circuits repo's constraint builder

[ChihChengLiang]

LGTM, modulo a small question.

[ChihChengLiang]

Why don't we use with cs.condition(rows[0].is_tx_log): here

[roynalnaruto]

Yes, I've documented the reason on top.

If we use:

with cs.condition(cond.expr()) as cs:
    cs.constrain_equal(foo(), bar.expr())

then the foo() is still computed (even if cond is 0), just that the constraint will be satisfied.

In the case of TxLog, the addr is a very large number:

addr += (int(TxLogFieldTag.Data) << 32) + (log_id << 48)

And the lt function would panic at the assertion failure, since it's only meant to allow up to N_BYTES_MEMORY_ADDRESS = 5 bytes (it's meant to check addr < src_addr_end kind of a constraint).

Hence I used an if instead of adding a cs.condition

[ChihChengLiang]

Oh, I see. What do you think we change the code comment from We skip tx_log because: to We use an if instead of adding a cs.condition?

[han0110]

LGTM! Nice fix.