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

Introduce PoRepConfig::new_groth16() #1632

Closed
vmx opened this issue Sep 27, 2022 · 2 comments
Closed

Introduce PoRepConfig::new_groth16() #1632

vmx opened this issue Sep 27, 2022 · 2 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@vmx
Copy link
Contributor

vmx commented Sep 27, 2022

Description

At a lot of places the PoRepConfig struct is created directly which almost always follows a pattern similar to:

let config = PoRepConfig {
    sector_size: SectorSize(SECTOR_SIZE_32_GIB),
    partitions: PoRepProofPartitions(
        *POREP_PARTITIONS
            .read()
            .expect("POREP_PARTITIONS poisoned")
            .get(&SECTOR_SIZE_32_GIB)
            .expect("unknown sector size") as usize,
    ),
    porep_id,
    api_version: ApiVersion::V1_1_0,
};

Instead introduce a PoRepConfig::new_groth16() which could look like this:

let config = PoRepConfig::new_groth16(SECTOR_SIZE_32_GIB, porep_id, ApiVersion::V1_1_0)

The reson for naming it new_groth16 and not just new is, that the Halo2 work introduces PoRepConfig::new_halo2.

Acceptance criteria

PoRepConfig::new_groth16() is used everywhere where currently the PoRepConfig struct is constructed directly.

Risks + pitfalls

I can't think of any.

Where to begin

Introduce PoRepConfig::new_groth16() and use it everywhere where currently the PoRepConfig is used directly.

@vmx vmx added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Sep 27, 2022
@DrPeterVanNostrand
Copy link
Contributor

DrPeterVanNostrand commented Sep 27, 2022

I totally agree. Using constructors would prevent production proofs from being configured with incorrect setup values via struct literals.

@0xpwnstar
Copy link

/assign
I would like to work on this.

vmx pushed a commit to hanbu97/rust-fil-proofs that referenced this issue Dec 9, 2022
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 vmx closed this as completed in 62ed86f Dec 12, 2022
storojs72 pushed a commit that referenced this issue Jan 11, 2023
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>
storojs72 pushed a commit that referenced this issue Jan 11, 2023
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>
storojs72 added a commit that referenced this issue Jan 19, 2023
* 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>
storojs72 added a commit that referenced this issue Jan 19, 2023
* 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>
storojs72 added a commit that referenced this issue Jan 19, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants