fix: avoid panic for sentinel blocks in debug builds

[xdustinface]

The workaround with the available heights also doesn't work in debug builds. I just didn't realize since I was always running release builds lately. Now while working on integration tests, this became an issue again.

Summary by CodeRabbit

• Refactor
  • Simplified block storage system by removing redundant in-memory height tracking logic
  • Updated block storage API signatures to use standard integer types for improved consistency
  • Optimized block retrieval mechanism for better performance

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR removes in-memory height tracking from PersistentBlockStorage by eliminating the available_heights HashSet and simplifies block storage operations to directly query SegmentCache instead. Public API signatures updated from CoreBlockHeight to u32 parameter types.

Changes

Cohort / File(s)	Summary
Sparse Data Access Support dash-spv/src/storage/segments.rs	Added get_item() method to SegmentCache for sparse height-based reads and get_single() method to Segment for raw offset-based access, filtering out sentinel values.
Storage Simplification dash-spv/src/storage/blocks.rs	Removed available_heights HashSet field; eliminated sentinel-based gap detection logic; simplified store_block() and load_block() to directly interface with SegmentCache; updated trait signatures from CoreBlockHeight to u32 parameter types.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant BlockStorage as PersistentBlockStorage
    participant Cache as SegmentCache
    participant Segment
    
    Note over Client,Segment: Previous Flow (with available_heights tracking)
    Client->>BlockStorage: store_block(height, block)
    BlockStorage->>BlockStorage: update available_heights HashSet
    
    Client->>BlockStorage: load_block(height)
    BlockStorage->>BlockStorage: check available_heights
    alt height found
        BlockStorage->>Cache: fetch via range query
        Cache->>Segment: get segment data
        Segment-->>Cache: return item
        Cache-->>BlockStorage: return item
    else height not found
        BlockStorage-->>Client: return None
    end
    
    Note over Client,Segment: New Flow (direct SegmentCache access)
    Client->>BlockStorage: store_block(height, block)
    BlockStorage->>Cache: direct storage operation
    
    Client->>BlockStorage: load_block(height)
    BlockStorage->>Cache: get_item(height)
    Cache->>Cache: compute segment & offset
    Cache->>Segment: fetch_segment(segment_id)
    Segment-->>Cache: segment data
    Cache->>Segment: get_single(offset)
    Segment-->>Cache: return item or sentinel
    Cache->>Cache: filter sentinel values
    Cache-->>BlockStorage: return Option<HashedBlock>
    BlockStorage-->>Client: return Option<HashedBlock>

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 HashSets be gone, no tracking in sight,
SegmentCache queries now shine so bright,
Sparse reads and sentinels, logic refined,
Simpler storage, peace of mind! 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: avoid panic for sentinel blocks in debug builds' directly addresses the main objective: fixing a panic for sentinel blocks in debug builds by replacing the available_heights workaround with sparse-safe SegmentCache methods.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/sparse-storage

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/blocks.rs (1)

14-25: ⚠️ Potential issue | 🟡 Minor

Update impl BlockStorage to use CoreBlockHeight in method signatures, matching the trait.

The BlockStorage trait declares parameters as CoreBlockHeight (lines 19, 24), while the impl block uses u32 directly (lines 62, 66). CoreBlockHeight is a type alias for u32, so they compile to the same type, but the inconsistency between trait and implementation makes the code harder to follow. Update the impl methods to use CoreBlockHeight to match the trait interface.

🧹 Nitpick comments (2)

dash-spv/src/storage/segments.rs (1)

496-500: Consider adding a debug_assert for offset bounds, consistent with insert.

insert (line 480) has debug_assert!(offset < Self::ITEMS_PER_SEGMENT). Adding the same guard here would maintain consistency and catch misuse early.

Suggested change

     pub fn get_single(&mut self, offset: u32) -> &I {
+        debug_assert!(offset < Self::ITEMS_PER_SEGMENT);
         self.last_accessed = Instant::now();
         &self.items[offset as usize]
     }

dash-spv/src/storage/blocks.rs (1)

66-68: load_block acquires a write lock for a conceptually read-only operation.

get_item requires &mut self (due to LRU last_accessed tracking in the segment cache), so a write lock is needed. This serializes all block loads. Not a regression from this PR, but worth noting for future optimization — e.g., using interior mutability (Cell<Instant>) for last_accessed could allow read-lock access.