chore: add periodic peer discovery

[renancloudwalk]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves asynchronous operations and peer discovery mechanisms which require careful consideration to ensure they are implemented correctly without causing race conditions or performance issues.
🧪 Relevant tests	No
⚡ Possible issues	Blocking Task in Async Context: The loop inside initialize_periodic_peer_discovery uses interval.tick().await, which is fine, but the discover_peers function might be blocking, which should not run directly in the tokio executor if it's not purely asynchronous.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/consensus/mod.rs
suggestion	Consider using tokio::spawn instead of named_spawn for launching asynchronous tasks unless named_spawn is a wrapper around tokio::spawn that also logs the task name for better debugging. If named_spawn does not handle task panics or other important aspects, it might be safer to switch to tokio::spawn. [important]
relevant line	named_spawn("consensus::peer_discovery", async move {

---

relevant file	src/eth/consensus/mod.rs
suggestion	Ensure that the discover_peers function called within the periodic peer discovery loop is non-blocking or offloaded to a separate thread if it involves IO operations or heavy computation to prevent blocking the async runtime. This can be achieved by using tokio::task::spawn_blocking if necessary. [important]
relevant line	Self::discover_peers(Arc::clone(&consensus)).await;

---

relevant file	src/eth/consensus/mod.rs
suggestion	Add error handling for the discover_peers function within the loop. Currently, if discover_peers fails, the error is not handled, which could lead to unreported issues. Consider logging the error or implementing a retry mechanism. [important]
relevant line	Self::discover_peers(Arc::clone(&consensus)).await;

---

relevant file	src/eth/consensus/mod.rs
suggestion	Implement logging for the outcome of each peer discovery attempt. This could involve logging successful connections or the number of peers discovered, which would be useful for monitoring and debugging the peer discovery process. [medium]
relevant line	tracing::info!("Starting periodic peer discovery...");

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add error handling for periodic peer discovery to improve robustness Consider adding error handling for the discover_peers method within the loop. This can prevent silent failures during periodic peer discovery, ensuring that any issues are logged and can be addressed. src/eth/consensus/mod.rs [163] -Self::discover_peers(Arc::clone(&consensus)).await; +if let Err(e) = Self::discover_peers(Arc::clone(&consensus)).await { + tracing::error!("Error during periodic peer discovery: {:?}", e); +} Suggestion importance[1-10]: 9 Why: Adding error handling for the discover_peers method within the loop is crucial for robustness. It ensures that any issues during peer discovery are logged and can be addressed, preventing silent failures.	9
	Implement a retry limit for periodic peer discovery to prevent potential infinite loops To avoid potential infinite loops or resource exhaustion, consider implementing a mechanism to break out of the loop under certain conditions, or to limit the number of retries for peer discovery. src/eth/consensus/mod.rs [160-163] +let mut attempts = 0; loop { interval.tick().await; tracing::info!("Starting periodic peer discovery..."); - Self::discover_peers(Arc::clone(&consensus)).await; + if let Err(e) = Self::discover_peers(Arc::clone(&consensus)).await { + tracing::error!("Failed periodic peer discovery: {:?}", e); + attempts += 1; + if attempts >= MAX_ATTEMPTS { + break; + } + } else { + attempts = 0; // reset on success + } } Suggestion importance[1-10]: 8 Why: Implementing a retry limit for periodic peer discovery is a good practice to avoid potential infinite loops or resource exhaustion. This suggestion enhances the reliability of the system.	8
Enhancement	Use a configurable time interval for peer discovery to enhance flexibility To improve the scalability and responsiveness of the periodic peer discovery, consider using a configurable time interval instead of a hard-coded value. src/eth/consensus/mod.rs [159] -let mut interval = tokio::time::interval(Duration::from_secs(30)); +let discovery_interval_secs = consensus.config.peer_discovery_interval.unwrap_or(30); +let mut interval = tokio::time::interval(Duration::from_secs(discovery_interval_secs)); Suggestion importance[1-10]: 7 Why: Using a configurable time interval for peer discovery improves the scalability and responsiveness of the system. This enhancement provides flexibility but is not critical for functionality.	7
Maintainability	Specify the return type for initialize_periodic_peer_discovery to enhance code clarity The initialize_periodic_peer_discovery function is missing a return type. Specifying the return type, even if it's (), can improve code clarity and maintainability. src/eth/consensus/mod.rs [157] -fn initialize_periodic_peer_discovery(consensus: Arc<Consensus>) { +fn initialize_periodic_peer_discovery(consensus: Arc<Consensus>) -> () { Suggestion importance[1-10]: 6 Why: Specifying the return type, even if it's (), can improve code clarity and maintainability. However, this is a minor enhancement and does not impact functionality.	6