fix: remove challenge seed from NI-PoRep SnarkPack transcript #1755
Conversation
There was a problem hiding this comment.
Not hashing the seed at all makes sense (instead of hashing some fixed value.
I think it would make sense to add a comment to the entry point of the sealing at https://github.com/filecoin-project/rust-fil-proofs/pull/1755/files#diff-7360cc04999e8c2b4559e9d4b8bf83afaeaeb2be3e07314287b00fcc6b81d380L348 to note that for the Non-Interactive PoRep the seed is not used and that it can be any arbitrary value.
Would it make sense to refactor the hash calculation into its own function, so that it's again clearer that it does exactly the same thing for proving as well as for verification?
| hasher.update(seed); | ||
| hasher.update(comm_r); | ||
| if porep_config.feature_enabled(ApiFeature::NonInteractivePoRep) { | ||
| for comm_r in comm_rs.iter() { |
There was a problem hiding this comment.
I think for Non-interactive PoRep, it's always only a single comm_r only. Should we even be stricter here and just hash that value, instead of looping through all (I know it's the same outcome, but it might be clearer to the reader).
There was a problem hiding this comment.
Yeah, I wasn't totally sure what to do here. The way that it's currently written leaves open the possibility of aggregating multiple NI-PoReps into a single SnarkPack proof, however I don't know if that behavior should be allowed.
It used to be a separate function and review feedback implied it wasn't needed to be a separate function. Also, no, it's pretty clear as it is. |
Yup, sounds like a good idea. I'll add it. |
| // Note: when using NI-PoRep the PoRep challenge generation seed is ignored, thus any value can | ||
| // be passed in here for `seed`. |
There was a problem hiding this comment.
Just a note (feel free to ignore). I know we don't really use API docs here (their building is even broken). Though it could make sense to put it into the top of the function (or another note there) as a doc comment, in case we ever properly generate those.
Alternative to #1754. This PR proposes that any PoRep challenge seed(s) (provided as API arguments) not be included in NI-PoRep's SnarkPack transcript, as opposed to using a constant value for NI-PoRep's challenge seed.
Imho, merging of either PR should be held off until a decision is made during the NI-PoRep audit.