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 support for sha256_process_block syscall #1545

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

varex83
Copy link
Contributor

@varex83 varex83 commented Oct 23, 2024

This PR adds support for the sha256_process_block syscall for cairo native's syscall handler.

@reviewable-StarkWare
Copy link

This change is Reviewable

@varex83 varex83 added the native integration Related with the integration of Cairo Native into the Blockifier label Oct 23, 2024
Copy link

Artifacts upload triggered. View details here

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.47%. Comparing base (e3165c4) to head (097ae6a).
Report is 319 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1545       +/-   ##
===========================================
+ Coverage   40.10%   67.47%   +27.37%     
===========================================
  Files          26      103       +77     
  Lines        1895    13913    +12018     
  Branches     1895    13913    +12018     
===========================================
+ Hits          760     9388     +8628     
- Misses       1100     4124     +3024     
- Partials       35      401      +366     

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

@rodrigo-pino rodrigo-pino force-pushed the rdr/update-testing-suite branch 2 times, most recently from c728762 to 4cf52c8 Compare October 24, 2024 11:51
@rodrigo-pino rodrigo-pino force-pushed the bohdan/sha256_syscall branch from a8a4e1b to 05ccd53 Compare October 26, 2024 12:40
@rodrigo-pino rodrigo-pino changed the base branch from rdr/update-testing-suite to rdr/add-syscall-counting October 26, 2024 12:41
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@varex83 varex83 changed the title feat: add support for sha256_process_block syscall feat(blockifier): add support for sha256_process_block syscall Oct 28, 2024
Copy link

Artifacts upload triggered. View details here

@rodrigo-pino rodrigo-pino force-pushed the rdr/add-syscall-counting branch 2 times, most recently from 147c1d3 to ded98cf Compare October 29, 2024 14:23
@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from 05ccd53 to 3cf4a05 Compare October 29, 2024 16:19
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

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: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @varex83)


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

    let versioned_constants = syscall_handler.context.versioned_constants();
    *syscall_handler.resources +=
        &versioned_constants.get_additional_os_syscall_resources(&syscall_handler.syscall_counter);

It seems like these lines are duplicated.

Suggestion:

    // todo(rodrigo): execution resources for native execution are still wip until future
    // development on both Cairo lang and the Native compiler
    let versioned_constants = syscall_handler.context.versioned_constants();
    *syscall_handler.resources +=
        &versioned_constants.get_additional_os_syscall_resources(&syscall_handler.syscall_counter);

crates/blockifier/src/test_utils/cairo_compile.rs line 198 at r5 (raw file):

    sierra_output.stdout
}

I don't think this should be part of this PR. Is it here because this PR is based on add_syscall_counting and not add_native_suppourt_in_testing_suite?

Code quote:

pub fn sierra_compile(
    path: String,
    git_tag_override: Option<String>,
    cargo_nightly_arg: Option<String>,
) -> Vec<u8> {
    prepare_cairo1_compiler_deps(git_tag_override);

    let cairo1_compiler_path = local_cairo1_compiler_repo_path();

    // Command args common to both compilation phases.
    let mut base_compile_args = vec![
        "run".into(),
        format!("--manifest-path={}/Cargo.toml", cairo1_compiler_path.to_string_lossy()),
        "--bin".into(),
    ];
    // Add additional cargo arg if provided. Should be first arg (base command is `cargo`).
    if let Some(nightly_version) = cargo_nightly_arg {
        base_compile_args.insert(0, format!("+nightly-{nightly_version}"));
    }

    // Cairo -> Sierra.
    let mut starknet_compile_commmand = Command::new("cargo");
    starknet_compile_commmand.args(base_compile_args.clone());
    starknet_compile_commmand.args(["starknet-compile", "--", "--single-file", &path]);
    let sierra_output = run_and_verify_output(&mut starknet_compile_commmand);

    sierra_output.stdout
}

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 r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @varex83)

@rodrigo-pino rodrigo-pino force-pushed the rdr/add-syscall-counting branch 5 times, most recently from 6e88aa9 to 55834b2 Compare October 31, 2024 14:45
@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from 429e95a to baab125 Compare October 31, 2024 16:36
Copy link

Artifacts upload triggered. View details here

@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from baab125 to 506c511 Compare October 31, 2024 16:41
Copy link

Artifacts upload triggered. View details here

Copy link
Collaborator

@noaov1 noaov1 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, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @varex83)


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

        )?;

        let data_as_bytes = sha2::digest::generic_array::GenericArray::from_exact_iter(

Suggestion:

let current_block_as_bytes = sha2::digest::generic_array::GenericArray:

@rodrigo-pino rodrigo-pino force-pushed the rdr/add-syscall-counting branch from 60e2392 to 538df34 Compare November 6, 2024 18:25
Copy link
Contributor

@avi-starkware avi-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, 1 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @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 1 of 4 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @varex83)

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: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)


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

        )?;

        let data_as_bytes = sha2::digest::generic_array::GenericArray::from_exact_iter(

Named like that just to be consistent with the current VM implementation:

@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from deb6167 to 1cd6470 Compare November 7, 2024 11:22
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

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, 1 unresolved discussion (waiting on @noaov1)

Copy link
Contributor

@avi-starkware avi-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, 1 unresolved discussion (waiting on @noaov1)

@rodrigo-pino rodrigo-pino force-pushed the rdr/add-syscall-counting branch from 538df34 to 861b3c2 Compare November 7, 2024 15:32
@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from 1cd6470 to bd4f9d6 Compare November 8, 2024 12:31
Copy link

github-actions bot commented Nov 8, 2024

Artifacts upload triggered. View details here

@rodrigo-pino rodrigo-pino force-pushed the rdr/add-syscall-counting branch from 861b3c2 to ffba45f Compare November 9, 2024 10:48
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, 1 unresolved discussion (waiting on @noaov1)

@rodrigo-pino rodrigo-pino force-pushed the rdr/add-syscall-counting branch 4 times, most recently from 139d79d to 3d4ab3d Compare November 10, 2024 11:48
@varex83 varex83 changed the base branch from rdr/add-syscall-counting to main November 11, 2024 09:45
@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from bd4f9d6 to 9eebe27 Compare November 11, 2024 09:56
Copy link

Artifacts upload triggered. View details here

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 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

@varex83 varex83 force-pushed the bohdan/sha256_syscall branch from 9eebe27 to 097ae6a Compare November 11, 2024 22:37
Copy link

Artifacts upload triggered. View details here

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 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @varex83)

@meship-starkware meship-starkware merged commit 8127efd into main Nov 12, 2024
12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2024
@meship-starkware meship-starkware deleted the bohdan/sha256_syscall branch November 19, 2024 08:51
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.

6 participants