Skip to content

WIP: feat(starknet_os): verify syscall_ptr #6327

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aner-starkware
Copy link
Contributor

No description provided.

@aner-starkware aner-starkware self-assigned this May 5, 2025
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented May 5, 2025

Artifacts upload workflows:

Copy link

github-actions bot commented May 5, 2025

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [30.284 ms 30.329 ms 30.376 ms]
change: [-1.6225% -1.4208% -1.2037%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
5 (5.00%) high mild

@aner-starkware aner-starkware force-pushed the aner/snos_verify_syscall_ptr branch 3 times, most recently from 5786289 to b114059 Compare May 6, 2025 07:21
Copy link

github-actions bot commented May 6, 2025

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [30.126 ms 30.165 ms 30.206 ms]
change: [-2.1521% -1.7151% -1.3829%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 9 files at r1.
Reviewable status: 3 of 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware)


a discussion (no related file):
I'd like to avoid (a) adding a new crate dependency just for pascal-casing and (b) avoid adding an extra field to the syscall processor.
instead, how about using the From<Felt> logic defined on the SyscallSelector?
see comments in your macro module on how I think this can work


crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

                syscall_hint_processor.set_syscall_ptr(
                    get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?
                );

here, you can just fetch the integer at the current syscall pointer and verify it's the correct selector, no?
or is the pointer not pointing to the correct value at this point in time?
(see below, I'm assuming you have a $selector) macro ident available)

let syscall_ptr = get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?;
let selector = SyscallSelector::from(vm.get_integer(syscall_ptr)?)?;
if selector != SyscallSelector::$selector {
    return Err(..);
}
syscall_hint_processor.set_syscall_ptr(syscall_ptr);

Code quote:

                let syscall_hint_processor = &mut hint_processor.deprecated_syscall_hint_processor;
                syscall_hint_processor.set_syscall_name(
                    stringify!($name).to_string().to_case(Case::Pascal)
                );
                syscall_hint_processor.set_syscall_ptr(
                    get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?
                );

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r1 (raw file):

create_syscall_func!(
    call_contract,
    delegate_call,

... etc.
If paste has a pascal-case functionality you can use it, but if not, I wouldn't use an extra crate just to reduce the boilerplate here...

Suggestion:

create_syscall_func!(
    (call_contract, CallContract),
    (delegate_call, DelegateCall),

@aner-starkware
Copy link
Contributor Author

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

Previously, dorimedini-starkware wrote…

here, you can just fetch the integer at the current syscall pointer and verify it's the correct selector, no?
or is the pointer not pointing to the correct value at this point in time?
(see below, I'm assuming you have a $selector) macro ident available)

let syscall_ptr = get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?;
let selector = SyscallSelector::from(vm.get_integer(syscall_ptr)?)?;
if selector != SyscallSelector::$selector {
    return Err(..);
}
syscall_hint_processor.set_syscall_ptr(syscall_ptr);

I don't understand the suggestion - you're not using the $name. Am I missing something?

@aner-starkware
Copy link
Contributor Author

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

Previously, aner-starkware wrote…

I don't understand the suggestion - you're not using the $name. Am I missing something?

Ahhh, OK, $selector you meant $name , right?

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware)


crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

Previously, aner-starkware wrote…

Ahhh, OK, $selector you meant $name , right?

in the verification area, I'm not. see below, instead of just the function name I'm passing the selector name (function name in pascal case).
alternatively, you can pass CallContract instead of call_contract, and then you can get the function name by using paste:

paste::paste! {
    fn [< $selector:snake >]( .. ) { .. }
}

@aner-starkware
Copy link
Contributor Author

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

Previously, dorimedini-starkware wrote…

in the verification area, I'm not. see below, instead of just the function name I'm passing the selector name (function name in pascal case).
alternatively, you can pass CallContract instead of call_contract, and then you can get the function name by using paste:

paste::paste! {
    fn [< $selector:snake >]( .. ) { .. }
}

OK, but in any case this solution checks it here, and not in verify_syscall_ptr - is this important?

@aner-starkware aner-starkware force-pushed the aner/snos_verify_syscall_ptr branch 2 times, most recently from 3d7683f to d86158c Compare May 8, 2025 08:38
@aner-starkware
Copy link
Contributor Author

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

Previously, aner-starkware wrote…

OK, but in any case this solution checks it here, and not in verify_syscall_ptr - is this important?

Additionally, the Err is on the pointers, but in this case we're comparing DeprecatedSyscallSelector - should I create a new Error type?

@aner-starkware aner-starkware force-pushed the aner/snos_verify_syscall_ptr branch from d86158c to bb5b22e Compare May 8, 2025 08:42
Copy link
Collaborator

@dorimedini-starkware dorimedini-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: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)


crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

OK, but in any case this solution checks it here, and not in verify_syscall_ptr - is this important?

this is a good question; can you check that it is equivalent? if the pointer doesn't change between this call and the verify call I think it's fine, but @Yoni-Starkware 's or @ilyalesokhin-starkware 's input may help

should I create a new Error type?

I am in favor, yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants