refactor: clarify h and h_select
#1696
Conversation
There was a problem hiding this comment.
I definitely support the clarified naming around h and h_select (i.e. the filecoin-proofs changes), however I don't prefer the removal of hs from the vanilla proof. By removing it, you effectively make circuit_hs (and SectorUpdateConfig.h) unnecessary as snap-deals encoding can only ever be run for 1 of the 6 allowed values in hs. Essentially making h a constant at the storage-proofs-update level of abstraction (which it is in practice atm due to it's hardcoding at the the api level), however hs was designed to be a parameter chosen from an audited/secure list hs of values (where each h in hs achieves a unique security-efficiency tradeoff) and removing it from the proof's code beneath the api feels (at least to me) like too invasive a change.
The circuit supports a fixed hard-coded list of `h` values (7-12). Those values are addressed by their index in the list. It can be seen as a 6-bit bit-pattern, which is called `h_select`. The vanilla proof also supports that list of `h` values. The proofs APIs use a fixed value, 1 for sector sizes <= 32KiB and 10 for bigger sector sizes. All this should make the code more consistent and easier to follow. BREAKING CHANGE: `hs` and `HSelect` are not longer part of the `filecoin-proofs` public API.
|
I decided to do a force push as the commit message wasn't accurate after the changes I made due to the review. I wanted the commit message to be correct and not trying to get it right when squashing. So please also have a look at the commit message. I also did not rebase the change yet, so that one can easily review the changes between the already reviewed and the new version. Thanks @DrPeterVanNostrand for the detailed review message, I think I understood what you mean and hope I made the changes in the expected way. Please let me know if I got it wrong. |
| @@ -19,7 +19,7 @@ impl SectorUpdateConfig { | |||
| sector_size: porep_config.sector_size, | |||
| nodes_count, | |||
| update_partitions: UpdateProofPartitions::from(partition_count(nodes_count)), | |||
| h_select: HSelect::from_nodes(nodes_count), | |||
| h: num_high_bits(nodes_count), | |||
There was a problem hiding this comment.
Last comment... maybe rename fn num_high_bits to fn h_default (or something similar, whose name contains "h"). If someone is not super familiar with the code (and doesn't read the docstring), it may not clear that num_high_bits and h are the same thing.
There was a problem hiding this comment.
Naming is hard, h_default is indeed a better name. I didn't want to use just h. Thanks.
|
Nice, thanks for the update; one last comment around the naming of |
It's a better name.
The circuit supports a fixed hard-coded list of
hvalues (7-12). Those values are addressed by their index in the list. It can be seen as a 6-bit bit-pattern, which is calledh_select.The vanilla proof always uses 1 for sector sizes <= 32KiB and 10 for bigger sector sizes. That value is now hard-coded.
All this should make the code more consistent and easier to follow.
BREAKING CHANGE:
hsandHSelectare not longer part of thefilecoin-proofspublic API.