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
171 stars 99 forks source link

fix(concurrency): stop the transaction committer when the scheduler is done #1989

Closed avi-starkware closed 1 month ago

avi-starkware commented 1 month ago

This change is Reviewable

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.61%. Comparing base (d924c79) to head (a1b8d05). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1989 +/- ## ======================================= Coverage 78.61% 78.61% ======================================= Files 62 62 Lines 8895 8895 Branches 8895 8895 ======================================= Hits 6993 6993 Misses 1455 1455 Partials 447 447 ```

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

avi-starkware commented 1 month ago

@barak-b-starkware said his flow test got stuck in an infinite loop, so I tried to think of places where the scheduler might get stuck. This is probably not the bug from Barak's flow test because his flow test does not use the method halt_scheduler, but it is still a bug.

barak-b-starkware commented 1 month ago

crates/blockifier/src/concurrency/scheduler.rs line 31 at r1 (raw file):

            *self.commit_index_guard < self.scheduler.chunk_size,
            "The commit index must be less than the chunk size, since the scheduler is not done."
        );

Why do we need this assertion? It feels trivial/redundant. What do I miss here?

Code quote:

        assert!(
            *self.commit_index_guard < self.scheduler.chunk_size,
            "The commit index must be less than the chunk size, since the scheduler is not done."
        );