starknet_os: os resources test - measure linear factor, add deploy#14134
Conversation
PR SummaryLow Risk Overview The Cairo1
Reviewed by Cursor Bugbot for commit 55dfa63. Bugbot is set up for automated code reviews on this repo. Configure here. |
a3596f2 to
ec08192
Compare
311e125 to
a775a63
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 5 files and all commit messages, made 9 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on dorimedini-starkware and Yoni-Starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 235 at r4 (raw file):
Previously, dorimedini-starkware wrote…
each measurable syscall can
- have a linear factor or not
- be a virtual builtin or not
- incur inner calls or not
- be keccak or not (which is in itself a special case)
since the above are not necessarily disjoint sets, I don't think an enum will help. what did you have in mind?
OK, I assumed the sets are disjoint.
crates/starknet_os_flow_tests/src/os_resources_test.rs line 79 at r5 (raw file):
/// Store a mapping from a linearly-charged syscall, with the number of "linear elements" in it's /// base measurement. For example, if we measure the base and linear costs of a [Selector::Deploy]
Base is a measurement with no linear elements.
Suggestion:
first measurementcrates/starknet_os_flow_tests/src/os_resources_test.rs line 83 at r5 (raw file):
/// ``` /// deploy_syscall(stable_class_hash, 3, array![2, 0, 0].span(), true).unwrap_syscall(); /// deploy_syscall(stable_class_hash, 3, array![3, 0, 0, 0].span(), true).unwrap_syscall();
What will be the value in the mapping for this example?
Code quote:
/// deploy_syscall(stable_class_hash, 3, array![2, 0, 0].span(), true).unwrap_syscall();
/// deploy_syscall(stable_class_hash, 3, array![3, 0, 0, 0].span(), true).unwrap_syscall();crates/starknet_os_flow_tests/src/os_resources_test.rs line 85 at r5 (raw file):
/// deploy_syscall(stable_class_hash, 3, array![3, 0, 0, 0].span(), true).unwrap_syscall(); /// ``` /// ... then the linear factor is exactly the difference between the two measurements, because the
Is the difference between the number of linear elements in both measurements always 1?
It would be more valuable to focus here on the assumptions about the linear syscalls in the contract (two consecutive readings), rather than the calculation details that covered in the code below.
Code quote:
exactly the difference between the two measurementscrates/starknet_os_flow_tests/src/os_resources_test.rs line 244 at r5 (raw file):
// iterator. let inner_overhead = fetch_inner_resources(selector); // The resources measured here are one of two types: constant, or base cost of a syscall
It still includes the linear cost.
You can remove the whole comment.
Code quote:
or base costcrates/starknet_os_flow_tests/src/os_resources_test.rs line 249 at r5 (raw file):
(syscall_trace.get_resources().unwrap() - &inner_overhead).filter_unused_builtins(); // If this if a syscall with a linear factor, the next syscall should be the linear cost.
Based on the second reading, we can infer the linear cost.
Code quote:
should be the linear costcrates/starknet_os_flow_tests/src/os_resources_test.rs line 265 at r5 (raw file):
let next_resources = (next_syscall_trace.get_resources().unwrap() - &next_inner_overhead) .filter_unused_builtins();
It is a duplication of the code above:
let inner_overhead = fetch_inner_resources(selector);
// The resources measured here are one of two types: constant, or base cost of a syscall
// with a linear factor.
let mut resources =
(syscall_trace.get_resources().unwrap() - &inner_overhead).filter_unused_builtins();
Consider reuse.
Code quote:
let next_inner_overhead = fetch_inner_resources(selector);
let next_resources = (next_syscall_trace.get_resources().unwrap()
- &next_inner_overhead)
.filter_unused_builtins();crates/starknet_os_flow_tests/src/os_resources_test.rs line 266 at r5 (raw file):
- &next_inner_overhead) .filter_unused_builtins(); let linear_factor_resources = (&next_resources - &resources).filter_unused_builtins();
Assuming the next syscall has exactly one more linear element. Is it the case?
Code quote:
let linear_factor_resources = (&next_resources - &resources).filter_unused_builtins();crates/starknet_os_flow_tests/src/os_resources_test.rs line 270 at r5 (raw file):
// Linear factor is computed; deduct the linear overhead from the base cost to get the // real base cost. resources = (&resources - &(&linear_factor_resources * *linear_count_in_base))
Suggestion:
constant_resources =1ae17f6 to
a616966
Compare
dba61e7 to
a2fa830
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 8 comments and resolved 1 discussion.
Reviewable status: 6 of 7 files reviewed, 8 unresolved discussions (waiting on yoavGrs and Yoni-Starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 79 at r5 (raw file):
Previously, yoavGrs wrote…
Base is a measurement with no linear elements.
Done.
crates/starknet_os_flow_tests/src/os_resources_test.rs line 83 at r5 (raw file):
Previously, yoavGrs wrote…
What will be the value in the mapping for this example?
Done.
crates/starknet_os_flow_tests/src/os_resources_test.rs line 85 at r5 (raw file):
Previously, yoavGrs wrote…
Is the difference between the number of linear elements in both measurements always 1?
It would be more valuable to focus here on the assumptions about the linear syscalls in the contract (two consecutive readings), rather than the calculation details that covered in the code below.
added a comment about it, better?
crates/starknet_os_flow_tests/src/os_resources_test.rs line 244 at r5 (raw file):
Previously, yoavGrs wrote…
It still includes the linear cost.
You can remove the whole comment.
Done.
crates/starknet_os_flow_tests/src/os_resources_test.rs line 249 at r5 (raw file):
Previously, yoavGrs wrote…
Based on the second reading, we can infer the linear cost.
better?
crates/starknet_os_flow_tests/src/os_resources_test.rs line 265 at r5 (raw file):
Previously, yoavGrs wrote…
It is a duplication of the code above:
let inner_overhead = fetch_inner_resources(selector); // The resources measured here are one of two types: constant, or base cost of a syscall // with a linear factor. let mut resources = (syscall_trace.get_resources().unwrap() - &inner_overhead).filter_unused_builtins();Consider reuse.
consolidated
crates/starknet_os_flow_tests/src/os_resources_test.rs line 266 at r5 (raw file):
Previously, yoavGrs wrote…
Assuming the next syscall has exactly one more linear element. Is it the case?
I mention the +1 thing in the new comment above, good enough?
and yes, this is always the case
crates/starknet_os_flow_tests/src/os_resources_test.rs line 270 at r5 (raw file):
// Linear factor is computed; deduct the linear overhead from the base cost to get the // real base cost. resources = (&resources - &(&linear_factor_resources * *linear_count_in_base))
Done.
a616966 to
da034b0
Compare
a2fa830 to
9174010
Compare
|
Artifacts upload workflows: |
da034b0 to
cf5157a
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 2 files and all commit messages, made 3 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware and Yoni-Starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 85 at r5 (raw file):
Previously, dorimedini-starkware wrote…
added a comment about it, better?
Best
crates/starknet_os_flow_tests/src/os_resources_test.rs line 234 at r6 (raw file):
ExecutionResources::default() }; (total - &to_deduct).filter_unused_builtins()
Consider, non-blocking
Suggestion:
if selector.is_calling_syscall() {
// TODO(Dori): Take opcodes (like blake) into account, instead of using the
// vm_resources field.
let to_deduct = inner_calls_iter.next().unwrap().resources.vm_resources;
(total - &to_deduct).filter_unused_builtins()
} else {
total
};
crates/starknet_os_flow_tests/src/os_resources_test.rs line 249 at r6 (raw file):
let resources = maybe_deduct_inner(syscall_trace.get_resources().unwrap(), selector); // If this if a syscall with a linear factor, the next syscall should be an invocation of
Suggestion:
this is acf5157a to
7167361
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on yoavGrs and Yoni-Starkware).
crates/starknet_os_flow_tests/src/os_resources_test.rs line 249 at r6 (raw file):
let resources = maybe_deduct_inner(syscall_trace.get_resources().unwrap(), selector); // If this if a syscall with a linear factor, the next syscall should be an invocation of
Done.
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 7167361. Configure here.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 1 comment.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on yoavGrs and Yoni-Starkware).
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Yoni-Starkware).
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Yoni-Starkware).
7167361 to
c34d43e
Compare
c34d43e to
55dfa63
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yoni-Starkware).
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on dorimedini-starkware).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware resolved 1 discussion and dismissed @Yoni-Starkware from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on dorimedini-starkware).


No description provided.