privacy-scaling-explorations / maci

Minimal Anti-Collusion Infrastructure (MACI)
https://maci.pse.dev/
Other
520 stars 137 forks source link

A blank state leaf is not counted as 1 signup #947

Closed ctrlc03 closed 9 months ago

ctrlc03 commented 9 months ago

Currently, we do not count the BLANK_STATE_LEAF added to the stateAq upon MACI's contract deployment as one signup. This could cause the tree to be filled with 1 extra leaf than supported by the circuits.

Furthermore, we wrongly calculate the number of batches in the proveOnChain cli command -> if we have 10 signups and the batch size is 5 -> we end up checking proofs for 3 batches instead of 2

baumstern commented 9 months ago

I see where there might be some confusion, but it's crucial to differentiate the BLANK_STATE_LEAF's role from the signup count. The BLANK_STATE_LEAF at index 0 is a technical safeguard—it's there to prevent a specific type of denial-of-service attack, not to represent an actual signup. Counting it as a signup would mix up the tree's structural integrity with the signup logic, which are separate concerns. So, in the context of signups, we should only count actual user registrations, not structural placeholders like the BLANK_STATE_LEAF.

ctrlc03 commented 9 months ago

I see where there might be some confusion, but it's crucial to differentiate the BLANK_STATE_LEAF's role from the signup count. The BLANK_STATE_LEAF at index 0 is a technical safeguard—it's there to prevent a specific type of denial-of-service attack, not to represent an actual signup. Counting it as a signup would mix up the tree's structural integrity with the signup logic, which are separate concerns. So, in the context of signups, we should only count actual user registrations, not structural placeholders like the BLANK_STATE_LEAF.

Sure, it is not a user signup per se, though given we add this blankStateLeaf inside the tree, it fills one space, and when it comes to signups, if we were to accept 5 stateTreeArity + 1, the last signup would not be recognized (if the tree was ever at capacity of course) because we'd only have as many ballots as 5 stateTreeArity and the last one would never be processed nor fed into the circuit

This is the same for the message tree

baumstern commented 9 months ago

I see where there might be some confusion, but it's crucial to differentiate the BLANK_STATE_LEAF's role from the signup count. The BLANK_STATE_LEAF at index 0 is a technical safeguard—it's there to prevent a specific type of denial-of-service attack, not to represent an actual signup. Counting it as a signup would mix up the tree's structural integrity with the signup logic, which are separate concerns. So, in the context of signups, we should only count actual user registrations, not structural placeholders like the BLANK_STATE_LEAF.

Sure, it is not a user signup per se, though given we add this blankStateLeaf inside the tree, it fills one space, and when it comes to signups, if we were to accept 5 stateTreeArity + 1, the last signup would not be recognized (if the tree was ever at capacity of course) because we'd only have as many ballots as 5 stateTreeArity and the last one would never be processed nor fed into the circuit

This is the same for the message tree

I think we have two options here:

ctrlc03 commented 9 months ago

I see where there might be some confusion, but it's crucial to differentiate the BLANK_STATE_LEAF's role from the signup count. The BLANK_STATE_LEAF at index 0 is a technical safeguard—it's there to prevent a specific type of denial-of-service attack, not to represent an actual signup. Counting it as a signup would mix up the tree's structural integrity with the signup logic, which are separate concerns. So, in the context of signups, we should only count actual user registrations, not structural placeholders like the BLANK_STATE_LEAF.

Sure, it is not a user signup per se, though given we add this blankStateLeaf inside the tree, it fills one space, and when it comes to signups, if we were to accept 5 stateTreeArity + 1, the last signup would not be recognized (if the tree was ever at capacity of course) because we'd only have as many ballots as 5 stateTreeArity and the last one would never be processed nor fed into the circuit

This is the same for the message tree

I think we have two options here:

  • guide users to manually adjust numSignUps: inform users that they need to manually subtract one from the numSignUps to account for the BLANK_STATE_LEAF

  • set Signup Limit to 5^n−1: with n being the state tree depth, adjusting the signup limit to 5^n−1 would inherently account for the BLANK_STATE_LEAF

Yes your comment on the PR made me think it might be a much better solution to adjust the limit to be 5**stateTreeDepth - 1, thanks for that