chore(blockifier): move test_stack_overflow from out_of_gas.rs to transaction_executor_test.rs

[avivg-starkware]

No description provided.

[github-actions]

Artifacts upload workflows:

• Started at 2025-01-20T13:11:19Z
• Started at 2025-01-20T14:41:50Z
• Started at 2025-01-20T14:52:48Z
• Started at 2025-01-20T15:55:45Z
• Started at 2025-01-20T17:15:17Z
• Started at 2025-01-21T16:10:45Z
• Started at 2025-01-21T18:03:22Z
• Started at 2025-01-22T09:29:01Z
• Started at 2025-01-22T09:30:00Z
• Started at 2025-01-22T09:50:13Z
• Started at 2025-01-22T15:41:14Z
• Started at 2025-01-22T15:41:45Z
• Started at 2025-01-22T15:43:45Z
• Started at 2025-01-22T15:44:56Z
• Started at 2025-01-27T10:28:58Z
• Started at 2025-01-27T11:03:14Z
• Started at 2025-01-27T14:55:01Z

[reviewable-StarkWare]

This change is 

[avivg-starkware]

• chore(blockifier): move test_stack_overflow from out_of_gas.rs to transaction_executor_test.rs #3515 👈 (View in Graphite)
• fix(cairo_native): change stack size to max using threads #3401
• main-v0.13.4

This stack of pull requests is managed by Graphite. Learn more about stacking.

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 405 at r1 (raw file):

fn test_stack_overflow(
    #[case] initial_gas: u64,
    #[case] _gas_consumed: u64, // TODO (AvivG): Should check gas consumed?

Yes, this is an important part of the test. We want to see that it is close to the initial gas

Code quote:

#[case] _gas_consumed: u64, // TODO (AvivG): Should check gas consumed?

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 440 at r1 (raw file):

        // let result = result.as_ref().unwrap();
        // assert!(result.is_reverted());
        Ok(execution_res) => execution_res.revert_error.clone().unwrap().to_string(),

When do we expect to get an error and not a reverted transaction?

Code quote:

        Err(execution_err) => execution_err.to_string(),
        // Reverted transaction, error info within revert_error.
        // TODO(AvivG): should check "is_reverted"?
        // let result = result.as_ref().unwrap();
        // assert!(result.is_reverted());
        Ok(execution_res) => execution_res.revert_error.clone().unwrap().to_string(),

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r1 (raw file):

    #[case] initial_gas: u64,
    #[case] _gas_consumed: u64, // TODO (AvivG): Should check gas consumed?
    #[values(CairoVersion::Cairo1(RunnableCairo1::Native))] cairo_version: CairoVersion,

Why? its only have one value I don't see a reason to add this. I don't think checking casm or cairo0 in this test would benefit us

Code quote:

 #[values(CairoVersion::Cairo1(RunnableCairo1::Native))] cairo_version: CairoVersion,

[avivg-starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 405 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Yes, this is an important part of the test. We want to see that it is close to the initial gas

Done.

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Why? its only have one value I don't see a reason to add this. I don't think checking casm or cairo0 in this test would benefit us

wanted to ask whether testing other values is needed. if this val is the only one needed - will remove

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 440 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

When do we expect to get an error and not a reverted transaction?

when resources aren't enough for validate

[meship-starkware]

Reviewed 4 of 13 files at r4.
Reviewable status: 1 of 13 files reviewed, 1 unresolved discussion (waiting on @noaov1)

[meship-starkware]

Reviewable status: 1 of 13 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

The problem with the stack only arises when running with native, but I think that in this state, the runnable version does not matter because it will be decided by the Sierra version. Now that I think about it, @noaov1, do we even run native in this case? What is the state reader?

It seems like we are using the DictStateReader, which cannot run native yet. Can we use the Papyrus State here and add Native to the contract_class_manager cache to make sure we are running in native? maybe we will have to move this test to the native blockifier tests

[meship-starkware]

Reviewable status: 1 of 13 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 437 at r5 (raw file):

    };
    print!("{}", resource_bounds.get_l2_bounds());
    // assert!(false);

Remove

Code quote:

    print!("{}", resource_bounds.get_l2_bounds());
    // assert!(false);

[meship-starkware]

Reviewed all commit messages.
Reviewable status: 1 of 13 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[avivg-starkware]

Reviewable status: 0 of 15 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It seems like we are using the DictStateReader, which cannot run native yet. Can we use the Papyrus State here and add Native to the contract_class_manager cache to make sure we are running in native? maybe we will have to move this test to the native blockifier tests

I'm not sure I understand your intention regarding DictStateReader and Papyrus State, however I do think I can address your concern-
I added a print under native_entry_point_execution::execute_entry_point_call --> that was indeed outputted.
I also managed to reach stack overflow which (if i'm not mistaken -) confirms the use of Native.

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 437 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Remove

Done.

[noaov1]

Reviewed 1 of 4 files at r6, all commit messages.
Reviewable status: 1 of 15 files reviewed, 5 unresolved discussions (waiting on @avivg-starkware and @meship-starkware)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r1 (raw file):

which cannot run native yet.

Why? If you set the native class in class_hash_to_class it will extract the native.

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 384 at r6 (raw file):

/// overflow.
///
/// Note: Testing the VM is unnecessary here as it does not utilize the stack.

Suggestion:

/// Note: Testing the VM is unnecessary here as it simulates the stack where the stack in the heap as a memory segment.

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 402 at r6 (raw file):

    let entry_point_args = vec![depth];
    let calldata = create_calldata(contract_address, "test_stack_overflow", &entry_point_args);
    let tx = executable_invoke_tx(invoke_tx_args! {

Suggestion:

let invoke_tx = executable_invoke_tx(invoke_tx_args! {

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r6 (raw file):

        calldata,
        nonce: nonce_manager.next(account_address),
    });

Can you please make sure that the ls_max_amount is at least execute_max_sierra_gas + validate_max_sierra_gas?

Code quote:

    let tx = executable_invoke_tx(invoke_tx_args! {
        sender_address: account_address,
        calldata,
        nonce: nonce_manager.next(account_address),
    });

[meship-starkware]

Reviewable status: 1 of 15 files reviewed, 4 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

which cannot run native yet.

Why? If you set the native class in class_hash_to_class it will extract the native.

Oh I see when creating the state we use the get_runnable_version. So the runnable version is important and we only need to use native for now

[meship-starkware]

Reviewed 3 of 4 files at r6, all commit messages.
Reviewable status: 4 of 15 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[avivg-starkware]

Reviewable status: 1 of 15 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 406 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please make sure that the ls_max_amount is at least execute_max_sierra_gas + validate_max_sierra_gas?

Done.

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 384 at r6 (raw file):

/// overflow.
///
/// Note: Testing the VM is unnecessary here as it does not utilize the stack.

Done

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 402 at r6 (raw file):

    let entry_point_args = vec![depth];
    let calldata = create_calldata(contract_address, "test_stack_overflow", &entry_point_args);
    let tx = executable_invoke_tx(invoke_tx_args! {

Done

[noaov1]

Reviewed 3 of 14 files at r7, all commit messages.
Reviewable status: 4 of 15 files reviewed, all discussions resolved (waiting on @meship-starkware)

[avivg-starkware]

Reviewed 1 of 4 files at r6, 14 of 14 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

[meship-starkware]

Reviewed 14 of 14 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

[noaov1]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)

---

crates/blockifier/src/blockifier/transaction_executor_test.rs line 395 at r7 (raw file):

    //      use crate::abi::constants::MAX_POSSIBLE_SIERRA_GAS;
    //      let os_constants = Arc::make_mut(&mut block_context.versioned_constants.os_constants);
    //      os_constants.execute_max_sierra_gas = MAX_POSSIBLE_SIERRA_GAS.into();

Please remove :)

Code quote:

    // TODO (AvivG): remove this comment:
    // To reach stack overflow, increase maximum gas by refactoring os_constants
    //      let mut block_context = BlockContext::create_for_account_testing();
    //      use std::sync::Arc;
    //      use crate::abi::constants::MAX_POSSIBLE_SIERRA_GAS;
    //      let os_constants = Arc::make_mut(&mut block_context.versioned_constants.os_constants);
    //      os_constants.execute_max_sierra_gas = MAX_POSSIBLE_SIERRA_GAS.into();

[avivg-starkware]

crates/blockifier/src/blockifier/transaction_executor_test.rs line 395 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please remove :)

Done.

[noaov1]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)