search

filecoin-project / rust-fil-proofs

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

fix: increase ni-porep min challenges for test sectors #1756

Closed DrPeterVanNostrand closed 5 months ago

DrPeterVanNostrand commented 5 months ago

Changes the minimum number NI-PoRep challenges for test sectors from 4 to ceil(12.8 * min_porep_challenges) (i.e. 26 challenges) and the NI-PoRep partition count for test sectors from 2 to 13; bumping the partition count allows NI-PoRep test sectors to use their current Groth16 keys (i.e. 2 PoRep challenges per circuit/partition).

vmx commented 5 months ago

It looks like the only longer running test where it makes a difference is test_ignored_release_filecoin_proofs where the test time goes from 30 to 40mins. But if it turns out that the total run time is a problem, I guess we could always split tests into separate jobs.

cryptonemo commented 5 months ago

It looks like the only longer running test where it makes a difference is test_ignored_release_filecoin_proofs where the test time goes from 30 to 40mins. But if it turns out that the total run time is a problem, I guess we could always split tests into separate jobs.

Since it's actually testing the 'non-interactivity' aspect now where it wasn't previously, I think it's important to keep it and not sweat the automated testing time.

DrPeterVanNostrand commented 5 months ago

The original "4" was chosen to have a similar run time to the Interactive PoRep times

Oh ok, that was a good idea. I didn't realize that was how the number of challenges was chosen so I wasn't even thinking about test times.

DrPeterVanNostrand commented 5 months ago

not sweat the automated testing time

Ok, I'm going to merge.

vmx commented 5 months ago

Since it's actually testing the 'non-interactivity' aspect now where it wasn't previously,

I compared it with the numbers that did already had the Non-interactive PoRep tests. Non-interactive with 4 PoReps add about 5mins to the job, increasing it to 26 adds about another 10mins. Though I agree it's not a deal breaker.