feat(ethexe-cli): Add multi-validator dev mode via --dev-validators flag#5201
feat(ethexe-cli): Add multi-validator dev mode via --dev-validators flag#5201
--dev-validators flag#5201Conversation
playX18
added
A0-pleasereview
|
Review changes with Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-validator development mode, configured via the --dev-validators flag. The changes span across the CLI, service, and Ethereum deployment logic to support spinning up multiple validator instances. The implementation is mostly solid, but I've found one area in the service configuration where error handling can be improved to prevent potential panics.
|
Converting to draft. Running multiple validators in one process does not seem feasibly. Instead will focus on deployment scripts and compose files that achieve the same functionality. Will close once solution is available. |
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-validator development mode, which is a significant feature enhancement. The implementation is mostly solid, with changes spanning across the CLI, service, and Ethereum deployment logic to support multiple validators. However, I've identified a critical issue in the concurrency handling within run_multi_validator where a blocking call is made in an async context, which could lead to runtime stalls. My review provides a specific code suggestion to address this by using tokio::task::spawn_blocking and futures::future::join_all for correctly handling thread joining in an async environment.
| // Wait for any thread to finish (join returns Result<Result<_, _>, _>) | ||
| for handle in handles { | ||
| if let Err(e) = handle.join() { | ||
| log::error!("Validator thread panicked: {:?}", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using std::thread::JoinHandle::join() within an async block is an anti-pattern as it blocks the asynchronous runtime's worker thread. This can lead to performance issues or even deadlocks. It's better to move blocking operations to a dedicated thread pool using tokio::task::spawn_blocking.
Additionally, the comment 'Wait for any thread to finish' is misleading, as the loop waits for all threads to complete sequentially. The implementation should be updated to wait for all threads concurrently without blocking the executor.
The suggested change uses futures::future::join_all, which may require adding use futures::future; to your imports.
// Wait for all threads to finish.
let join_tasks = handles.into_iter().map(|handle| {
tokio::task::spawn_blocking(move || {
if let Err(e) = handle.join() {
log::error!("Validator thread panicked: {:?}", e);
}
})
});
futures::future::join_all(join_tasks).await;References
- The current implementation has a concurrency risk by using a blocking
joincall inside an async block, which can stall the executor. The style guide (line 24) requires special attention to concurrency risks when code changes touch async or parallel processing behavior. (link)
2 participants
Required for #5169.
@gear-tech/dev