Bytecode, and a collection of bytecodes. #1529

ChihChengLiang · 2023-07-13T16:22:37Z

Description

We revisit the handling of the bytecodes in the codebase. We reduce manual manipulation of bytes and use a more streamlined API with eth_types::Bytecode and bus_mapping::CodeDB.

Issue Link

Missed out work in #1391

Type of change

Refactor

Removed witness::bytecode module.
Extend eth_types::Bytecode and bus_mapping::CodeDB.
- eth_types::Bytecode handles a single bytecode instance, and
- CodeDB handles a group of bytecode instances
rewrite the bytecode circuit witness assignment.
rewrite the bytecode circuit tests, so that overwriting will not be included in the production code.
deduplicated the Mock account testing code.

Rationale

I kept the scope of the change small to reduce the review burden.
I will add more rationale with review comments.

bus-mapping/src/state_db.rs

ChihChengLiang · 2023-07-13T16:34:04Z

eth-types/src/bytecode.rs

+            .code()
+            .iter()
+            .cloned()
+            .chain(0u8..((32 - len % 32) as u8))


I keep the original code from tracer tests. But I keep wondering if we should pad only zeros but not 0...something

oh right, This should a list of zeroes, not a sequence!
I guess most of the times it works because the first zero would be a STOP, so the rest are ignored. And also the although in memory it's multiples of 32, when using this bytecode in memory a length is specified, so the padding will be unused. But I still think it's confusing not having just zeroes!

I think it's worth changing this.

eth-types/src/geth_types.rs

ChihChengLiang · 2023-07-13T16:36:26Z

testool/src/statetest/executor.rs

-                std::borrow::Cow::Owned(Vec::new())
+                vec![]
            } else {
-                std::borrow::Cow::Borrowed(&builder.code_db.0[&actual.code_hash])


I don't get why we currently need std::borrow::Cow::Owned and std::borrow::Cow::Borrowed

Based on the usage of Borrowed/Owned It's can allow auto lazily clone only on write happened. It's for memory usage optimisation. Maybe it's still valuable to keep it in this place since bytecode might be pretty longer and introduce memory overhead

I discussed this with @ed255. We concluded that the Cow was there to prevent cloning twice.

Cow is used to keep actual_code a reference to the byte vector so that we don't copy bytecodes from codeDB here.

The line found: Bytes::from(actual_code.to_vec()), below clones bytes with the to_vec method.

I resolved with another method that only clones once.

ChihChengLiang · 2023-07-13T16:38:53Z

zkevm-circuits/src/bytecode_circuit/circuit.rs

        challenges: &Challenges<Value<F>>,
-        fail_fast: bool,


I removed fail_fast because it was never triggered.

ChihChengLiang · 2023-07-13T16:39:17Z

zkevm-circuits/src/bytecode_circuit/circuit.rs

@@ -484,13 +567,12 @@ impl<F: Field> BytecodeCircuitConfig<F> {
        &self,
        layouter: &mut impl Layouter<F>,
        size: usize,
-        witness: &[UnrolledBytecode<F>],
-        overwrite: &UnrolledBytecode<F>,


Overwrite is gone in the assignment code.

zkevm-circuits/src/bytecode_circuit/circuit.rs

hero78119

Just have a first pass, all changes outside bytecode circuit LGTM! thanks for using new api to clean up codebase quite a lot!

Will have second review on bytecode circuit logic thoroughly sooner.

eth-types/src/geth_types.rs

hero78119 · 2023-07-20T05:05:00Z

testool/src/statetest/executor.rs

-                std::borrow::Cow::Owned(Vec::new())
+                vec![]
            } else {
-                std::borrow::Cow::Borrowed(&builder.code_db.0[&actual.code_hash])


Based on the usage of Borrowed/Owned It's can allow auto lazily clone only on write happened. It's for memory usage optimisation. Maybe it's still valuable to keep it in this place since bytecode might be pretty longer and introduce memory overhead

bus-mapping/src/state_db.rs

hero78119

LGTM! The rewrite of bytecode witness assignment and unify padding/normal row into single set_row api is awesome, plus the bytecode test api reorganized.

Just have few nitpick regarding to function naming, and return type suggestion.

bus-mapping/src/state_db.rs

zkevm-circuits/src/bytecode_circuit/test.rs

ed255

Overall LGTM! Very nice clean up 🎉
I left 3 comments, please take a look!

eth-types/src/bytecode.rs

ed255 · 2023-07-24T12:54:37Z

eth-types/src/bytecode.rs

+            .code()
+            .iter()
+            .cloned()
+            .chain(0u8..((32 - len % 32) as u8))


oh right, This should a list of zeroes, not a sequence!
I guess most of the times it works because the first zero would be a STOP, so the rest are ignored. And also the although in memory it's multiples of 32, when using this bytecode in memory a length is specified, so the padding will be unused. But I still think it's confusing not having just zeroes!

I think it's worth changing this.

ed255 · 2023-07-24T12:56:24Z

eth-types/src/geth_types.rs

+            address: Address::repeat_byte(0xff),
+            code: code.into(),
+            nonce: U64::from(!is_empty as u64),
+            balance: if is_empty { 0 } else { 0xdeadbeefu64 }.into(),


From this I think this function generates an account that is either empty, or has code and positive balance. So I would change the function documentation accordingly, and maybe rename the function to mock_code_balance, or something that doesn't imply "code or balance".

Nice catch! I renamed the function and updated the documentation 2228e4d

ed255

LGTM! Great clean up 👯‍♂️

ChihChengLiang marked this pull request as ready for review July 13, 2023 16:23

ChihChengLiang commented Jul 13, 2023

View reviewed changes

hero78119 self-requested a review July 20, 2023 06:25

hero78119 reviewed Jul 20, 2023

View reviewed changes

ChihChengLiang force-pushed the prune-bytecode-before-bytecode-circuit branch from d9f720a to 84bc322 Compare July 21, 2023 14:12

ed255 self-requested a review July 24, 2023 12:39

hero78119 approved these changes Jul 24, 2023

View reviewed changes

bus-mapping/src/state_db.rs Show resolved Hide resolved

bus-mapping/src/state_db.rs Outdated Show resolved Hide resolved

zkevm-circuits/src/bytecode_circuit/test.rs Outdated Show resolved Hide resolved

ed255 reviewed Jul 24, 2023

View reviewed changes

ChihChengLiang added 17 commits July 24, 2023 16:08

prune bytecode

cb0ba31

replace with bytecodeCollection

c1c8df4

mino

1419cd2

get some more sense

937bffb

proper API call

b1c7b22

from codedb

59b11f3

WIP new idea on bytecode circuit

32c49cd

simplify padding row

d7a9614

simplify bytecode assignment

95efabe

WIP in the middle of no where

58e81de

getting somewhere

6d863a0

testing style

ee348cb

offset fixed

ed2cdc1

fixed most of the tests

4648764

clippy fix

b2101fd

minor

408f110

bytecode unroller can go

2c6cfd8

ChihChengLiang added 20 commits July 24, 2023 16:08

minor

63c9919

deduplication of storing code to mem

aeb16f6

deadcode

2575254

fixed circuit benchmark

7a094e5

bytecode unroller can go

407fbc7

bytecode table rows can go

d2b0532

rename BytecodeCircuitAssignment

2d6e262

make codedb the same

4e0c12f

codedb visibility control

830e5d1

replace bytecodecollection with codedb

e23801c

minor

4565ce0

minor

1270828

fixed mock 1 Ether

f51adcc

rm to_raw

a518f8c

prevent double copy

4c1b6d3

rename to get_from_u256 and get_from_h256

4d5ade2

rm get

dc2a85b

rm +3

6b5574b

doc store_code_to_mem and pad only zeros

3abae63

rename mock_code_balance

2228e4d

ChihChengLiang force-pushed the prune-bytecode-before-bytecode-circuit branch from 84bc322 to 2228e4d Compare July 24, 2023 14:46

ed255 approved these changes Jul 24, 2023

View reviewed changes

ChihChengLiang added this pull request to the merge queue Jul 24, 2023

Merged via the queue into main with commit 8a633f7 Jul 24, 2023
11 checks passed

ChihChengLiang deleted the prune-bytecode-before-bytecode-circuit branch July 24, 2023 16:26

ChihChengLiang mentioned this pull request Sep 12, 2023

handle gas uint overflow (ErrorGasUintOverflow) #1564

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bytecode, and a collection of bytecodes. #1529

Bytecode, and a collection of bytecodes. #1529

ChihChengLiang commented Jul 13, 2023 •

edited

Loading

ChihChengLiang Jul 13, 2023

ed255 Jul 24, 2023

ChihChengLiang Jul 13, 2023

hero78119 Jul 20, 2023

ChihChengLiang Jul 21, 2023

ChihChengLiang Jul 13, 2023

ChihChengLiang Jul 13, 2023

hero78119 left a comment

hero78119 Jul 20, 2023

hero78119 left a comment

ed255 left a comment

ed255 Jul 24, 2023

ed255 Jul 24, 2023

ChihChengLiang Jul 24, 2023

ed255 left a comment

Bytecode, and a collection of bytecodes. #1529

Bytecode, and a collection of bytecodes. #1529

Conversation

ChihChengLiang commented Jul 13, 2023 • edited Loading

Description

Issue Link

Type of change

Contents

Rationale

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hero78119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hero78119 left a comment

Choose a reason for hiding this comment

ed255 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ed255 left a comment

Choose a reason for hiding this comment

ChihChengLiang commented Jul 13, 2023 •

edited

Loading