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

Separate node service tasks into smaller tasks #2274

Closed
kariy opened this issue Aug 8, 2024 · 2 comments
Closed

Separate node service tasks into smaller tasks #2274

kariy opened this issue Aug 8, 2024 · 2 comments
Assignees
Labels
katana This issue is related to Katana

Comments

@kariy
Copy link
Member

kariy commented Aug 8, 2024

currently block production and messaging tasks are combined into one big task (ie NodeService). there are several reasons why i think separating the tasks into smaller ones would be beneficial:

(1) separation of concern

as messaging and block production are two separate tasks whose tasks output aren't dependent on one another. as you can see below, it is not necessary at all to include both in the same Future::poll(). the messaging portion can be taken out of the function and it wouldn't affect the rest of the logic.

separating the tasks would make it much easier to manage as each can be scoped into their own tiny little Future.


    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        let pin = self.get_mut();

        #[cfg(feature = "messaging")]
        if let Some(messaging) = pin.messaging.as_mut() {
            while let Poll::Ready(Some(outcome)) = messaging.poll_next_unpin(cx) {
                match outcome {
                    MessagingOutcome::Gather { msg_count, .. } => {
                        info!(target: LOG_TARGET, msg_count = %msg_count, "Collected messages from settlement chain.");
                    }
                    MessagingOutcome::Send { msg_count, .. } => {
                        info!(target: LOG_TARGET,  msg_count = %msg_count, "Sent messages to the settlement chain.");
                    }
                }
            }
        }

        // this drives block production and feeds new sets of ready transactions to the block
        // producer
        loop {
            while let Poll::Ready(Some(res)) = pin.block_producer.poll_next(cx) {
                match res {
                    Ok(outcome) => {
                        info!(target: LOG_TARGET, block_number = %outcome.block_number, "Mined block.");

                        let metrics = &pin.metrics.block_producer;
                        let gas_used = outcome.stats.l1_gas_used;
                        let steps_used = outcome.stats.cairo_steps_used;
                        metrics.l1_gas_processed_total.increment(gas_used as u64);
                        metrics.cairo_steps_processed_total.increment(steps_used as u64);
                    }

                    Err(err) => {
                        error!(target: LOG_TARGET, error = %err, "Mining block.");
                    }
                }
            }

            if let Poll::Ready(pool_txs) = pin.miner.poll(&pin.pool, cx) {
                // miner returned a set of transaction that we feed to the producer
                pin.block_producer.queue(pool_txs);
            } else {
                // no progress made
                break;
            }
        }

        Poll::Pending

(2) tasks management

we now have TaskManager that will act as the central point where tasks would be spawned on. the manager is mainly added to avoid dangling tasks and be able to cancel tasks from being further executed when we no longer need it to (refer https://blog.yoshuawuyts.com/async-cancellation-1/).

the scope and operation of each task are independent of one another, ie. the they don't rely on the outcome of one another to function correctly. if one of the task failed, ideally it shouldn't cause the other to failed as well. if its a failure that can be recovered from (ie network issues) then ideally we can simply restart. the idea of TaskManageris to allow to do just that. by separating each task into its own unit, we can allow each task to be independent, and depending on the nature of the error possible from each task, we can employ whatever recovery methods that make sense for it.

@kariy kariy added the katana This issue is related to Katana label Aug 8, 2024
@kariy kariy self-assigned this Aug 8, 2024
@kariy kariy changed the title katana: separate node services (ie block production, messaging) into their own tasks Separate node service tasks into smaller tasks Aug 23, 2024
@ScottyDavies
Copy link

Hi @kariy can i please work on this?

@kariy
Copy link
Member Author

kariy commented Sep 23, 2024

completed in #2413

@kariy kariy closed this as completed Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
katana This issue is related to Katana
Projects
None yet
Development

No branches or pull requests

2 participants