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

fix: hard-code rows to discard #1724

Merged
merged 1 commit into from
Oct 5, 2023
Merged

fix: hard-code rows to discard #1724

merged 1 commit into from
Oct 5, 2023

Conversation

vmx
Copy link
Contributor

@vmx vmx commented Sep 26, 2023

Only rows of the TreeR get discarded. Hence hard-code all other cases to 0 to add clarity, that they don't discard any rows. This makes the code easier to follow and reason about.

cryptonemo
cryptonemo previously approved these changes Oct 3, 2023
Copy link
Collaborator

@cryptonemo cryptonemo left a comment

Choose a reason for hiding this comment

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

This appears to be correct

let size = get_base_tree_size::<Tree>(SectorSize(sector_size))?;
let tree_c_config = StoreConfig {
path: PathBuf::from(output_dir.as_ref()),
id: CacheKey::CommCTree.to_string(),
size: Some(size),
rows_to_discard,
rows_to_discard: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

If TreeR has discarded rows, shouldn't also TreeC? Admittedly, I don't fully understand rows_to_discard and its implications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If TreeR has discarded rows, shouldn't also TreeC? Admittedly, I don't fully understand rows_to_discard and its implications

That's exactly the confusing thing, which motivated this change. The rows_to_discard really only ever applies to TreeR. Even if it's set, it would be ignored by a TreeC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To put it another way, we need to keep TreeR around for the long term for PoSt proving, where-as TreeC is removed after the initial PoRep proof usage. The idea was that if we need to keep TreeR around -- how could we optimize (down) the amount of space that it requires on disk (i.e. compaction/level cache store).

@DrPeterVanNostrand
Copy link
Contributor

Looks good to me too. I just have the one question about TreeC discarded rows, if TreeC has the same number of leaves and arity as TreeR.

Only rows of the TreeR get discarded. Hence hard-code all other cases to 0
to add clarity, that they don't discard any rows. This makes the code easier
to follow and reason about.
@vmx vmx force-pushed the hard-code-rows-to-discard branch from 3f15aaf to 3529426 Compare October 5, 2023 12:41
@vmx vmx changed the base branch from cleanup-cache-with-glob to master October 5, 2023 12:43
@vmx vmx dismissed cryptonemo’s stale review October 5, 2023 12:43

The base branch was changed.

@vmx
Copy link
Contributor Author

vmx commented Oct 5, 2023

Due to the rebasing, it makes it hard to review whether anything changes between the last review or not.

Easiest way to verify that nothing changed is diffing between the actual changes, e.g.:

git show 3f15aaf4c46f260a4a0c88f977ccdf455f204f53 > /tmp/a
git show 3529426a3a241fbd3362b0aa4889b402139201b5 > /tmp/b
$ diff /tmp/a /tmp/b
1c1
< commit 3f15aaf4c46f260a4a0c88f977ccdf455f204f53
---
> commit 3529426a3a241fbd3362b0aa4889b402139201b5
12c12
< index dc551681..1cb4af0c 100644
---
> index 4dda1712..a43ac363 100644
15c15
< @@ -10,7 +10,6 @@ use filecoin_proofs::{
---
> @@ -9,7 +9,6 @@ use filecoin_proofs::{
23c23
< @@ -20,7 +19,7 @@ use rayon::prelude::{
---
> @@ -19,7 +18,7 @@ use rayon::prelude::{
32c32
< @@ -194,12 +193,8 @@ pub fn create_replicas<Tree: 'static + MerkleTreeTrait<Hasher = PoseidonHasher>>
---
> @@ -193,12 +192,8 @@ pub fn create_replicas<Tree: 'static + MerkleTreeTrait>(
48c48
< index 1449679a..18398d39 100644
---
> index 5f541feb..1cad7544 100644
51c51
< @@ -15,7 +15,6 @@ use storage_proofs_core::{
---
> @@ -16,7 +16,6 @@ use storage_proofs_core::{
57,59c57,59
<  use storage_proofs_porep::stacked::{self, generate_replica_id, PublicParams, StackedDrg};
<  pub use storage_proofs_update::constants::TreeRHasher;
< @@ -338,16 +337,7 @@ where
---
>  use storage_proofs_porep::stacked::{
>      generate_replica_id, PersistentAux, PublicParams, StackedDrg, TemporaryAux,
> @@ -356,16 +355,7 @@ where
78c78
< index 03375e3b..8cd30167 100644
---
> index 69dd3932..59fc1481 100644
81c81
< @@ -156,11 +156,7 @@ where
---
> @@ -158,11 +158,7 @@ where
94c94
< @@ -271,11 +267,7 @@ where
---
> @@ -273,11 +269,7 @@ where
107c107
< @@ -1179,7 +1171,6 @@ where
---
> @@ -1202,7 +1194,6 @@ where
115c115
< @@ -1191,7 +1182,7 @@ where
---
> @@ -1214,7 +1205,7 @@ where
124c124
< @@ -1234,13 +1225,12 @@ where
---
> @@ -1257,13 +1248,12 @@ where
139c139
< @@ -1252,7 +1242,7 @@ where
---
> @@ -1275,7 +1265,7 @@ where
208c208
< index 1ba2baf8..251d4eca 100644
---
> index 3e5b1035..212f45d6 100644
220,222c220,222
<      self, LayerChallenges, PrivateInputs, PublicInputs, SetupParams, StackedBucketGraph,
< -    StackedDrg, TemporaryAuxCache, BINARY_ARITY, EXP_DEGREE,
< +    StackedDrg, TemporaryAuxCache, EXP_DEGREE,
---
>      LayerChallenges, PrivateInputs, PublicInputs, SetupParams, StackedBucketGraph, StackedDrg,
> -    TemporaryAux, TemporaryAuxCache, BINARY_ARITY, EXP_DEGREE,
> +    TemporaryAux, TemporaryAuxCache, EXP_DEGREE,

@vmx vmx merged commit 3cd931f into master Oct 5, 2023
31 checks passed
@vmx vmx deleted the hard-code-rows-to-discard branch October 5, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants