Add ZIP-221 (history tree) to finalized state

[conradoplg]

Motivation

ZIP-221 specifies a history tree used Heartwood-onward.

We must store the tree in the state to be able to validate the history tree for each block.

Specifications

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

Designs

Design was discussed in the issue: #2134

Solution

Instantiate a History Tree in the state on the Heartwood activation and keep it update with each commited block.

The tree is serialized with serde. This is something that I'd like feedback on. It was the simplest and fastest approach, but it seems we want to get rid of serde eventually (#2331). We can use byteorder and serialize manually instead. It should be fairly straightforward. Let me know if that's desirable.

Depends on #2531

Part of #2134

Review

ZIP-221 non-finalized state needs this but it does not prevent me from working on it. So it's not urgent, but should be done this sprint.

@teor2345 might want to review this since they already reviewed the (old) ZIP-221 non-finalized state PR.

Reviewer Checklist

• Check if the it syncs correctly using a CI workflow
• Provide feedback on the serialization strategy
• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

This does not do any validation. This will be done in a separate ticket (needs to be created).

---

This change is 

[teor2345]

I think we could merge this code as-is, as long as it works.
Have you run a Zebra sync up to Heartwood, and then up to the tip?

I've made a few other minor suggestions about documentation, log messages, and future tests. But they aren't blockers.

[teor2345]

The tree is serialized with serde. This is something that I'd like feedback on. It was the simplest and fastest approach, but it seems we want to get rid of serde eventually (#2331). We can use byteorder and serialize manually instead. It should be fairly straightforward. Let me know if that's desirable.

Right now our priority is writing quick code that works. But let's add it to the list of serde to get rid of in the serde ticket?

[conradoplg]

I think we could merge this code as-is, as long as it works.
Have you run a Zebra sync up to Heartwood, and then up to the tip?

Not yet, I was waiting to regenerate the cached state, but that is blocked now. I've manually deployed a node instead, based on this branch, and I'm waiting for that (and trying to find a way to access its status - I asked in the chat) Nervermind, found it, I'll update here with the status

[teor2345]

Since this is the first code that we're merging which calculates the history tree, I'd like us to validate the history tree root in the finalized state.

That way, our cached state tests will catch calculation bugs in this PR, by testing it on the entire mainnet and testnet chains.

We can only check Heartwood and Canopy for now, that's ok.

(Eventually, we'll need to return an error if validation fails, because this check is the first time Zebra validates checkpointed v5 transaction authorizing data.)

[teor2345]

Since this is the first code that we're merging which calculates the history tree, I'd like us to validate the history tree root in the finalized state.

That way, our cached state tests will catch calculation bugs in this PR, by testing it on the entire mainnet and testnet chains.

We can only check Heartwood and Canopy for now, that's ok.

(Eventually, we'll need to return an error if validation fails, because this check is the first time Zebra validates checkpointed v5 transaction authorizing data.)

Oh hang on, this will break a bunch of existing tests, until we fix the partial_chain proptest strategy. So let's open a separate ticket for this validation?

[conradoplg]

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

---

zebra-state/src/service/finalized_state.rs, line 272 at r1 (raw file):

Previously, teor2345 (teor) wrote…

I think we need to revert this change, but the reasoning is pretty complex. So I'd like you to double-check.

We'll also need to add comments documenting this information.

commit_finalized_direct handles both checkpointed and non-finalized blocks. (Needs docs.)

We can expect on the note commitment trees, because notes are bound by the non-malleable v5 transaction ID. (Needs docs.) And transaction IDs are bound to the transaction merkle root header field, block hash, chain of previous block hashes, and checkpoint list by the checkpoint verifier.

But we can't expect on the chain history hash, because the height data that's used in the chain history is only bound by the authorizing data commitment. (Needs docs?) Coinbase heights are stored as bitcoin scripts, so they're authorizing data:
https://zips.z.cash/zip-0244#a-1-transparent-scripts-digest

(There's an alternate height field in v5 transactions that's covered by the transaction ID. But Zebra doesn't check it yet, or use it for our block heights. See ticket #2387 for details.)

When we check the block commitment, we'll also need to be able to return an error here. Because it binds the authorizing data commitment, which binds the transaction authorizing data in the block. And if it's a checkpoint block, we haven't checked that data yet.

Also see my detailed answer here, starting with:

Unfortunately not, because we need to make sure that v5 transactions in checkpointed blocks haven't been modified

#2134 (comment)

Yes it's confusing... I think that there is a binding path:

authorization data -> authorizing data commitment -> hashBlockCommitments -> hashPrevBlock in the next block

So if we check the authorizing data commitment in

zebra/zebra-consensus/src/checkpoint.rs

Line 451 in 29a658b

fn check_block(&self, block: &Block) -> Result<block::Height, VerifyCheckpointError> {

it should work? Though that will require adding the commitment to the header as you've discussed previously with ECC.

...but in any case it's confusing, so it seems much better to just return an error, so I did that in 6bf9f71

---

zebra-state/src/service/finalized_state.rs, line 378 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Unfortunately, I don't think we can avoid returning errors, so we need to change this expect message.

Done in 6bf9f71

[conradoplg]

Oh hang on, this will break a bunch of existing tests, until we fix the partial_chain proptest strategy. So let's open a separate ticket for this validation?

Done in #2562

BTW I've also fixed a bug in this PR and I'll deploy a node again to test it.

[teor2345]

Reviewed 4 of 6 files at r1, 3 of 3 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @conradoplg)

---

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

// TODO: remove after this module gets to be used
#![allow(dead_code)]
#![allow(missing_docs)]

I'd like to keep requiring docs for all modules, but that's not a critical change.

I don't want to block on a docs lint, so I've added this module to the list in the docs issue #453

---

zebra-state/src/service/finalized_state.rs, line 272 at r1 (raw file):

Previously, conradoplg (Conrado Gouvea) wrote…

Yes it's confusing... I think that there is a binding path:

authorization data -> authorizing data commitment -> hashBlockCommitments -> hashPrevBlock in the next block

So if we check the authorizing data commitment in

zebra/zebra-consensus/src/checkpoint.rs

Line 451 in 29a658b

fn check_block(&self, block: &Block) -> Result<block::Height, VerifyCheckpointError> {

it should work? Though that will require adding the commitment to the header as you've discussed previously with ECC.

...but in any case it's confusing, so it seems much better to just return an error, so I did that in 6bf9f71

You're right, there is a binding path, but it needs a network protocol change.

We've offered to write that ZIP, but it's out of scope for NU5:
#2336

[teor2345]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @teor2345)

a discussion (no related file):
I don't want to approve this PR for merging until the node tests have passed.

---

[conradoplg]

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @teor2345)

a discussion (no related file):

Previously, teor2345 (teor) wrote…

I don't want to approve this PR for merging until the node tests have passed.

I've deployed a node and it seemed to synced correctly

"Aug 04 17:25:27.221  INFO {zebrad="7481dba" net="Main"}:sync:obtain_tips:state: zebra_state::util: created block locator tip_height=Height(1343350) min_locator_height=1343251 locators=[Height(1343350), Height(1343349), Height(1343348), Height(1343346), Height(1343342), Height(1343334), Height(1343318), Height(1343286), Height(1343251)]
"

without panics

---

---

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

Previously, teor2345 (teor) wrote…

I'd like to keep requiring docs for all modules, but that's not a critical change.

I don't want to block on a docs lint, so I've added this module to the list in the docs issue #453

I forgot to mention that this was added because of big_array!, I added this information in the ticket

---

zebra-state/src/service/finalized_state.rs, line 272 at r1 (raw file):

Previously, teor2345 (teor) wrote…

You're right, there is a binding path, but it needs a network protocol change.

We've offered to write that ZIP, but it's out of scope for NU5:
#2336

Got it!

[conradoplg]

I've also re-increased the database version since it was increased on main

[teor2345]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @conradoplg)