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

fix: followers gathering #956

Merged
merged 2 commits into from
May 30, 2024
Merged

fix: followers gathering #956

merged 2 commits into from
May 30, 2024

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 30, 2024 13:18
@renancloudwalk renancloudwalk enabled auto-merge (squash) May 30, 2024 13:19
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves refactoring and introducing concurrency with Arc and Mutex, which requires careful review to ensure thread safety and correct handling of asynchronous code. Additionally, the changes impact critical consensus and networking operations in a blockchain-like system, necessitating a thorough understanding of the existing system and the implications of the changes.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of unwrap_or_default() in initialize_append_entries_channel when discovering followers might lead to silent failures where no followers are discovered but the system continues to operate as if it's normal. This could lead to blocks not being sent to followers without any error or warning being raised.

Concurrency Concern: The use of Mutex around the receiver in a potentially high-throughput and low-latency context (blockchain consensus) might lead to performance bottlenecks. Lock contention and the overhead of locking and unlocking might significantly impact the performance.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/consensus.rs
suggestion      

Consider handling the potential error from discover_followers more explicitly rather than using unwrap_or_default(). This could involve logging a clear error message and potentially retrying the discovery or halting the operation until the issue is resolved. This change would prevent the system from operating under the assumption that followers are present when, in fact, they might not be. [important]

relevant linelet followers = Self::discover_followers().await.unwrap_or_default();

relevant filesrc/eth/consensus.rs
suggestion      

To reduce the potential for lock contention and improve performance, consider using an asynchronous queue or another non-blocking concurrency mechanism instead of a Mutex for managing access to the receiver. This could enhance the throughput and responsiveness of the consensus module. [important]

relevant linelet receiver = Arc::new(Mutex::new(receiver));

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Maintainability
Add a control mechanism to exit the infinite loop safely

Consider handling the potential infinite loop in initialize_append_entries_channel by
adding a mechanism to break out of the loop under certain conditions, such as a shutdown
signal or a maximum retry limit, to prevent potential resource exhaustion.

src/eth/consensus.rs [121-142]

-loop {
+while running.load(Ordering::SeqCst) {
     let mut lock = receiver.lock().await;
     if let Some(data) = lock.recv().await {
         ...
     }
 }
 
Suggestion importance[1-10]: 10

Why: Introducing a mechanism to break out of the infinite loop under certain conditions is crucial for preventing potential resource exhaustion and ensuring the system can shut down gracefully.

10
Possible issue
Improve error handling when discovering followers

Replace unwrap_or_default() with proper error handling to avoid silent failures and ensure
robustness. This change will help in debugging and maintaining the code by providing clear
error messages when discovering followers fails.

src/eth/consensus.rs [114]

-let followers = Self::discover_followers().await.unwrap_or_default();
+let followers = Self::discover_followers().await.expect("Failed to discover followers");
 
Suggestion importance[1-10]: 9

Why: This suggestion improves error handling by replacing unwrap_or_default() with expect(), which provides a clear error message if discovering followers fails. This enhances robustness and aids in debugging.

9
Performance
Optimize cloning of followers list to enhance performance

Consider using a more efficient cloning method for followers to avoid unnecessary resource
usage. Since followers is cloned for each block received, using Arc for the followers list
can improve performance by reducing the overhead of cloning large data structures.

src/eth/consensus.rs [131]

-match Self::append_block_commit_to_followers(data.clone(), followers.clone()).await {
+match Self::append_block_commit_to_followers(data.clone(), Arc::clone(&followers)).await {
 
Suggestion importance[1-10]: 8

Why: Using Arc::clone instead of followers.clone() can reduce the overhead of cloning large data structures, improving performance. This is a valid optimization for resource usage.

8
Use RwLock instead of Mutex for potentially improved concurrency

Replace the Mutex with an RwLock for the receiver to allow multiple concurrent reads,
which can improve the performance in scenarios where there are multiple reads and fewer
writes.

src/eth/consensus.rs [77]

-let receiver = Arc::new(Mutex::new(receiver));
+let receiver = Arc::new(RwLock::new(receiver));
 
Suggestion importance[1-10]: 7

Why: Replacing Mutex with RwLock allows multiple concurrent reads, which can improve performance in read-heavy scenarios. However, the actual performance gain depends on the specific read/write patterns.

7

@renancloudwalk renancloudwalk merged commit 21ff711 into main May 30, 2024
32 checks passed
@renancloudwalk renancloudwalk deleted the fix-followers-gathering branch May 30, 2024 13:45
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.

1 participant