refactor: storage manager trait splitted into multiple subtraits #311
Conversation
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
📝 WalkthroughWalkthroughThe PR refactors storage into per-component persistent backends, adds multiple new storage modules (blocks, filters, chainstate, masternode, metadata, transactions), updates SegmentCache APIs, replaces/removes the previous DiskStorageManager/state/headers subsystems, and adjusts consumers/tests and lifecycle shutdown error handling. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/storage/segments.rs (1)
413-413: Segment size violates documented storage architecture: change from 10,000 to 50,000 items per segment is not reflected in guidelines.The coding guideline specifies "Store headers in 10,000-header segments", but the implementation uses
const ITEMS_PER_SEGMENT: u32 = 50_000;(line 413). This 5x increase impacts memory usage, disk I/O patterns, and persistence granularity. Either revert to 10,000 items per segment to match the documented architecture, or update CLAUDE.md to reflect and justify the 50,000-item design decision.
🧹 Nitpick comments (15)
dash-spv/src/client/progress.rs (1)
41-43: Consider logging storage errors for observability.The API change from
Result<Option<u32>>toOption<u32>means storage errors are now silently ignored when retrieving the tip height. While acceptable for stats collection, consider logging storage failures at the debug or trace level to aid diagnostics.🔎 Example with error logging
-if let Some(header_height) = storage.get_tip_height().await { +match storage.get_tip_height().await { + Some(header_height) => { stats.header_height = header_height; + } + None => { + tracing::trace!("Could not retrieve tip height from storage for stats"); + } }dash-spv/src/client/core.rs (1)
192-198: Simplify the double??pattern.The chained
??operators on line 195 are harder to read than necessary. Consider flattening this logic:🔎 Suggested refactor
pub async fn tip_hash(&self) -> Option<dashcore::BlockHash> { let storage = self.storage.lock().await; let tip_height = storage.get_tip_height().await?; - let header = storage.get_header(tip_height).await.ok()??; + let header = storage.get_header(tip_height).await.ok().flatten()?; Some(header.block_hash()) }Alternatively, you could use
and_thenfor clarity:pub async fn tip_hash(&self) -> Option<dashcore::BlockHash> { let storage = self.storage.lock().await; - let tip_height = storage.get_tip_height().await?; - let header = storage.get_header(tip_height).await.ok()??; - - Some(header.block_hash()) + storage + .get_tip_height() + .await + .and_then(|height| storage.get_header(height).await.ok().flatten()) + .map(|header| header.block_hash()) }dash-spv/src/sync/transitions.rs (1)
180-180: Inconsistent error handling betweenget_tip_heightandget_filter_tip_height.
get_tip_height().await.unwrap_or(0)silently defaults to 0 on storage errors, whileget_filter_tip_height()(line 412-416) still maps and propagates errors. This inconsistency could mask storage issues during header sync while surfacing them during filter sync.Consider either:
- Using consistent error handling for both (propagate errors or use defaults)
- Adding a log/trace when defaulting to 0 to aid debugging
🔎 Suggestion to add trace logging
- let start_height = storage.get_tip_height().await.unwrap_or(0); + let start_height = storage.get_tip_height().await.unwrap_or_else(|| { + tracing::trace!("No tip height in storage, starting from height 0"); + 0 + });dash-spv/src/storage/chainstate.rs (2)
74-97: Consider using serde derive for ChainState deserialization.Manual JSON field extraction is verbose and error-prone. If
ChainStatederivesSerializeandDeserialize, you can simplify this significantly and ensure field consistency between store and load.🔎 Suggested simplification
If ChainState derives serde traits:
- let value: serde_json::Value = serde_json::from_str(&content).map_err(|e| { - crate::error::StorageError::Serialization(format!("Failed to parse chain state: {}", e)) - })?; - - let state = ChainState { - last_chainlock_height: value - .get("last_chainlock_height") - .and_then(|v| v.as_u64()) - .map(|h| h as u32), - // ... many more lines - }; + let mut state: ChainState = serde_json::from_str(&content).map_err(|e| { + crate::error::StorageError::Serialization(format!("Failed to parse chain state: {}", e)) + })?; + // Reset runtime-only field + state.masternode_engine = None;
43-61: Manual JSON construction could be replaced with serde serialization.If
ChainStatederivesSerialize, useserde_json::to_string(&state)for consistency and automatic handling of all fields.dash-spv/src/storage/transactions.rs (1)
39-42: Naming may be misleading for non-persistent storage.
PersistentTransactionStoragedoesn't actually persist data (as noted in line 57). Consider renaming toInMemoryTransactionStorageor adding a doc comment explaining the design rationale.🔎 Suggested documentation
+/// In-memory transaction storage for mempool data. +/// +/// Note: Despite the "Persistent" prefix (for trait conformance), this storage +/// intentionally does not persist mempool data to disk, as mempool state is +/// transient and rebuilds from network on restart. pub struct PersistentTransactionStorage { mempool_transactions: HashMap<Txid, UnconfirmedTransaction>, mempool_state: Option<MempoolState>, }dash-spv/src/storage/blocks.rs (1)
117-127:persist_dirtydoes not update the hash-to-height index.If the application crashes after calling
persist_dirtybut before a fullpersist, the index file may be stale on next startup. The fallback inload()(line 94) rebuilds the index from segments, so this is safe but potentially slow on restart.Consider persisting the index in
persist_dirtyas well, or documenting this as intentional behavior.dash-spv/src/sync/headers/manager.rs (4)
143-153: Consider combining tip retrieval into a single storage operation.The current implementation makes two separate async calls to get the tip height and then the header at that height. If storage state changes between calls (e.g., in concurrent scenarios), this could lead to inconsistency. While unlikely in the current architecture, a combined
get_tip_header()method would be more robust.
496-505: Checkpoint header is fetched unconditionally but only used in some branches.The
checkpoint_headeris retrieved at the start of the method but is only used wheneffective_tip_heightisNonewith checkpoint sync, or when at checkpoint height. This adds unnecessary I/O overhead for normal non-checkpoint sync paths.🔎 Proposed refactor: fetch checkpoint header lazily
Move the checkpoint header retrieval inside the branches that actually need it, or use a lazy evaluation pattern to avoid the upfront I/O cost when not needed.
523-541: Redundantget_header(0)calls.
storage.get_header(0)is called at line 523 and again at lines 530-535 for the same purpose. The second call checks if the header exists to decide whether to store it, but the first call already retrieved it.🔎 Proposed fix
- if let Some(genesis_header) = storage.get_header(0).await.map_err(|e| { - SyncError::Storage(format!( - "Error trying to get genesis block from storage: {}", - e - )) - })? { - // Store genesis in storage if not already there - if storage - .get_header(0) - .await - .map_err(|e| { - SyncError::Storage(format!("Failed to check genesis: {}", e)) - })? - .is_none() - { - tracing::info!("Storing genesis block in storage"); - storage.store_headers(&[genesis_header]).await.map_err(|e| { - SyncError::Storage(format!("Failed to store genesis: {}", e)) - })?; - } + let existing_genesis = storage.get_header(0).await.map_err(|e| { + SyncError::Storage(format!( + "Error trying to get genesis block from storage: {}", + e + )) + })?; + + if let Some(genesis_header) = existing_genesis { let genesis_hash = genesis_header.block_hash(); tracing::info!("Starting from genesis block: {}", genesis_hash); Some(genesis_hash)
655-679: Checkpoint header retrieved unconditionally; consider lazy loading.Similar to
prepare_sync, the checkpoint header is fetched at the start of the timeout handling path but is only used whencurrent_tip_heightisNoneand syncing from a checkpoint. For most timeout recovery scenarios (when we have a tip), this I/O is unnecessary.dash-spv/src/storage/mod.rs (4)
45-57: Consider adding documentation clarifying thread-safety expectations.Based on learnings, the
StorageManagertrait pattern with&mut selfmethods combined withSend + Syncbounds can be confusing. The implementations use interior mutability (Arc<RwLock<_>>), so explicit documentation would help clarify the thread-safety model and API design rationale.🔎 Suggested documentation
#[async_trait] +/// Composite trait for SPV storage operations. +/// +/// Implementations use interior mutability (e.g., `Arc<RwLock<_>>`) for thread-safe +/// concurrent access while maintaining the `Send + Sync` bounds required for async contexts. pub trait StorageManager: blocks::BlockHeaderStorage + filters::FilterHeaderStorage
78-87: Synchronous I/O in async context.
std::fs::create_dir_allat line 84 performs blocking I/O. In an async context, this could briefly block the executor. Consider usingtokio::fs::create_dir_allfor consistency with the rest of the async operations.🔎 Proposed fix
pub async fn new(storage_path: impl Into<PathBuf> + Send) -> StorageResult<Self> { - use std::fs; - let storage_path = storage_path.into(); // Create directories if they don't exist - fs::create_dir_all(&storage_path)?; + tokio::fs::create_dir_all(&storage_path).await?;
156-161:stop_workerdoes not clearworker_handleafter aborting.After calling
abort(), theworker_handlefield remainsSome(...)with an aborted handle. This could cause confusion ifstop_workeris called multiple times or if code checksworker_handle.is_some(). Theshutdownmethod correctly usestake().🔎 Proposed fix
/// Stop the background worker without forcing a save. - pub(super) fn stop_worker(&self) { - if let Some(handle) = &self.worker_handle { + pub(super) fn stop_worker(&mut self) { + if let Some(handle) = self.worker_handle.take() { handle.abort(); } }
218-227: Consider logging or propagating errors frompersist().The
persist()method silently ignores all errors. During shutdown, failure to persist data could result in data loss. Consider logging warnings or returning a result to allow callers to handle failures.🔎 Proposed fix: log persist errors
async fn persist(&self) { let storage_path = &self.storage_path; - let _ = self.block_headers.write().await.persist(storage_path).await; - let _ = self.filter_headers.write().await.persist(storage_path).await; - let _ = self.filters.write().await.persist(storage_path).await; - let _ = self.transactions.write().await.persist(storage_path).await; - let _ = self.metadata.write().await.persist(storage_path).await; - let _ = self.chainstate.write().await.persist(storage_path).await; + if let Err(e) = self.block_headers.write().await.persist(storage_path).await { + tracing::error!("Failed to persist block headers on shutdown: {}", e); + } + if let Err(e) = self.filter_headers.write().await.persist(storage_path).await { + tracing::error!("Failed to persist filter headers on shutdown: {}", e); + } + if let Err(e) = self.filters.write().await.persist(storage_path).await { + tracing::error!("Failed to persist filters on shutdown: {}", e); + } + if let Err(e) = self.transactions.write().await.persist(storage_path).await { + tracing::error!("Failed to persist transactions on shutdown: {}", e); + } + if let Err(e) = self.metadata.write().await.persist(storage_path).await { + tracing::error!("Failed to persist metadata on shutdown: {}", e); + } + if let Err(e) = self.chainstate.write().await.persist(storage_path).await { + tracing::error!("Failed to persist chainstate on shutdown: {}", e); + } }
…d command communciation
…ts to write the data
ZocoLini
force-pushed
the
refactor/storage-manager-trait
branch
from
2bc0490 to
e40b953
Compare
github-actions
bot
removed
the
merge-conflict
ZocoLini
force-pushed
the
refactor/storage-manager-trait
branch
from
e40b953 to
0248257
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @dash-spv/src/storage/mod.rs:
- Around line 186-195: persist() currently omits persisting masternodestate; add
a call to persist it the same way as other components by invoking
self.masternodestate.write().await.persist(storage_path).await inside persist(),
and also add the equivalent persist call where start_worker persists components
(i.e., update start_worker to include
self.masternodestate.write().await.persist(storage_path).await) so
masternodestate is saved along with block_headers, filter_headers, filters,
transactions, metadata, and chainstate.
- Around line 150-177: start_worker currently clones and persists block_headers,
filter_headers, filters, transactions, metadata, and chainstate but omits
masternodestate; update start_worker to also clone
Arc::clone(&self.masternodestate) into a local variable (e.g., masternodestate)
before tokio::spawn and add a persist call inside the loop: let _ =
masternodestate.write().await.persist(&storage_path).await, so masternode state
is saved on the same 5s interval and assigned alongside the other cloned
handles.
- Around line 200-239: The clear() method re-initializes all persistent storages
but omits the masternodestate field; update clear() to re-open and assign
self.masternodestate the same way as the other storages (e.g.,
self.masternodestate =
Arc::new(RwLock::new(PersistentMasternodeStateStorage::open(storage_path).await?)));
place this assignment alongside the other storage reinitializations (before
calling self.start_worker().await) so masternode data is reset after clearing.
In @dash-spv/tests/peer_test.rs:
- Around line 192-195: The TempDir is being dropped immediately because you call
.path() inline; instead create a local variable (e.g., storage_temp_dir) that
holds TempDir and pass storage_temp_dir.path() into DiskStorageManager::new so
the TempDir stays in scope for the test lifetime and is cleaned up at the end;
ensure storage_temp_dir lives at least as long as storage_manager (do not shadow
or drop it early).
In @dash-spv/tests/wallet_integration_test.rs:
- Around line 24-27: The TempDir is created inline and dropped immediately,
leaving DiskStorageManager with a dangling path; fix by creating and binding the
TempDir to a variable (e.g., let temp_dir = TempDir::new().expect(...)) that
stays in scope for the whole test, then call
DiskStorageManager::new(temp_dir.path()).await.expect(...) so storage_manager
uses temp_dir.path() while temp_dir remains alive until the test finishes.
♻️ Duplicate comments (3)
dash-spv/src/storage/metadata.rs (1)
52-61: Remove TOCTOU race condition in file existence check.The pattern of checking
path.exists()followed bytokio::fs::read(path)creates a race condition where the file could be deleted or created between the check and read. Instead, attempt the read directly and handle theNotFounderror.🔎 Recommended refactor to eliminate race condition
async fn load_metadata(&self, key: &str) -> StorageResult<Option<Vec<u8>>> { let path = self.storage_path.join(Self::FOLDER_NAME).join(format!("{key}.dat")); - if !path.exists() { - return Ok(None); - } - - let data = tokio::fs::read(path).await?; - Ok(Some(data)) + match tokio::fs::read(path).await { + Ok(data) => Ok(Some(data)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(e.into()), + } }dash-spv/src/storage/chainstate.rs (1)
63-67: Fix blocking I/O and TOCTOU race condition.This code has two issues:
path.exists()is a synchronous filesystem operation that blocks the async runtime- Checking
path.exists()then reading creates a TOCTOU race where the file could be deleted/created between the check and readInstead, read directly and handle the
NotFounderror, or usetokio::fs::try_exists().🔎 Recommended refactor (Option 1: Direct read)
async fn load_chain_state(&self) -> StorageResult<Option<ChainState>> { let path = self.storage_path.join(Self::FOLDER_NAME).join(Self::FILE_NAME); - if !path.exists() { - return Ok(None); - } - - let content = tokio::fs::read_to_string(path).await?; + + let content = match tokio::fs::read_to_string(path).await { + Ok(content) => content, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) => return Err(e.into()), + }; + let value: serde_json::Value = serde_json::from_str(&content).map_err(|e| { crate::error::StorageError::Serialization(format!("Failed to parse chain state: {}", e)) })?;🔎 Alternative refactor (Option 2: tokio::fs::try_exists)
async fn load_chain_state(&self) -> StorageResult<Option<ChainState>> { let path = self.storage_path.join(Self::FOLDER_NAME).join(Self::FILE_NAME); - if !path.exists() { + if !tokio::fs::try_exists(&path).await? { return Ok(None); }Note: Option 1 is preferred as it eliminates the TOCTOU race entirely.
dash-spv/src/storage/mod.rs (1)
164-173: Silent error suppression in background worker may hide persistent failures.This was flagged in a previous review and remains unaddressed. The worker ignores all
persisterrors withlet _ = .... While this prevents the worker from crashing, persistent storage failures would go unnoticed, potentially leading to data loss.
🧹 Nitpick comments (13)
dash-spv/tests/segmented_storage_debug.rs (1)
3-3: Consider removing unused import.
BlockHeaderStorageis imported but doesn't appear to be used in this test. While this may be for consistency with other test files in the broader refactoring, unused imports should be removed unless they serve a specific purpose.#!/bin/bash # Verify if BlockHeaderStorage is actually used in this test file rg -n "BlockHeaderStorage" dash-spv/tests/segmented_storage_debug.rsdash-spv/src/client/block_processor_test.rs (1)
7-7: Consider removing unused import.
BlockHeaderStorageis imported but not explicitly referenced in the test code. Unless it's required for trait bounds or type inference, this import could be removed to keep imports minimal.#!/bin/bash # Check if BlockHeaderStorage is actually needed in this test module rg -n "BlockHeaderStorage" dash-spv/src/client/block_processor_test.rsdash-spv/tests/reverse_index_test.rs (1)
51-54: Store TempDir to ensure proper cleanup.The
TempDiris created inline and immediately dropped after extracting the path. WhileDiskStorageManager::newrecreates the directory, the test loses automatic cleanup. This can leave test artifacts on disk.🔎 Suggested fix
- let mut storage = - DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path()) - .await - .expect("Failed to create tmp storage"); + let temp_dir = TempDir::new().expect("Failed to create tmp dir"); + let mut storage = DiskStorageManager::new(temp_dir.path()) + .await + .expect("Failed to create tmp storage");dash-spv/tests/simple_header_test.rs (1)
54-56: Store TempDir to ensure proper cleanup.The
TempDiris dropped immediately after path extraction, preventing automatic cleanup at test end.🔎 Suggested fix
- let storage = DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path()) - .await - .expect("Failed to create tmp storage"); + let temp_dir = TempDir::new().expect("Failed to create tmp dir"); + let storage = DiskStorageManager::new(temp_dir.path()) + .await + .expect("Failed to create tmp storage");dash-spv/src/storage/blocks.rs (1)
125-142: Potential height tracking issue instore_headers_at_height.The
heightvariable is shadowed (line 130) and then mutated in the loop (line 138), but thestore_items_at_heightcall on line 134 uses the originalheightvalue before the loop increments it. This is correct behavior, but the variable shadowing reduces clarity.Also, the hash computation (line 132) occurs before storage, which is correct for atomicity—if storage fails, the index won't be updated.
🔎 Optional: rename for clarity
async fn store_headers_at_height( &mut self, headers: &[BlockHeader], height: u32, ) -> StorageResult<()> { - let mut height = height; + let mut current_height = height; let hashes = headers.iter().map(|header| header.block_hash()).collect::<Vec<_>>(); self.block_headers.write().await.store_items_at_height(headers, height).await?; for hash in hashes { - self.header_hash_index.insert(hash, height); - height += 1; + self.header_hash_index.insert(hash, current_height); + current_height += 1; } Ok(()) }dash-spv/src/storage/filters.rs (2)
87-89: Read operation uses write lock unnecessarily.Similar to blocks.rs,
load_filter_headersacquires a write lock for a read operation. This limits concurrency when multiple readers could otherwise proceed in parallel.🔎 Consider using read lock if get_items doesn't require mutation
async fn load_filter_headers(&self, range: Range<u32>) -> StorageResult<Vec<FilterHeader>> { - self.filter_headers.write().await.get_items(range).await + self.filter_headers.read().await.get_items(range).await }Note: This requires
get_itemsto take&selfinstead of&mut self. Based on the segments.rs code,get_itemscurrently requires&mut selfdue to LRU cache updates. Consider whether this mutation is strictly necessary for read operations.
138-140: Inconsistent lock usage pattern inload_filters.Same issue as
load_filter_headers—uses write lock for a read operation.dash-spv/src/storage/segments.rs (3)
90-93: Hardcoded string slice indices are fragile.Line 90 uses
name[8..12]to extract the segment ID, assuming a fixed prefix length. IfSEGMENT_PREFIXchanges, this will silently break.🔎 Use computed indices based on prefix length
- let segment_id = match name[8..12].parse::<u32>() { + let prefix_len = BlockHeader::SEGMENT_PREFIX.len(); + let segment_id = match name[prefix_len + 1..prefix_len + 5].parse::<u32>() { Ok(id) => id, Err(_) => continue, };Note: This mirrors the approach used in
load_or_newat lines 134-135.
336-352: Persist errors are logged but not propagated.The
persistmethod logs errors but continues silently, making the return type()somewhat misleading. While this prevents cascading failures, callers have no way to know if persistence succeeded.Consider either:
- Returning
StorageResult<()>and propagating errors- Collecting errors and returning a summary
- Documenting that this is intentional fire-and-forget behavior
472-474: Potential panic on missing parent directory.Line 472 uses
.unwrap()onpath.parent(). While this should never fail for properly constructed paths, a defensive check would be safer.🔎 Add defensive check
- if let Err(e) = fs::create_dir_all(path.parent().unwrap()) { + let parent = path.parent().ok_or_else(|| { + StorageError::WriteFailed("Invalid segment path: no parent directory".to_string()) + })?; + if let Err(e) = fs::create_dir_all(parent) { return Err(StorageError::WriteFailed(format!("Failed to persist segment: {}", e))); }dash-spv/src/storage/transactions.rs (2)
36-57: Naming mismatch: "Persistent" storage doesn't persist.
PersistentTransactionStorageimplementsPersistentStoragebut thepersistmethod is a no-op (line 53-55). The struct is entirely in-memory and data is lost on restart. This naming is misleading.Consider either:
- Renaming to
InMemoryTransactionStorageorVolatileTransactionStorage- Adding actual persistence (e.g., serialize mempool state to disk)
- Adding documentation explaining this is intentionally transient
🔎 Add clarifying documentation
+/// In-memory transaction storage for mempool data. +/// +/// Note: Despite implementing `PersistentStorage`, this storage is intentionally +/// volatile. Mempool transactions are transient by nature and are not persisted +/// to disk. On restart, the mempool starts empty. pub struct PersistentTransactionStorage { mempool_transactions: HashMap<Txid, UnconfirmedTransaction>, mempool_state: Option<MempoolState>, }
82-86: Cloning entire HashMap on every read may impact performance.
get_all_mempool_transactionsclones the entireHashMap<Txid, UnconfirmedTransaction>. For large mempools, this could be expensive. Consider whether callers actually need ownership or if an iterator/reference would suffice.This is acceptable for now if mempool sizes are expected to remain small, but worth noting for future optimization.
dash-spv/src/storage/mod.rs (1)
179-184:stop_workerdoesn't clearworker_handle.After aborting the worker,
worker_handleremainsSome(...)with an aborted handle. This could cause issues ifstart_workeris called again (it will overwrite without checking), andstop_workerwould try to abort an already-aborted handle.🔎 Proposed fix
- fn stop_worker(&self) { + fn stop_worker(&mut self) { if let Some(handle) = &self.worker_handle { handle.abort(); } + self.worker_handle = None; }Note: This requires changing
&selfto&mut self, which may have signature implications.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
dash-spv/benches/storage.rsdash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/block_processor_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/lib.rsdash-spv/src/storage/blocks.rsdash-spv/src/storage/chainstate.rsdash-spv/src/storage/filters.rsdash-spv/src/storage/headers.rsdash-spv/src/storage/manager.rsdash-spv/src/storage/masternode.rsdash-spv/src/storage/metadata.rsdash-spv/src/storage/mod.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/state.rsdash-spv/src/storage/transactions.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/storage_test.rsdash-spv/tests/wallet_integration_test.rs
💤 Files with no reviewable changes (3)
- dash-spv/src/storage/manager.rs
- dash-spv/src/storage/state.rs
- dash-spv/src/storage/headers.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- dash-spv/src/client/lifecycle.rs
- dash-spv/tests/storage_consistency_test.rs
- dash-spv/tests/filter_header_verification_test.rs
- dash-spv/src/storage/masternode.rs
🧰 Additional context used
📓 Path-based instructions (8)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/benches/storage.rsdash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/src/storage/metadata.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/benches/storage.rsdash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/src/storage/metadata.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
dash-spv/src/client/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Files:
dash-spv/src/client/block_processor_test.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/storage_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/tests/**/*.rs: Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
**/*integration*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write integration tests for network operations
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/integration_real_node_test.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/metadata.rsdash-spv/src/storage/chainstate.rsdash-spv/src/storage/blocks.rsdash-spv/src/storage/filters.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/transactions.rsdash-spv/src/storage/mod.rs
🧠 Learnings (36)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Follow a layered, trait-based architecture with clear separation of concerns across modules: client/, network/, storage/, sync/, validation/, wallet/, with trait-based abstractions for swappable implementations
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/benches/storage.rsdash-spv/src/client/block_processor_test.rsdash-spv/tests/rollback_test.rsdash-spv/src/storage/metadata.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/benches/storage.rsdash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/src/storage/metadata.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/benches/storage.rsdash-spv/src/client/block_processor_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/benches/storage.rsdash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/src/storage/metadata.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/src/storage/metadata.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/header_sync_test.rsdash-spv/examples/simple_sync.rsdash-spv/tests/storage_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Cover critical parsing, networking, SPV, and wallet flows in tests; add regression tests for fixes; consider property tests with `proptest`
Applied to files:
dash-spv/src/client/block_processor_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/storage_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/block_processor_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/header_sync_test.rsdash-spv/src/storage/filters.rsdash-spv/src/storage/transactions.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash-spv/src/client/block_processor_test.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*integration*.rs : Write integration tests for network operations
Applied to files:
dash-spv/src/client/block_processor_test.rsdash-spv/tests/storage_test.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/blocks.rsdash-spv/src/storage/transactions.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Applied to files:
dash-spv/tests/wallet_integration_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv/tests/rollback_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/storage/metadata.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/filter_sync.rsdash-spv/src/storage/chainstate.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
dash-spv/examples/spv_with_wallet.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
dash-spv/examples/spv_with_wallet.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/examples/spv_with_wallet.rsdash-spv/src/lib.rsdash-spv/tests/simple_segmented_test.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/segments.rsdash-spv/tests/peer_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/examples/spv_with_wallet.rsdash-spv/src/lib.rsdash-spv/examples/filter_sync.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/storage/chainstate.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/src/storage/blocks.rsdash-spv/examples/simple_sync.rsdash-spv/src/storage/filters.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/transactions.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Follow a layered, trait-based architecture with clear separation of concerns across modules: client/, network/, storage/, sync/, validation/, wallet/, with trait-based abstractions for swappable implementations
Applied to files:
dash-spv/tests/edge_case_filter_sync_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Use trait-based abstractions (NetworkManager, StorageManager) for swappable implementations instead of concrete types
Applied to files:
dash-spv/tests/edge_case_filter_sync_test.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
dash-spv/src/lib.rsdash-spv/src/storage/filters.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
Applied to files:
dash-spv/src/storage/chainstate.rsdash-spv/src/storage/transactions.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Prefer `async`/`await` via `tokio` for asynchronous operations
Applied to files:
dash-spv/src/storage/chainstate.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Use tokio runtime for async operations in Rust
Applied to files:
dash-spv/src/storage/chainstate.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
Applied to files:
dash-spv/src/storage/chainstate.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to src/**/*.rs : Avoid `unwrap()` and `expect()` in library code; use proper error types (e.g., `thiserror`)
Applied to files:
dash-spv/src/storage/chainstate.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Tokio channels for inter-component message passing between async tasks
Applied to files:
dash-spv/src/storage/chainstate.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : Implement transaction classification and routing through `TransactionRouter` to avoid checking all accounts for every transaction
Applied to files:
dash-spv/src/storage/transactions.rs
🧬 Code graph analysis (20)
dash-spv/src/client/block_processor_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(163-165)
dash-spv/tests/wallet_integration_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/storage/metadata.rs (5)
dash-spv/src/storage/io.rs (1)
atomic_write(24-57)dash-spv/src/storage/mod.rs (5)
store_metadata(355-357)load_metadata(359-361)open(51-51)persist(53-53)persist(186-195)dash-spv/src/storage/blocks.rs (2)
open(74-101)persist(103-115)dash-spv/src/storage/chainstate.rs (2)
open(29-33)persist(35-38)dash-spv/src/storage/filters.rs (4)
open(60-69)open(110-119)persist(71-78)persist(121-129)
dash-spv/examples/spv_with_wallet.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/edge_case_filter_sync_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(163-165)
dash-spv/examples/filter_sync.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/reverse_index_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/segmented_storage_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(163-165)
dash-spv/tests/header_sync_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/simple_segmented_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(163-165)
dash-spv/tests/segmented_storage_debug.rs (1)
dash-spv/src/client/core.rs (1)
storage(163-165)
dash-spv/examples/simple_sync.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/storage/filters.rs (2)
dash-spv/src/storage/mod.rs (9)
store_filter_headers(289-291)load_filter_headers(293-295)get_filter_tip_height(297-299)get_filter_start_height(301-303)store_filter(308-310)load_filters(312-314)new(97-140)persist(53-53)persist(186-195)dash-spv/src/storage/segments.rs (6)
tip_height(355-357)start_height(360-362)load_or_new(113-163)new(392-402)persist(336-352)persist(464-488)
dash-spv/tests/storage_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/storage/segments.rs (2)
dash-spv/src/storage/blocks.rs (1)
persist(103-115)dash-spv/src/storage/filters.rs (2)
persist(71-78)persist(121-129)
dash-spv/tests/peer_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/storage/transactions.rs (1)
dash-spv/src/storage/mod.rs (10)
store_mempool_transaction(319-325)remove_mempool_transaction(327-329)get_mempool_transaction(331-336)get_all_mempool_transactions(338-342)store_mempool_state(344-346)load_mempool_state(348-350)open(51-51)new(97-140)persist(53-53)persist(186-195)
dash-spv/tests/integration_real_node_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/simple_header_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/storage/mod.rs (6)
dash-spv/src/storage/blocks.rs (13)
open(74-101)persist(103-115)store_headers(20-20)store_headers(120-123)headers(132-132)store_headers_at_height(22-26)store_headers_at_height(125-142)load_headers(28-28)load_headers(144-146)get_tip_height(50-50)get_tip_height(148-150)get_start_height(52-52)get_start_height(152-154)dash-spv/src/storage/chainstate.rs (6)
open(29-33)persist(35-38)store_chain_state(13-13)store_chain_state(43-61)load_chain_state(15-15)load_chain_state(63-100)dash-spv/src/storage/filters.rs (14)
open(60-69)open(110-119)persist(71-78)persist(121-129)store_filter_headers(14-14)store_filter_headers(83-85)load_filter_headers(16-16)load_filter_headers(87-89)get_filter_tip_height(38-38)get_filter_tip_height(91-93)store_filter(45-45)store_filter(134-136)load_filters(47-47)load_filters(138-140)dash-spv/src/storage/masternode.rs (6)
open(28-32)persist(34-37)store_masternode_state(12-12)store_masternode_state(42-57)load_masternode_state(14-14)load_masternode_state(59-75)dash-spv/src/storage/metadata.rs (6)
open(27-31)persist(33-36)store_metadata(12-12)store_metadata(41-50)load_metadata(14-14)load_metadata(52-61)dash-spv/src/storage/transactions.rs (14)
open(43-51)persist(53-56)store_mempool_transaction(14-18)store_mempool_transaction(61-68)remove_mempool_transaction(20-20)remove_mempool_transaction(70-73)get_mempool_transaction(22-25)get_mempool_transaction(75-80)get_all_mempool_transactions(27-29)get_all_mempool_transactions(82-86)store_mempool_state(31-31)store_mempool_state(88-91)load_mempool_state(33-33)load_mempool_state(93-95)
⏰ 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). (4)
- GitHub Check: Windows / core
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Thread Sanitizer
… other storages do
ZocoLini
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @dash-spv/src/storage/mod.rs:
- Around line 188-198: The persist method currently swallows all errors from
each component (block_headers, filter_headers, filters, transactions, metadata,
chainstate, masternodestate) by assigning to `_`, leaving callers unaware of
failures; change persist to return a Result (e.g., async fn persist(&self) ->
Result<(), ErrorType>) or at minimum log failures and propagate the
first/combined error: call .persist(storage_path).await on each write guard,
check the Result, log via the existing logger or collect errors into a Vec and
return an Err if any persistence failed, and update callers to handle the new
Result; keep references to the same members (self.block_headers,
self.filter_headers, self.filters, self.transactions, self.metadata,
self.chainstate, self.masternodestate) when implementing this change.
- Around line 317-319: The method load_filters currently takes &self but
acquires a write lock via self.filters.write().await which is unnecessary and
blocks readers; change it to acquire a read lock instead (use
self.filters.read().await) so the read-only call to load_filters(range).await
uses a shared lock and doesn't block concurrent readers; update the call in the
async fn load_filters to use the read lock on self.filters.
- Around line 298-300: The method load_filter_headers is a read-only operation
but currently acquires a write lock; change the lock from
self.filter_headers.write().await to self.filter_headers.read().await so it uses
a read lock and calls load_filter_headers(range).await on the read guard,
preserving the same return type and behavior in the async fn
load_filter_headers(&self, range: Range<u32>) ->
StorageResult<Vec<FilterHeader>>.
- Around line 47-54: Update the PersistentStorage trait docs to state the
thread-safety expectations and persistence semantics: document that
implementations of PersistentStorage (trait and its async methods open and
persist) are expected to be Send + Sync (even if they use interior mutability
such as Arc<RwLock<...>> as in DiskStorageManager), explain why the trait uses
&mut self (to express logical mutability while allowing interior mutability for
concurrent access), and specify expected persist guarantees (whether persist
should be atomic, how partial failures should be handled/retried, and what
callers can assume about durability and consistency). Ensure the documentation
references the PersistentStorage trait and its open and persist methods and
mentions DiskStorageManager and the common Arc<RwLock<...>> pattern as an
example.
- Around line 56-74: Add comprehensive doc comments to the StorageManager trait
explaining why it composes the specialized traits (BlockHeaderStorage,
FilterHeaderStorage, FilterStorage, TransactionStorage, MetadataStorage,
ChainStateStorage, MasternodeStateStorage), the intended usage as a facade
versus holding separate trait objects, and the thread-safety guarantees expected
from implementations (clarify that implementations must be Send + Sync and
typically use interior mutability to permit methods that take &self and &mut
self to coexist). Also document the semantics of clear() and shutdown() (what
state they affect, whether they block background tasks, and expected error
handling) and explicitly note how mixed mutability methods should be implemented
or wrapped to avoid deadlocks or surprising behavior.
- Around line 268-270: The load_headers method is using a write lock
unnecessarily for a read-only operation; change the lock on the block_headers
field from write().await to read().await so the method calls
self.block_headers.read().await.load_headers(range).await, preserving the async
call chain and return type while allowing concurrent readers.
♻️ Duplicate comments (1)
dash-spv/src/storage/mod.rs (1)
162-176: Silent error suppression in background worker may hide persistent failures.The worker ignores all persist errors with
let _ = .... While this prevents the worker from crashing, persistent storage failures would go unnoticed. Consider logging errors or accumulating them for health checks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/storage/mod.rs
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/storage/mod.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/mod.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/src/storage/mod.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Follow a layered, trait-based architecture with clear separation of concerns across modules: client/, network/, storage/, sync/, validation/, wallet/, with trait-based abstractions for swappable implementations
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/storage/mod.rs
⏰ 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). (19)
- GitHub Check: Windows / ffi
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: Thread Sanitizer
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: Address Sanitizer
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: conflict-check
🔇 Additional comments (5)
dash-spv/src/storage/mod.rs (5)
1-45: Module structure aligns well with the refactoring objectives.The split into specialized storage modules (blocks, chainstate, filters, masternode, metadata, transactions) with corresponding public trait exports provides clear separation of concerns and enables SDK users to provide custom implementations for individual storage systems.
76-140: Well-structured composite storage manager.The architecture correctly uses
Arc<RwLock<...>>for interior mutability with async operations, includes a lock file to prevent concurrent access to the same storage path, and properly initializes all storage components. The automatic background worker startup ensures periodic persistence without manual intervention.
203-244: Clear method correctly handles all storage components.The method properly stops the worker, removes all files with retry logic, re-initializes all storage components (including
masternodestate, which was previously missing), and restarts the worker. The retry mechanism for directory removal handles potential timing issues well.
246-252: Shutdown correctly persists all data before stopping.The method appropriately stops the background worker first, then explicitly persists all components to ensure no data loss.
391-566: Comprehensive test coverage.The tests cover critical scenarios: basic header storage/retrieval, checkpoint-based indexing with blockchain heights, hash-to-height reverse indexing, index rebuilding after reload, and shutdown flush behavior. The tests use appropriate async patterns and
tempfilefor isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
dash-spv/src/storage/mod.rs (4)
47-54: Add documentation about thread-safety and persistence semantics.The trait methods use
&mut selfbut implementations use interior mutability (as seen withArc<RwLock<...>>inDiskStorageManager). The lack of documentation about thread-safety expectations and persistence behavior (atomicity, partial failure handling) can cause confusion.Based on learnings, this pattern has been problematic in the past.
🔎 Suggested documentation additions
#[async_trait] pub trait PersistentStorage: Sized { + /// Opens or creates persistent storage at the given path. + /// + /// # Thread Safety + /// Implementations must be `Send + Sync` and typically use interior mutability + /// (e.g., `Arc<RwLock<...>>`) to allow concurrent access. The `&mut self` signature + /// expresses logical mutability while allowing interior mutability patterns. + /// + /// # Errors + /// Returns an error if the storage path cannot be accessed or initialized. async fn open(storage_path: impl Into<PathBuf> + Send) -> StorageResult<Self>; + /// Persists dirty state to disk. + /// + /// # Persistence Guarantees + /// Implementations should strive for atomic writes. Partial failures should be + /// handled gracefully, and callers can assume durability after a successful return. + /// + /// # Errors + /// Returns an error if the write operation fails. async fn persist(&mut self, storage_path: impl Into<PathBuf> + Send) -> StorageResult<()>; }
56-74: Add comprehensive documentation about the trait composition and usage patterns.The trait composes multiple specialized storage traits but lacks documentation about:
- The rationale for this composition pattern
- Thread-safety expectations (all implementations must be
Send + Sync)- Usage patterns (using
StorageManageras a facade vs. holding separate specialized trait objects)- Semantics of
clear()andshutdown()(state affected, blocking behavior, error handling)Based on learnings, documenting the mixed mutability pattern is important to avoid confusion.
🔎 Suggested documentation additions
#[async_trait] +/// A unified storage manager that composes specialized storage traits. +/// +/// # Design Rationale +/// This trait aggregates multiple specialized storage traits (BlockHeaderStorage, +/// FilterHeaderStorage, etc.) to support both facade usage (via `DiskStorageManager`) +/// and separate trait object usage (consumers can hold individual storage traits). +/// +/// # Thread Safety +/// Implementations must be `Send + Sync + 'static` and typically use interior mutability +/// to allow concurrent access. Methods may use either `&self` or `&mut self` depending +/// on the operation; implementations should use locks appropriately to avoid deadlocks. +/// +/// # Usage Patterns +/// - **Facade pattern**: Use `DiskStorageManager` which implements all traits +/// - **Separate fields**: SDK users can provide custom implementations for each trait pub trait StorageManager: BlockHeaderStorage + FilterHeaderStorage + FilterStorage + TransactionStorage + MetadataStorage + ChainStateStorage + MasternodeStateStorage + Send + Sync + 'static { - /// Deletes in-disk and in-memory data + /// Deletes all persisted data and resets in-memory state. + /// + /// This method stops background workers, removes all storage files, + /// and re-initializes storage components. async fn clear(&mut self) -> StorageResult<()>; - /// Stops all background tasks and persists the data. + /// Gracefully shuts down storage, stopping background workers and persisting data. + /// + /// Errors during persist are silently suppressed (see persist() implementation). async fn shutdown(&mut self); }
162-176: Silent error suppression in background worker may hide persistent failures.The worker ignores all
persisterrors withlet _ = .... While this prevents the worker from crashing, persistent storage failures would go unnoticed. Consider logging errors so operators can detect and respond to storage issues.🔎 Proposed fix: log persist errors
loop { ticker.tick().await; - let _ = block_headers.write().await.persist(&storage_path).await; - let _ = filter_headers.write().await.persist(&storage_path).await; - let _ = filters.write().await.persist(&storage_path).await; - let _ = transactions.write().await.persist(&storage_path).await; - let _ = metadata.write().await.persist(&storage_path).await; - let _ = chainstate.write().await.persist(&storage_path).await; - let _ = masternodestate.write().await.persist(&storage_path).await; + if let Err(e) = block_headers.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist block headers: {}", e); + } + if let Err(e) = filter_headers.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist filter headers: {}", e); + } + if let Err(e) = filters.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist filters: {}", e); + } + if let Err(e) = transactions.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist transactions: {}", e); + } + if let Err(e) = metadata.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist metadata: {}", e); + } + if let Err(e) = chainstate.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist chainstate: {}", e); + } + if let Err(e) = masternodestate.write().await.persist(&storage_path).await { + tracing::warn!("Failed to persist masternode state: {}", e); + } }
188-198: Persist errors are silently suppressed.All persist errors are ignored with
let _ = .... If explicit persistence fails (e.g., during shutdown), the caller has no way to detect or handle the failure. Consider returning aResultor at minimum logging errors so operators can detect storage issues.🔎 Suggested improvements
Option 1: Return a Result and propagate errors:
- async fn persist(&self) { + async fn persist(&self) -> StorageResult<()> { let storage_path = &self.storage_path; - let _ = self.block_headers.write().await.persist(storage_path).await; - let _ = self.filter_headers.write().await.persist(storage_path).await; - let _ = self.filters.write().await.persist(storage_path).await; - let _ = self.transactions.write().await.persist(storage_path).await; - let _ = self.metadata.write().await.persist(storage_path).await; - let _ = self.chainstate.write().await.persist(storage_path).await; - let _ = self.masternodestate.write().await.persist(storage_path).await; + self.block_headers.write().await.persist(storage_path).await?; + self.filter_headers.write().await.persist(storage_path).await?; + self.filters.write().await.persist(storage_path).await?; + self.transactions.write().await.persist(storage_path).await?; + self.metadata.write().await.persist(storage_path).await?; + self.chainstate.write().await.persist(storage_path).await?; + self.masternodestate.write().await.persist(storage_path).await?; + Ok(()) }Option 2: Log errors without changing signature:
async fn persist(&self) { let storage_path = &self.storage_path; - let _ = self.block_headers.write().await.persist(storage_path).await; + if let Err(e) = self.block_headers.write().await.persist(storage_path).await { + tracing::error!("Failed to persist block headers: {}", e); + } // ... similar for other components }
🧹 Nitpick comments (1)
dash-spv/src/storage/mod.rs (1)
96-140: Consider using async filesystem operations.Line 107 uses
std::fs::create_dir_allwhich is a blocking operation in an async context. For consistency with the rest of the codebase and to avoid blocking the tokio runtime, consider usingtokio::fs::create_dir_all.🔎 Proposed fix
pub async fn new(storage_path: impl Into<PathBuf> + Send) -> StorageResult<Self> { - use std::fs; - let storage_path = storage_path.into(); let lock_file = { let mut lock_file = storage_path.clone(); lock_file.set_extension("lock"); lock_file }; - fs::create_dir_all(&storage_path)?; + tokio::fs::create_dir_all(&storage_path).await?; let lock_file = LockFile::new(lock_file)?;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/storage/mod.rs
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/storage/mod.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/mod.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/src/storage/mod.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Follow a layered, trait-based architecture with clear separation of concerns across modules: client/, network/, storage/, sync/, validation/, wallet/, with trait-based abstractions for swappable implementations
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: When adding new features, define traits for abstractions, implement concrete types following existing patterns, add comprehensive unit tests, add integration tests for network interaction, and update error types in error.rs
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
Applied to files:
dash-spv/src/storage/mod.rs
⏰ 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). (16)
- GitHub Check: Windows / core
- GitHub Check: Ubuntu / core
- GitHub Check: Windows / spv
- GitHub Check: Ubuntu ARM / core
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Address Sanitizer
- GitHub Check: Thread Sanitizer
- GitHub Check: conflict-check
🔇 Additional comments (4)
dash-spv/src/storage/mod.rs (4)
203-244: LGTM - clear() implementation is complete.The
clear()method now properly handles all storage components includingmasternodestate(lines 237-238), addressing the previous review comment. The retry logic for directory removal and the re-initialization of all components is well-implemented.
254-290: LGTM - proper lock usage.All methods correctly use read locks for read-only operations and write locks for mutations, addressing the previous review comments about unnecessary write locks.
292-309: LGTM - proper lock usage.All methods correctly use read locks for read-only operations (
load_filter_headers,get_filter_tip_height,get_filter_start_height), addressing the previous review comment about unnecessary write locks.
311-320: LGTM - proper lock usage.The
load_filtersmethod correctly uses a read lock (line 318), addressing the previous review comment about unnecessary write locks.
xdustinface
changed the title
The idea of this PR is to split responsibilities when it comes to the storage system. Instead of a single large trait handling all storage operations, we would have multiple traits, each one specialized in a specific system (BlockHeaders, Filters, etc.).
The reason for this change is the need to refactor how peer reputation is stored, along with other systems that we may eventually need. This approach makes the code more scalable and testable, following good design patterns.
To make this change as smooth as possible, I preserve the
StorageManagertrait implementing all the new specialized traits, this wayDiskStorageManagercan still be used as a facade for all the specialized storage structs.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.