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

bug(super circuit): evm circuit / rw table / mpt table / block table assigned twice #987

Closed
lispc opened this issue Dec 14, 2022 · 6 comments · Fixed by #1024
Closed

bug(super circuit): evm circuit / rw table / mpt table / block table assigned twice #987

lispc opened this issue Dec 14, 2022 · 6 comments · Fixed by #1024
Labels
T-bug Type: bug

Comments

@lispc
Copy link
Collaborator

lispc commented Dec 14, 2022

maybe this line should be removed

self.evm_circuit.synthesize(config.evm_circuit, layouter)?;

Assigning to same columns in two region, can result the assigned "matrix" repeat twice vertically. At least it costs twice rows without any furthur problems if any

@han0110
Copy link
Collaborator

han0110 commented Dec 14, 2022

Yes I think this is duplicated and should be removed, but I wonder how current SuperCircuit pass the integration test, I guess it's because q_usable is not enabled in the final row (the row right after q_last_step), so the transition from EndBlock to BeginTx is not causing failure.

@lispc
Copy link
Collaborator Author

lispc commented Dec 15, 2022

oh i find rw_table / mpt_table also assigned twice in super circuit, once in state circuit, once standalone.. Will fix them next PR..

@lispc lispc changed the title bug(super circuit): evm circuit assigned twice bug(super circuit): evm circuit / rw table / mpt table / block table assigned twice Dec 15, 2022
@ed255 ed255 added the T-bug Type: bug label Jan 4, 2023
@SuccinctPaul
Copy link
Contributor

Hi, all.
I've seen the some table(like exp_table) can be assigned in two ways. Is this a bug?

#984 (comment)

@kunxian-xia
Copy link
Contributor

kunxian-xia commented Jan 5, 2023

It's not a bug, as the EVM test circuit does not include the synthesize_sub of exp circuit.

@SuccinctPaul
Copy link
Contributor

SuccinctPaul commented Jan 5, 2023

It's not a bug, as the EVM test circuit does not include the synthesize_sub of exp circuit.

The above seems like will be assigned twice in non-test situation.

@kunxian-xia
Copy link
Contributor

After a quick search, the only place that exp_table.load function is called is in the EVM test circuit.

CPerezz added a commit that referenced this issue Jan 5, 2023
The table is loaded now in the SuperCircuit and the double-assignment is
removed by removing the assignment from the StateCircuit `synthesize_sub` method.

Resolves: #987
CPerezz added a commit that referenced this issue Jan 5, 2023
* fix: Remove duplicate assignment of evm circuit

* fix: Remove mpt table load from StateCircuit

The table is loaded now in the SuperCircuit and the double-assignment is
removed by removing the assignment from the StateCircuit `synthesize_sub` method.

Resolves: #987

* fix: Add empty row in MPT table to prevent testing panics

As pointed by @lispc in
#1024 (comment),
Halo2 currently has a bug which prevents it to render correctly the
errors for an empty region with a `ConstraintError` on it.

An issue has been filled for this so that we can fix it in
privacy-scaling-explorations/halo2#117.

Meanwhile a new tag is not released for Halo2 with a fix, this fix will
temporarily be needed and a reminder to remove it has been created in #1032
noel2004 pushed a commit to noel2004/zkevm-circuits that referenced this issue Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants