-
Notifications
You must be signed in to change notification settings - Fork 841
Fix some bugs of CircuitInputBuilder
#434
Fix some bugs of CircuitInputBuilder
#434
Conversation
805ff76
to
b247535
Compare
There was a problem hiding this 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 :)
There was a problem hiding this 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
}
@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. |
b247535
to
9e40b22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM!
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-scaling-explorations/zkevm-specs#178 added! |
This PR aims to fix some bugs of
CircuitInputBuilder
to resolve #431, it includes:push_call
andhandle_return
for other call-family opcodes bydummy_gen_call_ops
.call_data_source
in structCallContext
struct forCALLDATACOPY
to be able to generate memory operations and removeunimplement!
. 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).CALLDATASIZE
has itsgen_associated_ops
implemented, I tried to replace handcrafted testing data for it in circuit testing to make sure it works (and unblock ImplementationExecutionState::STOP
#419).