-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get block hash by its height #634
Get block hash by its height #634
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, a couple of comments
6f3528b
to
0822cc0
Compare
@danielabrozzoni I have amended the code based on your recommendation. @evanlinjin I have also fixed the import issue. The test is passing for all blockchains but while testing against |
That looks like a spurious error... In CI, compact filters compilation is failing. You can reproduce with |
62b00fb
to
d0efd4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK d0efd4d
just a small nit
let block_hash = self.headers.get_block_hash(height as usize)?; | ||
match block_hash { | ||
Some(block_hash) => Ok(block_hash), | ||
None => Err(Error::CompactFilters( | ||
CompactFiltersError::BlockHashNotFound, | ||
)), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can re-write this as
self.headers.get_block_hash(height as usize)?.ok_or(Error::CompactFilters(
CompactFiltersError::BlockHashNotFound,
))
ok_or
makes it way more compact :)
d0efd4d
to
2b6801b
Compare
Also, you have conflicts with the changelog :( |
2b6801b
to
b985b12
Compare
@danielabrozzoni, thanks for pointing that out. Fixed! |
Create blockchain::GetBlockHash trait with a method to get block hash given a block height. Then, implement this trait for all backends (Electrum, RPC , Esplora, CBF). Referenced in issue 603.
b985b12
to
2af678a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2af678a
Looks good !
758dba1 Get block hash by its height (Vladimir Fomene) Pull request description: ### Description This PR create a new trait `blockchain::GetBlockHash` with a `get_block_hash` method which returns a block hash given the block height. This has been implemented for all blockchain backends. Fixes #603 ### Notes to the reviewers I haven't updated the `CHANGELOG.md` and docs. Am I suppose to update it for this change? ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [ ] I've added docs for the new feature * [ ] I've updated `CHANGELOG.md` ACKs for top commit: notmandatory: ACK 758dba1 Tree-SHA512: 9c084a6665ecbf27ee8170fdb06e0dc8373d6a901ce29e5f5a1bec111d1507cb3bee6b03a653a55fd20e0fabe7a5eada3353e24a1e21f3a11f01bb9881ae99e5
Description
This PR create a new trait
blockchain::GetBlockHash
with aget_block_hash
method which returns a block hash given the block height. This has been implemented for all blockchain backends.Fixes #603
Notes to the reviewers
I haven't updated the
CHANGELOG.md
and docs. Am I suppose to update it for this change?Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
CHANGELOG.md