filecoin-project / rust-fil-proofs

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

Make Pedersen hash `window_size` configurable #736

Closed porcuquine closed 5 years ago

porcuquine commented 5 years ago

Description

EDIT: the sapling-crypto side of this was done in https://github.com/filecoin-project/sapling-crypto/pull/5. It exposes a new method, JubjubBls12::new_with_window_size. We should call this everywhere instead of new, with the value of a default setting. Let's set the default to 16 now and call it PEDERSEN_HASH_EXP_WINDOW_SIZE.


This is the concrete issue based on 'Spedersen' #697. sapling_crypto already generates a cache when Jubjub parameters are created. Window size is hard coded (to 8): https://github.com/filecoin-project/sapling-crypto/blob/master/src/jubjub/mod.rs#L183-L185

In order to simplify benchmarking, ease tuning, and give miners the freedom to trade time/space as they see fit, we should make this value configurable. The simplest way to do this is probably to modify the JubjubBls12 implementation, adding a field for window size to the struct. (There are other approaches possible, but this seems the cleanest. Let's discuss if the implementor has a different idea.)

In order to minimize the surface area of change, we should probably add a new initialization method, leaving JubjubBls12::new intact. The new method should take window size as a parameter. We don't necessarily need to use the new initialization method everywhere, but should at least do so when replicating from the API and in benchmarks.

We should add a config setting for this and use that value as the default for now. It's conceivable that the right value for window size will vary by sector size (smaller sectors take less memory so can afford larger caches, all else equal) — so it should be convenient to override the default. Use the current hardcoded default (8) as the config default.

Acceptance criteria

Risks + pitfalls

Try to avoid propagating change beyond what's necessary to allow convenient modification of window size.

Where to begin

porcuquine commented 5 years ago

cc: @arielgabizon @DrPeterVanNostrand @nicola @dignifiedquire

porcuquine commented 5 years ago

Based on the benchmarks provided in #697, we should consider changing the default to 16. This seems to be a conservative default which brings most of the time benefit for a very low space cost.

@nicola @arielgabizon Does this seem reasonable?

arielgabizon commented 5 years ago

Seems reasonable. Seems you would never want to use less than 16 if it's only 50kb memory, in the filecoin setting. I don't know the whole picture, maybe in the end a larger default would also be good.

porcuquine commented 5 years ago

@DrPeterVanNostrand Let's plan to bump to 16. It would be worth sanity checking those benchmarks with the new default if you are touching benchmarks anyway, but I think that was already the plan.

porcuquine commented 5 years ago

I tried to do the easy thing and just change the default but bumped into a problem with circuit tests timing out. Needs more investigation, but we might require configurability to deploy this at all. https://github.com/filecoin-project/sapling-crypto/pull/5

cc: @DrPeterVanNostrand @arielgabizon

porcuquine commented 5 years ago

FYI, the issue noted above was resolved in https://github.com/filecoin-project/sapling-crypto/pull/5.