Update dependencies and Proof types for Synthetic PoRep support #1811

cryptonemo · 2023-07-07T13:07:43Z

No description provided.

codecov-commenter · 2023-07-07T13:18:21Z

Codecov Report

Merging #1811 (eb90e48) into master (8f33aa6) will decrease coverage by 0.11%.
The diff coverage is 2.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1811      +/-   ##
==========================================
- Coverage   75.20%   75.10%   -0.11%     
==========================================
  Files         149      149              
  Lines       14593    14615      +22     
==========================================
+ Hits        10975    10976       +1     
- Misses       3618     3639      +21

Impacted Files	Coverage Δ
fvm/src/gas/price_list.rs	`78.30% <ø> (ø)`
shared/src/sector/registered_proof.rs	`6.20% <2.43%> (-0.36%)`	⬇️

... and 8 files with indirect coverage changes

Stebalien · 2023-07-07T14:46:16Z

Is this work ready to land? If not, it may make sense to update the proofs APIs and bls signatures packages first before actually adding support for synthetic porep.

cryptonemo · 2023-07-07T15:41:00Z

My understanding is that an end to end is needed for testing well before this lands, and that it's needed by next week. I was trying to wire all of the deps together in order for it to happen so that it could be taken off my hands for that testing before actually cutting releases across the board.

This is tricky because it seems I'm hitting some deps that are also right around release boundaries.

Cc: @snadrus @rjan90

cryptonemo · 2023-07-07T15:43:12Z

Is this work ready to land? If not, it may make sense to update the proofs APIs and bls signatures packages first before actually adding support for synthetic porep.

As it is, I have branches called synthetic-porep across the board (across all deps)

arajasek

Generally looks good, a few notes / qs.

arajasek · 2023-07-11T17:55:38Z

fvm/src/gas/price_list.rs

@@ -139,6 +139,14 @@ lazy_static! {
            (
                RegisteredSealProof::StackedDRG64GiBV1P1,
                Gas::new(359272)
+            ),
+            (
+                RegisteredSealProof::StackedDRG32GiBV1P1_Feat_SyntheticPoRep,


This seems reasonable to me, but have we discussed whether or not these values need to change somewhere?

No, we haven't. Fundamentally, the PoRep doesn't change, so I think it should be ok to use the existing as the model -- but that's a bit out of my wheelhouse too

arajasek · 2023-07-11T17:58:04Z

shared/src/sector/registered_proof.rs

@@ -50,7 +57,16 @@ impl RegisteredSealProof {
                SectorSize::_32GiB => Self::StackedDRG32GiBV1P1,
                SectorSize::_64GiB => Self::StackedDRG64GiBV1P1,
            }
-        }
+        } /*


Hmm, this doesn't seem right. There is no-longer a 1-1 mapping from sector size to seal proof type, because both "regular" and SyntheticPoRep types are supported.

Does anything actually use this? I suspect we can delete it.

arajasek · 2023-07-11T17:58:43Z

shared/src/sector/registered_proof.rs

@@ -65,6 +81,18 @@ impl RegisteredSealProof {
        };
    }

+    /// Convert the original proof type to the v1 SyntheticPoRep proof added in network version ??.
+    pub fn update_to_v1_feat_synthetic_porep(&mut self) {


Also not sure that we need this.

Agreed, a lot of the conversions are starting to not make sense anymore, but it's not clear to me what's used and what isn't.

So, I hate to be a stickler but... can we try to better understand what's happening here? The fact that we appear to be making changes without really understanding if we need them is concerning.

arajasek · 2023-07-11T17:59:17Z

shared/src/sector/registered_proof.rs

+            | StackedDRG8MiBV1P1
+            | StackedDRG2KiBV1P1_Feat_SyntheticPoRep
+            | StackedDRG512MiBV1P1_Feat_SyntheticPoRep
+            | StackedDRG8MiBV1P1_Feat_SyntheticPoRep => Ok(192),


You know better than I do, but just want a 👍 that these values are unchanged for SynthPoRep

Yes, they are unchanged -- thanks for noting!

arajasek · 2023-07-11T18:00:53Z

shared/src/sector/registered_proof.rs

@@ -218,11 +272,21 @@ impl RegisteredSealProof {
    pub fn registered_winning_post_proof(self) -> Result<RegisteredPoStProof, String> {


Seems correct, but we might be able to delete this method too? I'd really like to get this kinda logic out of the FVM

cryptonemo · 2023-09-05T18:22:39Z

Is this work ready to land? If not, it may make sense to update the proofs APIs and bls signatures packages first before actually adding support for synthetic porep.

@Stebalien This is all finally ready and pointing to the latest published releases. Looks like we'll need an fvm3 release for the FFI PR (filecoin-project/filecoin-ffi#400)

vmx · 2023-09-05T18:33:24Z

I need to push one more commit, give me a sec.

In `filecoin-proofs-api` are the `cuda` and `cuda-supraseal` features mutally exclusive. Hence add this feature to fvm, so that it can be enabled on the `filecoin-ffi` level.

vmx · 2023-09-05T20:34:29Z

Sorry for the delay. It turned out that fvm2 caused some troubles within filecoin-ffi, but I sorted all out with patch releases. This PR is now ready to be merged.

Also if anyone finds the time to do a release, that would be great as this is the only thing that prevents us from cutting a new filecoin-ffi release.

Stebalien · 2023-09-05T20:54:42Z

We'll need both an FVMv3 and an FVMv2 release. Can we put together a quick PR for FVMv2 that updates these deps?

vmx · 2023-09-05T21:00:07Z

Which dependencies need to be updated for FVMv2? is it the release/v2 branch? filecoin-ffi works without any updates there, but it sounds like i miss something.

If we need updates, then I'd update blst and bls-signatures to the latest version, that's cleaner than the patch releases I made to get FVMv2 work.

Stebalien · 2023-09-05T22:40:30Z

I think we at least need blst updated due to linking issues. I generally just update everything to reduce the size of the dependency tree.

cryptonemo · 2023-09-05T22:56:23Z

I think we at least need blst updated due to linking issues. I generally just update everything to reduce the size of the dependency tree.

I took a stab at this here: #1875

Stebalien · 2023-09-05T23:29:16Z

shared/src/sector/registered_proof.rs

@@ -25,6 +26,12 @@ pub enum RegisteredSealProof {
    StackedDRG8MiBV1P1,
    StackedDRG32GiBV1P1,
    StackedDRG64GiBV1P1,
+
+    StackedDRG2KiBV1P1_Feat_SyntheticPoRep,


Ok.... why _Feat_SyntheticPoRep. Can we not just assign a new version?

Like.... we had V1. Then we tacked on a P1 instead of bumping it to V2. Now we're adding _Feat_SyntheticPoRep instead of just bumping it to P2 or V2 or something???

The PoRep proof version isn't changing, it's an optional feature that's been added -- and that's the naming convention we chose for optional features that are enabled by new proof types.

Where do we actually use this? In the builtin actors, it looks like we don't care.

Hm. Let's continue on slack and schedule a time tomorrow to talk about this. I think we can land this PR quickly, but doing this async will take too long.

Stebalien

Reviewed and discussed out of band. We need to deduplicate some of these enums/constants, but that's a later thing.

cryptonemo requested review from Stebalien and ZenGround0 July 7, 2023 13:07

cryptonemo mentioned this pull request Jul 7, 2023

Updates dependencies and tests for Synthetic PoRep support filecoin-project/builtin-actors#1331

Closed

arajasek reviewed Jul 11, 2023

View reviewed changes

magik6k mentioned this pull request Jul 13, 2023

feat: introduce SyntheticPoRep filecoin-project/lotus#11057

Closed

13 tasks

cryptonemo force-pushed the synthetic-porep branch 2 times, most recently from 3d09b11 to 6d11002 Compare August 30, 2023 14:15

Add support for Synthetic PoRep

700fef1

cryptonemo force-pushed the synthetic-porep branch from 6d11002 to 700fef1 Compare September 5, 2023 13:00

cryptonemo added 3 commits September 5, 2023 14:02

feat: update to the latest proofs version

b2c1f71

style: remove commented dependency

1b22161

style: allow clippy warning

6d94b2c

feat: add cuda-supraseal feature

ec4e9f9

In `filecoin-proofs-api` are the `cuda` and `cuda-supraseal` features mutally exclusive. Hence add this feature to fvm, so that it can be enabled on the `filecoin-ffi` level.

vmx force-pushed the synthetic-porep branch from be54e52 to ec4e9f9 Compare September 5, 2023 18:40

Stebalien reviewed Sep 5, 2023

View reviewed changes

Stebalien approved these changes Sep 6, 2023

View reviewed changes

Stebalien merged commit da5d0e3 into master Sep 6, 2023
14 checks passed

Stebalien deleted the synthetic-porep branch September 6, 2023 17:30

rjan90 mentioned this pull request Mar 26, 2024

Implement FIP-0090: Non-Interactive PoRep filecoin-project/builtin-actors#1534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dependencies and Proof types for Synthetic PoRep support #1811

Update dependencies and Proof types for Synthetic PoRep support #1811

cryptonemo commented Jul 7, 2023

codecov-commenter commented Jul 7, 2023 •

edited

Loading

Stebalien commented Jul 7, 2023

cryptonemo commented Jul 7, 2023 •

edited

Loading

cryptonemo commented Jul 7, 2023

arajasek left a comment

arajasek Jul 11, 2023

cryptonemo Jul 18, 2023

arajasek Jul 11, 2023

arajasek Jul 11, 2023

cryptonemo Jul 18, 2023

Stebalien Sep 5, 2023

arajasek Jul 11, 2023

cryptonemo Jul 18, 2023

arajasek Jul 11, 2023

cryptonemo Jul 18, 2023

cryptonemo commented Sep 5, 2023

vmx commented Sep 5, 2023

vmx commented Sep 5, 2023

Stebalien commented Sep 5, 2023

vmx commented Sep 5, 2023

Stebalien commented Sep 5, 2023

cryptonemo commented Sep 5, 2023

Stebalien Sep 5, 2023

cryptonemo Sep 5, 2023

Stebalien Sep 5, 2023

Stebalien Sep 5, 2023

Stebalien left a comment

		@@ -218,11 +272,21 @@ impl RegisteredSealProof {
		pub fn registered_winning_post_proof(self) -> Result<RegisteredPoStProof, String> {

Update dependencies and Proof types for Synthetic PoRep support #1811

Update dependencies and Proof types for Synthetic PoRep support #1811

Conversation

cryptonemo commented Jul 7, 2023

codecov-commenter commented Jul 7, 2023 • edited Loading

Codecov Report

Stebalien commented Jul 7, 2023

cryptonemo commented Jul 7, 2023 • edited Loading

cryptonemo commented Jul 7, 2023

arajasek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptonemo commented Sep 5, 2023

vmx commented Sep 5, 2023

vmx commented Sep 5, 2023

Stebalien commented Sep 5, 2023

vmx commented Sep 5, 2023

Stebalien commented Sep 5, 2023

cryptonemo commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 7, 2023 •

edited

Loading

cryptonemo commented Jul 7, 2023 •

edited

Loading