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 opaque porep_id as public input and use when constructing replica_id #1144

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

porcuquine
Copy link
Collaborator

@porcuquine porcuquine commented Jun 3, 2020

This PR adds a new 32-byte public input, porep_id, to SDR. This value is used to construct replica_id, the drg seed, and the Feistel keys. As a public input, it must be supplied (with identical value) by both prover and verifier. The intention at the protocol level is that proofs consumers should pass a value derived from the RegisteredProof — since it is guaranteed to distinguish all extant PoReps. However, from the perspective of the rust-fil-proofs the porep_id is treated as opaque and is required only to be used consistently. If consumers choose to duplicate porep_ids, they take responsibility for the possibility that replica_id or DRG graph might be reused between the set of concrete proofs the duplicated id is used to create. If the Filecoin spec is modified to require a distinct porep_id for every distinct concrete type, such usage will violate the protocol.

UPDATE: now supports Feistel keys

  • porep_id is a PublicParam
  • It is passed to derive_porep_domain_seed, which also takes domain separation tag.
  • In this way, the DRG seed and Feistel keys are created when the graphs are instantiated.

@porcuquine porcuquine marked this pull request as ready for review June 3, 2020 07:12
@porcuquine porcuquine added the cryptocomputelab CryptoComputeLab work label Jun 3, 2020
@nicola
Copy link
Contributor

nicola commented Jun 3, 2020

It's ok to be opaque, but it's important the at verification time, the chain correctly verifies the correct porep-id. Does this live in CompoundProof?

@porcuquine
Copy link
Collaborator Author

It is a public input. It can be derived from the RegisteredProof at the API layer, which handles routing and accounts for versioning. As long as proofs consumers provide the correct RegisteredProof at proof and verification time (without which verification could not be expected to work in any case), the correct porep_id can be attached and forwarded to the inner API.

@nicola
Copy link
Contributor

nicola commented Jun 3, 2020

Perfect. Future review of integration of proofs into Filecoin should catch this. I will make a note.

nicola
nicola previously approved these changes Jun 3, 2020
@nicola
Copy link
Contributor

nicola commented Jun 3, 2020

Just a quick note: this PR does not directly address issue "20. FeistelKeys, Seed_D should be specific to a particular Proof of Replication e.g. H(RegisteredProofID || random nonce)", but only "21. ReplicaID should contain the RegisteredProofID". Correct?

@porcuquine
Copy link
Collaborator Author

No, it address both. See the second commit: 5cb4331.

@porcuquine
Copy link
Collaborator Author

Reading more closely, it does handle the DRG part, but not the Feistel keys. I will add.

@porcuquine
Copy link
Collaborator Author

@nicola Updated with Feistel keys and DSTs.

@porcuquine porcuquine force-pushed the feat/opaque-porep-identifier branch from e620225 to c269e8b Compare June 3, 2020 12:15
@nicola nicola self-requested a review June 3, 2020 12:39
nicola
nicola previously approved these changes Jun 3, 2020
Copy link
Contributor

@nicola nicola left a comment

Choose a reason for hiding this comment

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

Perfect, it seems that this PR has correctly replaced the previous seed with the porepID

cryptonemo
cryptonemo previously approved these changes Jun 3, 2020
@porcuquine
Copy link
Collaborator Author

FYI, @cryptonemo: this change is going to require work in rust-filecoin-api — since porep_id will need to be set in the PorepConfig. This may also require some work to ensure the concrete enum values of RegisteredProofs agree between the spec (actors) and the source. I will skip discussion here of further, related changes which may be required but wanted to at least let you know there will be more than the minimum to change in rust-filecoin-api, and that this may end up affecting filecoin-ffi and spec actors as well. (cc: @laser )

dignifiedquire
dignifiedquire previously approved these changes Jun 3, 2020
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

fiiine

@porcuquine porcuquine dismissed stale reviews from dignifiedquire and cryptonemo via 9f1c86b June 3, 2020 19:30
@porcuquine porcuquine force-pushed the feat/opaque-porep-identifier branch from 9f1c86b to 30f5f0d Compare June 3, 2020 19:39
@porcuquine porcuquine merged commit b9126bf into master Jun 3, 2020
@porcuquine porcuquine deleted the feat/opaque-porep-identifier branch June 3, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cryptocomputelab CryptoComputeLab work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants