ZIP-221: Add Orchard support to history tree

[conradoplg]

Motivation

Orchard support for the history tree was postponed since librustzcash support wasn't ready yet.

It is now, and this adds the support for both HistoryTree (the "high-level" code) and Tree (the librustzcash wrapper)

Specifications

https://zips.z.cash/zip-0221

Designs

N/A

Solution

Change Tree to be a generic just like the underlying librustzcash primitive tree.
Change HistoryTree to use that (and abstract away the different versions).

This also:

• Handles network upgrades, which was a TODO
• Adds methods that will be useful for storing the tree in the state.

Closes #2283

Review

This will be needed by #2134, which hasn't started yet. Therefore it's not urgent, but should be reviewed this sprint.

One particular point I'd like feedback on is the code repetition in HistoryTree. I think we could use a trait and Box instead of the enum, but I'm not sure if that's a good solution. Another option would be to use a crate like enum_dispatch as suggested by @jvff . See this comment for reference.

The only reason this is a draft is because I'd like to add tests. But otherwise it's ready for review.

Reviewer Checklist

• Suggest a solution for avoiding code repetition, if any
• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

N/A

---

This change is 

[teor2345]

@dconnolly can you do the initial review here?

[dconnolly]

First pass review

[conradoplg]

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @conradoplg and @dconnolly)

---

zebra-chain/src/history_tree.rs, line 188 at r1 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

👀

I know 🙈

I was going to simply assign each field but that felt error-prone.

---

zebra-chain/src/primitives/zcash_history.rs, line 27 at r1 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

Instead of ignoring argument this below, should these types be Options?

I've considered that, but the issue is that we're keeping the Orchard tree all the way from the start of the state, even if it's not being used. So I think it we will never pass None anyway.

---

zebra-chain/src/primitives/zcash_history.rs, line 69 at r1 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

    /// Create a leaf Entry for the given block, its network, and the root of its
    /// Sapling or Orchard note commitment tree.

Improved in 9df206f - I went with a different wording because Orchard-onward you need to pass both Sapling and Orchard roots

---

zebra-chain/src/primitives/zcash_history.rs, line 276 at r1 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

panic, or error?

I'm assuming this will be implicitly checked by the state, which should only instantiate the history tree in Heartwood-onward and only push blocks with higher heights. I believe anything otherwise would be a programming error.

[teor2345]

Reviewed 4 of 5 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @conradoplg and @dconnolly)

---

zebra-chain/src/history_tree.rs, line 62 at r3 (raw file):
Typo

 /// The parameters must come from the values of [HistoryTree::size],

---

zebra-chain/src/primitives/zcash_history.rs, line 276 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Can remote nodes trigger this error?
Can Zebra recover from this error by rejecting a block, or is it a programmer bug?

Actually that makes sense (I didn't see your responses at first, because they were in Reviewable.)

---

zebra-chain/src/primitives/zcash_history.rs, line 244 at r3 (raw file):

    block: Arc<Block>,
    network: Network,
    sapling_root: &sapling::tree::Root,

It feels like this code is a bit confusing, because it returns an unnamed u64.

Would it be clearer if V2::block_to_history_node calls count_orchard_transactions directly?

[conradoplg]

Dismissed @dconnolly from a discussion.
Reviewable status: 3 of 6 files reviewed, 8 unresolved discussions (waiting on @conradoplg, @dconnolly, and @teor2345)

---

zebra-chain/src/history_tree.rs, line 188 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Probably worth documenting that the method replaces &mut self.

Done in ee7124a

---

zebra-chain/src/history_tree.rs, line 62 at r3 (raw file):

Previously, teor2345 (teor) wrote…

Typo

 /// The parameters must come from the values of [HistoryTree::size],

Done in ee7124a

---

zebra-chain/src/primitives/zcash_history.rs, line 27 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Or we could have different methods for pre-NU5 and post-NU5 trees, so it's easier to review the calling code.

(I don't have a lot of context here, so if this is just internal code, it might be ok.)

Since we'll start working on the finalized state for this soon, I'd prefer to postpone this decision since there we (or at least I 😅 ) will have a better understanding of the best API for this. But my hunch is that this is the easiest since there will always be a Orchard tree around, even if empty, so we can just pass its root.

But if we decide to address this now, I think Option is the way to go.

---

zebra-chain/src/primitives/zcash_history.rs, line 266 at r1 (raw file):

Previously, teor2345 (teor) wrote…

This feels like it could be a method on Block?
It probably doesn't matter much either way.

I like it - it would be useful in another PR too. Added in ee7124a

---

zebra-chain/src/primitives/zcash_history.rs, line 276 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Actually that makes sense (I didn't see your responses at first, because they were in Reviewable.)

Yep that's weird, Reviewable just posts all comments as a single comment in GitHub. I may go back to GitHub for conversations, but I'll keep using Reviewable here to avoid confusion.

---

zebra-chain/src/primitives/zcash_history.rs, line 319 at r1 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

    /// `orchard_root` is the root of the Orchard note commitment tree of the block.

Done in 2d20d11

---

zebra-chain/src/primitives/zcash_history.rs, line 244 at r3 (raw file):

Previously, teor2345 (teor) wrote…

It feels like this code is a bit confusing, because it returns an unnamed u64.

Would it be clearer if V2::block_to_history_node calls count_orchard_transactions directly?

That's indeed much better, I don't know what I was thinking 😅 Changed in ee7124a

[conradoplg]

Added tests in 38ca719 and moved from draft

[teor2345]

Reviewed 2 of 3 files at r4, 3 of 3 files at r6.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dconnolly)

---

zebra-chain/src/primitives/zcash_history.rs, line 27 at r1 (raw file):

Previously, conradoplg (Conrado Gouvea) wrote…

Since we'll start working on the finalized state for this soon, I'd prefer to postpone this decision since there we (or at least I 😅 ) will have a better understanding of the best API for this. But my hunch is that this is the easiest since there will always be a Orchard tree around, even if empty, so we can just pass its root.

But if we decide to address this now, I think Option is the way to go.

I think it's fine to go with a simpler API here, based on our previous choice to always have roots.

Oops, I didn't mean to approve this, let's wait for Deirdre

[conradoplg]

Improved tests in dae6c35 based on feedback from other PR with similar code

[dconnolly]

Reviewable status: 3 of 8 files reviewed, 3 unresolved discussions (waiting on @conradoplg and @teor2345)

---

zebra-chain/src/primitives/zcash_history.rs, line 27 at r1 (raw file):

Previously, teor2345 (teor) wrote…

I think it's fine to go with a simpler API here, based on our previous choice to always have roots.

👍

[dconnolly]

Reviewed 1 of 5 files at r1, 1 of 3 files at r4, 1 of 3 files at r6, 1 of 1 files at r7, 2 of 3 files at r9, 1 of 1 files at r10, 1 of 1 files at r12.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @conradoplg)

[conradoplg]

Dismissed @dconnolly from 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @conradoplg)

---

zebra-chain/src/history_tree.rs, line 18 at r10 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

Ooo nice! I like that

Done.

---

zebra-chain/src/history_tree.rs, line 62 at r10 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

    /// Recreate a [`HistoryTree`] from previously saved data.

Done.

---

zebra-chain/src/primitives/zcash_history.rs, line 27 at r1 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

👍

Done.