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

Calculate challenges per partition, ensuring total minimum is met. #621

Merged
merged 1 commit into from Apr 18, 2019

Conversation

@porcuquine
Copy link
Collaborator

commented Apr 18, 2019

Toward #422.

Included in this PR:

  • Calculate PoRep challenges based on number of partitions, such that minimum total challenges are met. The minimum is a constant across all sector sizes and proof sizes.
  • Generate parameters for all valid sector-size/partitions combinations. As of now, all combinations are valid, so all are generated.
  • Sort parameters when publishing so re-publishing without changes never produces a diff. (cc: @sidke).
  • Reduce default value of NUM_PROVING_THREADS to 1, since larger values may be problematic on machines with few cores. (This will begin to matter as we add partitions.)

NOTE: I published parameters here to verify that they had not changed. However, because parameters.json did not previously have a canonical ordering, this produced a diff in the changeset. I leave this in to ensure the manifest and code are in sync — but my intention is for the current work not to affect parameters or actual proofs.

@porcuquine porcuquine requested review from dignifiedquire and laser as code owners Apr 18, 2019

@porcuquine porcuquine force-pushed the multi-sector-internal branch from 81ee9e8 to 43a174d Apr 18, 2019

@laser
Copy link
Collaborator

left a comment

I have some questions and comments. Would you mind answering those before merging?

fn setup_params(sector_bytes: PaddedBytesAmount) -> layered_drgporep::SetupParams {
fn setup_params(
sector_bytes: PaddedBytesAmount,
partitions: usize,

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

SUGGESTION: I see that this function calls select_challenges with POREP_MINIMUM_CHALLENGES. I see that it has a partitions parameter which is of type usize. I think it makes sense to use PoRepProofPartitions as the type for the partitions parameter instead.

This comment has been minimized.

Copy link
@porcuquine

porcuquine Apr 18, 2019

Author Collaborator

select_challenges is purely about calculating a LayerChallenges that meets some challenge requirement. It could be logically housed in filecoin_proofs or used to reason about hypothetically (but not yet) supported partition amounts. (For example, see the unit test used to gain confidence in the correctness of its output.) If select_challenges were to take aPoRepProofPartitions we would still want another function exactly like the current one that it could call to do the real work. That seems like yet another layer of indirection I'd rather avoid.

#[derive(Clone, Copy, Debug)]
pub enum PoRepProofPartitions {
Two,
}

pub const POREP_PROOF_PARTITION_CHOICES: [PoRepProofPartitions; 1] = [PoRepProofPartitions::Two];

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

SUGGESTION: I don't think that you should do this. I created an enum so that the compiler could enforce exhaustiveness of pattern matches - and this array defeats that. I think that instead of putting each enum member into a fixed length array and iterating over that array's elements that you should instead pattern match. This will prevent you/me from needing to maintain the POREP_PROOF_PARTITION_CHOICES constant.

This comment has been minimized.

Copy link
@porcuquine

porcuquine Apr 18, 2019

Author Collaborator

Yes, I considered that. It's not clear to me that you can use wildcard pattern matching to enumerate the choices of an enum — which is what we'd like to be able to do. Especially when enumerating combinations, a nested loop will work, but I think we'd need to duplicate whatever manual/matching enumeration strategy was chosen — in each branch. I could be missing something, though.

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

After thinking about this again, I retract my original objection. If your intent is to generate parameters for each combination of sector size and PoRep proof partitions, then this is a fine approach. Alternatively, you could try to generate an Iter impl (see here).

@@ -10,6 +10,9 @@ pub enum SectorSize {
TwoHundredFiftySixMiB,
}

pub const SECTOR_SIZE_CHOICES: [SectorSize; 2] =

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

SUGGESTION: same comment as above, with POREP_PROOF_PARTITION_CHOICES

SectorSize::TwoHundredFiftySixMiB,
PoRepProofPartitions::Two,
));
for p in &POREP_PROOF_PARTITION_CHOICES {

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

SUGGESTION: See comment at POREP_PROOF_PARTITION_CHOICES.

.all_challenges()
};
// Update to ensure all supported PoRepProofPartitions options are represented here.
assert_eq!(vec![1, 1, 2, 2], f(usize::from(PoRepProofPartitions::Two)));

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

SUGGESTION: See comment at POREP_PROOF_PARTITION_CHOICES.

This comment has been minimized.

Copy link
@porcuquine

porcuquine Apr 18, 2019

Author Collaborator

As the number of choices grows, we could change this test to iterate through POREP_PROOF_PARTITION_CHOICES if we feel that's valuable. The value of having these lines here is less to test them as to expose the calculated result explicitly in code for reference. (It does also give an opportunity to sanity check correctness — but that should be primarily handled by the explicit tests below — which are fairly sparse now but could be supplemented if necessary.)

@@ -447,7 +485,7 @@ fn make_merkle_tree<T: Into<PathBuf> + AsRef<Path>>(
let mut data = Vec::new();
f_in.read_to_end(&mut data)?;

public_params(bytes).graph.merkle_tree(&data)
public_params(bytes, 1).graph.merkle_tree(&data)

This comment has been minimized.

Copy link
@laser

laser Apr 18, 2019

Collaborator

QUESTION: Why the hard-coded 1 here? make_merkle_tree is called during PoSt generation. I would expect that PoSt proof partitions would be plumbed in to make_merkle_tree instead of this hard-coded 1.

This comment has been minimized.

Copy link
@porcuquine

porcuquine Apr 18, 2019

Author Collaborator

I intended that as a later step. As we discussed, PoSt doesn't currently support multiple partitions so I defaulted to what it does — until such time as we need and plumb otherwise.

@porcuquine porcuquine force-pushed the multi-sector-internal branch from 43a174d to 38d80a1 Apr 18, 2019

@porcuquine porcuquine merged commit 5367617 into master Apr 18, 2019

11 checks passed

ci/circleci: bench_nightly Your tests passed on CircleCI!
Details
ci/circleci: build_darwin_release Your tests passed on CircleCI!
Details
ci/circleci: build_linux_release Your tests passed on CircleCI!
Details
ci/circleci: cargo_fetch Your tests passed on CircleCI!
Details
ci/circleci: clippy Your tests passed on CircleCI!
Details
ci/circleci: ffi_regression Your tests passed on CircleCI!
Details
ci/circleci: rustfmt Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test_ignored_release Your tests passed on CircleCI!
Details
ci/circleci: test_nightly Your tests passed on CircleCI!
Details
ci/circleci: test_release Your tests passed on CircleCI!
Details

@porcuquine porcuquine deleted the multi-sector-internal branch Jul 8, 2019

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