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

Improve async processing and error handling #26

Merged

Conversation

therustmonk
Copy link
Collaborator

@mskrzypkows Here is the PR for your #23

I've added some important improvements that help to to keep the code easy to maintain. In Rust, it's easy to open a branch of hell, so it's important to give the compiler everything that can be explicitly undeclared for now.

The PR:

  • avoids using thiserror, it's the main tempter :) prefer using anyhow as long as possible
  • avoids spawning tokio thread (try to use a single processing routine), separate couroutines are extremelly challenging to debug. I took the risk again in SFFL and agreed to prematurely spawn coroutines, and I still regret it 😢 Use a single coroutine with a sequence of operations. Don't worry about performance now.

The best Rust development flow is to follow the steps:
Easy and Compact Implementation -> Intensive Refactoring -> Optimization

I don't mind these things, we will need them, but they are more like decoration. For now, let's bake the cake first 🍰
I refactored the code but haven't tested it. Could you please check it?

@mskrzypkows
Copy link
Collaborator

Why do you prefer anyhow? We've been using thiserror in dkg-proto, and it was fine for descriptive errors.

Regarding the tokio::spawn, I know what you mean, I've been working a lot of with threads in C++ and have some experience with debugging concurrent software.
We can start with a self.node_rx.try_recv() but need to be aware of starvation problem, the self.main_block_preconfirmation_step().await?; may be run less frequent when there are a lot of incoming messages.
Have you seen this: https://www.notion.so/nethermind/AVS-Node-Software-Diagrams-df0105a1d3f54276b8b9cbd2f2a5a101 ?

self.send_preconfirmations_to_the_avs_p2p();
self.taiko.submit_new_l2_blocks();
if let Err(err) = self.step().await {
tracing::debug!("Node processing step failed: {}", err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not tracing::error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, correct, it has to be error! 👍

@mskrzypkows
Copy link
Collaborator

I've checked locally, works 👍

Copy link
Collaborator

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

@therustmonk Thank you sir

@smartprogrammer93 smartprogrammer93 merged commit 1d69135 into fetch_pending_txs_from_taiko Jun 21, 2024
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.

3 participants