Skip to content

fix: subscribe to SPV event monitors before startup#636

Merged
xdustinface merged 1 commit intov0.42-devfrom
fix/ffi-network-callback-race
Apr 10, 2026
Merged

fix: subscribe to SPV event monitors before startup#636
xdustinface merged 1 commit intov0.42-devfrom
fix/ffi-network-callback-race

Conversation

@xdustinface
Copy link
Copy Markdown
Collaborator

@xdustinface xdustinface commented Apr 8, 2026

Start sync, network, wallet, and progress monitors before calling start(). This avoids a race where the initial connection events can be missed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved network synchronization reliability by ensuring monitoring tasks are properly initialized before operations begin and cleanly terminated during shutdown or error conditions.

Start sync, network, wallet, and progress monitors before calling `start()`. This avoids a race where the initial connection events can be missed.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 8, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: af870e46-2735-4d27-8afd-b01d1bc2e681

📥 Commits

Reviewing files that changed from the base of the PR and between d87016e and 96742e2.

📒 Files selected for processing (1)
  • dash-spv/src/client/sync_coordinator.rs

📝 Walkthrough

Walkthrough

The DashSpvClient::run method was refactored to subscribe to event channels and spawn monitoring tasks before calling self.start().await, rather than starting first. Error handling was updated to cancel monitors, await their tasks, and report errors when start() fails, with shutdown behavior preserved.

Changes

Cohort / File(s) Summary
Sync Coordinator Initialization
dash-spv/src/client/sync_coordinator.rs
Reordered operations to spawn monitoring tasks before calling start(), adjusted error handling to properly cancel and await monitor tasks on start() failure, and added logging upon successful start.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A hop-skip-jump of order new,
Monitors first, then startup true!
Error paths now gracefully unwind,
With cancellations carefully designed,
Choreography of tasks aligned!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely describes the main change: subscribing to SPV event monitors before startup to fix a race condition.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ffi-network-callback-race

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.92%. Comparing base (d87016e) to head (96742e2).
⚠️ Report is 2 commits behind head on v0.42-dev.

Files with missing lines Patch % Lines
dash-spv/src/client/sync_coordinator.rs 42.85% 4 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #636      +/-   ##
=============================================
- Coverage      67.96%   67.92%   -0.05%     
=============================================
  Files            318      318              
  Lines          67976    67978       +2     
=============================================
- Hits           46199    46172      -27     
- Misses         21777    21806      +29     
Flag Coverage Δ
core 75.21% <ø> (ø)
ffi 39.00% <ø> (ø)
rpc 19.92% <ø> (ø)
spv 85.19% <42.85%> (-0.21%) ⬇️
wallet 67.56% <ø> (ø)
Files with missing lines Coverage Δ
dash-spv/src/client/sync_coordinator.rs 82.19% <42.85%> (-2.32%) ⬇️

... and 4 files with indirect coverage changes

@xdustinface
Copy link
Copy Markdown
Collaborator Author

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 9, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions github-actions bot added the ready-for-review CodeRabbit has approved this PR label Apr 9, 2026
@xdustinface xdustinface requested a review from ZocoLini April 9, 2026 05:37
@xdustinface xdustinface merged commit 3ce04c9 into v0.42-dev Apr 10, 2026
40 checks passed
@xdustinface xdustinface deleted the fix/ffi-network-callback-race branch April 10, 2026 12:31
shumkov added a commit that referenced this pull request Apr 11, 2026
wallet-run had been rebased onto newer v0.42-dev than wallet2; this
merge brings in the upstream fixes, plus resolves the mirror-commit
overlap on key-wallet. No branch-specific logic — every wallet-run
key-wallet commit has an identical mirror on wallet2.

v0.42-dev upstream bits:
  - fix: announce tip to new peers when synced (#490)
  - fix: subscribe to SPV event monitors before startup (#636)
  - refactor: unify logging on tracing (#635)
  - chore: cleanup unused dependencies (#633)
  - fix: process broadcast transactions via dispatch_local (#626)
  - fix: gate FilterHeadersSyncComplete on block header sync (#631)
  - refactor: use String for TransactionRecord::label (#624)

# Conflicts:
#	key-wallet/src/managed_account/transaction_record.rs
xdustinface added a commit to xdustinface/rust-dashcore that referenced this pull request Apr 11, 2026
Start sync, network, wallet, and progress monitors before calling `start()`. This avoids a race where the initial connection events can be missed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants