Fix: Memory.store_n and Helpers.slice_data for border cases (#217)

<!--- Please provide a general summary of your changes in the title
above -->

## Pull request type

<!-- Please try to limit your pull request to one type, submit multiple
pull requests if needed. -->

Please check the type of change your PR introduces:

- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

## What is the current behavior?

<!-- Please describe the current behavior that you are modifying, or
link to a relevant issue. -->

Adding test cases to the opcode 0x39 led to 2 failures. The failures
have been identified to be linked to corner cases in Memory.store_n and
Helpers.slice_data. Such a corner case is for example when taking a
slice with offset out of bound.

Issue Number: related to 98

## What is the new behavior?

<!-- Please describe the behavior or changes that are being added by
this PR. -->

Tests for the CODECOPY (0x39) opcode have been extended with more cases
taking different code-paths.

Memory.store_n and Helpers.slice_data have been reimplemented to cover
all cases.

Cases for slice_data:

1. slice fully overlaps with data: data_offset + slice_len <= data_len
2. slice partially overlaps with data: data_offset < data_len <
data_offset + slice_len
  3. slice does not overlap with data: data_len <= data_offset

For Memory.store_n, the new memory is composed of 3 parts:

1. Head: current bytes before offset. Might be zero padded in case len <
offset
2. New elements: always inserted between offset and offset + element_len
3. Tail: current bytes after the new elements. Might be zero padded in
case total memory length is not a multiple of 32

## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Co-authored-by: Thomas Zamojski <thomas.zamojski@datastorm.fr>

docs/supported_opcodes.md

@@ -56,7 +56,7 @@ This document describes the opcodes supported by Kakarot.
 | 0x36         | CALLDATASIZE   | Get size of input data in current environment                                     | ✅          |
 | 0x37         | CALLDATACOPY   | Copy input data in current environment to memory                                  | ✅          |
 | 0x38         | CODESIZE       | Get size of code running in current environment                                   | ✅          |
-| 0x39         | CODECOPY       | Copy code running in current environment to memory                                |             |
+| 0x39         | CODECOPY       | Copy code running in current environment to memory                                | ✅          |
 | 0x3a         | GASPRICE       | Get price of gas in current environment                                           |             |
 | 0x3b         | EXTCODESIZE    | Get size of an account's code                                                     |             |
 | 0x3c         | EXTCODECOPY    | Copy an account's code to memory                                                  |             |

src/kakarot/instructions.cairo

@@ -248,6 +248,8 @@ namespace EVMInstructions {
         add_instruction(instructions, 0x37, EnvironmentalInformation.exec_calldatacopy);
         // 0x38 - CODESIZE
         add_instruction(instructions, 0x38, EnvironmentalInformation.exec_codesize);
+        // 0x39 - CODECOPY
+        add_instruction(instructions, 0x39, EnvironmentalInformation.exec_codecopy);
         // 0x3d - RETURNDATASIZE
         add_instruction(instructions, 0x3d, EnvironmentalInformation.exec_returndatasize);
 

src/kakarot/instructions/environmental_information.cairo

@@ -28,6 +28,7 @@ from kakarot.interfaces.interfaces import IEth, IResgistry
 namespace EnvironmentalInformation {
     // Define constants.
     const GAS_COST_CODESIZE = 2;
+    const GAS_COST_CODECOPY = 3;
     const GAS_COST_CALLER = 2;
     const GAS_COST_RETURNDATASIZE = 2;
     const GAS_COST_CALLDATALOAD = 3;
@@ -331,4 +332,55 @@ namespace EnvironmentalInformation {
         let ctx = ExecutionContext.increment_gas_used(ctx, GAS_COST_CALLDATACOPY);
         return ctx;
     }
+
+    // @notice CODECOPY (0x39) operation.
+    // @dev Copies slice of code to memory
+    // @custom:since Frontier
+    // @custom:group Environmental Information
+    // @custom:gas 3
+    // @custom:stack_consumed_elements 0
+    // @custom:stack_produced_elements 1
+    // @return The pointer to the updated execution context.
+    func exec_codecopy{
+        syscall_ptr: felt*,
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+    }(ctx: model.ExecutionContext*) -> model.ExecutionContext* {
+        alloc_locals;
+        %{
+            import logging
+            logging.info("0x39 - CODECOPY")
+        %}
+
+        let stack = ctx.stack;
+
+        // Stack input:
+        // 0 - offset: memory offset of the work we save.
+        // 1 - code_offset: offset for code from where data will be copied.
+        // 2 - element_len: bytes length of the copied code.
+
+        let (stack, offset) = Stack.pop(stack);
+        let (stack, code_offset) = Stack.pop(stack);
+        let (stack, element_len) = Stack.pop(stack);
+
+        let code: felt* = ctx.code;
+        let code_len: felt = ctx.code_len;
+
+        let sliced_code: felt* = Helpers.slice_data(
+            data_len=code_len, data=code, data_offset=code_offset.low, slice_len=element_len.low
+        );
+
+        let memory: model.Memory* = Memory.store_n(
+            self=ctx.memory, element_len=element_len.low, element=sliced_code, offset=offset.low
+        );
+
+        // Update context memory.
+        let ctx = ExecutionContext.update_memory(ctx, memory);
+        // Update context stack.
+        let ctx = ExecutionContext.update_stack(ctx, stack);
+        // Increment gas used.
+        let ctx = ExecutionContext.increment_gas_used(ctx, GAS_COST_CODECOPY);
+        return ctx;
+    }
 }

src/kakarot/memory.cairo

@@ -111,58 +111,40 @@ namespace Memory {
         bitwise_ptr: BitwiseBuiltin*,
     }(self: model.Memory*, element_len: felt, element: felt*, offset: felt) -> model.Memory* {
         alloc_locals;
-        let (new_memory: felt*) = alloc();
-        if (self.bytes_len == 0) {
-            Helpers.fill(arr=new_memory, value=0, length=offset);
-        }
-
-        let is_offset_greater_than_length = is_le_felt(self.bytes_len, offset);
-        local max_copy: felt;
-        local total_len: felt = offset + element_len;
-        tempvar max_uint256_bytes: felt = 32;
-
-        // Add all the elements into new_memory
-        Helpers.fill_array(
-            fill_with=element_len, input_arr=element, output_arr=new_memory + offset
+        // New memory is composed of 3 parts: head, element and tail
+        // Head are the bytes before offset.
+        // Head is possibly zero-padded if offset is overbound
+        // Tail are the bytes after element (offset + element_len).
+        // Tail is zero-padded in case the total memory is not a multiple of 32.
+
+        local is_memory_expanded: felt;
+
+        local n_head: felt;
+        local n_head_pad: felt;
+        local n_tail: felt;
+        local n_tail_pad: felt;
+
+        let (local new_memory: felt*) = alloc();
+
+        let is_offset_overbound: felt = is_le_felt(self.bytes_len + 1, offset);
+        let n_head = offset + (self.bytes_len - offset) * is_offset_overbound;
+        let n_head_pad = offset - n_head;
+
+        let is_memory_expanded = is_le_felt(self.bytes_len + 1, offset + element_len);
+        let n_tail = (self.bytes_len - offset - element_len) * (1 - is_memory_expanded);
+        let (_, rem) = unsigned_div_rem(offset + element_len, 32);
+        let is_rem_positive = is_le_felt(1, rem);
+        let n_tail_pad = (32 - rem) * is_rem_positive * is_memory_expanded;
+
+        memcpy(dst=new_memory, src=self.bytes, len=n_head);
+        Helpers.fill(arr=new_memory + n_head, value=0, length=n_head_pad);
+        memcpy(dst=new_memory + offset, src=element, len=element_len);
+        memcpy(
+            dst=new_memory + offset + element_len, src=self.bytes + offset + element_len, len=n_tail
         );
-
-        let (local quotient, local remainder) = uint256_unsigned_div_rem(
-            Uint256(offset + element_len, 0), Uint256(max_uint256_bytes, 0)
-        );
-        local diff: felt;
-        if (remainder.low == 0) {
-            diff = 0;
-        } else {
-            diff = max_uint256_bytes - remainder.low;
-        }
-
-        if (is_offset_greater_than_length == 1) {
-            Helpers.fill(arr=new_memory + self.bytes_len, value=0, length=offset - self.bytes_len);
-            // Fill the unused bytes into 0
-            Helpers.fill(arr=new_memory + total_len, value=0, length=diff);
-            max_copy = self.bytes_len;
-        } else {
-            max_copy = offset;
-        }
-
-        if (self.bytes_len != 0) {
-            memcpy(dst=new_memory, src=self.bytes, len=max_copy);
-        }
-
-        let is_memory_growing = is_le_felt(self.bytes_len, total_len);
-
-        local new_bytes_len: felt;
-        if (is_memory_growing == 1) {
-            new_bytes_len = total_len + (diff);
-        } else {
-            memcpy(
-                dst=new_memory + total_len,
-                src=self.bytes + total_len,
-                len=self.bytes_len - (total_len),
-            );
-            new_bytes_len = self.bytes_len;
-        }
-        return new model.Memory(bytes=new_memory, bytes_len=new_bytes_len);
+        Helpers.fill(arr=new_memory + offset + element_len + n_tail, value=0, length=n_tail_pad);
+        let new_memory_len: felt = offset + element_len + n_tail + n_tail_pad;
+        return new model.Memory(bytes=new_memory, bytes_len=new_memory_len);
     }
 
     // @notice Load an element from the memory.

src/utils/utils.cairo

@@ -209,23 +209,21 @@ namespace Helpers {
         data_len: felt, data: felt*, data_offset: felt, slice_len: felt
     ) -> felt* {
         alloc_locals;
-        let (local sliced_data: felt*) = alloc();
+        local len: felt;
+        let (local new_data: felt*) = alloc();
 
-        let diff = data_len - data_offset;
+        // slice's len = min(slice_len, data_len-offset, 0)
+        // which corresponds to full, partial or empty overlap with data
+        // The result is zero-padded in case of partial or empty overlap.
 
-        let is_diff_greater_than_element_len: felt = is_le_felt(slice_len, diff);
+        let is_non_empty: felt = is_le_felt(data_offset, data_len);
+        let max_len: felt = (data_len - data_offset) * is_non_empty;
+        let is_within_bound: felt = is_le_felt(slice_len, max_len);
+        let len = max_len + (slice_len - max_len) * is_within_bound;
 
-        if (is_diff_greater_than_element_len == 0) {
-            memcpy(dst=sliced_data, src=data + data_offset, len=diff);
-
-            let pad_n: felt = slice_len - diff;
-
-            fill(arr=sliced_data + diff, value=0, length=pad_n);
-        } else {
-            memcpy(dst=sliced_data, src=data + data_offset, len=slice_len);
-        }
-
-        return (sliced_data);
+        memcpy(dst=new_data, src=data + data_offset, len=len);
+        fill(arr=new_data + len, value=0, length=slice_len - len);
+        return new_data;
     }
 
     func reverse(old_arr_len: felt, old_arr: felt*, new_arr_len: felt, new_arr: felt*) {

tests/test_zk_evm.py

@@ -1119,6 +1119,76 @@ async def zk_evm(starknet, eth):
         },
         "id": "PRElog4-1",
     },
+    {
+        "params": {
+            "code": "60026000600039",
+            "calldata": "",
+            "stack": "",
+            "memory": "6002000000000000000000000000000000000000000000000000000000000000",
+            "return_value": "",
+        },
+        "id": "Environment Information - CODECOPY (0x39) - code slice within bounds, memory offset > len with tail padding",
+    },
+    {
+        "params": {
+            "code": "7d00112233445566778899aabbccddeeff00112233445566778899aabbccdd60005260086003600139",
+            "calldata": "",
+            "stack": "",
+            "memory": "002233445566778899778899aabbccddeeff00112233445566778899aabbccdd",
+            "return_value": "",
+        },
+        "id": "Environmental Information - CODECOPY (0x39) - code slice within bounds, memory copy within bounds",
+    },
+    {
+        "params": {
+            "code": "7d00112233445566778899aabbccddeeff00112233445566778899aabbccdd60005260206003600139",
+            "calldata": "",
+            "stack": "",
+            "memory": "002233445566778899aabbccddeeff00112233445566778899aabbccdd6000526000000000000000000000000000000000000000000000000000000000000000",
+            "return_value": "",
+        },
+        "id": "Environmental Information - CODECOPY (0x39) - code slice within bounds, memory offset < len < offset + size",
+    },
+    {
+        "params": {
+            "code": "60386002600339",
+            "calldata": "",
+            "stack": "",
+            "memory": "00000060026003390000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
+            "return_value": "",
+        },
+        "id": "Environmental Information - CODECOPY (0x39) - code with padding + memory offset > len ",
+    },
+    {
+        "params": {
+            "code": "7d00112233445566778899aabbccddeeff00112233445566778899aabbccdd60005260056065600439",
+            "calldata": "",
+            "stack": "",
+            "memory": "000000110000000000778899aabbccddeeff00112233445566778899aabbccdd",
+            "return_value": "",
+        },
+        "id": "Environmental Information - CODECOPY (0x39) - code offset > len, memory offset + size < len",
+    },
+    {
+        "params": {
+            "code": "7dFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF7F0000000000000000000000000000000000000000000000000000000000000000505060326000600039",
+            "calldata": "",
+            "stack": "",
+            "memory": "7dffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f0000000000000000000000000000000000000000000000000000000000000000",
+            "return_value": "",
+        },
+        "id": "Environment Information - CODECOPY (0x39) - evmcode example 1",
+    },
+    {
+        "params": {
+            "code": "7dFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF7F00000000000000000000000000000000000000000000000000000000000000005050603260006000396008601f600039",
+            "calldata": "",
+            "stack": "",
+            "memory": "7f00000000000000ffffffffffffffffffffffffffffffffffffffffffffff7f0000000000000000000000000000000000000000000000000000000000000000",
+            "return_value": "",
+        },
+        "id": "Environment Information - CODECOPY (0x39) - evmcode example 1+2",
+    },
 ]