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

bus-mapping: fix return data offset and length bookkeeping#951

Merged
lispc merged 5 commits into
privacy-ethereum:mainfrom
pinkiebell:fix_bus
Dec 7, 2022
Merged

bus-mapping: fix return data offset and length bookkeeping#951
lispc merged 5 commits into
privacy-ethereum:mainfrom
pinkiebell:fix_bus

Conversation

@pinkiebell
Copy link
Copy Markdown
Contributor

@pinkiebell pinkiebell commented Nov 30, 2022

This resolves a problem with the returndatacopy opcode because of zero return_data.[offset, length]

@pinkiebell pinkiebell marked this pull request as ready for review November 30, 2022 08:03
@github-actions github-actions Bot added the crate-bus-mapping Issues related to the bus-mapping workspace member label Nov 30, 2022
han0110
han0110 approved these changes Nov 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

Nice catch!

@pinkiebell @CPerezz
bus-mapping: fix return data offset and length bookkeeping
d75a596 
This resolves a problem with the returndatacopy opcode because
of zero return_data.[offset, length]
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Nov 30, 2022

Just rebased so that we merge this ASAP 😄

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Nov 30, 2022

Getting errors in some RETURN/REVERT tests. We might need to take another look to it @pinkiebell
😢

@pinkiebell
Copy link
Copy Markdown
Contributor Author

pinkiebell commented Nov 30, 2022

Getting errors in some RETURN/REVERT tests. We might need to take another look to it @pinkiebell
😢

That's odd. Looks like call.return_data_offset is intended for something else 🤔

@pinkiebell
Copy link
Copy Markdown
Contributor Author

pinkiebell commented Nov 30, 2022

Well that worked. It's still a little bit confusing 🤷

han0110
han0110 approved these changes Dec 1, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

Well that worked. It's still a little bit confusing 🤷

Sorry, I was also confused, so call.return_data_offset and call.return_data_length is recording the value specified by caller (destination for return data to be copied to when call end). See:

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/83bfbef71fc006fab4a07fc8a830745e916055bc/bus-mapping/src/circuit_input_builder/input_state_ref.rs#L633-L646

@pinkiebell
Copy link
Copy Markdown
Contributor Author

pinkiebell commented Dec 1, 2022

Well that worked. It's still a little bit confusing 🤷

Sorry, I was also confused, so call.return_data_offset and call.return_data_length is recording the value specified by caller (destination for return data to be copied to when call end). See:

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/83bfbef71fc006fab4a07fc8a830745e916055bc/bus-mapping/src/circuit_input_builder/input_state_ref.rs#L633-L646

That makes more sense now 😅

@pinkiebell
Copy link
Copy Markdown
Contributor Author

pinkiebell commented Dec 5, 2022

@DreamWuGit Please review again :)

@lispc lispc merged commit 56642ad into privacy-ethereum:main Dec 7, 2022
@pinkiebell pinkiebell deleted the fix_bus branch December 7, 2022 07:32
pinkiebell added a commit to pinkiebell/zkevm-circuits that referenced this pull request Dec 15, 2022
@pinkiebell
Squashed commit of the following:
08315b8 
commit cc1e0d6
Author: z2trillion <z2trillion@users.noreply.github.com>
Date:   Thu Dec 15 04:02:49 2022 -0500

    Add TODO (privacy-ethereum#961)

    Co-authored-by: Mason Liang <mason@scroll.io>
    Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>
    Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

commit 20b4db0
Author: DreamWuGit <wwuwwei@126.com>
Date:   Thu Dec 15 15:58:43 2022 +0800

    fix dummy error handling  (privacy-ethereum#979)

    * fix dummy error

    * minor refacotr

commit 5710e30
Author: DreamWuGit <wwuwwei@126.com>
Date:   Thu Dec 15 10:15:24 2022 +0800

    fix balance test (privacy-ethereum#989)

commit 4840a5a
Author: Zhang Zhuo <mycinbrin@gmail.com>
Date:   Wed Dec 14 21:57:38 2022 +0800

    fix(evm): get_test_degree (privacy-ethereum#983)

    * fix(evm): get_test_degree

    * fmt

    * change bytecode table row estimation

commit c78c86d
Author: smtmfft <99081233+smtmfft@users.noreply.github.com>
Date:   Mon Dec 12 16:05:08 2022 +0800

    Math gadgets refactory (privacy-ethereum#920)

    * refactory is_zero

    * add a few gadget files

    * fix import warn

    * Split gadget into independent files

    * Fix lints

    * Add bye size gadget test case

    * Split tests into single function

    * Fix comments from brecht

    * update unit test organization for better self explain

    * fix review comments

    * fix review comments

    * fix review comments

    * remove useless tests

    * Update zkevm-circuits/src/evm_circuit/util/math_gadget/min_max.rs

    Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>

    * Update zkevm-circuits/src/evm_circuit/util/math_gadget/addwords.rs

    Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>

    * improve: mod dependency

    * keep fixing review comments

    * refine tests

    * Fix test framework issue

    Signed-off-by: smtmfft <smtm@taiko.xyz>

    * use macro to make better readability

    * remove dup marco def

    * use try_test macro

    * code refine

    * update test case

    * fix max carry test case

    * fix review comments

    * refine annotations

    * fix review comments

    * fix review comments

    Signed-off-by: smtmfft <smtm@taiko.xyz>
    Co-authored-by: smtmfft <smtm@taiko.xzy>
    Co-authored-by: john xu <john@taiko.xyz>
    Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>

commit ee1cfbd
Author: zengzengzenghuy <85567868+zengzengzenghuy@users.noreply.github.com>
Date:   Fri Dec 9 19:09:51 2022 +0800

    fix: typo in memory.rs (privacy-ethereum#962)

    * fix: typo in memory.rs

    * fix: typo in memory.rs

commit f444466
Author: Leo Lara <leolara@users.noreply.github.com>
Date:   Thu Dec 8 19:01:59 2022 +0700

    Integration tests to use actual prover (privacy-ethereum#878)

    Closes privacy-ethereum#789

commit a7f87b3
Author: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>
Date:   Wed Dec 7 16:34:59 2022 +0100

    CopyCircuit Challenge API integration (privacy-ethereum#965)

    * change: Port CopyCircuit to Challenge and Value APIs

    The CopyCircuit was not using the Value API for witness assignation.
    Therefore, the Challenge API has been integrated in both `configure` and
    `synthesize` steps and the `Value<F>` is the default operand.

    * change: Port Tx table to Challenge API

    Now the `TxTable` uses the `Challenges` in order to assign all the
    values on it's columns.

    * update: EVM and Super Circuits CopyTable calls

    Now that the CopyTable and CopyCircuit use the `Value` and `Challenges`
    APIs, the calls from EvmCircuit and SuperCircuit have changed and needed
    to be updated.

    * update: Cargo.lock

    * fix: Remove randomness instance from benchmarks

    * fix: Add `value` and `id` columns as SecondPhase

commit 56642ad
Author: pinkiebell <40266861+pinkiebell@users.noreply.github.com>
Date:   Wed Dec 7 02:38:23 2022 +0100

    bus-mapping: fix return data offset and length bookkeeping (privacy-ethereum#951)

    * bus-mapping: fix return data offset and length bookkeeping

    This resolves a problem with the returndatacopy opcode because
    of zero return_data.[offset, length]

    * fix conflict

    Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

commit 5def16d
Author: pinkiebell <40266861+pinkiebell@users.noreply.github.com>
Date:   Mon Dec 5 18:07:37 2022 +0100

    fix: keccak/plain: incremental update() broken (privacy-ethereum#964)

    * fix: keccak/plain: incremental update() broken

    Fixes: privacy-ethereum#922

    * always pad
noel2004 pushed a commit to noel2004/zkevm-circuits that referenced this pull request Nov 29, 2023
@roynalnaruto
another edge case (privacy-ethereum#951)
88414cc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@lispc lispc lispc approved these changes

@DreamWuGit DreamWuGit DreamWuGit approved these changes

@han0110 han0110 han0110 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

zkEVM Community Edition
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pinkiebell @CPerezz @lispc @DreamWuGit @han0110