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

cleanup(state): Refactor subtree read functions to accept ranges and check memory first #7739

Merged
merged 15 commits into from
Oct 20, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Oct 13, 2023

Motivation

The read::{sapling, orchard}_subtrees functions are reading from disk before checking the non-finalized state and minor cleanup of inconsistent behaviour between the Chain methods for retrieving subtrees from the non-finalized state compared to those of DiskDb.

Solution

  • Check if all of the subtrees in the range exist in the chain before reading from disk
  • Use RangeBounds instead of start/end bounds
  • Update Chain::{sapling, orchard}_subtrees_in_range to only query for the provided range
  • Move and consolidate checks for results containing start_index
  • Update docs for DiskDb::{sapling, orchard}_subtree_list_by_index_for_rpc() and Chain::{sapling, orchard}_subtrees_in_range

Review

Anyone can review.

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?
  • How do you know it works? Does it have tests?

@arya2 arya2 added C-cleanup Category: This is a cleanup P-Low ❄️ A-state Area: State / database changes labels Oct 13, 2023
@arya2 arya2 self-assigned this Oct 13, 2023
@arya2 arya2 requested a review from a team as a code owner October 13, 2023 01:11
@arya2 arya2 requested review from upbqdn and removed request for a team October 13, 2023 01:11
@github-actions github-actions bot added C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 13, 2023
@arya2 arya2 force-pushed the cleanup-read-subtree branch from 62aef12 to 5b55813 Compare October 13, 2023 03:30
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.

Thanks for this refactor. It looks great.

zebra-state/src/service/read/tree.rs Show resolved Hide resolved
upbqdn
upbqdn previously approved these changes Oct 13, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks like a good rewrite, and I appreciate how it simplifies the code.

All my comments on the Sapling methods also apply to Orchard.

I'm concerned that this change could lead to unexpected bugs, or significant extra testing. Did Pili approve a scope for this work, or is there a ticket with the scope of the work? In particular, supporting some range types might need extra tests, because the start bound gets special handling.

Since this is a substantial rewrite, do we need to re-do the manual tests with zcashd from #7450? Which unit tests do we need to add?

A few of your recent PRs have missed documentation updates and test coverage. Would it help if you make a personal checklist that you go through before you submit a PR? Or could we add a checklist for PR authors to the PR template?

(I am working on PR template updates at the moment.)

zebra-state/src/service/read/tree.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/tree.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/tree.rs Outdated Show resolved Hide resolved
zebra-state/src/service.rs Show resolved Hide resolved
zebra-state/src/service/read/tree.rs Outdated Show resolved Hide resolved
@upbqdn upbqdn added do-not-merge Tells Mergify not to merge this PR C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG do-not-merge Tells Mergify not to merge this PR labels Oct 16, 2023
@arya2 arya2 added the C-testing Category: These are tests label Oct 18, 2023
@arya2 arya2 requested a review from teor2345 October 19, 2023 01:46
@arya2 arya2 requested a review from upbqdn October 19, 2023 01:46
@mpguerra
Copy link
Contributor

I'm concerned that this change could lead to unexpected bugs, or significant extra testing. Did Pili approve a scope for this work, or is there a ticket with the scope of the work? In particular, supporting some range types might need extra tests, because the start bound gets special handling.

I was wondering which issue this PR related to... :P We should finish off the rest of the issues in this sprint and merge their associated PRs before doing any more work on this one

@teor2345
Copy link
Contributor

I'm concerned that this change could lead to unexpected bugs, or significant extra testing. Did Pili approve a scope for this work, or is there a ticket with the scope of the work? In particular, supporting some range types might need extra tests, because the start bound gets special handling.

I was wondering which issue this PR related to... :P We should finish off the rest of the issues in this sprint and merge their associated PRs before doing any more work on this one

This PR blocks ticket #7463, because they both change the subtree code.

We can start work on #7463, but this PR should merge before the PR for that ticket, otherwise there will be a lot of rework to merge them. Since #7463 is a fully scripted change, we can just re-run the script after this PR merges. (Any non-scripted parts of that ticket should be in a separate PR based on this PR.)

Conflicts like this are another reason why scheduling work is important!

teor2345
teor2345 previously approved these changes Oct 20, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Ok, this looks great, pending the minor function doc update.

@arya2 arya2 force-pushed the cleanup-read-subtree branch from 17c1a71 to 3416c11 Compare October 20, 2023 00:55
@arya2 arya2 requested a review from teor2345 October 20, 2023 02:05
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks!

mergify bot added a commit that referenced this pull request Oct 20, 2023
@mergify mergify bot merged commit 06a2983 into main Oct 20, 2023
104 checks passed
@mergify mergify bot deleted the cleanup-read-subtree branch October 20, 2023 05:06
@teor2345 teor2345 mentioned this pull request Nov 5, 2023
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-cleanup Category: This is a cleanup C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants