Calculate incremental note commitment trees

[dconnolly]

Motivation

We need to compute Orchard, Sapling, and Sprout note commitment trees incrementally (and thus their roots) as note commitments are produced as part of shielded transactions on the blockchain. We can then use these updatable data structures to check that anchors in future transactions are pointing to the roots of note commitment trees we have observed and computed ourselves before.

Specifications

Implementing an incremental merkle tree is not directly specified in the spec but the MerkleCRH hash functions are:

https://zips.z.cash/protocol/protocol.pdf#merklecrh
https://zips.z.cash/protocol/protocol.pdf#merklepath

Designs

The draft Treestate design RFC is related: https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/drafts/0005-treestate.md

Solution

Implement the Sapling and Orchard incremental note commitment trees, supporting append(), and checking conformance with empty root test vectors and incrementally added note commitments test vectors.

Resolves the sapling and orchard parts of #1287

Review

#2425 relies on this

Reviewer Checklist

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

Follow Up Work

Sprout

[teor2345]

Thanks for this PR.

My top priority is making sure we have good Orchard test coverage, and that we've fixed any potential bugs. (Or explained why they can't happen.)

I've also made a bunch of comments about code readability, but they are a lower priority.

[mpguerra]

@dconnolly: does this PR also cover #1320? I remember we discussed something along those lines previously but I'm not 100% sure

[conradoplg]

@dconnolly just to confirm: is the PR ready, and I'll just need to address the comments, or is some part of it incomplete?

Also I'm bit confused about the context, the PR basically replaces and old implementation. What was that old implementation? Just a placeholder?

[teor2345]

@dconnolly: does this PR also cover #1320? I remember we discussed something along those lines previously but I'm not 100% sure

This PR only calculates the incremental note commitment trees for sapling and orchard.

Here is the remaining work:

• calculate the incremental note commitment tree for sprout (part of Implement incremental, sparse note commitment trees for Sapling and Orchard #1287)
• update all 3 incremental note commitment trees in the state (finishes Implement incremental, sparse note commitment trees for Sapling and Orchard #1287)
• update anchors in the state (Add sapling and orchard anchors to zebra-state #1320), when we:
  • add a block to a non-finalized chain
  • fork a non-finalized chain
  • add a block to the finalized tip
• query and validate anchors in the transaction verifier (new ticket?)

The design for #1320 will be very similar to the chain history tree, but we won't need to validate anchors in the state.

[teor2345]

Also I'm bit confused about the context, the PR basically replaces and old implementation. What was that old implementation? Just a placeholder?

It was a buggy implementation based on an incorrect understanding of the spec (or possible an unclear spec).

[str4d]

FYI we've published (and are using in zcashd) the incrementalmerkletree crate, which you could either use directly or leverage in tests for cross-checking.

[teor2345]

Most of this PR was copied from librustzcash, specifically https://github.com/zcash/librustzcash/blob/4c4d226f404aef64df6a77caa289b30cc875ae4f/zcash_primitives/src/merkle_tree.rs#L91

The code looks ok, but I'm a bit concerned we now have 3 copies of almost the same incremental merkle tree code:

• a generic implementation in librustzcash,
• a Zebra Sapling implementation, and
• a Zebra Orchard implementation.

There is also a risk that we might have introduced bugs by copying and modifying the librustzcash code. So we'll need to add tests for Zebra's Orchard note commitments.

All these extra risks and tests make me wonder if we should just import librustzcash, like we did for the history trees.

We made a decision to do our own design and implementation a few weeks ago. But I didn't realise we were going to copy so much librustzcash code. So I'm not sure how to move forward here.

What is the fastest way to correctly implement sapling and orchard anchors?
(I don't want to spend a lot of time on this, because it's blocking other work.)

[conradoplg]

Added the suggestions and answered some comments, but haven't changed the code yet.

I've studied the code and incrementalmerkletree too and now I understand it much better.

I've managed to expand the Sapling test in this PR to also use incrementalmerkletree and compare the result and it works. So I think it will be really quick to change this PR to use it instead.

I'll start working on that.

[conradoplg]

I've added the test in 42a8d6f if you want to take a look. It's a bit ugly, it was just to validate the idea.

[teor2345]

I've added the test in 42a8d6f if you want to take a look. It's a bit ugly, it was just to validate the idea.

This looks great!

Looking forward to seeing the final PR.

[conradoplg]

Update: I've changed it to use incrementalmerkletree but I need to solve a bug. No need to review this right now unless you want to.

[conradoplg]

I've updated the code. This should be now ready for review.

• Solved the bug; it was a confusion between "layers" (what the spec uses, root is layer 0) and "levels" or Altitudes (what incrementalmerkletree uses, root level is 31).
• In an attempt to make that clearer, I've changed the code such that we use layers everywhere and convert any levels to layers when needed. Not sure if I succeeded 😅
• I've incorporated the recent spec change to MerkleCRH^Orchard that encodes "null" values as a zero byte vector, which simplifies some of the code by avoiding Options
• I've added test vectors for Orchard from zcash-test-vectors
• I've tried to organized the tests by moving them (and the vectors) to their own files. But the vectors.rs / test_vectors.rs split is a bit weird. Suggestions are welcome. (Maybe move test_vectors.rs to zebra-tests?

[dconnolly]

I've updated the code. This should be now ready for review.

• Solved the bug; it was a confusion between "layers" (what the spec uses, root is layer 0) and "levels" or Altitudes (what incrementalmerkletree uses, root level is 31).
• In an attempt to make that clearer, I've changed the code such that we use layers everywhere and convert any levels to layers when needed. Not sure if I succeeded 😅
• I've incorporated the recent spec change to MerkleCRH^Orchard that encodes "null" values as a zero byte vector, which simplifies some of the code by avoiding Options
• I've added test vectors for Orchard from zcash-test-vectors
• I've tried to organized the tests by moving them (and the vectors) to their own files. But the vectors.rs / test_vectors.rs split is a bit weird. Suggestions are welcome. (Maybe move test_vectors.rs to zebra-tests?

Levels, layers, leaves, altitudes, positions, 😵

[conradoplg]

Comments addressed, thanks!

[teor2345]

Looks great, let's merge.

(We can tweak the module structure later as needed.)

[conradoplg]

I took the liberty of resolving the remaining of @dconnolly's comments since: they were small; I believe they were properly addressed; @teor2345 approved the PR; and I don't want to bother her vacations just for those 🤣