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

Move sequencing to concurrent coroutine to unblock driver control loop #21

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

ujvl
Copy link

@ujvl ujvl commented Nov 27, 2023

  • Move sequencing functionality into its own coroutine (see: SequencingDriver)
  • Synchronize via async-compatible tokio::RwLock on engine_driver
  • Expose more engine_driver functionality and abstract out functions by whether or not they require &mut self. this is necessary to reduce the critical section for write-locking (e.g. shouldn't write-lock while sleeping for 2s in build_payload)

todo in future PR: avoid extraneous fork-choice updates (e.g. we can do a single update per control loop iteration)

Copy link

@LEAFERx LEAFERx left a comment

Choose a reason for hiding this comment

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

overal lgtm. most comments are about document/minor change

let engine_driver = driver.engine_driver.clone();
let state = driver.state.clone();
self.start_sequencing_driver(engine_driver, state)
};
Copy link

Choose a reason for hiding this comment

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

i suggest to use tokio::spawn for sequencing

Copy link
Author

Choose a reason for hiding this comment

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

tried doing this but need to wrestle w/ borrow checker, will defer

/// List of unfinalized L2 blocks with their epochs, L1 inclusions, and sequence numbers
unfinalized_blocks: Vec<(BlockInfo, Epoch, u64, u64)>,
/// Current finalized L1 block number
finalized_l1_block_number: u64,
/// List of unsafe blocks that have not been applied yet
future_unsafe_blocks: Vec<ExecutionPayload>,
/// State struct to keep track of global state
state: Arc<RwLock<State>>,
pub state: Arc<RwLock<State>>,
Copy link

Choose a reason for hiding this comment

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

can we also use tokio rwlock here? ref a16z#164

Copy link
Author

Choose a reason for hiding this comment

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

it's okay as long as you don't hold the lock across await points (clippy checks this). async RwLocks have additional overhead unfortunately. I'll look into it separately


if let Some(payload) = next_unsafe_payload {
_ = self.engine_driver.handle_unsafe_payload(payload).await;
Copy link

Choose a reason for hiding this comment

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

can we move this to engine_driver.rs? like a top-level function called handle_unsafe_payload

Copy link
Author

Choose a reason for hiding this comment

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

it's a bit awkward to because of where the first read lock is acquired/released, i can try to refactor later by having a function that takes an optional guard

@ujvl ujvl changed the title Move sequencing to coroutine to unblock driver control loop Move sequencing to concurrent coroutine to unblock driver control loop Nov 29, 2023
@ujvl ujvl merged commit c5beaed into develop Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants