Add debug expr in EVMConstraintBuilder

[ed255]

Description

Introduce the debug_expression method to the EVMConstraintBuilder which allows specifying an expression inside an ExecutionGadget to be evaluated and printed during assignment.

This can be very useful for debugging. With this we can print the value of cells, but also of arbitrary expressions. For example, it allows printing the evaluation of all the expressions used in a lookup (previous to the RLC).

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Contents

When calling cb.debug_expression, the expression is stored in the EVMConstraintBuilder along with a string. Later on, at assignment, whenever a step is assigned to an ExecutionState that had some debug expressions specified, those expressions will be evaluated and printed.

Example usage

In the PushGadget::configure method I include the following line:

cb.debug_expression("push.code_hash", code_hash.expr());

Then I run the a test that assigns this state (with --nocapture to see the stdout):

cargo test push_gadget_simple --release --all-features -- --nocapture

And I will get the following:

Debug expression "push.code_hash"=0x178027364c9ed08d00e38dbf197bbf65e45779c1d1b9dca1a229f7b7360892ce [offset=19, step=Op(PUSH1), expr=Advice { query_index: 591, column_index: 76, rotation: Rotation(0), phase: Phase(1) }]

[ChihChengLiang]

LGTM. I tried it locally, and it works!

[KimiWu123]

Awesome! LGTM, only one nit.

[KimiWu123]

Will is_final or is_final_step be better? Or has_next if final_step is not accurate. I understand to use is as prefix while it's a bool type but in this case is_next is not clear to know what it really means.

[ed255]

I think is_final or has_next doesn't hold the real meaning here. Let me explain why I call this is_next. Each step is assigned two times. The list of steps is iterated in a window of size 2 where we have (current_step, next_step), and we assign the next first and the current afterwards in each iteration. See

zkevm-circuits/zkevm-circuits/src/evm_circuit/execution.rs

Lines 1180 to 1206 in b6be489

	// Also set the witness of the next step.
	// These may be used in stored expressions and
	// so their witness values need to be known to be able
	// to correctly calculate the intermediate value.
	if let Some((transaction_next, call_next, step_next)) = next {
	self.assign_exec_step_int(
	region,
	offset + height,
	block,
	transaction_next,
	call_next,
	step_next,
	true,
	assign_pass,
	)?;
	}
	self.assign_exec_step_int(
	region,
	offset,
	block,
	transaction,
	call,
	step,
	false,
	assign_pass,
	)

So if we have the steps [a, b, c] we pair them like (cur=a, next=b), (cur=b, next=c), (cur=c, next=None). And we do the following assignments:

• b (next)
• a (cur)
• b (cur)
• c (next)
• c (cur)

So here the is_next means that we're assigning the next step before we assign the current steps for it's "side effects", but we'll go over and assign that step again later.

Do you have another suggestion instead of is_next that conveys this meaning?

[KimiWu123]

Thanks for your explanation, is_next makes sense to me now. :)

[hero78119]

This is nice proposed! Have try to use it and work like a charm!

Just have 2 suggestion

1. can use &'static str type instead of String (as I understand it took extra heap copy) .
2. swap first and second parameter of debug_expression(name, expr), as we used to have name go first

Here is commit bc7a15d which apply above change. Feel free to cherry-pick it :)

[ed255]

This is nice proposed! Have try to use it and work like a charm!

Just have 2 suggestion

1. can use `&'static str` type instead of `String` (as I understand it took extra heap copy) .

2. swap first and second parameter of `debug_expression(name, expr)`, as we used to have `name` go first

Thanks for the suggestions! I've swapped the argument order so that name goes first.
Regarding the first point, I agree that a String means some heap usage, but I think for debugging purposes is OK (and running the MockProver is already memory intensive, so a few more strings should be fine). Nevertheless I've updated the function to take Into<String> to make it easier to call with an str. So now you can do it either like static string:

cb.debug_expression("tx_id", tx_id.expr());
```rust
or dynamic String

cb.debug_expression(format!("limb{}", i), word.limbs[i].expr());

I think allowing the possibility for dynamic string is very useful because it allows creating strings in a loop for arrays for example.

[hero78119]

Thanks for address the reviews! Into<String> make sense to preserve more flexibility in debug purpose 👍