Introduce mutex locks in MirBFT node and add concurrency tests#24
Open
saiashok0981 wants to merge 1 commit into
Open
Introduce mutex locks in MirBFT node and add concurrency tests#24saiashok0981 wants to merge 1 commit into
saiashok0981 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue 2: Concurrent Map Writes and Missing Mutex Synchronization in MirBFT Node
Issue Description
Title: Bug: Data race and concurrent map writes in MirBFT Node methods
Description: The MirBFT struct defined in candidates/siddhantprateek/mirbft/mirbft-sample/main.go includes a Mutex sync.Mutex field, but it is never locked or unlocked in any of its methods. Methods such as Propose, ProcessProposal, and Commit read and write to several shared nested map variables:
mb.Pending
mb.Committed
mb.Received
Under concurrent scenarios (e.g., multiple nodes receiving messages or processing proposals concurrently), this results in a data race and triggers a runtime panic: fatal error: concurrent map writes.
Expected Behavior: All read and write accesses to shared maps (Pending, Committed, Received) inside the MirBFT instance must be synchronized using mb.Mutex to ensure thread-safety.
Pull Request Description
Title: Introduce mutex locks in MirBFT node and add concurrency tests
Branch: mirbft-data-race-fix
Description of Changes:
Thread-Safety: Added proper mb.Mutex.Lock() and mb.Mutex.Unlock() calls to Propose, ProcessProposal, and Commit methods around operations mutating or reading the shared maps.
Deadlock Avoidance: Structured the critical sections carefully, releasing locks before calling sibling methods (like calling Commit from ProcessProposal) or running simulated side-effects (SendMessage, ExecuteRequest) to avoid recursive deadlocks and reduce lock contention.
Added Concurrency Tests: Created a new test file candidates/siddhantprateek/mirbft/mirbft-sample/main_test.go containing concurrent tests TestMirBFTConcurrency and TestMirBFTProcessProposalConcurrency to verify thread-safety.