change(state): Deduplicate note commitment trees in non-finalized state

[upbqdn]

Motivation

Address a part of #4784.

Solution

This PR stores only the first tree in a series of identical trees in the non-finalized state.

Review

This PR doesn't contain any additional tests. If reviewers have suggestions for suitable tests, I'd be glad to hear them.

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?

[teor2345]

It looks like this contains a copy of PR #7218. Do you mind if I change the PR base to make the diff smaller to review?

(I'm a big fan of this PR and I'm glad we're getting it done.)

[upbqdn]

This PR is built atop #7218, so we should merge that first. I forgot to target that branch instead of main. I don't think I can change it now.

[teor2345]

This PR is built atop #7218, so we should merge that first. I forgot to target that branch instead of main. I don't think I can change it now.

The base branch can be changed using the PR name edit button. (It's a bit weird but it works.)

[upbqdn]

Excellent, I thought you couldn't change the base branch. It turns out you can.

[upbqdn]

(I wrote the two comments above at the same time as you posted yours, so they might seem a bit off.)

[teor2345]

This all looks pretty good!

I want to check we handle chain forks correctly, is there a test we can do to make sure it works?

I think we have a lot of unit and proptest coverage already. But we could manually query the last 100 or so heights using the RPC, and do it in a loop?

I don't know if we need to check the RPC responses against zcashd or an unmodified Zebra. Do you think it's more important to test for panics or wrong data?

[upbqdn]

Do you think it's more important to test for panics or wrong data?

Both would have a severe impact, so they seem equally important to me.

I'll try the RPC on the last 100 blocks. I'll compare it to an unmodified Zebra.

[teor2345]

Thanks!

If you have a script I'm happy to run it as well, it would be good to do a test during a fork.

[dconnolly]

generally looks good, just one q about panics vs returning errors around Height

The base branch was changed.

[upbqdn]

I synced the last ~ 1% of the Mainnet chain with this PR with no issues. Then, I got the tip with

zcash-cli getblockcount

which was 2160512. Then, I got the Sapling and Orchard treestates of the last 200 blocks with

for height in $(seq 2160312 2160512); do
    zcash-cli z_gettreestate "${height}"
done

Then, I switched to the main branch and collected the treestates of the same 200 blocks again. Zebra was still at the same tip after I collected the second batch. I diffed them, and there was no difference.

[upbqdn]

I want to check we handle chain forks correctly, is there a test we can do to make sure it works?

This test is passing https://github.com/zcashfoundation/zebra/blob/3e75cb50f66ac783a477eb0db92529e88df69115/zebra-state/src/service/non_finalized_state/tests/prop.rs#L117-L131. It checks that a forked chain has the same state, including note commitment trees (https://github.com/zcashfoundation/zebra/blob/3e75cb50f66ac783a477eb0db92529e88df69115/zebra-state/src/service/non_finalized_state/tests/prop.rs#L224 and https://github.com/zcashfoundation/zebra/blob/3e75cb50f66ac783a477eb0db92529e88df69115/zebra-state/src/service/non_finalized_state/chain.rs#L268-L270), as the original chain after pushing the same blocks.

[teor2345]

Thanks for the tests, that seems like a comprehensive check.