api: Fix vote status computation. #5228
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5228 +/- ##
==========================================
- Coverage 53.52% 46.93% -6.59%
==========================================
Files 441 441
Lines 55140 55138 -2
==========================================
- Hits 29511 25880 -3631
- Misses 23340 26820 +3480
- Partials 2289 2438 +149
... and 190 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
winder
force-pushed
the
will/vote-status-fix
branch
from
f8e1b78 to
bc69c49
Compare
|
For reference, the original issue looked like this: |
|
@winder please rebase |
| consensus := config.Consensus[protocol.ConsensusCurrentVersion] | ||
| upgradeVoteRounds := consensus.UpgradeVoteRounds | ||
| upgradeThreshold := consensus.UpgradeThreshold | ||
| votes := uint64(consensus.UpgradeVoteRounds) - uint64(votesToGo) | ||
| votes := consensus.UpgradeVoteRounds - votesToGo |
There was a problem hiding this comment.
Are you asking to change these to uint64? I think a negative number and an underflow are both equally wrong, but a negative number would be easier to debug.
There was a problem hiding this comment.
no I think I meant, "I followed the definitions of these things enough to convince me that this value is always nonnegative" in the normal use case
| // Check if the vote window is still open. | ||
| if stat.NextProtocolVoteBefore > stat.LastRound { | ||
| // subtract 1 because the variables are referring to "Last" round and "VoteBefore" | ||
| votesToGo = uint64(stat.NextProtocolVoteBefore - stat.LastRound - 1) |
There was a problem hiding this comment.
This can't be an underflow because it is inside a check that NextProtocolVoteBefore > LastRound.
| statusString = statusString + "\n" + fmt.Sprintf( | ||
| infoNodeStatusConsensusUpgradeVoting, | ||
| upgradeYesVotes, | ||
| upgradeNoVotes, | ||
| upgradeNextProtocolVoteBefore-stat.LastRound, | ||
| upgradeVoteRounds-upgradeYesVotes-upgradeNoVotes, |
There was a problem hiding this comment.
upgradeVoteRounds was calculated in algod so this more direct formula can be used in the client
winder
force-pushed
the
will/vote-status-fix
branch
from
bc69c49 to
8c06408
Compare
|
On Mar 29, 2023, at 21:51, Pavel Zbitskiy ***@***.***> wrote:
@will please rebaseI can try tomorrow, but I probably don’t have write permissions on this repo, sorry in advance
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I apology I trusted the autocomplete feature and mis-tagged you. |
Summary
Fix an off by one error which caused a display issue where the number of votes remaining show +1 compared to the true value and the number of no votes show -1 compared to the true value.
Test Plan
New unit test which reproduced an underflow when there were 0 no votes.
Manual test with this code rebased on
feature/alphanet:A consensus upgrade with no votes, Yes + No + Remaining = 10000. Here you can see 1249+116+8635=10000:
A consensus upgrade unanimous yes votes, Yes + No + Remaining = 10000. Again, here you can see 2628 + 0 + 7372 = 10000: