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

feat: add support for Non-interactive PoRep #453

Closed
wants to merge 7 commits into from
Closed

feat: add support for Non-interactive PoRep #453

wants to merge 7 commits into from

Conversation

vmx
Copy link
Contributor

@vmx vmx commented Apr 23, 2024

This commit adds support for ni-porep.

Also Rust >= 1.70 is needed due to the dependency on home v0.5.9 and fix the corresponding new Clippy warnings.


This is only the Rust side of things, the Go parts are missing.

@cryptonemo
Copy link
Contributor

This needs to be rebased on top of #450, not master

@vmx vmx changed the base branch from master to improved-error-handling April 23, 2024 19:13
@vmx
Copy link
Contributor Author

vmx commented Apr 23, 2024

Also rebased on your branch.

@cryptonemo cryptonemo force-pushed the improved-error-handling branch from 98638f2 to 80ff911 Compare April 25, 2024 19:36
Base automatically changed from improved-error-handling to master April 29, 2024 17:51
@vmx
Copy link
Contributor Author

vmx commented May 7, 2024

I've rebased this PR and also adapted it to the fixed names.

@cryptonemo
Copy link
Contributor

I think this needs to target feat/nv23 and not master. Sound correct, @rjan90?

@rjan90
Copy link
Contributor

rjan90 commented May 7, 2024

I think this needs to target feat/nv23 and not master. Sound correct, @rjan90?

Yeah, that is correct. That branch was based off master today, so it should not cause any issues.

@vmx vmx changed the base branch from master to feat/nv23 May 7, 2024 15:33
@vmx
Copy link
Contributor Author

vmx commented May 7, 2024

I've changed the base to feat/nv23.

@cryptonemo cryptonemo requested a review from rjan90 May 20, 2024 16:37
rust/src/proofs/types.rs Outdated Show resolved Hide resolved
rust/src/proofs/types.rs Outdated Show resolved Hide resolved
@rvagg rvagg mentioned this pull request May 27, 2024
@BigLep
Copy link
Member

BigLep commented Jun 11, 2024

Just going through and looking at nv23-related open items. A few questions:

  1. Who should we set the assignee to?
  2. What is remaining?
  3. Who is expected to do the reviewing?
  4. Have we looked into the CI failures?

@BigLep BigLep mentioned this pull request Jun 11, 2024
Make CI pass again.

This reverts commit 896d156.
@vmx
Copy link
Contributor Author

vmx commented Jun 12, 2024

  1. Who should we set the assignee to?

In the past did the FilCrypto team the Rust side of things and then the Lotus team added the Go parts and merged it.

2. What is remaining?

The Go parts which are done at #456

3. Who is expected to do the reviewing?

For the Rust side of things, Elliptic Research, for the Go side I'd guess FilOz.

4. Have we looked into the CI failures?

I've pushed a new commit which should get things green.

@vmx
Copy link
Contributor Author

vmx commented Jun 12, 2024

I should've added that in the past the Go side (Lotus team) owned the merging of things of the FFI, the FilCrypto team was just contributing to the Rust side of things (@cryptonemo please correct me if I'm wrong).

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this all seems good to me

although, I think we need to update feat/nv23 with the circle config for macos builders so we can get rid of it from this PR @rjan90

@vmx I think you could pull in the Rust changes from #456 into this PR, I included seal_commit_phase2_circuit_proofs there but it seems more appropriate coming from you.

@BigLep
Copy link
Member

BigLep commented Jun 13, 2024

@vmx : thanks for the explanation on how the filecoin-ffi work has been traditionally handled. I expect some of that may need to be reevaluated or at least documented in light of groups like FilOz and Elliptic Research being separate companies now, but that can be thought through and handled later.

For now, given this just has the rust changes and Elliptic Research is the owner of those, I've assigned this issue to you. Please go ahead and merge when you have approval (✅ here) and all the rust changes are included. (It sounds like per Rod there is a small change to bring over.)

@rjan90 rjan90 changed the base branch from feat/nv23 to master June 13, 2024 09:32
vmx added a commit that referenced this pull request Jun 13, 2024
This commit adds support for ni-porep.

It's a combination of the indivdual PRs #453, #456 and #458. Thanks everyone
involved working on those.

The changes are for the Go side (actors and FVM) as well as the Rust side
(proofs API).

Changes on the CI are:

 - Makes it match the Rust version in the rust-toolchain.toml
 - Newer Go version (1.21) on all platforms
 - Larger instance for the testsas the Ni-PoRep synthesis phase takes
   more resources.

Closes #453, #456, #458.
vmx added a commit that referenced this pull request Jun 13, 2024
This commit adds support for ni-porep.

It's a combination of the indivdual PRs #453, #456 and #458. Thanks everyone
involved working on those.

The changes are for the Go side (actors and FVM) as well as the Rust side
(proofs API).

Changes on the CI are:

 - Makes it match the Rust version in the rust-toolchain.toml
 - Newer Go version (1.21) on all platforms
 - Larger instance for the testsas the Ni-PoRep synthesis phase takes
   more resources.

Closes #453, #456, #458.
@BigLep
Copy link
Member

BigLep commented Jun 13, 2024

I'm closing given superseded by #459

@BigLep BigLep closed this Jun 13, 2024
vmx added a commit that referenced this pull request Jun 18, 2024
* feat: add support for Non-interactive PoRep

This commit adds support for ni-porep.

It's a combination of the indivdual PRs #453, #456 and #458. Thanks everyone
involved working on those.

The changes are for the Go side (actors and FVM) as well as the Rust side
(proofs API).

Changes on the CI are:

 - Makes it match the Rust version in the rust-toolchain.toml
 - Newer Go version (1.21) on all platforms
 - Larger instance for the testsas the Ni-PoRep synthesis phase takes
   more resources.

Closes #453, #456, #458.

---------

Co-authored-by: Steven Allen <steven@stebalien.com>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: nemo <nemo@ellipticresearch.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants