refactor: simplify FiltersManager.start_download#450
Conversation
📝 WalkthroughWalkthroughRefactors filter sync initialization and idle-state detection: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dash-spv/src/sync/filters/pipeline.rs (1)
83-86: Consider adding a dedicated unit test foris_idle().The implementation is correct —
completed_batchesneed not be included because it represents consumed output (cleared byinit()), andbatch_trackersis always empty when both coordinator counts are zero (trackers are removed on completion or retry-exhaustion, and re-queued items incrementpending_count). However, the new method has no direct unit test in this file; all tests continue to callpipeline.coordinator.active_count()directly. A focused test would lock in the combinedactive + pendingsemantic.🧪 Suggested test skeleton
#[test] fn test_pipeline_is_idle_initial() { let pipeline = FiltersPipeline::new(); assert!(pipeline.is_idle()); } #[test] fn test_pipeline_is_idle_with_pending() { let mut pipeline = FiltersPipeline::new(); pipeline.init(0, 999); // pending_count > 0 → not idle assert!(!pipeline.is_idle()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/filters/pipeline.rs` around lines 83 - 86, Add focused unit tests exercising FiltersPipeline::is_idle(): create a FiltersPipeline via FiltersPipeline::new() and assert is_idle() is true initially, then create a pipeline, call init(0, 999) (or another call that increases pending_count) and assert is_idle() is false; include both scenarios in the test module so the combined active+pending semantic is verified rather than calling coordinator.active_count() directly.dash-spv/src/sync/filters/manager.rs (1)
103-110: LGTM —filter_pipeline.is_idle()omittingcompleted_batchesis safe here.
pipeline.is_idle()only inspects the coordinator's active/pending counts. In theory,filter_pipeline.completed_batchescould be non-empty while returningtrue(iftake_completed_batches()hasn't been called yet). In practice this window is unreachable:start_downloadis only called fromhandle_new_filter_headerswhen the state isWaitingForConnections | WaitForEvents, which is always preceded byclear_in_flight_state()(full pipeline reset). Even if reached,init()instart_downloadclearscompleted_batchesunconditionally.One minor gap: there is no dedicated unit test for
is_idle(). Given its role as the basis of thedebug_assert, a few edge-case tests (fresh manager, partially in-flight, afterclear_in_flight_state) would be valuable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/filters/manager.rs` around lines 103 - 110, is_idle currently checks active_batches, blocks_remaining, filters_matched, pending_batches and filter_pipeline.is_idle() but there are no unit tests ensuring its correctness across edge cases; add unit tests for Manager::is_idle that cover a fresh manager, a partially in-flight state (populate active_batches/pending_batches/filters_matched as appropriate and assert is_idle == false), and the state after calling clear_in_flight_state() (ensure completed_batches behavior does not make is_idle return false unexpectedly). Reference Manager::is_idle, FilterPipeline::is_idle, FilterPipeline::take_completed_batches, Manager::clear_in_flight_state, Manager::start_download and handle_new_filter_headers when designing tests to simulate the lifecycle transitions that could affect completed_batches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 103-110: is_idle currently checks active_batches,
blocks_remaining, filters_matched, pending_batches and filter_pipeline.is_idle()
but there are no unit tests ensuring its correctness across edge cases; add unit
tests for Manager::is_idle that cover a fresh manager, a partially in-flight
state (populate active_batches/pending_batches/filters_matched as appropriate
and assert is_idle == false), and the state after calling
clear_in_flight_state() (ensure completed_batches behavior does not make is_idle
return false unexpectedly). Reference Manager::is_idle, FilterPipeline::is_idle,
FilterPipeline::take_completed_batches, Manager::clear_in_flight_state,
Manager::start_download and handle_new_filter_headers when designing tests to
simulate the lifecycle transitions that could affect completed_batches.
In `@dash-spv/src/sync/filters/pipeline.rs`:
- Around line 83-86: Add focused unit tests exercising
FiltersPipeline::is_idle(): create a FiltersPipeline via FiltersPipeline::new()
and assert is_idle() is true initially, then create a pipeline, call init(0,
999) (or another call that increases pending_count) and assert is_idle() is
false; include both scenarios in the test module so the combined active+pending
semantic is verified rather than calling coordinator.active_count() directly.
- remove `init`/`extend_target` blocks in `start_download()` and just always call `init()` since there shouldn't be any inflight state when `start_download` is called. - add `is_idle()` helper to `FiltersManager` and `FiltersPipeline` to verify this via `debug_assert` - remove `FiltersPipeline::active_count()` since its dead code after removing the relevant log
xdustinface
force-pushed
the
refactor/filters-start-logic
branch
from
31e835f to
24f52f9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dash-spv/src/sync/filters/pipeline.rs (2)
84-85:is_idle()silently passes when completed-but-unconsumed batches exist.
completed_batchesis populated byreceive_with_data()aftercoordinator.receive()is called, so once all in-flight batches finish,coordinator.active_count() == 0 && coordinator.pending_count() == 0is satisfied whilecompleted_batchesmay still hold data thattake_completed_batches()hasn't consumed yet. Thedebug_assert!(self.is_idle())inmanager.rs::start_downloadwould not catch this case.In practice this isn't a bug —
clear_in_flight_state()creates a freshFiltersPipeline::new()(droppingcompleted_batches) andinit()also explicitly clearscompleted_batches. But including the field in the predicate would make the invariant airtight:🛡️ Proposed strengthening of `is_idle()`
pub(super) fn is_idle(&self) -> bool { - self.coordinator.active_count() == 0 && self.coordinator.pending_count() == 0 + self.coordinator.active_count() == 0 + && self.coordinator.pending_count() == 0 + && self.completed_batches.is_empty() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/filters/pipeline.rs` around lines 84 - 85, The is_idle() predicate currently only checks coordinator.active_count() and coordinator.pending_count(), but it should also ensure there are no unconsumed completed batches; update FiltersPipeline::is_idle() to include a check that self.completed_batches is empty (e.g., add && self.completed_batches.is_empty()), so the invariant used by manager.rs::start_download (debug_assert!(self.is_idle())) correctly fails when receive_with_data() has populated completed_batches that take_completed_batches() hasn't consumed yet.
326-333:test_is_idleonly covers two of the three lifecycle states.The test verifies
true(empty) andfalse(afterinit), but not the round-trip: after all filters are received andtake_completed_batches()is called,is_idle()should returntrueagain. Adding that leg would catch any future regression incoordinator.receive()bookkeeping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/filters/pipeline.rs` around lines 326 - 333, Extend the test_is_idle unit test to cover the third lifecycle state: after creating FiltersPipeline::new() and calling pipeline.init(...), simulate receiving all filters via the coordinator.receive(...) path (use the actual FiltersCoordinator/receive method used in the module) so the pipeline reaches completion, call pipeline.take_completed_batches(), and assert pipeline.is_idle() returns true again; refer to the existing test name test_is_idle and the methods FiltersPipeline::init, FiltersPipeline::is_idle, FiltersPipeline::take_completed_batches and FiltersCoordinator::receive to locate where to add the final leg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dash-spv/src/sync/filters/pipeline.rs`:
- Around line 84-85: The is_idle() predicate currently only checks
coordinator.active_count() and coordinator.pending_count(), but it should also
ensure there are no unconsumed completed batches; update
FiltersPipeline::is_idle() to include a check that self.completed_batches is
empty (e.g., add && self.completed_batches.is_empty()), so the invariant used by
manager.rs::start_download (debug_assert!(self.is_idle())) correctly fails when
receive_with_data() has populated completed_batches that
take_completed_batches() hasn't consumed yet.
- Around line 326-333: Extend the test_is_idle unit test to cover the third
lifecycle state: after creating FiltersPipeline::new() and calling
pipeline.init(...), simulate receiving all filters via the
coordinator.receive(...) path (use the actual FiltersCoordinator/receive method
used in the module) so the pipeline reaches completion, call
pipeline.take_completed_batches(), and assert pipeline.is_idle() returns true
again; refer to the existing test name test_is_idle and the methods
FiltersPipeline::init, FiltersPipeline::is_idle,
FiltersPipeline::take_completed_batches and FiltersCoordinator::receive to
locate where to add the final leg.
2 participants
init/extend_targetblocks instart_download()and just always callinit()since there shouldn't be any inflight state whenstart_downloadis called.is_idle()helper toFiltersManagerandFiltersPipelineto verify this viadebug_assertFiltersPipeline::active_count()since its dead code after removing the relevant logSummary by CodeRabbit
Refactor
Bug Fixes
Tests