feat: Add atomic_write for atomic file writing#245
Conversation
WalkthroughA centralized crate-scoped atomic write helper was added and used across network and storage modules to replace direct async file writes; implementations now write to temp files, sync, and atomically rename to targets. Loader for filter headers and several tests were also added. Changes
Sequence DiagramsequenceDiagram
participant Caller as Module (persist/state/filters)
participant Atomic as atomic_write()
participant FS as Filesystem
participant Temp as TempFile
Caller->>Atomic: atomic_write(path, data)
activate Atomic
Atomic->>FS: ensure parent directories exist
Atomic->>Temp: create & write temp file
activate Temp
Temp-->>Atomic: write complete
deactivate Temp
Atomic->>Temp: fsync temp (durable)
activate Temp
Temp-->>Atomic: fsync complete
deactivate Temp
Atomic->>FS: rename temp -> target (atomic)
FS-->>Atomic: rename complete
Atomic-->>Caller: StorageResult<()>
deactivate Atomic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
xdustinface
force-pushed
the
feat/atomic-writes
branch
from
443b9a4 to
cc646d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
dash-spv/src/network/reputation.rs (1)
15-16: Atomic write usage is correct; consider passing the underlying error instead of just its string.The switch to
atomic_writehere looks sound and aligns with the rest of the crate’s persistence strategy. Two small nits:
- You can preserve the underlying
StorageErroras the cause instead of only its string by doingstd::io::Error::other(e)(assumingStorageError: std::error::Error), which keeps more context for callers and logs.- Minor layering note:
network::reputationnow depends directly onstorage::disk::io. If you care about keeping network/storage decoupled, consider movingatomic_writeinto a more generic crate‑internal I/O module later.Also applies to: 428-430
dash-spv/src/network/persist.rs (1)
7-9: Peer store write path correctly switched to atomic_write.The new
atomic_writeusage is correct and keeps error handling consistent (StorageErrorwrapped inSpvError::Storage). It also guarantees parent directory creation for the peers file, which is an improvement over a rawtokio::fs::write.Also applies to: 62-66
dash-spv/src/storage/disk/mod.rs (1)
22-25: Makingiopub(crate) is appropriate for sharing atomic I/O utilities.Exposing
ioaspub(crate)is a reasonable way to shareatomic_writeand related helpers across storage and network modules without widening the public API surface.dash-spv/src/storage/disk/state.rs (1)
98-109: Atomic writes for JSON, checkpoints, chainlocks, and metadata look consistent.Aside from the
store_chain_statelifetime bug above, the other conversions toatomic_writehere are consistent:
store_masternode_state,store_sync_stateserialize into aStringand then passjson.as_bytes().store_sync_checkpointandstore_chain_lockbuild deterministic filenames undercheckpoints/andchainlocks/;atomic_writehandles directory creation.store_metadatawrites arbitrary binary blobs tostate/{key}.dat.These preserve the original behavior while adding crash‑safe, atomic persistence.
Also applies to: 130-146, 185-203, 248-265, 326-331
dash-spv/src/storage/disk/io.rs (1)
18-30: Atomic I/O helper and its use in save_ functions look solid; consider handling rename failures too.*The new
get_temp_path+atomic_writeimplementation is well thought out:
- Unique temp names per target file via
(file_name, pid, counter).- Parent directory creation, full write,
sync_all, then atomic rename give strong crash‑safety guarantees.- The updated
save_segment_to_disk,save_filter_segment_to_disk, andsave_index_to_diskcorrectly build in‑memory buffers and delegate the actual write toatomic_write, keeping the encoding logic separate from I/O.One optional improvement: if
tokio::fs::renamefails, you currently return aWriteFailedbut leave the temp file behind. It’s rare, but you might want to best‑effortremove_file(&temp_path)in that error path too to avoid orphaned.tmpfiles.Also applies to: 32-68, 151-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
dash-spv/src/network/persist.rs(2 hunks)dash-spv/src/network/reputation.rs(2 hunks)dash-spv/src/storage/disk/filters.rs(2 hunks)dash-spv/src/storage/disk/io.rs(3 hunks)dash-spv/src/storage/disk/mod.rs(1 hunks)dash-spv/src/storage/disk/state.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use trait-based abstractions for core components likeNetworkManagerandStorageManagerto enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles withcargo check --all-features
Files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Storage module should provide abstraction via
StorageManagertrait with concrete implementations forMemoryStorageManagerandDiskStorageManager
Files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/disk/filters.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Network module should handle TCP connections, handshake management, message routing, and peer management following Dash protocol implementation
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/persist.rs
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
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-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`
Applied to files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations
Applied to files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Applied to files:
dash-spv/src/storage/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.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/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Applied to files:
dash-spv/src/storage/disk/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/disk/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.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/storage/disk/mod.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/client/**/*.rs : Client module should provide high-level API through `DashSpvClient` and configuration via `ClientConfig`
Applied to files:
dash-spv/src/storage/disk/mod.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with `cargo check --all-features`
Applied to files:
dash-spv/src/storage/disk/mod.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Keep Rust modules focused and organized
Applied to files:
dash-spv/src/storage/disk/mod.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rsdash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/error.rs : Maintain domain-specific error types in `error.rs` with variants for `NetworkError`, `StorageError`, `SyncError`, `ValidationError`, and `SpvError` (top-level wrapper)
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/network/persist.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/network/reputation.rsdash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-06-26T15:51:03.126Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:51:03.126Z
Learning: In Rust network code, using unwrap() on SystemTime::now().duration_since(UNIX_EPOCH) can panic if the system clock is before UNIX_EPOCH. Use unwrap_or with a safe fallback (e.g., Duration::from_secs(0)) to prevent panics in edge cases.
Applied to files:
dash-spv/src/network/reputation.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should be organized in `tests/` directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Write integration tests for network operations in Rust
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Applied to files:
dash-spv/src/storage/disk/state.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Wallet module should provide UTXO tracking via `Utxo` struct, balance calculation with confirmation states, and transaction processing via `TransactionProcessor`
Applied to files:
dash-spv/src/storage/disk/state.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true
Applied to files:
dash-spv/src/storage/disk/state.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
dash-spv/src/storage/disk/state.rs
🧬 Code graph analysis (4)
dash-spv/src/network/reputation.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-68)
dash-spv/src/storage/disk/state.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-68)
dash-spv/src/network/persist.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-68)
dash-spv/src/storage/disk/filters.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-68)
🔇 Additional comments (1)
dash-spv/src/storage/disk/filters.rs (1)
10-11: Filter persistence now benefits from atomic writes.The change to use
atomic_writeforstore_filteris correct: it handles parent directory creation and gives crash‑safe writes while keeping the API and error type unchanged.Also applies to: 186-190
xdustinface
force-pushed
the
feat/atomic-writes
branch
from
cc646d4 to
e7d51a4
Compare
Introduces atomic file writes for all SPV storage operations with a tempfile + rename pattern to ensure files dont get corrupted if a crash happens during write. - Adds `atomic_write` function - Updates all file writes: headers, filter headers, index, chain state, sync state, checkpoints, chainlocks, metadata, peers, peer-reputation
xdustinface
force-pushed
the
feat/atomic-writes
branch
from
e7d51a4 to
d44483a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dash-spv/src/network/reputation.rs (1)
428-430: Error mapping viastd::io::Error::otherhides underlying I/O error kindsSwapping to:
let json = serde_json::to_string_pretty(&data)?; atomic_write(path, json.as_bytes()).await.map_err(std::io::Error::other)changes the error surface: all failures from
atomic_writenow appear asErrorKind::Other, whereas a directtokio::fs::writewould have preserved OS error kinds (e.g.,PermissionDenied,NotFound). If any caller branches onio::ErrorKind, this is a silent behavioral change.If you want to keep richer error information and stay consistent with the rest of the crate’s storage code, consider returning
StorageResult<()>here and propagatingStorageErrordirectly, similar to other storage helpers, e.g.:- pub async fn save_to_storage(&self, path: &std::path::Path) -> std::io::Result<()> { + pub async fn save_to_storage( + &self, + path: &std::path::Path, + ) -> crate::error::StorageResult<()> { … - let json = serde_json::to_string_pretty(&data)?; - atomic_write(path, json.as_bytes()).await.map_err(std::io::Error::other) + let json = serde_json::to_string_pretty(&data) + .map_err(|e| crate::error::StorageError::Serialization(e.to_string()))?; + atomic_write(path, json.as_bytes()).await }(Adjust imports as needed.)
If you prefer to keep
std::io::Result, you may want to at least wrap the underlyingStorageErroras the error’s source so callers can still introspect it.dash-spv/src/storage/disk/io.rs (1)
157-174: Buffer-then-atomic-write pattern for segments and index is reasonableFor
save_segment_to_disk,save_filter_segment_to_disk, andsave_index_to_diskyou now:
- Encode all headers / filter headers / index entries into an in‑memory
Vec<u8>.- Delegate the actual write to
atomic_write.Given segment sizes and index size in this codebase, the extra buffering is acceptable, and this keeps the atomic write primitive focused on raw bytes instead of streaming encoders. If segment sizes ever grow significantly, you might later consider a streaming variant of
atomic_write(taking a closure that writes into a tempFile) to avoid holding very large buffers, but that’s not necessary right now.Also applies to: 177-190, 193-201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
dash-spv/src/network/persist.rs(2 hunks)dash-spv/src/network/reputation.rs(2 hunks)dash-spv/src/storage/disk/filters.rs(2 hunks)dash-spv/src/storage/disk/io.rs(3 hunks)dash-spv/src/storage/disk/mod.rs(1 hunks)dash-spv/src/storage/disk/state.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/src/storage/disk/mod.rs
- dash-spv/src/storage/disk/state.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use trait-based abstractions for core components likeNetworkManagerandStorageManagerto enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles withcargo check --all-features
Files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Network module should handle TCP connections, handshake management, message routing, and peer management following Dash protocol implementation
Files:
dash-spv/src/network/reputation.rsdash-spv/src/network/persist.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Storage module should provide abstraction via
StorageManagertrait with concrete implementations forMemoryStorageManagerandDiskStorageManager
Files:
dash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rs
🧠 Learnings (19)
📓 Common learnings
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-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
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.
📚 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/network/reputation.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.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/network/reputation.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.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/network/reputation.rsdash-spv/src/storage/disk/io.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/error.rs : Maintain domain-specific error types in `error.rs` with variants for `NetworkError`, `StorageError`, `SyncError`, `ValidationError`, and `SpvError` (top-level wrapper)
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/network/persist.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Write integration tests for network operations in Rust
Applied to files:
dash-spv/src/network/reputation.rsdash-spv/src/storage/disk/io.rs
📚 Learning: 2025-06-26T15:51:03.126Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:51:03.126Z
Learning: In Rust network code, using unwrap() on SystemTime::now().duration_since(UNIX_EPOCH) can panic if the system clock is before UNIX_EPOCH. Use unwrap_or with a safe fallback (e.g., Duration::from_secs(0)) to prevent panics in edge cases.
Applied to files:
dash-spv/src/network/reputation.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Follow mixed editions (2021/2024) and crate-specific idioms; prefer async via `tokio` where applicable
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Keep test vectors deterministic in Rust test code
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Unit tests should be placed near code with descriptive names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust
Applied to files:
dash-spv/src/storage/disk/io.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Applied to files:
dash-spv/src/network/persist.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should be organized in `tests/` directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration
Applied to files:
dash-spv/src/network/persist.rs
🧬 Code graph analysis (3)
dash-spv/src/network/reputation.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-69)
dash-spv/src/storage/disk/filters.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-69)
dash-spv/src/network/persist.rs (1)
dash-spv/src/storage/disk/io.rs (1)
atomic_write(34-69)
🔇 Additional comments (6)
dash-spv/src/network/reputation.rs (1)
15-16: Consistent reuse of atomic_write for reputation persistenceImporting
crate::storage::disk::io::atomic_writehere keeps reputation persistence aligned with the rest of the storage layer and gives you the same crash‑safety guarantees as other writes. No issues from a dependency or layering standpoint given this is crate‑internal.dash-spv/src/storage/disk/filters.rs (1)
10-11: Atomic writes for compact filters look correct and improve durabilitySwitching
store_filterto:let path = self.base_path.join(format!("filters/{}.dat", height)); atomic_write(&path, filter).awaitis a good fit: writes are now crash‑resilient, and
atomic_writewill also ensure thefiltersdirectory exists, which makes this path more robust than a baretokio::fs::write. No additional issues spotted.Also applies to: 186-190
dash-spv/src/network/persist.rs (1)
8-9: Atomic peer-store writes are consistent with storage layerUsing:
let json = serde_json::to_string_pretty(&saved) .map_err(|e| Error::Storage(StorageError::Serialization(e.to_string())))?; atomic_write(&self.path, json.as_bytes()).await.map_err(Error::Storage)?;nicely centralizes error handling via
StorageErrorand gives you atomic, fsync’d updates forpeers_*.json(plus automaticdata_dircreation). This is consistent with the new storage IO helpers and looks solid.Also applies to: 62-66
dash-spv/src/storage/disk/io.rs (3)
4-7: Temp-path generation and atomic_write implementation look correct and thread-safe
get_temp_path:
- Keeps temp files in the same directory as the target (
set_file_nameonly).- Uses
PID + AtomicU64for uniqueness, so concurrent calls within a process can’t collide.- Produces hidden,
.original_name.*.tmpfiles which your tests assert on.
atomic_write:
- Ensures
parentexists viatokio::fs::create_dir_all.- Writes to a unique temp path, fsyncs the temp file, then renames into place in the same directory, which preserves atomicity on typical filesystems.
- Cleans up temp files on both write and rename failures.
Taken together, this matches the usual tempfile+rename pattern and should behave well under concurrent writers (last writer wins). No functional issues spotted.
Also applies to: 18-30, 32-69
203-225: Temp-path test now platform-agnostic and aligned with prior feedback
test_get_temp_path_uniquenessnow:
- Asserts
temp1.parent() == path.parent()instead of string‑prefix checks.- Validates the temp file name via
file_name()(starts_with(".file.dat.")andends_with(".tmp")).This removes the previous POSIX‑specific path assumption and should work across platforms, including Windows. Nicely tightened up.
227-387: Atomic-write test coverage is thoroughThe async tests cover the key behaviors:
- File creation and parent directory creation.
- Overwrites preserving “last writer wins”.
- Absence of
.tmpfiles on success and on both write and rename failures.- Preservation of original content when writes fail.
- Correct handling of binary and large (1 MB) data.
The occasional use of
std::fs::read_dirin async tests is acceptable here given the small temp dirs and test context; no changes are strictly needed.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK; claude had one comment I didn't think of:
"""
-
Missing directory sync for durability (Minor)
-
For maximum crash safety on POSIX systems, you should also sync the parent directory after rename to ensure the directory entry is persisted:
// After rename succeeds
if let Some(parent) = path.parent() {
if let Ok(dir) = tokio::fs::File::open(parent).await {
let _ = dir.sync_all().await;
}
}
"""
Introduces atomic file writes for all SPV storage operations with a tempfile + rename pattern to ensure files dont get corrupted if a crash happens during write. - Adds `atomic_write` function - Updates all file writes: headers, filter headers, index, chain state, sync state, checkpoints, chainlocks, metadata, peers, peer-reputation
Introduces atomic file writes for all SPV storage operations with a tempfile + rename pattern to ensure files dont get corrupted if a crash happens during write. - Adds `atomic_write` function - Updates all file writes: headers, filter headers, index, chain state, sync state, checkpoints, chainlocks, metadata, peers, peer-reputation
2 participants
Introduces atomic file writes for all SPV storage operations with a tempfile + rename pattern to ensure files dont get corrupted if a crash happens during write.
atomic_writefunctionSummary by CodeRabbit
Improvements
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.