Skip to content

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Dec 6, 2025

This fixes an issue introduced in #211 where the receiver_filter_heights lock isn't released after inserting the received height, and then the same task tries to acquire the lock again just a bit below in mark_filter_received -> deadlock.

So, #211 should have not introduced the filter height updates here since they already exist in mark_filter_received. Yet the stats updates introduced in the same place are legit.

Summary by CodeRabbit

  • Bug Fixes

    • Simplified filter synchronization by removing redundant per-height received-filter tracking and related logging, preventing unnecessary contention.
  • Refactor

    • Streamlined internal filter state updates to reduce locking and improve performance while preserving reported filter counts and timestamps.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 6, 2025

Walkthrough

Removed the per-height received-filter tracking block from the message handler; the handler still updates overall filter statistics and last-filter-received timestamp but no longer inserts block heights into the shared received-filter-heights set.

Changes

Cohort / File(s) Change Summary
Filter tracking removal
dash-spv/src/sync/message_handlers.rs
Removed code that cloned and updated the shared received_filter_heights (per-height mutex set). Kept updates to stats.filters_received and last_filter_received_time; removed per-height insertion and associated log line.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Check for any callers or diagnostics relying on received_filter_heights.
  • Ensure concurrency expectations remain correct after removing the per-height mutex update.
  • Verify logging and metrics still reflect expected events.

Poem

🐰 I hopped through code, then gave a sigh,
The height-tracking leaves waved by,
Fewer locks, a tidier trail,
My whiskers twitch — lightening the sail! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'fix: Drop duplicated received filter update' directly and clearly describes the main change: removing duplicated filter height update logic that was causing a deadlock issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/filter-deadlock

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9521500 and ccccc6b.

📒 Files selected for processing (1)
  • dash-spv/src/sync/message_handlers.rs (0 hunks)
💤 Files with no reviewable changes (1)
  • dash-spv/src/sync/message_handlers.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: Core Components Tests
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: Key Wallet Components Tests
  • GitHub Check: SPV Components Tests
  • GitHub Check: Pre-commit (macos-latest)
  • GitHub Check: Pre-commit (ubuntu-latest)

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.

@xdustinface xdustinface marked this pull request as ready for review December 6, 2025 17:00
This fixes an issue introduced in #211 where the `receiver_filter_heights` lock isn't released after inserting the received height, and then the same task tries to acquire the lock again just a bit below in `mark_filter_received` -> deadlock.

So, #211 should have not introduced the filter height updates here since they already exist in `mark_filter_received`. Yet the stats updates introduced in the same place are legit.
@xdustinface xdustinface merged commit fb0de33 into v0.41-dev Dec 8, 2025
24 checks passed
pauldelucia pushed a commit that referenced this pull request Dec 12, 2025
This fixes an issue introduced in #211 where the `receiver_filter_heights` lock isn't released after inserting the received height, and then the same task tries to acquire the lock again just a bit below in `mark_filter_received` -> deadlock.

So, #211 should have not introduced the filter height updates here since they already exist in `mark_filter_received`. Yet the stats updates introduced in the same place are legit.
pauldelucia pushed a commit that referenced this pull request Dec 12, 2025
This fixes an issue introduced in #211 where the `receiver_filter_heights` lock isn't released after inserting the received height, and then the same task tries to acquire the lock again just a bit below in `mark_filter_received` -> deadlock.

So, #211 should have not introduced the filter height updates here since they already exist in `mark_filter_received`. Yet the stats updates introduced in the same place are legit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants