fix eth transaction receipt cache - #7420
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesTransaction receipt cache
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant eth_getTransactionReceipt
participant cache_eth_transaction_receipt
participant chain_lookup
eth_getTransactionReceipt->>cache_eth_transaction_receipt: request receipt by tx_hash and limit
cache_eth_transaction_receipt->>chain_lookup: compare head and compute receipt when needed
chain_lookup-->>cache_eth_transaction_receipt: receipt or not found within limit
cache_eth_transaction_receipt-->>eth_getTransactionReceipt: receipt filtered by finality and lookback
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
2966-2969: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
is_finalizedrefetches the head instead of reusing the caller'shead.
get_eth_transaction_receipt_with_cachealready fetchesheadat line 2986 and passeshead_keyinto the cache entry, butis_finalizedindependently callsctx.chain_store().heaviest_tipset()again. If the chain advances between these two fetches, the storedhead_keyand the head used to computefinalizedcan come from different tipsets, which is a subtle inconsistency in the cached record. Passing the already-known head epoch intois_finalizedavoids the redundant lookup and keeps the record internally consistent.♻️ Proposed refactor
-fn is_finalized(ctx: &Ctx, receipt_epoch: ChainEpoch) -> bool { - let head_epoch = ctx.chain_store().heaviest_tipset().epoch(); - receipt_epoch <= head_epoch - ctx.chain_config().policy.chain_finality -} +fn is_finalized(ctx: &Ctx, head_epoch: ChainEpoch, receipt_epoch: ChainEpoch) -> bool { + receipt_epoch <= head_epoch - ctx.chain_config().policy.chain_finality +}Then call it with the already-fetched
head.epoch()at the call site instead of re-deriving it inside the closure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth.rs` around lines 2966 - 2969, Update is_finalized to accept the caller’s already-fetched head epoch instead of reading ctx.chain_store().heaviest_tipset() internally, and pass head.epoch() from get_eth_transaction_receipt_with_cache when computing finalized. Preserve the existing finality comparison and cache behavior while eliminating the second head lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3016-3018: Update the receipt miss check near
eth_getTransactionReceiptLimited so it treats only positive limits as bounded,
matching max_lookback_epoch_inclusive; use the limit > 0 predicate instead of
checking limit.is_some(), ensuring Some(-1) remains unbounded and does not
return Uncacheable::NotFoundWithinLimit.
- Around line 2989-2995: Replace the separate CACHE.peek and CACHE.remove logic
in the stale-entry block with Cache::remove_if, using the existing !e.finalized
&& e.head_key != head_key predicate so the conditional removal is atomic.
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 2966-2969: Update is_finalized to accept the caller’s
already-fetched head epoch instead of reading
ctx.chain_store().heaviest_tipset() internally, and pass head.epoch() from
get_eth_transaction_receipt_with_cache when computing finalized. Preserve the
existing finality comparison and cache behavior while eliminating the second
head lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0c915d64-6b9a-4157-985b-b47600e7329d
📒 Files selected for processing (1)
src/rpc/methods/eth.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
369054b to
fe066ba
Compare
fe066ba to
650a10d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Summary of changes
Changes introduced in this pull request:
Not Foundresults for full searches. Limited searches that fail to find transaction receipts are not cached, since the receipt could still exist beyond the limit.Reference issue to close (if applicable)
Closes #7330
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit