RETURN gadget for non-create calls

[z2trillion]

This gadget implements the specs in https://github.com/scroll-tech/zkevm-specs/blob/master/specs/opcode/F3RETURN.md, except for "A. Returns the specified memory chunk as deployment code." I plan to add the copy lookup for code deployments in a follow up PR.

It also does handles the REVERT opcode, which is generally similar to RETURN.

TODO: add REVERT spec.

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1203088778995027

[ed255]

In general looks good! I noticed 2 things that may be missing (the memory expansion gas cost, and state transition when root), and I also added a few questions.

[ed255]

LGTM! Very nice work! 😃

[ed255]

@roynalnaruto could you help with a second review to this PR? Thanks!

[z2trillion]

#817 broke the test here, but I don't know why.

[ed255]

#817 broke the test here, but I don't know why.

Ah, the test that fails is evm_circuit::execution::r#return::test::test_return_nonroot. Maybe there's a bug in the memory reconstruction logic somewhere?

[ed255]

I think I found the bug, we were reading from geth_step memory in the circuit input builder, but that's not populated when enable_memory = false, so we should always avoid it and rebuild the memory / memory.length. This should fix it:

diff --git a/bus-mapping/src/circuit_input_builder/input_state_ref.rs b/bus-mapping/src/circuit_input_builder/input_state_ref.rs
index f243f0cb..3fe96207 100644
--- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs
+++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs
@@ -889,10 +889,7 @@ impl<'a> CircuitInputStateRef<'a> {
                 geth_step_next.stack.stack_pointer().0.into(),
             ),
             (CallContextField::GasLeft, caller_gas_left.into()),
-            (
-                CallContextField::MemorySize,
-                geth_step_next.memory.word_size().into(),
-            ),
+            (CallContextField::MemorySize, next_memory_word_size.into()),
             (
                 CallContextField::ReversibleWriteCounter,
                 self.caller_ctx()?.reversible_write_counter.into(),

@z2trillion can you try this change, along with enable_memory: false in LoggerConfig::default()?

[z2trillion]

I think I found the bug, we were reading from geth_step memory in the circuit input builder, but that's not populated when enable_memory = false, so we should always avoid it and rebuild the memory / memory.length. This should fix it:

diff --git a/bus-mapping/src/circuit_input_builder/input_state_ref.rs b/bus-mapping/src/circuit_input_builder/input_state_ref.rs
index f243f0cb..3fe96207 100644
--- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs
+++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs
@@ -889,10 +889,7 @@ impl<'a> CircuitInputStateRef<'a> {
                 geth_step_next.stack.stack_pointer().0.into(),
             ),
             (CallContextField::GasLeft, caller_gas_left.into()),
-            (
-                CallContextField::MemorySize,
-                geth_step_next.memory.word_size().into(),
-            ),
+            (CallContextField::MemorySize, next_memory_word_size.into()),
             (
                 CallContextField::ReversibleWriteCounter,
                 self.caller_ctx()?.reversible_write_counter.into(),

@z2trillion can you try this change, along with enable_memory: false in LoggerConfig::default()?

Thanks for finding the spot with the error. The correct new value is self.caller_ctx()?.memory.word_size().into(),

[roynalnaruto]

Everything looks good to me! I have a comment though, with regards to is_success not being constrained as boolean (in the ReturnGadget)