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

Make Pedersen hash window_size configurable #736

Closed
porcuquine opened this issue Jul 12, 2019 · 6 comments · Fixed by #783
Closed

Make Pedersen hash window_size configurable #736

porcuquine opened this issue Jul 12, 2019 · 6 comments · Fixed by #783
Assignees

Comments

@porcuquine
Copy link
Collaborator

porcuquine commented Jul 12, 2019

Description

EDIT: the sapling-crypto side of this was done in filecoin-project/sapling-crypto#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

  • Pedersen hashing window size can be modified by initializing a JubjubBls12 struct with a custom value.
  • Window size can be varied by setting an environment variable or using the config file.
  • The value of the window size being used is reported when benchmarking. (Talk to @dignifiedquire about how to do this — or better, @dignifiedquire please provide some guidance here.)
  • We have concrete benchmark runs showing the effect (on time, ideally also on actual memory usage) showing the effect of increasing window size to a variety of values: candidates (8, 12, 16, 20, 24). At some point, memory usage will be a limiting factor. Include benchmark results in PR.

Risks + pitfalls

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

Where to begin

@porcuquine
Copy link
Collaborator Author

@porcuquine
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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. filecoin-project/sapling-crypto#5

cc: @DrPeterVanNostrand @arielgabizon

@porcuquine
Copy link
Collaborator Author

FYI, the issue noted above was resolved in filecoin-project/sapling-crypto#5.

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 a pull request may close this issue.

4 participants