feat: dash spv shared base

[PastaPastaPasta]

Summary by CodeRabbit

• New Features

  • Mempool tracking with callbacks, balances, and configuration.
  • Detailed sync progress reporting and new completion/progress callbacks.
  • Bloom filter builder/manager and stats.
  • Chain features: reorg handling, fork detection, chain locks, checkpoints, terminal blocks.
  • iOS/Swift/C FFI additions and a Unified SDK option.
  • CLI enhancements: start height and optional terminal UI.
  • Network updates: headers2 groundwork, DNS discovery tuning, connection timeouts.

• Documentation

  • Overhauled guides; added Unified SDK, terminal blocks, UTXO rollback docs; README updates.

• Tests

  • Extensive new suites for mempool, callbacks, platform integration, bloom, chain, network, and examples.

• Refactor

  • Thread-safe/global error handling; sequential sync architecture.

[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]

Walkthrough

This PR adds UniFFI-based Dash Network bindings, overhauls dash-spv-ffi with mempool support, new callbacks, progress APIs, platform bridge, and error handling. It introduces Bloom, Chain, MempoolFilter subsystems, refactors client/network flow to SequentialSyncManager, adds terminal block data, numerous examples/tests, and updates documentation and configs.

Changes

Cohort / File(s)	Summary
Repo meta & docs /.gitignore, /README.md, /CLAUDE.md, /TEST_SUMMARY.md, dash-spv/CLAUDE.md, dash-spv/docs/*	Ignore compiled libs; README bullets; CLAUDE.md reoriented to Rust/Dash; added comprehensive test summary; updated architecture docs incl. sequential sync and terminal blocks.
iOS Unified SDK docs /UNIFIED_SDK.md, dash-spv-ffi/README.md	Added Unified SDK guide and expanded FFI README with build modes, usage, and iOS xcframework notes.
Dash Network FFI (UniFFI) dash-network-ffi/src/dash_network_ffi.swift, .../dash_network_ffiFFI.h, .../dash_network_ffiFFI.modulemap	New Swift and C headers/modulemap exposing NetworkInfo, Network, errors, initialization, and UniFFI plumbing.
SPV FFI: config/build dash-spv-ffi/Cargo.toml, dash-spv-ffi/cbindgen.toml	Added logging/tracing dev deps; excluded legacy Option_* callbacks from header generation.
SPV FFI: API surface dash-spv-ffi/include/dash_spv_ffi.h, dash-spv-ffi/src/{callbacks.rs,client.rs,config.rs,error.rs,types.rs,wallet.rs,lib.rs}	Introduced mempool strategy/types, detailed progress, new callbacks (including mempool), new sync APIs (with progress/cancel), global error storage, richer types (FFIString, FFIArray, unconfirmed tx), platform module export.
SPV FFI: platform integration dash-spv-ffi/src/platform_integration.rs	Added CoreSDKHandle and FFIResult; quorum key/platform height stubs with validation and error propagation.
SPV FFI: tests dash-spv-ffi/tests/*	Updated for new FFI shapes, added tests for callbacks, mempool, platform bridge, async ops, lifecycle, error handling, type conversions, and integration.
SPV core: Bloom module dash-spv/src/bloom/*, dash-spv/src/lib.rs	New Bloom builder/manager/stats/utils modules with extensive tests; exported via crate root.
SPV core: Chain module dash-spv/src/chain/*, dash-spv/src/lib.rs	New chain subsystems: tips, chainlocks, checkpoints, fork detector, orphan pool, reorg; tests added; public re-exports.
SPV core: Client pipeline dash-spv/src/client/{block_processor*.rs,config*.rs,consistency*.rs,message_handler*.rs,status_display.rs,watch_manager*.rs}, dash-spv/src/main.rs	Event streaming added, mempool config and validation, sequential sync message handling, safer watch manager, status progress fields, CLI/start-from-height, error-mapped flows.
SPV core: Network stack dash-spv/src/network/{mod.rs,connection.rs,handshake.rs,message_handler.rs,addrv2.rs,discovery.rs,mock.rs,constants.rs}	Connection refactor with timeouts; handshake state/capabilities, DSQ preference; Headers2 result paths; manager trait expansions; constants tuned; tests/mocks added.
SPV core: Examples dash-spv/examples/*	Added multiple examples for reorgs, headers2, initial sync, terminal blocks, and diagnostics.
SPV core: Terminal blocks data/docs dash-spv/data/{mainnet,testnet}/mod.rs, dash-spv/docs/TERMINAL_BLOCKS.md	Embedded terminal block JSON loaders and documentation; tests for parsing/loading.
SPV core: Scripts dash-spv/scripts/fetch_terminal_blocks.py	Script to fetch/generate terminal block data and Rust loaders.
Cargo updates dash-spv/Cargo.toml	Enabled additional dashcore features; added blsful, hex, indexmap.

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant FFI as FFIDashSpvClient
  participant Sync as SequentialSyncManager
  participant Net as NetworkManager
  participant MH as MessageHandler
  participant Store as Storage
  participant Wallet
  participant Events as EventChannel/FFI callbacks

  App->>FFI: sync_to_tip_with_progress(onProgress,onComplete)
  FFI->>Sync: start()
  Sync->>Net: connect + discover
  Net-->>MH: NetworkMessage(s)
  MH->>Sync: handle_message(...)
  MH->>Store: persist headers/filters/blocks
  MH->>Wallet: update balances/tx state
  MH-->>Events: SpvEvent{Progress/Block/Tx/Balance}
  Events-->>FFI: progress & event callbacks
  Sync-->>FFI: completion(status, error?)
  FFI-->>App: completion callback

Loading

sequenceDiagram
  participant Peer
  participant Net
  participant MH
  participant MF as MempoolFilter
  participant MS as MempoolState
  participant Store
  participant Events

  Peer-->>Net: Inv/Tx
  Net-->>MH: Tx message
  MH->>MF: is_transaction_relevant?
  alt relevant
    MH->>MS: record + track
    MH->>Store: persist if enabled
    MH-->>Events: MempoolTransactionAdded(txid, amount, addrs, IX)
  else not relevant
    MH-->>Net: ignore
  end

Loading

Estimated code review effort

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

Possibly related PRs

• feat: dash-spv-ffi #76 — Also restructures dash-spv-ffi APIs and headers; overlaps on FFI types, callbacks, and Cargo changes.
• feat: initial implementation of SPV client in rust-dashcode #75 — Modifies many dash-spv client/network/sync modules; overlaps with sequential sync and message handling refactors.
• chore: new BLS library #42 — Adjusts BLS dependencies/features; related to added blsful and dashcore feature flags in Cargo.toml.

Suggested reviewers

• pankcuf
• QuantumExplorer

Poem

In wires of rust and mempools deep,
I hop through headers in a leap.
Blooming filters, forks in sight,
ChainLocks glow in moonlit byte.
Callbacks squeak—progress sings,
A unified burrow for all the things.
— Your diligent bunny 🐇✨

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/dash-spv-shared-base

---

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.
  • Explain this complex logic.
  • 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. Examples:
  • @coderabbitai explain this code block.
• 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 src/utils.ts and explain its main purpose.
  • @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 comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai generate unit tests to generate unit tests for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

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

Documentation and Community

• 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: 57

🔭 Outside diff range comments (6)

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

129-130: Incorrect sync_start timestamp initialization

The sync_start field is being set to the current time on every call to sync_progress(), which doesn't represent when synchronization actually started. This should be tracked from when the sync process begins and persisted across calls.

Consider storing the actual sync start time in ChainState or SpvStats and using that value here.

dash-spv/src/client/consistency.rs (2)

162-167: addresses_synced stays at 0; either implement address syncing or remove the metric.

You populate address mismatch reporting but do not reconcile it. Either:

• Implement syncing (e.g., add/remove addresses in wallet based on watch_items) and increment addresses_synced, or
• Drop/rename the field to avoid misleading reporting.

I can draft a minimal reconciliation step that syncs watch addresses into the wallet and updates the counter if desired.

---

71-75: Refactor to avoid holding the RwLock guard across await points
Holding the RwLockReadGuard<Wallet> while calling wallet.get_utxos().await (and similarly for get_watched_addresses().await) keeps the lock held for the entire duration of the async call. This can lead to lock contention or deadlocks if Wallet’s own async methods also acquire locks internally.

Please update in dash-spv/src/client/consistency.rs:

• Around lines 71–75 (UTXO validation)
• Around lines 101–104 (address validation)
• Around lines 179–184 (UTXO recovery)
• Around lines 186–192 and 198–204 (adding/removing UTXOs in recovery)

Suggested patterns:

• Introduce a synchronous “snapshot” method on Wallet that clones the necessary data under the guard, then drop the guard before any await. For example:

  // in Wallet:
  pub fn snapshot_utxos(&self) -> Vec<Utxo> { /* clone internal state */ }
  // in ConsistencyManager:
  let wallet_utxos = {
      let guard = self.wallet.read().await;
      guard.snapshot_utxos()
  };  // guard dropped here
  // now you can await storage.get_all_utxos()

• Do the same for watched addresses.
• Alternatively, consider removing the outer RwLock<Wallet> and rely on Wallet’s own interior async locks, passing around Arc<Wallet> instead of Arc<RwLock<Wallet>>.

These changes will ensure the lock is only held briefly for data cloning, eliminating risk of deadlock and improving concurrency.

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

378-391: UTXO storage failures must stop block processing

Failing to store UTXOs will cause the wallet to lose track of spendable outputs, leading to incorrect balance calculations.

Return error instead of continuing:

                         let wallet = self.wallet.read().await;
                         if let Err(e) = wallet.add_utxo(utxo).await {
-                            tracing::error!("Failed to store UTXO {}: {}", outpoint, e);
-                            tracing::warn!(
-                                "Continuing block processing despite UTXO storage failure"
-                            );
+                            return Err(SpvError::Wallet(format!(
+                                "Failed to store UTXO {}: {}", outpoint, e
+                            )));
                         } else {

dash-spv-ffi/tests/unit/test_async_operations.rs (1)

85-96: Test doesn't actually test null callback handling

The test claims to test null callback safety but only creates and destroys a client without actually testing any null callbacks. The comment acknowledges this but the test name is misleading.

Either rename the test to reflect what it actually tests or add actual null callback testing:

-    fn test_callback_with_null_functions() {
+    fn test_client_destruction_safety() {

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

59-60: Catch-all pattern in network conversion could hide errors

The wildcard pattern in the From<Network> implementation silently converts unknown networks to Dash.

 impl From<Network> for FFINetwork {
     fn from(net: Network) -> Self {
         match net {
             Network::Dash => FFINetwork::Dash,
             Network::Testnet => FFINetwork::Testnet,
             Network::Regtest => FFINetwork::Regtest,
             Network::Devnet => FFINetwork::Devnet,
-            _ => FFINetwork::Dash,
+            // The Network enum is non-exhaustive, but we should handle this more explicitly
+            _ => {
+                tracing::warn!("Unknown network variant encountered, defaulting to Dash");
+                FFINetwork::Dash
+            }
         }
     }
 }

🧹 Nitpick comments (66)

.gitignore (1)

29-32: It looks like the verification script failed due to incorrect ripgrep flags. To ensure we’re not accidentally ignoring any tracked binaries or bundles, please rerun with one of these corrected commands and share the output:

#!/usr/bin/env bash
set -euo pipefail

echo "Checking tracked native libs and related artifacts..."
git ls-files | grep -En '\.(a|so($|\.)|dylib|dll|lib|exp|pdb)$' || true

echo "Checking tracked Apple bundles and debug symbols..."
git ls-files | grep -En '\.(dSYM|framework|xcframework)(/|$)' || true

If this prints nothing, we’re safe to extend .gitignore as suggested. Otherwise, you’ll see exactly which files would be ignored so you can relocate them or explicitly whitelist them.

dash-spv/examples/test_genesis.rs (4)

12-12: Use fixed-width hex for bits for consistent output.

Pad to 8 hex digits so values always align.

-    println!("Bits: {:x}", mainnet_genesis.header.bits.to_consensus());
+    println!("Bits: {:08x}", mainnet_genesis.header.bits.to_consensus());
@@
-    println!("Bits: {:x}", testnet_genesis.header.bits.to_consensus());
+    println!("Bits: {:08x}", testnet_genesis.header.bits.to_consensus());

Also applies to: 22-22

---

26-33: Replace “Expected Testnet Values” prints with assertions to catch regressions.

Right now, you print expected constants but don’t verify. Asserting makes this example fail fast if genesis constants ever change.

-    // Expected values
-    println!("=== Expected Testnet Values ===");
-    println!("Hash: 00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c");
-    println!("Time: 1390666206");
-    println!("Nonce: 3861367235");
-    println!("Bits: 1e0ffff0");
-    println!("Merkle root: e0028eb9648db56b1ac77cf090b99048a8007e2bb64b68f092c03c7f56a662c7");
+    // Validate expected testnet values
+    println!("=== Validating Testnet Genesis ===");
+    assert_eq!(
+        testnet_genesis.block_hash().to_string(),
+        "00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c"
+    );
+    assert_eq!(testnet_genesis.header.time, 1390666206);
+    assert_eq!(testnet_genesis.header.nonce, 3861367235);
+    assert_eq!(testnet_genesis.header.bits.to_consensus(), 0x1e0ffff0);
+    assert_eq!(
+        testnet_genesis.header.merkle_root.to_string(),
+        "e0028eb9648db56b1ac77cf090b99048a8007e2bb64b68f092c03c7f56a662c7"
+    );
+    println!("Testnet genesis values OK");

---

6-25: Deduplicate the repeated print logic with a tiny helper (optional).

Reduces repetition and keeps the example tidy.

Example helper to add above main:

use dashcore::Block;

fn print_genesis(label: &str, b: &Block) {
    println!("=== {label} Genesis ===");
    println!("Hash: {}", b.block_hash());
    println!("Time: {}", b.header.time);
    println!("Nonce: {}", b.header.nonce);
    println!("Bits: {:08x}", b.header.bits.to_consensus());
    println!("Merkle root: {}", b.header.merkle_root);
    println!();
}

Then call:

print_genesis("Mainnet", &mainnet_genesis);
print_genesis("Testnet", &testnet_genesis);

---

3-3: Naming nit: consider renaming the example to avoid “test_” prefix.

“examples/genesis_print.rs” or “examples/genesis_baseline.rs” avoids confusion with integration tests under tests/.

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

122-126: Multiple unimplemented features marked with TODO

There are several TODO items that need implementation:

• Getting actual peer count from network manager
• Implementing headers_synced flag
• Implementing filter_headers_synced flag
• Implementing masternodes_synced flag
• Getting filter_sync_available from network manager

Would you like me to create GitHub issues to track these unimplemented features?

dash-spv/examples/test_terminal_blocks.rs (1)

25-25: Use println!() without an empty string argument

The empty string argument is unnecessary.

-                println!("");
+                println!();

dash-spv/data/mainnet/mod.rs (2)

9-11: Consider logging parsing errors instead of silently ignoring them

The function silently ignores JSON parsing errors. Consider logging these errors to help with debugging if the embedded data is corrupted or the format changes.

-        if let Ok(state) = serde_json::from_str::<TerminalBlockMasternodeState>(data) {
-            manager.add_state(state);
-        }
+        match serde_json::from_str::<TerminalBlockMasternodeState>(data) {
+            Ok(state) => manager.add_state(state),
+            Err(e) => {
+                // Log the error for debugging purposes
+                eprintln!("Warning: Failed to parse terminal block 2000000: {}", e);
+            }
+        }

---

35-35: Remove trailing whitespace

-            let path = entry.path();
-            
+            let path = entry.path();
+

dash-spv/data/testnet/mod.rs (1)

5-110: Consider refactoring repetitive code and improving error handling

The function contains repetitive code blocks that could be simplified, and silently ignores parsing errors.

Since this is auto-generated code, consider updating the generator script to produce more maintainable code:

pub fn load_testnet_terminal_blocks(manager: &mut TerminalBlockDataManager) {
    const TERMINAL_BLOCKS: &[(&str, u32)] = &[
        (include_str!("terminal_block_387480.json"), 387480),
        (include_str!("terminal_block_400000.json"), 400000),
        // ... other blocks
        (include_str!("terminal_block_900000.json"), 900000),
    ];
    
    for (data, height) in TERMINAL_BLOCKS {
        match serde_json::from_str::<TerminalBlockMasternodeState>(data) {
            Ok(state) => manager.add_state(state),
            Err(e) => eprintln!("Warning: Failed to parse terminal block {}: {}", height, e),
        }
    }
}

This would:

1. Reduce code duplication
2. Make maintenance easier
3. Provide error visibility

dash-spv/docs/TERMINAL_BLOCKS.md (1)

28-37: Add language specifiers to fenced code blocks

The markdown linter identified code blocks without language specifiers.

 Without terminal blocks:
-```
+```text
 Request: Genesis (0) → Current (1,276,272)
 Diff size: ~500MB, covering 1.2M blocks

With terminal blocks:
- +text
Request: Terminal Block (900,000) → Current (1,276,272)
Diff size: ~100MB, covering 376K blocks

dash-spv/scripts/fetch_terminal_blocks.py (1)

161-170: Consider generating more maintainable Rust code

The generated Rust code contains repetitive blocks that could be simplified using arrays or macros, as suggested in the review of the generated files.

Consider generating code that uses a loop or macro to reduce repetition. This would make the generated code more maintainable and reduce binary size. See the suggestion in the testnet/mod.rs review for an example implementation.

dash-spv/src/client/consistency.rs (3)

185-195: Reduce lock churn: move wallet lock outside the loop.

You're acquiring a wallet read lock for each UTXO added. Hoist it outside to avoid repeated locking.

-        for (outpoint, storage_utxo) in &storage_utxos {
-            if !wallet_utxos.iter().any(|wu| &wu.outpoint == outpoint) {
-                let wallet = self.wallet.read().await;
-                if let Err(e) = wallet.add_utxo(storage_utxo.clone()).await {
+        let wallet = self.wallet.read().await;
+        for (outpoint, storage_utxo) in &storage_utxos {
+            if !wallet_utxos.iter().any(|wu| &wu.outpoint == outpoint) {
+                if let Err(e) = wallet.add_utxo(storage_utxo.clone()).await {
                     tracing::error!("Failed to sync UTXO {} to wallet: {}", outpoint, e);
                     recovery.success = false;
                 } else {
                     recovery.utxos_synced += 1;
                 }
             }
         }

Note: This still holds a lock across await; best is to expose a batch API on Wallet or avoid outer RwLock entirely if Wallet is internally synchronized.

---

198-212: Likewise, hoist the wallet lock outside the removal loop.

Same pattern applies for removals to avoid locking per iteration.

-        for wallet_utxo in &wallet_utxos {
-            if !storage_utxos.contains_key(&wallet_utxo.outpoint) {
-                let wallet = self.wallet.read().await;
-                if let Err(e) = wallet.remove_utxo(&wallet_utxo.outpoint).await {
+        let wallet = self.wallet.read().await;
+        for wallet_utxo in &wallet_utxos {
+            if !storage_utxos.contains_key(&wallet_utxo.outpoint) {
+                if let Err(e) = wallet.remove_utxo(&wallet_utxo.outpoint).await {
                     tracing::error!(
                         "Failed to remove UTXO {} from wallet: {}",
                         wallet_utxo.outpoint,
                         e
                     );
                     recovery.success = false;
                 } else {
                     recovery.utxos_removed += 1;
                 }
             }
         }

---

187-189: Mutation under read guard: verify thread-safety and lock layering.

Calling wallet.add_utxo(...).await and wallet.remove_utxo(...).await while holding an outer RwLock read guard suggests interior mutability inside Wallet. This can easily lead to lock-order inversions or deadlocks if Wallet also uses async locks.

• Confirm Wallet’s internal locking and ensure no attempt to acquire self.wallet’s RwLock inside these methods.
• If feasible, switch to write().await on the outer lock when mutating, or remove the outer lock and rely solely on Wallet’s own synchronization mechanism.

Happy to sketch a locking strategy document for StorageManager/Wallet to clarify expectations.

Also applies to: 201-205

dash-spv-ffi/tests/test_wallet.rs (1)

78-80: Assert new mempool fields are correctly mapped in FFIBalance.

You added mempool and mempool_instant to Balance but the test doesn’t verify their FFI mapping.

         let ffi_balance = FFIBalance::from(balance);
         assert_eq!(ffi_balance.confirmed, 100000);
         assert_eq!(ffi_balance.pending, 50000);
         assert_eq!(ffi_balance.instantlocked, 25000);
+        assert_eq!(ffi_balance.mempool, 0);
+        assert_eq!(ffi_balance.mempool_instant, 0);
         assert_eq!(ffi_balance.total, 175000);

Optionally add a second case with non-zero mempool values to ensure they contribute to total as intended.

dash-spv/README.md (1)

26-26: Consider linking to configuration knobs for reputation.

Add pointers to config keys (e.g., thresholds, ban duration, decay) so users can tune behavior from docs.

I can add a short “Configuration” subsection listing the relevant fields with defaults.

dash-spv-ffi/README.md (1)

5-6: Broken link & qualify size-reduction claim

The link to UNIFIED_SDK_ARCHITECTURE.md under ../../platform-ios/packages/rs-sdk-ffi/ appears to be invalid—please verify the correct path or update the reference.
Additionally, clarify the “79.4%” binary-size reduction: is this measured under specific build settings? If it varies, consider softening to “up to ~80%” or linking to a reproducible measurement script or log.

UNIFIED_SDK.md (1)

19-19: Verify the claimed size reduction.

The documentation claims a 79.4% size reduction (from 143MB to 29.5MB). This is a significant optimization claim that should be verified or documented with measurement methodology.

Consider adding a note about how this size reduction was measured (e.g., "measured on iOS release builds with specific configuration") to provide context for this optimization claim.

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

18-20: Consider using a more robust test address creation.

The hardcoded address string may become invalid if network parameters change. Consider using the existing create_test_address() helper from other test files for consistency.

Using the helper from relevant code snippets:

 fn create_test_address() -> Address {
-    Address::from_str("XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4").unwrap().assume_checked()
+    // Create a simple P2PKH address for testing
+    use dashcore::{PubkeyHash, ScriptBuf, Network};
+    use dashcore_hashes::Hash;
+    let pubkey_hash =
+        PubkeyHash::from_slice(&[1u8; 20]).expect("Valid 20-byte slice for pubkey hash");
+    let script = ScriptBuf::new_p2pkh(&pubkey_hash);
+    Address::from_script(&script, Network::Testnet)
+        .expect("Valid P2PKH script should produce valid address")
 }

dash-spv/src/bloom/stats.rs (1)

156-160: Questionable bandwidth estimation logic.

The bandwidth estimation multiplies transaction size by 10 as a "rough estimate" but this seems arbitrary and could be misleading. Consider documenting the rationale or making this configurable.

 } else {
     // Estimate bandwidth saved by not downloading unrelated transactions
-    // Assume average transaction size if this was a true positive
     self.stats.network_impact.bandwidth_saved_bytes += (tx_size * 10) as u64;
-    // Rough estimate
+    // Conservative estimate: assume we avoided downloading ~10 unrelated transactions
+    // This multiplier should be calibrated based on actual network conditions
 }

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

1127-1161: Improve shutdown reliability

The shutdown mechanism could miss some threads or take longer than necessary due to the 100ms polling interval in the event listener.

Consider using a more responsive shutdown mechanism:

 pub unsafe extern "C" fn dash_spv_ffi_client_destroy(client: *mut FFIDashSpvClient) {
     if !client.is_null() {
         let client = Box::from_raw(client);

         // Set shutdown signal to stop all threads
         client.shutdown_signal.store(true, Ordering::Relaxed);
+        
+        // Wake up any waiting threads immediately
+        // Consider using a Condvar or tokio::sync::Notify for immediate wakeup

         // Clean up any registered callbacks
         if let Some(ref callback_data) = *client.sync_callbacks.lock().unwrap() {
             CALLBACK_REGISTRY.lock().unwrap().unregister(callback_data.callback_id);
         }

dash-spv/src/chain/chain_work.rs (1)

63-77: Add overflow detection in combine function

The combine function could silently drop overflow from the most significant byte.

Consider adding overflow detection or documentation:

 pub fn combine(self, other: Self) -> Self {
     let mut result = [0u8; 32];
     let mut carry = 0u16;

     // Add from least significant byte (right) to most significant (left)
     for i in (0..32).rev() {
         let sum = self.work[i] as u16 + other.work[i] as u16 + carry;
         result[i] = (sum & 0xff) as u8;
         carry = sum >> 8;
     }
+    
+    // Note: If carry is non-zero here, we have an overflow
+    // This is astronomically unlikely in practice for chain work
+    debug_assert_eq!(carry, 0, "Chain work overflow detected");

     Self {
         work: result,
     }
 }

dash-spv/examples/test_initial_sync.rs (2)

19-21: Consider using DNS seeds instead of hardcoded IPs.

Hardcoded IP addresses may become unavailable over time. Consider using DNS seed discovery or making the peer addresses configurable via command-line arguments or environment variables for better maintainability.

-    config.peers =
-        vec!["54.68.235.201:19999".parse().unwrap(), "52.40.219.41:19999".parse().unwrap()];
+    // Use DNS discovery or make configurable
+    config.peers = if let Ok(peers_env) = std::env::var("DASH_TESTNET_PEERS") {
+        peers_env.split(',')
+            .filter_map(|p| p.parse().ok())
+            .collect()
+    } else {
+        // Fallback to known peers
+        vec!["54.68.235.201:19999".parse()?, "52.40.219.41:19999".parse()?]
+    };

---

49-50: Consider logging cleanup errors.

Ignoring the result of remove_dir_all could hide filesystem issues. Consider at least logging the error if cleanup fails.

     client.stop().await?;
-    let _ = std::fs::remove_dir_all(data_dir);
+    if let Err(e) = std::fs::remove_dir_all(data_dir) {
+        eprintln!("Warning: Failed to clean up temp directory: {}", e);
+    }

dash-spv/src/chain/chainlock_test.rs (1)

81-107: Consider using static error messages in expect().

Using format! inside expect() causes string allocation even when the test passes. Consider using a static message or moving the formatting outside the expect call.

             chainlock_manager
                 .process_chain_lock(chainlock, &chain_state, &mut storage)
                 .await
-                .expect(&format!("ChainLock at height {} should process successfully", height));
+                .unwrap_or_else(|e| panic!("ChainLock at height {} should process successfully: {:?}", height, e));

dash-spv/examples/test_headers2.rs (2)

21-22: Consider making seed addresses configurable.

Hardcoded DNS seed addresses may become stale over time. Consider reading them from environment variables or a configuration file for better maintainability.

-    config.peers =
-        vec!["seed.dash.org:9999".parse().unwrap(), "dnsseed.dash.org:9999".parse().unwrap()];
+    config.peers = std::env::var("DASH_TEST_SEEDS")
+        .unwrap_or_else(|_| "seed.dash.org:9999,dnsseed.dash.org:9999".to_string())
+        .split(',')
+        .filter_map(|s| s.parse().ok())
+        .collect();

---

39-96: Extract magic numbers as constants for clarity.

The example uses several magic numbers that would benefit from being named constants.

Add these constants at the top of the main function:

const MAX_ITERATIONS: u32 = 60;
const POLL_INTERVAL: Duration = Duration::from_secs(1);
const NO_PROGRESS_THRESHOLD: u32 = 10;
const SUCCESS_HEADER_COUNT: u32 = 1000;
const CONNECTION_CHECK_DELAY: u32 = 5;

Then update the code to use these constants instead of hardcoded values.

dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)

68-69: Consider adding mock-based tests for better CI coverage.

While properly marking network-dependent tests with #[ignore] is correct, it reduces test coverage in CI environments. Consider creating additional tests using a mock networking layer or test doubles to ensure these code paths are exercised in CI.

Would you like me to help create mock-based versions of these tests that can run in CI without real network connectivity?

Also applies to: 93-94, 112-113, 167-168, 254-255

dash-spv/src/client/watch_manager_test.rs (2)

71-71: Consider using a valid script format for testing.

While the dummy script vec![0x00, 0x14] works for testing storage/retrieval, consider using a valid Bitcoin script format to make tests more realistic and catch potential script-related issues.

-        let script = ScriptBuf::from(vec![0x00, 0x14]); // Dummy script
+        // Create a valid OP_RETURN script for testing
+        let script = ScriptBuf::from(vec![0x6a, 0x04, 0x74, 0x65, 0x73, 0x74]); // OP_RETURN "test"

---

34-34: Consider adding a comment about test address validity.

Using assume_checked() bypasses address validation. While acceptable for tests with known-valid addresses, a comment would help future maintainers understand this is intentional.

-        Address::from_str("XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4").unwrap().assume_checked()
+        // Using assume_checked() is safe here as this is a known-valid Dash testnet address
+        Address::from_str("XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4").unwrap().assume_checked()

dash-spv/src/chain/fork_detector_test.rs (1)

53-54: Consider using a more descriptive variable name.

The variable pre_checkpoint_hash could be more clearly named to indicate it's a synthetic/fake hash for testing.

-        let pre_checkpoint_hash =
-            BlockHash::from_raw_hash(dashcore_hashes::hash_x11::Hash::hash(&[99u8]));
+        let synthetic_pre_checkpoint_hash =
+            BlockHash::from_raw_hash(dashcore_hashes::hash_x11::Hash::hash(&[99u8]));

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

40-42: Consider documenting the null return behavior.

The function returns null both when there's no error and when the mutex is poisoned. FFI consumers should be aware of this behavior.

Add a doc comment explaining the return value semantics:

+/// Returns a pointer to the last error message, or null if:
+/// - No error has been set
+/// - The error storage mutex is poisoned (rare, indicates a panic in another thread)
+/// The returned pointer is valid until the next call to set_last_error or clear_error.
 #[no_mangle]
 pub extern "C" fn dash_spv_ffi_get_last_error() -> *const c_char {

dash-spv/src/chain/orphan_pool_test.rs (1)

39-39: Consider using tokio::time::sleep for async compatibility.

Using thread::sleep in tests can make them slower and less reliable. Consider using tokio's async sleep if the test framework supports it, or mock time for more deterministic testing.

-        thread::sleep(Duration::from_millis(150));
+        // Consider using tokio::time::sleep in async context or time mocking for deterministic tests
+        thread::sleep(Duration::from_millis(150));

dash-spv/src/bloom/tests.rs (1)

612-619: Handle edge cases gracefully in false positive rate tests.

Testing with a zero false positive rate (0.0) may cause issues in the underlying bloom filter implementation. Consider adding more explicit assertions about the expected behavior.

     fn test_builder_zero_false_positive_rate() {
         let builder = BloomFilterBuilder::new().false_positive_rate(0.0);
 
-        // Should handle edge case gracefully
-        let result = builder.build();
-        // Zero false positive rate might cause an error in the underlying library
-        // But our code should handle it gracefully
-        assert!(result.is_ok() || result.is_err());
+        // Zero false positive rate should either:
+        // 1. Be clamped to a minimum value and succeed, or
+        // 2. Return a specific error about invalid configuration
+        let result = builder.build();
+        match result {
+            Ok(filter) => {
+                // If successful, filter should be created with minimum viable FP rate
+                assert!(!filter.is_empty() || filter.is_empty());
+            }
+            Err(e) => {
+                // If error, should be a configuration error
+                assert!(e.to_string().contains("false positive") || e.to_string().contains("rate"));
+            }
+        }

dash-spv/examples/reorg_demo.rs (1)

14-14: Incorrect block time increment for Dash network

The comment mentions "10 minutes later" but Dash has a 2.5-minute (150 seconds) block time, not 10 minutes like Bitcoin.

Update the comment to reflect Dash's actual block time:

-    header.time = prev.time + 600; // 10 minutes later
+    header.time = prev.time + 150; // 2.5 minutes later (Dash block time)

dash-spv/src/chain/reorg_test.rs (3)

17-17: Incorrect block time increment for Dash network

Same issue as in reorg_demo.rs - Dash uses 2.5-minute block times, not 10 minutes.

-        header.time = prev.time + 600; // 10 minutes later
+        header.time = prev.time + 150; // 2.5 minutes later (Dash block time)

---

73-74: Add descriptive assertion messages for better test diagnostics

The assertions would be more helpful with custom error messages to identify which specific check failed during test execution.

-        assert_eq!(event.disconnected_headers.len(), 2); // block1 and block2
-        assert_eq!(event.connected_headers.len(), 3); // block1', block2', block3'
+        assert_eq!(event.disconnected_headers.len(), 2, "Expected 2 disconnected headers (block1 and block2)");
+        assert_eq!(event.connected_headers.len(), 3, "Expected 3 connected headers (block1', block2', block3')");

---

39-41: Consider verifying chain state before modifications

The comment indicates genesis is already added, but the code doesn't verify this assumption before proceeding.

Add an assertion to verify the initial state:

 // Update chain state - genesis is already added by new_for_network
+assert_eq!(chain_state.get_height(), 0, "Expected initial chain height to be 0 (genesis only)");
 chain_state.add_header(block1);
 chain_state.add_header(block2);

dash-spv/examples/test_headers2_fix.rs (1)

65-69: Consider using network constants for genesis hash

The hardcoded genesis hash bytes could be replaced with a constant or derived from the network's known genesis block.

Use the network's genesis block hash:

-    let genesis_hash = BlockHash::from_byte_array([
-        0x2c, 0xbc, 0xf8, 0x3b, 0x62, 0x91, 0x3d, 0x56, 0xf6, 0x05, 0xc0, 0xe5, 0x81, 0xa4, 0x88,
-        0x72, 0x83, 0x94, 0x28, 0xc9, 0x2e, 0x5e, 0xb7, 0x6c, 0xd7, 0xad, 0x94, 0xbc, 0xaf, 0x0b,
-        0x00, 0x00,
-    ]);
+    let genesis_hash = dashcore::blockdata::constants::genesis_block(network).block_hash();

dash-spv/examples/test_header_count.rs (1)

22-24: Improve error handling for directory operations

Directory removal operations could fail but errors might not be properly reported, potentially causing test flakiness.

Consider more robust cleanup:

 // Clean up any previous test data
 if std::path::Path::new(storage_dir).exists() {
-    std::fs::remove_dir_all(storage_dir)?;
+    std::fs::remove_dir_all(storage_dir)
+        .map_err(|e| format!("Failed to clean up previous test data: {}", e))?;
 }

And at the end:

 // Clean up
-std::fs::remove_dir_all(storage_dir)?;
+if let Err(e) = std::fs::remove_dir_all(storage_dir) {
+    eprintln!("Warning: Failed to clean up test data: {}", e);
+}

Also applies to: 98-98

dash-spv-ffi/tests/test_mempool_tracking.rs (1)

170-175: Add assertions for mempool balance fields

The test retrieves mempool balance but only asserts on the confirmed field. The comment mentions mempool fields but doesn't verify them.

Add comprehensive assertions:

 if !balance.is_null() {
     let balance_data = (*balance);
     assert_eq!(balance_data.confirmed, 0); // No confirmed balance in mempool
-                                           // mempool and mempool_instant fields contain the actual mempool balance
+    // Verify mempool fields are non-negative
+    assert!(balance_data.mempool >= 0, "Mempool balance should be non-negative");
+    assert!(balance_data.mempool_instant >= 0, "Mempool instant balance should be non-negative");
+    assert_eq!(balance_data.total, balance_data.mempool, "Total should equal mempool for mempool-only query");
     dash_spv_ffi_balance_destroy(balance);
 }

dash-spv-ffi/tests/test_event_callbacks.rs (1)

112-112: Document the test addresses being used

The hardcoded testnet addresses should be documented for clarity.

Add comments explaining these addresses:

-        let test_address = b"yNDp83M8aHDGNkXPFaVoJZa2D9KparfWDc\0".as_ptr() as *const c_char;
+        // Valid testnet P2PKH address for testing
+        let test_address = b"yNDp83M8aHDGNkXPFaVoJZa2D9KparfWDc\0".as_ptr() as *const c_char;

Also applies to: 185-186

dash-spv/src/network/mock.rs (1)

66-95: Optimize header lookup performance

The nested loop for finding locator hashes has O(n*m) complexity. For large header chains, this could be slow.

Consider using a HashMap for O(1) lookups:

pub struct MockNetworkManager {
    // ... existing fields ...
    headers_index: HashMap<BlockHash, usize>, // Add this field
}

fn process_getheaders(&self, msg: &GetHeadersMessage) -> Vec<BlockHeader> {
    let start_idx = if msg.locator_hashes.is_empty() {
        0
    } else {
        msg.locator_hashes
            .iter()
            .find_map(|hash| self.headers_index.get(hash).map(|&idx| idx + 1))
            .unwrap_or(0)
    };
    
    // Rest of the function remains the same
}

Update add_headers_chain to populate the index when adding headers.

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

239-241: Improve error message for address parsing

Add context to the unwrap for better test failure messages.

-        let address = dashcore::Address::from_str("XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4")
-            .unwrap()
+        let address = dashcore::Address::from_str("XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4")
+            .expect("Valid test address")
             .assume_checked();

dash-spv/src/chain/checkpoint_test.rs (1)

216-217: Misleading comment about edge case

The comment states "height before first checkpoint" but the test actually uses height 0, which IS the first checkpoint based on the loop starting at i=0 on line 206.

-        // Test edge case: height before first checkpoint
-        assert!(manager.last_checkpoint_before_height(0).is_some());
+        // Test edge case: height at first checkpoint
+        assert_eq!(manager.last_checkpoint_before_height(0).unwrap().height, 0);

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

397-397: Unnecessary clone of event_tx reference

The event_tx is already a reference to UnboundedSender, and you're passing a reference to the handler. The .clone() call is unnecessary here.

-            &event_tx.clone(),
+            &event_tx,

dash-spv/src/main.rs (1)

176-188: Consider using an enum for start height configuration

While using u32::MAX as a sentinel value for "now" works, consider using an enum for better type safety and clarity:

enum StartHeight {
    Specific(u32),
    Latest,
}

This would make the intent clearer throughout the codebase.

dash-spv/src/chain/chain_tip.rs (1)

99-108: Remove unnecessary clone on BlockHash.

BlockHash implements Copy, so the clone() is unnecessary.

-                    self.tips.insert(tip_hash.clone(), tip);
+                    self.tips.insert(*tip_hash, tip);

dash-spv/src/bloom/builder.rs (1)

49-52: Consider validating false positive rate range.

The false positive rate should be validated to ensure it's within a reasonable range (0 < rate < 1).

 pub fn false_positive_rate(mut self, rate: f64) -> Self {
+    debug_assert!(rate > 0.0 && rate < 1.0, "False positive rate must be between 0 and 1");
     self.false_positive_rate = rate;
     self
 }

dash-spv/src/chain/orphan_pool.rs (2)

122-123: Consider using a more efficient data structure for eviction queue if performance becomes an issue.

The linear search through eviction_queue has O(n) complexity. For the current MAX_ORPHAN_BLOCKS of 100 this is acceptable, but consider using a LinkedHashMap or similar if the limit increases significantly.

---

206-209: Remove unused variable assignment.

The _block_hash variable is computed but never used.

-        for header in &orphans {
-            let _block_hash = header.block_hash();
-            self.remove_orphan(&header.block_hash());
-        }
+        for header in &orphans {
+            self.remove_orphan(&header.block_hash());
+        }

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

73-114: Comprehensive input validation for quorum public key retrieval.

Excellent input validation including buffer size checks. The NotImplemented placeholder is properly documented with a TODO comment.

Would you like me to help implement the actual quorum public key retrieval logic or create an issue to track this TODO?

dash-spv/src/mempool_filter.rs (1)

574-615: Test coverage gap for process_transaction

The test methods test_process_transaction_outgoing and test_process_transaction_incoming are marked as #[ignore] because they require a real Wallet implementation. This leaves a critical part of the code untested.

Would you like me to help implement a more complete MockWallet that can support these tests, or create integration tests that use a real Wallet instance?

dash-spv/src/network/handshake.rs (3)

24-27: State machine documentation needed

The handshake state enum has been expanded with new states, but the transitions between states are not clearly documented. This could lead to confusion about valid state transitions.

Add documentation about the state machine:

/// Handshake state machine:
/// Init -> VersionSent -> VersionReceivedVerackSent -> Complete
///                     \-> VerackReceived -> Complete

---

266-269: Relay flag logic tied to mempool strategy

The relay flag is set based on mempool_strategy, with FetchAll enabling relay and others disabling it. This coupling makes sense but should be documented more clearly.

Add a comment explaining the relay flag logic:

             relay: match self.mempool_strategy {
+                // FetchAll strategy wants to receive all mempool transactions
                 MempoolStrategy::FetchAll => true,
+                // Other strategies are more selective about mempool transactions
                 _ => false,
             },

---

291-297: Headers2 feature disabled but negotiation code present

The negotiate_headers2 method always sends SendHeaders regardless of peer support, with a comment that headers2 is disabled due to protocol compatibility issues. However, the method name and structure suggest it should negotiate headers2 support.

Consider renaming the method or adding a TODO for when headers2 is re-enabled:

-    /// Negotiate headers2 support with the peer after handshake completion.
+    /// Configure header announcement preference after handshake completion.
+    /// TODO: Implement actual headers2 negotiation when protocol issues are resolved
     async fn negotiate_headers2(&self, connection: &mut TcpConnection) -> NetworkResult<()> {

dash-spv/src/error.rs (1)

151-153: Deprecated variant still in use

The SyncFailed variant is marked as deprecated but is still part of the public API. This could confuse users who might still use it.

Consider removing the deprecated variant in a future version or providing migration guidance:

/// Deprecated: Use specific error variants instead
/// Migration guide:
/// - For network errors, use SyncError::Network
/// - For validation errors, use SyncError::Validation
/// - For storage errors, use SyncError::Storage
/// - For timeout errors, use SyncError::Timeout
#[deprecated(since = "0.2.0", note = "Use specific error variants instead")]

dash-spv-ffi/tests/unit/test_async_operations.rs (1)

133-135: Network-dependent tests should use integration test directory

Tests that require network connections are marked with #[ignore] but remain in the unit test directory. These should be moved to integration tests with proper categorization.

Consider moving network-dependent tests to a separate integration test file:

tests/integration/test_network_operations.rs

dash-spv/src/chain/fork_detector.rs (1)

83-106: Consider atomic fork updates to prevent potential data loss.

The current implementation removes the fork from the HashMap before updating it (line 93), then re-inserts it (line 103). While unlikely in this case, if any operation fails between removal and re-insertion, the fork data could be lost.

Consider updating the fork in-place or using a more atomic update pattern:

-        let matching_fork = self
-            .forks
-            .iter()
-            .find(|(_, fork)| fork.tip_hash == prev_hash)
-            .map(|(_, fork)| fork.clone());
-
-        if let Some(mut fork) = matching_fork {
-            // Remove the old entry (indexed by old tip)
-            self.forks.remove(&fork.tip_hash);
-
-            // Update the fork
-            fork.headers.push(*header);
-            fork.tip_hash = header_hash;
-            fork.tip_height += 1;
-            fork.chain_work = fork.chain_work.add_header(header);
-
-            // Re-insert with new tip hash
-            let result_fork = fork.clone();
-            self.forks.insert(header_hash, fork);
-
-            return ForkDetectionResult::ExtendsFork(result_fork);
+        // Check if this extends a known fork
+        let extends_fork = self.forks.values().any(|fork| fork.tip_hash == prev_hash);
+        
+        if extends_fork {
+            // Remove the old entry and get the fork
+            let mut fork = self.forks.values()
+                .find(|fork| fork.tip_hash == prev_hash)
+                .cloned()
+                .unwrap();
+            
+            self.forks.remove(&fork.tip_hash);
+            
+            // Update the fork
+            fork.headers.push(*header);
+            let old_tip = fork.tip_hash;
+            fork.tip_hash = header_hash;
+            fork.tip_height += 1;
+            fork.chain_work = fork.chain_work.add_header(header);
+            
+            // Re-insert with new tip hash
+            let result_fork = fork.clone();
+            self.forks.insert(header_hash, fork);
+            
+            return ForkDetectionResult::ExtendsFork(result_fork);
         }

dash-spv/src/network/mod.rs (1)

314-318: Track the disabled Headers2 feature.

The has_headers2_peer method is hardcoded to return false due to protocol compatibility issues, overriding the trait's default implementation. Consider tracking this with a proper issue reference.

Would you like me to create an issue to track the Headers2 decompression fix?

dash-spv/src/bloom/manager.rs (1)

137-165: Unnecessary explicit lock scope management.

The comment "Explicitly drop the lock here" at line 143 (and similar at lines 173, 199) suggests concern about lock duration, but the scope block already ensures the lock is dropped. The pattern is correct but the comment is misleading.

Either remove the comments or make the drop truly explicit:

-        {
-            let mut addresses = self.addresses.write().await;
-            addresses.push(address.clone());
-        } // Explicitly drop the lock here
+        {
+            let mut addresses = self.addresses.write().await;
+            addresses.push(address.clone());
+        } // Lock dropped at end of scope

Or if you want to be explicit:

-        {
-            let mut addresses = self.addresses.write().await;
-            addresses.push(address.clone());
-        } // Explicitly drop the lock here
+        let mut addresses = self.addresses.write().await;
+        addresses.push(address.clone());
+        drop(addresses); // Explicitly drop the lock here

dash-spv/src/chain/checkpoints.rs (1)

403-414: Consider using TryFrom trait for better error handling

The parse_block_hash function could be more idiomatic by implementing TryFrom<&str> for BlockHash. Additionally, the error messages could be more specific about what went wrong.

-fn parse_block_hash(s: &str) -> Result<BlockHash, String> {
-    use hex::FromHex;
-    let bytes = Vec::<u8>::from_hex(s).map_err(|e| format!("Invalid hex: {}", e))?;
-    if bytes.len() != 32 {
-        return Err("Invalid hash length: expected 32 bytes".to_string());
-    }
-    let mut hash_bytes = [0u8; 32];
-    hash_bytes.copy_from_slice(&bytes);
-    // Reverse for little-endian
-    hash_bytes.reverse();
-    Ok(BlockHash::from_byte_array(hash_bytes))
-}
+impl TryFrom<&str> for BlockHashWrapper {
+    type Error = String;
+    
+    fn try_from(s: &str) -> Result<Self, Self::Error> {
+        use hex::FromHex;
+        let bytes = Vec::<u8>::from_hex(s)
+            .map_err(|e| format!("Invalid hex string '{}': {}", s, e))?;
+        if bytes.len() != 32 {
+            return Err(format!("Invalid hash length for '{}': expected 32 bytes, got {}", s, bytes.len()));
+        }
+        let mut hash_bytes = [0u8; 32];
+        hash_bytes.copy_from_slice(&bytes);
+        // Reverse for little-endian
+        hash_bytes.reverse();
+        Ok(BlockHashWrapper(BlockHash::from_byte_array(hash_bytes)))
+    }
+}

dash-spv/src/chain/chainlock_manager.rs (1)

306-324: Cache eviction logic could be more efficient

The current implementation collects keys before removing them, which involves extra allocations. Consider using drain for better performance.

-// Enforce cache size limit
-if by_height.len() > self.max_cache_size {
-    // Calculate how many entries to remove
-    let entries_to_remove = by_height.len() - self.max_cache_size;
-
-    // Collect keys to remove (oldest entries are at the beginning)
-    let keys_to_remove: Vec<(u32, BlockHash)> = by_height
-        .iter()
-        .take(entries_to_remove)
-        .map(|(height, entry)| (*height, entry.chain_lock.block_hash))
-        .collect();
-
-    // Batch remove from both maps
-    for (height, block_hash) in keys_to_remove {
-        by_height.shift_remove(&height);
-        by_hash.shift_remove(&block_hash);
-    }
-}
+// Enforce cache size limit
+while by_height.len() > self.max_cache_size {
+    // Remove oldest entry (IndexMap maintains insertion order)
+    if let Some((height, entry)) = by_height.shift_remove_index(0) {
+        by_hash.shift_remove(&entry.chain_lock.block_hash);
+    } else {
+        break;
+    }
+}

dash-spv/src/chain/reorg.rs (1)

116-133: Complex conditional logic for checkpoint-based sync

The logic for handling forks during checkpoint sync is complex and could benefit from clearer documentation or refactoring.

Consider extracting this logic into a separate method for better readability:

+/// Calculate reorg depth accounting for checkpoint sync mode
+fn calculate_reorg_depth(
+    current_tip: &ChainTip,
+    fork: &Fork,
+    chain_state: Option<&ChainState>,
+) -> Result<u32, String> {
+    if let Some(state) = chain_state {
+        if state.synced_from_checkpoint && state.sync_base_height > 0 {
+            // During checkpoint sync, validate fork compatibility
+            if fork.fork_height < state.sync_base_height {
+                return Err(format!(
+                    "Cannot reorg past checkpoint: fork height {} < checkpoint base {}",
+                    fork.fork_height, state.sync_base_height
+                ));
+            }
+        }
+    }
+    Ok(current_tip.height.saturating_sub(fork.fork_height))
+}

 // Check reorg depth - account for checkpoint sync
-let reorg_depth = if let Some(state) = chain_state {
-    // ... existing complex logic ...
-} else {
-    current_tip.height.saturating_sub(fork.fork_height)
-};
+let reorg_depth = calculate_reorg_depth(current_tip, fork, chain_state)?;

dash-network-ffi/src/dash_network_ffi.swift (1)

563-590: Consider error handling and performance optimization

Each method calls uniffiClonePointer() which may have performance implications for frequently called methods. Additionally, the use of try! suppresses error information that could be useful for debugging.

Consider:

1. Caching the cloned pointer if these methods are called frequently
2. Using proper error handling instead of try! to provide better error context
3. Documenting which methods might fail and under what conditions

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify test script existence.

The documentation references ./contrib/test.sh script. Confirm this script exists and the mentioned environment variables are supported.

---

🏁 Script executed:

#!/bin/bash
# Check if contrib/test.sh exists and what it supports
if [ -f "./contrib/test.sh" ]; then
    echo "Script found, checking for environment variable support:"
    grep -n "DO_COV\|DO_LINT\|DO_FMT" ./contrib/test.sh || echo "Environment variables not found in script"
else
    echo "contrib/test.sh not found"
fi

Length of output: 300

---

Ensure contrib/test.sh Supports or Remove Unused Env Vars

The documentation (CLAUDE.md, lines 95–96) calls out DO_COV, DO_LINT, and DO_FMT for ./contrib/test.sh, but the script itself doesn’t reference them. Please either:

• Add handling for these variables in contrib/test.sh, for example:

  if [[ "${DO_LINT:-false}" == "true" ]]; then
    # run linter
  fi
  # …and similarly for DO_FMT and DO_COV

• Or update CLAUDE.md to remove or revise the env-var references to match the script’s actual behavior.

Files to update:

• CLAUDE.md (lines 95–96)
• contrib/test.sh (add env-var checks)

🤖 Prompt for AI Agents

In CLAUDE.md around lines 95–96 and contrib/test.sh, the docs mention DO_COV,
DO_LINT, and DO_FMT but contrib/test.sh does not read them; either update the
docs to remove or revise those env-var references or add handling in
contrib/test.sh. Fix by adding conditional checks in contrib/test.sh that
default to false and run the corresponding steps when true (e.g., run linter
when DO_LINT=="true", formatter when DO_FMT=="true", coverage when
DO_COV=="true"), or alternatively edit CLAUDE.md to remove the env-var mention
or note that the script ignores them; ensure both files stay consistent after
the change.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify integration test environment variable.

The documentation mentions DASH_SPV_IP environment variable for real node tests, but the test command shown connects to 127.0.0.1:9999. Clarify if the environment variable is actually used or if this is outdated information.

---

🏁 Script executed:

#!/bin/bash
# Check if DASH_SPV_IP is actually used in integration tests
rg -n "DASH_SPV_IP" dash-spv/tests/ || echo "DASH_SPV_IP not found in tests"
rg -n "127\.0\.0\.1:9999" dash-spv/tests/ | head -5

Length of output: 713

---

🏁 Script executed:

#!/bin/bash
# Display lines 100–130 of CLAUDE.md to verify the context around lines 113–115
sed -n '100,130p' CLAUDE.md

Length of output: 561

---

🏁 Script executed:

#!/bin/bash
# Show the beginning of integration_real_node_test.rs to see how DASH_SPV_IP is handled
sed -n '1,200p' dash-spv/tests/integration_real_node_test.rs

Length of output: 6767

---

Update CLAUDE.md to remove outdated DASH_SPV_IP reference

The integration tests don’t actually read the DASH_SPV_IP env var—they connect to the hard-coded 127.0.0.1:9999. Please update the docs accordingly:

• Location: CLAUDE.md (around lines 113–115)
• Current:

# Run with real Dash node (requires DASH_SPV_IP environment variable)
cd dash-spv
cargo test --test integration_real_node_test -- --nocapture

• Proposed:

# Run with real Dash node (connects to 127.0.0.1:9999 by default)
cd dash-spv
cargo test --test integration_real_node_test -- --nocapture

Optional: if you’d like to support a custom address, update integration_real_node_test.rs to read from DASH_SPV_IP (e.g. std::env::var("DASH_SPV_IP").unwrap_or(DASH_NODE_ADDR.to_string())).

🤖 Prompt for AI Agents

In CLAUDE.md around lines 113–115, the doc incorrectly claims the integration
test requires a DASH_SPV_IP env var; update the comment to state the test
connects to 127.0.0.1:9999 by default (e.g. change the heading line to “Run with
real Dash node (connects to 127.0.0.1:9999 by default)”) and leave the test
invocation as-is. Optionally, if you want to support a custom address, modify
integration_real_node_test.rs to read DASH_SPV_IP with a fallback to the default
address.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify current branch information.

The documentation states the current development branch is v0.40-dev and mentions recent work removing interleaved sync logic. Verify this branch information is current and accurate.

---

🏁 Script executed:

#!/bin/bash
# Check current branch and recent branch activity
git branch -a | grep -E "(v0\.40|dev|master)" || echo "Mentioned branches not found"
git log --oneline --grep="interleaved" --grep="sequential" -n 5 || echo "No recent sync-related commits found"

Length of output: 119

---

Update CLAUDE.md with accurate branch and sync history

The documentation currently lists:

- Current development branch: `v0.40-dev`
- Main branch: `master`
- Recent work:
  - Removed interleaved sync logic from dash-spv (now uses sequential sync only)
  - Swift SDK and FFI improvements

I ran:

git branch -a | grep -E "(v0\.40|dev|master)"
# only `master` is present

git log --oneline --grep="interleaved" --grep="sequential" -n 5
# No recent sync-related commits found

Please address the following:

• Replace v0.40-dev with the actual development branch name (no v0.40-dev exists in the repo).
• Either link or reference the commit(s) that removed interleaved sync logic from dash-spv, or remove that bullet if it hasn’t been merged.

🤖 Prompt for AI Agents

In CLAUDE.md around lines 174 to 178, the doc incorrectly lists a non-existent
development branch `v0.40-dev` and an unverified claim about removing
interleaved sync logic; run `git branch -a` (or `git for-each-ref
--format="%(refname:short)" refs/heads refs/remotes`) to determine the actual
development branch name and replace `v0.40-dev` with that branch (or `master` if
no separate dev branch exists), then verify the dash-spv sync change with `git
log --grep="interleaved" --grep="sequential" --all --oneline` and either replace
the bullet with a link/reference to the commit(s) (commit hash and short
message) if found or remove the bullet entirely if there is no merged change to
cite.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify Dash Core version support claim.

The documentation claims support for Dash Core versions 0.18.0 - 0.21.0. This should be verified against the actual implementation and any version compatibility testing.

---

🏁 Script executed:

#!/bin/bash
# Look for version compatibility information in the codebase
rg -n "0\.(18|19|20|21)\." --type rust | head -10
rg -n "version.*compatibility" --type rust | head -5

Length of output: 619

---

🏁 Script executed:

#!/bin/bash
# Inspect version compatibility logic in SPV sync state
sed -n '240,300p' dash-spv/src/storage/sync_state.rs

Length of output: 2347

---

Update supported Dash Core version range

We’ve confirmed in the codebase that:

• rpc-json/src/lib.rs references features introduced or deprecated in Core v0.18.x, v0.19.0 and v0.20.0.
• dash/src/policy.rs pulls values from bitcoind v0.21.1.

The docs currently state support for 0.18.0–0.21.0, but your code is already aligning with v0.21.1. Please either:

• Confirm that RPC/consensus changes between Core 0.21.0 and 0.21.1 have no impact, or
• Update the documentation to reflect support through v0.21.1.

Suggested diff in CLAUDE.md around line 185:

- Support for Dash Core versions 0.18.0 - 0.21.0
+ Support for Dash Core versions 0.18.0 - 0.21.1

📝 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

	- Support for Dash Core versions 0.18.0 - 0.21.0
	- Support for Dash Core versions 0.18.0 - 0.21.1

🤖 Prompt for AI Agents

In CLAUDE.md around line 185, the supported Dash Core version range is outdated
(currently listed as 0.18.0 - 0.21.0) but the code references v0.21.1; update
the documentation to reflect support through v0.21.1 by changing the version
range to 0.18.0 - 0.21.1 (or 0.18.0–0.21.1) or, if you prefer to keep 0.21.0,
add a brief note confirming that RPC/consensus differences between 0.21.0 and
0.21.1 were audited and do not affect this codebase; make the corresponding
single-line edit at line 185.

[coderabbitai]

💡 Verification agent

❓ Verification inconclusive

Verify Unified SDK build path and add brief prerequisites.

Check the build script location and consider listing prerequisites (Rust, Xcode, cbindgen, uniffi-bindgen if needed).

Add a short “Requirements” subsection before the build commands.

---

🏁 Script executed:

#!/bin/bash
fd -a 'build_ios.sh' ../../platform-ios/packages/rs-sdk-ffi || true
# Optional: show script header to confirm usage
if fd -a 'build_ios.sh' ../../platform-ios/packages/rs-sdk-ffi >/dev/null; then
  head -n 20 ../../platform-ios/packages/rs-sdk-ffi/build_ios.sh || true
fi

Length of output: 378

---

🏁 Script executed:

#!/bin/bash
# Search for build_ios.sh anywhere in the repository to confirm its location
fd -a 'build_ios.sh' .

Length of output: 22

---

Update iOS Unified SDK instructions and add prerequisites

The README currently references a build_ios.sh script under platform-ios/packages/rs-sdk-ffi that doesn’t exist. Let’s:

• Add a Requirements section before the build steps, e.g.:

## Requirements
- Rust ≥ 1.60  
- Xcode command-line tools  
- cbindgen (`cargo install cbindgen`)  
- uniffi-bindgen (`cargo install uniffi-bindgen`)

• Verify and correct the build-script path/filename in dash-spv-ffi/README.md (lines 29–39). For example, if the script is named build.sh, update:

- cd ../../platform-ios/packages/rs-sdk-ffi
- ./build_ios.sh
+ cd ../../platform-ios/packages/rs-sdk-ffi
+ ./build.sh

• Ensure the instructions reference paths relative to the repo root:
platform-ios/packages/rs-sdk-ffi → [correct script name]

🤖 Prompt for AI Agents

In dash-spv-ffi/README.md around lines 29 to 39, the iOS Unified SDK section
references a non-existent script path
(`platform-ios/packages/rs-sdk-ffi/build_ios.sh`) and lacks prerequisites; add a
"Requirements" section before the build steps listing Rust ≥ 1.60, Xcode
command-line tools, cbindgen (`cargo install cbindgen`), and uniffi-bindgen
(`cargo install uniffi-bindgen`), and update the build commands to point to the
correct script name and path relative to the repo root (verify the actual script
filename in platform-ios/packages/rs-sdk-ffi and replace `build_ios.sh` with the
real filename, e.g., `build.sh`, ensuring the README commands use that verified
path).

[coderabbitai]

⚠️ Potential issue

Don't silently ignore address parsing failures in tests

The test continues silently if address parsing fails, which could mask issues.

Use explicit assertion:

-        let addr_str = "XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4"; // Example Dash mainnet address
-        if let Ok(address) = Address::from_str(addr_str) {
-            config = config.watch_address(address.assume_checked());
-            assert_eq!(config.watch_items.len(), 1);
-        }
+        let addr_str = "XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4"; // Example Dash mainnet address
+        let address = Address::from_str(addr_str)
+            .expect("Test address should be valid")
+            .assume_checked();
+        config = config.watch_address(address);
+        assert_eq!(config.watch_items.len(), 1);

📝 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

	let addr_str = "XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4"; // Example Dash mainnet address
	if let Ok(address) = Address::from_str(addr_str) {
	config = config.watch_address(address.assume_checked());
	assert_eq!(config.watch_items.len(), 1);
	}
	let addr_str = "XeNTGz5bVjPNZVPpwTRz6SnLbZGxLqJUg4"; // Example Dash mainnet address
	let address = Address::from_str(addr_str)
	.expect("Test address should be valid")
	.assume_checked();
	config = config.watch_address(address);
	assert_eq!(config.watch_items.len(), 1);

🤖 Prompt for AI Agents

In dash-spv/src/client/config_test.rs around lines 123 to 127, the test
currently ignores Address::from_str failures using if let Ok(...), which can
hide parsing errors; replace the conditional with an explicit assertion that
parsing succeeds (e.g., call Address::from_str(addr_str).expect("failed to parse
test address") or assert!(Address::from_str(addr_str).is_ok(), "...")), bind the
parsed address, then call assume_checked() and proceed with config.watch_address
and the assert_eq on watch_items.len().

[coderabbitai]

🛠️ Refactor suggestion

Use proper ServiceFlags constant instead of magic number

The hardcoded value 1 should use the proper constant for clarity.

-                services: Some(1),
+                services: Some(ServiceFlags::NETWORK.bits()),

📝 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

	services: Some(1),
	- services: Some(1),
	+ services: Some(ServiceFlags::NETWORK.bits()),

🤖 Prompt for AI Agents

In dash-spv/src/network/mock.rs around line 164, the code uses the magic number
`1` for service flags; replace this literal with the appropriate ServiceFlags
constant (for example ServiceFlags::NODE_NETWORK) and add the necessary
use/import for ServiceFlags so the intent is clear and type-safe.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify referenced artifacts exist (prevent broken links).

Docs now reference:

• dash-spv-ffi, key-wallet-ffi crates
• UNIFIED_SDK.md at repo root

Please verify these exist to avoid broken links.

---

🏁 Script executed:

#!/bin/bash
# Check for referenced crates and doc
fd -t d -E target -E .git -a 'dash-spv-ffi' || true
fd -t d -E target -E .git -a 'key-wallet-ffi' || true
fd -a 'UNIFIED_SDK.md' || true
# Additionally validate cbindgen config presence for FFI crate(s)
fd -a 'cbindgen.toml' | rg 'dash-spv-ffi' -n || true

Length of output: 305

---

FFI artifacts and doc links verified; add missing cbindgen.toml for key-wallet-ffi

• dash-spv-ffi/ directory exists and includes cbindgen.toml.
• key-wallet-ffi/ directory exists but no cbindgen.toml was found.
• UNIFIED_SDK.md exists at the repo root.

Please add a cbindgen.toml in key-wallet-ffi/ if it should expose C/Swift bindings.

🤖 Prompt for AI Agents

In README.md around lines 33 to 34, the repo notes FFI bindings but
key-wallet-ffi is missing a cbindgen.toml; add a cbindgen.toml file inside the
key-wallet-ffi/ directory that enables generation of C/Swift headers for that
crate (set the crate name to the key-wallet crate, specify output header file
path, choose language = "C" or "C++" as appropriate, configure include guard and
any export/allowlist rules needed for public types/functions), then commit the
file so the repository can produce C/Swift bindings for key-wallet-ffi.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Use neutral tone and auto-derived metrics to avoid drift.

• Replace first-person phrasing with project-neutral language.
• Derive test counts in CI to keep this document accurate over time.

I can turn this into a CI job that updates the summary automatically or fails if docs drift beyond a tolerance.

---

🏁 Script executed:

#!/bin/bash
# Rough test count by crate
echo "dash-spv test functions:"
rg -n "^\s*#\[test\]" dash-spv | wc -l
echo "dash-spv-ffi test functions:"
rg -n "^\s*#\[test\]" dash-spv-ffi | wc -l

# Module-specific sampling (adjust patterns as needed)
echo "Bloom tests:"
rg -n "mod tests|_test\.rs$" dash-spv/src/bloom | wc -l
echo "Validation tests:"
rg -n "_test\.rs$" dash-spv/src/validation | wc -l
echo "Chain tests:"
rg -n "_test\.rs$" dash-spv/src/chain | wc -l

Length of output: 499

---

🏁 Script executed:

#!/bin/bash
set -e

echo "=== Module-level test file and function counts ==="

# Bloom module
echo "Bloom module:"
echo -n "  Test files (tests.rs or *_test.rs): "
find dash-spv/src/bloom -type f \( -name "tests.rs" -o -name "*_test.rs" \) | wc -l
echo -n "  #[test] functions: "
rg -c "^\s*#\[test\]" dash-spv/src/bloom | awk -F: '{sum+=$2} END{print sum}'

# Validation module
echo "Validation module:"
echo -n "  Test files (*_test.rs): "
find dash-spv/src/validation -type f -name "*_test.rs" | wc -l
echo -n "  #[test] functions: "
rg -c "^\s*#\[test\]" dash-spv/src/validation | awk -F: '{sum+=$2} END{print sum}'

# Chain module
echo "Chain module:"
echo -n "  Test files (*_test.rs): "
find dash-spv/src/chain -type f -name "*_test.rs" | wc -l
echo -n "  #[test] functions: "
rg -c "^\s*#\[test\]" dash-spv/src/chain | awk -F: '{sum+=$2} END{print sum}'

# Crate-wide counts
echo "Overall dash-spv crate:"
echo -n "  Total #[test] functions: "
rg -c "^\s*#\[test\]" dash-spv | awk -F: '{sum+=$2} END{print sum}'
echo "dash-spv-ffi crate:"
echo -n "  Total #[test] functions: "
rg -c "^\s*#\[test\]" dash-spv-ffi | awk -F: '{sum+=$2} END{print sum}'

Length of output: 1487

---

Replace Static Test Counts with CI-Driven Metrics

The TEST_SUMMARY.md should use neutral, project-centric language and pull test counts from CI to prevent documentation drift.

– Remove all first-person phrasing (e.g. “I have…”) in favor of statements like “Tests cover…” or “Coverage by module:”.
– Swap hard-coded numbers for placeholders that your CI job will populate (for example: {{bloom_tests}}, {{validation_tests}}, {{chain_tests}}, {{total_tests}}).
– Add or update a CI step that runs a script (such as the one below) to count #[test] functions and inject those values into TEST_SUMMARY.md or fail the build if counts diverge by more than an acceptable threshold.

Current counts (for reference):
• Bloom Filter module: 26 tests
• Validation module: 54 tests
• Chain module: 61 tests
• Overall dash-spv crate: 273 tests
• dash-spv-ffi crate: 159 tests

File to update:

• TEST_SUMMARY.md (lines 1–112)

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify build script path and existence.

The documentation references ./build_ios.sh script in the platform-ios repository. Confirm this script exists and the build process works as documented.

---

🏁 Script executed:

#!/bin/bash
# Check if the build script exists
if [ -d "../platform-ios/packages/rs-sdk-ffi" ]; then
    ls -la ../platform-ios/packages/rs-sdk-ffi/build_ios.sh 2>/dev/null || echo "build_ios.sh not found"
    if [ -f "../platform-ios/packages/rs-sdk-ffi/build_ios.sh" ]; then
        echo "Script found, checking if it's executable:"
        ls -la ../platform-ios/packages/rs-sdk-ffi/build_ios.sh | cut -d' ' -f1
    fi
else
    echo "platform-ios/packages/rs-sdk-ffi directory not found"
fi

Length of output: 163

---

🏁 Script executed:

#!/bin/bash
# Search for the build_ios.sh script anywhere in the repository
fd build_ios.sh

Length of output: 17

---

Missing build_ios.sh script in platform-ios/packages/rs-sdk-ffi

I was unable to locate the referenced build_ios.sh anywhere in the repo. Please verify and either add the script or update the documentation to point at the correct build script/location.

• Location: UNIFIED_SDK.md (lines 27–30)
• Action:

• Confirm whether build_ios.sh should exist under platform-ios/packages/rs-sdk-ffi.
• If it’s been renamed or moved, update the path in UNIFIED_SDK.md.
• If it’s missing, add the script to that package with executable permissions.

🤖 Prompt for AI Agents

In UNIFIED_SDK.md around lines 27 to 30, the docs reference a nonexistent script
at platform-ios/packages/rs-sdk-ffi/build_ios.sh; verify whether that script
should live in that package or was renamed/moved, then update the documentation
or repo accordingly: if the script exists elsewhere, change the path in
UNIFIED_SDK.md to the correct location; if it was renamed, update the filename
in the doc; if it is missing, add a new build_ios.sh into
platform-ios/packages/rs-sdk-ffi with the intended build steps and make it
executable (chmod +x), and ensure the README or package manifest points to it.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue

Validate rustBuffer.data before force unwrapping

The force unwrap of rustBuffer.data! at line 54 could cause a crash if data is nil. Consider adding a guard or using nil-coalescing to handle this case safely.

 fileprivate extension Data {
     init(rustBuffer: RustBuffer) {
+        guard let data = rustBuffer.data else {
+            self.init()
+            return
+        }
         self.init(
-            bytesNoCopy: rustBuffer.data!,
+            bytesNoCopy: data,
             count: Int(rustBuffer.len),
             deallocator: .none
         )
     }
 }

📝 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

	init(rustBuffer: RustBuffer) {
	self.init(
	bytesNoCopy: rustBuffer.data!,
	count: Int(rustBuffer.len),
	deallocator: .none
	)
	}
	fileprivate extension Data {
	init(rustBuffer: RustBuffer) {
	guard let data = rustBuffer.data else {
	// Fallback to empty Data if the Rust buffer has no backing pointer
	self.init()
	return
	}
	self.init(
	bytesNoCopy: data,
	count: Int(rustBuffer.len),
	deallocator: .none
	)
	}
	}

🤖 Prompt for AI Agents

In dash-network-ffi/src/dash_network_ffi.swift around lines 52 to 58, replace
the force unwrap of rustBuffer.data with a nil check: use guard let data =
rustBuffer.data else { self.init(bytes: [], count: 0, deallocator: .none);
return } (or make the initializer failable and return nil) and then call
self.init(bytesNoCopy: data, count: Int(rustBuffer.len), deallocator: .none);
this ensures you don’t crash when data is nil and preserves safe initialization.

[coderabbitai]

⚠️ Potential issue

Potential memory leak if capacity differs from length

The dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx function reconstructs a Vec with raw_tx_len for both length and capacity, which might not match the original allocation.

 pub unsafe extern "C" fn dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx(
     raw_tx: *mut u8,
     raw_tx_len: usize,
+    raw_tx_capacity: usize,
 ) {
-    if !raw_tx.is_null() && raw_tx_len > 0 {
+    if !raw_tx.is_null() && raw_tx_capacity > 0 {
         // Reconstruct the Vec to properly deallocate the memory
-        let _ = Vec::from_raw_parts(raw_tx, raw_tx_len, raw_tx_len);
+        let _ = Vec::from_raw_parts(raw_tx, raw_tx_len, raw_tx_capacity);
     }
 }

The function needs to know the original capacity to properly deallocate the memory. Consider adding a capacity field to FFIUnconfirmedTransaction.

📝 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

	pub unsafe extern "C" fn dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx(
	raw_tx: *mut u8,
	raw_tx_len: usize,
	) {
	if !raw_tx.is_null() && raw_tx_len > 0 {
	// Reconstruct the Vec to properly deallocate the memory
	let _ = Vec::from_raw_parts(raw_tx, raw_tx_len, raw_tx_len);
	}
	}
	pub unsafe extern "C" fn dash_spv_ffi_unconfirmed_transaction_destroy_raw_tx(
	raw_tx: *mut u8,
	raw_tx_len: usize,
	raw_tx_capacity: usize,
	) {
	if !raw_tx.is_null() && raw_tx_capacity > 0 {
	// Reconstruct the Vec to properly deallocate the memory
	let _ = Vec::from_raw_parts(raw_tx, raw_tx_len, raw_tx_capacity);
	}
	}

🤖 Prompt for AI Agents

In dash-spv-ffi/src/types.rs around lines 445 to 453, the destructor
reconstructs a Vec using raw_tx_len for both length and capacity which can leak
if the original allocation capacity differed; add a capacity field (e.g.
raw_tx_cap) to FFIUnconfirmedTransaction and set it when allocating/serializing
the raw tx, then change the destructor to use Vec::from_raw_parts(raw_tx,
raw_tx_len, raw_tx_cap) (with checks for non-null and cap>0) so the original
allocation is deallocated correctly; also update all creation and drop paths to
populate and consume the new capacity field.

[coderabbitai]

🛠️ Refactor suggestion

Complex cleanup logic could lead to use-after-free

The cleanup of addresses involves reading each FFIString before destroying the array, which could lead to double-free if not handled carefully.

 // Destroy each FFIString in the addresses array
 if !tx.addresses.is_null() && tx.addresses_len > 0 {
-    // We need to read the addresses and destroy them one by one
-    for i in 0..tx.addresses_len {
-        let address_ptr = tx.addresses.add(i);
-        let address = std::ptr::read(address_ptr);
-        dash_spv_ffi_string_destroy(address);
-    }
-    // Destroy the addresses array itself
-    dash_spv_ffi_unconfirmed_transaction_destroy_addresses(tx.addresses, tx.addresses_len);
+    // Create a slice to safely iterate
+    let addresses = std::slice::from_raw_parts_mut(tx.addresses, tx.addresses_len);
+    for address in addresses.iter_mut() {
+        // Take ownership of the FFIString without reading the memory twice
+        let owned_address = std::mem::replace(address, FFIString { ptr: std::ptr::null_mut(), length: 0 });
+        dash_spv_ffi_string_destroy(owned_address);
+    }
+    // Now destroy the array itself
+    dash_spv_ffi_unconfirmed_transaction_destroy_addresses(tx.addresses, tx.addresses_len);
 }

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

🤖 Prompt for AI Agents

In dash-spv-ffi/src/types.rs around lines 500 to 508, the loop reads each
FFIString from the addresses array then calls dash_spv_ffi_string_destroy and
afterwards calls dash_spv_ffi_unconfirmed_transaction_destroy_addresses, which
can double-free if destroy_addresses also frees individual strings; fix by
clarifying ownership and applying one of two safe options: (1) if
destroy_addresses is responsible for destroying individual FFIString elements,
remove the per-item destruction loop and only call destroy_addresses, or (2) if
the per-item destroy is required, do not let destroy_addresses also free
elements — change destroy_addresses to only free the array buffer (or rename it)
so it does not free elements, and keep a safe per-item destroy that takes
ownership without reading/moving invalid memory; ensure the chosen approach is
applied consistently and add a comment documenting ownership semantics.

[coderabbitai]

⚠️ Potential issue

Improve error handling for lock acquisition

The set_masternode_engine method logs errors but doesn't propagate them, which could hide critical failures.

-pub fn set_masternode_engine(&self, engine: Arc<MasternodeListEngine>) {
-    match self.masternode_engine.write() {
-        Ok(mut guard) => {
-            *guard = Some(engine);
-            info!("Masternode engine set for ChainLock validation");
-        }
-        Err(e) => {
-            error!("Failed to set masternode engine: {}", e);
-        }
-    }
-}
+pub fn set_masternode_engine(&self, engine: Arc<MasternodeListEngine>) -> Result<(), String> {
+    match self.masternode_engine.write() {
+        Ok(mut guard) => {
+            *guard = Some(engine);
+            info!("Masternode engine set for ChainLock validation");
+            Ok(())
+        }
+        Err(e) => {
+            error!("Failed to set masternode engine: {}", e);
+            Err(format!("Lock poisoned: {}", e))
+        }
+    }
+}

📝 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

	match self.masternode_engine.write() {
	Ok(mut guard) => {
	*guard = Some(engine);
	info!("Masternode engine set for ChainLock validation");
	}
	Err(e) => {
	error!("Failed to set masternode engine: {}", e);
	}
	}
	}
	-pub fn set_masternode_engine(&self, engine: Arc<MasternodeListEngine>) {
	- match self.masternode_engine.write() {
	- Ok(mut guard) => {
	- *guard = Some(engine);
	- info!("Masternode engine set for ChainLock validation");
	- }
	- Err(e) => {
	- error!("Failed to set masternode engine: {}", e);
	- }
	- }
	-}
	+pub fn set_masternode_engine(&self, engine: Arc<MasternodeListEngine>) -> Result<(), String> {
	+ match self.masternode_engine.write() {
	+ Ok(mut guard) => {
	+ *guard = Some(engine);
	+ info!("Masternode engine set for ChainLock validation");
	+ Ok(())
	+ }
	+ Err(e) => {
	+ error!("Failed to set masternode engine: {}", e);
	+ Err(format!("Lock poisoned: {}", e))
	+ }
	+ }
	+}

🤖 Prompt for AI Agents

In dash-spv/src/chain/chainlock_manager.rs around lines 61 to 70, the
set_masternode_engine method currently swallows RwLock write errors by logging
only; change the method to return a Result<(), E> (use the crate's standard
error type such as anyhow::Error or your module's Error) and propagate the lock
acquisition error instead of just logging it: on Ok, set the guard as before and
return Ok(()); on Err(e) convert or wrap the PoisonError into the chosen error
type and return Err(e) (you may still log additional context before returning).
Update the function signature and all call sites accordingly to handle the
Result.

[coderabbitai]

🛠️ Refactor suggestion

Magic number should be a named constant

The ChainLock validation requires masternode list at (block_height - 8), but this magic number isn't documented or named.

+/// Number of blocks to look back for masternode list when validating ChainLocks (DIP8 requirement)
+const CHAINLOCK_MASTERNODE_LIST_OFFSET: u32 = 8;
+
 // Check if the error is due to missing masternode lists
 let error_string = e.to_string();
 if error_string.contains("No masternode lists in engine") {
     // ChainLock validation requires masternode list at (block_height - 8)
-    let required_height = chain_lock.block_height.saturating_sub(8);
+    let required_height = chain_lock.block_height.saturating_sub(CHAINLOCK_MASTERNODE_LIST_OFFSET);
     warn!("⚠️ Masternode engine exists but lacks required masternode lists for height {} (needs list at height {} for ChainLock validation), queueing ChainLock for later validation", 
         chain_lock.block_height, required_height);

🤖 Prompt for AI Agents

In dash-spv/src/chain/chainlock_manager.rs around lines 210 to 212, replace the
magic literal "8" used to compute the required masternode list height with a
clearly named constant (e.g. MASTERNODE_LIST_OFFSET or
CHAINLOCK_MASTERNODE_OFFSET) and add a short comment above the constant
describing that ChainLock validation requires the masternode list at
(block_height - OFFSET); define the constant at an appropriate module scope and
use it when computing required_height and in log messages so the meaning is
documented and easy to change.

[coderabbitai]

⚠️ Potential issue

Potential race condition in UTXO checking

The code checks if we have any UTXOs matching the transaction inputs to determine if it's outgoing. However, this check is not synchronized with potential UTXO updates, which could lead to incorrect classification if the UTXO set changes between this check and transaction processing.

Consider adding a note about the potential race condition or ensuring the wallet's UTXO set is locked during the entire transaction processing:

         // Determine if this is outgoing (we're spending)
+        // Note: This check assumes the UTXO set remains consistent during processing
         let is_outgoing = tx.input.iter().any(|input| wallet.has_utxo(&input.previous_output));

📝 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

	let is_outgoing = tx.input.iter().any(|input| wallet.has_utxo(&input.previous_output));
	// Determine if this is outgoing (we're spending)
	// Note: This check assumes the UTXO set remains consistent during processing
	let is_outgoing = tx.input.iter().any(|input| wallet.has_utxo(&input.previous_output));

🤖 Prompt for AI Agents

In dash-spv/src/mempool_filter.rs around line 199, the outgoing check uses
wallet.has_utxo(...) without synchronization, creating a race if the wallet's
UTXO set changes between this check and later processing; to fix, ensure the
UTXO set is read under the same lock or snapshot used during transaction
processing (e.g., acquire the wallet/UTXO read-lock for the duration of
classification and processing, or obtain an immutable snapshot/copy of the UTXO
set and base the check on that snapshot), or document the race clearly if
locking is not possible.

[coderabbitai]

🛠️ Refactor suggestion

Ineffective time manipulation in test

The comment at Lines 762-764 acknowledges that the test cannot actually set the first_seen time to be old, making the expiration test ineffective. The test essentially becomes a no-op.

Consider using a time abstraction that can be mocked for testing:

// Add a Clock trait that can be mocked
pub trait Clock: Send + Sync {
    fn now(&self) -> Instant;
}

// Use dependency injection to provide the clock
pub struct MempoolFilter {
    // ... other fields ...
    clock: Arc<dyn Clock>,
}

This would allow proper testing of time-based functionality.

🤖 Prompt for AI Agents

In dash-spv/src/mempool_filter.rs around lines 760 to 765, the test attempts to
set a transaction's first_seen time but can't modify Instant, so the expiration
test is a no-op; introduce a Clock abstraction (trait with now() -> Instant),
add a clock: Arc<dyn Clock> field to MempoolFilter and propagate it via
constructors, replace direct Instant::now() calls in production code with
clock.now(), and in tests inject a mock Clock (or fake) that returns a
manipulated older Instant so you can set up and assert expiration behavior
deterministically; update the test to use the mock clock to simulate old
first_seen and verify eviction.

[coderabbitai]

🛠️ Refactor suggestion

Code duplication between connect methods

The connect static method and connect_instance instance method have significant duplication in socket setup code. The TODO comment acknowledges this but doesn't provide a timeline for fixing it.

Extract the common socket setup logic into a private helper method:

fn setup_socket(stream: &TcpStream, read_timeout: Duration) -> NetworkResult<()> {
    stream.set_nodelay(true).map_err(|e| {
        NetworkError::ConnectionFailed(format!("Failed to set TCP_NODELAY: {}", e))
    })?;
    
    stream.set_read_timeout(Some(read_timeout)).map_err(|e| {
        NetworkError::ConnectionFailed(format!("Failed to set read timeout: {}", e))
    })?;
    
    Ok(())
}

🤖 Prompt for AI Agents

In dash-spv/src/network/connection.rs around lines 152 to 171, there is
duplicated socket setup code between the static connect and instance
connect_instance methods; extract that logic into a private helper fn (e.g., fn
setup_socket(stream: &TcpStream, read_timeout: Duration) -> NetworkResult<()>)
that sets TCP_NODELAY and read timeout and maps errors to
NetworkError::ConnectionFailed with descriptive messages, return Ok(()) on
success, and then replace the duplicated blocks in both connect and
connect_instance with calls to this helper (passing the stream and
self.read_timeout or the supplied read_timeout), keeping visibility private and
preserving existing error propagation.

[coderabbitai]

⚠️ Potential issue

Potential resource leak in disconnect

The disconnect method uses Arc::try_unwrap which will fail if there are other references to the state. In that case, the socket won't be properly shut down.

Consider always attempting to shut down the socket:

-        if let Some(state_arc) = self.state.take() {
-            if let Ok(state_mutex) = Arc::try_unwrap(state_arc) {
-                let state = state_mutex.into_inner();
-                let _ = state.stream.shutdown(std::net::Shutdown::Both);
-            }
-        }
+        if let Some(state_arc) = self.state.take() {
+            // Always try to shutdown, even if Arc has multiple references
+            if let Ok(state) = state_arc.try_lock() {
+                let _ = state.stream.shutdown(std::net::Shutdown::Both);
+            }
+        }

📝 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

	if let Some(state_arc) = self.state.take() {
	if let Ok(state_mutex) = Arc::try_unwrap(state_arc) {
	let state = state_mutex.into_inner();
	let _ = state.stream.shutdown(std::net::Shutdown::Both);
	}
	}
	if let Some(state_arc) = self.state.take() {
	// Always try to shutdown, even if Arc has multiple references
	if let Ok(state) = state_arc.try_lock() {
	let _ = state.stream.shutdown(std::net::Shutdown::Both);
	}
	}

🤖 Prompt for AI Agents

dash-spv/src/network/connection.rs around lines 193 to 198: the current
disconnect uses Arc::try_unwrap and only shuts the socket when there are no
other Arc refs, which leaks the socket if try_unwrap fails; instead always
attempt to shut down the underlying stream by: take the Option<Arc<Mutex<...>>>
as you do, then if Arc::try_unwrap succeeds use into_inner and shut the stream
as now, but if it fails call .lock() on the Arc<Mutex<...>> (handling
PoisonError) to get the guard and call shutdown on the contained stream, ignore
or log shutdown errors, and finally ensure you set self.state to None so the
connection is dropped; handle locking errors gracefully.

[coderabbitai]

⚠️ Potential issue

Potential message ordering issue

The code sends our version message after receiving the peer's version if we haven't sent it yet. However, the Bitcoin protocol typically expects both peers to send their version messages independently at connection start, not in response to receiving one.

This could cause handshake failures with some implementations. Consider always sending the version message immediately upon connection, not conditionally:

-                // If we haven't sent our version yet (peer initiated), send it now
-                if !self.version_sent {
-                    tracing::debug!("Peer initiated handshake, sending our version");
-                    self.send_version(connection).await?;
-                    self.version_sent = true;
-                }

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

🤖 Prompt for AI Agents

In dash-spv/src/network/handshake.rs around lines 155 to 161, the code
conditionally sends our version only after receiving the peer's version which
can create message-ordering issues; change the logic to send our version
immediately upon establishing the connection (unconditionally, e.g., at
connection setup or as the first step of the handshake) and keep tracking
version_sent to avoid duplicate sends; ensure send_version is awaited/handled
and version_sent is set to true right after the send so we no longer defer
sending until a peer-initiated message arrives.