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

[arya2]

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?

[upbqdn]

Thanks for this refactor. It looks great.

[teor2345]

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.)

[mpguerra]

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]

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]

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

[teor2345]

Thanks!