feat: wallet manager integration into dash spv #106
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors dash-spv to dependency-inject network, storage, and wallet components, convert many modules to generics over StorageManager/NetworkManager/WalletInterface, remove internal wallet/UTXO/rollback subsystems, add key-wallet-manager (SPVWalletManager & WalletInterface), update examples/tests, and remove UTXO methods from storage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Client as DashSpvClient<W,N,S>
participant Sync as SequentialSyncManager<S,N>
participant Proc as BlockProcessor<W,S>
participant Net as NetworkManager (N)
participant Store as StorageManager (S)
participant Wallet as WalletInterface (W)
App->>Client: new(config, network, storage, wallet)
App->>Client: start()
Client->>Sync: start_sync(network, storage)
Net-->>Sync: headers/filters/messages
Sync->>Client: enqueue tasks (blocks/filters)
Client->>Proc: ProcessCompactFilter(filter, hash)
Proc->>Wallet: check_compact_filter(filter, hash, network)
Wallet-->>Proc: bool (matches?)
Proc-->>Client: SpvEvent::CompactFilterMatched? / response
Client->>Proc: ProcessBlock(block)
Proc->>Store: get_header_height_by_hash(hash)
Store-->>Proc: height?
Proc->>Wallet: process_block(block, height, network)
Wallet-->>Proc: relevant txids
Proc-->>Client: SpvEvent::BlockProcessed
sequenceDiagram
autonumber
participant App
participant Client as DashSpvClient<W,N,S>
participant Store as StorageManager (S)
participant Watch as WatchManager
App->>Client: add_watch_item(item)
Client->>Store: lock() (Arc<Mutex<S>>)
Client->>Watch: add_watch_item(watch_items, updater, item, &mut S)
Watch-->>Client: Result
Client-->>App: Result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (15)
dash-spv/src/network/reputation.rs (2)
173-176: Ban expiration check is unreachable; peers remain banned indefinitelyThe condition
is_banned() && ban_time_remaining().is_none()can never be true. Ifis_banned()is true,ban_time_remaining()is necessarilySome(_). This prevents bans from expiring.Apply this diff to correctly clear expired bans:
- // Check if ban has expired - if self.is_banned() && self.ban_time_remaining().is_none() { - self.banned_until = None; - } + // Check if ban has expired + if let Some(until) = self.banned_until { + if Instant::now() >= until { + self.banned_until = None; + } + }
379-401: Serialization errors cannot be converted into std::io::Error; these methods won’t compileBoth
save_to_storageandload_from_storagereturnstd::io::Result, but they useserde_json::{to_string_pretty, from_str}which error withserde_json::Error.?cannot convertserde_json::Errorintostd::io::Error, causing a compile error.Use a proper error type with thiserror and propagate both I/O and JSON errors. Minimal changes:
- pub async fn save_to_storage(&self, path: &std::path::Path) -> std::io::Result<()> { + pub async fn save_to_storage(&self, path: &std::path::Path) -> Result<(), ReputationStorageError> { @@ - let json = serde_json::to_string_pretty(&data)?; - tokio::fs::write(path, json).await + let json = serde_json::to_string_pretty(&data)?; + tokio::fs::write(path, json).await?; + Ok(()) } @@ - pub async fn load_from_storage(&self, path: &std::path::Path) -> std::io::Result<()> { + pub async fn load_from_storage(&self, path: &std::path::Path) -> Result<(), ReputationStorageError> { @@ - let json = tokio::fs::read_to_string(path).await?; - let data: Vec<(SocketAddr, SerializedPeerReputation)> = serde_json::from_str(&json)?; + let json = tokio::fs::read_to_string(path).await?; + let data: Vec<(SocketAddr, SerializedPeerReputation)> = serde_json::from_str(&json)?; @@ - Ok(()) + Ok(()) }Add this error type near the top of the file (outside the selected range):
use thiserror::Error; #[derive(Debug, Error)] pub enum ReputationStorageError { #[error("I/O error: {0}")] Io(#[from] std::io::Error), #[error("Serialization error: {0}")] Serde(#[from] serde_json::Error), }This aligns with the project guideline to use thiserror and ensures MSRV 1.80 compatibility.
Also applies to: 409-411
dash-spv/src/network/message_handler.rs (1)
21-23: Silenceunused_asynclint onhandle_messageSince
handle_messageis still awaited in multiple call sites, it must remainasync. To prevent Clippy’sunused_asyncwarning (which fails CI), add an allow attribute above the function:
- File:
dash-spv/src/network/message_handler.rs- Around line 21, update as follows:
/// Handle an incoming message. - pub async fn handle_message(&mut self, message: NetworkMessage) -> MessageHandleResult { + #[allow(clippy::unused_async)] + pub async fn handle_message(&mut self, message: NetworkMessage) -> MessageHandleResult { self.stats.messages_received += 1;dash-spv/src/sync/chainlock_validation.rs (2)
327-353: LRU bookkeeping can evict recent entries and grow duplicates in the queue
cache_resultalwayspush_back’sblock_hashwithout removing existing occurrences fromcache_lru. Over time (e.g., after TTL refreshes),cache_lruaccumulates duplicates; eviction by key then removes the current entry irrespective of recency, breaking the LRU property. It can also let the queue grow unnecessarily.Deduplicate before pushing:
fn cache_result(&mut self, block_hash: BlockHash, is_valid: bool, height: u32) { // Remove oldest entry if cache is full if self.validation_cache.len() >= self.config.cache_size { if let Some(oldest) = self.cache_lru.pop_front() { self.validation_cache.remove(&oldest); } } + // Ensure no duplicate of this key remains in the LRU queue + if let Some(pos) = self.cache_lru.iter().position(|h| h == &block_hash) { + self.cache_lru.remove(pos); + } + self.validation_cache.insert( block_hash, CachedChainLockResult { is_valid, height, timestamp: Instant::now(), }, ); self.cache_lru.push_back(block_hash); }
214-254: Unused helper inchainlock_validation.rs: remove or gate to avoiddead_codeThe private method
find_chain_lock_quorumin
dash-spv/src/sync/chainlock_validation.rs(lines 214–254) is never called and will trigger a dead_code warning (clippy errors are denied in CI). To address this:• If it’s planned for future use, add an explicit allowance or feature-gate it:
#[allow(dead_code)] fn find_chain_lock_quorum(…) { … } // — or — #[cfg(feature = "chainlock-quorum")] fn find_chain_lock_quorum(…) { … }• Otherwise, remove the method to keep the codebase clean and CI-green.
dash-spv/src/bloom/builder.rs (1)
101-104: Size the Bloom filter based on actual inserted elements (addresses add 1–2 entries).
actual_elementsunderestimates because each address inserts its script and, for P2PKH, also the pubkey hash. This can inflate the FPR and degrade matching. Compute the count by iterating addresses before constructing the filter.Apply this diff to improve sizing:
- // Calculate actual elements - let actual_elements = - self.addresses.len() + self.outpoints.len() + self.data_elements.len(); - let elements = std::cmp::max(self.elements, actual_elements as u32); + // Calculate actual elements (each address inserts its script and maybe a pubkey hash) + let address_inserts = self + .addresses + .iter() + .map(|addr| { + let script = addr.script_pubkey(); + let mut count = 1; // script + if extract_pubkey_hash(&script).is_some() { + count += 1; // pubkey hash for P2PKH + } + count + }) + .sum::<usize>(); + let actual_elements = address_inserts + self.outpoints.len() + self.data_elements.len(); + let elements = self.elements.max(actual_elements as u32);dash-spv/tests/multi_peer_test.rs (1)
198-205: Incorrect assertion (and contradictory message) on MAX_PEERSThe test asserts MAX_PEERS == 3 while the message says “Default max peers should be 12”. MAX_PEERS is a library default; your test is configuring config.max_peers = 3 for this client instance. Don’t assert the global constant; assert your config value, or assert runtime behavior.
Two possible fixes:
- If you intended to verify the default constant:
- println!("Maximum peer limit is set to: {}", MAX_PEERS); - assert_eq!(MAX_PEERS, 3, "Default max peers should be 12"); + println!("Library default maximum peer limit: {}", MAX_PEERS); + assert_eq!(MAX_PEERS, 12, "Default max peers should be 12");
- If you intended to validate the test’s configured limit:
- println!("Maximum peer limit is set to: {}", MAX_PEERS); - assert_eq!(MAX_PEERS, 3, "Default max peers should be 12"); + // Validate test configuration, not the library default + println!("Configured maximum peer limit for this client: {}", config.max_peers); + assert_eq!(config.max_peers, 3, "Client max_peers should be 3 in this test");dash-spv/src/chain/chainlock_manager.rs (2)
192-209: Holding a std::sync::RwLock guard across .await can deadlock; drop before awaitingengine_guard is held across the await in store_chain_lock_with_validation, which can block writers (e.g., set_masternode_engine) and cause subtle deadlocks/starvation. Scope or explicitly drop the guard before any await.
Refactor to limit the guard’s lifetime and perform queueing after it’s dropped:
- let engine_guard = self - .masternode_engine - .read() - .map_err(|_| ValidationError::InvalidChainLock("Lock poisoned".to_string()))?; - - let mut validated = false; - - if let Some(engine) = engine_guard.as_ref() { - // Use the masternode engine's verify_chain_lock method - match engine.verify_chain_lock(&chain_lock) { - Ok(()) => { - info!( - "✅ ChainLock validated with masternode engine for height {}", - chain_lock.block_height - ); - validated = true; - } - Err(e) => { - // 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 - CHAINLOCK_VALIDATION_MASTERNODE_OFFSET) - let required_height = chain_lock - .block_height - .saturating_sub(CHAINLOCK_VALIDATION_MASTERNODE_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); - drop(engine_guard); // Release the read lock before acquiring write lock - self.queue_pending_chainlock(chain_lock.clone()).map_err(|e| { - ValidationError::InvalidChainLock(format!( - "Failed to queue pending ChainLock: {}", - e - )) - })?; - } else { - return Err(ValidationError::InvalidChainLock(format!( - "MasternodeListEngine validation failed: {:?}", - e - ))); - } - } - } - } else { - // Queue for later validation when engine becomes available - warn!("⚠️ Masternode engine not available, queueing ChainLock for later validation"); - drop(engine_guard); // Release the read lock before acquiring write lock - self.queue_pending_chainlock(chain_lock.clone()).map_err(|e| { - ValidationError::InvalidChainLock(format!( - "Failed to queue pending ChainLock: {}", - e - )) - })?; - } + let mut validated = false; + let mut should_queue_for_later = false; + { + let engine_guard = self + .masternode_engine + .read() + .map_err(|_| ValidationError::InvalidChainLock("Lock poisoned".to_string()))?; + + if let Some(engine) = engine_guard.as_ref() { + match engine.verify_chain_lock(&chain_lock) { + Ok(()) => { + info!( + "✅ ChainLock validated with masternode engine for height {}", + chain_lock.block_height + ); + validated = true; + } + Err(e) => { + let error_string = e.to_string(); + if error_string.contains("No masternode lists in engine") { + let required_height = chain_lock + .block_height + .saturating_sub(CHAINLOCK_VALIDATION_MASTERNODE_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); + should_queue_for_later = true; + } else { + return Err(ValidationError::InvalidChainLock(format!( + "MasternodeListEngine validation failed: {:?}", + e + ))); + } + } + } + } else { + warn!("⚠️ Masternode engine not available, queueing ChainLock for later validation"); + should_queue_for_later = true; + } + } // engine_guard dropped here + + if should_queue_for_later { + self.queue_pending_chainlock(chain_lock.clone()).map_err(|e| { + ValidationError::InvalidChainLock(format!( + "Failed to queue pending ChainLock: {}", + e + )) + })?; + } // Store the chain lock with appropriate validation status self.store_chain_lock_with_validation(chain_lock.clone(), storage, validated).await?;Also applies to: 246-248
334-341: Use StorageManager’s dedicated ChainLock APIs instead of metadataConsistent persistence matters for portability and tooling. StorageManager exposes store_chain_lock/load_chain_lock, and disk/memory impls already place data under chainlocks/. This code bypasses those and writes to generic metadata keys.
Apply these diffs:
- // Store persistently - let key = format!("chainlock:{}", chain_lock.block_height); - let data = bincode::serialize(&chain_lock) - .map_err(|e| StorageError::Serialization(e.to_string()))?; - storage.store_metadata(&key, &data).await?; + // Store persistently using dedicated API + storage.store_chain_lock(chain_lock.block_height, &chain_lock).await?;- for height in start_height..=end_height { - let key = format!("chainlock:{}", height); - if let Some(data) = storage.load_metadata(&key).await? { - match bincode::deserialize::<ChainLock>(&data) { - Ok(chain_lock) => { + for height in start_height..=end_height { + if let Some(chain_lock) = storage.load_chain_lock(height).await? { // Cache it let entry = ChainLockEntry { chain_lock: chain_lock.clone(), received_at: std::time::SystemTime::now(), validated: true, }; let mut by_height = self.chain_locks_by_height.write().map_err(|_| { StorageError::LockPoisoned("chain_locks_by_height".to_string()) })?; let mut by_hash = self.chain_locks_by_hash.write().map_err(|_| { StorageError::LockPoisoned("chain_locks_by_hash".to_string()) })?; by_height.insert(chain_lock.block_height, entry.clone()); by_hash.insert(chain_lock.block_hash, entry); chain_locks.push(chain_lock); - } - Err(e) => { - error!("Failed to deserialize chain lock at height {}: {}", height, e); - } - } } }Also applies to: 417-444
dash-spv/src/client/block_processor.rs (1)
408-415: Fix logging placeholders: duplicate txid passed for “input {}:{}” messageThe format string prints the txid twice; the second placeholder should be the input index, not the txid.
- tracing::info!( - "💸 TX {} input {}:{} spending explicitly watched outpoint {:?}", - txid, - txid, - vin, - watched_outpoint - ); + tracing::info!( + "💸 TX {} input #{} spending explicitly watched outpoint {:?}", + txid, + vin, + watched_outpoint + );dash-spv/src/sync/masternodes.rs (1)
734-739: Incorrect P2P message variant: use GetMnListDiffThe network message variant appears misspelled as
GetMnListD. This likely won’t compile or will send the wrong message.- let network_message = NetworkMessage::GetMnListD(get_mnlist_diff); + let network_message = NetworkMessage::GetMnListDiff(get_mnlist_diff);dash-spv/src/sync/headers_with_reorg.rs (1)
684-768: Remove unreachable code block or implement Headers2 support.The code block starting at line 685 is marked as unreachable but contains substantial implementation details. Either remove this dead code entirely or create a tracking issue for implementing Headers2 support properly.
- #[allow(unreachable_code)] - { - // If this is the first headers2 message, and we need to initialize compression state - if !headers2.headers.is_empty() { - // ... (lines 688-767) - } - } + // TODO: Implement Headers2 support - tracked in issue #XXXdash-spv/src/sync/filters.rs (2)
1669-1713: Skip genesis indownload_filter_header_for_blockto avoid invalidstart_height=0CFHeaders do not cover genesis at height 0. Currently, if
height == 0, the subsequentrequest_filter_headers(network, height, block_hash)will error on the 0 start height.Apply this guard:
let height = self .find_height_for_block_hash(&block_hash, storage, 0, header_tip_height) .await? .ok_or_else(|| { SyncError::Validation(format!( "Cannot find height for block {} - header not found", block_hash )) })?; + // CFHeaders start at height 1; skip genesis (height 0) + if height == 0 { + tracing::debug!( + "Skipping CFHeaders request for genesis block {} (no filter header at height 0)", + block_hash + ); + return Ok(()); + }
21-36: Unused constants will fail clippy -D warnings
RECEIVE_TIMEOUT_MILLIS,COMPLETION_CHECK_INTERVAL_MS, andMAX_TIMEOUTSare defined but not used. Given CI treats clippy warnings as errors, this will fail the pipeline.Remove them:
-const RECEIVE_TIMEOUT_MILLIS: u64 = 100; ... -const MAX_TIMEOUTS: u32 = 10; ... -const COMPLETION_CHECK_INTERVAL_MS: u64 = 100; // How often to check for completionsAlternatively, prefix with an underscore if you intend to use them shortly.
Also applies to: 28-28, 35-35
dash-spv/src/sync/sequential/mod.rs (1)
608-621: Fix “pending” metric: currently logs pending block downloads instead of pending filter requests
pending_download_count()returns pending block downloads, not pending filter requests. In the DownloadingFilters phase this misleads operators.Use the flow-control status to log correct pending filter request counts:
- // Check if we have any active requests - let active_count = self.filter_sync.active_request_count(); - let pending_count = self.filter_sync.pending_download_count(); + // Check if we have any active/pending filter requests (flow control) + let (pending_count, active_count, _) = + self.filter_sync.get_flow_control_status(); tracing::warn!( "Filter sync status: {} active requests, {} pending", active_count, pending_count );
♻️ Duplicate comments (1)
dash-spv/tests/instantsend_integration_test.rs (1)
167-167: Fix duplicate incorrect SPVWalletManager constructor usage.Same issue as above - the constructor doesn't take parameters.
| let genesis = genesis_block(network).header; | ||
| let mut storage = MemoryStorageManager::new().await.unwrap(); | ||
| let chain_state = ChainState::new_for_network(network); | ||
| let storage = MemoryStorage::new(); |
There was a problem hiding this comment.
Repeated storage initialization issue
The same missing await pattern appears in the other test functions.
- let storage = MemoryStorage::new();
+ let storage = MemoryStorage::new().await.unwrap();Also applies to: 99-99
🤖 Prompt for AI Agents
In dash-spv/src/chain/reorg_test.rs around lines 65 and 99, the MemoryStorage is
being constructed with let storage = MemoryStorage::new(); but the constructor
is async and must be awaited; change both occurrences to let storage =
MemoryStorage::new().await; (ensure the surrounding test functions are async and
have the appropriate #[tokio::test] or runtime attribute so the .await is
valid).
| tracing::info!("💰 TX {} output {}:{} to {:?} (value: {}) - Address {} balance impact: +{}", | ||
| txid, txid, vout, watch_item, amount, address, balance_impact); | ||
|
|
There was a problem hiding this comment.
Fix logging placeholders: duplicate txid used for “output {}:{}”
The second placeholder should be the vout, not the txid again. This currently logs “output :” while the txid is already included earlier.
- tracing::info!("💰 TX {} output {}:{} to {:?} (value: {}) - Address {} balance impact: +{}",
- txid, txid, vout, watch_item, amount, address, balance_impact);
+ tracing::info!(
+ "💰 TX {} output #{} to {:?} (value: {}) - Address {} balance impact: +{}",
+ txid,
+ vout,
+ watch_item,
+ amount,
+ address,
+ balance_impact
+ );🤖 Prompt for AI Agents
In dash-spv/src/client/block_processor.rs around lines 444 to 446, the
tracing::info! call uses txid twice for the "output {}:{}" placeholders
resulting in "output <txid>:<vout>" being logged; change the second placeholder
argument to vout (so the placeholders correspond to txid, vout) to log the
output as "txid:vout" correctly, keeping the rest of the arguments in the same
order.
| mod tests { | ||
| use super::*; | ||
| use crate::consensus::{Decodable, Encodable}; | ||
| use crate::hashes::Hash; |
There was a problem hiding this comment.
Remove unused import to satisfy clippy -D warnings
use crate::hashes::Hash; is not used in the test module and will trigger an unused import error under clippy with -D warnings. Remove it to keep CI green.
- use crate::hashes::Hash;📝 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.
| use crate::hashes::Hash; |
🤖 Prompt for AI Agents
In dash/src/blockdata/transaction/special_transaction/mnhf_signal.rs around line
78, the test module contains an unused import `use crate::hashes::Hash;` which
triggers clippy -D warnings; remove that import line from the test module (or
move it to where it's actually used) and run cargo clippy to confirm no other
unused imports remain.
| for account in collection.standard_bip44_accounts.values() { | ||
| // You would need to derive addresses here based on the account | ||
| // This is simplified - in practice you'd derive up to the gap limit | ||
| } | ||
| } |
There was a problem hiding this comment.
Complete the BIP44 address derivation implementation.
The comment indicates this is simplified and needs proper address derivation up to the gap limit. This incomplete implementation could miss relevant transactions.
The BIP44 address derivation logic is incomplete. Would you like me to help implement proper address derivation including gap limit handling? This is critical for ensuring all wallet addresses are checked against compact filters.
| use dashcore::bip158::BlockFilter; | ||
| use dashcore::prelude::CoreBlockHeight; | ||
| use dashcore::{Block, Transaction, Txid}; | ||
| use key_wallet::Network; |
There was a problem hiding this comment.
Wrong Network type imported; breaks trait consumers
The rest of the codebase (tests and SPV client) use dashcore::Network. Importing key_wallet::Network here will cause type mismatches when implementing/using this trait from dash-spv.
Apply this diff to align with the ecosystem:
- use key_wallet::Network;
+ use dashcore::Network;🤖 Prompt for AI Agents
In key-wallet-manager/src/wallet_interface.rs around line 9, the file imports
key_wallet::Network which conflicts with the rest of the codebase expecting
dashcore::Network; replace the import with use dashcore::Network (or fully
qualify dashcore::Network where referenced) so the trait signatures and
consumers match the dash-spv/tests types, and ensure dashcore is listed as a
dependency in Cargo.toml if not already.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (6)
dash-spv/src/storage/mod.rs (1)
71-90: Fix doctest: trait-object Mutex requires Box and update Header import path.The example as written won’t compile:
tokio::sync::Mutexrequires a sized type;dyn StorageManageris unsized. Wrap it inBox. Also, theHeaderimport path has changed.Apply:
-/// # use dashcore::blockdata::block::Header as BlockHeader; +/// # use dashcore::block::Header as BlockHeader; @@ -/// let storage: Arc<Mutex<dyn StorageManager>> = Arc::new(Mutex::new(MemoryStorageManager::new().await?)); +/// let storage: Arc<Mutex<Box<dyn StorageManager>>> = +/// Arc::new(Mutex::new(Box::new(MemoryStorageManager::new().await?)));dash-spv/src/storage/disk.rs (3)
320-323: Prevent tip underflow when highest segment has zero valid headers (corruption/truncation case).If the top segment exists but contains zero headers (e.g., truncated file),
valid_count - 1underflows and produces a huge tip. Guard zero-entries.- let tip_height = - segment_id * HEADERS_PER_SEGMENT + segment.valid_count as u32 - 1; - *self.cached_tip_height.write().await = Some(tip_height); + if segment.valid_count > 0 { + let tip_height = segment_id * HEADERS_PER_SEGMENT + + (segment.valid_count as u32) + - 1; + *self.cached_tip_height.write().await = Some(tip_height); + } else { + tracing::warn!( + "Highest segment {} contains 0 headers; leaving cached tip unchanged", + segment_id + ); + }
247-250: Derive tip from loaded index to avoid relying on segment inspection.When an index is present, it is authoritative for the tip calculation and robust against partial/truncated segment files. Set
cached_tip_heightfrom the index’s max height.- if let Ok(index) = self.load_index_from_file(&index_path).await { - *self.header_hash_index.write().await = index; - index_loaded = true; - } + if let Ok(index) = self.load_index_from_file(&index_path).await { + *self.header_hash_index.write().await = index; + if let Some(max_h) = self.header_hash_index.read().await.values().max().copied() { + *self.cached_tip_height.write().await = Some(max_h); + } + index_loaded = true; + }
293-298: Also set tip after index rebuild.Populate
cached_tip_heightimmediately after rebuilding the index to cover cold-starts without an index file.- *self.header_hash_index.write().await = new_index; - tracing::info!( - "Index rebuilt with {} entries", - self.header_hash_index.read().await.len() - ); + *self.header_hash_index.write().await = new_index; + let len = self.header_hash_index.read().await.len(); + if let Some(max_h) = self.header_hash_index.read().await.values().max().copied() { + *self.cached_tip_height.write().await = Some(max_h); + } + tracing::info!("Index rebuilt with {} entries", len);dash-spv/src/client/mod.rs (2)
2629-2660: Avoid hardcoding genesis parameters; use dashcore’s genesis_block to prevent drift.Hardcoding version, merkle root, bits, nonce, and time for mainnet/testnet violates the “no hardcoded network parameters” guideline and risks divergence if upstream constants change. Prefer the canonical
genesis_block(network)for all networks.Apply this diff to simplify and centralize:
- let genesis_header = match self.config.network { - dashcore::Network::Dash => { - // Use the actual Dash mainnet genesis block parameters - BlockHeader { - version: Version::from_consensus(1), - prev_blockhash: dashcore::BlockHash::from([0u8; 32]), - merkle_root: "e0028eb9648db56b1ac77cf090b99048a8007e2bb64b68f092c03c7f56a662c7" - .parse() - .unwrap_or_else(|_| dashcore::hashes::sha256d::Hash::all_zeros().into()), - time: 1390095618, - bits: CompactTarget::from_consensus(0x1e0ffff0), - nonce: 28917698, - } - } - dashcore::Network::Testnet => { - // Use the actual Dash testnet genesis block parameters - BlockHeader { - version: Version::from_consensus(1), - prev_blockhash: dashcore::BlockHash::from([0u8; 32]), - merkle_root: "e0028eb9648db56b1ac77cf090b99048a8007e2bb64b68f092c03c7f56a662c7" - .parse() - .unwrap_or_else(|_| dashcore::hashes::sha256d::Hash::all_zeros().into()), - time: 1390666206, - bits: CompactTarget::from_consensus(0x1e0ffff0), - nonce: 3861367235, - } - } - _ => { - // For other networks, use the existing genesis block function - dashcore::blockdata::constants::genesis_block(self.config.network).header - } - }; + let genesis_header = + dashcore::blockdata::constants::genesis_block(self.config.network).header;
1503-1517: Bug: get_all_balances will always return an empty map and log errors.
get_address_balancenow unconditionally returns an error (by design to push callers to the wallet).get_all_balancesstill calls it, so it silently drops all errors and returns an empty map. That’s misleading API behavior.Apply this diff to make the behavior explicit and guide consumers:
/// Get balances for all watched addresses. pub async fn get_all_balances( &self, ) -> Result<std::collections::HashMap<dashcore::Address, AddressBalance>> { - let mut balances = std::collections::HashMap::new(); - - let addresses = self.get_watched_addresses_from_items().await; - for address in addresses { - if let Some(balance) = self.process_address_balance(&address, |balance| balance).await { - balances.insert(address, balance); - } - } - - Ok(balances) + Err(SpvError::Config( + "Address balances must be queried via the wallet implementation (WalletInterface)".to_string() + )) }If you need a convenience wrapper, we can add an optional trait extension or a generic hook that wallets can implement to expose per-address balances.
♻️ Duplicate comments (3)
dash-spv/src/storage/mod.rs (1)
179-179: Confirm all UTXO storage call sites are removed/migrated to wallet.The trait surface now documents the UTXO API removal. Ensure no stale references remain across code and tests (delegate to the wallet interface).
Run:
#!/bin/bash # Expect: no matches (production and tests) rg -nP '\b(store_utxo|remove_utxo|get_utxos_for_address|get_all_utxos)\b' --type=rust -g '!**/target/**'dash-spv/examples/reorg_demo.rs (1)
41-41: Constructor call fixed; good.
SPVWalletManager::new()is now called without parameters as required.key-wallet-manager/src/spv_wallet_manager.rs (1)
257-265: Fix compact-filter cache: never populated + redundant delegation (use interior mutability and write-through).This method reads from
filter_matchesbut never writes to it, so the cache never hits. It also delegates tobase.check_compact_filteroutright. Make the cache effective by using interior mutability and storing the computed result before returning. This also avoids changing the trait signature.Apply this diff:
@@ use alloc::vec::Vec; +use std::sync::RwLock; @@ /// Filter match cache (per network) - caches whether a filter matched - filter_matches: BTreeMap<Network, BTreeMap<BlockHash, bool>>, + filter_matches: RwLock<BTreeMap<Network, BTreeMap<BlockHash, bool>>>, @@ - filter_matches: BTreeMap::new(), + filter_matches: RwLock::new(BTreeMap::new()), @@ async fn check_compact_filter( &self, filter: &BlockFilter, block_hash: &BlockHash, network: Network, ) -> bool { - // Check if we've already evaluated this filter - if let Some(network_cache) = self.filter_matches.get(&network) { - if let Some(&matched) = network_cache.get(block_hash) { - return matched; - } - } - - self.base.check_compact_filter(filter, block_hash, network) + // Fast-path: try read cache without holding a write lock + if let Ok(cache) = self.filter_matches.read() { + if let Some(network_cache) = cache.get(&network) { + if let Some(&matched) = network_cache.get(block_hash) { + return matched; + } + } + } + + // Compute using base logic + let matched = self.base.check_compact_filter(filter, block_hash, network); + + // Write-through cache update + if let Ok(mut cache) = self.filter_matches.write() { + cache + .entry(network) + .or_insert_with(BTreeMap::new) + .insert(*block_hash, matched); + } + + matched }Also applies to: 41-43, 95-98, 9-16
🧹 Nitpick comments (10)
dash-spv/src/storage/disk.rs (1)
167-169: Prefertracing::error!overeprintln!in async worker for consistent logging.Aligns with existing tracing usage and avoids writing directly to stderr.
- eprintln!("Failed to save segment {}: {}", segment_id, e); + tracing::error!("Failed to save segment {}: {}", segment_id, e); @@ - eprintln!("Failed to save filter segment {}: {}", segment_id, e); + tracing::error!("Failed to save filter segment {}: {}", segment_id, e); @@ - eprintln!("Failed to save index: {}", e); + tracing::error!("Failed to save index: {}", e);Also applies to: 188-189, 206-207
dash-spv/tests/error_handling_test.rs (1)
381-385: Store sync state stub aligns with trait; OK.Consider adding a no-op
shutdown()to the mock before re-enabling the suite.dash-spv/examples/reorg_demo.rs (2)
9-12: Temporary disablement is fine, but track re-enabling.Given guidelines require runnable examples, please open an issue to update this demo to the new DI architecture and re-enable it.
I can open a follow-up issue and draft the migration plan (wallet DI + storage/network generics) if helpful.
37-43: Avoid hard-coding network in examples (minor).To comply with “no hardcoded network parameters,” consider selecting the network via CLI or env var, defaulting to mainnet.
Proposed tweak:
- Read
DASH_NETWORKenv (Dash, Testnet, Regtest, Devnet); default toDash.- Or add a simple
--networkarg when re-enabling the example.dash-spv/src/storage/memory.rs (1)
222-225: UTXO metrics placeholders: optional cleanup.Since UTXOs moved out, consider removing
utxos/utxo_address_indexmetrics entirely to avoid confusion in consumer dashboards.- component_sizes.insert("utxos".to_string(), utxo_size as u64); - component_sizes.insert("utxo_address_index".to_string(), utxo_address_index_size as u64); + // UTXO metrics removed; now handled by external walletkey-wallet-manager/src/spv_wallet_manager.rs (2)
101-114: Clarify return semantics of queue_block_download (currently returns true even on duplicate).As written, the function returns
truewhen the queue isn't full, even if the hash was not enqueued due to being a duplicate. This can mislead callers expectingtrueto mean "enqueued".Apply this diff to return whether the hash was actually enqueued:
pub fn queue_block_download(&mut self, network: Network, block_hash: BlockHash) -> bool { let queue = self.download_queues.entry(network).or_insert_with(VecDeque::new); if queue.len() >= self.max_download_queue { return false; } - if !queue.contains(&block_hash) { - queue.push_back(block_hash); - } - - true + if !queue.contains(&block_hash) { + queue.push_back(block_hash); + true + } else { + false + } }
214-218: Stats never initialized for network; counters won’t update.
self.stats.get_mut(&network)will beNoneunless an entry was pre-initialized elsewhere. Useentry(...).or_default()so stats update consistently.Apply this diff:
- // Update statistics - if let Some(stats) = self.stats.get_mut(&network) { - stats.blocks_downloaded += 1; - stats.transactions_found += relevant_tx_ids.len() as u64; - stats.sync_height = height; - } + // Update statistics + let stats = self.stats.entry(network).or_default(); + stats.blocks_downloaded += 1; + stats.transactions_found += relevant_tx_ids.len() as u64; + stats.sync_height = height;dash-spv/src/client/mod.rs (3)
501-503: Remove unnecessary wallet read guard in get_mempool_balance.
let _wallet = self.wallet.read().await;isn’t used and only adds lock contention. Dropping it has no functional impact.Apply this diff:
- let _wallet = self.wallet.read().await; let mempool_state = self.mempool_state.read().await;
1124-1151: Avoid cloning NetworkMessage in hot path.
message.clone()can be expensive. If possible, restructure to passmessageby value to the handler and perform the special CLSig/ISLock processing via a callback or move that logic into the handler to avoid the extra clone.Example approach:
- Move CLSig/ISLock post-processing into
MessageHandler::handle_network_messageand signal results via a lightweight enum, or- Consume
message(no clone), and for special cases, extract the necessary data before handing it off.
2837-2837: Comment contradicts the test content.The comment says “Test removed - needs external wallet implementation,” but the test below still validates core mempool balance logic. Update the comment to reflect intent to avoid confusion.
Apply this diff:
- // Test removed - needs external wallet implementation + // This test validates mempool balance logic without requiring a wallet implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
dash-spv/examples/reorg_demo.rs(1 hunks)dash-spv/src/client/consistency.rs(0 hunks)dash-spv/src/client/consistency_test.rs(0 hunks)dash-spv/src/client/mod.rs(78 hunks)dash-spv/src/storage/disk.rs(10 hunks)dash-spv/src/storage/memory.rs(5 hunks)dash-spv/src/storage/mod.rs(3 hunks)dash-spv/tests/error_handling_test.rs(2 hunks)dash-spv/tests/instantsend_integration_test.rs(3 hunks)key-wallet-manager/src/spv_wallet_manager.rs(1 hunks)
💤 Files with no reviewable changes (2)
- dash-spv/src/client/consistency_test.rs
- dash-spv/src/client/consistency.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/tests/instantsend_integration_test.rs
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{rs,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rsdash-spv/src/storage/memory.rsdash-spv/tests/error_handling_test.rsdash-spv/examples/reorg_demo.rsdash-spv/src/storage/disk.rskey-wallet-manager/src/spv_wallet_manager.rs
**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rsdash-spv/src/storage/memory.rsdash-spv/tests/error_handling_test.rsdash-spv/examples/reorg_demo.rsdash-spv/src/storage/disk.rskey-wallet-manager/src/spv_wallet_manager.rs
{dash-network/**,dash-spv/**,rpc-client/**,rpc-json/**}/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
Always validate inputs from untrusted sources
Files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rsdash-spv/src/storage/memory.rsdash-spv/tests/error_handling_test.rsdash-spv/examples/reorg_demo.rsdash-spv/src/storage/disk.rs
dash-spv/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Code must be rustfmt-formatted (cargo fmt --check must pass)
Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rsdash-spv/src/storage/memory.rsdash-spv/tests/error_handling_test.rsdash-spv/examples/reorg_demo.rsdash-spv/src/storage/disk.rs
dash-spv/**/storage/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/storage/disk.rs
dash-spv/**/{network,storage}/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Expose swappable implementations behind trait objects (e.g., Arc, Arc) for runtime polymorphism
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/storage/disk.rs
{rpc-integration-test/**,dash-spv/tests/**}/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
{rpc-integration-test/**,dash-spv/tests/**}/**/*.rs: Provide integration tests for network operations
Ensure tests cover both mainnet and testnet configurations
Files:
dash-spv/tests/error_handling_test.rs
**/tests/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
Files:
dash-spv/tests/error_handling_test.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Place integration tests under the tests/ directory and organize them as standalone test files
Files:
dash-spv/tests/error_handling_test.rs
dash-spv/examples/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Provide runnable examples under the examples/ directory (e.g., simple_sync, filter_sync)
Files:
dash-spv/examples/reorg_demo.rs
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/{network,storage}/**/*.rs : Expose swappable implementations behind trait objects (e.g., Arc<dyn NetworkManager>, Arc<dyn StorageManager>) for runtime polymorphism
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : When accessing SPV functionality, use DashSDK public APIs instead of direct client access; add public wrapper methods to DashSDK if needed
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/storage/**/*.rs : Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rsdash-spv/src/storage/memory.rsdash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/{network,storage}/**/*.rs : Expose swappable implementations behind trait objects (e.g., Arc<dyn NetworkManager>, Arc<dyn StorageManager>) for runtime polymorphism
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rskey-wallet-manager/src/spv_wallet_manager.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/storage/**/*.rs : Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rsdash-spv/src/storage/memory.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/disk.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:15:57.225Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : When accessing SPV functionality, use DashSDK public APIs instead of direct client access; add public wrapper methods to DashSDK if needed
Applied to files:
dash-spv/src/client/mod.rsdash-spv/examples/reorg_demo.rs
📚 Learning: 2025-08-16T04:15:57.225Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/SPVClient.swift : Do not place type definitions in SPVClient.swift; keep types in dedicated files instead
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/error.rs : Define and maintain domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) in the central error module
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-06-26T15:51:03.126Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:51:03.126Z
Learning: In Rust network code, using unwrap() on SystemTime::now().duration_since(UNIX_EPOCH) can panic if the system clock is before UNIX_EPOCH. Use unwrap_or with a safe fallback (e.g., Duration::from_secs(0)) to prevent panics in edge cases.
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Ensure tests cover both mainnet and testnet configurations
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the tests/ directory and organize them as standalone test files
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv/src/client/mod.rs
🧬 Code Graph Analysis (7)
dash-spv/src/storage/mod.rs (3)
dash-spv/src/storage/memory.rs (1)
shutdown(483-485)dash-spv/src/client/mod.rs (1)
shutdown(651-653)dash-spv/src/storage/disk.rs (1)
shutdown(1717-1731)
dash-spv/src/client/mod.rs (13)
dash-spv/src/storage/memory.rs (7)
std(199-199)std(200-200)std(206-206)std(213-213)std(220-220)std(220-220)stats(195-255)dash-spv/src/mempool_filter.rs (3)
new(31-47)new(298-304)network(314-316)dash-spv/src/client/message_handler.rs (1)
new(33-59)dash-spv/src/sync/headers_with_reorg.rs (3)
new(74-109)new(1287-1291)default(39-46)dash-spv/src/sync/sequential/mod.rs (1)
new(78-103)dash-spv/src/sync/headers.rs (1)
new(32-42)dash-spv/src/client/config.rs (2)
new(240-246)default(180-235)dash-spv/src/client/status_display.rs (2)
stats(108-111)chain_state(114-117)dash-spv/src/network/message_handler.rs (1)
stats(110-112)dash-spv/src/sync/validation.rs (2)
stats(508-516)default(45-56)dash-spv/src/chain/checkpoints.rs (1)
default(74-80)dash-spv/src/client/block_processor.rs (1)
get_address_balance(585-594)dash-spv/src/client/watch_manager.rs (3)
add_watch_item(19-67)remove_watch_item(70-116)load_watch_items(119-144)
dash-spv/src/storage/memory.rs (2)
dash-spv/src/storage/mod.rs (1)
shutdown(266-266)dash-spv/src/storage/disk.rs (1)
shutdown(1717-1731)
dash-spv/tests/error_handling_test.rs (4)
dash-spv/src/storage/memory.rs (3)
load_filter_headers(121-130)store_sync_state(283-296)load_sync_state(298-308)dash-spv/src/storage/mod.rs (3)
load_filter_headers(127-127)store_sync_state(182-182)load_sync_state(185-185)dash-spv/src/storage/disk.rs (3)
load_filter_headers(1192-1224)store_sync_state(1457-1477)load_sync_state(1479-1495)dash-spv/src/client/mod.rs (1)
storage(2801-2803)
dash-spv/examples/reorg_demo.rs (7)
dash/src/blockdata/constants.rs (1)
genesis_block(112-185)dash-spv/src/chain/reorg.rs (2)
create_test_header(546-552)new(61-67)dash-spv/src/chain/fork_detector.rs (1)
create_test_header(243-248)dash-spv/src/sync/headers2_state.rs (1)
create_test_header(208-217)dash-spv/src/types.rs (3)
new_for_network(202-239)new(197-199)new(1076-1096)key-wallet-manager/src/spv_wallet_manager.rs (1)
new(90-99)dash-spv/src/chain/chain_work.rs (1)
from_bytes(85-89)
dash-spv/src/storage/disk.rs (4)
dash-spv/src/storage/memory.rs (1)
shutdown(483-485)dash-spv/src/storage/mod.rs (1)
shutdown(266-266)dash-spv/src/client/mod.rs (1)
shutdown(651-653)dash-spv/src/network/multi_peer.rs (1)
shutdown(953-983)
key-wallet-manager/src/spv_wallet_manager.rs (3)
key-wallet-manager/src/wallet_manager/mod.rs (2)
new(67-73)new(98-104)key-wallet-manager/src/wallet_interface.rs (5)
process_block(16-21)process_mempool_transaction(24-24)handle_reorg(27-32)check_compact_filter(36-41)as_any(44-44)key-wallet-manager/src/wallet_manager/process_block.rs (3)
process_block(8-36)handle_reorg(38-60)check_compact_filter(62-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
🔇 Additional comments (9)
dash-spv/src/storage/mod.rs (1)
265-266: Good addition: explicit shutdown hook in StorageManager.Introducing
shutdown()formalizes lifecycle management for storage backends and aligns with disk/memory impls.dash-spv/src/storage/disk.rs (1)
1716-1731: Shutdown path looks correct.Saves dirty segments before signaling the worker and awaits task join. Matches the new trait hook.
dash-spv/tests/error_handling_test.rs (2)
255-258: Signature formatting matches trait; OK.No behavioral change. When re-enabling this file, ensure the mock also implements
shutdown()per the updated trait.Would you like me to patch this mock with a no-op
shutdown()now to avoid future compile breaks?
391-394: Load sync state stub aligns; OK. Also reconcile StorageStats fields before re-enabling.Elsewhere in this file,
StorageStatsstruct usage appears to reference fields likeheaders_countand size-bytes which don’t exist anymore. Update those when removing#![cfg(skip_mock_implementation_incomplete)].I can generate a focused diff for the
StorageStatsfield renames when you plan to re-enable these tests.dash-spv/src/storage/memory.rs (3)
7-7: Import adjustments are consistent with mod.rs.Matching aliases and types ensure coherence across storage backends.
190-192: Good: clear mempool state onclear().Brings memory storage semantics in line with disk backend.
483-485: No-opshutdown()is appropriate for in-memory storage.Ensure callers consistently invoke
shutdown()on storage during client teardown so disk and memory backends share the same lifecycle.I can audit call sites and add a single
client.shutdown().awaitpath in examples/tests if needed.key-wallet-manager/src/spv_wallet_manager.rs (1)
187-195: Incorrect dead code warning on ProcessBlockResultProcessBlockResult is publicly re-exported in
key-wallet-manager/src/lib.rs(line 44), so it is part of the crate’s public API and counts as a usage. Nodead_codewarning will be emitted, and no#[allow(dead_code)]or removal is needed.dash-spv/src/client/mod.rs (1)
42-51: DI architecture via generics and external wallet looks solid.Introducing
wallet: Arc<RwLock<W>>and genericDashSpvClient<W, N, S>cleanly separates concerns and enables flexible backends. Bounds in the impl enforce Send/Sync neatly.Also applies to: 95-103
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
dash-spv/src/storage/disk.rs (2)
729-839: Bug: Partial-segment writes don’t advance valid_count correctly, making first checkpoint segment unreadableIn
store_headers_from_height, when starting at a checkpoint height that is not aligned to segment start, you write intooffset > 0. Sincevalid_countonly increments whenoffset == valid_count, the first partial segment remains withvalid_count == 0, soload_headersandget_headerwon’t return these entries. This breaks reads for the first partial segment during checkpoint sync.Minimal fix: bump
valid_countto cover the highest written offset + 1 if it expands the contiguous prefix.- if offset == segment.valid_count { - segment.valid_count += 1; - } + if offset == segment.valid_count { + segment.valid_count += 1; + } else if offset > segment.valid_count { + // We are writing beyond the current contiguous prefix. + // If there are gaps, valid_count should not leap over them. + // However, because we pad with sentinels, those gaps remain invalid. + // To support random-access reads (get_header), we should at least make + // this header readable by height; see save/load note below. + // If you want to keep "contiguous prefix" semantics for ranged reads, + // leave valid_count unchanged here and rely on direct get-by-height. + }Note: This addresses immediate get-by-height reads if you additionally fix persistence (see next comment), or if you don’t rely on
valid_countfor those lookups. The core issue is persistence format losing offset info; see below.
845-874: Critical: Segment file format drops offsets; reloading corrupts height-to-offset mapping
save_segment_to_diskskips sentinel entries and writes only real headers, butload_headers_from_filethen loads them as a dense vector starting at index 0. After padding, all real headers end up at the beginning of the segment, destroying their original offsets. As a result:
get_header(height)computes (segment_id, offset) and may return the wrong header or None- Index rebuild (
load_segment_metadata) uses enumerate() to infer heights, which is incorrect for partial segments- Ranged reads (
load_headers) become inconsistentYou need a persistence format that preserves offsets. Two options:
Option A (preferred): Store (offset, header) pairs.
- On save: write a u32 count, then for each non-sentinel item, write its u32 offset followed by the header.
- On load: read count; for each entry, read offset then header and place header at that offset in the vector.
Patch sketch:
async fn save_segment_to_disk(path: &Path, headers: &[BlockHeader]) -> StorageResult<()> { tokio::task::spawn_blocking({ let path = path.to_path_buf(); - let headers = headers.to_vec(); + let headers = headers.to_vec(); move || { let file = OpenOptions::new().create(true).write(true).truncate(true).open(&path)?; let mut writer = BufWriter::new(file); - - // Only save actual headers, not sentinel headers - for header in headers { - // Skip sentinel headers (used for padding) - if header.version.to_consensus() == i32::MAX - && header.time == u32::MAX - && header.nonce == u32::MAX - && header.prev_blockhash == BlockHash::from_byte_array([0xFF; 32]) - { - continue; - } - header.consensus_encode(&mut writer).map_err(|e| { - StorageError::WriteFailed(format!("Failed to encode header: {}", e)) - })?; - } + // Collect (offset, header) pairs for non-sentinels + let mut items: Vec<(u32, BlockHeader)> = Vec::new(); + for (idx, header) in headers.into_iter().enumerate() { + if header.version.to_consensus() == i32::MAX + && header.time == u32::MAX + && header.nonce == u32::MAX + && header.prev_blockhash == BlockHash::from_byte_array([0xFF; 32]) + { + continue; + } + items.push(idx as u32, header); + } + // Write count + let count = items.len() as u32; + writer.write_all(&count.to_le_bytes())?; + // Write entries: offset + header + for (offset, header) in items { + writer.write_all(&offset.to_le_bytes())?; + header.consensus_encode(&mut writer).map_err(|e| { + StorageError::WriteFailed(format!("Failed to encode header: {}", e)) + })?; + } writer.flush()?; Ok(()) } })async fn load_headers_from_file(&self, path: &Path) -> StorageResult<Vec<BlockHeader>> { tokio::task::spawn_blocking({ let path = path.to_path_buf(); move || { let file = File::open(&path)?; let mut reader = BufReader::new(file); - let mut headers = Vec::new(); - - loop { - match BlockHeader::consensus_decode(&mut reader) { - Ok(header) => headers.push(header), - Err(encode::Error::Io(ref e)) - if e.kind() == std::io::ErrorKind::UnexpectedEof => - { - break - } - Err(e) => { - return Err(StorageError::ReadFailed(format!( - "Failed to decode header: {}", - e - ))) - } - } - } - - Ok(headers) + // Read count + use std::io::Read; + let mut count_bytes = [0u8; 4]; + reader.read_exact(&mut count_bytes).map_err(|e| { + StorageError::ReadFailed(format!("Failed to read segment count: {}", e)) + })?; + let count = u32::from_le_bytes(count_bytes) as usize; + + // Prepare a dense temporary vector of (offset, header) + let mut pairs: Vec<(u32, BlockHeader)> = Vec::with_capacity(count); + for _ in 0..count { + let mut off_bytes = [0u8; 4]; + reader.read_exact(&mut off_bytes).map_err(|e| { + StorageError::ReadFailed(format!("Failed to read entry offset: {}", e)) + })?; + let offset = u32::from_le_bytes(off_bytes); + let header = BlockHeader::consensus_decode(&mut reader).map_err(|e| { + StorageError::ReadFailed(format!("Failed to decode header: {}", e)) + })?; + pairs.push((offset, header)); + } + // Reconstruct header vector with sentinels at missing positions + let mut headers = vec![create_sentinel_header(); HEADERS_PER_SEGMENT as usize]; + let mut valid_max = 0usize; + for (offset, header) in pairs { + let idx = offset as usize; + if idx < headers.len() { + headers[idx] = header; + valid_max = valid_max.max(idx + 1); + } + } + // Truncate to highest valid index to keep downstream logic efficient + headers.truncate(valid_max.max(0)); + Ok(headers) } })Option B: Always write full segments (including sentinels). Simpler to load, at the cost of file size.
Pick one and make format consistent across save/load and index rebuild.
Also applies to: 648-679
♻️ Duplicate comments (1)
dash-spv/src/client/mod.rs (1)
638-640: Storage shutdown correctly calls trait method.The storage shutdown now properly calls the trait method directly without downcasting, which maintains the abstraction provided by the
StorageManagertrait.
🧹 Nitpick comments (11)
key-wallet-manager/src/spv_wallet_manager.rs (2)
214-218: Stats are updated only if pre-initialized; use entry-or-default to avoid missing updates
self.stats.get_mut(&network)may return None unless the caller remembered to initialize stats. Safer to ensure the stats entry exists on first update.- // Update statistics - if let Some(stats) = self.stats.get_mut(&network) { - stats.blocks_downloaded += 1; - stats.transactions_found += relevant_tx_ids.len() as u64; - stats.sync_height = height; - } + // Update statistics (auto-initialize for this network) + let stats = self.stats.entry(network).or_default(); + stats.blocks_downloaded += 1; + stats.transactions_found += relevant_tx_ids.len() as u64; + stats.sync_height = height;
145-165: Sync status may report Synced when target not set; consider explicit target handlingIf
stats.target_heightis zero (default) andsync_heightis also zero,Syncedis returned. If you rely on explicit targets, consider treating target=0 as “unknown” and returning Idle/Checking instead.Action: either initialize
target_heightexplicitly when creating the manager, or add a guard:
- If
target_height == 0andsync_height == 0, returnIdle.dash-spv/tests/instantsend_integration_test.rs (2)
13-13: Entire file gated out; OK to defer, but ensure CI doesn’t silently skip critical coverageThe crate-level cfg disables compilation. This is fine as a placeholder, but we’ll lose coverage for InstantSend path regressions. Please add a tracking issue and a plan to unskip once the SPVWalletManager API is in place.
99-123: API drift vs. SPVWalletManager: code references methods not in SPV wrapperThe test uses wallet APIs such as add_utxo, add_watched_address, get_utxos, get_balance/get_total_balance, process_verified_instantlock which are noted as missing. Given this file is cfg-skipped, that’s fine for now. When re-enabling:
- Either add these APIs to SPVWalletManager (or expose them through
base) or- Update the tests to use the new wallet interface methods
I can help write an adapter layer for the tests to decouple from the evolving API.
Also applies to: 167-223
dash-spv/tests/error_handling_test.rs (1)
34-41: Outdated wallet import referenceThis test still imports
dash_spv::wallet::Utxo. Wallet-related types were moved out in this PR. The file is cfg-skipped, but when re-enabling, update imports accordingly or remove wallet-specific tests from this file.dash-spv/src/storage/memory.rs (2)
195-255: StorageStats keys still include utxo fields set to 0; optional cleanupYou retain “utxos” and “utxo_address_index” as zeroed components. That’s harmless but slightly misleading. Consider removing them from component_sizes to reflect the new responsibility split.
281-286: Persisting sync state in metadata is acceptable for in-memory; note non-persistence in docsGiven this backend is ephemeral, the inline JSON-in-metadata approach is fine. Consider a doc comment on
load_sync_state/store_sync_stateindicating it does not survive process restart to prevent false expectations.Also applies to: 298-309
dash-spv/src/client/mod.rs (4)
719-723: Consider making timing intervals configurable.The status update and timeout check intervals are hardcoded. Consider moving these to the
ClientConfigfor better flexibility:- let status_update_interval = Duration::from_millis(500); + let status_update_interval = Duration::from_millis(self.config.status_update_interval_ms.unwrap_or(500)); // Timer for request timeout checking let mut last_timeout_check = Instant::now(); - let timeout_check_interval = Duration::from_secs(1); + let timeout_check_interval = Duration::from_secs(self.config.timeout_check_interval_secs.unwrap_or(1));
1075-1076: Consider making the no-message sleep duration configurable.The hardcoded 100ms sleep when no messages are available could impact responsiveness. Consider making it configurable:
- tokio::time::sleep(Duration::from_millis(100)).await; + tokio::time::sleep(Duration::from_millis(self.config.message_poll_interval_ms.unwrap_or(100))).await;
1333-1349: Remove commented code or convert to proper documentation.Lines 1334-1336 contain a comment about UTXO tracking being handled by the wallet. If this is important context, consider converting it to proper documentation; otherwise, remove the redundant comment:
// Process inputs first (spending UTXOs) if !is_coinbase { for input in transaction.input.iter() { - // UTXO tracking is now handled internally by the wallet - // Check against explicitly watched outpoints + // Check against explicitly watched outpoints (UTXO tracking is handled by the wallet) for watch_item in watch_items {
2887-2891: Simplify error handling for Address creation.The
from_scriptmethod already returns a Result, so the pattern matching is appropriate. However, consider logging failed address conversions for debugging:if let Ok(out_addr) = dashcore::Address::from_script( &output.script_pubkey, dashcore::Network::Dash, ) { if out_addr == address { address_balance_change += output.value as i64; } + } else { + tracing::trace!("Failed to parse output script as address"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
dash-spv/examples/reorg_demo.rs(1 hunks)dash-spv/src/client/consistency.rs(0 hunks)dash-spv/src/client/consistency_test.rs(0 hunks)dash-spv/src/client/mod.rs(78 hunks)dash-spv/src/storage/disk.rs(10 hunks)dash-spv/src/storage/memory.rs(5 hunks)dash-spv/src/storage/mod.rs(3 hunks)dash-spv/tests/error_handling_test.rs(2 hunks)dash-spv/tests/instantsend_integration_test.rs(3 hunks)key-wallet-manager/src/spv_wallet_manager.rs(1 hunks)
💤 Files with no reviewable changes (2)
- dash-spv/src/client/consistency_test.rs
- dash-spv/src/client/consistency.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/src/storage/mod.rs
- dash-spv/examples/reorg_demo.rs
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{rs,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/src/storage/disk.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/memory.rskey-wallet-manager/src/spv_wallet_manager.rs
**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/src/storage/disk.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/memory.rskey-wallet-manager/src/spv_wallet_manager.rs
{dash-network/**,dash-spv/**,rpc-client/**,rpc-json/**}/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
Always validate inputs from untrusted sources
Files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/src/storage/disk.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/memory.rs
dash-spv/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Code must be rustfmt-formatted (cargo fmt --check must pass)
Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/src/storage/disk.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/memory.rs
{rpc-integration-test/**,dash-spv/tests/**}/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
{rpc-integration-test/**,dash-spv/tests/**}/**/*.rs: Provide integration tests for network operations
Ensure tests cover both mainnet and testnet configurations
Files:
dash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_handling_test.rs
**/tests/**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
Files:
dash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_handling_test.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Place integration tests under the tests/ directory and organize them as standalone test files
Files:
dash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_handling_test.rs
dash-spv/**/storage/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
Files:
dash-spv/src/storage/disk.rsdash-spv/src/storage/memory.rs
dash-spv/**/{network,storage}/**/*.rs
📄 CodeRabbit Inference Engine (dash-spv/CLAUDE.md)
Expose swappable implementations behind trait objects (e.g., Arc, Arc) for runtime polymorphism
Files:
dash-spv/src/storage/disk.rsdash-spv/src/storage/memory.rs
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/{network,storage}/**/*.rs : Expose swappable implementations behind trait objects (e.g., Arc<dyn NetworkManager>, Arc<dyn StorageManager>) for runtime polymorphism
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : When accessing SPV functionality, use DashSDK public APIs instead of direct client access; add public wrapper methods to DashSDK if needed
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/storage/**/*.rs : Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/SPVClient.swift : Do not place type definitions in SPVClient.swift; keep types in dedicated files instead
📚 Learning: 2025-08-16T04:15:57.225Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : When accessing SPV functionality, use DashSDK public APIs instead of direct client access; add public wrapper methods to DashSDK if needed
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rskey-wallet-manager/src/spv_wallet_manager.rs
📚 Learning: 2025-08-16T04:15:57.225Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/SPVClient.swift : Do not place type definitions in SPVClient.swift; keep types in dedicated files instead
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/{network,storage}/**/*.rs : Expose swappable implementations behind trait objects (e.g., Arc<dyn NetworkManager>, Arc<dyn StorageManager>) for runtime polymorphism
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/error_handling_test.rskey-wallet-manager/src/spv_wallet_manager.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/client/mod.rsdash-spv/src/storage/disk.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/error.rs : Define and maintain domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) in the central error module
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/storage/**/*.rs : Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
Applied to files:
dash-spv/src/client/mod.rsdash-spv/src/storage/disk.rsdash-spv/tests/error_handling_test.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-06-26T15:51:03.126Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:51:03.126Z
Learning: In Rust network code, using unwrap() on SystemTime::now().duration_since(UNIX_EPOCH) can panic if the system clock is before UNIX_EPOCH. Use unwrap_or with a safe fallback (e.g., Duration::from_secs(0)) to prevent panics in edge cases.
Applied to files:
dash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Ensure tests cover both mainnet and testnet configurations
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the tests/ directory and organize them as standalone test files
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/tests/integration_real_node_test.rs : Integration tests that connect to a real Dash Core node (127.0.0.1:9999) must gracefully skip when the node is unavailable
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Code must be rustfmt-formatted (cargo fmt --check must pass)
Applied to files:
dash-spv/tests/error_handling_test.rs
🧬 Code Graph Analysis (6)
dash-spv/src/client/mod.rs (4)
dash-spv/src/client/message_handler.rs (1)
new(33-59)dash-spv/src/storage/mod.rs (1)
stats(163-163)dash-spv/src/client/block_processor.rs (1)
get_address_balance(585-594)dash-spv/src/client/watch_manager.rs (2)
add_watch_item(19-67)remove_watch_item(70-116)
dash-spv/tests/instantsend_integration_test.rs (3)
dash-spv/src/client/mod.rs (3)
network(467-469)network(1532-1535)new(215-282)key-wallet-manager/src/spv_wallet_manager.rs (1)
new(90-99)dash-spv/src/client/watch_manager_test.rs (1)
new(23-28)
dash-spv/src/storage/disk.rs (4)
dash-spv/src/storage/memory.rs (1)
shutdown(483-485)dash-spv/src/storage/mod.rs (1)
shutdown(266-266)dash-spv/src/client/mod.rs (1)
shutdown(651-653)dash-spv/src/network/multi_peer.rs (1)
shutdown(953-983)
dash-spv/tests/error_handling_test.rs (4)
dash-spv/src/storage/memory.rs (3)
load_filter_headers(121-130)store_sync_state(283-296)load_sync_state(298-308)dash-spv/src/storage/mod.rs (3)
load_filter_headers(127-127)store_sync_state(182-182)load_sync_state(185-185)dash-spv/src/storage/disk.rs (3)
load_filter_headers(1192-1224)store_sync_state(1457-1477)load_sync_state(1479-1495)dash-spv/src/client/mod.rs (1)
storage(2801-2803)
dash-spv/src/storage/memory.rs (3)
dash-spv/src/storage/mod.rs (1)
shutdown(266-266)dash-spv/src/storage/disk.rs (1)
shutdown(1717-1731)dash-spv/src/client/mod.rs (1)
shutdown(651-653)
key-wallet-manager/src/spv_wallet_manager.rs (3)
key-wallet-manager/src/wallet_manager/mod.rs (2)
new(67-73)new(98-104)key-wallet-manager/src/wallet_interface.rs (5)
process_block(16-21)process_mempool_transaction(24-24)handle_reorg(27-32)check_compact_filter(36-41)as_any(44-44)key-wallet-manager/src/wallet_manager/process_block.rs (3)
process_block(8-36)handle_reorg(38-60)check_compact_filter(62-83)
🔇 Additional comments (14)
dash-spv/tests/error_handling_test.rs (1)
255-263: Signature reformatting only; no functional change detectedThe multi-line async signatures match the trait. No issues here.
Also applies to: 381-389, 391-399
dash-spv/src/storage/memory.rs (2)
182-193: Good: clear() now resets mempool structures tooThe clear path properly resets mempool state after UTXO removal from storage responsibilities. Looks consistent with the trait expectations.
482-486: No-op shutdown aligns with new trait hookAdding a no-op
shutdownis good for symmetry with disk storage.dash-spv/src/storage/disk.rs (3)
563-645: Good: background saving state transitions fixed; avoids marking dirty segments as clean prematurelyMarking segments as Saving and flipping to Clean only on notification resolves prior race conditions. Nice.
1716-1731: Graceful shutdown looks solidSaving dirty segments, signaling the worker, and awaiting join handles is the right sequence. This aligns with the trait’s new shutdown hook and with the network manager’s shutdown semantics.
272-299: Index rebuild enumeration is correct for the current on-disk formatAfter inspecting the save/load paths and tests:
save_segment_to_diskwrites only the active headers in order (validated by the storage test at line 1804).load_headers_from_file(lines 657–664) decodes headers sequentially until EOF.- The rebuild snippet (lines 282–287) uses
enumerate()on that Vec, which correctly maps each decoded header to its offset for a dense file.No changes are needed unless you introduce an offset-preserving serialization format in the future—in that case you’d decode offsets along with headers and rebuild the index using those recorded offsets.
dash-spv/src/client/mod.rs (8)
12-12: LGTM! Proper use of Arc for shared storage.The addition of
Muteximport aligns with the new design of wrapping storage inArc<Mutex<S>>for thread-safe shared access across components.
43-50: Good separation of concerns with generic type parameters.The refactoring to make
DashSpvClientgeneric overW: WalletInterface,N: NetworkManager, andS: StorageManagerprovides excellent flexibility and testability. Wrapping storage inArc<Mutex<S>>properly addresses the concurrent access requirements while maintaining encapsulation.
214-220: Well-designed constructor with dependency injection.The new constructor signature properly accepts external implementations of network, storage, and wallet, following dependency injection principles. The storage is correctly wrapped in
Arc<Mutex>for shared access.
1124-1174: Good pattern for handling storage lock scope.The implementation properly limits the storage lock scope by releasing it before processing special messages (CLSig, ISLock), preventing potential deadlocks and improving concurrency.
1489-1499: Address balance queries should be redirected to wallet implementation.The implementation correctly returns an error directing users to query the wallet directly for balance information, maintaining proper separation of concerns.
1775-1778: Good defensive validation of quorum type.The code properly validates the quorum type and logs a warning for invalid types, preventing potential issues downstream.
2800-2803: Good API design for storage access.The storage accessor method properly returns the Arc<Mutex
> wrapper, maintaining thread-safe access patterns.
2544-2545: Fix incorrect use of Version::from_consensus.The Version constructor expects a signed i32 but you're passing an unsigned literal. Use the proper constructor:
- version: Version::from_consensus(536870912), // Version 0x20000000 is common for modern blocks + version: Version::from_consensus(0x20000000i32), // Version 0x20000000 is common for modern blocksLikely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores