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

test(blockifier): add builtins_test #3381

Merged

Conversation

Yonatan-Starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch 7 times, most recently from 91981c2 to de294d3 Compare January 19, 2025 17:23
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 4 of 5 files at r1, 2 of 5 files at r2, 2 of 4 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 119 at r3 (raw file):

    let gas_costs = &VersionedConstants::create_for_testing().os_constants.gas_costs;
    let expected_gas = 2 * gas_costs.builtins.poseidon + gas_costs.base.syscall_base_gas_cost;

This is no longer right when running with sierra version >= 1.7.0, right?

Code quote:

    let expected_gas = 2 * gas_costs.builtins.poseidon + gas_costs.base.syscall_base_gas_cost;

crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 30 at r2 (raw file):

    let gas_costs = &VersionedConstants::create_for_testing().os_constants.gas_costs;
    let expected_gas = 27*gas_costs.builtins.range_check + 108*gas_costs.base.step_gas_cost + 2240;

So when people get this magic number, they can see it from this calculation. Please write the number this calculation returns if its different than 882425.

Suggestion:

    // The calculation is: 882425
    let expected_gas = 27*gas_costs.builtins.range_check + 108*gas_costs.base.step_gas_cost + 2240;

crates/blockifier/feature_contracts/cairo1/sierra/test_contract.sierra.json line 5 at r3 (raw file):

    "0x1",
    "0x6",
    "0x0",

That should be 1.7.0. Please run the compilation on compiler version 2.10.0-rc.0

Code quote:

    "0x1",
    "0x6",
    "0x0",

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: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)


crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 750 at r3 (raw file):

        let _h: u32 = x.try_into().unwrap_or_default();
        let _j: u32 = x.try_into().unwrap_or_default();
        let _k: u32 = x.try_into().unwrap_or_default();

Also, check for how to use a loop in cairo1 or just use a simple recursive here. Finally, document the reason range check is different

Suggestion:

        let mut y: u32 = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();
        y = x.try_into().unwrap_or_default();

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 805 at r3 (raw file):

            .done()
            .eval(modulus)
            .unwrap();

Can you explain what you are doing here and where these numbers came from, I think its also worth documenting

Code quote:

        let modulus = TryInto::<_, CircuitModulus>::try_into([7, 0, 0, 0]).unwrap();
        let _outputs = (mul, add, inv)
            .new_inputs()
            .next([3, 0, 0, 0])
            .next([6, 0, 0, 0])
            .done()
            .eval(modulus)
            .unwrap();

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 r3.
Reviewable status: 5 of 6 files reviewed, 5 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch from de294d3 to a017bc7 Compare January 23, 2025 06:45
@Yonatan-Starkware
Copy link
Contributor Author

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 805 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Can you explain what you are doing here and where these numbers came from, I think its also worth documenting

Done.

Copy link
Contributor Author

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


crates/blockifier/feature_contracts/cairo1/sierra/test_contract.sierra.json line 5 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

That should be 1.7.0. Please run the compilation on compiler version 2.10.0-rc.0

Done.

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch 2 times, most recently from 5097a58 to 1fe7796 Compare January 23, 2025 11:35
Copy link
Contributor Author

@Yonatan-Starkware Yonatan-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: 1 of 7 files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1)


crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 750 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Also, check for how to use a loop in cairo1 or just use a simple recursive here. Finally, document the reason range check is different

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 119 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This is no longer right when running with sierra version >= 1.7.0, right?

Done.

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch 2 times, most recently from 7a0dd02 to 549f61f Compare January 23, 2025 13:54
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 4 of 5 files at r6.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)


crates/blockifier/cairo_native line 1 at r6 (raw file):

Subproject commit 76e83965d3bf1252eb6c68200a3accd5fd1ec004

Revert this. Try to run git submodule update --init --recursive if it wont work then let's talk about it f2f

Code quote:

Subproject commit 76e83965d3bf1252eb6c68200a3accd5fd1ec004

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch from 549f61f to e988cfd Compare January 23, 2025 15:11
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 5 files at r1, 2 of 5 files at r4, 5 of 5 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @Yonatan-Starkware)


crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 805 at r3 (raw file):

Previously, Yonatan-Starkware wrote…

Done.

I agree with Meshi


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 12 at r7 (raw file):

use crate::test_utils::initial_test_state::test_state;
use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, RunnableCairo1, BALANCE};

Consider defining one test with different cases that gets as input the entry_point_selector and the gas_consumed (which should be calculated in a formula using the builtin costs from the versioned constants and not as a magic number)


crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 758 at r7 (raw file):

    }

    // Used resources in this function- Poseidon: 1, Step: 17, Hole: 3, RangeCheck: 0.

Please remove and use this number and you calculate the expected gas consumed (the calculation will, in fact, document it)

Code quote:

    // Used resources in this function- Poseidon: 1, Step: 17, Hole: 3, RangeCheck: 0.

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch from e988cfd to c952b8a Compare January 26, 2025 07:22
Copy link
Contributor Author

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


crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 758 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please remove and use this number and you calculate the expected gas consumed (the calculation will, in fact, document it)

Done.

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch 2 times, most recently from abd302a to ccf4981 Compare January 26, 2025 15:47
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 3 files at r7, 2 of 2 files at r8.
Reviewable status: 4 of 7 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)

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 5 files at r14, 3 of 4 files at r15, all commit messages.
Reviewable status: 9 of 11 files reviewed, 7 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch from 35ed7d7 to 8161aa6 Compare February 2, 2025 11:58
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: 7 of 12 files reviewed, 8 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 74 at r15 (raw file):

    if selector_name == "test_circuit" {
        expected_difference -= gas_costs.base.memory_hole_gas_cost;
    }

Do we know what is the difference? We can write something in this manner

Suggestion:

    //There is a slight difference between the gas consumed and vm_resources as the equation 
    // we are using to transform VM resources to gas price is not precise (the ratio is 51/100 pr builtin).
    let mut expected_difference = gas_costs.base.step_gas_cost;
    if selector_name == "test_circuit" {
        // VM resources dose not include memory holes 
        expected_difference -= gas_costs.base.memory_hole_gas_cost;
    }

@Yonatan-Starkware Yonatan-Starkware force-pushed the yonatank/blockifier/add_builtins_test branch 3 times, most recently from b91883f to e7773aa Compare February 2, 2025 15:33
Copy link
Contributor Author

@Yonatan-Starkware Yonatan-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: 5 of 13 files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @noaov1)


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 56 at r10 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This doesn't seem to be the problem. I re-checked, and these tracked resources are only for reverts. Therefore, they are always in Sierra Gas, even in regular runs. Maybe that explains why the Sierra gas is not zero when we increase the minimum Sierra version

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 26 at r13 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

What do you do about the native case? Dose it still pass the test if not, you can ignore it for now and add a todo to add it when native bug is solved

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 55 at r13 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Remove

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 74 at r15 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Do we know what is the difference? We can write something in this manner

Done.

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 4 files at r16, 4 of 5 files at r17, all commit messages.
Reviewable status: 11 of 13 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 47 at r17 (raw file):

    match selector_name {
        "test_pedersen" => {
            os_constants.gas_costs.builtins.pedersen = 10000000;

Extract to constant

Code quote:

 10000000;

crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 53 at r17 (raw file):

        }
        "test_ecop" => {
            os_constants.gas_costs.builtins.ecop = 10000000;

is changing the builtin to 1000000 without setting the other to zero. I think this should be enough

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 5 files at r17.
Reviewable status: 12 of 13 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)

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 r16.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)

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 5 files at r14, 4 of 4 files at r16, 5 of 5 files at r17.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Yonatan-Starkware)


crates/blockifier/src/execution/syscalls/syscall_tests/get_execution_info.rs line 27 at r17 (raw file):

use starknet_types_core::felt::Felt;
use test_case::test_case;

Please revert this file.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 30 at r16 (raw file):

    let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]);

    let calldata = Calldata(vec![].into());

Suggestion:

let calldata = calldata![];

crates/blockifier/src/test_utils/struct_impls.rs line 69 at r17 (raw file):

    }

    pub fn execute_directly_with_block_context(

Suggestion:

pub fn execute_directly_given_block_context(

crates/blockifier/src/test_utils/struct_impls.rs line 87 at r17 (raw file):

    }

    pub fn execute_directly_given_tx_context(

Why was this added?

Code quote:

 pub fn execute_directly_given_tx_context(

crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 37 at r17 (raw file):

    change_builtins_gas_cost(&mut block_context, selector_name);

    let call_info_while_tracking_vm_resources =

Suggestion:

let call_info =

crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 40 at r17 (raw file):

        entry_point_call.execute_directly_with_block_context(&mut state, block_context).unwrap();

    assert!(call_info_while_tracking_vm_resources.execution.gas_consumed >= 1000000);

Suggestion:

    let mut minimal_gas = 1000000;
    if selector_name == "test_circuit" {
        minimal_gas*=2;
    }
    assert!(call_info_while_tracking_vm_resources.execution.gas_consumed >= minimal_gas);

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 9 files at r12, 1 of 5 files at r14, 1 of 4 files at r15, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Yonatan-Starkware)


crates/blockifier/src/execution/deprecated_syscalls/deprecated_syscalls_test.rs line 457 at r17 (raw file):

fn test_tx_info(#[values(false, true)] only_query: bool) {
    use crate::context::{BlockContext, TransactionContext};

Please revert the changes

@meship-starkware meship-starkware force-pushed the yonatank/blockifier/add_builtins_test branch from e7773aa to dd05e9b Compare February 4, 2025 09:24
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 5 of 6 files at r18, all commit messages.
Reviewable status: 12 of 13 files reviewed, 9 unresolved discussions (waiting on @meship-starkware and @Yonatan-Starkware)

@meship-starkware meship-starkware force-pushed the yonatank/blockifier/add_builtins_test branch 3 times, most recently from de60e67 to ce4bb5e Compare February 4, 2025 09:41
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 4 files at r20, 3 of 3 files at r21, all commit messages.
Reviewable status: 10 of 13 files reviewed, 9 unresolved discussions (waiting on @meship-starkware and @Yonatan-Starkware)

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: 10 of 13 files reviewed, 5 unresolved discussions (waiting on @noaov1 and @Yonatan-Starkware)


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 47 at r17 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Extract to constant

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 53 at r17 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

is changing the builtin to 1000000 without setting the other to zero. I think this should be enough

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/get_execution_info.rs line 27 at r17 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please revert this file.

Done.


crates/blockifier/src/test_utils/struct_impls.rs line 87 at r17 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why was this added?

Done.


crates/blockifier/src/execution/deprecated_syscalls/deprecated_syscalls_test.rs line 457 at r17 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please revert the changes

Done.


crates/blockifier/src/test_utils/struct_impls.rs line 69 at r17 (raw file):

    }

    pub fn execute_directly_with_block_context(

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 37 at r17 (raw file):

    change_builtins_gas_cost(&mut block_context, selector_name);

    let call_info_while_tracking_vm_resources =

Done.


crates/blockifier/src/execution/syscalls/syscall_tests/builtins_test.rs line 40 at r17 (raw file):

        entry_point_call.execute_directly_with_block_context(&mut state, block_context).unwrap();

    assert!(call_info_while_tracking_vm_resources.execution.gas_consumed >= 1000000);

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.

Reviewed 3 of 4 files at r20.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yonatan-Starkware)

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 6 files at r18, 1 of 4 files at r20, 3 of 3 files at r21, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yonatan-Starkware)

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 3 of 4 files at r20.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yonatan-Starkware)

@meship-starkware meship-starkware force-pushed the yonatank/blockifier/add_builtins_test branch from ce4bb5e to 5cbc247 Compare February 4, 2025 11:13
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 4 of 4 files at r22, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yonatan-Starkware)

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

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yonatan-Starkware)

@meship-starkware meship-starkware merged commit 31a73d6 into main-v0.13.4 Feb 4, 2025
12 checks passed
@meship-starkware meship-starkware deleted the yonatank/blockifier/add_builtins_test branch February 4, 2025 11:49
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2025
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.

4 participants