Fix interstitial sprout anchors check

.github/workflows/test.yml

@@ -54,7 +54,7 @@ jobs:
           --container-image rust:buster \
           --container-mount-disk mount-path='/mainnet',name="zebrad-cache-$SHORT_SHA-mainnet-canopy" \
           --container-restart-policy never \
-          --create-disk name="zebrad-cache-$SHORT_SHA-mainnet-canopy",image=zebrad-cache-13c6a826-mainnet-canopy \
+          --create-disk name="zebrad-cache-$SHORT_SHA-mainnet-canopy",image=zebrad-cache-1558f3378-mainnet-canopy \
           --machine-type n2-standard-8 \
           --service-account cos-vm@zealous-zebra.iam.gserviceaccount.com \
           --scopes cloud-platform \

book/src/dev/rfcs/0005-state-updates.md

@@ -600,29 +600,29 @@ order on byte strings is the numeric ordering).
 
 We use the following rocksdb column families:
 
-| Column Family                  | Keys                   | Values                               | Updates |
-|--------------------------------|------------------------|--------------------------------------|---------|
-| `hash_by_height`               | `block::Height`        | `block::Hash`                        | Never   |
-| `height_tx_count_by_hash`      | `block::Hash`          | `BlockTransactionCount`              | Never   |
-| `block_header_by_height`       | `block::Height`        | `block::Header`                      | Never   |
-| `tx_by_location`               | `TransactionLocation`  | `Transaction`                        | Never   |
-| `hash_by_tx`                   | `TransactionLocation`  | `transaction::Hash`                  | Never   |
-| `tx_by_hash`                   | `transaction::Hash`    | `TransactionLocation`                | Never   |
-| `utxo_by_outpoint`             | `OutLocation`          | `transparent::Output`                | Delete  |
-| `balance_by_transparent_addr`  | `transparent::Address` | `Amount \|\| FirstOutLocation`       | Update  |
-| `utxo_by_transparent_addr_loc` | `FirstOutLocation`     | `AtLeastOne<OutLocation>`            | Up/Del  |
-| `tx_by_transparent_addr_loc`   | `FirstOutLocation`     | `AtLeastOne<TransactionLocation>`    | Append  |
-| `sprout_nullifiers`            | `sprout::Nullifier`    | `()`                                 | Never   |
-| `sprout_anchors`               | `sprout::tree::Root`   | `()`                                 | Never   |
-| `sprout_note_commitment_tree`  | `block::Height`        | `sprout::tree::NoteCommitmentTree`   | Delete  |
-| `sapling_nullifiers`           | `sapling::Nullifier`   | `()`                                 | Never   |
-| `sapling_anchors`              | `sapling::tree::Root`  | `()`                                 | Never   |
-| `sapling_note_commitment_tree` | `block::Height`        | `sapling::tree::NoteCommitmentTree`  | Delete  |
-| `orchard_nullifiers`           | `orchard::Nullifier`   | `()`                                 | Never   |
-| `orchard_anchors`              | `orchard::tree::Root`  | `()`                                 | Never   |
-| `orchard_note_commitment_tree` | `block::Height`        | `orchard::tree::NoteCommitmentTree`  | Delete  |
-| `history_tree`                 | `block::Height`        | `NonEmptyHistoryTree`                | Delete  |
-| `tip_chain_value_pool`         | `()`                   | `ValueBalance`                       | Update  |
+| Column Family                  | Keys                   | Values                              | Updates |
+| ------------------------------ | ---------------------- | ----------------------------------- | ------- |
+| `hash_by_height`               | `block::Height`        | `block::Hash`                       | Never   |
+| `height_tx_count_by_hash`      | `block::Hash`          | `BlockTransactionCount`             | Never   |
+| `block_header_by_height`       | `block::Height`        | `block::Header`                     | Never   |
+| `tx_by_location`               | `TransactionLocation`  | `Transaction`                       | Never   |
+| `hash_by_tx`                   | `TransactionLocation`  | `transaction::Hash`                 | Never   |
+| `tx_by_hash`                   | `transaction::Hash`    | `TransactionLocation`               | Never   |
+| `utxo_by_outpoint`             | `OutLocation`          | `transparent::Output`               | Delete  |
+| `balance_by_transparent_addr`  | `transparent::Address` | `Amount \|\| FirstOutLocation`      | Update  |
+| `utxo_by_transparent_addr_loc` | `FirstOutLocation`     | `AtLeastOne<OutLocation>`           | Up/Del  |
+| `tx_by_transparent_addr_loc`   | `FirstOutLocation`     | `AtLeastOne<TransactionLocation>`   | Append  |
+| `sprout_nullifiers`            | `sprout::Nullifier`    | `()`                                | Never   |
+| `sprout_anchors`               | `sprout::tree::Root`   | `sprout::tree::NoteCommitmentTree`  | Never   |
+| `sprout_note_commitment_tree`  | `block::Height`        | `sprout::tree::NoteCommitmentTree`  | Delete  |
+| `sapling_nullifiers`           | `sapling::Nullifier`   | `()`                                | Never   |
+| `sapling_anchors`              | `sapling::tree::Root`  | `()`                                | Never   |
+| `sapling_note_commitment_tree` | `block::Height`        | `sapling::tree::NoteCommitmentTree` | Delete  |
+| `orchard_nullifiers`           | `orchard::Nullifier`   | `()`                                | Never   |
+| `orchard_anchors`              | `orchard::tree::Root`  | `()`                                | Never   |
+| `orchard_note_commitment_tree` | `block::Height`        | `orchard::tree::NoteCommitmentTree` | Delete  |
+| `history_tree`                 | `block::Height`        | `NonEmptyHistoryTree`               | Delete  |
+| `tip_chain_value_pool`         | `()`                   | `ValueBalance`                      | Update  |
 
 Zcash structures are encoded using `ZcashSerialize`/`ZcashDeserialize`.
 Other structures are encoded using `IntoDisk`/`FromDisk`.
@@ -753,15 +753,25 @@ So they should not be used for consensus-critical checks.
   It also includes the `TransactionLocation` from the `FirstOutLocation`.
   (This duplicate data is small, and helps simplify the code.)
 
-- Each incremental tree consists of nodes for a small number of peaks.
-  Peaks are written once, then deleted when they are no longer required.
-  New incremental tree nodes can be added each time the finalized tip changes,
-  and unused nodes can be deleted.
-  We only keep the nodes needed for the incremental tree for the finalized tip.
-  TODO: update this description based on the incremental merkle tree code
-
-- The history tree indexes its peaks using blocks since the last network upgrade.
-  But we map those peak indexes to heights, to make testing and debugging easier.
+- Each `*_note_commitment_tree` stores the note commitment tree state
+  at the tip of the finalized state, for the specific pool. There is always
+  a single entry for those; they are indexed by height just to make testing
+  and debugging easier (so for each block committed, the old tree is
+  deleted and a new one is inserted by its new height). Each tree is stored
+  as a "Merkle tree frontier" which is basically a (logarithmic) subset of
+  the Merkle tree nodes as required to insert new items.
+
+- `history_tree` stores the ZIP-221 history tree state at the tip of the finalized
+  state. There is always a single entry for it; it is indexed by height just
+  to make testing and debugging easier. The tree is stored as the set of "peaks"
+  of the "Merkle mountain range" tree structure, which is what is required to
+  insert new items.
+
+- Each `*_anchors` stores the anchor (the root of a Merkle tree) of the note commitment
+  tree of a certain block. We only use the keys since we just need the set of anchors,
+  regardless of where they come from. The exception is `sprout_anchors` which also maps
+  the anchor to the matching note commitment tree. This is required to support interstitial
+  treestates, which are unique to Sprout.
 
 - The value pools are only stored for the finalized tip.
 

zebra-state/src/constants.rs

@@ -18,7 +18,7 @@ pub use zebra_chain::transparent::MIN_TRANSPARENT_COINBASE_MATURITY;
 pub const MAX_BLOCK_REORG_HEIGHT: u32 = MIN_TRANSPARENT_COINBASE_MATURITY - 1;
 
 /// The database format version, incremented each time the database format changes.
-pub const DATABASE_FORMAT_VERSION: u32 = 11;
+pub const DATABASE_FORMAT_VERSION: u32 = 12;
 
 /// The maximum number of blocks to check for NU5 transactions,
 /// before we assume we are on a pre-NU5 legacy chain.

zebra-state/src/service/check/anchors.rs

@@ -1,7 +1,7 @@
 //! Checks for whether cited anchors are previously-computed note commitment
 //! tree roots.
 
-use std::collections::HashSet;
+use std::collections::HashMap;
 
 use zebra_chain::sprout;
 
@@ -10,34 +10,14 @@ use crate::{
     PreparedBlock, ValidateContextError,
 };
 
-/// Check that the Sprout, Sapling, and Orchard anchors specified by
+/// Checks that the Sprout, Sapling, and Orchard anchors specified by
 /// transactions in this block have been computed previously within the context
-/// of its parent chain. We do not check any anchors in checkpointed blocks, which avoids
-/// JoinSplits<BCTV14Proof>
+/// of its parent chain. We do not check any anchors in checkpointed blocks,
+/// which avoids JoinSplits<BCTV14Proof>
 ///
 /// Sprout anchors may refer to some earlier block's final treestate (like
 /// Sapling and Orchard do exclusively) _or_ to the interstisial output
 /// treestate of any prior `JoinSplit` _within the same transaction_.
-///
-/// > For the first JoinSplit description of a transaction, the anchor MUST be
-/// > the output Sprout treestate of a previous block.[^sprout]
-///
-/// > The anchor of each JoinSplit description in a transaction MUST refer to
-/// > either some earlier block’s final Sprout treestate, or to the interstitial
-/// > output treestate of any prior JoinSplit description in the same transaction.[^sprout]
-///
-/// > The anchor of each Spend description MUST refer to some earlier
-/// > block’s final Sapling treestate. The anchor is encoded separately in
-/// > each Spend description for v4 transactions, or encoded once and
-/// > shared between all Spend descriptions in a v5 transaction.[^sapling]
-///
-/// > The anchorOrchard field of the transaction, whenever it exists (i.e. when
-/// > there are any Action descriptions), MUST refer to some earlier block’s
-/// > final Orchard treestate.[^orchard]
-///
-/// [^sprout]: <https://zips.z.cash/protocol/protocol.pdf#joinsplit>
-/// [^sapling]: <https://zips.z.cash/protocol/protocol.pdf#spendsandoutputs>
-/// [^orchard]: <https://zips.z.cash/protocol/protocol.pdf#actions>
 #[tracing::instrument(skip(finalized_state, parent_chain, prepared))]
 pub(crate) fn anchors_refer_to_earlier_treestates(
     finalized_state: &FinalizedState,
@@ -46,55 +26,80 @@ pub(crate) fn anchors_refer_to_earlier_treestates(
 ) -> Result<(), ValidateContextError> {
     for transaction in prepared.block.transactions.iter() {
         // Sprout JoinSplits, with interstitial treestates to check as well.
-        //
-        // The FIRST JOINSPLIT in a transaction MUST refer to the output treestate
-        // of a previous block.
         if transaction.has_sprout_joinsplit_data() {
-            // > The anchor of each JoinSplit description in a transaction MUST refer to
-            // > either some earlier block’s final Sprout treestate, or to the interstitial
-            // > output treestate of any prior JoinSplit description in the same transaction.
-            //
-            // https://zips.z.cash/protocol/protocol.pdf#joinsplit
-            let mut interstitial_roots: HashSet<sprout::tree::Root> = HashSet::new();
-
-            let mut interstitial_note_commitment_tree = parent_chain.sprout_note_commitment_tree();
+            let mut interstitial_trees: HashMap<
+                sprout::tree::Root,
+                sprout::tree::NoteCommitmentTree,
+            > = HashMap::new();
 
             for joinsplit in transaction.sprout_groth16_joinsplits() {
-                // Check all anchor sets, including the one for interstitial anchors.
+                // Check all anchor sets, including the one for interstitial
+                // anchors.
                 //
-                // Note that [`interstitial_roots`] is always empty in the first
-                // iteration of the loop. This is because:
+                // The anchor is checked and the matching tree is obtained,
+                // which is used to create the interstitial tree state for this
+                // JoinSplit:
                 //
-                // > "The anchor of each JoinSplit description in a transaction
-                // > MUST refer to [...] to the interstitial output treestate of
-                // > any **prior** JoinSplit description in the same transaction."
-                if !parent_chain.sprout_anchors.contains(&joinsplit.anchor)
-                    && !finalized_state.contains_sprout_anchor(&joinsplit.anchor)
-                    && (!interstitial_roots.contains(&joinsplit.anchor))
-                {
-                    // TODO: This check fails near:
-                    // - mainnet block 1_047_908
-                    //   with anchor 019c435cd1e8aca9a4165f7e126ac6e548952439d50213f4d15c546df9d49b61
-                    // - testnet block 1_057_737
-                    //   with anchor 3ad623811ffa4fe8498b23f3d6bb4e086dca32269afef6c8e572fd9ee6d0c0ea
-                    //
-                    // Restore after finding the cause and fixing it.
-                    // return Err(ValidateContextError::UnknownSproutAnchor {
-                    //     anchor: joinsplit.anchor,
-                    // });
-                    tracing::warn!(?joinsplit.anchor, ?prepared.height, ?prepared.hash, "failed to find sprout anchor")
-                }
+                // > For each JoinSplit description in a transaction, an
+                // > interstitial output treestate is constructed which adds the
+                // > note commitments and nullifiers specified in that JoinSplit
+                // > description to the input treestate referred to by its
+                // > anchor. This interstitial output treestate is available for
+                // > use as the anchor of subsequent JoinSplit descriptions in
+                // > the same transaction.
+                //
+                // <https://zips.z.cash/protocol/protocol.pdf#joinsplit>
+                //
+                // # Consensus
+                //
+                // > The anchor of each JoinSplit description in a transaction
+                // > MUST refer to either some earlier block’s final Sprout
+                // > treestate, or to the interstitial output treestate of any
+                // > prior JoinSplit description in the same transaction.
+                //
+                // > For the first JoinSplit description of a transaction, the
+                // > anchor MUST be the output Sprout treestate of a previous
+                // > block.
+                //
+                // <https://zips.z.cash/protocol/protocol.pdf#joinsplit>
+                //
+                // Note that in order to satisfy the latter consensus rule above,
+                // [`interstitial_trees`] is always empty in the first iteration
+                // of the loop.
+                let input_tree = interstitial_trees
+                    .get(&joinsplit.anchor)
+                    .cloned()
+                    .or_else(|| {
+                        parent_chain
+                            .sprout_trees_by_anchor
+                            .get(&joinsplit.anchor)
+                            .cloned()
+                            .or_else(|| {
+                                finalized_state
+                                    .sprout_note_commitment_tree_by_anchor(&joinsplit.anchor)
+                            })
+                    });
+
+                let mut input_tree = match input_tree {
+                    Some(tree) => tree,
+                    None => {
+                        tracing::warn!(?joinsplit.anchor, ?prepared.height, ?prepared.hash, "failed to find sprout anchor");
+                        return Err(ValidateContextError::UnknownSproutAnchor {
+                            anchor: joinsplit.anchor,
+                        });
+                    }
+                };
 
                 tracing::debug!(?joinsplit.anchor, "validated sprout anchor");
 
                 // Add new anchors to the interstitial note commitment tree.
                 for cm in joinsplit.commitments {
-                    interstitial_note_commitment_tree
+                    input_tree
                         .append(cm)
                         .expect("note commitment should be appendable to the tree");
                 }
 
-                interstitial_roots.insert(interstitial_note_commitment_tree.root());
+                interstitial_trees.insert(input_tree.root(), input_tree);
 
                 tracing::debug!(?joinsplit.anchor, "observed sprout anchor");
             }
@@ -103,6 +108,20 @@ pub(crate) fn anchors_refer_to_earlier_treestates(
         // Sapling Spends
         //
         // MUST refer to some earlier block’s final Sapling treestate.
+        //
+        // # Consensus
+        //
+        // > The anchor of each Spend description MUST refer to some earlier
+        // > block’s final Sapling treestate. The anchor is encoded separately
+        // > in each Spend description for v4 transactions, or encoded once and
+        // > shared between all Spend descriptions in a v5 transaction.
+        //
+        // <https://zips.z.cash/protocol/protocol.pdf#spendsandoutputs>
+        //
+        // This rule is also implemented in
+        // [`zebra_chain::sapling::shielded_data`].
+        //
+        // The "earlier treestate" check is implemented here.
         if transaction.has_sapling_shielded_data() {
             for anchor in transaction.sapling_anchors() {
                 tracing::debug!(?anchor, "observed sapling anchor");
@@ -120,6 +139,14 @@ pub(crate) fn anchors_refer_to_earlier_treestates(
         // Orchard Actions
         //
         // MUST refer to some earlier block’s final Orchard treestate.
+        //
+        // # Consensus
+        //
+        // > The anchorOrchard field of the transaction, whenever it exists
+        // > (i.e. when there are any Action descriptions), MUST refer to some
+        // > earlier block’s final Orchard treestate.
+        //
+        // <https://zips.z.cash/protocol/protocol.pdf#actions>
         if let Some(orchard_shielded_data) = transaction.orchard_shielded_data() {
             tracing::debug!(?orchard_shielded_data.shared_anchor, "observed orchard anchor");