starkware-libs / blockifier

Blockifier is a Rust implementation for the transaction-executing component in the StarkNet sequencer, in charge of creating state diffs and blocks.
Apache License 2.0
170 stars 107 forks source link

chore: move tx-too-big check to execute_raw #1959

Closed Yoni-Starkware closed 3 months ago

Yoni-Starkware commented 3 months ago

This change is Reviewable

codecov-commenter commented 3 months ago

Codecov Report

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

Project coverage is 78.20%. Comparing base (c35c254) to head (917944a).

Files Patch % Lines
...lockifier/src/transaction/transaction_execution.rs 75.00% 0 Missing and 4 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1959 +/- ## ======================================= Coverage 78.19% 78.20% ======================================= Files 62 62 Lines 8834 8845 +11 Branches 8834 8845 +11 ======================================= + Hits 6908 6917 +9 + Misses 1486 1483 -3 - Partials 440 445 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

avi-starkware commented 3 months ago

crates/blockifier/src/bouncer_test.rs line 187 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…
No worries, tx-too-large is covered in this test: https://github.com/starkware-libs/blockifier/blob/7d9ebe996796cc6c4f38a104599630da0177b13c/crates/blockifier/src/blockifier/transaction_executor_test.rs#L240

The test only checks the first case you removed here. Don't we want the case "too_large_with_keccak" and the case "too_larg_with_keccak_ecdsa"?

avi-starkware commented 3 months ago

crates/blockifier/src/concurrency/worker_logic.rs line 223 at r3 (raw file):

                        panic!("Bouncer update failed. {error:?}: {error}");
                    }
                }

Nonblocking

Suggestion:

                if error == TransactionExecutorError::BlockFull {
                    return false
                } else {
                    // TODO(Avi, 01/07/2024): Consider propagating the error.
                    panic!("Bouncer update failed. {error:?}: {error}");
                }