feat(blockifier): add non-recoverable error handling for native syscall handler

[rodrigo-pino]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion

---

crates/blockifier/src/execution/errors.rs line 98 at r1 (raw file):

    #[cfg(feature = "cairo_native")]
    #[error(transparent)]
    NativeExecutionError(#[from] Box<SyscallExecutionError>),

I am not sure if this is the best approach. The Native Syscall Handler will catch non-recoverable SyscallExecutionError, nonetheless, the entry_point_execution needs to return an EntryPointExecution error.

It's not very clear to me in the current context what would be the best and simplest way to map between one to the other.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.06%. Comparing base (e3165c4) to head (b0fa3a9).
Report is 396 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1948       +/-   ##
===========================================
+ Coverage   40.10%   69.06%   +28.95%     
===========================================
  Files          26      105       +79     
  Lines        1895    13659    +11764     
  Branches     1895    13659    +11764     
===========================================
+ Hits          760     9433     +8673     
- Misses       1100     3823     +2723     
- Partials       35      403      +368     

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

[noaov1]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, and @rodrigo-pino)

---

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

            SyscallExecutionError::SyscallError { error_data } => error_data,
            _ => {
                self.unrecoverable_error = Some(error);

I think we can override this, so it may not necessarily contain the original error. Right?
We might need to check this variable at the beginning of each syscall.

Code quote:

self.unrecoverable_error = Some(error);

[rodrigo-pino]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, and @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

I think we can override this, so it may not necessarily contain the original error. Right?
We might need to check this variable at the beginning of each syscall.

Because remaining_gas is set to 0 and every syscall pre charges at the beginning this should never happen.

As a second thought, I think having the sanity check will be also worth it in case of a native bug 🤔. Not sure what would be your preferred approach in this instance

[noaov1]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @meship-starkware)

---

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

Previously, rodrigo-pino (Rodrigo) wrote…

Because remaining_gas is set to 0 and every syscall pre charges at the beginning this should never happen.

As a second thought, I think having the sanity check will be also worth it in case of a native bug 🤔. Not sure what would be your preferred approach in this instance

@ilyalesokhin-starkware noted that some syscalls have a cost of 0, so in these cases, setting the remaining gas to 0 will not trigger an abort. You could also make this parameter optional, allowing it to be set only when it is None.

[noaov1]

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, and @rodrigo-pino)

---

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

Previously, noaov1 (Noa Oved) wrote…

@ilyalesokhin-starkware noted that some syscalls have a cost of 0, so in these cases, setting the remaining gas to 0 will not trigger an abort. You could also make this parameter optional, allowing it to be set only when it is None.

I see that it's already optional :)

---

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

    if let Some(error) = syscall_handler.unrecoverable_error {
        return Err(EntryPointExecutionError::NativeExecutionError(Box::new(error)));
    }

You'll need to add a special case for this type of error in execute_inner_call. You may want to set unrecoverable_error with the specified error.

Code quote:

    if let Some(error) = syscall_handler.unrecoverable_error {
        return Err(EntryPointExecutionError::NativeExecutionError(Box::new(error)));
    }

[ilyalesokhin-starkware]

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

            SyscallExecutionError::SyscallError { error_data } => error_data,
            _ => {
                self.unrecoverable_error = Some(error);

Can we add an assert that we are not overriding the error?

Suggestion:

 assert!(self.unrecoverable_error.is_none())
 self.unrecoverable_error = Some(error);

[ilyalesokhin-starkware]

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

    /// Handles all gas-related logics and additional metadata such as `SyscallCounter`. In native,
    /// we need to explicitly call this method at the beginning of each syscall.
    fn pre_execute_syscall(

Please check self.unrecoverable_error in pre_execute_syscall

Code quote:

 fn pre_execute_syscall(

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, and @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

You'll need to add a special case for this type of error in execute_inner_call. You may want to set unrecoverable_error with the specified error.

I don't think the check will ever get executed if we put it on execute_inner_call.

If we get to actually call execute_inner_call, it means there were no errors before because they were already checked for it at the beginning (adding a check to pre_execute_syscall) or if they appeared during the syscall execution, it would immediately return them.

During execute_inner_call execution, if an error occured, it will be handled accordingly, setting the flag and returning immediately.

---

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

Previously, ilyalesokhin-starkware wrote…

Please check self.unrecoverable_error in pre_execute_syscall

Done

---

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

Previously, noaov1 (Noa Oved) wrote…

I see that it's already optional :)

Done

---

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

Previously, ilyalesokhin-starkware wrote…

Can we add an assert that we are not overriding the error?

Done

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

Reviewed 1 of 2 files at r2.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 3 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

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

[Yoni-Starkware]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @rodrigo-pino)

---

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

Previously, rodrigo-pino (Rodrigo) wrote…

I don't think the check will ever get executed if we put it on execute_inner_call.

If we get to actually call execute_inner_call, it means there were no errors before because they were already checked for it at the beginning (adding a check to pre_execute_syscall) or if they appeared during the syscall execution, it would immediately return them.

During execute_inner_call execution, if an error occured, it will be handled accordingly, setting the flag and returning immediately.

Ok, are you saying that this case will reach the _ => { arm in handle_error?

[Yoni-Starkware]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @rodrigo-pino)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Ok, are you saying that this case will reach the _ => { arm in handle_error?

Hmmm, the problem is that you are wrapping with EntryPointExecutionError::NativeExecutionError() on every level.

How about moving unrecoverable_error to EntryPointExecutionContext, and doing nothing here?

[avi-starkware]

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

[rodrigo-pino]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

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

Hmmm, the problem is that you are wrapping with EntryPointExecutionError::NativeExecutionError() on every level.

I am not fully understanding this. I believe the wrapping only happens once, it is not back-propagated in the case of multiple inner calls. We get the error, we store it, once Native finishes we return it. Nothing after the error "counts" execution wise

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @rodrigo-pino)

---

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

Previously, rodrigo-pino (Rodrigo) wrote…

Hmmm, the problem is that you are wrapping with EntryPointExecutionError::NativeExecutionError() on every level.

I am not fully understanding this. I believe the wrapping only happens once, it is not back-propagated in the case of multiple inner calls. We get the error, we store it, once Native finishes we return it. Nothing after the error "counts" execution wise

I think it is propagated.
If the error happens in the inner most call, with N inner calls, we call execute_entry_point_call N times

[rodrigo-pino]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

I think it is propagated.
If the error happens in the inner most call, with N inner calls, we call execute_entry_point_call N times

I understand. It makes sense. Would moving it to EntryPointExecutionContext just for Native will be ok?

[rodrigo-pino]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

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

Previously, rodrigo-pino (Rodrigo) wrote…

I understand. It makes sense. Would moving it to EntryPointExecutionContext just for Native will be ok?

If we were to modify EntryPointExecutionContext, I believe logic changes would be more extensive as well. What do you think of detecting these NativeExecutionErrors inside and non-rewrapping them.

This logic would be defined in self.handle_error and it will be less invasive

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @rodrigo-pino)

---

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

Previously, rodrigo-pino (Rodrigo) wrote…

If we were to modify EntryPointExecutionContext, I believe logic changes would be more extensive as well. What do you think of detecting these NativeExecutionErrors inside and non-rewrapping them.

This logic would be defined in self.handle_error and it will be less invasive

Sounds good, let's do this

[meship-starkware]

Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @noaov1, and @rodrigo-pino)

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Sounds good, let's do this

Done, please re-review

[meship-starkware]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @noaov1, and @Yoni-Starkware)

[meship-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @noaov1, and @Yoni-Starkware)

[Yoni-Starkware]

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, @noaov1, and @rodrigo-pino)

---

crates/blockifier/src/execution/errors.rs line 98 at r5 (raw file):

    #[cfg(feature = "cairo_native")]
    #[error(transparent)]
    NativeExecutionError(#[from] Box<SyscallExecutionError>),

WDYT?

Suggestion:

NativeUnrecoverableError

[Yoni-Starkware]

Reviewed 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @ilyalesokhin-starkware, @meship-starkware, and @noaov1)

[rodrigo-pino]

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

---

crates/blockifier/src/execution/errors.rs line 98 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

WDYT?

Done

[github-actions]

Artifacts upload triggered. View details here

OOO