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

feat: Add Incremental Flag to Proof Generation #2337

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

felixjordandev
Copy link

Prevents regeneration of existing proofs by adding an --incremental flag that reuses existing proofs when available, avoiding commitment mismatches due to random salts.

Copy link

vercel bot commented Apr 5, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
maci-website ❌ Failed (Inspect) Apr 8, 2025 3:26pm

@felixjordandev
Copy link
Author

@crisgarner This PR aims to fix #1895
Please review and let me know if everything looks good

Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@felixjordandev thanks, left some comments and please fix all the jobs (they all must be green before merging).

@felixjordandev
Copy link
Author

@0xmad I have made the changes you requested and all the jobs are passing. Please merge if everything looks ok. Thanks

Copy link
Collaborator

@ctrlc03 ctrlc03 left a comment

Choose a reason for hiding this comment

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

Thanks - however this does not solve the issue and will end up storing more proofs than needed as when running this.poll.tallyVotes() it will just start from zero. The logic would need to be applied to the Poll class so that it can:

  • either skip the batch(or more) that is already processed and continue processing (but it would require a way to load the previous results)
  • load the already used salts (which should be stored alongside proofs) and recalculate the proofs which will end up with the same commitments due to using the same salts (and not requiring to add a way to reload previous results)

Also it should work for both processMessages and tallyVotes

@felixjordandev
Copy link
Author

Thanks - however this does not solve the issue and will end up storing more proofs than needed as when running this.poll.tallyVotes() it will just start from zero. The logic would need to be applied to the Poll class so that it can:

  • either skip the batch(or more) that is already processed and continue processing (but it would require a way to load the previous results)
  • load the already used salts (which should be stored alongside proofs) and recalculate the proofs which will end up with the same commitments due to using the same salts (and not requiring to add a way to reload previous results)

Also it should work for both processMessages and tallyVotes

I implemented the second approach: loading the already used salts and recalculating the proofs. Please let me know if the changes look good to you. Thanks.


logMagenta({ text: info(`Progress: ${this.poll.numBatchesProcessed} / ${totalMessageBatches}`) });
const batchIndex = this.poll.numBatchesProcessed;
const proofPath = path.join(this.outputDir, `process_${batchIndex}.json`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const proofPath = path.join(this.outputDir, `process_${batchIndex}.json`);
const proofPath = path.resolve(this.outputDir, `process_${batchIndex}.json`);

const exists = fs.existsSync(proofPath);
if (exists) {
// Read file synchronously to avoid await in loop
const existingProof = JSON.parse(fs.readFileSync(proofPath, "utf8")) as Proof;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still sync read here (should be async)

Comment on lines +307 to +313
const batchIndex = this.poll.numBatchesTallied;
const proofPath = path.join(this.outputDir, `tally_${batchIndex}.json`);

let shouldGenerateNewProof = true;

// Check if proof exists and incremental flag is set
if (this.incremental) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is duplicated in proof generation for mp, makes sense to reuse it.

Comment on lines +523 to +524
const foundPoll = poll.polls.get(pollId);
if (!foundPoll) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const foundPoll = poll.polls.get(pollId);
if (!foundPoll) {
const foundPoll = poll.polls.get(pollId);
if (!foundPoll) {

Comment on lines +530 to +531
const statePath = path.join(this.outputDir, `poll_${pollId}_state.json`);
if (fs.existsSync(statePath)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const statePath = path.join(this.outputDir, `poll_${pollId}_state.json`);
if (fs.existsSync(statePath)) {
const statePath = path.resolve(this.outputDir, `poll_${pollId}_state.json`);
if (fs.existsSync(statePath)) {

@@ -49,6 +49,8 @@ import type { PathElements } from "@maci-protocol/crypto";
import { STATE_TREE_ARITY, VOTE_OPTION_TREE_ARITY } from "./utils/constants";
import { ProcessMessageErrors, ProcessMessageError } from "./utils/errors";

import fs from "fs";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Poll shouldn't contain this module

throw new ProcessMessageError(ProcessMessageErrors.InsufficientVoiceCredits);
}
// Calculate vote start and end indices
const voteStartIndex = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this hardcoded?

Comment on lines +544 to +545
* Create circuit input for poll results
* @returns The circuit input
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no poll results circuit.

Comment on lines +566 to +567
* Create circuit input for poll state
* @returns The circuit input
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no poll state circuit

Comment on lines +623 to +624
// (In a real implementation, we would retrieve the stored circuit inputs)
return {} as IProcessMessagesCircuitInputs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can't be merged like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants