Query block hash by by block height #13

gcomte · 2022-09-16T19:42:51Z

Give the possibility to query a block hash by providing its corresponding block height

coveralls · 2022-09-19T14:11:19Z

Pull Request Test Coverage Report for Build 3227896333

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

37 of 43 (86.05%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 78.615%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/async.rs	14	16	87.5%
src/blocking.rs	13	17	76.47%

Totals
Change from base Build 3128702855:	0.3%
Covered Lines:	647
Relevant Lines:	823

💛 - Coveralls

notmandatory · 2022-09-19T14:12:37Z

This looks good, @tnull and @vladimirfomene are also working on examples for testing, once their stuff is in you should add a similar test for this new query also.

tnull

Generally looks good. Maybe we want to reuse these methods in get_header, since it also gets the block hash by height?

vladimirfomene · 2022-09-21T03:45:14Z

I believe we need to be careful when reusing some of these methods in other methods as it might result into doing more than one round trip to the server.

tnull · 2022-09-21T06:40:42Z

I believe we need to be careful when reusing some of these methods in other methods as it might result into doing more than one round trip to the server.

Ah, excuse the mistake, I meant get_header, in which two request are sent anyways currently?:

rust-esplora-client/src/blocking.rs

Lines 130 to 148 in 590a01b

    
               pub fn get_header(&self, block_height: u32) -> Result<BlockHeader, Error> { 
        
                   let resp = self 
        
                       .agent 
        
                       .get(&format!("{}/block-height/{}", self.url, block_height)) 
        
                       .call(); 
        
                   let bytes = match resp { 
        
                       Ok(resp) => Ok(into_bytes(resp)?), 
        
                       Err(ureq::Error::Status(code, _)) => Err(Error::HttpResponse(code)), 
        
                       Err(e) => Err(Error::Ureq(e)), 
        
                   }?; 
        
                   let hash = 
        
                       std::str::from_utf8(&bytes).map_err(|_| Error::HeaderHeightNotFound(block_height))?; 
        
                   let resp = self 
        
                       .agent 
        
                       .get(&format!("{}/block/{}/header", self.url, hash)) 
        
                       .call();

gcomte · 2022-09-21T10:08:29Z

Imo reusing get_block_hash within get_header makes sense.

Looking at the get_header code, I realize that I should check specifically for HTTP Error 404, in order to return HeaderHeightNotFound. However, this is only done by the async client, the blocking one just directly returns the HTTP error code. What's the reasoning behind this difference?

tnull

Generally looks good, two nits.

tnull · 2022-09-23T07:37:13Z

src/async.rs

-        let bytes = resp.bytes().await?;
-        let hash =
-            std::str::from_utf8(&bytes).map_err(|_| Error::HeaderHeightNotFound(block_height))?;
+        let block_hash = self.get_block_hash(block_height).await?.to_string();


nit: I think clippy will complain otherwise since BlockHash implements Display.

Suggested change

let block_hash = self.get_block_hash(block_height).await?.to_string();

let block_hash = self.get_block_hash(block_height).await?;

tnull · 2022-09-23T07:38:27Z

src/blocking.rs

-
-        let hash =
-            std::str::from_utf8(&bytes).map_err(|_| Error::HeaderHeightNotFound(block_height))?;
+        let block_hash = self.get_block_hash(block_height)?.to_string();


nit: Likewise.

Suggested change

let block_hash = self.get_block_hash(block_height)?.to_string();

let block_hash = self.get_block_hash(block_height)?;

gcomte · 2022-09-23T12:27:41Z

Generally looks good, two nits.

Fixed and rebased onto master. Clippy happy now.

vladimirfomene · 2022-09-26T12:10:53Z

@gcomte, I think it will be cleaner if we squash these changes into one commit.

gcomte · 2022-09-26T14:21:40Z

Yeah, that could be done when merging, right?

vladimirfomene · 2022-10-05T11:43:17Z

Hey @gcomte ! can you squash it here? It makes things easy for us.

gcomte · 2022-10-05T15:59:29Z

Hey @gcomte ! can you squash it here? It makes things easy for us.

Done 👍

gcomte · 2022-10-05T16:04:59Z

I guess I should write some tests as well, as this PR has been merged now: #10

vladimirfomene · 2022-10-11T14:14:50Z

@gcomte, can you add test to this so that I can merge it?

gcomte · 2022-10-11T14:55:24Z

@vladimirfomene : b6d663b

Do you want me to squash it into the other commit?

tnull · 2022-10-11T15:08:35Z

src/lib.rs

+        let (blocking_client, async_client) = setup_clients().await;
+        let block_hash = blocking_client.get_block_hash(21).unwrap();
+        let block_hash_async = async_client.get_block_hash(21).await.unwrap();
+        assert_eq!(block_hash, block_hash_async);


You may want to add another check comparing the Esplora results to RPC, i.e., BITCOIND.client.get_block_hash(21)

That's right. Added: 7c66cf1

92606cc Test `get_header_by_hash` rather than `get_height` (Elias Rohrer) 6673912 Deprecate `get_height` (Elias Rohrer) 9e233ef Allow to retrieve a block header by hash (Elias Rohrer) Pull request description: ~~Based on #13 since it probably land soon.~~ This PR adds a method allowing to retrieve a `BlockHeader` via a given `BlockHash` and reuses it in `get_height`. Top commit has no ACKs. Tree-SHA512: 2be894f7e745e317b02512183e073806e90145cbde9ad89f10e5782d6bc302da6d72adb0cef0fce7cd746e4f7b93fba0844a640bc30457ccafbcf19dd5673254

tnull reviewed Sep 19, 2022

View reviewed changes

tnull reviewed Sep 23, 2022

View reviewed changes

gcomte force-pushed the feature/query-block-hash-by-height branch from 3072fd0 to 6b2f84f Compare September 23, 2022 12:26

tnull approved these changes Sep 23, 2022

View reviewed changes

gcomte force-pushed the feature/query-block-hash-by-height branch from 6b2f84f to 05e1045 Compare October 5, 2022 15:58

tnull mentioned this pull request Oct 6, 2022

Allow to retrieve a block header by hash #17

Merged

Query block hash by by block height

a0f1aca

gcomte force-pushed the feature/query-block-hash-by-height branch from 05e1045 to b6d663b Compare October 11, 2022 14:53

tnull reviewed Oct 11, 2022

View reviewed changes

Add integration test for get_block_hash()

7c66cf1

gcomte force-pushed the feature/query-block-hash-by-height branch from b6d663b to 7c66cf1 Compare October 11, 2022 15:36

tnull approved these changes Oct 11, 2022

View reviewed changes

vladimirfomene approved these changes Oct 12, 2022

View reviewed changes

vladimirfomene merged commit 8364d05 into bitcoindevkit:master Oct 12, 2022

gcomte deleted the feature/query-block-hash-by-height branch October 12, 2022 07:39

notmandatory added this to the Release 0.2.0 milestone Oct 22, 2022

	let block_hash = self.get_block_hash(block_height).await?.to_string();
	let block_hash = self.get_block_hash(block_height).await?;

Query block hash by by block height #13

Query block hash by by block height #13

Uh oh!

Conversation

gcomte commented Sep 16, 2022

Uh oh!

coveralls commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3227896333

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

notmandatory commented Sep 19, 2022

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirfomene commented Sep 21, 2022

Uh oh!

tnull commented Sep 21, 2022

Uh oh!

gcomte commented Sep 21, 2022

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

gcomte commented Sep 23, 2022

Uh oh!

vladimirfomene commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcomte commented Sep 26, 2022

Uh oh!

vladimirfomene commented Oct 5, 2022

Uh oh!

gcomte commented Oct 5, 2022

Uh oh!

gcomte commented Oct 5, 2022

Uh oh!

vladimirfomene commented Oct 11, 2022

Uh oh!

gcomte commented Oct 11, 2022

Uh oh!

tnull Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

gcomte Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Sep 19, 2022 •

edited

Loading

tnull left a comment •

edited

Loading

vladimirfomene commented Sep 26, 2022 •

edited

Loading