feat: FIP-0061: Add new PoSt proof types #1727

arajasek · 2023-03-24T19:22:58Z

codecov-commenter · 2023-03-24T19:30:57Z

Codecov Report

Merging #1727 (96e7365) into master (37643fc) will decrease coverage by 0.10%.
The diff coverage is 4.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1727      +/-   ##
==========================================
- Coverage   55.62%   55.52%   -0.10%     
==========================================
  Files         146      146              
  Lines       14016    14037      +21     
==========================================
- Hits         7796     7794       -2     
- Misses       6220     6243      +23

Impacted Files	Coverage Δ
shared/src/sector/registered_proof.rs	`0.74% <0.00%> (-0.07%)`	⬇️
shared/src/version/mod.rs	`7.69% <ø> (ø)`
fvm/src/gas/price_list.rs	`32.93% <20.00%> (ø)`
fvm/src/machine/default.rs	`53.12% <100.00%> (ø)`

... and 2 files with indirect coverage changes

ZenGround0

You'll probably want someone who knows ref-fvm a little better but once the typo is fixed this LGTM.

If you have time please explore removing registered_seal_proof or updating it to use current seal proofs. ~~It would also be helpful to get an explanation for why we need nv20 specified here~~.

--edit--
I was a bad person who didn't review tests and now I see why we need nv20

ZenGround0 · 2023-03-24T21:09:49Z

shared/src/sector/registered_proof.rs

@@ -130,19 +145,19 @@ impl RegisteredPoStProof {
    pub fn registered_seal_proof(self) -> Result<RegisteredSealProof, String> {
        use RegisteredPoStProof::*;
        match self {
-            StackedDRGWindow64GiBV1 | StackedDRGWinning64GiBV1 => {
+            StackedDRGWindow64GiBV1P1 | StackedDRGWindow64GiBV1 | StackedDRGWinning64GiBV1 => {


Not sure what's going on here but it seems off that we're not returning V1P1 seal proofs

Can't find any callers in builtin actors, is it possible we can just kill this function?

Opened #1728 for this

ZenGround0 · 2023-03-24T21:11:24Z

shared/src/sector/registered_proof.rs

+            Ok(RegisteredPoStProof::StackedDRGWindow32GiBV1)
+        }
+        RegisteredPoStProof::StackedDRGWindow64GiBV1P1 => {
+            Ok(RegisteredPoStProof::StackedDRGWindow64GiBV1P1)


Typo StackedDRGWindow64GiBV1

E A G L E E Y E S

ZenGround0 · 2023-03-24T21:12:05Z

shared/src/sector/registered_proof.rs

+            Self::StackedDRG32GiBV1 | Self::StackedDRG32GiBV1P1 => Ok(StackedDRGWindow32GiBV1P1),
+            Self::StackedDRG2KiBV1 | Self::StackedDRG2KiBV1P1 => Ok(StackedDRGWindow2KiBV1P1),
+            Self::StackedDRG8MiBV1 | Self::StackedDRG8MiBV1P1 => Ok(StackedDRGWindow8MiBV1P1),
+            Self::StackedDRG512MiBV1 | Self::StackedDRG512MiBV1P1 => Ok(StackedDRGWindow512MiBV1P1),


shared/src/version/mod.rs

Stebalien · 2023-03-27T20:28:01Z

fvm/Cargo.toml

@@ -31,7 +31,7 @@ lazy_static = "1.4.0"
 derive-getters = "0.2.0"
 derive_more = "0.99.17"
 replace_with = "0.1.7"
-filecoin-proofs-api = { version = "13", default-features = false }
+filecoin-proofs-api = { version = "14", default-features = false }


Is this network breaking? I.e., will updating to version 14 cause a hard-fork?

The answer should be no, but I will let @cryptonemo confirm.

The answer is yes, this is nv19 scope and beyond.

To clarify, it's yes once the new PoSt Proof types are used/accepted. If you continue using the old PoSt proof types in the new version of proofs, it's compatible.

Ok, that's all we care about. Adding support for new types is backwards compatible.

Stebalien · 2023-03-27T20:30:59Z

shared/src/sector/registered_proof.rs

@@ -108,6 +108,11 @@ pub enum RegisteredPoStProof {
    StackedDRGWindow512MiBV1,
    StackedDRGWindow32GiBV1,
    StackedDRGWindow64GiBV1,
+    StackedDRGWindow2KiBV1P1,


Nit but... can't we just call these V2?

Stebalien · 2023-03-27T20:32:01Z

shared/src/sector/registered_proof.rs

            Invalid(i) => Err(format!("unsupported proof type: {}", i)),
        }
    }
 }

+/// Convert the v1_1 PoSt Proof type to the older v1 types (used in nv18 and below)
+pub fn convert_window_post_proof_v1p1_to_v1(


Is there any reason to put this here rather in, e.g., the builtin actors? I'd like to shrink the amount of proofs logic in this crate in general.

We can, and honestly we should -- I was following the pattern we have the versioned seal proofs above (hence the V1P1 name too), but I'm quite happy to break the pattern instead.

Ah... Well, if we already have the pattern of versions and are using them in the crypto code... it's probably not worth it?

I'm gonna keep the naming pattern for now, because I think diverging there will be confusing. But I will move the helpers to actors code, because they're only used there, and I don't want anything else depending on it.

Stebalien · 2023-03-27T20:32:07Z

shared/src/sector/registered_proof.rs

+}
+
+/// Convert the v1_1 PoSt Proof type to the older v1 types (used in nv18 and below)
+pub fn is_window_post_proof_v1p1(rpp: RegisteredPoStProof) -> bool {


Ditto above.

0x5459 · 2023-04-23T08:04:32Z

shared/src/sector/registered_proof.rs

+            StackedDRGWindow2KiBV1P1 => Ok(Self::StackedDrgWindow2KiBV1),
+            StackedDRGWindow8MiBV1P1 => Ok(Self::StackedDrgWindow8MiBV1),
+            StackedDRGWindow512MiBV1P1 => Ok(Self::StackedDrgWindow512MiBV1),
+            StackedDRGWindow32GiBV1P1 => Ok(Self::StackedDrgWindow32GiBV1),
+            StackedDRGWindow64GiBV1P1 => Ok(Self::StackedDrgWindow64GiBV1),


Suggested change

StackedDRGWindow2KiBV1P1 => Ok(Self::StackedDrgWindow2KiBV1),

StackedDRGWindow8MiBV1P1 => Ok(Self::StackedDrgWindow8MiBV1),

StackedDRGWindow512MiBV1P1 => Ok(Self::StackedDrgWindow512MiBV1),

StackedDRGWindow32GiBV1P1 => Ok(Self::StackedDrgWindow32GiBV1),

StackedDRGWindow64GiBV1P1 => Ok(Self::StackedDrgWindow64GiBV1),

StackedDRGWindow2KiBV1P1 => Ok(Self::StackedDrgWindow2KiBV1_2),

StackedDRGWindow8MiBV1P1 => Ok(Self::StackedDrgWindow8MiBV1_2),

StackedDRGWindow512MiBV1P1 => Ok(Self::StackedDrgWindow512MiBV1_2),

StackedDRGWindow32GiBV1P1 => Ok(Self::StackedDrgWindow32GiBV1_2),

StackedDRGWindow64GiBV1P1 => Ok(Self::StackedDrgWindow64GiBV1_2),

typo?

arajasek mentioned this pull request Mar 24, 2023

feat: implement FIP-0061 filecoin-project/builtin-actors#1257

Merged

ZenGround0 approved these changes Mar 24, 2023

View reviewed changes

arajasek force-pushed the feat/grindability branch from 96e7365 to 6471127 Compare March 27, 2023 19:59

arajasek mentioned this pull request Mar 27, 2023

feat: update proofs to version 14.0.0 #1725

Closed

Stebalien reviewed Mar 27, 2023

View reviewed changes

arajasek force-pushed the feat/grindability branch from 6471127 to 5936cc5 Compare March 27, 2023 21:20

cryptonemo and others added 2 commits March 28, 2023 11:33

feat: update proofs to version 14.0.0

182c877

feat: FIP-0061: Add new PoSt proof types

4ca30aa

arajasek force-pushed the feat/grindability branch from 5936cc5 to 4ca30aa Compare March 28, 2023 15:34

Stebalien approved these changes Mar 28, 2023

View reviewed changes

Stebalien merged commit 676142e into master Mar 28, 2023

Stebalien deleted the feat/grindability branch March 28, 2023 16:08

0x5459 reviewed Apr 23, 2023

View reviewed changes

0x5459 mentioned this pull request Apr 23, 2023

PoSt failed:invalid post was submitted filecoin-project/lotus#10733

Closed

11 tasks

jennijuju mentioned this pull request Jun 15, 2023

Integrate Synthetic PoRep #1791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: FIP-0061: Add new PoSt proof types #1727

feat: FIP-0061: Add new PoSt proof types #1727

arajasek commented Mar 24, 2023 •

edited

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading

ZenGround0 left a comment •

edited

Loading

ZenGround0 Mar 24, 2023

ZenGround0 Mar 24, 2023

arajasek Mar 27, 2023

ZenGround0 Mar 24, 2023

arajasek Mar 27, 2023

ZenGround0 Mar 24, 2023

Stebalien Mar 27, 2023

arajasek Mar 27, 2023

cryptonemo Mar 28, 2023

cryptonemo Mar 28, 2023

Stebalien Mar 28, 2023

Stebalien Mar 27, 2023

Stebalien Mar 27, 2023

arajasek Mar 27, 2023

Stebalien Mar 27, 2023

arajasek Mar 28, 2023

Stebalien Mar 27, 2023

0x5459 Apr 23, 2023 •

edited

Loading

feat: FIP-0061: Add new PoSt proof types #1727

feat: FIP-0061: Add new PoSt proof types #1727

Conversation

arajasek commented Mar 24, 2023 • edited Loading

codecov-commenter commented Mar 24, 2023 • edited Loading

Codecov Report

ZenGround0 left a comment • edited Loading

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

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

0x5459 Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

arajasek commented Mar 24, 2023 •

edited

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading

ZenGround0 left a comment •

edited

Loading

0x5459 Apr 23, 2023 •

edited

Loading