Skip check when the 100% check is achieved. #4625
Merged
Conversation
This file contains 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
Frozen
approved these changes
Feb 1, 2024
GheisMohammadi
approved these changes
Feb 2, 2024
sophoah
approved these changes
Feb 2, 2024
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.
We are currently experiencing an issue in our devnet, leading to frequent view changes and extending block finality time from 1 second to 10 seconds. This problem became apparent after transitioning the network to operate exclusively on external nodes. These nodes are located in the same datacenter and region, eliminating geographical constraints. As a result, we achieve nearly 100% voting rate for almost every block.
This issue has not occurred in our testnet or mainnet, where nodes are globally distributed, making it rare to reach a 100% voting rate.
Through research conducted by Soph, we identified a specific logic block in the code that bypasses the precommit and propose phases. After reviewing the code, i believe this block is unnecessary. Our primary concern is achieving a 2/3 vote rate. The conditions
consensus.decider.IsAllSigsCollected()andconsensus.decider.IsQuorumAchieved(quorum.Commit)in the codebase use the same structure to determine if the Commit phase has received sufficient votes. You can see this in the code at line 296, IsQuorumAchieved, and IsAllSigsCollected. Meaning that even if we reach a 100% we will run the same logic as for when we have 2/3 or more the lines of code being run at the commented code are also run at the main block rendering this block of code redundant.EDIT: I've also added the
consensus.decider.IsAllSigsCollected()in case we go from 0 to a 100% signatures in one go, so we run the same logic.