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

fix multi copy step data index#510

Merged
icemelon merged 7 commits into
privacy-ethereum:mainfrom
scroll-tech:fix_multi_copylog_step
May 23, 2022
Merged

fix multi copy step data index#510
icemelon merged 7 commits into
privacy-ethereum:mainfrom
scroll-tech:fix_multi_copylog_step

Conversation

@DreamWuGit

@DreamWuGit DreamWuGit commented May 16, 2022

Copy link
Copy Markdown
Collaborator

this PR can be part of #476 when doing multiple copy step in buss mapping , for TxLogFieldTag::Data index should start from zero and consecutively increasing in multi copy log steps. i.e. total copies size is 66 bytes, data index should be monotone increasing by 1 from 0 across all three copy steps.

step data_start_index copy range
CopyStep1 0 0 --> 31
CopyStep2 32 32 --> 63
CopyStep2 64 64 -> 65

while old is as below though Data value is correct !

step data_start_index copy range
CopyStep1 0 0 --> 31
CopyStep2 0 0 --> 31
CopyStep2 0 0 --> 31

found in issue #511

@DreamWuGit DreamWuGit requested review from a team and ChihChengLiang and removed request for a team May 16, 2022 08:10
@github-actions github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels May 16, 2022
@DreamWuGit DreamWuGit force-pushed the fix_multi_copylog_step branch from fdf8c88 to e8253bb Compare May 16, 2022 08:18
This was referenced May 16, 2022
Comment thread zkevm-circuits/src/evm_circuit/witness.rs

@icemelon icemelon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should add a test case that could cover the new fix code

@DreamWuGit DreamWuGit force-pushed the fix_multi_copylog_step branch from 0106092 to b48580d Compare May 18, 2022 03:48
@DreamWuGit

Copy link
Copy Markdown
Collaborator Author

should add a test case that could cover the new fix code

tests of copy_to_log_multi_step cover this, the incoming log test with multiple steps can indirect check this !

@ed255 ed255 self-requested a review May 18, 2022 09:08
Comment thread bus-mapping/src/circuit_input_builder/execution.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/copy_to_log.rs
@DreamWuGit DreamWuGit force-pushed the fix_multi_copylog_step branch from f7c8b38 to e592c5e Compare May 19, 2022 03:20

@ed255 ed255 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix :)

@icemelon icemelon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@icemelon icemelon merged commit a5b1069 into privacy-ethereum:main May 23, 2022
@icemelon icemelon deleted the fix_multi_copylog_step branch May 23, 2022 06:16
lispc pushed a commit that referenced this pull request Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants