feat(blockifier): add entry point execution logic for native

[rodrigo-pino]

This is a newer version of #622 (which was a copy of #551) with addressed comments and updated git history.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 84 lines in your changes missing coverage. Please review.

Project coverage is 75.80%. Comparing base (b0cfe82) to head (c773bf1).
Report is 442 commits behind head on main.

Files with missing lines	Patch %	Lines
...fier/src/execution/native/entry_point_execution.rs	0.00%	76 Missing ⚠️
crates/blockifier/src/execution/execution_utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
+ Coverage   74.18%   75.80%   +1.61%     
==========================================
  Files         359      360       +1     
  Lines       36240    38485    +2245     
  Branches    36240    38485    +2245     
==========================================
+ Hits        26886    29173    +2287     
+ Misses       7220     7089     -131     
- Partials     2134     2223      +89     

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.859 ms 34.887 ms 34.918 ms]
change: [-1.8445% -1.5249% -1.2593%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

[meship-starkware]

Reviewed 1 of 4 files at r1, 1 of 7 files at r3, 3 of 4 files at r4, all commit messages.
Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 104 at r6 (raw file):

            n_memory_holes: 0,
            builtin_instance_counter: HashMap::default(),
        },

I am not sure all zero is what we want here @noaov1 WDYT? Maybe the estimated value of the versioned constants will be a better
solution. Anyway, I think this is outside of PR's scope.

Code quote:

        // todo(rodrigo): execution resources rely heavily on how the VM work, therefore
        // the dummy values
        resources: ExecutionResources {
            n_steps: 0,
            n_memory_holes: 0,
            builtin_instance_counter: HashMap::default(),
        },

[meship-starkware]

Reviewed 1 of 1 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

[meship-starkware]

Reviewed 4 of 5 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

[rodrigo-pino]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 104 at r6 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure all zero is what we want here @noaov1 WDYT? Maybe the estimated value of the versioned constants will be a better
solution. Anyway, I think this is outside of PR's scope.

If this is non-blocking maybe we can open an issue and continue with the mergin' of this PR?

[noaov1]

Reviewed 1 of 4 files at r4, 1 of 5 files at r8, 1 of 1 files at r10, 1 of 2 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @rodrigo-pino)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 36 at r14 (raw file):

        resources,
        context,
    );

I suggest having the call as a field of the syscall handler (non-blocking and should be done in a separate PR).

Code quote:

    let syscall_handler: NativeSyscallHandler<'_> = NativeSyscallHandler::new(
        state,
        call.caller_address,
        call.storage_address,
        call.entry_point_selector,
        resources,
        context,
    );

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 58 at r14 (raw file):

            Err(EntryPointExecutionError::NativeUnexpectedError { source: runner_err })
        }
        Ok(res) if res.failure_flag => Err(EntryPointExecutionError::NativeExecutionError {

And then you don't need to dcode. We currently return Vec<Felt>.
See here.

Suggestion:

Err(EntryPointExecutionError::ExecutionFailed {

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 68 at r14 (raw file):

    }?;

    create_callinfo(call.clone(), run_result, syscall_handler)

Is the clone needed?

Suggestion:

    create_callinfo(call, run_result, syscall_handler)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 85 at r14 (raw file):

                info: "Overflow: gas consumed bigger than 64 bit".into(),
            });
        }

WDYT?

Suggestion:

    let gas_consumed = gas.to_u64().ok_or(PostExecutionError::MalformedReturnData {
        error_message: format!("Unexpected remaining gas: {gas}."),
    })?;

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 86 at r14 (raw file):

            });
        }
        call.initial_gas - low

What will happen in case low > call.initial_gas

Code quote:

call.initial_gas - low

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 104 at r14 (raw file):

            n_memory_holes: 0,
            builtin_instance_counter: HashMap::default(),
        },

Suggestion:

        resources: ExecutionResources::default(),

[avi-starkware]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @rodrigo-pino)

---

crates/blockifier/src/execution/errors.rs line 96 at r11 (raw file):

        #[source]
        source: NativeRunnerError,
    },

Why not use from instead of source? (As in the other examples in this enum)

Suggestion:

    NativeUnexpectedError(#[from] NativeRunnerError),

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 54 at r14 (raw file):

    );

    let run_result = match execution_result {

I think this name makes more sense. run_result sounds the same as execution_result, so it was slightly confusing to me. call_result implies that this result is related to the call_info, which is the output of the current function.

Non-blocking

Suggestion:

    let call_result = match execution_result 

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 78 at r14 (raw file):

    let gas_consumed = {
        // We can use `.unwrap()` directly in both cases because the most significant bit is could
        // be only 63 here (128 = 64 + 64).

Non-blocking

Suggestion:

        // We can use `.unwrap()` directly in both cases because the most significant bit can
        // be at most 63 here (128 = 64 + 64).

[avi-starkware]

Reviewed 1 of 3 files at r11, 1 of 1 files at r13.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @rodrigo-pino)

[rodrigo-pino]

Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/execution/errors.rs line 96 at r11 (raw file):

Previously, avi-starkware wrote…

Why not use from instead of source? (As in the other examples in this enum)

Done!

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 36 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I suggest having the call as a field of the syscall handler (non-blocking and should be done in a separate PR).

Makes sense

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 54 at r14 (raw file):

Previously, avi-starkware wrote…

I think this name makes more sense. run_result sounds the same as execution_result, so it was slightly confusing to me. call_result implies that this result is related to the call_info, which is the output of the current function.

Non-blocking

Done!

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 58 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

And then you don't need to dcode. We currently return Vec<Felt>.
See here.

I don't fully understand this one. Assuming the way it starts there was a first part somewhere else?

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 68 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Is the clone needed?

Not at all!!! I think it might have been a refactor left-over

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 85 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

WDYT?

Yes! A lot shorter, thank you!

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 86 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What will happen in case low > call.initial_gas

In this case low is the gas used by the execution. At this point in the code is not possible (assuming the correctness of Native & Syscalls) for low to be greater than call.initial_gas

If there had been an out_of_gas exception this method wouldn't execute. The code would take the "report error route" and not the "create call info" one. You can see in the function above (run_native_executor) where property failure_flag is used to determine the correct execution flow.

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 104 at r14 (raw file):

            n_memory_holes: 0,
            builtin_instance_counter: HashMap::default(),
        },

Done

[rodrigo-pino]

Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 36 at r14 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

Makes sense

#1380

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.731 ms 34.780 ms 34.832 ms]
change: [-4.0820% -2.6729% -1.4462%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild

[noaov1]

Reviewed 1 of 4 files at r19, 1 of 2 files at r20, 1 of 2 files at r23, 1 of 1 files at r24, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @rodrigo-pino)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 58 at r14 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

I don't fully understand this one. Assuming the way it starts there was a first part somewhere else?

When executing a casm, if the failure flag is on, we return an ExecutionFailed error, containing the vector of felts (without decoding the error). We can keep this behaviour for consistency (see here).

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 86 at r14 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

In this case low is the gas used by the execution. At this point in the code is not possible (assuming the correctness of Native & Syscalls) for low to be greater than call.initial_gas

If there had been an out_of_gas exception this method wouldn't execute. The code would take the "report error route" and not the "create call info" one. You can see in the function above (run_native_executor) where property failure_flag is used to determine the correct execution flow.

Is low the gas used by the execution or the gas remained after the execution?
Anyway, I think that we can consume a negative amount of gas.
Currently, in the casm execution, we return an error in this case (see here.

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 59 at r25 (raw file):

                decode_felts_as_str(&res.return_values)
            } else {
                String::from("Unknown error")

Why is it considered an unknown error in this case?

Code quote:

                String::from("Unknown error")

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 75 at r25 (raw file):

    // todo(rodro): Even if the property is called `remaining_gas` it behaves like gas used.
    // Update once gas works on Native side has been completed (or at least this part)
    let gas_used =

Suggestion:

let remaining_gas =

[rodrigo-pino]

Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 58 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

When executing a casm, if the failure flag is on, we return an ExecutionFailed error, containing the vector of felts (without decoding the error). We can keep this behaviour for consistency (see here).

Right, I've added the enable_revert flag as a condition for the error as well

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 86 at r14 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Is low the gas used by the execution or the gas remained after the execution?
Anyway, I think that we can consume a negative amount of gas.
Currently, in the casm execution, we return an error in this case (see here.

Sorry, low is remaining gas.

Regarding the other stuff, makes sense!

With the removal of NativeExecutionError there is also no reason to decode felts the way we did since you have a similar function.(format_panic_data). Therefore I am removing decode_felts_as_str function.

By chance do you guys have a method for encoding string as felts :)?

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 59 at r25 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is it considered an unknown error in this case?

Good question, luckily is no longer relevant since we are using ExecutionFailed error now

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 75 at r25 (raw file):

    // todo(rodro): Even if the property is called `remaining_gas` it behaves like gas used.
    // Update once gas works on Native side has been completed (or at least this part)
    let gas_used =

Done

[noaov1]

Reviewed 2 of 5 files at r8, 1 of 7 files at r27, 1 of 2 files at r28, 1 of 1 files at r32, 4 of 4 files at r34, 1 of 1 files at r35, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

---

.github/workflows/main.yml line 110 at r35 (raw file):

  codecov:
    runs-on: starkware-ubuntu-latest-large

Why?

Code quote:

runs-on: starkware-ubuntu-latest-large

---

crates/blockifier/src/execution/native/entry_point_execution.rs line 86 at r14 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

Sorry, low is remaining gas.

Regarding the other stuff, makes sense!

With the removal of NativeExecutionError there is also no reason to decode felts the way we did since you have a similar function.(format_panic_data). Therefore I am removing decode_felts_as_str function.

By chance do you guys have a method for encoding string as felts :)?

Not that I know of.

[noaov1]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rodrigo-pino)