fix(rpc): improve performance of eth_GetBlockByNumber#6189
Conversation
WalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
| if let Ok(receipts) = Receipt::get_receipts(self.blockstore(), receipt_root) | ||
| && !receipts.is_empty() | ||
| { | ||
| self.receipt_event_cache_handler |
There was a problem hiding this comment.
Previously, without receipt_event_cache_handler cache being populated, the below function always calls compute_tipset_state to get message receipts
#[instrument(skip(self))]
pub async fn tipset_message_receipts(
self: &Arc<Self>,
tipset: &Arc<Tipset>,
) -> anyhow::Result<Vec<Receipt>> {
let key = tipset.key();
let ts = tipset.clone();
let this = Arc::clone(self);
self.receipt_event_cache_handler
.get_receipt_or_else(
key,
Box::new(move || {
Box::pin(async move {
let StateOutput { receipt_root, .. } = this
.compute_tipset_state(ts, NO_CALLBACK, VMTrace::NotTraced)
.await?;
trace!("Completed tipset state calculation");
Receipt::get_receipts(this.blockstore(), receipt_root)
})
}),
)
.await
}There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/state_manager/mod.rs (1)
218-233: Good refactor that aligns with PR objectives.The extraction of local variables improves readability, and the receipt caching logic correctly mirrors the pattern in
update_cache_with_state_output(lines 469-474). This should successfully avoid recomputation in RPC methods as intended.One minor observation: consider adding a trace-level log when
Receipt::get_receiptsfails, similar to other cache operations, to aid debugging without impacting performance.For example:
- if let Ok(receipts) = Receipt::get_receipts(self.blockstore(), receipt_root) - && !receipts.is_empty() - { - self.receipt_event_cache_handler - .insert_receipt(key, receipts); - } + match Receipt::get_receipts(self.blockstore(), receipt_root) { + Ok(receipts) if !receipts.is_empty() => { + self.receipt_event_cache_handler + .insert_receipt(key, receipts); + } + Err(e) => { + trace!("Failed to load receipts for cache population at epoch {}: {}", parent.epoch(), e); + } + _ => {} // Empty receipts, skip caching + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/state_manager/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/state_manager/mod.rs (3)
src/blocks/tipset.rs (2)
key(336-339)key(530-533)src/shim/executor.rs (2)
key(305-310)get_receipts(206-225)src/state_manager/cache.rs (3)
get_receipts(154-154)get_receipts(210-212)get_receipts(267-269)
⏰ 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). (6)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
🔇 Additional comments (1)
src/state_manager/mod.rs (1)
98-98: LGTM! Helpful debugging enhancement.Adding the
Debugderive to the publicStateEventsstruct improves observability without breaking compatibility.
|
@hanabi1224 did you check performance gain on your side? |
@LesnyRumcajs Just checked calibnet@3141000 with the curl command in #5633 |
I mean, the performance gain between current |
@LesnyRumcajs Just checked mainnet@5441650 main: |
LesnyRumcajs
left a comment
There was a problem hiding this comment.
LGTM! Might be worth a changelog entry.
@LesnyRumcajs changelog updated in #6192 given this has been in the merge queue. |
Summary of changes
Populate message receipt cache together with state output cache to avoid unnecessarily re-computing state in some RPC methods like
eth_getBlockByHashandeth_getBlockByNumber@AlexeyKrasnoperov please re-evaluate #5633 and #6149 once this is merged.
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Release Notes