Skip to content

Conversation

@meship-starkware
Copy link
Collaborator

No description provided.

@meship-starkware
Copy link
Collaborator Author

Src Dst crates/blockifier/src/bouncer_test.rs
Src Dst crates/blockifier/src/transaction/objects_test.rs

@reviewable-StarkWare
Copy link

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Jul 3, 2025

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 3 of 4 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: 4 of 8 files reviewed, all discussions resolved (waiting on @avivg-starkware)

Copy link
Contributor

@avivg-starkware avivg-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 6 files at r6, 1 of 4 files at r7, 1 of 1 files at r8.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/bouncer.rs line 385 at r8 (raw file):

            add_mod: 312,
            mul_mod: 604,
            range_check96: 56,

I see the builtins values were changed again?

Code quote:

            pedersen: 10125,
            range_check: 70,
            ecdsa: 1666666,
            ec_op: 714875,
            bitwise: 583,
            keccak: 510707,
            poseidon: 6250,
            add_mod: 312,
            mul_mod: 604,
            range_check96: 56,

crates/blockifier/src/test_utils/struct_impls.rs line 205 at r8 (raw file):

                    ..BouncerWeights::max()
                },
                // TODO(Meshi): Check what should be the values here.

making sure this is not needed?

Code quote:

// TODO(Meshi): Check what should be the values here.

crates/blockifier/src/bouncer_test.rs line 203 at r8 (raw file):

    #[case] scenario: &'static str,
) {
    let block_context = BlockContext::create_for_account_testing();

The use of the fixtures was lost here.
Also, consider adding a TODO to try to make use of the other fixtures

//TODO(AvivG): consider use of fixtures here.

Suggestion:

fn test_bouncer_try_update_gas_based(
    #[case] sierra_gas: GasAmount,
    #[case] scenario: &'static str,
    block_context: BlockContext,
) {

crates/blockifier/src/bouncer_test.rs line 278 at r8 (raw file):

    let block_max_capacity = BouncerWeights { sierra_gas: GasAmount(20), ..Default::default() };
    let bouncer_config =
        BouncerConfig { block_max_capacity, builtin_weights: BuiltinWeights::default() };

Consider...

//TODO(AvivG): consider use of fixtures here.

Suggestion:

fn test_transaction_too_large_sierra_gas_based(
     block_context: BlockContext,
) {
    let mut state = test_state(&block_context.chain_info, Fee(0), &[]);
    let mut transactional_state = TransactionalState::create_transactional(&mut state);
    let block_max_capacity = BouncerWeights { sierra_gas: GasAmount(20), ..Default::default() };
    let bouncer_config =
        BouncerConfig { block_max_capacity, builtin_weights: BuiltinWeights::default() };

crates/native_blockifier/src/py_block_executor.rs line 362 at r8 (raw file):

                    ..BouncerWeights::max()
                },
                // TODO(Meshi): Check what should be the values here.

making sure Ok

Code quote:

// TODO(Meshi): Check what should be the values here.

@noaov1 noaov1 requested a review from avivg-starkware July 3, 2025 12:53
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 6 files at r6, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avivg-starkware and @meship-starkware)


crates/blockifier/src/bouncer_test.rs line 203 at r8 (raw file):

Previously, avivg-starkware wrote…

The use of the fixtures was lost here.
Also, consider adding a TODO to try to make use of the other fixtures

//TODO(AvivG): consider use of fixtures here.

Nice catch!
Same applies for block_max_capacity (though you need to override the proving_gas field), bouncer_config and state.


crates/blockifier/src/bouncer_test.rs line 200 at r9 (raw file):

#[case::proving_gas_block_full(GasAmount(0), "proving_gas_block_full")]
fn test_bouncer_try_update_gas_based(
    #[case] sierra_gas: GasAmount,

Why is this needed?
@avivg-starkware
(can you do the same "trick" you do with the proving gas?)
(separate PR)

Code quote:

    #[case] sierra_gas: GasAmount,

@meship-starkware meship-starkware force-pushed the meshi/merge-main-v0.13.6-into-main-v0.14.0-1751523971 branch from b6166d7 to 3a8eaa9 Compare July 3, 2025 12:57
Copy link
Collaborator 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: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @avivg-starkware and @noaov1)


crates/blockifier/src/bouncer.rs line 385 at r8 (raw file):

Previously, avivg-starkware wrote…

I see the builtins values were changed again?

Yes, these are the numbers according to the fact that we do not open most of the contract entries


crates/blockifier/src/bouncer_test.rs line 203 at r8 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Nice catch!
Same applies for block_max_capacity (though you need to override the proving_gas field), bouncer_config and state.

Done.


crates/blockifier/src/bouncer_test.rs line 278 at r8 (raw file):

Previously, avivg-starkware wrote…

Consider...

//TODO(AvivG): consider use of fixtures here.

Done.


crates/blockifier/src/test_utils/struct_impls.rs line 205 at r8 (raw file):

Previously, avivg-starkware wrote…

making sure this is not needed?

Done.


crates/native_blockifier/src/py_block_executor.rs line 362 at r8 (raw file):

Previously, avivg-starkware wrote…

making sure Ok

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 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avivg-starkware)

Copy link
Contributor

@avivg-starkware avivg-starkware 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 2 of 4 files at r7, 2 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware)


crates/blockifier/src/bouncer_test.rs line 200 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is this needed?
@avivg-starkware
(can you do the same "trick" you do with the proving gas?)
(separate PR)

@meship-starkware - Add a TODO and I'll take a look? I think it can be added here:

@noaov1 - sounds Ok?

Code snippet (i):

 let builtin_counters = match scenario {
        "proving_gas_block_full" => max_capacity_builtin_counters.clone(),
        // Use a minimal or empty map.
        "ok" | "sierra_gas_block_full" => {
            HashMap::from([(BuiltinName::range_check, range_check_count - 1)])
        }
        _ => panic!("Unexpected scenario: {}", scenario),
    };

Code snippet (ii):

---->
let (builtin_counters, sierra_gas) = match scenario { ....

@avivg-starkware
Copy link
Contributor

crates/blockifier/src/bouncer_test.rs line 218 at r10 (raw file):

        }
        _ => panic!("Unexpected scenario: {}", scenario),
    };

Following @noaov1 's request above (I ran it, this should work)

Suggestion:

#[rstest]
#[case::sierra_gas_positive_flow("ok")]
#[case::sierra_gas_block_full("sierra_gas_block_full")]
#[case::proving_gas_positive_flow("ok")]
#[case::proving_gas_block_full("proving_gas_block_full")]
fn test_bouncer_try_update_gas_based(#[case] scenario: &'static str, block_context: BlockContext) {
    let state = &mut test_state(&block_context.chain_info, Fee(0), &[]);
    let mut transactional_state = TransactionalState::create_transactional(state);
    let builtin_weights = BuiltinWeights::default();

    let range_check_count = 2;
    let max_capacity_builtin_counters =
        HashMap::from([(BuiltinName::range_check, range_check_count)]);
    let builtin_counters = match scenario {
        "proving_gas_block_full" => max_capacity_builtin_counters.clone(),
        // Use a minimal or empty map.
        "ok" | "sierra_gas_block_full" => {
            HashMap::from([(BuiltinName::range_check, range_check_count - 1)])
        }
        _ => panic!("Unexpected scenario: {}", scenario),
    };

    // Derive sierra_gas from scenario
    let sierra_gas = match scenario {
        "sierra_gas_block_full" => GasAmount(11), // Exceeds capacity
        "ok" | "proving_gas_block_full" => GasAmount(1), // Within capacity
        _ => panic!("Unexpected scenario: {}", scenario),
    };

@avivg-starkware
Copy link
Contributor

crates/blockifier/src/bouncer_test.rs line 200 at r9 (raw file):

Previously, avivg-starkware wrote…

@meship-starkware - Add a TODO and I'll take a look? I think it can be added here:

@noaov1 - sounds Ok?

see suggestion below

Copy link
Collaborator 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.

Reviewed 3 of 6 files at r6, 4 of 5 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

Copy link
Contributor

@avivg-starkware avivg-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 r11, 1 of 1 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

@meship-starkware meship-starkware added this pull request to the merge queue Jul 6, 2025
@meship-starkware meship-starkware removed this pull request from the merge queue due to a manual request Jul 6, 2025
@meship-starkware meship-starkware added this pull request to the merge queue Jul 6, 2025
Merged via the queue into main-v0.14.0 with commit 464f038 Jul 6, 2025
18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 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.

5 participants