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

fix(execution): forbid calling cairo0 contract with cairo1 only builtins #128

Conversation

meship-starkware
Copy link
Contributor

@meship-starkware meship-starkware commented Jul 28, 2024

This change is Reviewable

@meship-starkware meship-starkware changed the base branch from main to main-v0.13.2 July 28, 2024 07:57
@meship-starkware meship-starkware force-pushed the meship/blockifier/forbid_cairo1_builtins_calls_in_cairo0_contract branch from 801e36d to d13501f Compare July 28, 2024 11:00
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 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @meship-starkware)


crates/blockifier/src/execution/deprecated_entry_point_execution.rs line 85 at r1 (raw file):

    context: &'a mut EntryPointExecutionContext,
) -> Result<VmExecutionContext<'a>, PreExecutionError> {
    // Resolve initial PC from EP indicator.

Please move it to the relevant line of code.

Code quote:

    // Resolve initial PC from EP indicator.

crates/blockifier/src/execution/deprecated_entry_point_execution.rs line 94 at r1 (raw file):

            unsupported_builtins_set.iter().map(|&item| *item).collect(),
        ));
    }

Suggestion:

    // Verify use of cairo0 builtins only.
    let program_builtins: HashSet<&BuiltinName> =
        HashSet::from_iter(contract_class.program.iter_builtins());
    let unsupported_builtins =
        &program_builtins - &HashSet::from_iter(CAIRO0_BUILTINS_NAMES.iter());
    if !unsupported_builtins_set.is_empty() {
        return Err(PreExecutionError::UnsupportedCairo0Builtin(
            unsupported_builtins_set.iter().map(|&item| *item).collect(),
        ));
    }

@meship-starkware meship-starkware force-pushed the meship/blockifier/forbid_cairo1_builtins_calls_in_cairo0_contract branch from d13501f to 58af0ae Compare July 28, 2024 11:40
Copy link
Contributor Author

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


crates/blockifier/src/execution/deprecated_entry_point_execution.rs line 85 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please move it to the relevant line of code.

Done.


crates/blockifier/src/execution/deprecated_entry_point_execution.rs line 94 at r1 (raw file):

            unsupported_builtins_set.iter().map(|&item| *item).collect(),
        ));
    }

Done.

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.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware)

@meship-starkware meship-starkware force-pushed the meship/blockifier/forbid_cairo1_builtins_calls_in_cairo0_contract branch from 58af0ae to bbc9f2a Compare July 28, 2024 13:53
@meship-starkware meship-starkware merged commit 7ee04db into main-v0.13.2 Jul 28, 2024
20 of 22 checks passed
@meship-starkware meship-starkware deleted the meship/blockifier/forbid_cairo1_builtins_calls_in_cairo0_contract branch July 28, 2024 14:28
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants