chore(cairo_native): add native to stack trace test

[meship-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-10T13:47:43Z
• Started at 2024-12-16T12:03:05Z
• Started at 2024-12-16T12:04:33Z
• Started at 2024-12-16T13:20:02Z
• Started at 2024-12-16T13:25:14Z
• Started at 2024-12-16T16:55:00Z
• Started at 2024-12-22T09:40:36Z
• Started at 2024-12-22T10:12:19Z
• Started at 2024-12-22T10:30:59Z
• Started at 2024-12-25T14:06:30Z
• Started at 2024-12-25T14:07:50Z

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/test_utils/contracts.rs line 404 at r1 (raw file):

                    panic!("Not implemented for cairo native contracts")
                }
            }

This is waiting for LC to add the entry_point_by_type to the AotContractExacutor.

Code quote:

            RunnableCompiledClass::V1Native(_) => {
                #[cfg(test)]
                {
                    EntryPointOffset(10000)
                }
                #[cfg(not(test))]
                {
                    panic!("Not implemented for cairo native contracts")
                }
            }

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.54%. Comparing base (e3165c4) to head (a305cca).
Report is 807 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2633       +/-   ##
===========================================
+ Coverage   40.10%   71.54%   +31.44%     
===========================================
  Files          26       98       +72     
  Lines        1895    13634    +11739     
  Branches     1895    13634    +11739     
===========================================
+ Hits          760     9755     +8995     
- Misses       1100     3450     +2350     
- Partials       35      429      +394     

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

[noaov1]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @meship-starkware)

---

crates/blockifier/src/test_utils/contracts.rs line 404 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This is waiting for LC to add the entry_point_by_type to the AotContractExacutor.

Consider using the casm field

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/test_utils/contracts.rs line 404 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider using the casm field

Do we still need the entry_point_by_type for the V1Native class, or can we assume that the casm would be enough?

[meship-starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 344 at r2 (raw file):

            .map_err(|error| self.handle_error(remaining_gas, error.into()))?;

        let class_hash = self.base.get_class_hash_at(contract_address).unwrap_or_default();

This shouldn't be unwrap_or_default. Should this be expect if we can't get the class_hash error?
At the VM, we return the error in the function. I can add handle error flow here.

Code quote:

    let class_hash = self.base.get_class_hash_at(contract_address).unwrap_or_default();

[meship-starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 348 at r3 (raw file):

            .base
            .get_class_hash_at(contract_address)
            .map_err(|e| self.handle_error(remaining_gas, e))?;

It's the same as the VM, but I am not entirely sure why it should be like this.

Code quote:

        let class_hash = self
            .base
            .get_class_hash_at(contract_address)
            .map_err(|e| self.handle_error(remaining_gas, e))?;

[noaov1]

Reviewed 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 348 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It's the same as the VM, but I am not entirely sure why it should be like this.

WDYM?

---

crates/blockifier/src/test_utils/contracts.rs line 404 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Do we still need the entry_point_by_type for the V1Native class, or can we assume that the casm would be enough?

Wait- why is it needed? You don't need the pc in the native expected case, no? Please revert.

---

crates/blockifier/src/execution/native/syscall_handler.rs line 321 at r3 (raw file):

                    match e {
                        SyscallExecutionError::SyscallError { .. } => e,
                        _ => e.as_call_contract_execution_error(

No?

Suggestion:

                        _ => e.as_lib_call_execution_error(

---

crates/blockifier/src/execution/stack_trace.rs line 715 at r3 (raw file):

        EntryPointExecutionError::NativeUnrecoverableError(error) => {
            extract_syscall_execution_error_into_stack_trace(error_stack, depth, error)
        }

Nice!
In the VM, we needed to extract it from CairoRunError::VmException(VirtualMachineError::Hint(Box(HintError::Internal(VirtualMachineError::Other(error.into())))).

Code quote:

        #[cfg(feature = "cairo_native")]
        EntryPointExecutionError::NativeUnrecoverableError(error) => {
            extract_syscall_execution_error_into_stack_trace(error_stack, depth, error)
        }

---

crates/blockifier/src/execution/stack_trace_test.rs line 648 at r3 (raw file):

        CairoVersion::Cairo1(RunnableCairo1::Casm),
        CairoVersion::Cairo1(RunnableCairo1::Native)
    )]

Consider defining a const CAIRO_VERSIONS with two different values (condition on the cairo native feature flag), to avoid code duplication.

Code quote:

    #[values(
        CairoVersion::Cairo0,
        CairoVersion::Cairo1(RunnableCairo1::Casm),
        CairoVersion::Cairo1(RunnableCairo1::Native)
    )]

---

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

    }

    fn execute_inner_call(

I think that using this function makes the code more readable. Can you leave it with the needed modification? (non-blocking)

Code quote:

fn execute_inner_call(

[meship-starkware]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

---

crates/blockifier/src/execution/stack_trace_test.rs line 648 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining a const CAIRO_VERSIONS with two different values (condition on the cairo native feature flag), to avoid code duplication.

It does not work. Lets stay with this for now.

---

crates/blockifier/src/execution/native/syscall_handler.rs line 321 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

No?

You are right
Done.

---

crates/blockifier/src/execution/native/syscall_handler.rs line 348 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

WDYM?

I though we might want to wrap it as well but it is not an inner call error so I think its ok.

---

crates/blockifier/src/test_utils/contracts.rs line 404 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Wait- why is it needed? You don't need the pc in the native expected case, no? Please revert.

Done.

[noaov1]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

---

crates/blockifier/src/execution/stack_trace_test.rs line 250 at r4 (raw file):

"
        )
        .to_string(),

Why is it needed now?

Code quote:

        .to_string(),

---

crates/blockifier/src/execution/native/syscall_handler.rs line 127 at r4 (raw file):

        remaining_gas: &mut u64,
        class_hash: ClassHash,
    ) -> SyscallResult<Retdata> {

WDYT?

Suggestion:

    fn execute_inner_call(
        &mut self,
        entry_point: CallEntryPoint,
        remaining_gas: &mut u64,
        class_hash: ClassHash,
        format_error: F
    ) -> SyscallResult<Retdata> 
    where
    F: FnOnce(SyscallExecutionError, ClassHash, ContractAddress, EntryPointSelector) -> SyscallExecutionError,
{

[meship-starkware]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)

---

crates/blockifier/src/execution/stack_trace_test.rs line 250 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is it needed now?

I think its becuse we return this now and not saving this as an argument

[noaov1]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)