feat(storage-proofs): implement rational post #763

dignifiedquire · 2019-07-29T15:50:43Z

Follows the definition in filecoin-project/specs#332

PRs for other repos

storage-proofs/src/circuit/rational_post.rs

storage-proofs/src/rational_post.rs

laser

BLOCKING: This PR needs to generate and publish new Groth parameters.

Note: Breaking API Changes

generate_post challenge_seed parameter becomes &ChallengeSeed
generate_post now accepts &[u64] for faults
generate_post has new return type
verify_post now accepts &[u8] instead of Vec<Vec<u8>> for proofs
verify_post challenge_seed parameter becomes &ChallengeSeed
verify_post now accepts &[u64] for faults
SectorClass drops PoStProofPartitions

laser · 2019-08-21T20:33:18Z

filecoin-proofs/src/api.rs

+
+    let commitments: Vec<_> = challenges
+        .iter()
+        .map(|c| commitments_all[c.sector as usize])


BLOCKING: Please use Vec::get and early-return on a None instead of indexing into the thing directly (which will panic if there is no value corresponding to the provided index).

Counterpoint: derive_challenges had damn well better make sure c.sector is in range, and if it's not then we risk generating an invalid PoSt and need the moral equivalent of a panic (although I agree that should be caught and handled gracefully at the API boundary).

We don't have any panic-recovery tooling yet, so this will unwind the stack and crash the Filecoin node. Until we build those tools, please avoid using panics.

filecoin-proofs/src/api.rs

storage-proofs/src/rational_post.rs

filecoin-proofs/src/api.rs

porcuquine

Good stuff, @dignifiedquire. Much simpler now…

filecoin-proofs/src/api.rs

porcuquine · 2019-08-22T07:27:03Z

filecoin-proofs/src/api.rs

+
+    let commitments: Vec<_> = challenges
+        .iter()
+        .map(|c| commitments_all[c.sector as usize])


Counterpoint: derive_challenges had damn well better make sure c.sector is in range, and if it's not then we risk generating an invalid PoSt and need the moral equivalent of a panic (although I agree that should be caught and handled gracefully at the API boundary).

porcuquine · 2019-08-22T07:43:41Z

filecoin-proofs/src/api.rs

+                )
+                .unwrap()
+            } else {
+                panic!("faults are not yet supported")


I think we should give ReplicaInfo (or whatever you call it) a method which can convert it into a tree. We should pass those as private inputs, then only treeify them if the sector is challenged. This assumes there will be enough time to do that. This is a whole question I'm trying to get answers to now and will create an issue if necessary.

In any case, we can't hold all the trees in memory (and I think go-filecoin) is currently blowing up because of this.

filecoin-proofs/src/api.rs

porcuquine · 2019-08-22T08:56:29Z

storage-proofs/src/circuit/multi_proof.rs

+
+    pub fn to_vec(&self) -> Vec<u8> {
+        let mut out = Vec::new();
+        self.write(&mut out).expect("known allocation target");


Why do you defile the holy MultiProof so?

dignifiedquire · 2019-08-23T11:03:49Z

BLOCKING: This PR needs to generate and publish new Groth parameters.

yes, will do once this PR is fully settled and approved

filecoin-proofs/src/api.rs

laser · 2019-08-23T20:44:19Z

filecoin-proofs/src/api.rs

+    let challenged_replicas: Vec<_> = challenges
+        .iter()
+        .map(|c| {
+            if let Some(replica) = replicas.get(c.sector as usize) {


BLOCKING: replicas.get(c.sector as usize) is going to index into replicas by sector id. replicas is a Vec (i.e. not sector id-indexed data structure).

How do I get the sector id from the the replica then?

You'll need to extend ReplicaInfo to include this information, e.g.

#[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct ReplicaInfo { /// Path to the replica. access: String, /// The replica commitment. commitment: Commitment, sector_id: SectorId }

We should pause this thread until we're all on the same page regarding whether or not GeneratePoSt should accept a SectorSet (a sector id-indexed map) or an ordered list of sector ids.

laser · 2019-08-26T14:56:14Z

storage-proofs/src/sector.rs

+use byteorder::ByteOrder;
+
+/// An ordered set of `SectorId`s.
+pub type SectorSet = BTreeSet<SectorId>;


Suggestion: Change the name of this type to something other than SectorSet. SectorIdSet would be fine by me.

Rationale: The protocol already specifies SectorSet to be a map from sector id to replica commitment. Your type is not a map from sector id to replica commitment; I am certain that the name collision will cause some confusion in the future.

changed to OrderedSectorSet

storage-proofs/src/rational_post.rs

filecoin-proofs/src/api.rs

porcuquine

BLOCKING: Please look into my questions about apparently conflating SectorId with offset, even after changes to disentangle them. I made comments in a number of places to highlight the issue, and they should be taken as a group.

porcuquine · 2019-08-26T16:22:47Z

storage-proofs/src/rational_post.rs

+                let challenged_sector = u64::from(challenge.sector);
+                let challenged_leaf = challenge.leaf;
+
+                let tree = priv_inputs.trees[challenged_sector as usize];


challenged_sector, which is the challenge.sector, seems to be used as an index into the trees here, but the type of challenge.Sector is SectorId. This seems like a mismatch.

storage-proofs/src/rational_post.rs

porcuquine · 2019-08-26T16:25:14Z

storage-proofs/src/rational_post.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Challenge {
+    // The index of the sector into the sector list.
+    pub sector: SectorId,


It it the index, or is it the Id?

ID, forgot to update the comment

storage-proofs/src/rational_post.rs

porcuquine · 2019-08-26T16:29:55Z

storage-proofs/src/rational_post.rs

+            .iter()
+            .zip(pub_inputs.commitments.iter())
+            .map(|(challenge, commitment)| {
+                let challenged_sector = u64::from(challenge.sector);


Why are you turning a SectorId into a u64? It seems like this is defeating the type system and allowing you to look up a tree in an array by the integer value of its SectorId.

porcuquine · 2019-08-26T16:40:33Z

storage-proofs/src/rational_post.rs

+        faults.insert(1.into());
+        let mut sectors = SectorSet::new();
+        sectors.insert(0.into());
+        sectors.insert(1.into());


I think this test needs to be made more robust. It's not catching the fact that we're indexing into the trees using the SectorId and not the calculated challenge offset.

I believe the test will break and expose this if you change line 297 to read sectors.insert(2.into());.

We should also go further and add at least one more non-faulted sector and ensure that the test specifically validates that it has been selected correctly. In generally, we should avoid only inserting sectors whose SectorIds are consecutive, starting from 0 — since this is very special case in which SectorId and offset into an ordered set happen to be identical.

filecoin-proofs/src/api/post.rs

…spec

* feat(add SectorId type and transforms) * fixup(clippy) * feat(implement Display for SectorId) * fixup(derive Ord) * fixup(revert sector_id field change to sector as per spec)

filecoin-proofs/src/api/post.rs

storage-proofs/src/rational_post.rs

laser

Nice work, but your tests are busted.

porcuquine

LGTM

Some non-blocking thoughts to consider for future iterations (or not).

porcuquine · 2019-08-27T18:53:30Z

filecoin-proofs/src/api/post.rs

+    /// The replica commitment.
+    commitment: Commitment,
+    /// Is this sector marked as a fault?
+    is_fault: bool,


Not necessary, but it might be nice to make this whole type be an Enum with faulty and non-faulty variants — the faulty version omitting the access. This would make it impossible to generate merkle trees from faulty sectors, potentially heading off some unpleasant failure modes we might accidentally stumble into if it's possible to do so.

porcuquine · 2019-08-27T19:40:40Z

storage-proofs/src/rational_post.rs

+
+                if let Some(tree) = priv_inputs.trees.get(&challenge.sector) {
+                    if commitment != &tree.root() {
+                        return Err(Error::InvalidCommitment);


Not sure this is the right name for the error here. The commitment is valid by definition, right?

dignifiedquire requested a review from porcuquine as a code owner July 29, 2019 15:50

dignifiedquire requested review from nicola and laser July 29, 2019 16:24

porcuquine reviewed Jul 29, 2019

View reviewed changes

storage-proofs/src/circuit/rational_post.rs Outdated Show resolved Hide resolved

storage-proofs/src/rational_post.rs Outdated Show resolved Hide resolved

storage-proofs/src/rational_post.rs Outdated Show resolved Hide resolved

This was referenced Jul 30, 2019

feat: update for rational post filecoin-project/rust-fil-proofs-ffi#12

Merged

feat: update for rational post filecoin-project/rust-fil-sector-builder#23

Merged

dignifiedquire force-pushed the feat/rational-post branch from f1f4886 to c243861 Compare August 21, 2019 11:58

laser suggested changes Aug 21, 2019

View reviewed changes

laser reviewed Aug 22, 2019

View reviewed changes

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

porcuquine previously approved these changes Aug 23, 2019

View reviewed changes

dignifiedquire dismissed porcuquine’s stale review via aa495ae August 23, 2019 12:20

dignifiedquire force-pushed the feat/rational-post branch from 6c40628 to aa495ae Compare August 23, 2019 12:20

laser reviewed Aug 23, 2019

View reviewed changes

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

laser reviewed Aug 23, 2019

View reviewed changes

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

dignifiedquire mentioned this pull request Aug 23, 2019

chore: replace xorrng seeds with global constants #838

Closed

laser reviewed Aug 23, 2019

View reviewed changes

dignifiedquire mentioned this pull request Aug 23, 2019

refactor: introduce SectorID and SortedSectorIDs #839

Closed

laser reviewed Aug 26, 2019

View reviewed changes

storage-proofs/src/rational_post.rs Show resolved Hide resolved

laser reviewed Aug 26, 2019

View reviewed changes

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

laser reviewed Aug 26, 2019

View reviewed changes

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

porcuquine requested changes Aug 26, 2019

View reviewed changes

dignifiedquire force-pushed the feat/rational-post branch from d8d695b to a65d77a Compare August 27, 2019 11:26

laser reviewed Aug 27, 2019

View reviewed changes

filecoin-proofs/src/api/post.rs Show resolved Hide resolved

dignifiedquire added 5 commits August 27, 2019 17:51

feat(storage-proofs): implement vanilla rational post

717dbda

feat(storage-proofs): compound proof for rational post

e66fb43

fix naming

4804254

compound proof for rational post

4983276

implement correct inputs

6255689

dignifiedquire and others added 14 commits August 27, 2019 17:51

fixup: apply first round of CR

66501c5

simplify api surface for post

0e58ee5

rebase fixes

5acc9f9

chore: remove unused deps

cea74be

refactor: cleanup unused post constructions

5033654

chore: happy clippy

694a241

feat(rational-post): implement challenge derivation according to the …

8ce06ae

…spec

apply cr round 1

74faa53

happy clippy

b2606cf

feat(add SectorId type and transforms)

6ddb867

* feat(add SectorId type and transforms) * fixup(clippy) * feat(implement Display for SectorId) * fixup(derive Ord) * fixup(revert sector_id field change to sector as per spec)

introduce SectorSet, to avoid ambiguity between list of numbers

1108724

refactor(filecoin-proofs): split out post api

16c7585

use the typesystem to enforce more things

cd1dc77

test: improve rational post fault & challenge coverage

78419f6

dignifiedquire force-pushed the feat/rational-post branch from a65d77a to 78419f6 Compare August 27, 2019 15:51

make @laser happy

8f46e26

laser reviewed Aug 27, 2019

View reviewed changes

filecoin-proofs/src/api/post.rs Show resolved Hide resolved

laser reviewed Aug 27, 2019

View reviewed changes

storage-proofs/src/rational_post.rs Show resolved Hide resolved

dignifiedquire added 3 commits August 27, 2019 20:06

fix: handle all faulty sectors

64ee1b4

rename SectorSet to OrderedSectorSet

c7d9575

remove last piece of postproofpartitions

5942a97

laser previously approved these changes Aug 27, 2019

View reviewed changes

fix comparision operator.

15107cb

dignifiedquire dismissed laser’s stale review via 15107cb August 27, 2019 19:04

Update parameters.json

e84aa0f

porcuquine previously approved these changes Aug 27, 2019

View reviewed changes

reall publish new params

9c340e9

dignifiedquire dismissed porcuquine’s stale review via 9c340e9 August 27, 2019 20:23

dignifiedquire merged commit d2b39cc into master Aug 27, 2019

dignifiedquire deleted the feat/rational-post branch August 27, 2019 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage-proofs): implement rational post #763

feat(storage-proofs): implement rational post #763

dignifiedquire commented Jul 29, 2019 •

edited

Loading

laser left a comment •

edited

Loading

laser Aug 21, 2019

porcuquine Aug 22, 2019

laser Aug 23, 2019

porcuquine left a comment

porcuquine Aug 22, 2019

porcuquine Aug 22, 2019

porcuquine Aug 22, 2019

dignifiedquire commented Aug 23, 2019

laser Aug 23, 2019

dignifiedquire Aug 23, 2019

laser Aug 23, 2019

laser Aug 23, 2019 •

edited

Loading

laser Aug 26, 2019

dignifiedquire Aug 27, 2019

porcuquine left a comment

porcuquine Aug 26, 2019

dignifiedquire Aug 27, 2019

porcuquine Aug 26, 2019

dignifiedquire Aug 27, 2019

porcuquine Aug 26, 2019

dignifiedquire Aug 27, 2019

porcuquine Aug 26, 2019

dignifiedquire Aug 27, 2019

laser left a comment •

edited

Loading

porcuquine left a comment

porcuquine Aug 27, 2019

porcuquine Aug 27, 2019

feat(storage-proofs): implement rational post #763

feat(storage-proofs): implement rational post #763

Conversation

dignifiedquire commented Jul 29, 2019 • edited Loading

laser left a comment • edited Loading

Choose a reason for hiding this comment

Note: Breaking API Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porcuquine 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

dignifiedquire commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laser Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porcuquine 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

laser left a comment • edited Loading

Choose a reason for hiding this comment

porcuquine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Jul 29, 2019 •

edited

Loading

laser left a comment •

edited

Loading

laser Aug 23, 2019 •

edited

Loading

laser left a comment •

edited

Loading