Skip to content

Conversation

@AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

AvivYossef-starkware commented Dec 16, 2025

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

@AvivYossef-starkware AvivYossef-starkware marked this pull request as ready for review December 16, 2025 17:53
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/split_execute_tx_logic to graphite-base/10849 December 16, 2025 19:18
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch from 658bcbe to 6f72331 Compare December 17, 2025 09:29
@AvivYossef-starkware AvivYossef-starkware changed the base branch from graphite-base/10849 to aviv/split_execute_tx_logic December 17, 2025 09:30
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch 2 times, most recently from e899104 to 92b91ea Compare December 17, 2025 09:45
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/split_execute_tx_logic to graphite-base/10849 December 17, 2025 10:02
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch from 92b91ea to eca0c08 Compare December 17, 2025 10:02
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/split_execute_tx_logic to graphite-base/10849 December 18, 2025 09:11
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch from 5ec05e6 to b830776 Compare December 18, 2025 09:11
@AvivYossef-starkware AvivYossef-starkware changed the base branch from graphite-base/10849 to aviv/split_execute_tx_logic December 18, 2025 09:12
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/split_execute_tx_logic to main December 18, 2025 09:40
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch from b830776 to 4092854 Compare December 18, 2025 10:50
@AvivYossef-starkware AvivYossef-starkware changed the title starknet_os_runner: execution data provider starknet_os_runner: virtual block executor Dec 18, 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.

@noaov1 reviewed 6 files and all commit messages, and made 4 comments.
Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @AvivYossef-starkware).


crates/starknet_os_runner/src/virtual_block_executor.rs line 21 at r3 (raw file):

/// Captures execution data for a virtual block (multiple transactions).
///
/// A virtual block is a set of transactions executed together without block preprocessing,

?

Code quote:

block preprocessing,

crates/starknet_os_runner/src/virtual_block_executor.rs line 23 at r3 (raw file):

/// A virtual block is a set of transactions executed together without block preprocessing,
/// useful for OS input generation and proving. This struct contains all the execution
/// outputs, block context, and initial state reads needed for proof generation.

Suggestion:

/// useful for OS input generation and proving. This struct contains all the transaction execution
/// outputs, block context, and initial state reads needed for proof generation.

crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

/// # Note
///
/// Currently only Invoke transactions are supported.

Why does it matter?

Code quote:

/// Currently only Invoke transactions are supported.

crates/starknet_os_runner/src/virtual_block_executor.rs line 59 at r3 (raw file):

/// ```
pub trait VirtualBlockExecutor {
    /// Executes a virtual block at the given block number.

Suggestion:

    /// Executes a virtual block based on the state abd context of the given block number.

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AvivYossef-starkware made 2 comments and resolved 2 discussions.
Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1).


crates/starknet_os_runner/src/virtual_block_executor.rs line 21 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

?

here


crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why does it matter?

The way I convert it to a blockifier transaction.

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.

@noaov1 reviewed 2 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avi-starkware and @AvivYossef-starkware).


crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

Previously, AvivYossef-starkware wrote…

The way I convert it to a blockifier transaction.

Yes, I saw. Why do you enforce it?
Why not use api_txs_to_blockifier_txs_next_block (and rename it)?


crates/starknet_os_runner/src/virtual_block_executor.rs line 26 at r5 (raw file):

pub struct VirtualBlockExecutionData {
    /// Execution outputs for all transactions in the virtual block.
    pub execution_outputs: Vec<TransactionExecutionOutput>,

Why do we need the state maps?

Code quote:

TransactionExecutionOutput

crates/starknet_os_runner/src/virtual_block_executor.rs line 45 at r5 (raw file):

///
/// - Currently only Invoke transactions are supported.
/// - Validation and fee charging are always skipped (useful for simulation/proving).

What do you mean by validations? __validate__?

Code quote:

/// - Validation and fee charging are always skipped (useful for simulation/proving).

crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

            validate: true,
            charge_fee: false,
            strict_nonce_check: false,

Why?

Code quote:

            strict_nonce_check: false,

crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 205 at r5 (raw file):

    /// Creates an RpcStateReader from a node URL, chain ID, and block number.
    pub fn new_with_default_config(

Suggestion:

pub fn new_with_config_from_url(

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch from db71d8f to 083945b Compare December 18, 2025 13:33
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AvivYossef-starkware made 4 comments and resolved 1 discussion.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware).


crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Yes, I saw. Why do you enforce it?
Why not use api_txs_to_blockifier_txs_next_block (and rename it)?

it would fail in declare tx here
I wanted to clarify this.


crates/starknet_os_runner/src/virtual_block_executor.rs line 26 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why do we need the state maps?

should I verify that the state diff is empty?


crates/starknet_os_runner/src/virtual_block_executor.rs line 45 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What do you mean by validations? __validate__?

Done


crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why?

@Yoni-Starkware

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/starknet_os_runner_simulate_tx branch from 083945b to dc89ace Compare December 18, 2025 13:46
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.

@noaov1 reviewed 2 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @AvivYossef-starkware, and @Yoni-Starkware).


crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

Previously, AvivYossef-starkware wrote…

it would fail in declare tx here
I wanted to clarify this.

I see


crates/starknet_os_runner/src/virtual_block_executor.rs line 26 at r5 (raw file):

Previously, AvivYossef-starkware wrote…

should I verify that the state diff is empty?

I thought that we will verify it in the os itself.

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.

@noaov1 made 1 comment and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @AvivYossef-starkware, and @Yoni-Starkware).


crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

Previously, AvivYossef-starkware wrote…

@Yoni-Starkware

Oh, I see, we don't want to enforce strict nonce as the state of the account does not matter

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:

@noaov1 made 1 comment and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware and @Yoni-Starkware).

@AvivYossef-starkware AvivYossef-starkware added this pull request to the merge queue Dec 21, 2025
Merged via the queue into main with commit cd04346 Dec 21, 2025
33 checks passed
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yoni-Starkware made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved.


crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Oh, I see, we don't want to enforce strict nonce as the state of the account does not matter

@AvivYossef-starkware, you should set the charge_fee flag the same as the blockifier. Currently, the OS skips the fee charging only if the resource bounds are "zero"

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yoni-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion.


crates/starknet_os_runner/src/virtual_block_executor.rs line 205 at r6 (raw file):

        Ok(VirtualBlockExecutionData { execution_outputs, block_context, initial_reads })
    }

Most of this code is general. Since you defined a trait for that, please move it to the trait, and extract out the necessary parts (like state reader fetch)

Code quote:

    fn execute_inner(
        &self,
        block_number: BlockNumber,
        txs: Vec<BlockifierTransaction>,
    ) -> Result<VirtualBlockExecutionData, VirtualBlockExecutorError> {
        // Create RPC state reader for the given block.
        let rpc_state_reader = RpcStateReader::new_with_config_from_url(
            self.node_url.clone(),
            self.chain_id.clone(),
            block_number,
        );

        // Get block context from RPC.
        let block_context = rpc_state_reader
            .get_block_context()
            .map_err(|e| VirtualBlockExecutorError::ReexecutionError(Box::new(e)))?;

        // Create state reader with contract manager.
        let state_reader_and_contract_manager = StateReaderAndContractManager::new(
            rpc_state_reader,
            self.contract_class_manager.clone(),
            None,
        );

        let block_state = CachedState::new(state_reader_and_contract_manager);

        // Create executor WITHOUT preprocessing (no pre_process_block call).
        let mut transaction_executor = TransactionExecutor::new(
            block_state,
            block_context.clone(),
            TransactionExecutorConfig::default(),
        );

        // Execute all transactions.
        let execution_results = transaction_executor.execute_txs(&txs, None);

        // Collect results, returning error if any transaction fails.
        let execution_outputs: Vec<TransactionExecutionOutput> = execution_results
            .into_iter()
            .map(|result| {
                result.map_err(|e| {
                    VirtualBlockExecutorError::TransactionExecutionError(e.to_string())
                })
            })
            .collect::<Result<Vec<_>, _>>()?;

        // Get initial state reads.
        let initial_reads = transaction_executor
            .block_state
            .as_ref()
            .ok_or(VirtualBlockExecutorError::StateUnavailable)?
            .get_initial_reads()
            .map_err(|e| VirtualBlockExecutorError::ReexecutionError(Box::new(e.into())))?;

        Ok(VirtualBlockExecutionData { execution_outputs, block_context, initial_reads })
    }

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yoni-Starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions.


crates/starknet_os_runner/src/virtual_block_executor.rs line 37 at r6 (raw file):

/// A virtual block executor runs transactions without block preprocessing
/// (`pre_process_block`), which is useful for simulating execution or generating
/// OS input for proving.

Suggestion:

/// A virtual block executor runs transactions on top of a given, finalized block. This means that some parts, like block preprocessing
/// (`pre_process_block`), are skipped. Useful for simulating execution or generating
/// OS input for proving.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 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