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

chore(blockifier): add new constructor to AccountTransaction #2431

Merged

Conversation

avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@avivg-starkware avivg-starkware marked this pull request as ready for review December 3, 2024 13:29
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 9b59221 to 73a8553 Compare December 3, 2024 13:42
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 58.92%. Comparing base (e3165c4) to head (4bb5f5b).
Report is 728 commits behind head on main.

Files with missing lines Patch % Lines
crates/native_blockifier/src/py_transaction.rs 0.00% 3 Missing ⚠️
...lockifier/src/transaction/transaction_execution.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2431       +/-   ##
===========================================
+ Coverage   40.10%   58.92%   +18.82%     
===========================================
  Files          26      174      +148     
  Lines        1895    19663    +17768     
  Branches     1895    19663    +17768     
===========================================
+ Hits          760    11587    +10827     
- Misses       1100     7336     +6236     
- Partials       35      740      +705     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from 0ebdd7d to 313f055 Compare December 3, 2024 14:07
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 73a8553 to b4d5140 Compare December 3, 2024 14:08
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from 313f055 to 32c94bb Compare December 3, 2024 14:09
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from b4d5140 to 0c0f8a5 Compare December 3, 2024 14:09
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from 32c94bb to ccec3e2 Compare December 3, 2024 14:10
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 0c0f8a5 to 9f3f05f Compare December 3, 2024 14:10
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from ccec3e2 to 43209f7 Compare December 3, 2024 14:46
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 9f3f05f to efd7365 Compare December 3, 2024 14:47
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from 43209f7 to ccb5c8d Compare December 3, 2024 15:25
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from efd7365 to b79941b Compare December 3, 2024 15:25
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from ccb5c8d to db01d8d Compare December 3, 2024 15:34
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from b79941b to 62af6a7 Compare December 3, 2024 15:35
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from db01d8d to c75942e Compare December 4, 2024 14:00
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 62af6a7 to 126a0c7 Compare December 4, 2024 14:00
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 3 of 6 files at r1, 6 of 8 files at r2, all commit messages.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware)


crates/blockifier/src/transaction/account_transaction.rs line 132 at r2 (raw file):

    );

    pub fn new(tx: Transaction) -> Self {

Suggestion:

pub fn new_with_default_flags(t

crates/blockifier/src/transaction/account_transaction.rs line 143 at r2 (raw file):

        };
        AccountTransaction { tx, execution_flags }
    }

It seems like there is only one place that needs this flag. So better to remove it.

Also, don't use default here - for readability.

Suggestion:

    // Creates an account transaction with flags that are suitable for sequencing.
    pub fn new_for_sequencing(tx: Transaction) -> Self {
        let execution_flags = ExecutionFlags {
            only_query: false,
            charge_fee: enforce_fee(&tx, false),
            validate: true,
        };
        AccountTransaction { tx, execution_flags }
    }

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from c75942e to b6d3db3 Compare December 5, 2024 09:57
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 126a0c7 to 6de5468 Compare December 5, 2024 09:57
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from b6d3db3 to 8fdc0c9 Compare December 5, 2024 10:00
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 6de5468 to e76d5a4 Compare December 5, 2024 10:00
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: 0 of 11 files reviewed, 2 unresolved discussions (waiting on @Yoni-Starkware)


crates/blockifier/src/transaction/account_transaction.rs line 143 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

It seems like there is only one place that needs this flag. So better to remove it.

Also, don't use default here - for readability.

Done


crates/blockifier/src/transaction/account_transaction.rs line 132 at r2 (raw file):

    );

    pub fn new(tx: Transaction) -> Self {

Done

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

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from 8fdc0c9 to c594964 Compare December 5, 2024 14:30
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from e76d5a4 to 3204c4d Compare December 5, 2024 14:30
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from c594964 to be786da Compare December 5, 2024 15:46
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 3204c4d to df889dd Compare December 5, 2024 15:46
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from be786da to 5c9934e Compare December 5, 2024 19:51
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from df889dd to 1cdcccb Compare December 5, 2024 19:51
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/new_struct_execution_flags_in_acc_tx branch from 5c9934e to 0557829 Compare December 5, 2024 20:22
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 1cdcccb to 186a1e0 Compare December 5, 2024 20:22
@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/new_struct_execution_flags_in_acc_tx to graphite-base/2431 December 5, 2024 21:21
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from 186a1e0 to ea92627 Compare December 5, 2024 21:21
@avivg-starkware avivg-starkware changed the base branch from graphite-base/2431 to main December 5, 2024 21:21
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/add_account_tx_default_constructor branch from ea92627 to 4bb5f5b Compare December 5, 2024 21:22
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 7 of 11 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware merged commit 6810267 into main Dec 5, 2024
13 checks passed
FrancoGiachetta added a commit to lambdaclass/sequencer that referenced this pull request Dec 6, 2024
* chore(starknet_batcher): delete obsolete todo (starkware-libs#2389)

* chore(blockifier): add global max_sierra_gas to versioned constants (starkware-libs#2330)

* feat(starknet_api): checked mul for gas vector (starkware-libs#2300)

* feat(consensus): proposer rotates across validators (starkware-libs#2405)

* feat(sequencing): validate streamed proposals (starkware-libs#2305)

* feat(ci): deny rust warnings in all workflows (starkware-libs#2388)

Signed-off-by: Dori Medini <dori@starkware.co>

* feat(blockifier): compute allocation cost (starkware-libs#2301)

* chore(starknet_sequencer_infra): add dynamic logging util fn

commit-id:9ffe9fbe

* chore(starknet_sequencer_infra): add tracing test

commit-id:76d16e9a

* chore(starknet_sequencer_infra): add run_until utility fn

commit-id:194a4b6c

* chore(infra_utils): change run_until to support async functions

commit-id:92e1f8a3

* chore(starknet_integration_tests): use run_until to await for block creation

commit-id:667e001c

* chore(infra_utils): rename logger struct

commit-id:6520ae54

* chore(blockifier): explicit creation of AccountTransaction (starkware-libs#2331)

* test(starknet_integration_tests): test commit blocks by running multiple heights

* chore(starknet_batcher): set temp gas prices in propose block input (starkware-libs#2341)

* chore(starknet_batcher): set use_kzg_da flag in build block input (starkware-libs#2345)

* feat(ci): inherit the rust toolchain toml in moonrepo action (starkware-libs#2423)

Signed-off-by: Dori Medini <dori@starkware.co>

* chore(blockifier): enforce_fee() impl by api::executable_transaction::AccountTransaction (starkware-libs#2377)

* chore(starknet_integration_tests): inherit sequencer node's stdout (starkware-libs#2427)

* chore(blockifier): invoke() declare() deploy_account() change ret val to api_tx (starkware-libs#2412)

* chore(starknet_consensus_manager): set proposer address in propose block input (starkware-libs#2346)

* feat(consensus): add observer mode

* feat(consensus): sequencer context broadcasts votes (starkware-libs#2422)

* chore(deployment): support unified deployment config (starkware-libs#2378)

* feat(starknet_api): add sierra version to class info (starkware-libs#2313)

* refactor(starknet_api): change default sierra contract class to valid one (starkware-libs#2439)

* feat(starknet_l1_provider): add uninitiailized state and make it the default (starkware-libs#2434)

This is to comply with upcoming integration with infra, which separates
instantiation with initialization. In particular, `Pending` state should
be already post-syncing with L1, whereas `Uninitialized` is unsynced and
unusable.

Co-Authored-By: Gilad Chase <gilad@starkware.com>

* feat(papyrus_storage)!: bump storage version for version 13.4 (starkware-libs#2333)

* feat(native_blockifier): allow deserialization of  python l1_data_gas (starkware-libs#2447)

* refactor(blockifier): split FC to groups base on their tags (starkware-libs#2236)

* test(consensus): remove warning on into mock propsal part (starkware-libs#2448)

* chore(blockifier): use test_utils::invoke_tx() instead of trans::test_utils::account_invoke_tx() (starkware-libs#2428)

* chore(blockifier): save sierra to Feature contracts (starkware-libs#2370)

* feat(blockifier): don't count Sierra gas in CairoSteps mode (starkware-libs#2440)

* chore(blockifier): convert Sierra gas to L1 gas if in L1 gas mode (starkware-libs#2451)

* feat(blockifier): add comprehensive state diff versioned constant (starkware-libs#2407)

* chore(starknet_consensus_manager): add chain_id to config

* refactor(papyrus_p2p_sync): add random_header utility function (starkware-libs#2381)

* chore(starknet_batcher): pass block info from consensus to batcher (starkware-libs#2238)

* test(starknet_mempool): tx added to mempool are forwarded to propagator client (starkware-libs#2288)

* fix: fix CR comments

* test(starknet_mempool): tx added to mempool are forwarded to propagator client

* feat(sequencing): cache proposals from bigger heights(starkware-libs#2325)

* fix: change to latest ubuntu version in feature combo CI (starkware-libs#2414)

* chore(blockifier): replace entry_point_gas_cost with initial_budget (starkware-libs#2247)

* test(starknet_gateway): handle todo in test_get_block_info (starkware-libs#2267)

* chore(starknet_api): revert use get_packaget_dir instead of env var

This reverts commit c45f5cc.

commit-id:a48736e7

* chore(starknet_api): rely on env::current_dir() instead of CARGO_MANIFEST_DIR

commit-id:301ed4eb

* chore(blockifier): move env var from run time to compile time

commit-id:80a7265d

* chore(starknet_sierra_compile): move env var from run time to compile time

commit-id:6e7f2a75

* chore: remove the use of zero as a validator id (starkware-libs#2411)

* refactor(papyrus_p2p_sync): add_test receives query size instead of constant (starkware-libs#2379)

* fix(blockifier): merge state diff with squash (starkware-libs#2310)

* feat(blockifier): get revert receipt only in case of revert (starkware-libs#2471)

* chore(starknet_integration_tests): create chain info once (starkware-libs#2482)

* chore(starknet_sierra_compile): split build utils

commit-id:0f504fd7

* chore(starknet_sierra_compile): set runtime-accessible out_dir env var

commit-id:539f16db

* chore(starknet_sierra_compile): avoid using OUT_DIR in run time

commit-id:cd6fba29

* refactor(starknet_api): use const in sierra version (starkware-libs#2477)

* chore: cleanups of OUT_DIR env var (starkware-libs#2484)

commit-id:18d61b1d

* chore(starknet_api): shorten executable_transaction  usage path

* fix(sequencing): remove old proposal pipes from consensus (starkware-libs#2452)

* test(starknet_integration_tests): match sequencer address with default validator id (starkware-libs#2486)

* fix(ci): move specific versioned deps to root toml (starkware-libs#2487)

* fix(ci): move specific versioned deps to root toml

Signed-off-by: Dori Medini <dori@starkware.co>

* fix(starknet_sierra_compile): fix build.rs

Signed-off-by: Dori Medini <dori@starkware.co>

* chore(starknet_batcher): in block builder use the consensus suplied sequncer address (starkware-libs#2409)

* chore: cleanups of CARGO_MANIFEST_DIR env var

commit-id:c96f2d88

* fix(starknet_sierra_compile): missing import in feature (starkware-libs#2495)

commit-id:abd0a286

* chore(blockifier): add keccak_builtin_gas_cost (starkware-libs#2327)

* chore(blockifier): add struct ExecutionFlags to AccTx & use instead of transaction::ExecutionFlags (starkware-libs#2429)

* chore(blockifier): add new constructor to AccountTransaction (starkware-libs#2431)

* chore(blockifier): remove only_qury from IvokeTxArgs (starkware-libs#2437)

* chore(blockifier): remove struct ExecutionFlags and replace w concurrency_mode bool (starkware-libs#2470)

* chore(blockifier): remove declare.rs deploy_account.rs invoke.rs from blockifier (starkware-libs#2478)

---------

Signed-off-by: Dori Medini <dori@starkware.co>
Co-authored-by: YaelD <70628564+Yael-Starkware@users.noreply.github.com>
Co-authored-by: aner-starkware <147302140+aner-starkware@users.noreply.github.com>
Co-authored-by: yoavGrs <97383386+yoavGrs@users.noreply.github.com>
Co-authored-by: matan-starkware <97523054+matan-starkware@users.noreply.github.com>
Co-authored-by: guy-starkware <guy.n@starkware.co>
Co-authored-by: dorimedini-starkware <dori@starkware.co>
Co-authored-by: Itay Tsabary <itayt@starkware.co>
Co-authored-by: avivg-starkware <aviv.g@starkware.co>
Co-authored-by: Yair Bakalchuk <yair@starkware.co>
Co-authored-by: Arnon Hod <arnon@starkware.co>
Co-authored-by: Alon Haramati <91828241+alonh5@users.noreply.github.com>
Co-authored-by: Asmaa Magdoub <asmaa@starkware.co>
Co-authored-by: alon-dotan-starkware <alon.dotan@starkware.co>
Co-authored-by: AvivYossef-starkware <141143145+AvivYossef-starkware@users.noreply.github.com>
Co-authored-by: giladchase <gilad@starkware.co>
Co-authored-by: Gilad Chase <gilad@starkware.com>
Co-authored-by: DvirYo-starkware <115620476+DvirYo-starkware@users.noreply.github.com>
Co-authored-by: nimrod-starkware <143319383+nimrod-starkware@users.noreply.github.com>
Co-authored-by: Meshi Peled <141231558+meship-starkware@users.noreply.github.com>
Co-authored-by: Yoni <78365039+Yoni-Starkware@users.noreply.github.com>
Co-authored-by: Yael Doweck <yael@starkware.co>
Co-authored-by: ShahakShama <70578257+ShahakShama@users.noreply.github.com>
Co-authored-by: Alon-Lukatch-Starkware <alon.l@starkware.co>
Co-authored-by: Yonatan-Starkware <yonatan.k@starkware.co>
Co-authored-by: Yair <92672946+yair-starkware@users.noreply.github.com>
Co-authored-by: Itay-Tsabary-Starkware <106665835+Itay-Tsabary-Starkware@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2024
@avivg-starkware avivg-starkware deleted the avivg/blockifier/add_account_tx_default_constructor branch December 8, 2024 09:31
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.

3 participants