Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(blockifier): add native entry point execution #622

Closed

Conversation

varex83
Copy link
Contributor

@varex83 varex83 commented Aug 27, 2024

Note, that this PR is a copy of #551 but with changed "from" branch


This change is Reviewable

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r1.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @varex83)


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

    );

    let run_result = match execution_result {

When are we expecting the result failure flag to be false? If there is a clear difference between the case of failure flag is true and an error was returned, could you document it (for example, Cairo failure vs. program failure)


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

        }
        Ok(res) => Ok(res),
    }?;

Just to be consistent with the other, I also find the Ok, Err, Ok pattern a bit confusing WDYT?

Suggestion:

    let run_result = match execution_result {
        Err(runner_err) =>
            Err(EntryPointExecutionError::NativeUnexpectedError { source: runner_err }),
        Ok(res) if res.failure_flag => Err(EntryPointExecutionError::NativeExecutionError {
            info: if !res.return_values.is_empty() {
                decode_felts_as_str(&res.return_values)
            } else {
                String::from("Unknown error")
            },
        }),
        Ok(res) => Ok(res),
    }?;

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @varex83)


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

    storage_read_values: Vec<Felt>,
    accessed_storage_keys: HashSet<StorageKey, RandomState>,
) -> Result<CallInfo, EntryPointExecutionError> {

Please change this function to return CallInfo.
see discussion: https://reviewable.io/reviews/starkware-libs/sequencer/551#-O5HvHgF9ef4_sc5aZh6

Copy link
Contributor Author

@varex83 varex83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, meship-starkware (Meshi Peled) wrote…

When are we expecting the result failure flag to be false? If there is a clear difference between the case of failure flag is true and an error was returned, could you document it (for example, Cairo failure vs. program failure)

As far as I remember, when compilation is successful and we get this error from the runtime as output, we get it together with failure_flag set to true


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

Previously, meship-starkware (Meshi Peled) wrote…

Just to be consistent with the other, I also find the Ok, Err, Ok pattern a bit confusing WDYT?

Will do, good catch!


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

Previously, meship-starkware (Meshi Peled) wrote…

Please change this function to return CallInfo.
see discussion: https://reviewable.io/reviews/starkware-libs/sequencer/551#-O5HvHgF9ef4_sc5aZh6

Will do!

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @varex83)

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83)

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @varex83)

Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@varex83 this PR doesn't match the latest updates that we have in the Blockifier. There has been a refactor specially targeting this part of the code. Please, update before proceeding. Or did you have something else in mind?

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @varex83)


crates/blockifier/src/execution/native/utils.rs line 31 at r5 (raw file):

// An arbitrary number, chosen to avoid accidentally aligning with actually calculated gas
// To be deleted once cairo native gas handling can be used.
pub const NATIVE_GAS_PLACEHOLDER: u64 = 12;

I believe there is no need for this constant anymore. Is there a reason for it to be still part of the PR?


crates/blockifier/src/execution/native/utils.rs line 77 at r5 (raw file):

}

pub fn create_callinfo(

Please update to use the latest version of the Call Info

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @varex83)


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

use super::syscall_handler::NativeSyscallHandler;
use super::utils::run_native_executor;

Please avoid using super

Code quote:

use super::syscall_handler::NativeSyscallHandler;
use super::utils::run_native_executor;

crates/blockifier/src/execution/native/utils.rs line 55 at r5 (raw file):

        Err(runner_err) => {
            Err(EntryPointExecutionError::NativeUnexpectedError { source: runner_err })
        }

Suggestion:

        Err(runner_err) => 
            Err(EntryPointExecutionError::NativeUnexpectedError { source: runner_err })
        

@rodrigo-pino rodrigo-pino added the native integration Related with the integration of Cairo Native into the Blockifier label Sep 23, 2024
@rodrigo-pino
Copy link
Contributor

Solved in #1158

@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
native integration Related with the integration of Cairo Native into the Blockifier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants