src/state_manager/cache.rs (1)

97-114: Pending entry not cleared on compute error can leave stuck “pending” entries

If compute().await fails, the key remains in inner.pending indefinitely. Subsequent calls will always go down the “Empty” path, lock the same mutex, and re-attempt compute (good), but the pending list will grow with permanently failing keys, causing memory bloat.

Clear the pending entry on error as well. Example change:

-                        let value = compute().await?;
-
-                        // Write back to cache, release lock and return value
-                        self.insert(key.clone(), value.clone());
-                        Ok(value)
+                        let res = compute().await;
+                        // Ensure we don't leave a stale pending entry on error.
+                        self.with_inner(|inner| {
+                            inner.pending.retain(|(k, _)| k != key);
+                        });
+                        match res {
+                            Ok(value) => {
+                                // Write back to cache, release lock and return value
+                                self.insert(key.clone(), value.clone());
+                                Ok(value)
+                            }
+                            Err(e) => Err(e),
+                        }

Also applies to: 123-146

src/chain/store/chain_store.rs (1)

577-584: Use a single lookup to avoid TOCTOU and remove the panic

The current contains→get path can race under concurrency and trigger the "cache entry disappeared!" panic. Replace it with one get (or get_cloned) call and fall back to insertion:

-        if self.cache.contains(key) {
-            Ok(self.get(key).expect("cache entry disappeared!"))
-        } else {
-            let v = f()?;
-            self.insert(key.clone(), v.clone());
-            Ok(v)
-        }
+        match self.get(key) {
+            Some(v) => Ok(v),
+            None => {
+                let v = f()?;
+                self.insert(key.clone(), v.clone());
+                Ok(v)
+            }
+        }

• This removes the benign TOCTOU and eliminates the panic.
• Since SizeTrackingLruCache doesn’t expose an atomic get-or-insert helper, duplicate computation under contention is still possible. We can add a get_or_try_insert_with API to the cache as a follow-up if you’d like.

src/chain_sync/bad_block_cache.rs (1)

28-29: Metrics label rename to "bad_block" — LGTM. Consider exposing contains().

The rename aligns with the metrics standardization.

Optional: provide a contains() that uses the new SizeTrackingLruCache.contains() to avoid cloning/allocations elsewhere.

 impl BadBlockCache {
     pub fn new(cap: NonZeroUsize) -> Self {
         Self {
             cache: SizeTrackingLruCache::new_with_default_metrics_registry("bad_block".into(), cap),
         }
     }
 
+    /// Returns true if the block CID is in the bad block cache.
+    /// Does not update LRU head position.
+    pub fn contains(&self, c: &Cid) -> bool {
+        self.cache.contains(&(*c).into())
+    }
+
     pub fn push(&self, c: Cid) {
         self.cache.push(c.into(), ());
     }
 
     /// Returns `Some` if the block CID is in bad block cache.
     /// This function does not update the head position of the `Cid` key.
     pub fn peek(&self, c: &Cid) -> Option<()> {
         self.cache.peek_cloned(&(*c).into())
     }
 }

src/beacon/drand.rs (1)

271-274: Use contains() for membership check to avoid locking + value compare.

You can simplify is_verified by checking key presence only; the round uniquely identifies the entry content.

-    fn is_verified(&self, entry: &BeaconEntry) -> bool {
-        let cache = self.verified_beacons.cache().read();
-        cache.peek(&entry.round()) == Some(entry)
-    }
+    fn is_verified(&self, entry: &BeaconEntry) -> bool {
+        self.verified_beacons.contains(&entry.round())
+    }

If there’s any possibility that the cached value for a round could differ from entry, keep the equality check.

src/shim/sector.rs (1)

22-22: GetSize for PoStProof — LGTM.

Accurately accounts for heap via proof_bytes. If fvm_shared adds additional heap fields in PoStProofV4 later, consider delegating to the inner type (if it ever implements GetSize) to avoid drift.

Also applies to: 381-385

src/shim/address.rs (1)

17-17: GetSize for Address — LGTM.

Returning 0 matches the zero-heap expectation for this wrapper.

Consider a brief comment explaining why heap size is 0 (helps future maintainers avoid double-counting).

Also applies to: 263-267

src/blocks/tipset.rs (1)

149-156: Derive GetSize for Tipset and annotate headers — LGTM.

The size_fn for nunny::Vec ensures precise heap tracking for the headers collection.

If you want similar visibility for FullTipset memory, consider deriving GetSize there too:

-#[derive(Debug, Clone, Eq)]
+#[derive(Debug, Clone, Eq, GetSize)]
 pub struct FullTipset {
-    blocks: NonEmpty<Block>,
+    #[get_size(size_fn = nunny_vec_heap_size_helper)]
+    blocks: NonEmpty<Block>,
     // key is lazily initialized via `fn key()`.
     key: OnceLock<TipsetKey>,
 }

src/utils/get_size/mod.rs (1)

54-58: MSRV-safe byte math and minor doc tweak for BigInt helper.

The estimate is fine. If MSRV predates div_ceil on integers, prefer the add-then-divide pattern. Also clarify the comment to reflect that this measures magnitude bytes (sign/overhead ignored).

-// This is a rough estimation. Use `b.allocation_size()`
-// once https://github.com/rust-num/num-bigint/pull/333 is accepted and released.
+// Rough estimation of magnitude bytes. Ignores sign/struct overhead.
+// Switch to `b.allocation_size()` once
+// https://github.com/rust-num/num-bigint/pull/333 is released.

-pub fn big_int_heap_size_helper(b: &BigInt) -> usize {
-    b.bits().div_ceil(8) as usize
-}
+pub fn big_int_heap_size_helper(b: &BigInt) -> usize {
+    // MSRV-safe: (bits + 7) / 8
+    let bits = b.bits() as usize;
+    (bits.saturating_add(7)) / 8
+}

src/blocks/vrf_proof.rs (1)

30-34: Optional: derive GetSize for VRFProof and drop the manual impl.

Keeps consistency with other types deriving GetSize and reduces boilerplate.

-#[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Default, Serialize, Deserialize, Hash)]
+#[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Default, Serialize, Deserialize, Hash, GetSize)]
 pub struct VRFProof(#[serde(with = "serde_byte_array")] pub Vec<u8>);

-impl GetSize for VRFProof {
-    fn get_heap_size(&self) -> usize {
-        self.0.get_heap_size()
-    }
-}

src/shim/message.rs (1)

44-49: Remove dead commented code.

Stale skeleton impl adds noise; derive is already in use.

-// impl GetSize for Message{
-//     fn get_heap_size(&self) -> usize {
-
-//     }
-// }

src/utils/cache/lru.rs (2)

127-134: New contains() is correct and concurrency-safe

Read lock with LruCache::contains is appropriate and does not update recency. Consider a short doc comment noting that contains does not affect LRU order, so callers shouldn’t rely on it for touch semantics.

---

186-191: Metric rename to "cache_..._size": ensure consistency and update dashboards

The size metric now uses the "cache_" prefix. Please:

• Verify dashboards/alerts scraped the new name.
• Consider aligning len/cap metrics with the same "cache_" prefix for consistency (optional).

src/blocks/header.rs (2)

201-233: Manual GetSize impl: clarify exclusion rationale and potential undercount

• You explicitly exclude CIDs (state_root, message_receipts, messages). If this is to avoid relying on a non-existent GetSize impl for cid::Cid, please replace the comment “derive macro does not compile for some reason” with a concrete note so future readers don’t regress this.
• If cid::Cid (or its multihash) allocates on the heap in your version, excluding it here will undercount. If the multihash is stack/inlined for 32-byte digests in your build, the exclusion is fine. Please confirm the intended behavior.

Nit: big_int_heap_size_helper(b) accounts only for payload bits; it doesn’t include Vec/limb overhead. If you want a slightly less optimistic estimate, consider adding a small constant overhead per BigInt, but that’s optional.

Example comment tweak:

-// The derive macro does not compile for some reason
+// Note: We can’t derive GetSize here because some external types (e.g., cid::Cid)
+// don’t implement it. We manually sum known heap-allocating fields and exclude CIDs,
+// which are treated as fixed-size/inlined for our purposes.

Would you like me to search the repo to verify whether there’s any GetSize impl for cid::Cid (or wrappers), and whether we should account for it here?

---

237-241: Deriving GetSize and ignoring OnceLock is reasonable—confirm CID treatment is consistent

Deriving GetSize on CachingBlockHeader while ignoring the cached CID avoids double-counting and also sidesteps the lack of a GetSize impl for Cid. This matches the manual exclusion in RawBlockHeader, but it means CID heap (if any) is never counted. If that’s intentional, consider adding a brief comment to the field.

Optional:

 #[derive(Debug, GetSize)]
 pub struct CachingBlockHeader {
     uncached: RawBlockHeader,
-    #[get_size(ignore)]
+    // CID heap is excluded; tracked via RawBlockHeader fields as needed (currently excluded too).
+    #[get_size(ignore)]
     cid: OnceLock<Cid>,
     has_ever_been_verified_against_any_signature: AtomicBool,
 }

If you’d like, I can also scan for a GetSize impl on TipsetKey and Signature to ensure size tracking stays sound across caches.

src/chain/store/chain_store.rs (2)

560-567: Constructor and metrics wiring LGTM

Using new_with_default_metrics_registry is consistent with similar caches. Consider standardizing the cache name (e.g., “msgs_in_tipset”) if you want all cache metrics to follow a consistent pluralization.

---

591-598: NonZeroUsize env parsing is fine; optional: guard invalid values explicitly

Current parsing silently falls back to DEFAULT. Optionally log or trace when parsing fails or when the value is zero/invalid to aid ops.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: hanabi1224
PR: ChainSafe/forest#5841
File: src/utils/get_size/mod.rs:10-10
Timestamp: 2025-07-17T15:21:40.753Z
Learning: The get_size2 crate's GetSize trait provides default implementations: get_stack_size() uses std::mem::size_of, get_heap_size() returns 0, and get_size() returns their sum. An empty impl like `impl GetSize for MyType {}` is valid and uses these defaults, making it suitable for simple wrapper types that don't allocate heap memory.

src/chain/store/index.rs (1)

60-66: Cache-disabled env still performs writes and increments miss metrics — gate both

When the cache is disabled, we shouldn’t populate it or emit hit/miss metrics. Gate the push and miss counter on cache_enabled to avoid memory usage and misleading metrics when caching is disabled.

Apply this change:

-        let ts_opt = Tipset::load(&self.db, tsk)?.map(Arc::new);
-        if let Some(ts) = &ts_opt {
-            self.ts_cache.push(tsk.clone(), ts.clone());
-            metrics::LRU_CACHE_MISS
-                .get_or_create(&metrics::values::TIPSET)
-                .inc();
-        }
+        let ts_opt = Tipset::load(&self.db, tsk)?.map(Arc::new);
+        if let Some(ts) = &ts_opt {
+            if cache_enabled {
+                self.ts_cache.push(tsk.clone(), ts.clone());
+                metrics::LRU_CACHE_MISS
+                    .get_or_create(&metrics::values::TIPSET)
+                    .inc();
+            }
+        }

.clippy.toml (1)

47-54: Ban coverage for LruCache constructors — LGTM; addresses earlier review

Blocking LruCache::new and LruCache::with_hasher aligns with the migration and closes the loophole called out earlier. Nice.

.clippy.toml (1)

56-62: Ban unbounded constructors — good; unify reason wording and capitalization

The bans for unbounded and unbounded_with_hasher are correct. Minor consistency nit: standardize the reason string (capitalize LRU and simplify wording).

Apply this small wording cleanup:

- reason = """Avoid unbounded lru cache for potential memory leak, use SizeTrackingLruCache instead."""
+ reason = """Avoid unbounded LRU caches; use SizeTrackingLruCache instead."""

Repeat the same change for both entries (Lines 57 and 61).

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: hanabi1224
PR: ChainSafe/forest#5841
File: src/utils/get_size/mod.rs:10-10
Timestamp: 2025-07-17T15:21:40.753Z
Learning: The get_size2 crate's GetSize trait provides default implementations: get_stack_size() uses std::mem::size_of, get_heap_size() returns 0, and get_size() returns their sum. An empty impl like `impl GetSize for MyType {}` is valid and uses these defaults, making it suitable for simple wrapper types that don't allocate heap memory.