fix: eth new block filter stateful test#7392
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)
WalkthroughThe stateful API test replaces WebSocket tipset waiting with bounded chain-head epoch polling. Block filter verification now uses one controlled flow across several epochs, accepts empty or changed hash sets, and removes the outer retry loop. ChangesBlock filter verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tool/subcommands/api_cmd/stateful_tests.rs (2)
882-896: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winComment mischaracterizes why a block filter poll could be unchanged;
ensure!lacks a diagnostic message.
eth_newBlockFilter/eth_getFilterChangesreports new block hashes, not filtered event/log hashes — the "epoch without events" framing on lines 882-884 seems copied from log-filter reasoning. The actual reason a poll can be unchanged is a null round (no block produced that epoch), not "executed events." Consider rewording so future readers don't misapply this reasoning to event/log filter tests.Separately, the
ensure!on line 894 has no message, so a failure here will only surface the raw boolean expression rather than something like "block filter hashes did not change across 3 epochs," making CI failures harder to diagnose quickly.✏️ Suggested comment/message tweak
- // The filter derives its hashes from executed events, so an epoch - // without events legitimately leaves the poll unchanged, allow a - // few more epochs before declaring the filter stuck. + // A given epoch can be a "null round" with no block produced, + // legitimately leaving the poll unchanged; allow a few more + // epochs before declaring the filter stuck. let mut hashes = Vec::new(); for _ in 0..3 { wait_next_epoch(client).await?; hashes = poll().await?; verify_hashes(&hashes).await?; if hashes != prev_hashes { break; } } - anyhow::ensure!((prev_hashes.is_empty() && hashes.is_empty()) || prev_hashes != hashes); + anyhow::ensure!( + (prev_hashes.is_empty() && hashes.is_empty()) || prev_hashes != hashes, + "block filter hashes did not change across 3 epochs: prev={prev_hashes:?}, current={hashes:?}" + );🤖 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/tool/subcommands/api_cmd/stateful_tests.rs` around lines 882 - 896, Update the explanatory comment in the block-filter polling loop around wait_next_epoch and poll to describe unchanged hashes as resulting from null rounds with no block produced, not epochs without events. Add a diagnostic message to the anyhow::ensure! assertion identifying that block filter hashes did not change across the polling attempts, while preserving its existing condition.
198-210: 🩺 Stability & Availability | 🔵 TrivialCumulative worst-case wait can reach ~9 minutes.
wait_next_epochis bounded at 180s, butprocess_filter(lines 886-893) can call it up to 3 times, so a stuck-but-not-erroring chain could keep the test running for up to ~9 minutes before failing. Worth confirming this fits the CI test-timeout budget for stateful tests.🤖 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/tool/subcommands/api_cmd/stateful_tests.rs` around lines 198 - 210, Review the cumulative timeout between wait_next_epoch and process_filter, where up to three 180-second waits can extend a stalled stateful test to roughly nine minutes. Adjust the timeout or retry behavior in these symbols so the total worst-case duration fits the CI stateful-test budget, while preserving successful epoch advancement handling.
🤖 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/tool/subcommands/api_cmd/stateful_tests.rs`:
- Around line 899-911: Update the cleanup flow around process_filter so an
uninstall failure cannot replace an earlier process_filter error. Preserve and
return the original result when it is already an error, while still propagating
uninstall failures when processing succeeded; retain the existing filter removal
validation.
---
Nitpick comments:
In `@src/tool/subcommands/api_cmd/stateful_tests.rs`:
- Around line 882-896: Update the explanatory comment in the block-filter
polling loop around wait_next_epoch and poll to describe unchanged hashes as
resulting from null rounds with no block produced, not epochs without events.
Add a diagnostic message to the anyhow::ensure! assertion identifying that block
filter hashes did not change across the polling attempts, while preserving its
existing condition.
- Around line 198-210: Review the cumulative timeout between wait_next_epoch and
process_filter, where up to three 180-second waits can extend a stalled stateful
test to roughly nine minutes. Adjust the timeout or retry behavior in these
symbols so the total worst-case duration fits the CI stateful-test budget, while
preserving successful epoch advancement handling.
🪄 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: 8355bc2e-2c62-43c7-b9e9-6920f547e60a
📒 Files selected for processing (1)
src/tool/subcommands/api_cmd/stateful_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 21 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
2 participants
Summary of changes
Changes introduced in this pull request:
EthNewBlockFilterrelied on theChainNotifyand was failing if the tipset reverted was returned in the Chain Notify responseChainHeadto report a new headReference issue to close (if applicable)
Closes #7385
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Summary