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

support variable length PoSt and PoRep proofs #2607

Merged
merged 15 commits into from Apr 30, 2019

Conversation

@laser
Copy link
Contributor

commented Apr 17, 2019

Blocked by this upstream rust-fil-proofs PR.

Fixes #2591.

Why's this PR needed?

Miners can choose proof partitions in order to trade off between RAM usage and on-chain proof size. What this means for go-filecoin is that the number of bytes in a PoSt or PoRep proof is no longer fixed.

What's in this PR?

  • Make PoSt and PoRep (seal) proofs []byte instead of [192]byte and [384]byte
  • BREAKING CHANGE: JSON encode the types.PoStProof in the types.Block, using the default encoder. This will produce a base64-encoded string (or null) instead of a JSON array.

@laser laser changed the title [WIP - DO NOT MERGE] consume new variable-length proofs SHA [WIP - DO NOT MERGE] support PoSt and PoRep proofs of differing lengths Apr 17, 2019

@laser laser force-pushed the feat/2591-variable-length-proofs branch 5 times, most recently from a164778 to 7668ca0 Apr 18, 2019

@laser laser force-pushed the feat/2591-variable-length-proofs branch from 7668ca0 to 34e6499 Apr 25, 2019

@laser laser changed the title [WIP - DO NOT MERGE] support PoSt and PoRep proofs of differing lengths support variable length PoSt and PoRep proofs Apr 26, 2019

@shannonwells
Copy link
Contributor

left a comment

Nothing blocking; I don't have enough knowledge to thoroughly review, but all LGTM given that, so 🤷‍♀️

// See https://github.com/filecoin-project/go-filecoin/issues/1302
// bool(resPtr.is_valid),
IsValid: true,
IsValid: bool(resPtr.is_valid),

This comment has been minimized.

Copy link
@shannonwells

shannonwells Apr 26, 2019

Contributor

does this always return true or are we really validating the PoSTResponse now?

// Int returns an integer representing the number of PoSt partitions
func (p PoStProofPartitions) Int() int {
switch p {
case OnePoStProofPartition:

This comment has been minimized.

Copy link
@shannonwells

shannonwells Apr 26, 2019

Contributor

I assume this is because at some point >1 partition may be expected, but maybe include a comment to that extent so nobody is tempted to convert these all to if-elses.

This comment has been minimized.

Copy link
@laser

laser Apr 26, 2019

Author Contributor

Yeah, we will definitely allow miners to select from a set of PoSt and PoRep proof partitions.

@acruikshank
Copy link
Contributor

left a comment

The Partition enums seem over-engineered, but I'm having trouble visualizing how it plays out once there is more than one valid value.

I would like to seem some testing and documentation on the transforms.

I'd also like to see attempts to produce an invalid Partitions enum fail immediately.

Other than that, it looks good.

proofs/sectorbuilder/rustsectorbuilder.go Outdated Show resolved Hide resolved
return C.GoBytes(unsafe.Pointer(src), C.int(size))
}

func goPoStProofs(partitions C.uint8_t, src *C.uint8_t, size C.size_t) ([]types.PoStProof, error) {

This comment has been minimized.

Copy link
@acruikshank

acruikshank Apr 26, 2019

Contributor

Once these utility methods move to a separate file, I start to feel uncomfortable having them private, undocumented and untested. I can see how it's tough to to test effectively, but documentation would be helpful.

This comment has been minimized.

Copy link
@laser

laser Apr 26, 2019

Author Contributor

I start to feel uncomfortable having them private

Unfortunately, I can't export these things (and share them across packages, for instance). This is due to a limitation in CGO:

Cgo translates C types into equivalent unexported Go types. Because the translations are unexported, a Go package should not expose C types in its exported API: a C type used in one Go package is different from the same C type used in another.

They aren't documented because they aren't exported. They aren't tested because they aren't part of the public (exported) API.

That said, I am happy to write some documentation for these unexported functions.

// #include "./include/libfilecoin_proofs.h"
import "C"

func cPoStProofs(src []types.PoStProof) (C.uint8_t, *C.uint8_t, C.size_t) {

This comment has been minimized.

Copy link
@acruikshank

acruikshank Apr 26, 2019

Contributor

Please document functions and add tests for the size calculations.

This comment has been minimized.

Copy link
@laser

laser Apr 26, 2019

Author Contributor

These functions aren't exported, so they're not tested directly. Under what circumstances is it appropriate to test unexported functions/methods?

Note that these functions are tested indirectly through integration tests.

// ProofPartitions returns the number of partitions used to create the PoRep
// proof.
func (s PoRepProof) ProofPartitions() PoRepProofPartitions {
return NewPoRepProofPartitions(len(s) / SinglePartitionProofLen)

This comment has been minimized.

Copy link
@acruikshank

acruikshank Apr 26, 2019

Contributor

This should really fail here if len(s) is not 2*SinglePartitionProofLen. Getting a UnknownPoStProofPartitions that's going to produce bad numbers somewhere else will just make it hard to track bugs.

Also, using integer math, this is going to produce:

  • UnknownPoStProofPartitions for 0 <= len(s) < 384
  • TwoPoRepProofPartitions for 384 <= len(s) < 576
  • UnknownPoStProofPartitions for 576 <= len(s) < ♾

Is this what you want? Is 500 bytes a valid TwoPoRepProofPartitions?

This comment has been minimized.

Copy link
@laser

laser Apr 26, 2019

Author Contributor

I really wanted the type of this function to be (s PoRepProof) ProofPartitions() PoRepProofPartitions and not (s PoRepProof) ProofPartitions() (PoRepProofPartitions, error).

Is 500 bytes a valid TwoPoRepProofPartitions?

The rust-fil-proofs system will reject (with an error) a 500 byte PoRep proof. The rust-fil-proofs system will never produce a 500 byte PoRep proof, either. By relying on the rust-fil-proofs system to reject the buggy proof, I could avoid duplicating the runtime validation into go-filecoin.

That said, it's not much software to enforce the invariant on both sides of the FFI wall. Would you agree to a panic in the event that we can't map the PoRepProof bytes cleanly to a PoRepProofPartitions value?

types/sector_class.go Show resolved Hide resolved

@laser laser force-pushed the feat/2591-variable-length-proofs branch from e2757c4 to fbd4d92 Apr 29, 2019

@laser laser force-pushed the feat/2591-variable-length-proofs branch from 70f74c5 to 8238ef5 Apr 30, 2019

@laser laser merged commit dcd250d into master Apr 30, 2019

5 checks passed

ci/circleci: build_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: deps_linux Your tests passed on CircleCI!
Details
ci/circleci: functional_test_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: integration_test_linux Your tests passed on CircleCI!
Details
ci/circleci: unit_test_linux Your tests passed on CircleCI!
Details

@laser laser deleted the feat/2591-variable-length-proofs branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.