feat: charge explicit gas for batch verification

This won't currently make a difference on mainnet because we only call this from the cron actor (in an implicit message). However, we _will_ need this when we have the miner actor call it directly.
filecoin-project · Aug 22, 2023 · 3941f00 · 3941f00
1 parent dd01512
commit 3941f00
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 47 deletions.
diff --git a/fvm/src/gas/price_list.rs b/fvm/src/gas/price_list.rs
@@ -27,6 +27,9 @@ use crate::kernel::SupportedHashes;
 // https://docs.rs/wasmtime/2.0.2/wasmtime/struct.InstanceLimits.html#structfield.table_elements
 const TABLE_ELEMENT_SIZE: u32 = 8;
 
+// Parallelism for batch seal verification.
+const BATCH_SEAL_PARALLELISM: usize = 8;
+
 /// Create a mapping from enum items to values in a way that guarantees at compile
 /// time that we did not miss any member, in any of the prices, even if the enum
 /// gets a new member later.
@@ -129,7 +132,10 @@ lazy_static! {
         tipset_cid_historical: Gas::new(215_000),
 
         compute_unsealed_sector_cid_base: Gas::new(98647),
-        verify_seal_base: Gas::new(2000), // TODO revisit potential removal of this
+        verify_seal_batch: ScalingCost {
+            flat: Gas::zero(), // TODO: Determine startup overhead.
+            scale: Gas::new(34721049), // TODO: Maybe re-benchmark?
+        },
 
         verify_aggregate_seal_per: [
             (
@@ -426,7 +432,7 @@ pub struct PriceList {
     pub(crate) tipset_cid_historical: Gas,
 
     pub(crate) compute_unsealed_sector_cid_base: Gas,
-    pub(crate) verify_seal_base: Gas,
+    pub(crate) verify_seal_batch: ScalingCost,
     pub(crate) verify_aggregate_seal_per: HashMap<RegisteredSealProof, Gas>,
     pub(crate) verify_aggregate_seal_steps: HashMap<RegisteredSealProof, StepCost>,
 
@@ -613,9 +619,20 @@ impl PriceList {
 
     /// Returns gas required for seal verification.
     #[inline]
-    pub fn on_verify_seal(&self, _info: &SealVerifyInfo) -> GasCharge {
-        GasCharge::new("OnVerifySeal", self.verify_seal_base, Zero::zero())
+    pub fn on_batch_verify_seal(&self, vis: &[SealVerifyInfo]) -> GasCharge {
+        // Charge based on the expected parallelism, rounding up.
+        let mut count = vis.len();
+        let remainder = count % BATCH_SEAL_PARALLELISM;
+        if remainder > 0 {
+            count += BATCH_SEAL_PARALLELISM - remainder;
+        }
+        GasCharge::new(
+            "OnBatchVerifySeal",
+            self.verify_seal_batch.apply(count),
+            Zero::zero(),
+        )
     }
+
     #[inline]
     pub fn on_verify_aggregate_seals(
         &self,

diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs
@@ -575,53 +575,34 @@ where
     }
 
     fn batch_verify_seals(&self, vis: &[SealVerifyInfo]) -> Result<Vec<bool>> {
-        // NOTE: gas has already been charged by the power actor when the batch verify was enqueued.
-        // Lotus charges "virtual" gas here for tracing only.
-        let mut items = Vec::new();
-        for vi in vis {
-            let t = self
-                .call_manager
-                .charge_gas(self.call_manager.price_list().on_verify_seal(vi))?;
-            items.push((vi, t));
-        }
-        log::debug!("batch verify seals start");
-        let out = items.par_drain(..)
-            .with_min_len(vis.len() / *NUM_CPUS)
-            .map(|(seal, timer)| {
-                let start = GasTimer::start();
-                let verify_seal_result = std::panic::catch_unwind(|| verify_seal(seal));
-                let ok = match verify_seal_result {
-                    Ok(res) => {
-                        match res {
-                            Ok(correct) => {
-                                if !correct {
-                                    log::debug!(
-                                        "seal verify in batch failed (miner: {}) (err: Invalid Seal proof)",
-                                        seal.sector_id.miner
-                                    );
-                                }
-                                correct // all ok
-                            }
-                            Err(err) => {
-                                log::debug!(
-                                    "seal verify in batch failed (miner: {}) (err: {})",
-                                    seal.sector_id.miner,
-                                    err
-                                );
-                                false
-                            }
-                        }
+        let t = self
+            .call_manager
+            .charge_gas(self.call_manager.price_list().on_batch_verify_seal(vis))?;
+
+        // TODO: consider optimizing for one proof (i.e., don't charge a batch overhead?)
+        let out = vis
+            .par_iter()
+            .map(|seal| {
+                match catch_and_log_panic("verifying seals", || verify_seal(seal)) {
+                    Ok(true) => return true,
+                    Ok(false) => {
+                        log::debug!(
+                            "seal verify in batch failed (miner: {}) (err: Invalid Seal proof)",
+                            seal.sector_id.miner
+                        );
                     }
-                    Err(e) => {
-                        log::error!("seal verify internal fail (miner: {}) (err: {:?})", seal.sector_id.miner, e);
-                        false
+                    Err(err) => {
+                        log::debug!(
+                            "seal verify in batch failed (miner: {}) (err: {})",
+                            seal.sector_id.miner,
+                            err
+                        );
                     }
-                };
-                timer.stop_with(start);
-                ok
+                }
+                false
             })
             .collect();
-        log::debug!("batch verify seals end");
+        t.stop();
         Ok(out)
     }