From 2f87cf92a6128d68947ef30106cdb3a6c84c5712 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 22 Aug 2023 15:39:44 -0700 Subject: [PATCH] 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. --- fvm/src/gas/price_list.rs | 25 +++++++++++--- fvm/src/kernel/default.rs | 70 ++++++++++++++------------------------- 2 files changed, 46 insertions(+), 49 deletions(-) diff --git a/fvm/src/gas/price_list.rs b/fvm/src/gas/price_list.rs index 8216575a6..c5653a037 100644 --- 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, pub(crate) verify_aggregate_seal_steps: HashMap, @@ -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 index 0819f658c..2426757d4 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -27,8 +27,7 @@ use fvm_shared::sys::out::vm::ContextFlags; use fvm_shared::{commcid, ActorID}; use lazy_static::lazy_static; use multihash::MultihashDigest; -use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator}; -use rayon::prelude::ParallelDrainRange; +use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; use std::io::Write; use super::blocks::{Block, BlockRegistry}; @@ -575,53 +574,34 @@ where } fn batch_verify_seals(&self, vis: &[SealVerifyInfo]) -> Result> { - // 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) }