feat: introduce PersistentBlockStorage
#397
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughAdds persistent block storage to Dash SPV: a new async BlockStorage trait and PersistentBlockStorage that caches and persists HashedBlock instances, integrates into DiskStorageManager, and writes blocks during sync processing with unit tests and test helpers. Changes
Sequence DiagramsequenceDiagram
participant Handler as Block Message Handler
participant Manager as DiskStorageManager
participant BlockStore as PersistentBlockStorage
participant Cache as SegmentCache
participant Disk as Filesystem
Handler->>BlockStore: store_block(height, hashed_block)
BlockStore->>BlockStore: Insert height into available_heights
BlockStore->>Cache: insert(hashed_block, at height)
Cache->>Cache: segment insert
Note over Manager,BlockStore: background persistence loop
Manager->>BlockStore: persist(storage_path)
BlockStore->>Cache: persist()
Cache->>Disk: write segment files
Handler->>BlockStore: load_block(height)
BlockStore->>BlockStore: check available_heights
alt height tracked
BlockStore->>Cache: load(height)
Cache->>BlockStore: return HashedBlock
BlockStore->>Handler: Some(HashedBlock)
else not tracked
BlockStore->>Handler: None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/sync/legacy/message_handlers.rs (1)
642-655: Skip persistence when height is unknown (and drop wallet lock before I/O).
Ifget_header_height_by_hashreturnsNone, persisting at height0can corrupt the cache (overwriting genesis). Also, the new storage await happens while holding the wallet write lock; dropping the lock earlier reduces contention.🔧 Suggested fix
- let block_height = storage + let block_height = storage .get_header_height_by_hash(&block_hash) .await .map_err(|e| SyncError::Storage(format!("Failed to get block height: {}", e)))? - .unwrap_or(0); + ; - let result = wallet.process_block(block, block_height).await; + let result = wallet.process_block(block, block_height.unwrap_or(0)).await; + + drop(wallet); - storage - .store_block(block_height, HashedBlock::from(block)) - .await - .map_err(|e| SyncError::Storage(e.to_string()))?; + if let Some(height) = block_height { + storage + .store_block(height, HashedBlock::from(block)) + .await + .map_err(|e| SyncError::Storage(e.to_string()))?; + } else { + tracing::warn!("Skipping block persistence: height unknown for {}", block_hash); + } - - drop(wallet);
🤖 Fix all issues with AI agents
In `@dash-spv/src/storage/blocks.rs`:
- Around line 57-64: The current rebuild of available_heights uses
SegmentCache::get_items which asserts no sentinels and can panic on sparse
storage; change this to use a gap-tolerant iterator that scans the raw segment
entries and skips HashedBlock::sentinel values instead of calling get_items. Add
a helper (e.g., SegmentCache::scan_items_or a new segments::scan_raw_items) that
yields Option<HashedBlock> per index or filters out sentinels, then update the
loop that builds available_heights (the block-height loop referencing
blocks.start_height(), blocks.tip_height(), and HashedBlock::sentinel()) to
iterate that helper and only insert heights for non-sentinel entries.
- Around line 83-85: store_block currently unconditionally inserts into storage
which can trigger Segment::insert debug-assert if a slot already contains data;
update store_block to guard against duplicate height inserts by checking for
existing entries before inserting (e.g., consult available_heights or the
segment at height) and short-circuit when the height is already present, or call
an explicit overwrite/replace path on blocks (the call site
blocks.write().await.store_items_at_height should be replaced with a guard +
either skip insert or invoke a replace method). Ensure you reference and handle
available_heights and the blocks.write().await store operation so duplicate
insertions (reorgs/duplicates) don't hit Segment::insert debug panics.
In `@dash-spv/src/types.rs`:
- Around line 165-173: When decoding HashedBlock in the Decodable impl
(consensus_decode), after reading hash via BlockHash::consensus_decode and block
via Block::consensus_decode, compute the block's canonical hash (e.g.,
block.hash() or Block::hash()) and compare it to the decoded hash; if they
differ, return an Err(dashcore::consensus::encode::Error::ParseFailed(...)) (or
another appropriate consensus encode Error) to reject the payload so the
invariant hash ↔ block is enforced and corrupted/tampered data is not accepted.
ZocoLini
reviewed
Jan 30, 2026
dash-spv/src/storage/blocks.rs
Outdated
| let mut blocks: SegmentCache<HashedBlock> = | ||
| SegmentCache::load_or_new(&blocks_folder).await.map_err(|e| { | ||
| tracing::error!("Failed to load block segments from {:?}: {}", blocks_folder, e); | ||
| e |
Collaborator
There was a problem hiding this comment.
creating some logs here when we are propagating the error doesn't make sense to me, its the caller responsability to handle it/log it or continue the propagation until someone knows what to do, if not we end up with multiple logs for the same error
dash-spv/src/storage/mod.rs
Outdated
| block_headers: Arc<RwLock<PersistentBlockHeaderStorage>>, | ||
| filter_headers: Arc<RwLock<PersistentFilterHeaderStorage>>, | ||
| filters: Arc<RwLock<PersistentFilterStorage>>, | ||
| block_storage: Arc<RwLock<PersistentBlockStorage>>, |
Collaborator
There was a problem hiding this comment.
would prefer blocks here, following the other fields name convention
This is to allow storing blocks during sync to avoid the need of re-fetching for potential rescan. At the moment we don't have persistens wallets in the CLI and storing the blocks also helps there to make restarts ~15s on my machine compared to ~50s without cached blocks. I was thinking about maybe making storing blocks optional by config although i think some "clear sync cache" command for the spv client would be better since usually there are not many blocks stored anyway unless its super heavy used wallets.
xdustinface
force-pushed
the
feat/block-storage
branch
from
a15b8c8 to
dedb607
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
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/sync/legacy/message_handlers.rs (1)
643-654: Avoid persisting blocks when header height is unknown (cache corruption risk).If
get_header_height_by_hashreturnsNone, the currentunwrap_or(0)will store the block at height 0, which can overwrite/poison the genesis entry and misindex the cache.🔧 Suggested guard to skip storage when height is unknown
- let block_height = storage + let block_height_opt = storage .get_header_height_by_hash(&block_hash) .await .map_err(|e| SyncError::Storage(format!("Failed to get block height: {}", e)))? - .unwrap_or(0); + ; + let block_height = block_height_opt.unwrap_or(0); @@ - storage - .store_block(block_height, HashedBlock::from(block)) - .await - .map_err(|e| SyncError::Storage(e.to_string()))?; + if let Some(height) = block_height_opt { + storage + .store_block(height, HashedBlock::from(block)) + .await + .map_err(|e| SyncError::Storage(e.to_string()))?; + } else { + tracing::warn!( + "Skipping block storage for {}: height not found in headers", + block_hash + ); + }
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to allow storing blocks during sync to avoid the need of re-fetching for potential rescan. At the moment we don't have persistens wallets in the CLI and storing the blocks also helps there to make restarts ~15s on my machine compared to ~50s without cached blocks.
I was thinking about maybe making storing blocks optional by config although i think some "clear sync cache" command for the spv client would be better since usually there are not many blocks stored anyway unless its super heavy used wallets.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.