Topup feature does not work - incorrectly throw insufficient funds error

[yuetloo]

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

Given the following test case, genProof should generate correct tally result.

contribute 1 voice credits
topup 2 voice credits
vote 2 voice credit to project 1
vote 1 voice credits to project 2

The genProof output, tally.json file should produce the following result:

totalSpentVoiceCredits.spent = 3
perVOSpentVoiceCredits.tally[1] = 2
perVOSpentVoiceCredits.tally[2] = 1

Current Behavior

What is the current behavior?

genProofs throws InsufficientVoiceCredits error.

https://github.com/privacy-scaling-explorations/maci/blob/dev/core/ts/Poll.ts#L281

      // If the remaining voice credits is insufficient, do nothing
      if (voiceCreditsLeft < 0n) {
        throw new ProcessMessageError(ProcessMessageErrors.InsufficientVoiceCredits);
      }

Failure Information

The issue is that topup and vote messages are processed in reverse order instead of the order they show up in the logs, see the following line here:
https://github.com/privacy-scaling-explorations/maci/blob/dev/core/ts/Poll.ts#L507

As a result, when it processes the first vote message with 2 voice credits, it hasn't processed the topup message, which is supposed to update the state tree balance here:

https://github.com/privacy-scaling-explorations/maci/blob/dev/core/ts/Poll.ts#L580-L582

newStateLeaf.voiceCreditBalance += amount;
              // save it
              this.stateLeaves[stateIndex] = newStateLeaf;

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]

thanks @yuetloo - "The issue is that topup and vote messages are processed in reverse order instead of the order they show up in the logs, see the following line here:" yes that's how processing works, so you would need to reverse how you send the topup and the message which goes over the voice credits limit.

contribute 1 voice credits

topup 2 voice credits
vote 2 voice credit to project 1
vote 1 voice credits to project 2

should be

contribute 1 voice credits

vote 2 voice credit to project 1
topup 2 voice credits
vote 1 voice credits to project 2

[yuetloo]

@ctrlc03 , I don't see how I can reorder the log events as suggested. The events are logged by the contract and the contract has to enforce the order and verify the contributor has enough voice credits before they can vote.

contribute 1 voice credits
vote 2 voice credit to project 1. => contract logic should revert this transaction as the contributor does not have enough voice credits at this point

[yuetloo]

@ctrlc03 , I see that MACI doesn't validate votes submitted by the publishMessage(), so, clrfund could do this:

tx:1
contribute 1 voice credits

tx:2
vote 2 voice credits to project A
vote 1 voice credit to project B
topup 2 voice credits

[ctrlc03]

@ctrlc03 , I don't see how I can reorder the log events as suggested. The events are logged by the contract and the contract has to enforce the order and verify the contributor has enough voice credits before they can vote.

contribute 1 voice credits
vote 2 voice credit to project 1. => contract logic should revert this transaction as the contributor does not have enough voice credits at this point

the contract verifies that the caller has enough voice credits? where? feel like I'm missing something

[ctrlc03]

@ctrlc03 , I see that MACI doesn't validate votes submitted by the publishMessage(), so, clrfund could do this:

tx:1 contribute 1 voice credits

tx:2 vote 2 voice credits to project A vote 1 voice credit to project B topup 2 voice credits

yes correct, votes are encrypted, so they cannot be validated on chain

[yuetloo]

@ctrlc03 , there's an issue here because a coordinator can process topup messages in an order that would cause votes to be ignored and still produce a zk proof that passes the off-chain and on-chain verifications.

In MACI v0, if the coordinator didn't process the signup messages correctly that contain the initial voice credits information, the zk proof would fail the on-chain verification.

[yuetloo]

@ctrlc03 , I don't see how I can reorder the log events as suggested. The events are logged by the contract and the contract has to enforce the order and verify the contributor has enough voice credits before they can vote.

contribute 1 voice credits
vote 2 voice credit to project 1. => contract logic should revert this transaction as the contributor does not have enough voice credits at this point

the contract verifies that the caller has enough voice credits? where? feel like I'm missing something

Sorry, yes, you're right, contract cannot validate voice credits because vote messages are encrypted.

[ctrlc03]

@ctrlc03 , there's an issue here because a coordinator can process topup messages in an order that would cause votes to be ignored and still produce a zk proof that passes the off-chain and on-chain v

In MACI v1 that is the same, all signups must be processed.
Topup messages are just like any other vote message, they are stored in the same merkle tree and processed in reverse order together with other vote messages

[samajammin]

Maybe more relevant in a discussion vs. this GH issue but... should we consider removing the topup feature?

Do we have any sense of the demand for this feature? Is it worth the added complexity?

[ctrlc03]

Closing this as the topup feature was removed until a new design is implemented. #1421