Optimize FlatChainStore and improve tests
#493
Merged
+158
−59
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
Previously we were doing:
```rust
unsafe fn get_header_by_hash(
&self,
hash: BlockHash,
) -> Result<Option<DiskBlockHeader>, FlatChainstoreError> {
let header = self
.block_index
.get_index_for_hash(hash, |height| self.get_block_header_by_index(height))?
.map(|height| self.get_block_header_by_index(height));
Ok(header.transpose()?.map(|x| x.header))
}
```
Which we can optimize/simplify by making the `get_index_for_hash` function directly return the header as well, which is free since it's already fetched.
To make this easier I make the `IndexBucket` enum contain the header if occupied, and named the fields for clarity.
In the `ChainStore::get_block_hash` impl we handle the `NotFound` case. Then I have improved the headers tests by verifying our assumptions are holding.
Davidson-Souza
approved these changes
May 21, 2025
There was a problem hiding this comment.
ACK 2eb173f
Pretty nice what you did in the tests. Testing edge cases is something that's somewhat lacking inside our unit tests
Thanks! I wanted to actually verify that we get zero hashes if it is uninitialized data |
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.
What is the purpose of this pull request?
Which crates are being modified?
Description
Previously we were doing:
Which we can optimize/simplify by making the
get_index_for_hashfunction directly return the header as well, which is free since it's already fetched.To make this easier I made the
IndexBucketenum contain the header if occupied (I think this is good as it's data related to the entry we searched for), and named the fields for clarity.In the
ChainStore::get_block_hashimpl we handle theNotFoundcase. Then I have improved the headers tests by verifying our assumptions are holding.