Mason/bytecode refactor 2 #1402

z2trillion · 2024-08-21T16:17:08Z

Description

[PR description]

Issue Link

[link issue here]

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

Rationale

[design decisions and extended information]

How Has This Been Tested?

[explanation]

How to fill a PR description

Please give a concise description of your PR.

The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request.

MUST: Reference the issue to resolve

Single responsibility

Is RECOMMENDED to create single responsibility commits, but not mandatory.

Anyway, you MUST enumerate the changes in a unitary way, e.g.

This PR contains:
- Cleanup of xxxx, yyyy
- Changed xxxx to yyyy in order to bla bla
- Added xxxx function to ...
- Refactored ....

Design choices

RECOMMENDED to:

What types of design choices did you face?
What decisions you have made?
Any valuable information that could help reviewers to think critically

DreamWuGit · 2024-08-23T07:48:27Z

bus-mapping/src/evm/opcodes/codecopy.rs

@@ -69,7 +69,7 @@ fn gen_copy_event(
        state.gen_copy_steps_for_bytecode(exec_step, &bytecode, src_addr, dst_addr, length)?;

    Ok(CopyEvent {
-        src_type: CopyDataType::Bytecode,
+        src_type: CopyDataType::Bytecode(Default::default()),


hh, have tried similar solution at earlier time when starting to implement byte code refractor:
added is_first_bytecode info into CopyEvent and set it to correct value in block_convert method. but what not good of the solution are:

enlarge changing scope, copy event related files (like files under evm/opcodes folder here)

more complex code: first time CopyDataType::Bytecode not correct, correct it in different place.

so later changed to remain CopyEvent unchanged(no any bus mapping related changes), only get the bytecode information when required(CopyTable::assignment).

first time CopyDataType::Bytecode not correct, correct it in different place.
I think "fixing" it in block_convert is fine, because that's also the place where we partition the bytecodes. we partition the bytecodes at https://github.com/scroll-tech/zkevm-circuits/pull/1402/files#diff-e71dae4a71b5661aaa2385201c4554b7d56790191c6e5d0a54ebdcac4c63dd92R593 and then two lines after, we update the copy events.

if we can avoid to set it twice, would that be better?

yes, that would be better. i'm not sure it's possible though, since the bus code mapping can't know which tables the bytecodes will end up in until the whole block has been seen.

keep CopyEvent unchanged and only get the bytecode table information when required(CopyTable::assignment).

DreamWuGit · 2024-08-23T07:56:31Z

zkevm-circuits/src/bytecode_circuit/circuit.rs

-                first_bytecodes
-                    .iter()
-                    .map(|bytecode| bytecode.bytes.len() + 1)
+        #[cfg(feature = "dual_bytecode")]


one of bytecode_map functionality is used to check if configure feature dual_bytecode. code like:

if bytecode_map.is_some(){
// enable dual_bytecode` feature case
}else{
// default : no feature case
}

this can save multiple code cfg(feature) statement like used here.
#[cfg(feature = "dual_bytecode")] {
....
}
#[cfg(not(feature = "dual_bytecode"))]{
.....
}
not let many #[cfg(feature) code mess up the codes.

it's better to directly check using #[cfg(feature = "dual_bytecode") than checking that bytecode_map is None.

checking using #[cfg(feature = "dual_bytecode") is done at compile time, which ensures that both cases are handled correctly, since we check with and without the dual_bytecode feature enabled. This prevents other people from mistakenly setting bytecode_map = Some(...) when the dual_bytecode feature is enabled.

At the beginning, we tried to make all dual_bytecode related changes to use #[cfg(feature = "dual_bytecode")]，so codes are identical to old one bytecode if not enable feature = "dual_bytecode" . However, during the refactoring stage, we realized that having many feature cfg checking codes, even though they are at compile time, are not ideal for long-term code maintenance purposes. tried to reduce most of them later, including making "is_first_bytecode_table" a default (not feature-related, with a default value of true), as long as it is compatible with existing functionality.

DreamWuGit · 2024-08-23T09:15:24Z

zkevm-circuits/src/witness/block.rs

-
-        (first_subcircuit_bytecodes, second_subcircuit_bytecodes)
+        self.bytecodes.get(code_hash).map_or(true, |b| {
+            #[cfg(feature = "dual_bytecode")]


this is also what i mean feature declare statement.
#[cfg(feature = "dual_bytecode")] {
....
}
#[cfg(not(feature = "dual_bytecode"))]{
.....
}

zkevm-circuits/src/witness/bytecode.rs

DreamWuGit · 2024-08-23T09:21:19Z

zkevm-circuits/src/witness/block.rs

-pub fn get_bytecodes(code_db: &CodeDB) -> BTreeMap<Word, Bytecode> {
+// Generate bytecode map from CodeDB.
+fn get_bytecodes(code_db: &CodeDB) -> BTreeMap<Word, Bytecode> {
+    #[cfg(feature = "dual_bytecode")]


more feature configure declare required and the code behind as well this way.

DreamWuGit · 2024-08-23T10:19:36Z

overall, will pick some code into original PR but remain bytecode_map for above reasons.

lispc · 2024-08-29T07:19:16Z

in general, the dual bytecode table design is more like a tmp trick to reduce proving cost, not a long-term good design.

So i also prefer not add the notion "dual bytecode" into bus-mapping, and don't use CopyDataType { ... Bytecode(BytecodeTable) ... }. I want to "split the wired stuff" as a "aux" piece.

Dream pr is like "add an auxiliary component", while this pr is more like a "wired local optimum"

I will not pick suggestions here.

z2trillion added 2 commits August 21, 2024 11:47

Add table field to Bytecode

cd14842

remove unused helper functions

0fa5260

github-actions bot added CI crate-bus-mapping crate-circuit-benchmarks crate-eth-types crate-external-tracer crate-gadgets crate-geth-utils crate-integration-tests crate-keccak crate-mock crate-prover crate-zkevm-circuits T-bench labels Aug 21, 2024

z2trillion changed the base branch from refactor/bytecode to bytecode_refactor August 21, 2024 16:17

Merge branch 'bytecode_refactor' into mason/bytecode_refactor_2

82fe918

github-actions bot removed crate-integration-tests crate-prover CI crate-circuit-benchmarks T-bench crate-gadgets crate-keccak crate-eth-types crate-geth-utils crate-external-tracer crate-mock labels Aug 21, 2024

z2trillion mentioned this pull request Aug 21, 2024

opt: add another bytecode circuit to improve super circuit capacity #1368

Merged

22 tasks

rename table to in_first_table

ba059ad

z2trillion added 4 commits August 21, 2024 12:45

fix todo

8a66105

Add new copy data type

b263d5e

remove dead code

3a64cda

update comment

39e9ec7

z2trillion marked this pull request as ready for review August 21, 2024 20:17

fix build when dual_bytecode not set

d7cd21d

z2trillion force-pushed the mason/bytecode_refactor_2 branch from 319780d to d7cd21d Compare August 22, 2024 03:48

z2trillion added 2 commits August 22, 2024 15:01

fix default build

1d2a4b5

switch to bytetable enum

97d377e

DreamWuGit reviewed Aug 23, 2024

View reviewed changes

zkevm-circuits/src/witness/bytecode.rs Show resolved Hide resolved

DreamWuGit reviewed Aug 23, 2024

View reviewed changes

z2trillion added 3 commits August 23, 2024 11:19

clean up

c3759f3

remove unneeded clones

ba7f3b8

fix doc string

d10fb39

lispc closed this Aug 29, 2024

z2trillion deleted the mason/bytecode_refactor_2 branch August 29, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mason/bytecode refactor 2 #1402

Mason/bytecode refactor 2 #1402

z2trillion commented Aug 21, 2024

DreamWuGit Aug 23, 2024 •

edited

Loading

z2trillion Aug 23, 2024

DreamWuGit Aug 26, 2024

z2trillion Aug 26, 2024

DreamWuGit Aug 26, 2024

DreamWuGit Aug 23, 2024 •

edited

Loading

z2trillion Aug 23, 2024

DreamWuGit Aug 26, 2024 •

edited

Loading

DreamWuGit Aug 23, 2024

DreamWuGit Aug 23, 2024 •

edited

Loading

DreamWuGit commented Aug 23, 2024

lispc commented Aug 29, 2024

Mason/bytecode refactor 2 #1402

Mason/bytecode refactor 2 #1402

Conversation

z2trillion commented Aug 21, 2024

Description

Issue Link

Type of change

Contents

Rationale

How Has This Been Tested?

How to fill a PR description

Single responsibility

Design choices

DreamWuGit Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

z2trillion Aug 23, 2024

Choose a reason for hiding this comment

DreamWuGit Aug 26, 2024

Choose a reason for hiding this comment

z2trillion Aug 26, 2024

Choose a reason for hiding this comment

DreamWuGit Aug 26, 2024

Choose a reason for hiding this comment

DreamWuGit Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

z2trillion Aug 23, 2024

Choose a reason for hiding this comment

DreamWuGit Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

DreamWuGit Aug 23, 2024

Choose a reason for hiding this comment

DreamWuGit Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

DreamWuGit commented Aug 23, 2024

lispc commented Aug 29, 2024

DreamWuGit Aug 23, 2024 •

edited

Loading

DreamWuGit Aug 23, 2024 •

edited

Loading

DreamWuGit Aug 26, 2024 •

edited

Loading

DreamWuGit Aug 23, 2024 •

edited

Loading