-
Notifications
You must be signed in to change notification settings - Fork 29
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
Caphash optimization #45
base: master
Are you sure you want to change the base?
Changes from 7 commits
a3038f9
e03582d
87828ca
62cff94
62245b9
12259be
5b276ee
e967012
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,11 @@ bcs_transformation_parameters<FieldT, MT_root_hash> default_bcs_params( | |
params.hash_enum = hash_type; | ||
/* TODO: Push setting leaf hash into internal BCS code. Currently 2 is fine, as leaf size is internally unused. */ | ||
const size_t leaf_size = 2; | ||
params.leafhasher_ = get_leafhash<FieldT, MT_root_hash>(hash_type, security_parameter, leaf_size); | ||
params.cap_size = 2; | ||
params.leafhasher_ = get_leafhash<MT_root_hash, FieldT>(hash_type, security_parameter, leaf_size); | ||
params.compression_hasher = get_two_to_one_hash<MT_root_hash, FieldT>(hash_type, security_parameter); | ||
params.hashchain_ = | ||
get_hashchain<FieldT, MT_root_hash>(hash_type, security_parameter); | ||
params.cap_hasher = get_cap_hash<MT_root_hash, FieldT>(hash_type, security_parameter); | ||
params.hashchain_ = get_hashchain<FieldT, MT_root_hash>(hash_type, security_parameter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed above, we are switching Do we want to apply the same refactoring to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I'd prefer to keep it the same for this PR, unless it causes merge conflicts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's acceptable, I'd like to keep them both the current order for now. We can refactor both at the same time in a future PR. This PR is already quite big, and it's been a long time since I wrote this so I don't remember all the context |
||
|
||
// Work per hash. Todo generalize this w/ proper explanations of work amounts | ||
const size_t work_per_hash = (hash_type == 1) ? 1 : 128; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this just a bug before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I switched the order to be consistent with similar functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was here before was right I believe https://github.com/scipr-lab/libiop/blob/master/libiop/bcs/hashing/hash_enum.hpp#L33-L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still believe the template ordering before was right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, you switched the template ordering of the internal command. Currently there are inconsistent orderings throughout, I believe the old
<FieldT, MT_root_hash>
was consistently used everywhere. We should consistently be using one or the other, I don't really have a preference for which one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched all instances of the leaf hash functions to be the same as the hashchain. If you insist, I can change it back, but it is currently consistent as well.