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

change(rpc): Add value pool balances to getblockchaininfo RPC method response #8769

Merged
merged 9 commits into from
Aug 16, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Aug 16, 2024

Motivation

We want to add value pool balances at the chain tip to Zebra's getblockchaininfo RPC method response.

Closes #8767.

Specifications & References

https://zcash.github.io/rpc/getblockchaininfo.html

Solution

  • Moves the Zec type in zebra-rpc out from behind the getblocktemplate-rpcs feature flag.
  • Adds a ValuePoolBalance type to a new types::get_blockchain_info module with a fn for creating one from a ValueBalance.
  • Updates the getblockchaininfo RPC method return type to a BoxFuture to make async calls to the state service.
  • Adds a new state service request for getting the tip height, tip hash, and the value balance.
    • Checks that the tip hasn't changed while reading the value balance from the finalized state. This could cause the RPC method to return an error after a few retries, which could be avoided by adding the value balance information to the ChainTipBlock instead of adding a new state service request, but in practice, the tip and value balance should be in-memory when there is no non-finalized best chain and blocks are being committed rapidly, so it should be unlikely to return that error. Still, let me know if we want to add it to the ChainTipBlock instead, we could do that here or in a follow-up PR.
  • Updates the getblockchaininfo method to get the chain tip height, hash, value balance, and block time from state requests.
  • Updates snapshots

Related Changes:

  • Minor refactor of estimated_height assignment
  • Uses generic error construction in the last couple places where possible and removes the outdated TODOs

Tests

Updates existing snapshots (and some prop tests to handle the BoxFuture wrapped return type).

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@arya2 arya2 added C-enhancement Category: This is an improvement NU-6 Network Upgrade: NU6 specific tasks A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes P-High 🔥 labels Aug 16, 2024
@arya2 arya2 added this to the NU6 milestone Aug 16, 2024
@arya2 arya2 self-assigned this Aug 16, 2024
@arya2 arya2 requested a review from a team as a code owner August 16, 2024 04:48
@arya2 arya2 requested review from oxarbitrage and removed request for a team August 16, 2024 04:48
@arya2 arya2 added the C-cleanup Category: This is a cleanup label Aug 16, 2024
oxarbitrage
oxarbitrage previously approved these changes Aug 16, 2024
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good to me, great work, thanks!

@oxarbitrage
Copy link
Contributor

oxarbitrage commented Aug 16, 2024

It seems the tests are hang somewhere so the CI is giving up. https://github.com/ZcashFoundation/zebra/actions/runs/10414872254/job/28844504197?pr=8769

I am having the same issue locally when running the suite:

cargo test

@arya2
Copy link
Contributor Author

arya2 commented Aug 16, 2024

It seems the tests are hang somewhere so the CI is giving up

Ah, yes, I forgot to handle the mock service requests in the prop tests.

@mergify mergify bot merged commit 83c6725 into main Aug 16, 2024
157 of 158 checks passed
@mergify mergify bot deleted the add-value-pools-to-getblockchaininfo branch August 16, 2024 19:54
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-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement NU-6 Network Upgrade: NU6 specific tasks P-High 🔥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add valuePools object to getblockchaininfo RPC
2 participants