Fix sync status genesis #95
Merged
Conversation
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
…d checkpoint sync Previously, the status display used different logic for calculating header heights: - Checkpoint sync: correctly used sync_base_height + storage_count - Genesis sync: incorrectly checked chain_state.headers which was kept minimal This caused status to show all zeros when syncing from genesis, while headers were actually being synced to disk successfully. The fix unifies both paths to use the same formula: - Genesis sync: 0 + storage_count = actual height - Checkpoint sync: checkpoint_height + storage_count = actual height This ensures accurate progress reporting regardless of sync method. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
WalkthroughRefactored status display height calculations into unified storage-based formulas, replacing multiple branches. Added helper methods to compute header and filter header heights and updated logging. Adjusted sync progress and status update paths to use the new helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant SD as StatusDisplay
participant ST as Storage
participant CS as ChainState
UI->>SD: update_status_display()
SD->>CS: read sync_base_height, blockchain_height
SD->>ST: get header storage_tip
ST-->>SD: storage_tip (optional)
SD->>SD: calculate_header_height(sync_base + storage_tip or base)
SD->>ST: get filter header storage_height
ST-->>SD: storage_height (optional)
SD->>SD: calculate_filter_header_height(sync_base + storage_height or base)
SD-->>UI: render status (heights, progress)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 0
🔭 Outside diff range comments (4)
dash-spv/src/client/status_display.rs (4)
122-131: Do not hold RwLock read guards across .await (deadlock risk).A read guard to self.state is held while awaiting storage I/O via calculate_header_height_with_logging. Refactor to snapshot the needed value(s) first, drop the lock, then await.
Apply this diff:
- // Get header height - when syncing from checkpoint, use the actual blockchain height - let header_height = { - let state = self.state.read().await; - self.calculate_header_height_with_logging(&state, true).await - }; + // Snapshot base and compute header height without holding locks across await + let sync_base_height = { + let state = self.state.read().await; + state.sync_base_height + }; + let header_height = + self.calculate_header_height_with_logging(sync_base_height, true).await; - // Get filter header height - convert from storage height to blockchain height - let filter_height = { - let state = self.state.read().await; - self.calculate_filter_header_height(&state).await - }; + // Compute filter header height without holding locks across await + let filter_height = + self.calculate_filter_header_height(sync_base_height).await;
173-182: Same issue in logging path: avoid holding locks across .await.The state read lock is held while awaiting the helper methods. Snapshot first.
Apply this diff:
- let header_height = { - let state = self.state.read().await; - self.calculate_header_height_with_logging(&state, true).await - }; + let sync_base_height = { + let state = self.state.read().await; + state.sync_base_height + }; + let header_height = + self.calculate_header_height_with_logging(sync_base_height, true).await; - // Get filter header height - convert from storage height to blockchain height - let filter_height = { - let state = self.state.read().await; - self.calculate_filter_header_height(&state).await - }; + let filter_height = + self.calculate_filter_header_height(sync_base_height).await;
74-104: sync_progress holds RwLocks across .await; snapshot values before awaiting.You hold both state and stats read guards while awaiting header/filter height helpers. This can lead to deadlocks and writer starvation. Snapshot the needed fields, drop locks, then perform awaits.
Apply this diff to restructure:
- let state = self.state.read().await; - let stats = self.stats.read().await; - - // Calculate last synced filter height from received filter heights - let last_synced_filter_height = if let Ok(heights) = stats.received_filter_heights.lock() { - heights.iter().max().copied() - } else { - None - }; - - // Calculate the actual header height considering checkpoint sync - let header_height = self.calculate_header_height(&state).await; - - // Calculate filter header height considering checkpoint sync - let filter_header_height = self.calculate_filter_header_height(&state).await; + // Snapshot stats-derived values without holding locks across await + let (last_synced_filter_height, filters_downloaded) = { + let stats = self.stats.read().await; + let last = stats + .received_filter_heights + .lock() + .ok() + .and_then(|heights| heights.iter().max().copied()); + (last, stats.filters_received) + }; + + // Snapshot state-derived values + let (sync_base_height, masternode_height) = { + let state = self.state.read().await; + (state.sync_base_height, state.last_masternode_diff_height.unwrap_or(0)) + }; + + // Compute heights without holding any RwLocks + let header_height = self.calculate_header_height(sync_base_height).await; + let filter_header_height = self.calculate_filter_header_height(sync_base_height).await; Ok(SyncProgress { header_height, filter_header_height, - masternode_height: state.last_masternode_diff_height.unwrap_or(0), + masternode_height, peer_count: 1, // TODO: Get from network manager headers_synced: false, // TODO: Implement filter_headers_synced: false, // TODO: Implement masternodes_synced: false, // TODO: Implement filter_sync_available: false, // TODO: Get from network manager - filters_downloaded: stats.filters_received, + filters_downloaded, last_synced_filter_height, sync_start: std::time::SystemTime::now(), // TODO: Track properly last_update: std::time::SystemTime::now(), })
41-45: Make helpers take sync_base_height by value to prevent lock usage across awaits.Passing &ChainState encourages holding a read guard across the await in callers. Make the helpers take sync_base_height: u32 instead.
Apply these diffs:
- async fn calculate_header_height_with_logging( - &self, - state: &ChainState, - with_logging: bool, - ) -> u32 { + async fn calculate_header_height_with_logging( + &self, + sync_base_height: u32, + with_logging: bool, + ) -> u32 { @@ - if let Ok(Some(storage_tip)) = self.storage.get_tip_height().await { - let blockchain_height = state.sync_base_height + storage_tip; + if let Ok(Some(storage_tip)) = self.storage.get_tip_height().await { + let blockchain_height = sync_base_height + storage_tip; if with_logging { tracing::debug!( - "Status display: storage_tip={}, sync_base={}, blockchain_height={}", - storage_tip, - state.sync_base_height, - blockchain_height + "Status display: storage_tip={}, sync_base={}, blockchain_height={}", + storage_tip, + sync_base_height, + blockchain_height ); } blockchain_height } else { - // No headers in storage yet - state.sync_base_height + // No headers in storage yet + sync_base_height } } @@ - async fn calculate_header_height(&self, state: &ChainState) -> u32 { - self.calculate_header_height_with_logging(state, false).await + async fn calculate_header_height(&self, sync_base_height: u32) -> u32 { + self.calculate_header_height_with_logging(sync_base_height, false).await }- async fn calculate_filter_header_height(&self, state: &ChainState) -> u32 { + async fn calculate_filter_header_height(&self, sync_base_height: u32) -> u32 { @@ - if let Ok(Some(storage_height)) = self.storage.get_filter_tip_height().await { - // The blockchain height is sync_base_height + storage_height - state.sync_base_height + storage_height + if let Ok(Some(storage_height)) = self.storage.get_filter_tip_height().await { + // The blockchain height is sync_base_height + storage_height + sync_base_height + storage_height } else { - // No filter headers in storage yet - state.sync_base_height + // No filter headers in storage yet + sync_base_height } }Update call sites as shown in the earlier comments to pass a copied sync_base_height instead of &state.
Also applies to: 59-63, 66-70, 212-227
🧹 Nitpick comments (1)
dash-spv/src/client/status_display.rs (1)
46-49: Comment wording mismatch (storage_count vs storage_tip/height).The comments say storage_count, but variables and logic use storage_tip/storage_height. This is confusing for future readers.
Apply these minimal comment fixes:
- // Unified formula for both checkpoint and genesis sync: - // For genesis sync: sync_base_height = 0, so height = 0 + storage_count - // For checkpoint sync: height = checkpoint_height + storage_count + // Unified formula for both checkpoint and genesis sync: + // For genesis sync: sync_base_height = 0, so height = 0 + storage_tip + // For checkpoint sync: height = checkpoint_height + storage_tip- // Unified formula for both checkpoint and genesis sync: - // For genesis sync: sync_base_height = 0, so height = 0 + storage_count - // For checkpoint sync: height = checkpoint_height + storage_count + // Unified formula for both checkpoint and genesis sync: + // For genesis sync: sync_base_height = 0, so height = 0 + filter_storage_tip + // For checkpoint sync: height = checkpoint_height + filter_storage_tipAlternatively, explicitly state “relative tip height” if that’s the intent.
Also applies to: 217-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/client/status_display.rs(2 hunks)
⏰ 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). (10)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
🔇 Additional comments (1)
dash-spv/src/client/status_display.rs (1)
41-63: No changes required:get_tip_heightreturns a 0-indexed tip, so addingsync_base_heightyields the correct absolute height.• Both
MemoryStorageManagerandDiskStorageManagerreturn the highest header index (orNoneif empty).
• Numerous tests (e.g.storage_test.rs“0-indexed” assertions,edge_case_filter_sync_test.rs) confirm thatget_tip_height()is 0-based.
• Summingstate.sync_base_height + storage_tiptherefore produces the intended absolute blockchain height.
This was referenced Aug 26, 2025
This was referenced Sep 26, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by CodeRabbit