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

Incorrect BallotsTallied condition #1137

Closed
3 tasks done
yuetloo opened this issue Jan 30, 2024 · 0 comments · Fixed by #1139
Closed
3 tasks done

Incorrect BallotsTallied condition #1137

yuetloo opened this issue Jan 30, 2024 · 0 comments · Fixed by #1139
Assignees

Comments

@yuetloo
Copy link
Contributor

yuetloo commented Jan 30, 2024

Prerequisites

Please answer the following questions for yourself before submitting an issue.

  • I am not running a deprecated version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Expected Behavior

Please describe the behavior you are expecting

The condition to check if all ballots were tallied should be batchStartIndex >= numSignUps to account for the numSignUps === tallyBatchSize case.

For example, if tallyBatchSize = 25, and numSignUps = 25, the function will not emit the BallotsTallied event because (cachedBatchNum + 1) * tallyBatchSize = 25 which is not greater than 25.

https://github.com/privacy-scaling-explorations/maci/blob/dev/contracts/contracts/Tally.sol#L138-L140

    (uint8 intStateTreeDepth, , , ) = poll.treeDepths();
    uint256 tallyBatchSize = TREE_ARITY ** intStateTreeDepth;
    uint256 batchStartIndex = cachedBatchNum * tallyBatchSize;
    (uint256 numSignUps, ) = poll.numSignUpsAndMessages();

    // Require that there are untalied ballots left
    if (batchStartIndex > numSignUps) {
      revert AllBallotsTallied();
    }

https://github.com/privacy-scaling-explorations/maci/blob/dev/contracts/contracts/Tally.sol#L151-L153

    if ((cachedBatchNum + 1) * tallyBatchSize > numSignUps) {
      emit BallotsTallied(address(poll));
    }

Current Behavior

What is the current behavior?

Failure Information

Please help provide information about the failure if this is a bug. If it is not a bug, please remove the rest of this template.

Steps to Reproduce

Please provide detailed steps for reproducing the issue.

  1. step 1
  2. step 2
  3. you get it...

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

  • MACI version:
  • Firmware Version:
  • Operating System:
  • SDK version:
  • Toolchain version:

Failure Logs

Please include any relevant log snippets or files here.

@ctrlc03 ctrlc03 self-assigned this Jan 31, 2024
@ctrlc03 ctrlc03 added this to MACI Jan 31, 2024
@ctrlc03 ctrlc03 moved this to In Progress in MACI Jan 31, 2024
ctrlc03 added a commit that referenced this issue Jan 31, 2024
Ensure we stop tallying ballots correctly, and that an event is emitted even if batchStartIndex ==
number of ballots

fix #1137
ctrlc03 added a commit that referenced this issue Jan 31, 2024
Ensure we stop tallying ballots correctly, and that an event is emitted even if batchStartIndex ==
number of ballots

fix #1137
ctrlc03 added a commit that referenced this issue Jan 31, 2024
Ensure we stop tallying ballots correctly, and that an event is emitted even if batchStartIndex ==
number of ballots

fix #1137
@github-project-automation github-project-automation bot moved this from In Progress to Done in MACI Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants