Skip to content

feat: validate cfilter hash against cfheader chain #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 27, 2025
Merged

feat: validate cfilter hash against cfheader chain #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d8a164
fix(ffi-cli): do not pre-maturely break out, continue sync
PastaPastaPasta Sep 26, 2025
f636fdf
feat(spv): verify incoming cfilters against expected CFHeaders chain
PastaPastaPasta Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions dash-spv-ffi/src/bin/ffi_cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,6 @@ fn main() {
break;
}
dash_spv_ffi_sync_progress_destroy(prog_ptr);
} else {
// If progress is unavailable, assume sync finished or errored
break;
}
thread::sleep(Duration::from_millis(300));
}
Expand Down
49 changes: 48 additions & 1 deletion dash-spv/src/sync/filters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Filter synchronization functionality.

use dashcore::{
bip158::{BlockFilterReader, Error as Bip158Error},
bip158::{BlockFilter, BlockFilterReader, Error as Bip158Error},
hash_types::FilterHeader,
network::message::NetworkMessage,
network::message_blockdata::Inventory,
Expand Down Expand Up @@ -102,6 +102,53 @@ pub struct FilterSyncManager<S: StorageManager, N: NetworkManager> {
impl<S: StorageManager + Send + Sync + 'static, N: NetworkManager + Send + Sync + 'static>
FilterSyncManager<S, N>
{
/// Verify that the received compact filter hashes to the expected filter header
/// based on previously synchronized CFHeaders.
pub async fn verify_cfilter_against_headers(
&self,
filter_data: &[u8],
height: u32,
storage: &S,
) -> SyncResult<bool> {
// We expect filter headers to be synced before requesting filters.
// If we're at height 0 (genesis), skip verification because there is no previous header.
if height == 0 {
tracing::debug!("Skipping cfilter verification at genesis height 0");
return Ok(true);
}

// Load previous and expected headers
let prev_header = storage.get_filter_header(height - 1).await.map_err(|e| {
SyncError::Storage(format!("Failed to load previous filter header: {}", e))
})?;
let expected_header = storage.get_filter_header(height).await.map_err(|e| {
SyncError::Storage(format!("Failed to load expected filter header: {}", e))
})?;

let (Some(prev_header), Some(expected_header)) = (prev_header, expected_header) else {
tracing::warn!(
"Missing filter headers in storage for height {} (prev and/or expected)",
height
);
return Ok(false);
};

// Compute the header from the received filter bytes and compare
let filter = BlockFilter::new(filter_data);
let computed_header = filter.filter_header(&prev_header);

let matches = computed_header == expected_header;
if !matches {
tracing::error!(
"CFilter header mismatch at height {}: computed={:?}, expected={:?}",
height,
computed_header,
expected_header
);
}

Ok(matches)
}
Comment on lines +118 to +151
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Checkpoint-start filters are rejected forever

When we sync from a checkpoint the first filter range starts at sync_base_height. This code then tries to load the previous filter header (height - 1), but we never store headers below the checkpoint base. As a result prev_header is None, verify_cfilter_against_headers returns Ok(false), and every filter at the base height is dropped. Because that height never marks as received, the DownloadingFilters phase can’t finish and the sync stalls. Please skip header-chain verification for heights at or below the checkpoint base (mirroring the CFHeader logic) instead of rejecting those filters.

         if height == 0 {
             tracing::debug!("Skipping cfilter verification at genesis height 0");
             return Ok(true);
         }
 
+        if self.sync_base_height > 0 && height <= self.sync_base_height {
+            tracing::debug!(
+                "Skipping cfilter verification at checkpoint base height {}",
+                height
+            );
+            return Ok(true);
+        }
+
         // Load previous and expected headers
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
}
// Load previous and expected headers
let prev_header = storage.get_filter_header(height - 1).await.map_err(|e| {
SyncError::Storage(format!("Failed to load previous filter header: {}", e))
})?;
let expected_header = storage.get_filter_header(height).await.map_err(|e| {
SyncError::Storage(format!("Failed to load expected filter header: {}", e))
})?;
let (Some(prev_header), Some(expected_header)) = (prev_header, expected_header) else {
tracing::warn!(
"Missing filter headers in storage for height {} (prev and/or expected)",
height
);
return Ok(false);
};
// Compute the header from the received filter bytes and compare
let filter = BlockFilter::new(filter_data);
let computed_header = filter.filter_header(&prev_header);
let matches = computed_header == expected_header;
if !matches {
tracing::error!(
"CFilter header mismatch at height {}: computed={:?}, expected={:?}",
height,
computed_header,
expected_header
);
}
Ok(matches)
}
if height == 0 {
tracing::debug!("Skipping cfilter verification at genesis height 0");
return Ok(true);
}
if self.sync_base_height > 0 && height <= self.sync_base_height {
tracing::debug!(
"Skipping cfilter verification at checkpoint base height {}",
height
);
return Ok(true);
}
// Load previous and expected headers
let prev_header = storage.get_filter_header(height - 1).await.map_err(|e| {
SyncError::Storage(format!("Failed to load previous filter header: {}", e))
})?;
let expected_header = storage.get_filter_header(height).await.map_err(|e| {
SyncError::Storage(format!("Failed to load expected filter header: {}", e))
})?;
let (Some(prev_header), Some(expected_header)) = (prev_header, expected_header) else {
tracing::warn!(
"Missing filter headers in storage for height {} (prev and/or expected)",
height
);
return Ok(false);
};
// Compute the header from the received filter bytes and compare
let filter = BlockFilter::new(filter_data);
let computed_header = filter.filter_header(&prev_header);
let matches = computed_header == expected_header;
if !matches {
tracing::error!(
"CFilter header mismatch at height {}: computed={:?}, expected={:?}",
height,
computed_header,
expected_header
);
}
Ok(matches)
}
🤖 Prompt for AI Agents 
In dash-spv/src/sync/filters.rs around lines 118 to 151, the current logic
rejects filters at the checkpoint base because it attempts to load a previous
header that we never store below the checkpoint; update the function to skip
header-chain verification for heights at or below the checkpoint base (mirror
CFHeader logic) by checking the sync/checkpoint base value early and returning
Ok(true) for those heights so the base-range filters are accepted; otherwise
keep the normal previous-header lookup and verification flow and only log/error
when a real mismatch occurs.

/// Scan backward from `abs_height` down to `min_abs_height` (inclusive)
/// to find the nearest available block header stored in `storage`.
/// Returns the found `(BlockHash, height)` or `None` if none available.
Expand Down
40 changes: 40 additions & 0 deletions dash-spv/src/sync/sequential/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,6 +1362,32 @@ impl<
let mut wallet = self.wallet.write().await;

// Check filter against wallet if available
// First, verify filter data matches expected filter header chain
let height = storage
.get_header_height_by_hash(&cfilter.block_hash)
.await
.map_err(|e| SyncError::Storage(format!("Failed to get filter block height: {}", e)))?
.ok_or_else(|| {
SyncError::Validation(format!(
"Block height not found for cfilter block {}",
cfilter.block_hash
))
})?;

let header_ok = self
.filter_sync
.verify_cfilter_against_headers(&cfilter.filter, height, &*storage)
.await?;

if !header_ok {
tracing::warn!(
"Rejecting CFilter for block {} at height {} due to header mismatch",
cfilter.block_hash,
height
);
return Ok(());
}
Comment on lines +1365 to +1389
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don’t leak filter requests on header mismatch

We bail out before calling mark_filter_received, so the outstanding request stays in the FilterSyncManager’s active set. That slot is never released, so the download phase can stall indefinitely once a single bad filter is seen. Please ensure we notify the filter sync manager (e.g., mark the request as failed and requeue it) before returning.

🤖 Prompt for AI Agents 
In dash-spv/src/sync/sequential/mod.rs around lines 1365-1389, we currently
return early on header mismatch without notifying the FilterSyncManager, leaking
the active request slot; before returning when header_ok is false, call the
filter sync manager API to mark this cfilter request as failed (or mark it
received with an error) so the manager can release the active slot and requeue
or drop the request, await that call and handle/log any error, then return;
ensure this notification happens on the same code path where we log the header
mismatch so the request is never left active.


let matches = self
.filter_sync
.check_filter_for_matches(
Expand Down Expand Up @@ -1963,6 +1989,20 @@ impl<
.map_err(|e| SyncError::Storage(format!("Failed to get filter block height: {}", e)))?
.ok_or(SyncError::InvalidState("Filter block height not found".to_string()))?;

// Verify against expected header chain before storing
let header_ok = self
.filter_sync
.verify_cfilter_against_headers(&cfilter.filter, height, &*storage)
.await?;
if !header_ok {
tracing::warn!(
"Rejecting post-sync CFilter for block {} at height {} due to header mismatch",
cfilter.block_hash,
height
);
return Ok(());
}
Comment on lines +1993 to +2004
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Post-sync filters need a retry path on header mismatch

In the FullySynced path we drop the filter and return without re-requesting it, leaving that block without a stored filter (and the next header validation will continue to fail). We should treat this the same as the main sync path—clear the outstanding request and schedule a retry from another peer—instead of silently abandoning it.

🤖 Prompt for AI Agents 
In dash-spv/src/sync/sequential/mod.rs around lines 1993 to 2004, the
FullySynced branch currently drops a post-sync CFilter on header mismatch and
returns, leaving the block without a stored filter; instead replicate the main
sync failure handling: clear the outstanding request for this block/filter and
schedule a retry from another peer (i.e., invoke the same functions used in the
main sync path to remove the request and re-queue the fetch or peer-request for
this block), and log the retry; ensure the function returns after scheduling the
retry rather than silently abandoning the filter.


// Store the filter
storage
.store_filter(height, &cfilter.filter)
Expand Down
Loading