Repository navigation
fix(voting): report unanimous timeout and violation votes accurately - #1777
HusseinAdeiza wants to merge 1 commit into
Conversation
|
This PR targeted I retargeted it to |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The unanimity short circuit in getMajorityVote only branched on Agree and sent every other unanimous vote to MajorityDisagree. A round where all validators timed out, or all reported a deterministic violation, was therefore recorded as MajorityDisagree, which drives the wrong fee and validator rotation handling downstream. The Python mirror of the same decision procedure, determine_consensus_from_votes in backend/consensus/utils.py, already gets this right: it checks timeout_count against the majority threshold and returns ConsensusResult.TIMEOUT. The Solidity has an extra unanimity short circuit the Python does not, and that short circuit is what drops the Timeout and DeterministicViolation outcomes. This is therefore a bug fix rather than a consensus rule change, and the two implementations now agree. Unanimous Agree still returns MajorityAgree and unanimous Disagree still returns MajorityDisagree, unchanged. Those two match the existing behavior and the Python mirror, which maps agree_count and effective_disagree above the majority to MAJORITY_AGREE and MAJORITY_DISAGREE. A unanimous NotVoted vote also keeps its existing MajorityDisagree result, so no other VoteType variant changes result as a side effect. Refs genlayerlabs#1769
67244d7 to
9b169f3
Compare
|
Rebased onto current |
1 participant
Fixes #1769
What
The unanimity short circuit in
getMajorityVoteonly branched onAgree, so every other unanimous vote fell through toMajorityDisagree. A round where all validators timed out, or all reported a deterministic violation, was recorded as a disagreement, which then drives the wrong fee and validator rotation handling. Timeout and DeterministicViolation are now resolved explicitly inside the unanimity branch. UnanimousAgreestill returnsMajorityAgreeand unanimousDisagreestill returnsMajorityDisagree, and a unanimousNotVotedvote keeps its existingMajorityDisagreeresult, so no other vote type changes as a side effect.Why
This is a bug rather than a consensus rule change because the Python mirror of the same decision procedure,
determine_consensus_from_votesinbackend/consensus/utils.py, already does the right thing. It checkstimeout_countagainst the majority threshold and returnsConsensusResult.TIMEOUT. The Solidity has an extra unanimity short circuit that the Python does not have, and that short circuit is what drops the Timeout and DeterministicViolation outcomes. The two implementations now agree, and the existingtests/unit/test_consensus_voting.pyexpectations for the Python side are untouched.Testing done
New
hardhat/test/voting/001_voting_majority_vote.test.jscovers unanimous timeout, unanimous violation, unanimous agree, unanimous disagree, unanimous not-voted, non-unanimous majority timeout and violation, and the no-majority case. 8 passing. The Python mirror suite still reports 13 passing.Decisions made
Kept the change local to the unanimity branch rather than restructuring the whole decision procedure, so the non-unanimous path and the threshold logic stay byte-identical.
Checks
Reviewing tips
The two assertions worth a close look are the ones that lock in behavior that must not change: unanimous agree and unanimous disagree.
User facing release notes
Rounds where every validator times out, or where every validator reports a deterministic violation, are no longer counted as unanimous disagreement.
Summary by CodeRabbit