Fix some bugs of CircuitInputBuilder#434
Conversation
805ff76 to
b247535
Compare
ed255
left a comment
There was a problem hiding this comment.
I've left two minor suggestions. Feel free to apply them or not!
LGTM, thanks for fixing this :)
There was a problem hiding this comment.
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
}
@roynalnaruto Thanks for the code snippet, I tried that once but failed, then I realized privacy-ethereum/zkevm-specs#134 has not been addressed yet, so I think it would be better to fix that in spec and then circuit. |
b247535 to
9e40b22
Compare
9e40b22 to
4d0ee80
Compare
4d0ee80 to
ef81390
Compare
|
@han0110 The above bytecode fails via bus-mapping because So we then end up with this error: |
|
Ah, I didn't expect 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-ethereum/zkevm-specs#178 added! |
This PR aims to fix some bugs of
CircuitInputBuilderto resolve #431, it includes:push_callandhandle_returnfor other call-family opcodes bydummy_gen_call_ops.call_data_sourcein structCallContextstruct forCALLDATACOPYto be able to generate memory operations and removeunimplement!. However, it doesn't work when I tried to apply to circuit testing, need more investigation (CALLDATACOPYshould read caller's memory zkevm-specs#134 also needs to be addressed).CALLDATASIZEhas itsgen_associated_opsimplemented, I tried to replace handcrafted testing data for it in circuit testing to make sure it works (and unblock ImplementationExecutionState::STOP#419).