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

Add proof types for synthetic porep (FIP-0059) #1409

Merged
merged 8 commits into from
Sep 14, 2023
Merged

Conversation

cryptonemo
Copy link
Contributor

@Stebalien Does this help as an alternative to #1335?

@cryptonemo
Copy link
Contributor Author

Alternatively, after your branch is merged, I could rebase against master. Let me know what makes sense.

@Stebalien
Copy link
Member

I was going to do it in two PRs.

@cryptonemo
Copy link
Contributor Author

Ok, that works, thanks

Base automatically changed from chore/update-fvm to master September 7, 2023 20:39
@Stebalien
Copy link
Member

This looks correct, but I think this'll need to be tested in a devnet before we can actually be sure we haven't missed something.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #1409 (81b39e5) into master (2b1df06) will decrease coverage by 0.07%.
The diff coverage is 89.47%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1409      +/-   ##
==========================================
- Coverage   91.07%   91.01%   -0.07%     
==========================================
  Files         145      145              
  Lines       27397    27409      +12     
==========================================
- Hits        24951    24945       -6     
- Misses       2446     2464      +18     
Files Changed Coverage Δ
actors/miner/src/commd.rs 82.85% <60.00%> (ø)
actors/miner/src/policy.rs 92.00% <100.00%> (ø)
runtime/src/runtime/policy.rs 98.51% <100.00%> (+0.14%) ⬆️

... and 3 files with indirect coverage changes

@@ -502,7 +505,8 @@ mod miner_actor_test_commitment {
),
);
rt.reset();
precommit_params.seal_proof = RegisteredSealProof::StackedDRG32GiBV1P1;
precommit_params.seal_proof =
RegisteredSealProof::StackedDRG32GiBV1P1_Feat_SyntheticPoRep;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to change any existing tests here. Possibly we need some new tests, but if this value is just plumbed directly to the syscall, we're probably ok without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! If we do end up needing a separate test, let's figure out the best way to do that.

actors/miner/tests/util.rs Outdated Show resolved Hide resolved
integration_tests/src/tests/commit_post_test.rs Outdated Show resolved Hide resolved
@anorth
Copy link
Member

anorth commented Sep 7, 2023

@Stebalien everything needs testing in devnet, right? I don't think that should hold us off from merging code towards FIP-approved changes into master. Integration will be difficult enough regardless.

@Stebalien
Copy link
Member

Yeah, that's fine by me. I'd say that this change is especially hard to get confidence on without that, but there's no harm in merging and testing later.

@rjan90
Copy link
Contributor

rjan90 commented Sep 8, 2023

Fwiw, we have done a couple of rounds of testing (local + butterfly) with a locally built actors bundle based of https://github.com/filecoin-project/builtin-actors/tree/dev/synth-porep-1 in Lotus.

Plan on the Lotus side is to do a round of SynthPoRep-regression test in a devnet, and also reset butterfly next week-ish. It would be really useful to have these changes in "tag/release", so we can get a bundle as an artifact, instead of building it locally.

Stebalien and others added 7 commits September 8, 2023 12:53
We now need to explicitly specify the sha3/ripemd features when
testing (it was removed from shared).
feat: updates to use latest fvm releases
@cryptonemo
Copy link
Contributor Author

Rebased against master

@anorth anorth changed the title Synthetic porep Add proof types for synthetic porep (FIP-0059) Sep 10, 2023
@anorth
Copy link
Member

anorth commented Sep 10, 2023

@Stebalien this LGTM, could you please merge after verifying for yourself?

@Stebalien
Copy link
Member

@rjan90's testing is enough for me and I don't have a setup where I could verify this.

Merged via the queue into master with commit 66bb5f7 Sep 14, 2023
@Stebalien Stebalien deleted the synthetic-porep2 branch September 14, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants