Skip to content
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

Conversation

vladimirfomene
Copy link
Contributor

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:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

src/blockchain/electrum.rs Outdated Show resolved Hide resolved
src/blockchain/esplora/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@danielabrozzoni danielabrozzoni left a 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

src/testutils/blockchain_tests.rs Outdated Show resolved Hide resolved
src/testutils/blockchain_tests.rs Outdated Show resolved Hide resolved
src/testutils/blockchain_tests.rs Outdated Show resolved Hide resolved
src/blockchain/mod.rs Outdated Show resolved Hide resolved
@vladimirfomene vladimirfomene force-pushed the implement-getblockhash-by-height branch from 6f3528b to 0822cc0 Compare June 30, 2022 12:09
@vladimirfomene
Copy link
Contributor Author

@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 rpc blockchain, the following tests are failing:
failing-test

@danielabrozzoni
Copy link
Member

That looks like a spurious error...

In CI, compact filters compilation is failing. You can reproduce with cargo build --features compact_filters --no-default-features

@vladimirfomene vladimirfomene force-pushed the implement-getblockhash-by-height branch 2 times, most recently from 62b00fb to d0efd4d Compare July 1, 2022 15:19
@notmandatory notmandatory added the new feature New feature or request label Jul 4, 2022
Copy link
Member

@danielabrozzoni danielabrozzoni left a 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

Comment on lines 265 to 271
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,
)),
}
Copy link
Member

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 :)

@vladimirfomene vladimirfomene force-pushed the implement-getblockhash-by-height branch from d0efd4d to 2b6801b Compare July 6, 2022 14:50
@danielabrozzoni
Copy link
Member

Also, you have conflicts with the changelog :(

@vladimirfomene vladimirfomene force-pushed the implement-getblockhash-by-height branch from 2b6801b to b985b12 Compare July 6, 2022 16:44
@vladimirfomene
Copy link
Contributor Author

@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.
@vladimirfomene vladimirfomene force-pushed the implement-getblockhash-by-height branch from b985b12 to 2af678a Compare July 6, 2022 17:03
Copy link
Member

@notmandatory notmandatory left a 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 !

@notmandatory notmandatory merged commit 73d4f6d into bitcoindevkit:master Jul 6, 2022
notmandatory added a commit to bitcoindevkit/rust-esplora-client that referenced this pull request Aug 2, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add blockchain::GetBlockHash trait and implement for blockchains
4 participants