fix: return reverted field of logs set to true if tipset was reverted#7172
Draft
akaladarshi wants to merge 7 commits into
Draft
fix: return reverted field of logs set to true if tipset was reverted#7172akaladarshi wants to merge 7 commits into
akaladarshi wants to merge 7 commits into
Conversation
Contributor
|
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)
WalkthroughImplements Changeseth_subscribe reorg-removed logs pipeline
Sequence Diagram(s)sequenceDiagram
participant ChainHeadChanges
participant RunLogsFeed
participant EthLogsForHeadChange
participant CollectEventsFromMessages
participant LogsFeed
participant SpawnLogs
participant LogMatches
participant SubscriptionSink
ChainHeadChanges->>RunLogsFeed: emit apply or revert head change
RunLogsFeed->>EthLogsForHeadChange: process head change
EthLogsForHeadChange->>CollectEventsFromMessages: collect events with revert status
CollectEventsFromMessages-->>EthLogsForHeadChange: return logs with removed flag
EthLogsForHeadChange-->>RunLogsFeed: return log batch
RunLogsFeed->>LogsFeed: broadcast log batch
LogsFeed-->>SpawnLogs: receive shared batch
SpawnLogs->>LogMatches: filter by EthFilterSpec
LogMatches-->>SpawnLogs: matched logs
SpawnLogs->>SubscriptionSink: forward matched logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
akaladarshi
force-pushed
the
akaladarshi/return-logs-reverted
branch
from
acbad44 to
7141458
Compare
akaladarshi
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
akaladarshi
force-pushed
the
akaladarshi/return-logs-reverted
branch
2 times, most recently
from
1d0b496 to
e1043a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 3318-3320: The `eth_getFilterLogs` method is incorrectly using the
`poll_event_filter` helper which mutates internal state by updating
`seen_positions` at lines 3353-3361. This makes the method non-idempotent,
causing repeated calls to return different results and breaking subsequent
`eth_getFilterChanges` calls. Instead of calling `poll_event_filter`, directly
collect the filter's full result set from the canonical chain without updating
any poll state. Keep the `poll_event_filter` usage only in
`eth_getFilterChanges` where state mutation is appropriate. Extract the core
logic for collecting events from the filter without the state mutation part.
In `@src/state_manager/state_computation.rs`:
- Around line 70-84: The load_executed_tipset_with_receipt method is bypassing
the stale-cache validation and removal guard that is implemented in the
load_executed_tipset_with_cache method. This causes stale cached ExecutedTipset
entries to be returned after head reset or garbage collection events. Apply the
same cache validity checking and invalidation logic that exists in
load_executed_tipset_with_cache (Lines 99-107) to the
load_executed_tipset_with_receipt method before or after the get_or_insert_async
call to ensure stale entries are properly removed from the cache in the
receipt-specific flow.
🪄 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: 9018da5a-761b-49ea-8e98-3507519807fa
📒 Files selected for processing (14)
CHANGELOG.mdsrc/daemon/mod.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/filter/event.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/eth/pubsub.rssrc/rpc/methods/sync.rssrc/rpc/mod.rssrc/state_manager/state_computation.rssrc/tool/offline_server/server.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rssrc/tool/subcommands/api_cmd/stateful_tests.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Comment on lines
+70
to
+84
| pub async fn load_executed_tipset_with_receipt( | ||
| &self, | ||
| msg_ts: &Tipset, | ||
| receipt_ts: &Tipset, | ||
| ) -> anyhow::Result<ExecutedTipset> { | ||
| self.cache | ||
| .get_or_insert_async(msg_ts.key(), async move { | ||
| self.load_executed_tipset_inner( | ||
| msg_ts, | ||
| Some(receipt_ts), | ||
| Self::rpc_state_recompute_policy(), | ||
| ) | ||
| .await | ||
| }) | ||
| .await |
Contributor
There was a problem hiding this comment.
Receipt-specific executed-tipset loader skips cache validity checks used by the main RPC path.
At Line 75, this path directly uses get_or_insert_async and bypasses the stale-cache validation/removal guard used in load_executed_tipset_with_cache (Line 99-Line 107). That can return stale cached ExecutedTipset entries after head reset/GC in this new non-canonical receipt flow.
Suggested fix
pub async fn load_executed_tipset_with_receipt(
&self,
msg_ts: &Tipset,
receipt_ts: &Tipset,
) -> anyhow::Result<ExecutedTipset> {
+ // Keep cache validity behavior aligned with load_executed_tipset_with_cache.
+ if msg_ts.epoch() >= self.heaviest_tipset().epoch()
+ && let Some(cached) = self.cache.get(msg_ts.key())
+ {
+ if StateTree::new_from_root(self.db(), &cached.state_root).is_ok() {
+ return Ok(cached);
+ } else {
+ self.cache.remove(msg_ts.key());
+ }
+ }
+
self.cache
.get_or_insert_async(msg_ts.key(), async move {
self.load_executed_tipset_inner(
msg_ts,
Some(receipt_ts),
Self::rpc_state_recompute_policy(),
)
.await
})
.await
}🤖 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/state_manager/state_computation.rs` around lines 70 - 84, The
load_executed_tipset_with_receipt method is bypassing the stale-cache validation
and removal guard that is implemented in the load_executed_tipset_with_cache
method. This causes stale cached ExecutedTipset entries to be returned after
head reset or garbage collection events. Apply the same cache validity checking
and invalidation logic that exists in load_executed_tipset_with_cache (Lines
99-107) to the load_executed_tipset_with_receipt method before or after the
get_or_insert_async call to ensure stale entries are properly removed from the
cache in the receipt-specific flow.
3 tasks
akaladarshi
force-pushed
the
akaladarshi/return-logs-reverted
branch
from
a967d70 to
33c3fb8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| }; | ||
|
|
||
| self.load_executed_tipset_with_cache(ts, policy).await | ||
| // https://github.com/ChainSafe/forest/issues/7118 |
Collaborator
Author
There was a problem hiding this comment.
Removed, It was left over from the rebase conflict.
akaladarshi
force-pushed
the
akaladarshi/return-logs-reverted
branch
from
33c3fb8 to
9a4b513
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/state_manager/state_computation.rs (1)
63-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign receipt-path cache reads with stale-cache invalidation logic
load_executed_tipset_with_receiptbypasses the stale-cache validation used byload_executed_tipset_with_cache, so it can return invalid cached entries after head reset/GC. Please apply the same pre-read validation/removal guard here beforeget_or_insert_async(Line 63-Line 77).Suggested patch
pub async fn load_executed_tipset_with_receipt( &self, msg_ts: &Tipset, receipt_ts: &Tipset, ) -> anyhow::Result<ExecutedTipset> { + // Keep cache-validity behavior consistent with load_executed_tipset_with_cache. + if msg_ts.epoch() >= self.heaviest_tipset().epoch() + && let Some(cached) = self.cache.get(msg_ts.key()) + { + if StateTree::new_from_root(self.db(), &cached.state_root).is_ok() { + return Ok(cached); + } else { + self.cache.remove(msg_ts.key()); + } + } + self.cache .get_or_insert_async(msg_ts.key(), async move { self.load_executed_tipset_inner( msg_ts, Some(receipt_ts), Self::rpc_state_recompute_policy(), ) .await }) .await }🤖 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/state_manager/state_computation.rs` around lines 63 - 77, The load_executed_tipset_with_receipt method does not perform the same stale-cache validation that load_executed_tipset_with_cache applies before reading from cache, which allows invalid cached entries to persist after head reset or garbage collection operations. Apply the same pre-read validation and removal guard logic to load_executed_tipset_with_receipt before the get_or_insert_async call to ensure stale cache entries are properly invalidated before retrieval.
🤖 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.
Duplicate comments:
In `@src/state_manager/state_computation.rs`:
- Around line 63-77: The load_executed_tipset_with_receipt method does not
perform the same stale-cache validation that load_executed_tipset_with_cache
applies before reading from cache, which allows invalid cached entries to
persist after head reset or garbage collection operations. Apply the same
pre-read validation and removal guard logic to load_executed_tipset_with_receipt
before the get_or_insert_async call to ensure stale cache entries are properly
invalidated before retrieval.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4aa9f30f-5851-46ce-8802-e0a7032902b1
📒 Files selected for processing (14)
CHANGELOG.mdsrc/daemon/mod.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/filter/event.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/eth/pubsub.rssrc/rpc/methods/sync.rssrc/rpc/mod.rssrc/state_manager/state_computation.rssrc/tool/offline_server/server.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rssrc/tool/subcommands/api_cmd/stateful_tests.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
✅ Files skipped from review due to trivial changes (4)
- src/tool/offline_server/server.rs
- src/tool/subcommands/api_cmd/generate_test_snapshot.rs
- CHANGELOG.md
- src/rpc/methods/sync.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/tool/subcommands/api_cmd/test_snapshot.rs
- src/daemon/mod.rs
- src/rpc/methods/chain.rs
- src/rpc/methods/eth/filter/event.rs
- src/rpc/mod.rs
- src/rpc/methods/eth.rs
- src/tool/subcommands/api_cmd/stateful_tests.rs
- src/rpc/methods/eth/pubsub.rs
- src/rpc/methods/eth/filter/mod.rs
| }); | ||
| let (mut ws_stream, subscription_id) = | ||
| open_eth_subscription(&client, SubscriptionKind::Logs, Some(filter)).await?; | ||
| let contract = EthAddress::from_filecoin_address(&tx.to)?; |
Member
There was a problem hiding this comment.
this test is huge and it's unclear to me what it does. Tests must be easy to follow, either with clear code, e.g., by extracting certain chunks to separate functions or at least with meaningful, commented blocks. I also see there are tons of magical timeouts, each with different value. Why? How long is this method supposed to run, realistically?
Collaborator
Author
There was a problem hiding this comment.
It basically test the response of EthSubscription API for Logs under the different kind of filters.
filter_all, filter_wrong, filter_topic, filter_wild, filter_constrained.
But yeah test got too complicated, will refactor it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #7096
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
eth_subscribelogs during reorgs so reorg-reverted logs are re-emitted withremoved: true, ordered before logs from the replacing chain head.eth_getFilterLogs/eth_getFilterChangesto return only newly seen events by tracking delivered log positions for each filter.eth_subscribecoverage into a filter-matrix validating address/topic wildcard behavior (including trailing wildcards) and correct inclusion/exclusion across concurrent subscriptions.