fix: Stop loading headers twice into ChainState::headers (#258)

* Add test

Results on current `v0.41-dev`

---- test_load_headers_from_storage::genesis_1_block stdout ----

thread 'test_load_headers_from_storage::genesis_1_block' (8727544) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 0
 right: 1

---- test_load_headers_from_storage::checkpoint_1_block stdout ----

thread 'test_load_headers_from_storage::checkpoint_1_block' (8727541) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 1
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test_load_headers_from_storage::checkpoint_60000_blocks stdout ----

thread 'test_load_headers_from_storage::checkpoint_60000_blocks' (8727542) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 60000
 right: 120000

---- test_load_headers_from_storage::genesis_60000_blocks stdout ----

thread 'test_load_headers_from_storage::genesis_60000_blocks' (8727545) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 59999
 right: 60000

* Stop loading headers twice into `ChainState::headers`

We currently load the headers twice into the header vector in `HeaderSyncManager::load_headers_from_storage`:

- First1. within `storage.load_chain_state`
- And then within `load_headers_from_storage` itself further down

```
2025-12-04T16:05:36.717779Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:05:36.727966Z  INFO ✅ Loaded 2 headers into HeaderSyncManager in 0.00s (1129 headers/sec)
....

2025-12-05T04:31:15.980982Z ERROR Sequential sync manager error handling message: Validation error: Block at height 2300000 does not match checkpoint
```

```
2025-12-04T16:09:37.959349Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:09:37.966703Z  INFO ✅ Loaded 1 headers into HeaderSyncManager in 0.00s (862 headers/sec)
´´´

Author: xdustinface

dash-spv/Cargo.toml

@@ -60,6 +60,7 @@ tempfile = "3.0"
 tokio-test = "0.4"
 env_logger = "0.10"
 hex = "0.4"
+test-case = "3.3"
 
 [[bin]]
 name = "dash-spv"

dash-spv/src/sync/headers/manager.rs

@@ -102,116 +102,37 @@ impl<S: StorageManager + Send + Sync + 'static, N: NetworkManager + Send + Sync
 
     /// Load headers from storage into the chain state
     pub async fn load_headers_from_storage(&mut self, storage: &S) -> SyncResult<u32> {
+        let start_time = std::time::Instant::now();
+        let mut loaded_count = 0;
+        let mut tip_height = 0;
         // First, try to load the persisted chain state which may contain sync_base_height
         if let Ok(Some(stored_chain_state)) = storage.load_chain_state().await {
             tracing::info!(
                 "Loaded chain state from storage with sync_base_height: {}",
                 stored_chain_state.sync_base_height,
             );
-            // Update our chain state with the loaded one to preserve sync_base_height
+            // Update our chain state with the loaded one
             {
+                loaded_count = stored_chain_state.headers.len();
+                tip_height = stored_chain_state.tip_height();
                 self.cached_sync_base_height = stored_chain_state.sync_base_height;
                 let mut cs = self.chain_state.write().await;
                 *cs = stored_chain_state;
             }
         }
 
-        // Get the current tip height from storage
-        let tip_height = storage
-            .get_tip_height()
-            .await
-            .map_err(|e| SyncError::Storage(format!("Failed to get tip height: {}", e)))?;
-
-        let Some(tip_height) = tip_height else {
-            tracing::debug!("No headers found in storage");
-            // If we're syncing from a checkpoint, this is expected
-            if self.is_synced_from_checkpoint() {
-                tracing::info!("No headers in storage for checkpoint sync - this is expected");
-                return Ok(0);
-            }
-            return Ok(0);
-        };
-
-        if tip_height == 0 && !self.is_synced_from_checkpoint() {
-            tracing::debug!("Only genesis block in storage");
-            return Ok(0);
-        }
-
-        tracing::info!("Loading {} headers from storage into HeaderSyncManager", tip_height);
-        let start_time = std::time::Instant::now();
-
-        // Load headers in batches
-        const BATCH_SIZE: u32 = 10_000;
-        let mut loaded_count = 0u32;
-
-        // For checkpoint syncs we start at the checkpoint base; otherwise we skip genesis (already present).
-        let mut current_height = self.get_sync_base_height().max(1);
-
-        while current_height <= tip_height {
-            let end_height = (current_height + BATCH_SIZE - 1).min(tip_height);
-
-            // Load batch from storage
-            let headers_result = storage.load_headers(current_height..end_height + 1).await;
-
-            match headers_result {
-                Ok(headers) if !headers.is_empty() => {
-                    // Add headers to chain state
-                    {
-                        let mut cs = self.chain_state.write().await;
-                        for header in headers {
-                            cs.add_header(header);
-                            loaded_count += 1;
-                        }
-                    }
-                }
-                Ok(_) => {
-                    // Empty headers - this can happen for checkpoint sync with minimal headers
-                    tracing::debug!(
-                        "No headers found for range {}..{} - continuing",
-                        current_height,
-                        end_height + 1
-                    );
-                    // Break out of the loop since we've reached the end of available headers
-                    break;
-                }
-                Err(e) => {
-                    // For checkpoint sync with only 1 header stored, this is expected
-                    if self.is_synced_from_checkpoint() && loaded_count == 0 && tip_height == 0 {
-                        tracing::info!(
-                            "No additional headers to load for checkpoint sync - this is expected"
-                        );
-                        return Ok(0);
-                    }
-                    return Err(SyncError::Storage(format!("Failed to load headers: {}", e)));
-                }
-            }
-
-            // Progress logging
-            if loaded_count.is_multiple_of(50_000) || loaded_count == tip_height {
-                let elapsed = start_time.elapsed();
-                let headers_per_sec = loaded_count as f64 / elapsed.as_secs_f64();
-                tracing::info!(
-                    "Loaded {}/{} headers ({:.0} headers/sec)",
-                    loaded_count,
-                    tip_height,
-                    headers_per_sec
-                );
-            }
-
-            current_height = end_height + 1;
-        }
-
         self.total_headers_synced = tip_height;
 
         let elapsed = start_time.elapsed();
         tracing::info!(
-            "✅ Loaded {} headers into HeaderSyncManager in {:.2}s ({:.0} headers/sec)",
+            "✅ Loaded {} headers for tip height {} into HeaderSyncManager in {:.2}s ({:.0} headers/sec)",
             loaded_count,
+            tip_height,
             elapsed.as_secs_f64(),
             loaded_count as f64 / elapsed.as_secs_f64()
         );
 
-        Ok(loaded_count)
+        Ok(loaded_count as u32)
     }
 
     /// Handle a Headers message

dash-spv/tests/header_sync_test.rs

@@ -5,7 +5,8 @@ use std::time::Duration;
 use dash_spv::{
     client::{ClientConfig, DashSpvClient},
     network::PeerNetworkManager,
-    storage::{MemoryStorageManager, StorageManager},
+    storage::{DiskStorageManager, MemoryStorageManager, StorageManager},
+    sync::{HeaderSyncManager, ReorgConfig},
     types::{ChainState, ValidationMode},
 };
 use dashcore::{block::Header as BlockHeader, block::Version, Network};
@@ -14,6 +15,8 @@ use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
 use key_wallet_manager::wallet_manager::WalletManager;
 use log::{debug, info};
 use std::sync::Arc;
+use tempfile::TempDir;
+use test_case::test_case;
 use tokio::sync::RwLock;
 
 #[tokio::test]
@@ -332,3 +335,45 @@ async fn test_header_storage_consistency() {
 
     info!("Header storage consistency test completed");
 }
+
+#[test_case(0, 0 ; "genesis_0_blocks")]
+#[test_case(0, 1 ; "genesis_1_block")]
+#[test_case(0, 60000 ; "genesis_60000_blocks")]
+#[test_case(100, 0 ; "checkpoint_0_blocks")]
+#[test_case(170000, 1 ; "checkpoint_1_block")]
+#[test_case(12345, 60000 ; "checkpoint_60000_blocks")]
+#[tokio::test]
+async fn test_load_headers_from_storage(sync_base_height: u32, header_count: usize) {
+    // Setup: Create storage with 100 headers
+    let temp_dir = TempDir::new().expect("Failed to create temp dir");
+    let mut storage = DiskStorageManager::new(temp_dir.path().to_path_buf())
+        .await
+        .expect("Failed to create storage");
+
+    let test_headers = create_test_header_chain(header_count);
+
+    // Store chain state
+    let mut chain_state = ChainState::new_for_network(Network::Dash);
+    chain_state.sync_base_height = sync_base_height;
+    chain_state.headers = test_headers.clone();
+    storage.store_chain_state(&chain_state).await.expect("Failed to store chain state");
+
+    // Create HeaderSyncManager and load headers
+    let config = ClientConfig::new(Network::Dash);
+    let chain_state = Arc::new(RwLock::new(ChainState::new_for_network(Network::Dash)));
+    let mut header_sync = HeaderSyncManager::<DiskStorageManager, PeerNetworkManager>::new(
+        &config,
+        ReorgConfig::default(),
+        chain_state.clone(),
+    )
+    .expect("Failed to create HeaderSyncManager");
+
+    // Load headers from storage
+    let loaded_count =
+        header_sync.load_headers_from_storage(&storage).await.expect("Failed to load headers");
+
+    let cs = chain_state.read().await;
+
+    assert_eq!(loaded_count as usize, header_count, "Loaded count mismatch");
+    assert_eq!(header_count, cs.headers.len(), "Chain state count mismatch");
+}