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

add(rpc): Add block times to verbose output of getblock RPC method #8384

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Apr 4, 2024

Motivation

This field is provided in zcashd and was asked about on Discord.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

  • Adds BlockHeader state requests/responses (there were already FindBlockHeaders requests/responses, this one is simpler)
  • Requests the block header from the state service in the getblock() RPC method
  • Adds the timestamp (seconds since the unix epoch) as a time field to the verbose return value from the getblock() RPC

Testing

Update the existing snapshot and vectors tests to include block time.

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

  • Adding other fields from the block header to the getblock RPC's verbose output

…1 (it's already included with verbosity = 0 but this makes it easier to use).
@arya2 arya2 added C-enhancement Category: This is an improvement A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes P-Low ❄️ labels Apr 4, 2024
@arya2 arya2 self-assigned this Apr 4, 2024
@arya2 arya2 requested a review from a team as a code owner April 4, 2024 14:46
@arya2 arya2 requested review from oxarbitrage and removed request for a team April 4, 2024 14:46
@mpguerra mpguerra added the I-usability Zebra is hard to understand or use label Apr 8, 2024
@arya2
Copy link
Contributor Author

arya2 commented Apr 8, 2024

We may want to get the full block data from the state service for verbosity = 1 instead, because it'll be needed for returning the serialized block sizes and auth data roots (see https://github.com/zcash/zcash/blob/master/src/rpc/blockchain.cpp#L225).

Or we may not need to match zcashd's output exactly.

Update:
There's a note about this RPC being used for lightwalletd's initial sync, so, for now it needs to load all its fields very efficiently.

If we do want to match zcashd's output, we could:

  • Move or duplicate the debug_like_zcashd config from the mining section to the rpc section and only read the entire block when it's enabled, and/or
  • Add an optional argument for callers to provide a list of fields to be included in the return value.

@arya2 arya2 marked this pull request as draft April 9, 2024 02:56
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
@mpguerra
Copy link
Contributor

mpguerra commented Apr 9, 2024

It seems like the scope for this PR is possibly expanding. Should we create an issue for all of the things we may want to do in order to support lightwalletd and potentially also match zcashd behaviour?

@arya2 arya2 marked this pull request as ready for review April 10, 2024 03:45
mergify bot added a commit that referenced this pull request Apr 12, 2024
@mergify mergify bot merged commit ad34585 into main Apr 12, 2024
157 checks passed
@mergify mergify bot deleted the add-block-time branch April 12, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-enhancement Category: This is an improvement I-usability Zebra is hard to understand or use P-Low ❄️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants