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): Implement the z_getsubtreesbyindex RPC #7436

Merged
merged 1 commit into from
Sep 4, 2023
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 31, 2023

Motivation

This PR implements the z_getsubtreesbyindex RPC method and return type.

It needs data from PR #7350 for testing, but it can merge without that PR and we can write tests later.

Close #6954.

Specifications

See https://github.com/zcash/zcash/pull/6677/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1458-R1475

The specs for this RPC aren't on the RPC spec site yet.

Complex Code or Requirements

This RPC must return the same data and format as zcashd.

The data for this RPC must be added to the database from the non-finalized tip backwards (or all at the same time), see ticket #7407 for details.

Solution

Implement the RPC method, arguments, and return type

  • Validate the pool argument
  • Match zcashd's behaviour of returning an empty list if the start_index is not available
  • Don't error if start_index + limit would overflow, matching zcashd's behaviour

Testing

We'll split tests into separate tickets, as requested here:
#6954 (comment)

New RPC methods will need:

  • new snapshot tests
  • new integration tests
  • manual zcash-rpc-diff test results in a comment on the PR, to make sure we match zcashd

And possibly:

  • new unit tests

Review

This PR is ready for review, we will write tests in other tickets.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?

@teor2345 teor2345 added P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd C-feature Category: New features A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Aug 31, 2023
@teor2345 teor2345 self-assigned this Aug 31, 2023
@teor2345 teor2345 changed the base branch from main to z-get-subtree-stub August 31, 2023 01:05
@teor2345
Copy link
Contributor Author

Temporary network error on a draft PR, doesn't need fixing or even restarting:

Error: buildx failed with: ERROR: failed to solve: error writing layer blob: failed to authorize: failed to fetch oauth token: Get "https://us-docker.pkg.dev/v2/token?scope=repository%3Azfnd-dev-zebra%2Fzebra-caching%2Fzebrad-test%3Apull%2Cpush&service=us-docker.pkg.dev": read tcp 10.1.0.161:46942->142.250.115.82:443: read: connection reset by peer

https://github.com/ZcashFoundation/zebra/actions/runs/6031782300/job/16365993221?pr=7436#step:10:1811

@oxarbitrage oxarbitrage added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Aug 31, 2023
Base automatically changed from z-get-subtree-stub to main September 3, 2023 22:18
@teor2345 teor2345 marked this pull request as ready for review September 4, 2023 00:18
@teor2345 teor2345 requested a review from a team as a code owner September 4, 2023 00:18
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team September 4, 2023 00:18
Revert "Temporarily remove the z_get_subtrees_by_index RPC method"
This reverts commit 30d049e.
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for oxarbitrage September 4, 2023 16:28
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

utACK

mergify bot added a commit that referenced this pull request Sep 4, 2023
@mergify mergify bot merged commit 5e8b4f3 into main Sep 4, 2023
@mergify mergify bot deleted the z-get-subtree-rpc branch September 4, 2023 21:05
arya2 pushed a commit that referenced this pull request Sep 29, 2023
Revert "Temporarily remove the z_get_subtrees_by_index RPC method"
This reverts commit 30d049e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement z_getsubtreesbyindex RPC
3 participants