Skip to content

chore(blockifier): fn execute_txs_sequentially return state_diff + change privacy pub fn to fn #3580

Merged
avivg-starkware merged 1 commit intomain-v0.13.4from
avivg/blockifier/execute_return_stetediff
Feb 2, 2025
Merged

chore(blockifier): fn execute_txs_sequentially return state_diff + change privacy pub fn to fn #3580
avivg-starkware merged 1 commit intomain-v0.13.4from
avivg/blockifier/execute_return_stetediff

Conversation

@avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

avivg-starkware commented Jan 21, 2025

@avivg-starkware avivg-starkware marked this pull request as ready for review January 21, 2025 17:06
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch from ee8cf86 to ab31c95 Compare January 21, 2025 17:06
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch from ab31c95 to 0da3430 Compare January 21, 2025 17:19
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 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)


crates/blockifier/src/blockifier/transaction_executor.rs line 119 at r1 (raw file):

                let tx_state_changes_keys =
                    transactional_state.get_actual_state_changes()?.state_maps.into_keys();
                let state_diff = transactional_state.to_state_diff()?.state_maps.into();

Which is not the final object, right?

Code quote:

                let state_diff = transactional_state.to_state_diff()?.state_maps.into();

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.

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


crates/blockifier/src/blockifier/transaction_executor.rs line 139 at r1 (raw file):

        &mut self,
        txs: &[Transaction],
    ) -> Vec<TransactionExecutorResult<TransactionExecutionInfo>> {

Let's return the state maps also by this method (and the ones that call it)

Code quote:

Vec<TransactionExecutorResult<TransactionExecutionInfo>> {

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch 2 times, most recently from c81d582 to 2888f08 Compare January 28, 2025 16:15
Copy link
Contributor Author

@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.

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


crates/blockifier/src/blockifier/transaction_executor.rs line 139 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Let's return the state maps also by this method (and the ones that call it)

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


crates/blockifier/src/blockifier/transaction_executor.rs line 106 at r2 (raw file):

        &mut self,
        tx: &Transaction,
    ) -> TransactionExecutorResult<TransactionExecutionOutput> {

What is TransactionExecutionOutput?

Code quote:

TransactionExecutorResult<TransactionExecutionOutput> {

crates/blockifier/src/blockifier/transaction_executor.rs line 237 at r2 (raw file):

        &mut self,
        txs: &[Transaction],
    ) -> Vec<TransactionExecutorResult<TransactionExecutionInfo>> {

Let's return the state diffs from this method as well

Code quote:

    pub fn execute_txs_sequentially(
        &mut self,
        txs: &[Transaction],
    ) -> Vec<TransactionExecutorResult<TransactionExecutionInfo>> {

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 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch 4 times, most recently from cf3715f to ff7bbf6 Compare January 30, 2025 15:17
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch 2 times, most recently from 0a92285 to 36d662e Compare January 30, 2025 15:48
@avivg-starkware avivg-starkware changed the title chore(blockifier): can statediff be returned from execute? chore(blockifier): fn execute_txs_sequentially return state_diff Jan 30, 2025
Copy link
Contributor Author

@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.

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/blockifier/transaction_executor.rs line 106 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What is TransactionExecutionOutput?

sorry it was missing. should appear now

Code snippet:

pub type TransactionExecutionOutput = (TransactionExecutionInfo, StateMaps);

crates/blockifier/src/blockifier/transaction_executor.rs line 237 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Let's return the state diffs from this method as well

Done.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch from 36d662e to e0d6c5b Compare January 30, 2025 15:53
@avivg-starkware avivg-starkware changed the title chore(blockifier): fn execute_txs_sequentially return state_diff chore(blockifier): chore(blockifier): fn execute_txs_sequentially return state_diff + change privacy pub fn to fn Jan 30, 2025
@avivg-starkware avivg-starkware changed the title chore(blockifier): chore(blockifier): fn execute_txs_sequentially return state_diff + change privacy pub fn to fn chore(blockifier): fn execute_txs_sequentially return state_diff + change privacy pub fn to fn Jan 30, 2025
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch from e0d6c5b to 42d8447 Compare January 30, 2025 18:50
@avivg-starkware
Copy link
Contributor Author

crates/blockifier/src/blockifier/transaction_executor.rs line 207 at r4 (raw file):

                .into_iter()
                .map(|res| res.map(|(tx_execution_info, _state_diff)| tx_execution_info))
                .collect()

in the next PR fn execute_txs ret val changes to Vec<TransactionExecutorResult<TransactionExecutionOutput >> and this code is removed

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.

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


crates/blockifier/src/blockifier/transaction_executor.rs line 121 at r4 (raw file):

                let tx_state_changes_keys =
                    transactional_state.get_actual_state_changes()?.state_maps.into_keys();
                let state_diff = transactional_state.to_state_diff()?.state_maps;

You're doing the same work twice: swap order and replace into_keys with keys

Code quote:

                let tx_state_changes_keys =
                    transactional_state.get_actual_state_changes()?.state_maps.into_keys();
                let state_diff = transactional_state.to_state_diff()?.state_maps;

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/execute_return_stetediff branch from 69a927b to b445c22 Compare February 2, 2025 12:52
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.

:lgtm:

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

Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)


crates/blockifier/src/blockifier/transaction_executor.rs line 121 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You're doing the same work twice: swap order and replace into_keys with keys

I created an appropriate fn keys() WDYT? I'm indecisive about whether this implementation is better than just using into_keys()

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 1 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yoni-Starkware)

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.

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

Copy link
Contributor Author

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

@avivg-starkware avivg-starkware merged commit c267de1 into main-v0.13.4 Feb 2, 2025
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 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