chore: Introduce PoRepConfig::new_groth16() #1635

hanbu97 · 2022-10-26T06:23:03Z

https://github.com/filecoin-project/rust-fil-proofs/issues/1632

hanbu97 · 2022-10-26T06:51:32Z

#1631
finished

hanbu97 · 2022-10-27T01:37:45Z

Please rerun the workflow, clippy is fixed. Thank you.

shawnrader · 2022-11-09T17:43:31Z

Please rerun the workflow, clippy is fixed. Thank you.

I am still showing some clippy issues after re-running the workflow (some unused imports): https://app.circleci.com/pipelines/github/filecoin-project/rust-fil-proofs/4397/workflows/fd19d319-f5cf-422e-9885-8ff4b8314310/jobs/96937?invite=true#step-105-355

hanbu97 · 2022-11-10T02:02:38Z

Got it. Clippy in tests fixed.

DrPeterVanNostrand

Thank you!

I definitely like the idea of having a simple PorepConfig constructor.

My only concerns with this PR are:

PorepConfig::new_groth16 should be named new until additional proof systems are supported.
Minimum PoRep challenges should be kept as a mutable setting to allow benchy to benchmark non-default PoRep configurations.

DrPeterVanNostrand · 2022-11-10T15:31:43Z

fil-proofs-tooling/src/bin/benchy/prodbench.rs

-        .write()
-        .expect("POREP_MINIMUM_CHALLENGES poisoned")
-        .insert(inputs.sector_size_bytes(), inputs.porep_challenges);
+    // POREP_MINIMUM_CHALLENGES


I think that it still makes sense to allow benchy to configure the number of PoRep challenges, as benchy is used to benchmark varrying PoRep configurations, not just the the default/production configurations.

DrPeterVanNostrand · 2022-11-10T15:32:14Z

filecoin-proofs/src/constants.rs

@@ -47,24 +47,25 @@ pub const PUBLISHED_SECTOR_SIZES: [u64; 10] = [
    SECTOR_SIZE_64_GIB,
 ];

+pub struct PorepMinimumChallenges;
+impl PorepMinimumChallenges {
+    pub const fn porep_partitions(sector_size: u64) -> usize {


I think that a better name for this function would be from_sector_size, however to allow for varying PoRep configurations during benchmarking (e.g. benchy), I think that it's better to leave this as a mutable HashMap rather than hardcoded constants.

DrPeterVanNostrand · 2022-11-10T15:35:03Z

filecoin-proofs/src/types/porep_config.rs

@@ -54,6 +55,22 @@ impl From<PoRepConfig> for SectorSize {
 }

 impl PoRepConfig {
+    /// construct PoRepConfig by groth16
+    pub fn new_groth16(sector_size: u64, porep_id: [u8; 32], api_version: ApiVersion) -> Self {


Until additional proof systems are supported in rust-fil-proofs, I think that it's better to exclude the groth16 identifier from the function name as Groth16 is the only proof system currently supported. If/when additional proof systems are supported, I definitely think that it makes sense to use the function name new_groth16, but for now, I think that new is a better name.

vmx · 2022-11-10T15:49:23Z

1. `PorepConfig::new_groth16` should be named `new` until additional proof systems are supported.

I proposed in the original issue at #1632 naming it new_groth16(). I still think it's a good idea (and less hassle for the future), but if @DrPeterVanNostrand feels strongly about it, we can also change it to just new().

storojs72

I support this change (though it still requires to fix merge conflicts), as it removes more lines of code than adds (+88; -218), that makes codebase a bit more readable.

hanbu97 · 2022-11-11T04:44:25Z

What should I do next, or some tasks else.

vmx · 2022-11-11T10:44:37Z

@hanbu97 The comment about making things again a mutable HashMap is something you could work on. I'll talk with @DrPeterVanNostrand directly whether it should be new or new_groth16 and let you know once we've come to a decision.

vmx

Thanks a lot, I really like the changes!

DrPeterVanNostrand

Looks good to me. My only concern is the naming of the function PorepMinimumChallenges::porep_partitions.

DrPeterVanNostrand · 2022-11-15T15:55:11Z

filecoin-proofs/src/constants.rs

+        self.0.write().expect("POREP_MINIMUM_CHALLENGES poisoned")
+    }
+
+    pub fn porep_partitions(&self, sector_size: u64) -> usize {


To me, the name of this function implies that we are deriving the partition count from the number of porep challenges. I think that a better name for this function would be something like from_sector_size or for_sector_size as we are retrieving the minimum challenge count for a given sector size.

I agree with you, will make it more readable. Changes done.

Awesome, thank you!

vmx

Looks good to me. I'm just waiting for @DrPeterVanNostrand for final approval.

DrPeterVanNostrand · 2022-12-08T15:21:39Z

Looks good to me. Thanks @hanbu97!

vmx

As @DrPeterVanNostrand also approved it, I hereby approve it.

@hanbu97 could you please rebase one more time? Best would be if you would squash it into a single commit.

hanbu97 · 2022-12-09T02:28:20Z

@hanbu97 could you please rebase one more time? Best would be if you would squash it into a single commit.

When I tried to squash commits. Some error occurs.

Unable to squash. Squashing replays all commits up to the last one required for the squash. A merge commit cannot exist among those commits.

It seems because I merged some changes at developing time. I will pay attention to it next time. If the squash is required I may create a new branch.

Instead of constructing the `PoRepConfig` directly, use a constructor function. This simplifies the code and makes things less error-prone. Fixes filecoin-project#1632.

vmx · 2022-12-09T13:59:32Z

@hanbu97 I dared to squash it into a single commit for you. Please let me know if you agree with that commit and the commit message.

I did a git rebase -i master and the fixed up the commits.

hanbu97 · 2022-12-10T06:30:05Z

Sure go ahead. Let's make it done. Thank you.

vmx · 2022-12-12T10:13:26Z

Thanks again @hanbu97 and sorry that it took so long to get it merged.

Instead of constructing the `PoRepConfig` directly, use a constructor function. This simplifies the code and makes things less error-prone. Fixes #1632. Co-authored-by: 卜翰 <buhan970731@gmail.com>

* fix: use current process binding to limit thread cores (#1633) Use the current processes bound cores to limit the possible cores that threads can be bound to. This allows core binding to work properly when the lotus-worker service is limited to certain CPUs by cgroups. * fix: update ec-gpu-gen (#1638) `ec-gpu-gen` needs to be updated to v0.5 as v0.4 has dependencies that depend on yanked version. It's an indirect dependency of `bellperson` and `neptune`, which are upgraded here. Moving from `memmap` (which is deprecated) to `memmap2` was also needed als dependencies also switched. `chrono` updated their API, so there was also a small change needed. * fix: broken Links in README.md #1637 (#1649) * feat: Introduce PoRepConfig::new_groth16() (#1635) Instead of constructing the `PoRepConfig` directly, use a constructor function. This simplifies the code and makes things less error-prone. Fixes #1632. Co-authored-by: 卜翰 <buhan970731@gmail.com> * fix: clean up tree definitions (#1655) There were quite a few public type definitions, that were mostly replaced by the `SectorShape*` types. This commit cleans them up and moves them around if appropriate. This should make the code easier to follow and the public API surface smaller. BREAKING CHANGE: `BinaryLCMerkleTree`, `BinaryMerkleTree`, `BinarySubMerkleTree`, `LCMerkleTree`, `LCStore`, `MerkleStore`, `MerkleTree`, `OctLCMerkleTree`, `OctLCSubMerkleTree`, `OctLCTopMerkleTree`, `OctMerkleTree`, `OctSubMerkleTree`, `OctTopMerkleTree`, `QuadLCMerkleTree` and `QuadMerkleTree` are removed from the public interface. * fix: update ec-gpu-gen (#1638) `ec-gpu-gen` needs to be updated to v0.5 as v0.4 has dependencies that depend on yanked version. It's an indirect dependency of `bellperson` and `neptune`, which are upgraded here. Moving from `memmap` (which is deprecated) to `memmap2` was also needed als dependencies also switched. `chrono` updated their API, so there was also a small change needed. * chore: update Cargo.lock * fix: there was a memmap -> memmap2 missing * fix: make poseidon tests pass Neptune currently is a fork of pasta_curves. That needs patching as well, in order to get the correct names for the fields out. * ci: Apply rustfmt and fix clippy * Pick relevant branch of neptune * fix: Use SHA256 hasher for binary trees Co-authored-by: Clint Armstrong <clint@clintarmstrong.net> Co-authored-by: Volker Mische <volker.mische@gmail.com> Co-authored-by: hanbu97 <98807352+hanbu97@users.noreply.github.com> Co-authored-by: 卜翰 <buhan970731@gmail.com>

hanbu97 requested review from cryptonemo, DrPeterVanNostrand and vmx as code owners October 26, 2022 06:23

vmx requested a review from shawnrader November 10, 2022 13:05

DrPeterVanNostrand requested changes Nov 10, 2022

View reviewed changes

storojs72 previously approved these changes Nov 10, 2022

View reviewed changes

hanbu97 requested review from DrPeterVanNostrand and removed request for shawnrader November 11, 2022 04:49

hanbu97 dismissed storojs72’s stale review via a4bee38 November 11, 2022 13:05

hanbu97 requested review from storojs72 and removed request for DrPeterVanNostrand November 14, 2022 01:27

vmx previously approved these changes Nov 14, 2022

View reviewed changes

DrPeterVanNostrand requested changes Nov 15, 2022

View reviewed changes

hanbu97 dismissed vmx’s stale review via 4fe5187 November 16, 2022 01:51

hanbu97 requested review from DrPeterVanNostrand and vmx and removed request for storojs72 and DrPeterVanNostrand November 16, 2022 06:00

vmx previously approved these changes Nov 17, 2022

View reviewed changes

hanbu97 dismissed vmx’s stale review via 18d1f81 December 6, 2022 02:33

hanbu97 requested a review from DrPeterVanNostrand December 8, 2022 02:54

vmx previously approved these changes Dec 8, 2022

View reviewed changes

DrPeterVanNostrand previously approved these changes Dec 8, 2022

View reviewed changes

feat: Introduce PoRepConfig::new_groth16()

13517c7

Instead of constructing the `PoRepConfig` directly, use a constructor function. This simplifies the code and makes things less error-prone. Fixes filecoin-project#1632.

vmx dismissed stale reviews from DrPeterVanNostrand and themself via 13517c7 December 9, 2022 13:58

vmx force-pushed the master branch from ee4d7fa to 13517c7 Compare December 9, 2022 13:58

shawnrader approved these changes Dec 9, 2022

View reviewed changes

vmx approved these changes Dec 12, 2022

View reviewed changes

vmx merged commit 62ed86f into filecoin-project:master Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Introduce PoRepConfig::new_groth16() #1635

chore: Introduce PoRepConfig::new_groth16() #1635

hanbu97 commented Oct 26, 2022

hanbu97 commented Oct 26, 2022

hanbu97 commented Oct 27, 2022

shawnrader commented Nov 9, 2022

hanbu97 commented Nov 10, 2022

DrPeterVanNostrand left a comment

DrPeterVanNostrand Nov 10, 2022

DrPeterVanNostrand Nov 10, 2022

DrPeterVanNostrand Nov 10, 2022 •

edited

Loading

vmx commented Nov 10, 2022

storojs72 left a comment •

edited

Loading

hanbu97 commented Nov 11, 2022

vmx commented Nov 11, 2022

vmx left a comment

DrPeterVanNostrand left a comment

DrPeterVanNostrand Nov 15, 2022

hanbu97 Nov 16, 2022

DrPeterVanNostrand Dec 8, 2022

vmx left a comment

DrPeterVanNostrand commented Dec 8, 2022

vmx left a comment

hanbu97 commented Dec 9, 2022

vmx commented Dec 9, 2022

hanbu97 commented Dec 10, 2022

vmx commented Dec 12, 2022

chore: Introduce PoRepConfig::new_groth16() #1635

chore: Introduce PoRepConfig::new_groth16() #1635

Conversation

hanbu97 commented Oct 26, 2022

hanbu97 commented Oct 26, 2022

hanbu97 commented Oct 27, 2022

shawnrader commented Nov 9, 2022

hanbu97 commented Nov 10, 2022

DrPeterVanNostrand left a comment

Choose a reason for hiding this comment

DrPeterVanNostrand Nov 10, 2022

Choose a reason for hiding this comment

DrPeterVanNostrand Nov 10, 2022

Choose a reason for hiding this comment

DrPeterVanNostrand Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

vmx commented Nov 10, 2022

storojs72 left a comment • edited Loading

Choose a reason for hiding this comment

hanbu97 commented Nov 11, 2022

vmx commented Nov 11, 2022

vmx left a comment

Choose a reason for hiding this comment

DrPeterVanNostrand left a comment

Choose a reason for hiding this comment

DrPeterVanNostrand Nov 15, 2022

Choose a reason for hiding this comment

hanbu97 Nov 16, 2022

Choose a reason for hiding this comment

DrPeterVanNostrand Dec 8, 2022

Choose a reason for hiding this comment

vmx left a comment

Choose a reason for hiding this comment

DrPeterVanNostrand commented Dec 8, 2022

vmx left a comment

Choose a reason for hiding this comment

hanbu97 commented Dec 9, 2022

vmx commented Dec 9, 2022

hanbu97 commented Dec 10, 2022

vmx commented Dec 12, 2022

DrPeterVanNostrand Nov 10, 2022 •

edited

Loading

storojs72 left a comment •

edited

Loading