feat: add eth_block_cache to improve performance of a few Eth RPC method #6289
Conversation
WalkthroughThis PR introduces caching optimizations and cleaner type handling across the codebase. Key changes include: a per-block ETH cache in the RPC layer to improve block fetch performance; a heaviest-tipset in-memory cache in ChainStore with RwLock for concurrent reads; GetSize trait implementations for Ethereum types to enable heap-size accounting; systematic refactoring to use the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/chain/store/chain_store.rs (1)
227-238: Consider populating the cache on miss.The fallback path (lines 231-237) loads the tipset from the provider but doesn't update the cache. This means repeated calls before the first
set_heaviest_tipsetwill keep hitting the provider.For a frequently called method, consider populating the cache on miss:
pub fn heaviest_tipset(&self) -> Tipset { if let Some(ts) = &*self.heaviest_tipset_cache.read() { return ts.clone(); } let tsk = self .heaviest_tipset_key_provider .heaviest_tipset_key() .unwrap_or_else(|_| TipsetKey::from(nunny::vec![*self.genesis_block_header.cid()])); - self.chain_index + let ts = self.chain_index .load_required_tipset(&tsk) - .expect("failed to load heaviest tipset") + .expect("failed to load heaviest tipset"); + *self.heaviest_tipset_cache.write() = Some(ts.clone()); + ts }This would eliminate redundant loads during startup or after restarts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/dictionary.txt(1 hunks)docs/docs/users/reference/env_variables.md(1 hunks)src/beacon/drand.rs(3 hunks)src/chain/store/chain_store.rs(6 hunks)src/cli/subcommands/chain_cmd/list.rs(2 hunks)src/libp2p/service.rs(2 hunks)src/rpc/methods/eth.rs(9 hunks)src/rpc/methods/eth/types.rs(4 hunks)src/shim/message.rs(0 hunks)src/state_migration/common/mod.rs(1 hunks)src/state_migration/common/state_migration.rs(2 hunks)src/tool/subcommands/state_compute_cmd.rs(2 hunks)
💤 Files with no reviewable changes (1)
- src/shim/message.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-07-17T15:21:40.753Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 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.
Applied to files:
src/rpc/methods/eth/types.rssrc/rpc/methods/eth.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/state_migration/common/state_migration.rssrc/state_migration/common/mod.rs
📚 Learning: 2025-08-18T03:09:47.932Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Applied to files:
src/chain/store/chain_store.rs
📚 Learning: 2025-10-17T14:24:47.046Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6167
File: src/tool/subcommands/state_compute_cmd.rs:89-91
Timestamp: 2025-10-17T14:24:47.046Z
Learning: In `src/tool/subcommands/state_compute_cmd.rs`, when using `ReadOpsTrackingStore` to generate minimal snapshots, `HEAD_KEY` should be written to `db.tracker` (not `db` itself) before calling `export_forest_car()`, because the export reads from the tracker MemoryDB which accumulates only the accessed data during computation.
Applied to files:
src/chain/store/chain_store.rs
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
src/chain/store/chain_store.rs
🧬 Code graph analysis (4)
src/rpc/methods/eth/types.rs (3)
src/rpc/methods/eth.rs (3)
get_heap_size(141-143)get_heap_size(169-171)get_heap_size(183-185)src/shim/econ.rs (1)
get_heap_size(40-42)src/shim/executor.rs (2)
get_heap_size(138-144)get_heap_size(348-357)
src/state_migration/common/state_migration.rs (2)
src/beacon/drand.rs (1)
new(245-263)src/state_migration/common/mod.rs (1)
new(32-39)
src/chain/store/chain_store.rs (1)
src/chain_sync/bad_block_cache.rs (1)
default(20-22)
src/rpc/methods/eth.rs (3)
src/utils/cache/lru.rs (2)
cache(99-101)new_with_metrics(83-87)src/utils/get_size/mod.rs (1)
big_int_heap_size_helper(56-58)src/rpc/methods/eth/types.rs (5)
get_heap_size(123-125)get_heap_size(407-409)new(73-83)new(423-428)new(435-437)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Bootstrap checks - Lotus
- GitHub Check: Devnet checks
- GitHub Check: Diff snapshot export checks
- GitHub Check: V2 snapshot export checks
- GitHub Check: Wallet tests
- GitHub Check: Calibnet eth mapping check
- GitHub Check: Forest CLI checks
- GitHub Check: Calibnet check
🔇 Additional comments (23)
docs/dictionary.txt (1)
31-31: LGTM!The addition of "Eth" to the dictionary is correctly placed alphabetically and aligns with the Ethereum-related changes in this PR.
docs/docs/users/reference/env_variables.md (1)
54-54: LGTM!The documentation for
FOREST_ETH_BLOCK_CACHE_SIZEis clear and follows the established format. The default value of 500 is reasonable for a block cache.src/libp2p/service.rs (1)
38-38: LGTM!The refactoring to use
nonzero!(20usize)is cleaner and more concise than the previous manual construction. The macro provides compile-time guarantees while maintaining identical runtime behavior.Also applies to: 210-210
src/state_migration/common/mod.rs (1)
136-136: LGTM!The test code refactoring to use the
nonzero!macro improves readability while maintaining identical test behavior.Also applies to: 140-140
src/tool/subcommands/state_compute_cmd.rs (1)
18-18: LGTM!The refactoring of the default value to use
nonzero!(1usize)is cleaner and more maintainable while preserving the same behavior.Also applies to: 120-120
src/cli/subcommands/chain_cmd/list.rs (1)
8-8: LGTM!The refactoring to use
nonzero!(30usize)for the default count value is cleaner and aligns with the project-wide refactoring effort.Also applies to: 29-29
src/state_migration/common/state_migration.rs (2)
7-11: LGTM!The consolidated imports and addition of the
nonzeromacro import improve code organization and align with the refactoring effort.Also applies to: 16-16
82-82: LGTM!The refactoring to use
nonzero!(10_000usize)is more concise and eliminates the need for the.expect("infallible")call while maintaining the same cache size.src/beacon/drand.rs (1)
22-22: LGTM!The refactoring to make
CACHE_SIZEaNonZeroUsizeconstant using thenonzero!macro improves type safety and eliminates the need for runtime wrapping. This is a clean improvement to the code.Also applies to: 247-247, 260-260
src/chain/store/chain_store.rs (4)
34-35: LGTM on imports.The
nonzero_ext::nonzeromacro andRwLockimports are appropriate for the new caching functionality.
68-70: LGTM on cache field and initialization.Using
Arc<RwLock<Option<Tipset>>>is appropriate for a read-heavy cache with occasional writes. The cache will warm up naturally on first access toheaviest_tipset().Also applies to: 138-138
150-158: LGTM on set_heaviest_tipset update.The cache is correctly updated after persisting to the provider. The ordering ensures durability before caching.
610-617: LGTM on nonzero! macro usage.Clean usage of the
nonzero!macro for compile-time non-zero guarantees, consistent with the pattern used elsewhere in the codebase.src/rpc/methods/eth/types.rs (3)
8-8: LGTM on GetSize for EthBytes.Deriving
GetSizeonEthBytescorrectly accounts for theVec<u8>heap allocation. The derive macro will properly track the underlying vector's heap size.Also applies to: 20-38
122-126: LGTM on GetSize for EthAddress.Returning 0 for heap size is correct since
ethereum_types::Address(H160) is a fixed-size[u8; 20]stored inline with no heap allocation. Based on learnings about the GetSize trait's default implementations.
406-410: LGTM on GetSize for EthHash.Returning 0 for heap size is correct since
ethereum_types::H256is a fixed-size[u8; 32]stored inline with no heap allocation.src/rpc/methods/eth.rs (7)
56-56: LGTM on new imports.The imports for
SizeTrackingLruCache,GetSize,NonZeroUsize, andnonzeroare appropriately added to support the new caching functionality.Also applies to: 59-59, 69-69, 72-72, 76-76
140-144: LGTM on GetSize for EthBigInt.Using
big_int_heap_size_helpercorrectly accounts for the BigInt's heap allocation, consistent with the pattern used insrc/shim/econ.rs.
168-186: LGTM on GetSize for Nonce and Bloom.Both types wrap fixed-size arrays (
H64is 8 bytes,ethereum_types::Bloomis 256 bytes) with no heap allocation, so returning 0 for heap size is correct.
206-206: LGTM on GetSize derives.Deriving
GetSizeonEthUint64,Transactions,Block, andApiEthTxenables accurate heap size tracking for theSizeTrackingLruCache. The derive macro will correctly propagate size calculations through nested types.Also applies to: 474-474, 506-506, 551-551
1417-1425: LGTM on static cache initialization.The
LazyLockpattern ensures thread-safe single initialization. The 500-block default is reasonable, and theFOREST_ETH_BLOCK_CACHE_SIZEenvironment variable follows the project's pattern for configurable cache sizes.
1427-1487: Solid caching implementation with full transaction storage.The approach of always caching blocks with
Transactions::Fulland converting toTransactions::Hashon-the-fly is a good design choice - it maximizes cache utility by storing the richer representation.One minor note: the block is cloned from cache and then potentially mutated (line 1483). This is correct since
get_clonedreturns an owned copy, but worth noting for clarity.
1482-1485: LGTM on transaction format conversion.The let-chain pattern is idiomatic and efficient. Converting from
Transactions::FulltoTransactions::Hashon-demand avoids duplicating cache entries.
| let (state_root, msgs_and_receipts) = execute_tipset(&data, &tipset).await?; | ||
| static ETH_BLOCK_CACHE: LazyLock<SizeTrackingLruCache<CidWrapper, Block>> = | ||
| LazyLock::new(|| { | ||
| const DEFAULT_CACHE_SIZE: NonZeroUsize = nonzero!(500usize); |
There was a problem hiding this comment.
What's the expected memory footprint of a maxed-out eth block cache?
There was a problem hiding this comment.
Based on the metrics, a rough estimation would be 265108 * 500 / 3 = 44184667 ~= 42MiB
# HELP cache_eth_block_14_size_bytes Size of LruCache eth_block_14 in bytes
# TYPE cache_eth_block_14_size_bytes gauge
# UNIT cache_eth_block_14_size_bytes bytes
cache_eth_block_14_size_bytes 265108
# HELP eth_block_14_len Length of LruCache eth_block_14
# TYPE eth_block_14_len gauge
eth_block_14_len 3
# HELP eth_block_14_cap Capacity of LruCache eth_block_14
# TYPE eth_block_14_cap gauge
eth_block_14_cap 500
Summary of changes
Fix/mitigate #5633 and #6149
Changes introduced in this pull request:
eth_block_cacheto improve performance of a few Eth RPC methodfn heaveist_tipsetCalibnet
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.