Add verbosity support to getblockheader RPC#1006
Conversation
| /// Formats block header data for JSON-RPC responses. | ||
| /// Computes all necessary header fields (height, confirmations, chain work, difficulty, etc.) | ||
| /// in the correct format ready for `getblockheader` and `getblock` RPC methods. | ||
| fn handle_get_block_header_verbose( |
There was a problem hiding this comment.
nice work @moisesPompilio ..
little nit: handle_get_block_header_verbose reads like a request handler, bt here it just formats...i'd suggest format_block_header_verbose or block_header_verbose_fields .
|
Needs rebase. |
34c2fe2 to
1cf7b61
Compare
| confirmations, | ||
| difficulty, | ||
| hash: header.block_hash().to_string(), | ||
| height: height as i64, |
There was a problem hiding this comment.
I dunno what is the project's default approach for using as, since it can be unsafe (as is same as try_into().unwrap()).
Since all the casts here goes from u32 to i64 this is safe, but it can silently introduce a panic if the source type changes to something bigger.
There was a problem hiding this comment.
Good catch. I replaced the as casts with into, so it warns me if the conversion is safe. However, some of them were not safe, so I used try_into instead and added automatic error conversion to report the overflow that happened
|
LGTM |
849cc5d to
257d59c
Compare
|
257d59c I did the rebase and converted the test change I made to the |
|
|
||
| /// Formats block header data for JSON-RPC responses. | ||
| /// Computes all necessary header fields (height, confirmations, chain work, difficulty, etc.) | ||
| /// in the correct format ready for `getblockheader` and `getblock` RPC methods. |
There was a problem hiding this comment.
| /// in the correct format ready for `getblockheader` and `getblock` RPC methods. | |
| /// Same as `get_block_header_inner` but verbose. |
I prefer something more straight to the point, cutting implicit info, partially mentioning aditional fields is unecessary since one can check this looking at the returned struct but the latter is good too
| let height = header.get_height(&self.chain)?; | ||
| let median_time = header.calculate_median_time_past(&self.chain)?; | ||
| let chain_work = header.calculate_chain_work(&self.chain)?.to_string_hex(); | ||
| let confirmations = header.get_confirmations(&self.chain)?; | ||
| let version_hex = header.get_version_hex(); | ||
|
|
||
| let next_block_hash = header | ||
| .get_next_block_hash(&self.chain)? | ||
| .map(|h| h.to_string()); | ||
|
|
||
| let bits = header.get_bits_hex(); | ||
| let difficulty = header.get_difficulty(); | ||
| let target = header.get_target_hex(); |
There was a problem hiding this comment.
Yes, getblock and getblockheader differ in that one includes the transactions and the other doesn’t, basically. The rest of the fields would be the same.
|
|
||
| #[derive(Debug, Deserialize, Serialize)] | ||
| #[serde(untagged)] | ||
| pub enum GetBlockHeaderRes { |
|
|
||
| #[derive(Debug, Deserialize, Serialize)] | ||
| #[serde(untagged)] | ||
| pub enum GetBlockHeaderRes { |
| - `previousblockhash` - (string, optional) The hash of the previous block. | ||
| - `nextblockhash` - (string, optional) The hash of the next block. | ||
|
|
||
| ### Error Enum `CommandError` |
There was a problem hiding this comment.
| ### Error Enum `CommandError` | |
| ### Error Enum |
| # This causes the median time of recent blocks to be different from earlier blocks, which is | ||
| # necessary for proper testing. |
There was a problem hiding this comment.
| # This causes the median time of recent blocks to be different from earlier blocks, which is | |
| # necessary for proper testing. | |
| # We need different timestamps to cause the median time of recent blocks to be different from earlier blocks, which is | |
| # necessary for proper testing. |
| MempoolAccept(MempoolError), | ||
|
|
||
| /// A numeric conversion overflows, e.g., u64 to u32 | ||
| ConversionOverflow(String), |
There was a problem hiding this comment.
Wait, it feels really off to have this kind of error here, surely theres an existing alternative for this
There was a problem hiding this comment.
I don’t think there are alternatives, because there are some situations where we’re working with usize, whose size varies. So sometimes we need to convert usize to u32 or i64, and there’s a risk of overflow depending on the computer’s architecture.
|
Needs rebase |
| Methods::GetBlockHeader { hash } => { | ||
| serde_json::to_string_pretty(&client.get_block_header(hash)?)? | ||
| Methods::GetBlockHeader { hash, verbosity } => { | ||
| serde_json::to_string_pretty(&client.get_block_header(hash, verbosity)?)? |
| node_manager: Any = None | ||
|
|
||
| @pytest.mark.rpc | ||
| def test_get_blockheader( |
There was a problem hiding this comment.
This test is very very nice. Can you extend it with failures cases too?
739a8c5 to
d04f180
Compare
| Raw(String), | ||
| /// A verbose block header, as returned by `getblockheader` with verbosity true | ||
| Verbose(Box<GetBlockHeaderVerbose>), | ||
| } |
There was a problem hiding this comment.
| Raw(String), | |
| /// A verbose block header, as returned by `getblockheader` with verbosity true | |
| Verbose(Box<GetBlockHeaderVerbose>), | |
| } | |
| Raw(String), | |
| /// A verbose block header, as returned by `getblockheader` with verbosity true | |
| Verbose(Box<GetBlockHeaderVerbose>), | |
| } |
nit
There was a problem hiding this comment.
I had a pending review with this, forgot to publish.
Agree 👍
|
ACK d04f180 |
Implement verbosity parameter for `getblockheader` RPC with both raw and verbose modes. Extract `get_block_header_inner()` helper method to retrieve headers by hash, consolidating previously duplicated logic. Introduce `get_block_header_verbose_inner()` for consistent header formatting across RPC methods. Expose the verbosity optional parameter in the FlorestaRPC trait's `get_block_header` method, allowing clients to choose between raw (hex string) and verbose (JSON object) response formats. Both `getblockheader` and `getblock` now reuse the same header formatting logic, reducing code duplication and improving maintainability. Closes: getfloresta#606
Improve the getblockheader integration test to validate responses across multiple block scenarios: genesis block, a random block from the chain, and the tip block. Test both verbosity modes (false and true) to ensure correct formatting in all cases. Rename helper method to validate_headers_match for better clarity.
d04f180 to
44fffb5
Compare
Description and Notes
Add support for the
verbosityparameter to thegetblockheaderRPC, bringing it in line with Bitcoin Core behavior.Previously,
getblockheaderonly supported the raw mode, returning the block header as a hexadecimal string. With this change, it now also supports verbose mode, returning a structured JSON object with the full block header details.The verbose response follows the same RPC shape and default verbosity behavior as Bitcoin Core. In addition, the verbose output reuses the same header formatting logic as
getblock, since both methods expose the same header fields. To avoid duplication, the shared header data is now produced by a helper function that returns a fully populated object ready for use by both RPCs.Closes: #606
How to verify the changes you have done?
getblockheaderand confirm they pass.getblockheaderoutput against Bitcoin Core correctly.getblockheaderhas been updated properly.getblockheaderis exposed with theverbosityparameter in the CLI.