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

remove superfluous conditional #1449

Merged
merged 1 commit into from
May 23, 2024

Conversation

waitfreemaxi
Copy link

The if statement removed always evaluates to true by the following lemma:

Lemma 1:
The confirmation count of the N-th recent lockout (zero-index) is greater than N.

Proof by contradiction: If the Nth recent lockout has a confirmation count less than or equal to N, then, due to the monotonicity of confirmation counts, the (N-1)th most recent lockout must be less than or equal to (N-1). Applying induction, the first most recent lockout (index = 0) must have a confirmation count less less than or equal to 0. However, it is impossible for a vote to have a confirmation count of 0. So there is a contradiction.

@mergify mergify bot requested a review from a team May 21, 2024 16:04
@waitfreemaxi
Copy link
Author

Alternatively, it's probably faster and cheaper to just check the lockout threshold every time >.<, instead of iterating through a 31 element tower starting from the vote with the highest confirmation count

@waitfreemaxi
Copy link
Author

@AshwinSekar

@AshwinSekar AshwinSekar added the CI Pull Request is ready to enter CI label May 23, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label May 23, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.7%. Comparing base (b711bda) to head (6284aee).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1449     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         871      871             
  Lines      370376   370374      -2     
=========================================
- Hits       306561   306540     -21     
- Misses      63815    63834     +19     

Copy link

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the change.
As for your comment about improving the check, it could be possible if we had an index into the threshold_vote slot in the old tower. Remember we want to check threshold only if the threshold vote slot has doubled its lockouts, so we can't just compare the 4 / 8 deep slot to see if it changed.

@AshwinSekar AshwinSekar merged commit 1eaa5cf into anza-xyz:master May 23, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants