filecoin-project / rust-fil-proofs

Proofs for Filecoin in Rust
Other
489 stars 314 forks source link

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

Closed vmx closed 4 months ago

vmx commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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