fix: change default rows_to_discard for oct trees (#1165)

* fix: change default rows_to_discard for oct trees
* feat: move default value from merkle_light to internal
* fix: update comment
* fix: apply some review feedback

fil-proofs-tooling/src/bin/benchy/stacked.rs

@@ -26,6 +26,7 @@ use storage_proofs::porep::stacked::{
 use storage_proofs::porep::PoRep;
 use storage_proofs::proof::ProofScheme;
 use storage_proofs::test_helper::setup_replica;
+use storage_proofs::util::default_rows_to_discard;
 use tempfile::TempDir;
 
 fn dump_proof_bytes<Tree: MerkleTreeTrait>(
@@ -116,7 +117,7 @@ where
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         let replica_id = H::Domain::random(rng);

filecoin-proofs/src/api/mod.rs

@@ -14,6 +14,7 @@ use storage_proofs::porep::stacked::{
 };
 use storage_proofs::porep::PoRep;
 use storage_proofs::sector::SectorId;
+use storage_proofs::util::default_rows_to_discard;
 use typenum::Unsigned;
 
 use crate::api::util::{as_safe_commitment, get_base_tree_leafs, get_base_tree_size};
@@ -149,7 +150,7 @@ where
     let config = StoreConfig::new(
         cache_path.as_ref(),
         CacheKey::CommDTree.to_string(),
-        StoreConfig::default_rows_to_discard(
+        default_rows_to_discard(
             base_tree_leafs,
             <DefaultBinaryTree as MerkleTreeTrait>::Arity::to_usize(),
         ),

filecoin-proofs/src/api/post.rs

@@ -17,6 +17,7 @@ use storage_proofs::merkle::{
 use storage_proofs::multi_proof::MultiProof;
 use storage_proofs::post::fallback;
 use storage_proofs::sector::*;
+use storage_proofs::util::default_rows_to_discard;
 
 use crate::api::util::{as_safe_commitment, get_base_tree_leafs, get_base_tree_size};
 use crate::caches::{get_post_params, get_post_verifying_key};
@@ -149,7 +150,7 @@ impl<Tree: 'static + MerkleTreeTrait> PrivateReplicaInfo<Tree> {
             "post: base tree size {}, base tree leafs {}, rows_to_discard {}, arities [{}, {}, {}]",
             base_tree_size,
             base_tree_leafs,
-            StoreConfig::default_rows_to_discard(base_tree_leafs, Tree::Arity::to_usize()),
+            default_rows_to_discard(base_tree_leafs, Tree::Arity::to_usize()),
             Tree::Arity::to_usize(),
             Tree::SubTreeArity::to_usize(),
             Tree::TopTreeArity::to_usize(),
@@ -158,7 +159,7 @@ impl<Tree: 'static + MerkleTreeTrait> PrivateReplicaInfo<Tree> {
         let mut config = StoreConfig::new(
             self.cache_dir_path(),
             CacheKey::CommRLastTree.to_string(),
-            StoreConfig::default_rows_to_discard(base_tree_leafs, Tree::Arity::to_usize()),
+            default_rows_to_discard(base_tree_leafs, Tree::Arity::to_usize()),
         );
         config.size = Some(base_tree_size);
 

filecoin-proofs/src/api/seal.rs

@@ -21,6 +21,7 @@ use storage_proofs::porep::stacked::{
 };
 use storage_proofs::proof::ProofScheme;
 use storage_proofs::sector::SectorId;
+use storage_proofs::util::default_rows_to_discard;
 
 use crate::api::util::{
     as_safe_commitment, commitment_from_fr, get_base_tree_leafs, get_base_tree_size,
@@ -127,19 +128,18 @@ where
         );
 
         trace!(
-            "seal phase 1: sector_size {}, base tree size {}, base tree leafs {}, rows to discard {}",
+            "seal phase 1: sector_size {}, base tree size {}, base tree leafs {}",
             u64::from(porep_config.sector_size),
             base_tree_size,
             base_tree_leafs,
-            StoreConfig::default_rows_to_discard(base_tree_leafs, BINARY_ARITY)
         );
 
         // MT for original data is always named tree-d, and it will be
         // referenced later in the process as such.
         let mut config = StoreConfig::new(
             cache_path.as_ref(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(base_tree_leafs, BINARY_ARITY),
+            default_rows_to_discard(base_tree_leafs, BINARY_ARITY),
         );
         let data_tree = create_base_merkle_tree::<BinaryMerkleTree<DefaultPieceHasher>>(
             Some(config.clone()),
@@ -247,11 +247,10 @@ where
             "seal phase 2: base tree size {}, base tree leafs {}, rows to discard {}",
             base_tree_size,
             base_tree_leafs,
-            StoreConfig::default_rows_to_discard(base_tree_leafs, BINARY_ARITY)
+            default_rows_to_discard(base_tree_leafs, BINARY_ARITY)
         );
         ensure!(
-            config.rows_to_discard
-                == StoreConfig::default_rows_to_discard(base_tree_leafs, BINARY_ARITY),
+            config.rows_to_discard == default_rows_to_discard(base_tree_leafs, BINARY_ARITY),
             "Invalid cache size specified"
         );
 

storage-proofs/core/src/merkle/builders.rs

@@ -13,7 +13,7 @@ use rayon::prelude::*;
 
 use crate::error::*;
 use crate::hasher::{Domain, Hasher, PoseidonArity};
-use crate::util::{data_at_node, NODE_SIZE};
+use crate::util::{data_at_node, default_rows_to_discard, NODE_SIZE};
 
 use super::*;
 
@@ -409,7 +409,7 @@ where
         let config = StoreConfig::new(
             &temp_path,
             format!("test-lc-tree-{}", id),
-            StoreConfig::default_rows_to_discard(nodes, Tree::Arity::to_usize()),
+            default_rows_to_discard(nodes, Tree::Arity::to_usize()),
         );
 
         let mut tree =

storage-proofs/core/src/settings.rs

@@ -34,7 +34,7 @@ impl Default for Settings {
             column_write_batch_size: 262_144,
             use_gpu_tree_builder: false,
             max_gpu_tree_batch_size: 700_000,
-            rows_to_discard: 0,
+            rows_to_discard: 2,
         }
     }
 }

storage-proofs/core/src/util.rs

@@ -2,8 +2,11 @@ use crate::error;
 use anyhow::ensure;
 use bellperson::gadgets::boolean::{self, AllocatedBit, Boolean};
 use bellperson::{ConstraintSystem, SynthesisError};
+use merkletree::merkle::get_merkle_tree_row_count;
 use paired::Engine;
 
+use super::settings;
+
 pub const NODE_SIZE: usize = 32;
 
 /// Returns the start position of the data, 0-indexed.
@@ -142,6 +145,36 @@ pub fn reverse_bit_numbering(bits: Vec<boolean::Boolean>) -> Vec<boolean::Boolea
         .collect()
 }
 
+// If the tree is large enough to use the default value (per-arity), use it.  If it's too small to cache anything (i.e. not enough rows), don't discard any.
+pub fn default_rows_to_discard(leafs: usize, arity: usize) -> usize {
+    let row_count = get_merkle_tree_row_count(leafs, arity);
+    if row_count <= 2 {
+        // If a tree only has a root row and/or base, there is
+        // nothing to discard.
+        return 0;
+    } else if row_count == 3 {
+        // If a tree only has 1 row between the base and root,
+        // it's all that can be discarded.
+        return 1;
+    }
+
+    // row_count - 2 discounts the base layer (1) and root (1)
+    let max_rows_to_discard = row_count - 2;
+
+    // This configurable setting is for a default oct-tree
+    // rows_to_discard value, which defaults to 2.
+    let rows_to_discard = settings::SETTINGS.lock().unwrap().rows_to_discard as usize;
+
+    // Discard at most 'constant value' rows (coded below,
+    // differing by arity) while respecting the max number that
+    // the tree can support discarding.
+    match arity {
+        2 => std::cmp::min(max_rows_to_discard, 7),
+        4 => std::cmp::min(max_rows_to_discard, 5),
+        _ => std::cmp::min(max_rows_to_discard, rows_to_discard),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

storage-proofs/porep/src/drg/circuit.rs

@@ -321,7 +321,7 @@ mod tests {
         merkle::MerkleProofTrait,
         proof::ProofScheme,
         test_helper::setup_replica,
-        util::data_at_node,
+        util::{data_at_node, default_rows_to_discard},
     };
 
     use super::super::compound::DrgPoRepCompound;
@@ -349,7 +349,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.
@@ -395,10 +395,7 @@ mod tests {
         let priv_inputs = drg::PrivateInputs::<PedersenHasher> {
             tree_d: &aux.tree_d,
             tree_r: &aux.tree_r,
-            tree_r_config_rows_to_discard: StoreConfig::default_rows_to_discard(
-                nodes,
-                BINARY_ARITY,
-            ),
+            tree_r_config_rows_to_discard: default_rows_to_discard(nodes, BINARY_ARITY),
         };
 
         let proof_nc = drg::DrgPoRep::<PedersenHasher, _>::prove(&pp, &pub_inputs, &priv_inputs)

storage-proofs/porep/src/drg/compound.rs

@@ -296,6 +296,7 @@ mod tests {
         merkle::{BinaryMerkleTree, MerkleTreeTrait},
         proof::NoRequirements,
         test_helper::setup_replica,
+        util::default_rows_to_discard,
     };
 
     use crate::stacked::BINARY_ARITY;
@@ -335,7 +336,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.
@@ -380,10 +381,7 @@ mod tests {
         let private_inputs = drg::PrivateInputs {
             tree_d: &aux.tree_d,
             tree_r: &aux.tree_r,
-            tree_r_config_rows_to_discard: StoreConfig::default_rows_to_discard(
-                nodes,
-                BINARY_ARITY,
-            ),
+            tree_r_config_rows_to_discard: default_rows_to_discard(nodes, BINARY_ARITY),
         };
 
         // This duplication is necessary so public_params don't outlive public_inputs and private_inputs.

storage-proofs/porep/src/drg/vanilla.rs

@@ -619,7 +619,7 @@ mod tests {
         merkle::{BinaryMerkleTree, MerkleTreeTrait},
         table_tests,
         test_helper::setup_replica,
-        util::data_at_node,
+        util::{data_at_node, default_rows_to_discard},
     };
     use tempfile;
 
@@ -639,7 +639,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.
@@ -718,7 +718,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.
@@ -809,7 +809,7 @@ mod tests {
             let config = StoreConfig::new(
                 cache_dir.path(),
                 CacheKey::CommDTree.to_string(),
-                StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+                default_rows_to_discard(nodes, BINARY_ARITY),
             );
 
             // Generate a replica path.
@@ -854,10 +854,7 @@ mod tests {
             let priv_inputs = PrivateInputs::<Tree::Hasher> {
                 tree_d: &aux.tree_d,
                 tree_r: &aux.tree_r,
-                tree_r_config_rows_to_discard: StoreConfig::default_rows_to_discard(
-                    nodes,
-                    BINARY_ARITY,
-                ),
+                tree_r_config_rows_to_discard: default_rows_to_discard(nodes, BINARY_ARITY),
             };
 
             let real_proof = DrgPoRep::<Tree::Hasher, _>::prove(&pp, &pub_inputs, &priv_inputs)

storage-proofs/porep/src/stacked/circuit/proof.rs

@@ -356,6 +356,7 @@ mod tests {
         merkle::{get_base_tree_count, DiskTree, MerkleTreeTrait},
         proof::ProofScheme,
         test_helper::setup_replica,
+        util::default_rows_to_discard,
     };
 
     use crate::stacked::{
@@ -412,7 +413,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.
@@ -608,7 +609,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.

storage-proofs/porep/src/stacked/vanilla/proof.rs

@@ -25,7 +25,7 @@ use storage_proofs_core::{
     },
     merkle::*,
     settings,
-    util::NODE_SIZE,
+    util::{default_rows_to_discard, NODE_SIZE},
 };
 use typenum::{U11, U2, U8};
 
@@ -973,34 +973,31 @@ impl<'a, Tree: 'static + MerkleTreeTrait, G: 'static + Hasher> StackedDrg<'a, Tr
             CacheKey::CommDTree.to_string(),
             Some(get_merkle_tree_len(nodes_count, BINARY_ARITY)?),
         );
-        tree_d_config.rows_to_discard =
-            StoreConfig::default_rows_to_discard(nodes_count, BINARY_ARITY);
+        tree_d_config.rows_to_discard = default_rows_to_discard(nodes_count, BINARY_ARITY);
 
         let mut tree_r_last_config = StoreConfig::from_config(
             &config,
             CacheKey::CommRLastTree.to_string(),
             Some(get_merkle_tree_len(nodes_count, Tree::Arity::to_usize())?),
         );
 
-        // A default 'rows_to_discard' value will be chosen for
-        // tree_r_last, unless the user overrides this value via the
-        // environment setting (FIL_PROOFS_ROWS_TO_DISCARD).  If this
-        // value is specified, no checking is done on it and it may
+        // A default 'rows_to_discard' value will be chosen for tree_r_last, unless the user overrides this value via the
+        // environment setting (FIL_PROOFS_ROWS_TO_DISCARD).  If this value is specified, no checking is done on it and it may
         // result in a broken configuration.  Use with caution.
-        let env_rows_to_discard = settings::SETTINGS.lock().unwrap().rows_to_discard as usize;
-        tree_r_last_config.rows_to_discard = if env_rows_to_discard != 0 {
-            env_rows_to_discard
-        } else {
-            StoreConfig::default_rows_to_discard(nodes_count, Tree::Arity::to_usize())
-        };
+        tree_r_last_config.rows_to_discard =
+            default_rows_to_discard(nodes_count, Tree::Arity::to_usize());
+        trace!(
+            "tree_r_last using rows_to_discard={}",
+            tree_r_last_config.rows_to_discard
+        );
 
         let mut tree_c_config = StoreConfig::from_config(
             &config,
             CacheKey::CommCTree.to_string(),
             Some(get_merkle_tree_len(nodes_count, Tree::Arity::to_usize())?),
         );
         tree_c_config.rows_to_discard =
-            StoreConfig::default_rows_to_discard(nodes_count, Tree::Arity::to_usize());
+            default_rows_to_discard(nodes_count, Tree::Arity::to_usize());
 
         let labels = LabelsCache::<Tree>::new(&label_configs)?;
         let configs = split_config(tree_c_config.clone(), tree_count)?;
@@ -1264,7 +1261,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.
@@ -1441,7 +1438,7 @@ mod tests {
         let config = StoreConfig::new(
             cache_dir.path(),
             CacheKey::CommDTree.to_string(),
-            StoreConfig::default_rows_to_discard(nodes, BINARY_ARITY),
+            default_rows_to_discard(nodes, BINARY_ARITY),
         );
 
         // Generate a replica path.