starknet_os: os resources test - remove all fee transfer syscalls#14137
Conversation
PR SummaryLow Risk Overview The OS resources test contract’s In Reviewed by Cursor Bugbot for commit 1b6cf5e. Bugbot is set up for automated code reviews on this repo. Configure here. |
0d44efb to
cd7ac82
Compare
2587a2e to
a547012
Compare
07b4e34 to
6683ff3
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs made 3 comments.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 121 at r3 (raw file):
/// the measurements, we use a stable dummy contract (that is not recompiled when the Cairo1 /// compiler's version changes), and we set the `deploy_from_zero` flag to `true` to make sure /// changes in the deploying contract address are not reflected in the measurements.
Move this doc down.
Doc this new test.
Code quote:
/// Measure the OS overhead for each syscall, and compare the results with the latest VC.
///
/// This test relies on the [starknet_os::hint_processor::os_logger::OsLogger] to capture the
/// resources used by the OS when running a syscall. A "checkpoint" is made before entering a
/// syscall implementation, and after the syscall execution returns, the difference between the two
/// is stored in the logger's traces.
///
/// Some notes about these measurements:
/// 1. Some syscalls incur inner calls ([Selector::CallContract], for example). The resources
/// consumed by the inner logic must be subtracted from the measured overhead to get the actual
/// OS overhead.
/// 2. Some syscalls incur overhead that depends linearly on the length of the input to the syscall.
/// In these cases, the measuring contract calls the syscall twice in a row, the second call
/// having "one more" input than the first call; subtracting the sequential measurements gives
/// the linear factor of the syscall. One caveat here is that the linear factor of
/// [Selector::Keccak] is stored as a separate syscall cost ([Selector::KeccakRound]).
/// 3. The SHA family syscalls are implemented as "virtual builtins": the syscall execution only
/// pushes the inputs to a special memory segment, and the "heavy lifting" is done later. This
/// means that the overhead of the syscall is not captured by the `OsLogger`. These syscalls have
/// separate tests to measure their overhead.
/// 4. The [Selector::Deploy] syscall's overhead depends on the deployed contract address in a non-
/// trivial way (see the `normalize_address` function in the cairo-lang core). To avoid noise in
/// the measurements, we use a stable dummy contract (that is not recompiled when the Cairo1
/// compiler's version changes), and we set the `deploy_from_zero` flag to `true` to make sure
/// changes in the deploying contract address are not reflected in the measurements.crates/starknet_os_flow_tests/src/os_resources_test.rs line 275 at r3 (raw file):
let all_syscalls = test_output.runner_output.txs_trace.last().unwrap().get_syscalls().clone(); let (syscall_traces, fee_transfer_syscall_traces) = all_syscalls.split_at(all_syscalls.len() - FEE_TRANSFER_SYSCALLS.len());
Consider splitting the iterator (take / skip) instead of the syscalls vector.
non-blocking
Code quote:
let all_syscalls = test_output.runner_output.txs_trace.last().unwrap().get_syscalls().clone();
let (syscall_traces, fee_transfer_syscall_traces) =
all_syscalls.split_at(all_syscalls.len() - FEE_TRANSFER_SYSCALLS.len());crates/starknet_os_flow_tests/src/os_resources_test.rs line 360 at r3 (raw file):
// Make sure there are no more dangling syscalls. let dangling_syscall = syscalls_iter.next();
How can it happen after while let Some(syscall_trace) = syscalls_iter.next()?
Code quote:
// Make sure there are no more dangling syscalls.
let dangling_syscall = syscalls_iter.next();6683ff3 to
3ee3ec7
Compare
a547012 to
18f0bed
Compare
3ee3ec7 to
8a9154e
Compare
18f0bed to
21be0eb
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 4 comments.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware, yoavGrs, and Yoni-Starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 80 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
You can run this tx with trivial fee bounds and make this test a bit cleaner
I don't want to break if for some reason we want to avoid trivial resource bounds... we need to override the VC already to run in cairo-steps mode, I can imagine a scenario in which we will no longer allow "free" txs in cairo-steps mode. the current way adds more code but it is closer to a "real flow"
crates/starknet_os_flow_tests/src/os_resources_test.rs line 121 at r3 (raw file):
Previously, yoavGrs wrote…
Move this doc down.
Doc this new test.
Done.
crates/starknet_os_flow_tests/src/os_resources_test.rs line 275 at r3 (raw file):
Previously, yoavGrs wrote…
Consider splitting the iterator (take / skip) instead of the syscalls vector.
non-blocking
what is the benefit? I want to verify the second part is equal to the expected fee-transfer syscalls, and iterate over the first part
crates/starknet_os_flow_tests/src/os_resources_test.rs line 360 at r3 (raw file):
Previously, yoavGrs wrote…
How can it happen after
while let Some(syscall_trace) = syscalls_iter.next()?
can't, leftover code. thanks
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 1 file and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yoni-Starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 275 at r3 (raw file):
Previously, dorimedini-starkware wrote…
what is the benefit? I want to verify the second part is equal to the expected fee-transfer syscalls, and iterate over the first part
Avoid cloning, but that's not important enough in tests
8a9154e to
fa3879f
Compare
98571a1 to
cc6f1b8
Compare
fa3879f to
b72cdb7
Compare
b72cdb7 to
fd9c8c3
Compare
cc6f1b8 to
3fe49f1
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3fe49f1. Configure here.
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on dorimedini-starkware).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on dorimedini-starkware).
3fe49f1 to
1b6cf5e
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on dorimedini-starkware).


No description provided.