Feat/mempool bloom filters chain management

[PastaPastaPasta]

Summary by CodeRabbit

• New Features
  • QRInfo-based masternode sync with smart DKG-window fetching for faster, lighter sync.
  • Embedded MNListDiffs to accelerate initial sync.
  • Optional chain lock and quorum validation with caching and state snapshots.
  • New client options for QRInfo extra share and timeout.
  • CLI now auto-creates a temporary data directory if none provided.
• Improvements
  • Added QRInfo message handling and request support.
  • Implemented FFI quorum public key and platform activation height retrieval.
  • Adjusted header sync timeouts; performance tuning in FFI runtime.
• Changes
  • Removed terminal block system and related data/scripts.
  • Updated mainnet checkpoints.
  • Swift package no longer builds FFI target (provided via unified SDK).
• Documentation
  • Added guides for FFI usage, validation phases, and QRInfo plan.
• Tests
  • Extensive QRInfo, smart fetch, safety, and integration test coverage added.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds a new test-utils crate; integrates QRInfo-based masternode sync with engine-driven discovery; removes terminal-block system and related storage APIs; expands validation (engines, state, chainlock checks); updates headers/reorg sync; introduces DKG windowing and LLMQ network helpers; extends client/config/network APIs; implements FFI quorum pubkey lookup; updates checkpoints; adjusts Swift package; adds docs and tests.

Changes

Cohort / File(s)	Summary
Workspace & Utilities Cargo.toml, test-utils/*, dash-spv-ffi/Cargo.toml	Adds test-utils workspace crate and dev-dep; provides builders/fixtures/helpers/macros for tests.
FFI Enhancements dash-spv-ffi/src/{platform_integration.rs,client.rs,error.rs}, dash-spv-ffi/include/dash_spv_ffi.h, dash-spv-ffi/tests/*, dash-spv-ffi/CLAUDE.md, swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h	Implements ffi_dash_spv_get_quorum_public_key and platform activation height; widens client inner visibility; derives traits for FFIErrorCode; renames header params; adds safety/minimal tests and docs.
Swift Package Adjustments swift-dash-core-sdk/Package.swift, swift-dash-core-sdk/Sources/SwiftDashCoreSDK/Core/SPVClient.swift, swift-dash-core-sdk/build-ios.sh	Removes DashSPVFFI target; adds FFI handle accessor and debug logs; centralizes cargo build steps in iOS build script.
QRInfo Feature & Network Layer dash-spv/src/network/{mod.rs,message_handler.rs,tests.rs,multi_peer.rs}, dash-spv/src/client/{message_handler.rs,mod.rs}, dash-spv/tests/qrinfo_integration_test.rs	Adds QRInfo message handling, stats, and request API; routes QRInfo to sync; exposes masternode/quorum lookups on client; tests QRInfo flow.
Masternode Sync Refactor dash-spv/src/sync/{masternodes.rs,sequential/*,mod.rs,discovery.rs,embedded_data.rs}, dash-spv/tests/smart_fetch_integration_test.rs	Replaces terminal-block flow with QRInfo-driven sync, embedded diffs, hybrid planning counters, network-aware transitions, new methods; adds discovery struct and embedded MNListDiffs; smart-fetch tests.
Validation Subsystem dash-spv/src/sync/{validation.rs,validation_state.rs,chainlock_validation.rs}, dash-spv/src/validation/test_summary.md, dash-spv/src/sync/validation_test.rs, dash-spv/PHASE_4_IMPLEMENTATION.md	Introduces validation engine/config/results, state manager with snapshot/rollback, chainlock validator; adds docs and tests.
Headers Sync & Reorg dash-spv/src/sync/{headers_with_reorg.rs,headers.rs}, dash-spv/src/sync/sequential/{mod.rs,phases.rs,transitions.rs,recovery.rs}	Refactors batch processing with checkpoints, exposes set_chain_state, adjusts timeouts, adds hybrid counters, makes transitions network-aware, reclassifies a recovery error.
Storage API Changes dash-spv/src/storage/{mod.rs,disk.rs,memory.rs,types.rs}	Removes terminal-block types and methods; introduces blockchain vs storage index accounting; adjusts MasternodeState fields.
Terminal-Block Removal & Data dash-spv/src/sync/terminal_blocks.rs, dash-spv/src/sync/terminal_block_data/*, dash-spv/data/{mainnet,testnet}/mod.rs, dash-spv/docs/TERMINAL_BLOCKS.md, dash-spv/examples/test_terminal_blocks.rs, dash-spv/scripts/fetch_terminal_blocks.py, dash-spv/tests/terminal_block_test.rs	Deletes terminal-block feature, loaders, data, docs, script, and tests.
Client Config & Public Exports dash-spv/src/client/{config.rs,config_test.rs}, dash-spv/src/lib.rs, dash-spv/src/main.rs	Adds QRInfo config fields and builders; updates defaults/tests; exports QuorumHash, Hash, LLMQType; makes data-dir optional with temp default.
Client & Tests API Adjustments dash-spv/src/client/{block_processor_test.rs,message_handler_test.rs,consistency_test.rs,watch_manager_test.rs}, dash-spv/tests/{block_download_test.rs,chainlock_validation_test.rs,error_handling_test.rs,error_recovery_integration_test.rs,error_types_test.rs}	Updates for new wallet/storage APIs, event/stat changes, SequentialSyncManager::new signature/result, NetworkManager trait changes; minor test fixes.
Checkpoints & Chainlock Manager dash-spv/src/chain/checkpoints.rs, dash-spv/src/chain/chainlock_manager.rs	Updates mainnet checkpoints; introduces named constant for chainlock MN offset.
LLMQ & DKG Support (dash crate) dash/src/sml/llmq_type/{mod.rs,network.rs}, dash/src/sml/masternode_list_engine/mod.rs, dash/Cargo.toml	Adds DKGWindow and LLMQType helpers; per-network LLMQ/window planning; expands masternode engine APIs for heights, diffs, QRInfo feeding; adds log dependency.
Examples & Headers2 Tests Removal dash-spv/examples/*, dash-spv/tests/{headers2_*.rs}	Removes assorted examples and headers2 tests.
Docs & Plans PLAN.md, qr_info_spv_plan/*, dash-spv/src/validation/test_summary.md, test_smart_algo.sh	Adds planning docs/phases, summary, script for smart algo run; updates plan notes.

Sequence Diagram(s)

sequenceDiagram
  participant Peer
  participant Network as NetworkManager
  participant Client as DashSpvClient
  participant Sync as SequentialSyncManager
  participant MN as MasternodeSyncManager
  participant Engine as MasternodeListEngine
  Note over Client: Startup
  Client->>Network: request_qr_info(base_hashes, tip_hash, extra_share)
  Network-->>Peer: GetQRInfo
  Peer-->>Network: QRInfo
  Network-->>Client: MessageHandleResult::QRInfo
  Client->>Sync: handle_message(QRInfo)
  Sync->>MN: handle_qrinfo_message(qr_info, storage, network, sync_base)
  MN->>Engine: feed_qr_info(qr_info, verify flags, fetch_height)
  Engine-->>MN: updated state
  MN-->>Sync: completion/update

Loading

sequenceDiagram
  participant FFI as dash-spv-ffi
  participant Core as FFIDashSpvClient.inner
  participant Eng as MasternodeListEngine
  Note over FFI: ffi_dash_spv_get_quorum_public_key
  FFI->>Core: lock client, read params
  Core->>Eng: lookup quorum(type, hash, height)
  Eng-->>Core: pubkey (48 bytes) or error
  Core-->>FFI: copy to out_pubkey, return FFIResult

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120+ minutes

Possibly related PRs

• refactor: simplify the code, fix various test compilation issues #98 — Implements engine-driven QRInfo/masternode-sync refactor overlapping network, sync phases, and manager APIs.
• feat: Deterministic Masternode List and Quorum Rotation (DIP24) support #51 — Adds LLMQType helpers and DKGWindow; intersects with new DKG/window logic and network gating.
• feat: dash spv shared base #94 — Modifies dash-spv-ffi API and tests; aligns with FFI quorum public key implementation here.

Suggested reviewers

• pankcuf
• QuantumExplorer

Poem

A rabbit taps keys by the moon’s soft glow,
DKGs align, the QRInfo flows.
Terminal stones hop out of the lane,
Engines hum—validation’s reign.
From FFI burrows to headers’ crest,
We sync by thumps—then pause to rest.
Carrot-approve: this patch is best! 🥕🐇

✨ Finishing Touches

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/mempool-bloom-filters-chain-management

---

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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

• Visit our Status Page to check the current availability of CodeRabbit.
• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[PastaPastaPasta]

@coderabbitai review

[coderabbitai]

✅ 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.

[coderabbitai]

Actionable comments posted: 34

🔭 Outside diff range comments (9)

swift-dash-core-sdk/Package.swift (1)

40-51: Critical: absolute -L search paths hard-code a developer machine and include a removed path.

The linkerSettings use .unsafeFlags with absolute -L paths to /Users/quantum/... and even reference Sources/DashSPVFFI which was removed. This is not portable and will break builds on CI and other machines.

Recommended fixes:

• Replace ad-hoc -L flags with an SPM binaryTarget that embeds the prebuilt library as an .xcframework, and make KeyWalletFFI a binary target instead of a C target.
• If you must keep a C target, ensure link search paths are relative to the repo (e.g., Artifacts/...) and checked-in; avoid absolute paths. Better yet, remove custom -L entirely and rely on the binaryTarget mechanism.

I can help convert KeyWalletFFI to a binaryTarget and wire KeyWalletFFISwift to depend on it to remove all unsafe flags.

dash-spv/src/network/multi_peer.rs (2)

1159-1181: get_message_sender does not route GetMnListD to a single peer (inconsistent with send_message)

Messages sent via the channel created by get_message_sender currently treat GetMnListD as broadcast, diverging from send_message behavior.

Apply this diff to align behavior:

                 match &message {
                     NetworkMessage::GetHeaders(_)
                     | NetworkMessage::GetCFHeaders(_)
                     | NetworkMessage::GetCFilters(_)
-                    | NetworkMessage::GetData(_) => {
+                    | NetworkMessage::GetData(_)
+                    | NetworkMessage::GetMnListD(_) => {
                         // Send to a single peer for sync messages including GetData for block downloads
                         let connections = pool.get_all_connections().await;
                         if let Some((_, conn)) = connections.first() {
                             let mut conn_guard = conn.write().await;
                             let _ = conn_guard.send_message(message).await;
                         }
                     }

---

1285-1288: Infinite recursion bug in trait method get_last_message_peer_id

This calls itself recursively. You intended to call the inherent method on Self.

Apply this diff:

-    async fn get_last_message_peer_id(&self) -> crate::types::PeerId {
-        // Call the instance method to avoid code duplication
-        self.get_last_message_peer_id().await
-    }
+    async fn get_last_message_peer_id(&self) -> crate::types::PeerId {
+        // Call the inherent method to avoid recursion
+        MultiPeerNetworkManager::get_last_message_peer_id(self).await
+    }

dash/src/sml/masternode_list_engine/mod.rs (1)

48-51: Fix inconsistent block_hash <-> height mapping in feed_block_height()

Inserting new mappings without removing prior ones can leave stale reverse entries (height->old hash or hash->old height), causing lookups to disagree. Make feed_block_height maintain a bijection by removing conflicting entries before inserting.

 pub fn feed_block_height(&mut self, height: CoreBlockHeight, block_hash: BlockHash) {
-    self.block_heights.insert(block_hash, height);
-    self.block_hashes.insert(height, block_hash);
+    // If this height was previously mapped to a different hash, remove the stale reverse entry.
+    if let Some(prev_hash) = self.block_hashes.insert(height, block_hash) {
+        if prev_hash != block_hash {
+            self.block_heights.remove(&prev_hash);
+        }
+    }
+    // If this hash was previously mapped to a different height, remove the stale forward entry.
+    if let Some(prev_height) = self.block_heights.insert(block_hash, height) {
+        if prev_height != height {
+            self.block_hashes.remove(&prev_height);
+        }
+    }
 }

dash-spv/src/sync/sequential/recovery.rs (1)

241-246: Abort mapped to SyncError::Network — remaining SyncFailed usages found; fix required

Mapping Abort -> Err(SyncError::Network(...)) will collapse all abort kinds into "Network". A quick search shows leftover uses of the deprecated SyncError::SyncFailed, so either update those callsites/pattern matches to the new classification or preserve the original SyncError in the strategy.

Files that need attention:

• dash-spv-ffi/src/client.rs:535 — constructs SpvError::Sync(SyncError::SyncFailed(msg))
• dash-spv/src/error.rs:200 — matches SyncError::SyncFailed in category()
• dash-spv/src/error.rs:275–284 — tests creating/asserting SyncFailed
• dash-spv/tests/error_types_test.rs:416 — test constructing SyncError::SyncFailed

Recommended actions (pick one):

• If you intend all Aborts to be Network: update the remaining SyncFailed references to Network (and adjust tests/pattern matches).
• Otherwise preserve error granularity by changing RecoveryStrategy::Abort to carry a SyncError (not just a String) and return it directly, e.g.:

  RecoveryStrategy::Abort { error } => {
      tracing::error!("❌ Aborting sync: {}", error);
      Err(error) // where `error: SyncError`
  }
  

dash-spv-ffi/src/client.rs (1)

529-536: Update deprecated SyncError::SyncFailed -> SyncError::Network in FFI client

rg output shows SyncError::SyncFailed still exists (deprecated) in the repo, and the FFI client still constructs it — update the FFI usage to the new variant.

• Fix this location:
  • dash-spv-ffi/src/client.rs:535 — replace deprecated variant with the new one.
• No change required for:
  • dash-spv/src/error.rs and dash-spv/tests/error_types_test.rs — the deprecated variant is intentionally kept/covered by tests (they use #[allow(deprecated)]).

Replace:

// before
Err(dash_spv::SpvError::Sync(dash_spv::SyncError::SyncFailed(msg)))

with:

// after
Err(dash_spv::SpvError::Sync(dash_spv::SyncError::Network(msg)))

dash-spv/src/storage/disk.rs (2)

803-839: Segment addressing mixes blockchain height and storage index, causing reads/writes to diverge.

store_headers_from_height calculates segment_id/offset from blockchain height, while all retrieval paths (get_header, load_headers, etc.) use storage-relative heights. This will put checkpoint-stored headers into different segment files than the readers expect, breaking reads.

Use storage index for segment addressing and reverse index for blockchain heights:

-            // Use blockchain height for segment calculation
-            let segment_id = Self::get_segment_id(blockchain_height);
-            let offset = Self::get_segment_offset(blockchain_height);
+            // Use storage index for segment calculation (consistent with readers)
+            let segment_id = Self::get_segment_id(storage_index);
+            let offset = Self::get_segment_offset(storage_index);
...
-            // Update reverse index with blockchain height
-            reverse_index.insert(header.block_hash(), blockchain_height);
+            // Update reverse index with blockchain height (external consumers expect blockchain heights)
+            reverse_index.insert(header.block_hash(), blockchain_height);
...
-        // Save dirty segments periodically (every 1000 headers)
-        if headers.len() >= 1000 || blockchain_height % 1000 == 0 {
+        // Save dirty segments periodically (every 1000 headers)
+        if headers.len() >= 1000 || storage_index % 1000 == 0 {
             self.save_dirty_segments().await?;
         }

Also applies to: 841-867, 873-876

---

298-324: Rebuilt index uses storage-relative heights; add sync_base_height to restore blockchain heights.

When index.dat is missing, you rebuild mapping as segment_id*HEADERS_PER_SEGMENT + offset. That produces storage indices, not blockchain heights. Downstream APIs (e.g., get_header_height_by_hash) now return blockchain heights elsewhere. Add sync_base_height from chain state when rebuilding.

Example adjustment:

             if !index_loaded && !all_segment_ids.is_empty() {
                 tracing::info!("Index file not found, rebuilding from segments...");
                 let mut new_index = HashMap::new();
+                // Load sync_base_height (0 if not checkpointed)
+                let sync_base_height = self
+                    .load_chain_state()
+                    .await
+                    .ok()
+                    .flatten()
+                    .map(|cs| cs.sync_base_height)
+                    .unwrap_or(0);
...
                         for (offset, header) in headers.iter().enumerate() {
-                            let height = start_height + offset as u32;
+                            let storage_height = start_height + offset as u32;
+                            let height = sync_base_height + storage_height;
                             let hash = header.block_hash();
                             new_index.insert(hash, height);
                         }

dash-spv/tests/chainlock_validation_test.rs (1)

427-435: Inject a masternode engine before asserting update succeeds

As written, updated will remain false because the engine isn’t actually set (the sync_manager mutation is commented out). Mirror the approach used earlier in this file by injecting the engine via chainlock_manager().

Apply:

-    let mock_engine = MasternodeListEngine::default_for_network(Network::Dash);
+    let mock_engine = MasternodeListEngine::default_for_network(Network::Dash);
@@
-    // Note: sync_manager is private, can't access directly
-    // client.sync_manager.masternode_sync_mut().set_engine(Some(mock_engine));
+    // Inject the engine via the chainlock manager
+    client
+        .chainlock_manager()
+        .set_masternode_engine(Arc::new(mock_engine));
@@
-    let updated = client.update_chainlock_validation().unwrap();
+    let updated = client.update_chainlock_validation().unwrap();
     assert!(updated);

[coderabbitai]

🛠️ Refactor suggestion

⚠️ Potential issue

Unsafe raw copy from an opaque public key type risks UB; copy serialized bytes instead.

Casting public_key to *const u8 assumes a packed 48-byte layout. If this is a struct, this is undefined behavior. Extract the 48-byte representation via a dedicated API then copy.

Suggested approach (pick the API that exists on your public_key type):

-            // Copy the public key directly from the global index
-            let pubkey_ptr = public_key as *const _ as *const u8;
-            std::ptr::copy_nonoverlapping(pubkey_ptr, out_pubkey, QUORUM_PUBKEY_SIZE);
+            // Serialize/convert to bytes and copy safely
+            // Option A (if it returns [u8; 48]):
+            // let bytes: [u8; QUORUM_PUBKEY_SIZE] = public_key.to_bytes();
+            // std::ptr::copy_nonoverlapping(bytes.as_ptr(), out_pubkey, QUORUM_PUBKEY_SIZE);
+            //
+            // Option B (if it returns &[u8]):
+            // let bytes: &[u8] = public_key.as_bytes();
+            // std::ptr::copy_nonoverlapping(bytes.as_ptr(), out_pubkey, QUORUM_PUBKEY_SIZE);
+            //
+            // Option C (explicit serialize method):
+            // let bytes = public_key.serialize(); // Vec<u8> or [u8; 48]
+            // std::ptr::copy_nonoverlapping(bytes.as_ptr(), out_pubkey, QUORUM_PUBKEY_SIZE);

If the engine stores [u8; 48] directly, use that array and copy its .as_ptr(); otherwise, expose a conversion on the key type and use it here.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In dash-spv-ffi/src/platform_integration.rs around lines 174 to 177, do not cast
the opaque public_key value to *const u8 and copy it directly; instead obtain
the public key's canonical 48-byte representation (for example via a
to_bytes()/as_bytes()/serialize() API or by exposing a &[u8;48] on the key
type), verify the length is 48, then call std::ptr::copy_nonoverlapping from
that byte slice's .as_ptr() into out_pubkey; if no such API exists add a
conversion method that returns the fixed 48-byte array and use it here to avoid
undefined behavior from raw struct memory copies.

[coderabbitai]

🛠️ Refactor suggestion

Consider refactoring tests to avoid testing private fields.

Multiple tests reference private fields (processed_blocks, failed) with comments about being unable to access them. This suggests the tests are attempting to verify implementation details rather than observable behavior.

Consider refactoring these tests to verify observable behavior through public APIs:

1. For duplicate detection (lines 173-174): Verify via returned result or by checking that stats don't increment
2. For failed state (lines 205-206, 223-225): Test through public API responses
3. For error recovery (lines 336-337, 349-351, 356-357): Verify through subsequent successful operations

Example refactor for duplicate detection:

-        // Can't access private field processed_blocks
-        // Skip this test or refactor to test duplicate detection differently
+        // Process a block
+        let block = create_test_block();
+        let (response_tx1, response_rx1) = oneshot::channel();
+        task_tx.send(BlockProcessingTask::ProcessBlock {
+            block: block.clone(),
+            response_tx: response_tx1,
+        }).unwrap();
+        assert!(response_rx1.await.unwrap().is_ok());
+        
+        // Try to process same block again
+        let (response_tx2, response_rx2) = oneshot::channel();
+        task_tx.send(BlockProcessingTask::ProcessBlock {
+            block,
+            response_tx: response_tx2,
+        }).unwrap();
+        
+        // Should succeed but stats shouldn't increment
+        assert!(response_rx2.await.unwrap().is_ok());
+        let stats_guard = stats.read().await;
+        assert_eq!(stats_guard.blocks_processed, 1); // Not 2

Also applies to: 189-190, 205-206, 223-225, 336-337, 349-351, 356-357

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Incorrect store_utxo call — still using old single-arg signature

This will fail to compile against the updated trait. Pass the outpoint and the UTXO.

-        storage.store_utxo(&utxo1).await.unwrap();
+        storage.store_utxo(&utxo1.outpoint, &utxo1).await.unwrap();

To catch any other outdated call sites in the repo, run:

---

🏁 Script executed:

#!/bin/bash
# Find single-argument store_utxo calls (likely outdated)
rg -nP -C2 --type=rust '\bstore_utxo\s*\(\s*[^,]*\s*\)\s*\.await'

Length of output: 449

---

Incorrect store_utxo call — update to two-arg signature

The test still uses the old single-argument API and will fail to compile. ripgrep confirms this is the only single-arg call.

• Location:
  • dash-spv/src/client/consistency_test.rs:257

Suggested fix:

-        storage.store_utxo(&utxo1).await.unwrap();
+        storage.store_utxo(&utxo1.outpoint, &utxo1).await.unwrap();

📝 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

	storage.store_utxo(&utxo1).await.unwrap();
	storage.store_utxo(&utxo3).await.unwrap();
	storage.store_utxo(&utxo3.outpoint, &utxo3).await.unwrap();
	storage.store_utxo(&utxo1.outpoint, &utxo1).await.unwrap();
	storage.store_utxo(&utxo3.outpoint, &utxo3).await.unwrap();

🤖 Prompt for AI Agents

In dash-spv/src/client/consistency_test.rs around lines 257 to 259, the test
calls storage.store_utxo with the old single-argument API which no longer
exists; update the call to the two-argument signature by passing the UTXO's
outpoint as the first argument and the UTXO reference as the second (match the
pattern used for utxo3), then run a build to ensure types align and adjust
imports or references if necessary.

[coderabbitai]

⚠️ Potential issue

Remove duplicate Network import

There's a duplicate import of Network from dashcore.

-    use dashcore::Network;
     use dashcore::{Block, BlockHash, Network, Transaction};

📝 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

	use dashcore::Network;
	use dashcore::{Block, BlockHash, Network, Transaction};
	use dashcore_hashes::Hash;
	use std::collections::HashSet;
	use std::sync::Arc;
	use std::sync::Mutex;
	use tokio::sync::{mpsc, RwLock};
	use dashcore::{Block, BlockHash, Network, Transaction};
	use dashcore_hashes::Hash;
	use std::collections::HashSet;
	use std::sync::Arc;
	use std::sync::Mutex;
	use tokio::sync::{mpsc, RwLock};

🤖 Prompt for AI Agents

In dash-spv/src/client/message_handler_test.rs around lines 20 to 26, there is a
duplicate import of Network from dashcore; remove the redundant `Network` entry
so the `use dashcore::{Block, BlockHash, Network, Transaction};` line contains a
single Network import (or consolidate to a single `use dashcore::Network;` and
keep other imports unchanged) to eliminate the duplicate import.

[coderabbitai]

⚠️ Potential issue

Fix moved-value error in GetQRInfo arm (use of message after move).

message is moved into the match and cannot be reused in this arm. This will not compile. Reconstruct the message using the bound request value:

-            NetworkMessage::GetQRInfo(_) => {
+            NetworkMessage::GetQRInfo(req) => {
                 // We don't serve QRInfo requests, only make them
                 tracing::warn!("Received unexpected GetQRInfo request");
                 self.stats.other_messages += 1;
-                MessageHandleResult::Unhandled(message)
+                MessageHandleResult::Unhandled(NetworkMessage::GetQRInfo(req))
             }

📝 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

	NetworkMessage::QRInfo(qr_info) => {
	self.stats.qrinfo_messages += 1;
	MessageHandleResult::QRInfo(qr_info)
	}
	NetworkMessage::GetQRInfo(_) => {
	// We don't serve QRInfo requests, only make them
	tracing::warn!("Received unexpected GetQRInfo request");
	self.stats.other_messages += 1;
	MessageHandleResult::Unhandled(message)
	}
	NetworkMessage::QRInfo(qr_info) => {
	self.stats.qrinfo_messages += 1;
	MessageHandleResult::QRInfo(qr_info)
	}
	NetworkMessage::GetQRInfo(req) => {
	// We don't serve QRInfo requests, only make them
	tracing::warn!("Received unexpected GetQRInfo request");
	self.stats.other_messages += 1;
	MessageHandleResult::Unhandled(NetworkMessage::GetQRInfo(req))
	}

🤖 Prompt for AI Agents

In dash-spv/src/network/message_handler.rs around lines 91 to 100, the GetQRInfo
match arm attempts to return MessageHandleResult::Unhandled(message) but
`message` has been moved into the match and cannot be reused; replace the use of
the moved `message` by reconstructing the original message from the bound
request value (the underscore in NetworkMessage::GetQRInfo(_) should be replaced
with a binding like `req`), then return
MessageHandleResult::Unhandled(NetworkMessage::GetQRInfo(req)) (or otherwise
wrap the bound value back into the same enum variant) and keep the stats and
warning log as-is.

[coderabbitai]

⚠️ Potential issue

Fix method signature issue

The find_scheduled_request method is called but not implemented. You might need to modify the approach:

     /// Handle a failed request - reschedule with backoff if retries available
-    pub fn handle_request_failure(&mut self, mut request: QRInfoRequest, error: &ParallelExecutionError) {
-        if let Some(mut scheduled) = self.find_scheduled_request(&request) {
+    pub fn handle_request_failure(&mut self, request: QRInfoRequest, error: &ParallelExecutionError) {
+        // Create a new scheduled request for retry instead of finding existing one
+        let mut scheduled = ScheduledRequest {
+            request: request.clone(),
+            priority: SchedulePriority::Normal, // Will be adjusted below
+            scheduled_time: Instant::now(),
+            retry_count: 1, // This would need to be tracked differently
+            max_retries: 3,
+        };

Committable suggestion skipped: line range outside the PR's diff.

[coderabbitai]

🛠️ Refactor suggestion

Resolve trait object safety issues

The recovery strategies use trait objects which may have issues with async methods. Consider using an enum-based approach:

-        let mut recovery_strategies: Vec<Box<dyn RecoveryStrategy>> = vec![
-            Box::new(ExponentialBackoffStrategy::new()),
-            Box::new(NetworkSwitchStrategy::new()),
-            Box::new(FallbackToSequentialStrategy::new()),
-            Box::new(MnListDiffFallbackStrategy::new()),
-        ];
+        let recovery_strategies = vec![
+            RecoveryStrategyType::ExponentialBackoff(ExponentialBackoffStrategy::new()),
+            RecoveryStrategyType::NetworkSwitch(NetworkSwitchStrategy::new()),
+            RecoveryStrategyType::FallbackToSequential(FallbackToSequentialStrategy::new()),
+            RecoveryStrategyType::MnListDiffFallback(MnListDiffFallbackStrategy::new()),
+        ];

This avoids trait object issues with async trait methods.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In qr_info_spv_plan/PHASE_3.md around lines 1064 to 1069, the Vec<Box<dyn
RecoveryStrategy>> uses trait objects which can cause trait-object/async method
safety problems; replace the boxed trait-object approach with an enum-based
strategy: define an enum RecoveryStrategyEnum with variants for
ExponentialBackoff, NetworkSwitch, FallbackToSequential, MnListDiffFallback,
implement an async method on the enum (e.g. async fn execute(&self, /* params
*/) { match self { RecoveryStrategyEnum::ExponentialBackoff(s) =>
s.execute(/*...*/), ... } }) that forwards to the concrete strategy
implementations, change the Vec type to Vec<RecoveryStrategyEnum>, and update
all call sites to use the enum's execute method instead of trait-dynamic
dispatch.

[coderabbitai]

🛠️ Refactor suggestion

Formalize the “non-standalone” state or make the package self-contained.

Two actionable paths:

• If this package is truly only consumable via the unified SDK, reflect that structurally:
  • Remove public products that cannot be built outside the unified SDK, or move these targets/products into the unified SDK repo.
  • Add clear README docs and a package-level note explaining the limitation and supported consumption mode.
• If you want SPM users to consume this package directly, reintroduce the FFI as a binaryTarget (preferred) and remove ad-hoc link flags. Example approach:
  • Produce an XCFramework for the FFI and declare it with .binaryTarget(name: "DashSPVFFI", path: "Artifacts/DashSPVFFI.xcframework").
  • Depend on that binary target from your Swift targets that need it.

Also, there are still linker search paths (Lines 42–49) pointing to the removed DashSPVFFI directory and local absolute paths. Those will not work on other machines and should be removed or replaced with a binaryTarget-based setup.

I can draft a binaryTarget-based Package.swift layout if you confirm the intended distribution model (standalone vs. unified-SDK-only).

🤖 Prompt for AI Agents

In swift-dash-core-sdk/Package.swift around lines 26–27 (and note linker flags
at ~42–49), the package claims a removed DashSPVFFI target but still exposes
products and uses local linker/search paths; either formalize that this package
is unified-SDK-only by removing/moving any public products that require
DashSPVFFI and adding a clear package-level README/note stating the consumption
mode, or make the package self-contained by reintroducing DashSPVFFI as a
.binaryTarget pointing to an XCFramework (e.g.,
Artifacts/DashSPVFFI.xcframework) and have Swift targets depend on that binary
target; in either case remove the ad-hoc -L/-l linker flags and any
absolute/local search paths (lines ~42–49) and replace them with the
binaryTarget-based dependency or remove the dead configuration and update
products/targets accordingly.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Script uses unsupported CLI flags: replace --checkpoint/--checkpoint-hash with --start-height

The current dash-spv CLI (see dash-spv/src/main.rs) does not define --checkpoint or --checkpoint-hash, so the script will fail. Use --start-height to target the nearest checkpoint to your desired height.

Run this to confirm no checkpoint flags exist:

Apply this patch:

-    --checkpoint 1100000 \
-    --checkpoint-hash 00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c \
+    --start-height 1100000 \

---

🏁 Script executed:

#!/bin/bash
rg -nP "Arg::new\([\"']checkpoint" dash-spv/src/main.rs -n -C2 || true

Length of output: 73

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "Searching repository for relevant CLI flags and arg definitions..."

echo; echo "1) occurrences of 'checkpoint' (context):"
rg -n --hidden -S -C2 '\bcheckpoint\b' || true

echo; echo "2) literal '--checkpoint' occurrences:"
rg -n --hidden -S -C2 -- '--checkpoint' || true

echo; echo "3) occurrences of 'checkpoint-hash':"
rg -n --hidden -S -C2 'checkpoint-hash' || true

echo; echo "4) occurrences of 'start-height' and 'start_height':"
rg -n --hidden -S -C2 'start-height' || true
rg -n --hidden -S -C2 'start_height' || true

echo; echo "5) common Arg/Clap patterns referencing checkpoint or start:"
rg -n --hidden -S -C2 "Arg::new\\([\"']checkpoint" || true
rg -n --hidden -S -C2 "Arg::new\\([\"']checkpoint-hash" || true
rg -n --hidden -S -C2 '\.long\([\"\']checkpoint' || true
rg -n --hidden -S -C2 '\.long\([\"\']start-height' || true
rg -n --hidden -S -C2 '#\\[derive\\(Parser\\)\\]' || true
rg -n --hidden -S -C2 'clap::Parser' || true

echo; echo "6) show dash-spv/src/main.rs (if present, first 300 lines):"
if [ -f dash-spv/src/main.rs ]; then
  sed -n '1,300p' dash-spv/src/main.rs || true
else
  echo "dash-spv/src/main.rs not found"
fi

Length of output: 231024

---

Replace unsupported CLI flags in test_smart_algo.sh — use --start-height

The dash-spv binary exposes --start-height (dash-spv/src/main.rs) and does not accept --checkpoint / --checkpoint-hash. Update the script to use --start-height and remove the checkpoint-hash argument.

Files to change:

• test_smart_algo.sh (lines ~8–13): replace the two checkpoint flags.
• Evidence: dash-spv/src/main.rs defines Arg::new("start-height") (.long("start-height")) and reads matches.get_one("start-height") (see lines ~112–117 and ~185–196).

Apply this patch:

-    --checkpoint 1100000 \
-    --checkpoint-hash 00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c \
+    --start-height 1100000 \

📝 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

	./target/debug/dash-spv \
	--network testnet \
	--data-dir ./test-smart-algo \
	--checkpoint 1100000 \
	--checkpoint-hash 00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c \
	2>&1 | tee smart_algo_debug.log
	./target/debug/dash-spv \
	--network testnet \
	--data-dir ./test-smart-algo \
	--start-height 1100000 \
	2>&1 | tee smart_algo_debug.log

🤖 Prompt for AI Agents

In test_smart_algo.sh around lines 8 to 13, the script passes unsupported CLI
flags --checkpoint and --checkpoint-hash to dash-spv; replace those two flags
with the supported --start-height flag and drop the checkpoint hash. Concretely,
remove "--checkpoint 1100000 --checkpoint-hash 00000baf..." and add
"--start-height 1100000" so the dash-spv invocation uses the supported argument.

[coderabbitai]

⚠️ Potential issue

Fix the assertion logic for genesis hash comparison

The test is asserting that the string representation of the BlockHash equals the original hex constants, but this will fail because BlockHash::to_string() returns the hash in reverse byte order (little-endian display format), while the constants are in big-endian format.

After reversing the bytes in the mainnet_genesis_hash() and testnet_genesis_hash() functions, the BlockHash::to_string() will output the hash in a different format than the input constants.

Either:

1. Remove these assertions if they're not needed
2. Store both the big-endian and little-endian representations
3. Use a proper comparison method that accounts for the byte order

-        assert_eq!(mainnet.to_string(), MAINNET_GENESIS_HASH);
-        assert_eq!(testnet.to_string(), TESTNET_GENESIS_HASH);
+        // The BlockHash is stored internally in little-endian but displayed in big-endian
+        // so we can't directly compare the string representation with our constants
+        // Instead, verify they're different networks
+        assert_ne!(mainnet, testnet, "Genesis hashes should be different for mainnet and testnet");

📝 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

	assert_ne!(mainnet, testnet);
	assert_eq!(mainnet.to_string(), MAINNET_GENESIS_HASH);
	assert_eq!(testnet.to_string(), TESTNET_GENESIS_HASH);
	assert_ne!(mainnet, testnet);
	// The BlockHash is stored internally in little-endian but displayed in big-endian
	// so we can't directly compare the string representation with our constants
	// Instead, verify they're different networks
	assert_ne!(mainnet, testnet, "Genesis hashes should be different for mainnet and testnet");

🤖 Prompt for AI Agents

In test-utils/src/fixtures.rs around lines 101 to 103, the assertions compare
BlockHash::to_string() (which prints little-endian) to big-endian hex constants,
causing false failures; fix by comparing the hash values directly or normalizing
endianness: either remove the string equality asserts, or parse the hex
constants into BlockHash and assert equality of the BlockHash instances (or
compare to the reversed-byte hex if you must use strings). Update the test to
assert on BlockHash equality (or on appropriately endian-normalized strings)
instead of comparing to the original big-endian constant via to_string().

[DCG-Claude]

🤖 CodeRabbitAI Fixes Summary

I've automatically addressed CodeRabbitAI review comments from this PR. Here's what was done:

📊 Statistics

• Total issues identified: 28
• Commits created: 19
• Agents spawned: 28

📝 Commits

The following commits were created to address the issues:

e9de17a3 fix: integrate Phase 3 with existing dash-spv interfaces instead of duplicating
086e8c40 fix(qr_info_spv_plan): replace trait object Vec with enum-based recovery strategies
9a4f0760 fix(qr_info_spv_plan): replace send_error with proper Result type handling
5786b788 fix(phase-3): correct processing_time to store elapsed duration instead of instant
ef25429b fix(dash-spv): update MockStorageManager to match current StorageManager trait
4d34ad4c fix(dash-spv): replace hardcoded quorum size with dynamic LLMQ size
789c4814 fix(docs): update API documentation to match actual dash-spv implementation
ab36dd1a fix(sml): use per-window mining_start for LLMQ type skip check
2e34b1bd fix(dash-spv): add bounds checking to sync phase completion methods
9cb695f7 fix(dash-spv): correct test assertion for qr_info_extra_share default
ae5dbe58 fix(dash-spv): replace MemoryStorage with MemoryStorageManager in validation tests
93b12b52 fix(dash-spv): use actual block height in MnListDiff validation errors
d7743fd0 fix(dash-spv): resolve storage handle shadowing in chainlock validation test
59339989 fix(dash-spv): align block message expectations with runtime behavior in MnList phase
6be28eb8 fix(dash-spv): add explicit error handling to find_chain_lock_quorum function
87f221d7 fix(dash-spv): correct NetworkMessage enum variant name from GetMnListD to GetMnListDiff
0d583ef5 fix: resolve all dash-spv test compilation errors and failures
7fcdf8b9 fix(dash-spv): unify status display height calculation for genesis and checkpoint sync
1b38e9c6 fix(dash-spv): unify status display height calculation for genesis and checkpoint sync

✅ Verification

• All changes passed cargo check
• Each fix was committed atomically for easy review

🔍 Review Instructions

1. Review each commit individually to verify the fixes
2. Run tests to ensure no regressions
3. Check that the fixes align with project standards

---

This summary was generated automatically by /fix-coderabbit-pr