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

Update Quorum Check to Use GTE for Consistency #4806

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

GheisMohammadi
Copy link
Contributor

Issue

This PR updates the IsQuorumAchievedByMask function in the stakedVoteWeight struct to use GTE (greater than or equal to) instead of GT (greater than) for comparing the voting power with the quorum threshold. It ensures the consistency across all quorum checks in the codebase

@GheisMohammadi GheisMohammadi self-assigned this Nov 28, 2024
@GheisMohammadi GheisMohammadi force-pushed the improve/quorom_check_gte branch from cb1ad64 to db714bb Compare November 28, 2024 11:46
@GheisMohammadi GheisMohammadi marked this pull request as draft November 29, 2024 02:22
@sophoah
Copy link
Contributor

sophoah commented Jan 24, 2025

can you explain

It ensures the consistency across all quorum checks in the codebase

is there any other places in the code we use GTE and not GT ?

For me it's just a small detail , but in our consensus the threshold is actually 2/3 +1 not 2/3, might have been on purpose that we initially used GT and not GTE.

what do you think ?

@GheisMohammadi
Copy link
Contributor Author

can you explain

It ensures the consistency across all quorum checks in the codebase

is there any other places in the code we use GTE and not GT ?

For me it's just a small detail , but in our consensus the threshold is actually 2/3 +1 not 2/3, might have been on purpose that we initially used GT and not GTE.

what do you think ?

there’s a mismatch in how the two IsQuorumAchievedByMask functions handle quorum checks. In uniformVoteWeight, quorum is considered achieved if the currentTotalPower is greater than or equal to the threshold (>=). But in stakedVoteWeight, quorum is only achieved if the currentTotalPower is strictly greater than the threshold (>). This means one allows quorum when the voting power exactly equals the threshold, while the other doesn’t.

We need to decide how quorum should work. should it include the threshold (>=) or only exceed it (>)? Usually, quorum is about meeting or surpassing a threshold (>=), so it makes sense to align both functions to that. If we agree, we just need to tweak stakedVoteWeight to match the logic in uniformVoteWeight for consistency.

@sophoah
Copy link
Contributor

sophoah commented Jan 24, 2025

ok if you ask me i would go with GT, not GTE.

@GheisMohammadi
Copy link
Contributor Author

we just need to tweak stakedVoteWeight to match the logic in uniformVoteWeight for consistency.

Then we need to tweak uniformVoteWeight to match the logic in stakedVoteWeight.

@GheisMohammadi GheisMohammadi force-pushed the improve/quorom_check_gte branch 2 times, most recently from de4b8bc to 366fce0 Compare January 28, 2025 02:36
@GheisMohammadi GheisMohammadi marked this pull request as ready for review January 28, 2025 02:44
@sophoah
Copy link
Contributor

sophoah commented Jan 28, 2025

Consensus/quorum/one-node-one-vote.go is no longer used in mainnet/testnet/devnet. The only env that uses it in localnet and happens before the staking epoch

@GheisMohammadi GheisMohammadi marked this pull request as draft January 28, 2025 04:02
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.

2 participants