Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

add error pruning on step_first/step_last to avoid use against lookup #1664

Merged

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented Oct 20, 2023

Description

This works

cb.step_first(|cb| {
            cb.require_equal("xxxxx", A, B); // it works!
});

However it doesnt works on any lookup wrap by step, e.g.

cb.step_first(|cb| {
       cb.call_context_lookup_write(Some(call_id.expr()), field_tag, value); // lookup will happen on any step other than step 1
});

The reason is because lookup constraints expression not including location selector when query lookup cell.
https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs#L1489-L1496
While the location selector are created in execution.rs at later phase

let sel_step: &dyn Fn(&mut VirtualCells<F>) -> Expression<F> =
&|meta| meta.query_advice(q_step, Rotation::cur());
let sel_step_first: &dyn Fn(&mut VirtualCells<F>) -> Expression<F> =
&|meta| meta.query_selector(q_step_first);
let sel_step_last: &dyn Fn(&mut VirtualCells<F>) -> Expression<F> =
&|meta| meta.query_selector(q_step_last);
let sel_not_step_last: &dyn Fn(&mut VirtualCells<F>) -> Expression<F> = &|meta| {
meta.query_advice(q_step, Rotation::cur()) * not::expr(meta.query_selector(q_step_last))
};
for (selector, constraints) in [
(sel_step, constraints.step),
(sel_step_first, constraints.step_first),
(sel_step_last, constraints.step_last),
(sel_not_step_last, constraints.not_step_last),
] {
if !constraints.is_empty() {
meta.create_gate(name, |meta| {
let q_usable = meta.query_selector(q_usable);
let selector = selector(meta);
constraints.into_iter().map(move |(name, constraint)| {
(name, q_usable.clone() * selector.clone() * constraint)
})
});
}
}

when converting constraints to custom gate. This works for normal constraints but not works for lookup expression.

This PR add error pruning step to avoid lookup usage against step_first/step_last

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

@hero78119 hero78119 marked this pull request as draft October 20, 2023 07:19
@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Oct 20, 2023
@hero78119 hero78119 force-pushed the sm/exp_fix_cb branch 2 times, most recently from f5c9d19 to 5ed683a Compare October 23, 2023 04:18
@hero78119 hero78119 changed the title fix constraint location missing on lookup expression add error pruning on step_first/step_last to avoid use against lookup Oct 23, 2023
@hero78119 hero78119 marked this pull request as ready for review October 23, 2023 04:19
Copy link
Collaborator

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

@KimiWu123 KimiWu123 self-requested a review October 23, 2023 05:50
Copy link
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hero78119 hero78119 added this pull request to the merge queue Oct 23, 2023
Merged via the queue into privacy-scaling-explorations:main with commit eff327f Oct 23, 2023
13 checks passed
@hero78119 hero78119 deleted the sm/exp_fix_cb branch October 23, 2023 06:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants