feat: proof upgrade testing #1284

cryptonemo · 2020-09-21T20:41:52Z

This PR is NOT intended to be merged, as it contains incompatible proofs behaviour. At the moment, this branch is for testing.

porcuquine · 2020-09-29T02:07:03Z

filecoin-proofs/src/api/post.rs

-    }
+            })
+        })
+        .collect::<Result<_>>()?;


I think the problem with this change is that it assumed each sector could only appear once in the set of challenged sectors. But in the case of Winning PoSt, there is just one sector, yet it is challenged repeatedly (each time with only a single challenge).

Yes, this is strange and confusing. Yes, the pervasive naming makes this almost impossible to understand.

porcuquine · 2020-09-29T02:08:19Z

filecoin-proofs/src/api/post.rs

@@ -452,6 +452,7 @@ pub fn generate_fallback_sector_challenges<Tree: 'static + MerkleTreeTrait>(
    randomness: &ChallengeSeed,
    pub_sectors: &[SectorId],
    _prover_id: ProverId,
+    is_winning: bool,


If we want to avoid completely splitting Window and Winning PoSt, we need to be able to signal which we are using. We could try to 'detect' somehow based on the actual values, but that would be very brittle and a bad path to follow.

porcuquine · 2020-09-29T02:10:02Z

filecoin-proofs/src/api/post.rs

-        .collect::<Result<_>>()?;
+            });
+        }
+    }


We need to roll this back. Winning PoSt will not actually change its behavior. We need the challenges of Winning PoSt to be sensitive to order, because otherwise they would all be the same. This is because a single challenge appears repeatedly at different positions within the list of challenged sectors. Each instance needs fresh challenges.

porcuquine · 2020-09-29T02:13:12Z

storage-proofs/post/src/fallback/compound.rs

+                        i
+                    } else {
+                        challenge_index
+                    } as u64,


This works, but it's a hack which now depends on each Winning PoSt sector being challenged only once. To be more correct, the true branch of this conditional should fully restore previous behavior. In practice, this won't matter with current parameterization, but we shouldn't depend on that. Certainly we would need at least an assertion if we intended to. That might actually be preferable. We have an enormous amount of complexity in place to support a generality I don't think we need. If we can close off some possibilities for the sake of simpler, clearer, code that might be worth doing.

porcuquine · 2020-09-29T02:14:25Z

storage-proofs/post/src/fallback/compound.rs

@@ -234,6 +238,7 @@ mod tests {
                sector_size: sector_size as u64,
                challenge_count,
                sector_count,
+                is_winning: false,


For the sake of getting this working quickly, I ended up specializing all the tests to Window PoSt (is_winning: false). To be thorough, they should probably be extended to exercise both code paths.

filecoin-proofs/tests/api.rs

storage-proofs/post/src/fallback/vanilla.rs

storage-proofs/post/src/fallback/compound.rs

porcuquine · 2020-09-30T19:08:06Z

storage-proofs/post/src/fallback/vanilla.rs

-                });
+                // Winning PoSt and Window PoSt have different proof shapes regarding inclusion
+                // and sector proofs, so we are careful to partition them appropriately.
+                if pub_params.is_winning {


My experience of reading this whole section now is that it feels obfuscated and is very hard to follow. I am starting to suspect it would be better to just select on type (is_winning) then clearly and concisely perform the appropriate work in each branch. Treating these as 'same but different' with all the conditionals, hidden assumptions, and gymnastics doesn't seem worth it. If nothing else, it makes the code hard to follow — and it also makes it hard to reason about well enough to gain confidence. I think that goal might be more achievable with the alternate structure I just proposed. What do you think?

It's a good idea for sure -- but the reason I'd hold off is because we already do something like this here (arguably the 'improved' version, or at least inspired by the idea): https://github.com/filecoin-project/rust-fil-proofs/blob/master/filecoin-proofs/src/api/post.rs#L672 and ideally, as part of a larger re-factor, we can share code across as needed better (rather than duplicating).

Do you want to do that now then? My concern is trying to avoid accidentally releasing broken code. I'm not saying I believe this code is necessarily broken, but if we let the mandate of an upgrade complicate the code base, then we're at risk of making it very hard to continue maintaining in case further change becomes required.

I would personally prefer to maximize simplicity, even at the cost of some 'duplication' which can be refactored away later. Or do the refactor now, if not too difficult. The only reason the code isn't duplicated now is because we have two different methods, one of which is very hard to comprehend.

porcuquine · 2020-10-20T21:09:46Z

@cryptonemo I think my previous commit is ready for review. This is about all the refactoring that feels needed for now (apart from whatever is logically implied that I might have overlooked).

dignifiedquire · 2020-11-06T14:12:43Z

fil-proofs-tooling/src/bin/benchy/prodbench.rs

@@ -313,18 +317,18 @@ fn generate_params(i: &ProdbenchInputs) {
        sector_size,
        partitions,
        porep_id: dummy_porep_id,
+        api_version: FIXED_API_VERSION,


this should probably a parameter of the tooling

dignifiedquire · 2020-11-06T14:12:57Z

fil-proofs-tooling/src/bin/benchy/window_post.rs

@@ -514,6 +516,7 @@ pub fn run_window_post_bench<Tree: 'static + MerkleTreeTrait>(
            .expect("unknown sector size"),
        typ: PoStType::Window,
        priority: true,
+        api_version: APIVersion::V1_1,


should probably a parameter

dignifiedquire · 2020-11-09T15:46:23Z

storage-proofs/core/src/api_version.rs

+
+impl FromStr for APIVersion {
+    type Err = anyhow::Error;
+    fn from_str(api_version: &str) -> Result<Self> {


Lets associate a Semver version with each of the enum and use that for parsing using: https://docs.rs/semver/

For this, do we want ApiVersion::{V1_0_0, V1_1_0} for the constants also?

good question, I am not sure tbh

dignifiedquire · 2020-11-09T15:47:11Z

storage-proofs/core/src/api_version.rs

@@ -1,5 +1,21 @@
+use anyhow::{self, Result};
+use std::str::FromStr;
+
 #[derive(Debug, Copy, Clone)]
 pub enum APIVersion {


Lets call this ApiVersion, it is a little bit more consistent with naming conventions in rust

feat: add tests paths for both v1_0 and v1_1 versions

cryptonemo force-pushed the upgrade-testing branch from a27661c to 439dc81 Compare September 28, 2020 12:10

porcuquine reviewed Sep 29, 2020

View reviewed changes

cryptonemo force-pushed the upgrade-testing branch from 1a3b591 to 0fc5c7c Compare September 29, 2020 13:21

cryptonemo changed the title ~~feat: proof upgrade testing (winning PoSt currently broken)~~ feat: proof upgrade testing Sep 29, 2020

porcuquine reviewed Sep 30, 2020

View reviewed changes

filecoin-proofs/tests/api.rs Outdated Show resolved Hide resolved

porcuquine reviewed Sep 30, 2020

View reviewed changes

storage-proofs/post/src/fallback/vanilla.rs Outdated Show resolved Hide resolved

porcuquine reviewed Sep 30, 2020

View reviewed changes

storage-proofs/post/src/fallback/compound.rs Outdated Show resolved Hide resolved

porcuquine reviewed Sep 30, 2020

View reviewed changes

cryptonemo force-pushed the upgrade-testing branch 2 times, most recently from 218b615 to cf50b17 Compare October 7, 2020 14:55

porcuquine mentioned this pull request Oct 15, 2020

a sanity check failed of window post haven't catch. #1308

Closed

porcuquine force-pushed the upgrade-testing branch 4 times, most recently from 69eb6ed to ea56e52 Compare October 18, 2020 22:35

porcuquine force-pushed the upgrade-testing branch from ea56e52 to ca6d31a Compare October 20, 2020 23:01

cryptonemo force-pushed the upgrade-testing branch 3 times, most recently from 910651e to 27a5563 Compare October 27, 2020 18:12

cryptonemo force-pushed the upgrade-testing branch from 27a5563 to 750f845 Compare November 3, 2020 18:37

dignifiedquire reviewed Nov 6, 2020

View reviewed changes

cryptonemo force-pushed the upgrade-testing branch from 750f845 to 10d247a Compare November 9, 2020 14:15

dignifiedquire reviewed Nov 9, 2020

View reviewed changes

cryptonemo and others added 4 commits November 9, 2020 12:58

feat: proof upgrade testing (winning PoSt currently broken)

1723be1

Fix winning PoSt.

cd0dffb

feat: parameterize tests (although they fail if enabled)

adca330

fix: remove incorrectly parameterized test

576f762

cryptonemo and others added 7 commits November 9, 2020 12:58

fix: add back in generate_winning_post code path for testing

4ada221

fix: apply challenge change to legacy winning post API

fe33bbd

fix: apply some review feedback

5a24fb7

Refactor using PoStShape.

48c0a5f

feat: add api version enum for determining runtime behaviour

b275241

feat: add tests paths for both v1_0 and v1_1 versions

feat: allow api_version to be used as cli argument

38a40f3

fix: renamed ApiVersion and use semver string parsing

43dad7f

cryptonemo force-pushed the upgrade-testing branch from faf72ff to 43dad7f Compare November 9, 2020 17:59

style: rust fmt

27a2aa4

cryptonemo mentioned this pull request Nov 9, 2020

Re-factor graph fix / upgrade #1357

Closed

dignifiedquire closed this Nov 24, 2020

cryptonemo mentioned this pull request Mar 24, 2021

Window PoSt challenge generation should not depend on sector order #1270

Closed

porcuquine deleted the upgrade-testing branch September 16, 2021 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: proof upgrade testing #1284

feat: proof upgrade testing #1284

cryptonemo commented Sep 21, 2020

porcuquine Sep 29, 2020

porcuquine Sep 29, 2020

porcuquine Sep 29, 2020

porcuquine Sep 29, 2020

porcuquine Sep 29, 2020

porcuquine Sep 30, 2020

cryptonemo Sep 30, 2020

porcuquine Sep 30, 2020

porcuquine commented Oct 20, 2020

dignifiedquire Nov 6, 2020

dignifiedquire Nov 6, 2020

dignifiedquire Nov 9, 2020

cryptonemo Nov 9, 2020

dignifiedquire Nov 9, 2020

dignifiedquire Nov 9, 2020

feat: proof upgrade testing #1284

feat: proof upgrade testing #1284

Conversation

cryptonemo commented Sep 21, 2020

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

porcuquine commented Oct 20, 2020

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