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

Fix some bugs of CircuitInputBuilder #434

Merged

Conversation

han0110
Copy link
Contributor

@han0110 han0110 commented Apr 6, 2022

This PR aims to fix some bugs of CircuitInputBuilder to resolve #431, it includes:

  1. Doing push_call and handle_return for other call-family opcodes by dummy_gen_call_ops.
  2. Add a copy of caller's memory as call_data_source in struct CallContext struct for CALLDATACOPY to be able to generate memory operations and remove unimplement!. However, it doesn't work when I tried to apply to circuit testing, need more investigation (CALLDATACOPY should read caller's memory zkevm-specs#134 also needs to be addressed).
  3. Add address into access set when code information (e.g. length and bytes) is accessed.
  4. Since CALLDATASIZE has its gen_associated_ops implemented, I tried to replace handcrafted testing data for it in circuit testing to make sure it works (and unblock Implementation ExecutionState::STOP #419).

@han0110 han0110 requested a review from a team as a code owner April 6, 2022 15:19
@han0110 han0110 requested review from ed255 and removed request for a team April 6, 2022 15:19
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue labels Apr 6, 2022
@han0110 han0110 force-pushed the fix/circuit-input-builder branch 2 times, most recently from 805ff76 to b247535 Compare April 8, 2022 04:34
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

I've left two minor suggestions. Feel free to apply them or not!
LGTM, thanks for fixing this :)

Copy link
Collaborator

@roynalnaruto roynalnaruto left a comment

Choose a reason for hiding this comment

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

I had uncommitted changes trying to do something similar in #402, but I will wait for this to be merged before, so I can wrap up the CALLDATALOAD internal call case :)

The changes look good to me! Just a tiny suggestion, it would be better to add a test to the CALLDATACOPY opcode implementation that uses these bus-mapping updates to generate witness:

#[test]
fn calldatacopy_busmapping_internal() {
    let creator_code: Vec<u8> = hex::decode("666020600060003760005260076019F3").unwrap();
    let code = bytecode! {
        // 1. Store the following bytes to memory
        PUSH16(Word::from_big_endian(&creator_code))
        PUSH1(0x00) // offset
        MSTORE

        // 2. Create a contract with code: 6020 6000 6000 37
        PUSH1(0x10) // size
        PUSH1(0x10) // offset
        PUSH1(0x00) // value
        CREATE

        // 3. Call created contract, i.e. CALLDATACOPY (37) is in internal call
        PUSH1(0x00)   // retSize
        PUSH1(0x00)   // retOffset
        PUSH1(0x20)   // argsSize
        PUSH1(0x00)   // argsOffset
        PUSH1(0x00)   // value
        DUP6          // address
        PUSH2(0xFFFF) // gas
        CALL
    };

    // TODO: build and handle block
    // TODO: assertions on the CALLDATACOPY step
}

@han0110
Copy link
Contributor Author

han0110 commented Apr 8, 2022

it would be better to add a test to the CALLDATACOPY opcode implementation that uses these bus-mapping updates to generate witness:

@roynalnaruto Thanks for the code snippet, I tried that once but failed, then I realized privacy-scaling-explorations/zkevm-specs#134 has not been addressed yet, so I think it would be better to fix that in spec and then circuit.

@han0110 han0110 force-pushed the fix/circuit-input-builder branch from b247535 to 9e40b22 Compare April 8, 2022 15:28
Copy link
Collaborator

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

overall LGTM!

@han0110 han0110 force-pushed the fix/circuit-input-builder branch from 9e40b22 to 4d0ee80 Compare April 9, 2022 02:26
@han0110 han0110 force-pushed the fix/circuit-input-builder branch from 4d0ee80 to ef81390 Compare April 9, 2022 03:11
@icemelon icemelon merged commit 602e33d into privacy-scaling-explorations:main Apr 9, 2022
@han0110 han0110 deleted the fix/circuit-input-builder branch April 11, 2022 02:07
@roynalnaruto
Copy link
Collaborator

@han0110 The above bytecode fails via bus-mapping because CREATE does not push a call, but RETURN (soon following the CREATE opcode) pops the call_ctx while state.handle_return().

So we then end up with this error:

panicked at 'called `Result::unwrap()` on an `Err` value: InvalidGethExecTrace("Call stack is empty but call is used")'

@han0110
Copy link
Contributor Author

han0110 commented Apr 11, 2022

Ah, I didn't expect CREATE to be used so soon :( Let me try to implement a dummy_gen_create_ops in CircuitInputBuilder.

To unblock #450 first, could we just set the code of callee directly instead of deploy the callee?

@roynalnaruto
Copy link
Collaborator

To unblock #450 first, could we just set the code of callee directly instead of deploy the callee?

Yes, I am currently doing that, taking inspiration from #402 (comment).

Will wrap up the tests in #450 , although the bus-mapping impl has been fixed, as well as privacy-scaling-explorations/zkevm-specs#178 added!

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 T-opcode Type: opcode-related and focused PR/Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testnet: Call stack is empty but call is used
4 participants