Track anchors and note commitment trees in zebra-state #2458

dconnolly · 2021-07-07T05:43:47Z

Motivation

In order to maintain shielded note commitment trees, we will need to update our state for non-finalized and finalized chains, keep those trees up to date in the non-finalized state as we validate blocks, and save down the serialized trees to finalized state for the tip, and update our validated anchor sets as we update the trees.

Specifications

Designs

#2425

Solution

Depends on #2407
Resolves #1320

Review

Reviewer Checklist

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

Follow Up Work

Use the new state requests to look up anchors during transaction validation.

This change is

teor2345

This PR also contains changes from #2407.

We'll need to rebase on #2407 and change the merge base before we can do a review.

teor2345

This PR looks ok so far, but we will probably want to make the following changes before we merge:

split the sprout-specific code into another PR, because there are no sprout note commitment trees yet
add tests

zebra-state/src/service/non_finalized_state/chain.rs

conradoplg

I've addressed comments and advanced a bit on the implementation.

There is one point in particular that I'd like feedback on, I've mentioned in the design ticket. But I think the first approach I've mentioned is being used for nullifiers too so I think it seems the best approach. You can check aee5e8f specifically to validate if the approach makes sense.

Current status:
I implemented the finalized state handling of Orchard anchors and trees. I implemented serde::(De)Serialize for the Orchard tree node (to allow using incrementalmerkletree serialization) and still need to call it from (Into)FromDisk. Because of that tests are currently failing.

zebra-state/src/service/non_finalized_state/chain.rs

…ed data

…SplitData

…tTrees

Append note commitments to their trees when doing update_chain_state_with, then use the resulting Sapling and Orchard roots to pass to history_tree, and add new roots to the anchor sets.

…tree in revert_chain_state_with

…inalize state

…trees

…n Chain

…t require height

dconnolly · 2021-07-28T02:51:46Z

I think we're almost ready to merge, I just have 2 final questions:

How can we have sapling note commitments in block 1, before sapling activation?

Have we tested a full sync with the new state version, up to the current tip on testnet and mainnet?

On 1, I made a comment:
#2458 (comment)

dconnolly · 2021-07-28T02:52:41Z

I've just realized that this PR uses sapling_note_commitment_tree but 0005-state-updates.md uses sapling_incremental and similarly for Orchard. Which one is best?

Since we have multiple trees, I like the naming in this PR

dconnolly · 2021-07-28T03:00:34Z

I think we're almost ready to merge, I just have 2 final questions:

How can we have sapling note commitments in block 1, before sapling activation?

Have we tested a full sync with the new state version, up to the current tip on testnet and mainnet?

On 2, I've kicked off a single node sync for Mainnet (https://github.com/ZcashFoundation/zebra/actions/runs/1073455087) based on this branch. EDIT: synced at least to block height 1212205 on Mainnet https://console.cloud.google.com/logs/query;query=insertId%3D%221v1sdh9o692ecbd7a%22%20timestamp%3E%222021-07-27T12:22:17.617Z%22%20resource.type%3D%22gce_instance%22%20resource.labels.instance_id%3D%222142592117257369472%22%20resource.labels.zone%3D%22us-central1-a%22%20severity%3E%3DDEFAULT;timeRange=2021-07-28T12:21:07.000Z%2F2021-07-28T12:21:07.000Z--PT0S?authuser=1&project=zealous-zebra

dconnolly

Made it to block height 1333338 on mainnet, lgtm!

Reviewed 1 of 11 files at r1, 2 of 14 files at r2, 5 of 10 files at r5, 13 of 13 files at r8, 3 of 3 files at r9, 1 of 2 files at r10, 1 of 1 files at r11.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @conradoplg, @dconnolly, @jvff, and @teor2345)

dconnolly · 2021-07-28T17:54:53Z

@conradoplg since I originally opened this PR, I can't approve it, or commandeer it, but I'm good to merge, maybe @teor2345 can give final merge approval?

dconnolly · 2021-07-28T17:55:16Z

@conradoplg since I originally opened this PR, I can't approve it, or commandeer it, but I'm good to merge, maybe @teor2345 can give final merge approval?

Or I could do an admin override merge 😏
(oops accidental close)

conradoplg · 2021-07-28T18:36:03Z

@dconnolly thanks for testing that!

There's a pending request change from @teor2345, though you have already answered it. I'm fine with dismissing it if you think it's OK, or we can just wait for their approval.

teor2345 · 2021-07-28T21:09:36Z

@dconnolly thanks for testing that!

There's a pending request change from @teor2345, though you have already answered it. I'm fine with dismissing it if you think it's OK, or we can just wait for their approval.

I'd like to do a quick fix to the comments and test variable names to avoid confusion.

Also please remind @mpguerra to update the changelog, if you merge this PR before the release is tagged.

zebra-chain/src/sapling/tests/tree.rs

…on similar test

teor2345

Thanks this is great!

I'm having a bit of trouble keeping track of all the conversations and reviewable comments, if I've missed one, please merge anyway.

* Draft CHANGELOG for Zebra 1.0.0-alpha.14 * Add PR #2533 to CHANGELOG * Apply suggestions from code review Co-authored-by: teor <teor@riseup.net> * Remove entry about updating the changelog * move #2497 * add #2529 * Add a missing space * Add #2458, #2525, #2486, #2542 and #2539 to CHANGELOG Co-authored-by: teor <teor@riseup.net> Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>

* Add Orchard support to HistoryTree * Handle network upgrades in HistoryTree * Add additional methods to save/load HistoryTree * Apply suggestions from code review Co-authored-by: Deirdre Connolly <deirdre@zfnd.org> * Clarification of Entry documentation * Improvements from code review * Add HistoryTree tests * Improved test comments and variable names based on feedback from #2458 on similar test * Update zebra-chain/src/history_tree.rs Co-authored-by: Deirdre Connolly <deirdre@zfnd.org> * Use type aliases for V1 and V2 history trees Co-authored-by: Deirdre Connolly <deirdre@zfnd.org> Co-authored-by: teor <teor@riseup.net>

dconnolly added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code labels Jul 7, 2021

zfnd-bot bot assigned dconnolly Jul 7, 2021

teor2345 changed the base branch from main to incremental-note-commitment-trees July 8, 2021 02:08

teor2345 changed the base branch from incremental-note-commitment-trees to main July 8, 2021 02:08

teor2345 suggested changes Jul 8, 2021

View reviewed changes

conradoplg force-pushed the tree-state branch 2 times, most recently from 510c1af to efb6548 Compare July 15, 2021 21:14

conradoplg mentioned this pull request Jul 16, 2021

Validate note commitment trees in non-finalized state #2425

Closed

11 tasks

conradoplg reviewed Jul 16, 2021

View reviewed changes

zebra-state/src/service/non_finalized_state/chain.rs Show resolved Hide resolved

zebra-state/src/service/non_finalized_state/chain.rs Show resolved Hide resolved

zebra-state/src/service/non_finalized_state/chain.rs Outdated Show resolved Hide resolved

conradoplg force-pushed the tree-state branch 3 times, most recently from e17e547 to 423fd22 Compare July 20, 2021 23:00

dconnolly and others added 16 commits July 21, 2021 15:48

Tidy chain Cargo.toml

65e32db

Organize imports

7a457e3

Add method to get note commitments from all Actions in Orchard shield…

a2c8ec7

…ed data

Add method to get note commitments from all JoinSplits in Sprout Join…

096e3fc

…SplitData

Add Request and Response variants for awaiting anchors

062127a

Add anchors and note commitment trees to finalized state db

8036fa1

Add (From|Into)Disk impls for tree::Roots and stubs for NoteCommitmen…

8ab5a65

…tTrees

Track anchors and note commitment trees in Chain

e5a9e65

Append note commitments to their trees when doing update_chain_state_with, then use the resulting Sapling and Orchard roots to pass to history_tree, and add new roots to the anchor sets.

Handle errors when appending to note commitment trees

8090cbe

Add comments explaining why note commitment are not removed from the …

452a36c

…tree in revert_chain_state_with

Implementing note commitments in finalized state

7e045ce

Finish serialization of Orchard tree; remove old tree when updating f…

cb56568

…inalize state

Add serialization and finalized state updates for Sprout and Sapling …

12eb3c2

…trees

Partially handle trees in non-finalized state. Use Option for trees i…

4e181eb

…n Chain

Rebuild trees when forking; change finalized state tree getters to no…

b2559cd

…t require height

Pass empty trees to tests; use empty trees by default in Chain

7020e04

dconnolly commented Jul 28, 2021

View reviewed changes

dconnolly requested a review from teor2345 July 28, 2021 17:53

dconnolly closed this Jul 28, 2021

dconnolly reopened this Jul 28, 2021

Merge branch 'main' into tree-state

9703a39

Improve comments and structure of the test

5ae1c53

dconnolly commented Jul 28, 2021

View reviewed changes

zebra-chain/src/sapling/tests/tree.rs Outdated Show resolved Hide resolved

conradoplg added a commit that referenced this pull request Jul 28, 2021

Improved test comments and variable names based on feedback from #2458 …

dae6c35

…on similar test

Improve variable names again

637f86d

teor2345 previously approved these changes Jul 28, 2021

View reviewed changes

dconnolly mentioned this pull request Jul 28, 2021

CHANGELOG for Zebra 1.0.0-alpha.14 #2538

Merged

6 tasks

teor2345 and others added 3 commits July 29, 2021 10:40

Merge branch 'main' into tree-state

bf25af2

Merge branch 'main' into tree-state

4a131dd

Merge branch 'main' into tree-state

7eea269

dconnolly dismissed teor2345’s stale review via 7eea269 July 29, 2021 13:17

Rustfmt

816e340

dconnolly merged commit e719c46 into main Jul 29, 2021

dconnolly deleted the tree-state branch July 29, 2021 13:37

dconnolly mentioned this pull request Jul 29, 2021

Implement incremental, sparse note commitment trees for Sapling and Orchard #1287

Closed

2 tasks

mpguerra added a commit that referenced this pull request Jul 29, 2021

Add #2458, #2525, #2486, #2542 and #2539 to CHANGELOG

6495925

mpguerra mentioned this pull request Nov 9, 2021

Validate Sprout anchors in non-finalized state #3043

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track anchors and note commitment trees in zebra-state #2458

Track anchors and note commitment trees in zebra-state #2458

dconnolly commented Jul 7, 2021 •

edited

Loading

teor2345 left a comment

teor2345 left a comment

conradoplg left a comment

dconnolly commented Jul 28, 2021

dconnolly commented Jul 28, 2021

dconnolly commented Jul 28, 2021 •

edited

Loading

dconnolly left a comment

dconnolly commented Jul 28, 2021

dconnolly commented Jul 28, 2021 •

edited

Loading

conradoplg commented Jul 28, 2021

teor2345 commented Jul 28, 2021

teor2345 left a comment

Track anchors and note commitment trees in zebra-state #2458

Track anchors and note commitment trees in zebra-state #2458

Conversation

dconnolly commented Jul 7, 2021 • edited Loading

Motivation

Specifications

Designs

Solution

Review

Reviewer Checklist

Follow Up Work

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

conradoplg left a comment

Choose a reason for hiding this comment

dconnolly commented Jul 28, 2021

dconnolly commented Jul 28, 2021

dconnolly commented Jul 28, 2021 • edited Loading

dconnolly left a comment

Choose a reason for hiding this comment

dconnolly commented Jul 28, 2021

dconnolly commented Jul 28, 2021 • edited Loading

conradoplg commented Jul 28, 2021

teor2345 commented Jul 28, 2021

teor2345 left a comment

Choose a reason for hiding this comment

dconnolly commented Jul 7, 2021 •

edited

Loading

dconnolly commented Jul 28, 2021 •

edited

Loading

dconnolly commented Jul 28, 2021 •

edited

Loading