Voting misclassifies unanimous Timeout/DeterministicViolation; Idleness.checkIdle trusts a caller-built Transaction; txId can collide within a block #1769

Activity

1. HusseinAdeiza commented on Sep 26, 2026

   

   HusseinAdeiza

   on Sep 26, 2026

   Contributor

   More actions

   Taking item 1, the getMajorityVote unanimity case. The other two look like they need a threat model discussion rather than a patch, and item 2 in particular depends on which staking contract is deployed against which Idleness ABI, which is a maintainer decision.

   What convinced me item 1 is a real bug rather than a design choice: the forcing only kicks in when maxVotes == validatorCount, and in that state the vote is unanimous, so there is no ambiguity about what the round decided. Reporting a unanimous Timeout as MajorityDisagree loses the distinction that the Timeout path exists to draw. The asymmetry is what makes it look unintentional, since a non-unanimous Timeout does take the Timeout branch.

   One thing I want to confirm against your reading before I write it: does the unanimous case need to fall through to the same resolution as a simple majority, or should a unanimous vote short-circuit to a terminal state directly? Your description says the result is "forced to MajorityAgree/MajorityDisagree", which suggests the forcing happens before the Timeout/DeterministicViolation checks rather than after them. If it is a simple ordering issue, reordering the branches to test unanimity after the timeout and violation checks would be a small, local change. If the terminal state is meant to differ, then it is a consensus-behavior change and I would want your call on the intended outcome rather than guessing at it.

   I'll write the test first, against the current behavior, so the change is visible in the diff.
2. byfor8 commented on Sep 28, 2026

   

   byfor8

   on Sep 28, 2026

   Author

   More actions

   It's an ordering issue, not an intended terminal state. The intended outcome for a unanimous Timeout is the same ResultType.Timeout the simple-majority branch produces, just from the unanimity path. The forcing should not swallow the vote value.

   Two things downstream depend on the distinction:

   1. FeeManager._voteAlignedWithResult aligns a validator's Timeout vote only with ResultType.Timeout, never with MajorityDisagree. So a unanimous-Timeout round recorded as MajorityDisagree marks every validator unaligned, which flows into the fee split (the same validatorsAligned counter commitFinalFees uses). DeterministicViolation has the same problem.

   2. Status outcomes are unaffected either way (MajorityAgree/Agree and MajorityDisagree/Disagree are treated as synonyms for accepted/undetermined), so this is result-type and fee-accounting only, not a consensus-state change.

   So: reorder so unanimity is tested after the Timeout/DeterministicViolation checks, or make the unanimous branch switch on majorityVote exactly like the > validatorCount / 2 branch does. Second form is the smaller diff. Agree on writing the test against current behavior first.
3. HusseinAdeiza commented on Sep 28, 2026

   

   HusseinAdeiza

   on Sep 28, 2026

   Contributor

   More actions

   Confirmed on both counts, and PR #1777 is already up implementing the second form you preferred.

   I checked the FeeManager path you flagged. _voteAlignedWithResult (hardhat/contracts/v2_contracts/FeeManager.sol:805-822) aligns Timeout only against ResultType.Timeout and DeterministicViolation only against ResultType.DeterministicViolation, so a unanimous-Timeout round recorded as MajorityDisagree marks every validator unaligned. That counter is the validatorsAligned increment at FeeManager.sol:715, which is the same one commitFinalFees divides by at FeeManager.sol:732-733, so the effect is that the whole validatorTimeoutFees pool gets divided by zero alignments rather than by the validator count. Worth noting the division is the sharper edge here, not just the split: feeToDistributePerValidator = (totalAmountToDistribute * 1e18) / (validatorsAligned * 1e18) reverts on validatorsAligned == 0.

   I also tried to confirm the "Status outcomes are unaffected" point and I do not think it holds as stated. _calculateNextStatus (hardhat/contracts/v2_contracts/Transactions.sol:571-582) branches only on MajorityAgree || Agree for Accepted, and sends everything else down the else arm. So MajorityDisagree and Timeout are synonyms there, as you say, but that is only true because neither reaches Accepted. Under the current buggy behavior a unanimous Timeout lands in the else arm and gets Undetermined on even rounds, or on odd rounds the "successfully appealed" path at :584-586 which re-proposes the transaction and starts round+1. Under the fix it takes the identical else arm, since the fix changes the result type and not the Accepted test. So the status sequence is unchanged in both cases. The thing that is not a synonym is fee accounting, which is exactly your point 1. I think we agree and I just want to be precise about which layer absorbs the difference.

   On the fix shape: I took your second form, the smaller diff, rather than reordering the branches. The unanimity short circuit now switches on majorityVote for Agree, Timeout and DeterministicViolation explicitly, and everything else still falls through to MajorityDisagree. Six added lines in Voting.sol, no branch movement:

   if (maxVotes == validatorCount) {
       if (majorityVote == ITransactions.VoteType.Agree) {
           result = ITransactions.ResultType.MajorityAgree;
       } else if (majorityVote == ITransactions.VoteType.Timeout) {
           result = ITransactions.ResultType.Timeout;
       } else if (
           majorityVote == ITransactions.VoteType.DeterministicViolation
       ) {
           result = ITransactions.ResultType.DeterministicViolation;
       } else {
           result = ITransactions.ResultType.MajorityDisagree;
       }
   }

   Reordering would have been equivalent for Timeout and DeterministicViolation but I preferred not to move the maxVotes == validatorCount boundary, since that also keeps the diff reviewable as a pure addition.

   The test is written against the current behavior first, as we agreed: hardhat/test/voting/001_voting_majority_vote.test.js covers the five unanimous VoteType variants and pins NotVoted to its existing MajorityDisagree so no other variant shifts as a side effect. All pass on the patched tree.

   One extra piece of supporting evidence I found while writing it, which is why I am fairly confident this is a bug and not a rule: the Python mirror determine_consensus_from_votes in backend/consensus/utils.py already returns ConsensusResult.TIMEOUT when the timeout count clears the majority threshold. It has no unanimity short circuit at all, which is exactly the branch that drops these two outcomes on the Solidity side. The two implementations disagreed before this and agree after.

   On items 2 and 3, I still think they need the threat-model discussion rather than a patch, and item 2 is a maintainer call on which staking contract is deployed against which Idleness ABI. I am not going to push on those unless you want to. Happy to look again if you tell me the staking deployment you are targeting.
4. HusseinAdeiza

   added a commit that references this issue on Oct 3, 2026

   fix(voting): report unanimous timeout and violation votes accurately

   ...

   9b169f3