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

fix: use a fixed seed for Non-interactive PoRep #1754

Closed
wants to merge 1 commit into from

Conversation

vmx
Copy link
Contributor

@vmx vmx commented May 8, 2024

The Interactive PoRep requires a seed for the proof. The Non-interactive PoRep does not. In order to keep the public APIs simple, just make sure that the passed in seed in case of a Non-interactive PoRep is a fixed one. It was choosen to be all zeros as such a byte array is easy to generate.

That fixed seed is used for the proving (and the internal aggregation) as well as the verification.

This commit also removes the restriction that a seed cannot be all zeros, which it also totally could be by coincidence (it's randomly generated in case of the Interactive PoRep).

The Interactive PoRep requires a seed for the proof. The Non-interactive PoRep
does not. In order to keep the public APIs simple, just make sure that the
passed in seed in case of a Non-interactive PoRep is a fixed one. It was choosen
to be all zeros as such a byte array is easy to generate.

That fixed seed is used for the proving (and the internal aggregation) as well as
the verification.

This commit also removes the restriction that a seed cannot be all zeros, which
it also totally could be by coincidence (it's randomly generated in case of the
Interactive PoRep).
@cryptonemo
Copy link
Collaborator

This commit also removes the restriction that a seed cannot be all zeros, which it also totally could be by coincidence (it's randomly generated in case of the Interactive PoRep).

No, you can't remove this. It changes past chain behaviour and could cause an unnecessary fork. There's also no reason to do this.

Instead of requiring NI-PoRep get passed an arbitrary fixed seed, I'd rather see it overwritten/generated internally and wired through as a fixed value. So user passed in value is ignored and we pass in our own internal one. It should also be listed in the spec (the specific value).

@vmx
Copy link
Contributor Author

vmx commented May 8, 2024

So user passed in value is ignored and we pass in our own internal one. It should also be listed in the spec (the specific value).

I intentionally would like the outside callers need to pass in the fixed value, so that it's clear from the public API that the Non-interactive PoRep doesn't need the same seen as the interactive PoRep. It might be obvious today, but it might not be obvious for people new to the code down the line. This way I hope someone new to the code would not wonder "according to the spec it's a fixed/no seed, but the API still requires one, that is weird".

@DrPeterVanNostrand
Copy link
Contributor

My idea for the fix may be even simpler/dumber than the two above (either requiring a constant be passed in for the seed or overwriting the passed in seed with a constant); just don't hash the seed that was passed in by the user into the SnarkPack transcript (because the public inputs for NI-PoRep don't depend on the seed, and afaik, that means that it shouldn't be hashed into the transcript).

So changing this code block and this code block in PoRep aggregation and verification to something like...

let hashed_seeds_and_comm_rs: [u8; 32] = {
    let mut hasher = Sha256::new();
    if porep_config.feature_enabled(ApiFeature::NonInteractivePoRep) {
        for comm_r in comm_rs.iter() {
            hasher.update(seed);
        }
    } else {
        for (seed, comm_r) in seeds.iter().zip(comm_rs.iter()) {
            hasher.update(seed);
            hasher.update(comm_r);
    }
    hasher.finalize().into()
};

@vmx
Copy link
Contributor Author

vmx commented May 13, 2024

I'm closing this one in favour of #1755.

@vmx vmx closed this May 13, 2024
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