ZIP-221: Validate chain history commitments in the non-finalized state

[conradoplg]

Motivation

ZIP-221 specifies that the block header must have a commitment to the chain history. This implements that validation logic for non-finalized state.

Specifications

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

Designs

Following the generic design from https://zebra.zfnd.org/dev/rfcs/0005-state-updates.html

Solution

Depends on #2396, which was split off from here.

This adds a new step in the contextual validation logic that is invoked after a Chain is obtained. The Chain keeps a HistoryTree reference to keep track of the history tree and validate if incoming blocks have the correct commitment.

Review

@teor2345 reviewed the draft PR and will probably want to finish reviewing this

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

Part of #2135
Proper Orchard support will be added later, see #2396.

[teor2345]

This looks like a good start, I've suggested some ways we could handle the state and errors.

[conradoplg]

I've expanded the implementation. Parts I'm currently blocked on:

• How to solve the borrowing issue in validate_and_commit
• Which specific types to use in mmr.rs (which bridges zebra-state and zcash_history). The types (blocks and block iterators) that come from zebra-state are a bit different from the types we've used in zcash_history. Should we use the same for both (which ones), or should we convert between them?

[teor2345]

As a general change, we might want to rename MerkleMountainRange and mmr to include the word history.

I also wonder if the mmr module belongs in zebra-chain, it seems like a primitive that the finalized and non-finalized state will want to use.

[teor2345]

* Which specific types to use in `mmr.rs` (which bridges `zebra-state` and `zcash_history`). The types (blocks and block iterators) that come from `zebra-state` are a bit different from the types we've used in `zcash_history`. Should we use the same for both (which ones), or should we convert between them?

In general I'd like to use IntoIterator<Item = Arc<Block>>, but I'm not too concerned Item becomes &Block, because we're not actually storing any of those blocks in the Tree.

Anywhere we store blocks, Arc<Block> is a requirement to reduce memory usage.

[conradoplg]

Another round of changes.

I'm not currently blocked and can keep on working on this, so feel free to delay any feedback if needed.

As a general change, we might want to rename MerkleMountainRange and mmr to include the word history.

I also wonder if the mmr module belongs in zebra-chain, it seems like a primitive that the finalized and non-finalized state will want to use

I've changed MerkleMountainRange to HistoryTree and mmr variables to history_tree.

Do you think mmr.rs should be moved to zebra-state instead? (Also I'll rename it to history_tree.rs too)

[conradoplg]

@teor2345 could you please take another look?

When doing the proper error handling I had to propagate a lot of stuff, and I'm not sure if there is a better way to handle everything.

Maybe considering carefully in which cases librustzcash can return errors, we could .expect() most of the errors since they don't seem to be possible in the "regular" use of the tree. But it seems risky.

What's missing for now:

• Pruning the tree
• Final commitment computation (that's actually ZIP-244 but it seems to make sense to include it here. Also it's relatively simple)

[conradoplg]

Some leftover fixes form the last review

[dconnolly]

Looks great! 🎉

[teor2345]

@conradoplg this needs a rebase after merging another PR, then we can merge.

[teor2345]

Let's merge after the rebase on main is done

[teor2345]

(Coverage builds are failing due to crates.io network issues.)