Bytecode Circuit: Refactor word_rlc into word lo/hi

[leolara]

Description

Bytecode Circuit: Refactor word_rlc into word lo/hi

Issue Link

Closes #1380

Type of change

• Refactor
• Lo/Hi

Contents

Bytecode Circuit: Refactor word_rlc into word lo/hi

Rationale

In Issue

How Has This Been Tested?

Running unit tests

[leolara]

Should I remove the hash_rlc references?

[leolara]

The test do not pass I think because the keccak does not has the hash_word yet.

[hero78119]

The test do not pass I think because the keccak does not has the hash_word yet.

Keccak already have hash_word type, https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/word-lo-hi/zkevm-circuits/src/table/keccak_table.rs#L13 and with dev_load it should be able to handle lookup from bytecode circuit . I think we should be able to fix unitttest in bytecode circuit in this pr.

[leolara]

I fixed the tests by deleting the lookup to the output_rlc, from the comment before I checked dev_load and saw that this column was not being assigned. Hence, I will remove the hash_rlc references

[leolara]

Now I have eliminated the code_hash(rlc) and renamed the code_hash_word to code_hash.

[leolara]

@ChihChengLiang @hero78119 please review again, I think it is good now

[hero78119]

Except for few word type usage, other LGTM!

[ChihChengLiang]

LGTM after the comments are addressed.

[ChihChengLiang]

We don't need to convert to 32 limbs of expressions and then merge to 2 limbs.

Suggested change

	let empty_hash_word: Word<Expression<F>> =
	Word32::new(EMPTY_CODE_HASH_LE.map(|v| Expression::Constant(F::from(v as u64))))
	.to_word();
	let empty_hash_word: Word<Expression<F>> = Word::from(EMPTY_CODE_HASH).to_word();

[leolara]

Word::from(CodeDB::empty_code_hash()) will produce a Word<Value> not a Word<Expression>.

[leolara]

Do you want me to implement to_word trait for Word<Value>? How should it behave if Value::unknown?

[leolara]

@ChihChengLiang I have merged the base branch and that has substituted the From by From which is good

[hero78119]

Hi @leolara, here is another pr to add subcircuit unittest back one by one word-lo-hi branch #1488 I think bytecode_circuit pr can depend on this
Appreciate you can also help to review and hopefully we can merge #1488 pr first 🙏

[leolara]

@hero78119 I can add tests in another Pr if that is what you mean, I don't understand why this PR depend on the other one? I wouldn't prolong this PR much more, it is getting long and I think that could create more conflicts.

[leolara]

@ChihChengLiang please check again :-)

[hero78119]

Notice there are some error message when running unittest related to bytecode_circuit for this pr

cargo test --verbose --release --all --all-features --exclude integration-tests --exclude circuit-benchmarks --package zkevm-circuits --lib -- bytecode_circuit

some error msg snippet

...
[2023-06-21T08:33:29Z ERROR zkevm_circuits::bytecode_circuit::test] Constraint 0 ('cur.is_code == (cur.push_data_left == 0)') in gate 5 ('Byte row') is not satisfied in Region 2 ('assign bytecode') at offset 4
    - Column('Advice', 0 - tag)@0 = 1
    - Column('Advice', 2 - is_code)@0 = 0
    - Column('Fixed', 0 - BYTECODE_q_enable)@0 = 1
    - Column('Fixed', 2 - BYTECODE_q_last)@0 = 0

[2023-06-21T08:33:29Z ERROR zkevm_circuits::bytecode_circuit::test] Constraint 5 ('next.push_data_left == cur.is_code ? cur.push_data_size : cur.push_data_left - 1') in gate 8 ('Byte to Byte row') is not satisfied in Region 2 ('assign bytecode') at offset 4
    - Column('Advice', 0 - tag)@0 = 1
    - Column('Advice', 0 - tag)@1 = 1
    - Column('Advice', 2 - is_code)@0 = 0
    - Column('Advice', 15 - BYTECODE_push_data_left)@0 = 0
    - Column('Advice', 15 - BYTECODE_push_data_left)@1 = 0
    - Column('Advice', 18 - BYTECODE_push_data_size)@0 = 0
    - Column('Fixed', 0 - BYTECODE_q_enable)@0 = 1
    - Column('Fixed', 2 - BYTECODE_q_last)@0 = 0

[2023-06-21T08:33:29Z ERROR zkevm_circuits::bytecode_circuit::test] Lookup keccak256_table_lookup(cur.value_rlc, cur.length, cur.hash_word)(index: 1) is not satisfied in Region 2 ('assign bytecode') at offset 7
[2023-06-21T08:33:30Z ERROR zkevm_circuits::bytecode_circuit::test] Constraint 0 ('cur.is_code == (cur.push_data_left == 0)') in gate 5 ('Byte row') is not satisfied in Region 2 ('assign bytecode') at offset 7
    - Column('Advice', 0 - tag)@0 = 1
    - Column('Advice', 2 - is_code)@0 = 1
    - Column('Fixed', 0 - BYTECODE_q_enable)@0 = 1
    - Column('Fixed', 2 - BYTECODE_q_last)@0 = 0

test bytecode_circuit::test::bytecode_invalid_is_code ... ok
[2023-06-21T08:33:30Z ERROR zkevm_circuits::bytecode_circuit::test] Lookup push_data_size_table_lookup(cur.value, cur.push_data_size)(index: 0) is not satisfied in Region 2 ('assign bytecode') at offset 3
[2023-06-21T08:33:30Z ERROR zkevm_circuits::bytecode_circuit::test] Lookup keccak256_table_lookup(cur.value_rlc, cur.length, cur.hash_word)(index: 1) is not satisfied in Region 2 ('assign bytecode') at offset 7

please kindly help to check

[ChihChengLiang]

All good. LGTM

[leolara]

@hero78119

When I ran that comment in my local branch I get:

test result: ok. 12 passed; 0 failed; 1 ignored; 0 measured; 411 filtered out; finished in 0.53s

Those errors are from tests where the positive result is an invalid proof.

I don't see how #1488 is connected to this PR, they don't change the same files

[hero78119]

cargo test --verbose --release --all --all-features --exclude integration-tests --exclude circuit-benchmarks --package zkevm-circuits --lib -- bytecode_circuit

Hi @leolara
Are we able to fix the error msg even the test pass, to make it be less false alarm

Besides https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/word-lo-hi/.github/workflows/ci.yml#L80 here can just append bytecode_circuit so ci will run unittest related to bytecode_circuit as well. Right now most of sub circuit is covered

[ChihChengLiang]

@hero78119 I tried your script on the main branch, and it shows the same error logs. So the error logs are not specific to this pull request.

Fixing the error logs is out of the scope of this pull request. Can we fix them in future PRs?

[hero78119]

@hero78119 I tried your script on the main branch, and it shows the same error logs. So the error logs are not specific to this pull request.

Fixing the error logs is out of the scope of this pull request. Can we fix them in future PRs?

Yeah it's ok we fixed in future pr., my opinion is it will be better if we can identify the root cause. Just worry that actually error msg hide something so we can't know whether the word-lo-hi apply on bytecode_circuit break something or not.

Then one thing lefted is adding bytecode_circuit to ci then I think all good to go now

[ChihChengLiang]

@hero78119 By "adding bytecode_circuit to ci," we're saying adding bytecode_circuit to the following place right?

zkevm-circuits/.github/workflows/ci.yml

Line 80 in cb6c17c

args: --verbose --release --all --all-features --exclude integration-tests --exclude circuit-benchmarks --package zkevm-circuits --lib -- evm_circuit keccak_circuit exp_circuit pi_circuit state_circuit # tx_circuit/copy_circuit/bytecode_circuit/root_circuit ...more subcircuits

[leolara]

I think:

• The errors output is ok and the expected result of the tests
• removing the errors output is out of the scope of this task.
• adding something to CI is out of the scope of this task.

[hero78119]

I think:

• The errors output is ok and the expected result of the tests
• removing the errors output is out of the scope of this task.
• adding something to CI is out of the scope of this task.

Hi @leolara
Previous we have ci to run light test, just when work on word-lo-hi ci unittest job this job is temporarily masked. Now it's added, and each subcircuit has it responsiblilty to add it back, when unittest is passed and verify locally.

It's just one line change, maybe I didn't explain it clear. Please see @ChihChengLiang comment above #1468 (comment)

[leolara]

The light tests pass in Ci, when you add the light test back you can add bytecode_circuit. I can either:

• Add back the CI with only the bytecode_circuit, which will create a conflict with the other PR
• As the the circuit pass the tests, we can leave it as it is and in the other PR add the bytecode_circuit to CI.

I think the second option is the way to go because:

• It does not create conflict between branches
• It gets merged the oldest PR first, having PRs open for long will just lead to more conflicts

[hero78119]

Ok then all LGTM! I will add bytrcde circuit to ci later

[leolara]

@hero78119 thanks