fix(state): Always return updated trees in `NoteCommitmentTree` update methods

[arya2]

Motivation

This PR makes the code a bit more clear.

~Arc::make_mut() will clone the value if there are other references to the value, so if a note commitment tree is being updated after its been requested from the state service but before its been dropped, it could, in rare cases, drop the updated note commitment tree and add the new block to the state with the previous note commitment tree.~

~This only affects the non-finalized state, and note commitment trees are only read by the getblock and z_gettreestate RPC methods.~

PR Author Checklist

Check before marking the PR as ready for review:

• [x] Will the PR name make sense to users?
• [x] Does the PR have a priority label?
• [x] Have you added or updated tests?
• [x] Is the documentation up to date?

For significant changes:

• [x] Is there a summary in the CHANGELOG?
• [x] Can these changes be split into multiple PRs?

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

Solution

• Use .unwrap_or_clone() instead to get a copy of the note commitment trees
• Update and return the local copy of the note commitment trees

Related Cleanups:

• Avoids creating a rayon scope if there are no note commitments in the block

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.

[arya2]

There was a known test failure on Windows: https://github.com/ZcashFoundation/zebra/actions/runs/8820859044/job/24215403949?pr=8459#step:12:4697

[mpguerra]

There was a known test failure on Windows: https://github.com/ZcashFoundation/zebra/actions/runs/8820859044/job/24215403949?pr=8459#step:12:4697

Do we need to disable some tests on Windows?

[upbqdn]

@arya2, what issue is this PR supposed to fix?

[arya2]

Do we need to disable some tests on Windows?

@mpguerra That seems like a good idea for now, I opened #8468 for disabling the test that's failing and #8461 for checking if it's failing because of a bug in the production code on Windows or if it's a bug in the test.

what issue is this PR supposed to fix?

@upbqdn It doesn't close any issues, I noticed this while double-checking that the z_get_treestate RPC method output includes notes in the block height at the hash or height provided as an argument, and it usually does, unless there are other Arc pointers to the same NoteCommitmentTree allocation.

Only the first commit is necessary, the second one, I thought for a moment that it would cause duplicate allocations for the same data, realized it won't because it only clones the NoteCommitmentTree if there are new notes that need to be appended, and added the comment for clarity around that.

Update:

This was not an issue, I misunderstood what make_mut() was doing, the new code is equivalent to what it was doing before.

We also noticed that it's always cloning the note commitment trees that are being updated, I'll see if I can fix that quickly in this PR.

[arya2]

This PR doesn't really change anything.