zcash_client_backend: Ensure checkpoint at the end of each block.

This fixes an error wherein a note commitment in the last note
commitment position in a block was not being correctly marked as a
checkpoint.

This would occur when a block contained both Sapling and Orchard note
commitments, but the final transaction in the block contained only
either Sapling or Orchard note commitments, but not both.

Fixes #1302

zcash_client_backend/src/data_api/chain.rs

@@ -372,7 +372,7 @@ impl ChainState {
 /// ## Panics
 ///
 /// This method will panic if `from_height != from_state.block_height() + 1`.
-#[tracing::instrument(skip(params, block_source, data_db))]
+#[tracing::instrument(skip(params, block_source, data_db, from_state))]
 #[allow(clippy::type_complexity)]
 pub fn scan_cached_blocks<ParamsT, DbT, BlockSourceT>(
     params: &ParamsT,

zcash_client_backend/src/scanning.rs

@@ -12,6 +12,7 @@ use sapling::{
 };
 use subtle::{ConditionallySelectable, ConstantTimeEq, CtOption};
 
+use tracing::{debug, trace};
 use zcash_keys::keys::UnifiedFullViewingKey;
 use zcash_note_encryption::{batch, BatchDomain, Domain, ShieldedOutput, COMPACT_NOTE_SIZE};
 use zcash_primitives::{
@@ -666,6 +667,11 @@ where
     ) -> Option<ScanError> {
         if let Some(prev) = prior_block_metadata {
             if block.height() != prev.block_height() + 1 {
+                debug!(
+                    "Block height discontinuity at {:?}, previous was {:?} ",
+                    block.height(),
+                    prev.block_height()
+                );
                 return Some(ScanError::BlockHeightDiscontinuity {
                     prev_height: prev.block_height(),
                     new_height: block.height(),
@@ -673,6 +679,7 @@ where
             }
 
             if block.prev_hash() != prev.block_hash() {
+                debug!("Block hash discontinuity at {:?}", block.height());
                 return Some(ScanError::PrevHashMismatch {
                     at_height: block.height(),
                 });
@@ -686,6 +693,8 @@ where
         return Err(scan_error);
     }
 
+    trace!("Block continuity okay at {:?}", block.height());
+
     let cur_height = block.height();
     let cur_hash = block.hash();
     let zip212_enforcement = zip212_enforcement(params, cur_height);
@@ -736,6 +745,12 @@ where
             },
             Ok,
         )?;
+    let sapling_final_tree_size = sapling_commitment_tree_size
+        + block
+            .vtx
+            .iter()
+            .map(|tx| u32::try_from(tx.outputs.len()).unwrap())
+            .sum::<u32>();
 
     #[cfg(feature = "orchard")]
     let mut orchard_commitment_tree_size = prior_block_metadata
@@ -782,8 +797,14 @@ where
             },
             Ok,
         )?;
+    #[cfg(feature = "orchard")]
+    let orchard_final_tree_size = orchard_commitment_tree_size
+        + block
+            .vtx
+            .iter()
+            .map(|tx| u32::try_from(tx.actions.len()).unwrap())
+            .sum::<u32>();
 
-    let compact_block_tx_count = block.vtx.len();
     let mut wtxs: Vec<WalletTx<AccountId>> = vec![];
     let mut sapling_nullifier_map = Vec::with_capacity(block.vtx.len());
     let mut sapling_note_commitments: Vec<(sapling::Node, Retention<BlockHeight>)> = vec![];
@@ -793,7 +814,7 @@ where
     #[cfg(feature = "orchard")]
     let mut orchard_note_commitments: Vec<(MerkleHashOrchard, Retention<BlockHeight>)> = vec![];
 
-    for (tx_idx, tx) in block.vtx.into_iter().enumerate() {
+    for tx in block.vtx.into_iter() {
         let txid = tx.txid();
         let tx_index =
             u16::try_from(tx.index).expect("Cannot fit more than 2^16 transactions in a block");
@@ -836,9 +857,9 @@ where
 
         let (sapling_outputs, mut sapling_nc) = find_received(
             cur_height,
-            compact_block_tx_count,
+            sapling_final_tree_size
+                == sapling_commitment_tree_size + u32::try_from(tx.outputs.len()).unwrap(),
             txid,
-            tx_idx,
             sapling_commitment_tree_size,
             &scanning_keys.sapling,
             &spent_from_accounts,
@@ -870,9 +891,9 @@ where
         #[cfg(feature = "orchard")]
         let (orchard_outputs, mut orchard_nc) = find_received(
             cur_height,
-            compact_block_tx_count,
+            orchard_final_tree_size
+                == orchard_commitment_tree_size + u32::try_from(tx.actions.len()).unwrap(),
             txid,
-            tx_idx,
             orchard_commitment_tree_size,
             &scanning_keys.orchard,
             &spent_from_accounts,
@@ -1021,9 +1042,8 @@ fn find_received<
     NoteCommitment,
 >(
     block_height: BlockHeight,
-    block_tx_count: usize,
+    last_commitments_in_block: bool,
     txid: TxId,
-    tx_idx: usize,
     commitment_tree_size: u32,
     keys: &HashMap<IvkTag, SK>,
     spent_from_accounts: &HashSet<AccountId>,
@@ -1080,7 +1100,8 @@ fn find_received<
     {
         // Collect block note commitments
         let node = extract_note_commitment(output);
-        let is_checkpoint = output_idx + 1 == decoded.len() && tx_idx + 1 == block_tx_count;
+        // If the commitment is the last in the block, ensure that is is retained as a checkpoint
+        let is_checkpoint = output_idx + 1 == decoded.len() && last_commitments_in_block;
         let retention = match (decrypted_note.is_some(), is_checkpoint) {
             (is_marked, true) => Retention::Checkpoint {
                 id: block_height,

zcash_client_sqlite/src/lib.rs

@@ -46,14 +46,7 @@ use std::{
     path::Path,
 };
 use subtle::ConditionallySelectable;
-use zcash_primitives::{
-    block::BlockHash,
-    consensus::{self, BlockHeight},
-    memo::{Memo, MemoBytes},
-    transaction::{components::amount::NonNegativeAmount, Transaction, TxId},
-    zip32::{self, DiversifierIndex, Scope},
-};
-use zip32::fingerprint::SeedFingerprint;
+use tracing::{debug, trace, warn};
 
 use zcash_client_backend::{
     address::UnifiedAddress,
@@ -72,6 +65,14 @@ use zcash_client_backend::{
     wallet::{Note, NoteId, ReceivedNote, Recipient, WalletTransparentOutput},
     DecryptedOutput, PoolType, ShieldedProtocol, TransferType,
 };
+use zcash_primitives::{
+    block::BlockHash,
+    consensus::{self, BlockHeight},
+    memo::{Memo, MemoBytes},
+    transaction::{components::amount::NonNegativeAmount, Transaction, TxId},
+    zip32::{self, DiversifierIndex, Scope},
+};
+use zip32::fingerprint::SeedFingerprint;
 
 use crate::{error::SqliteClientError, wallet::commitment_tree::SqliteShardStore};
 
@@ -757,6 +758,26 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
 
                 last_scanned_height = Some(block.height());
                 let block_commitments = block.into_commitments();
+                trace!(
+                    "Sapling commitments for {:?}: {:?}",
+                    last_scanned_height,
+                    block_commitments
+                        .sapling
+                        .iter()
+                        .map(|(_, r)| *r)
+                        .collect::<Vec<_>>()
+                );
+                #[cfg(feature = "orchard")]
+                trace!(
+                    "Orchard commitments for {:?}: {:?}",
+                    last_scanned_height,
+                    block_commitments
+                        .orchard
+                        .iter()
+                        .map(|(_, r)| *r)
+                        .collect::<Vec<_>>()
+                );
+
                 sapling_commitments.extend(block_commitments.sapling.into_iter().map(Some));
                 #[cfg(feature = "orchard")]
                 orchard_commitments.extend(block_commitments.orchard.into_iter().map(Some));
@@ -898,6 +919,11 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
                 {
                     let mut sapling_subtrees_iter = sapling_subtrees.into_iter();
                     wdb.with_sapling_tree_mut::<_, _, Self::Error>(|sapling_tree| {
+                        debug!(
+                            "Sapling initial tree size at {:?}: {:?}",
+                            from_state.block_height(),
+                            from_state.final_sapling_tree().tree_size()
+                        );
                         sapling_tree.insert_frontier(
                             from_state.final_sapling_tree().clone(),
                             Retention::Checkpoint {
@@ -942,6 +968,11 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
                 {
                     let mut orchard_subtrees = orchard_subtrees.into_iter();
                     wdb.with_orchard_tree_mut::<_, _, Self::Error>(|orchard_tree| {
+                        debug!(
+                            "Orchard initial tree size at {:?}: {:?}",
+                            from_state.block_height(),
+                            from_state.final_orchard_tree().tree_size()
+                        );
                         orchard_tree.insert_frontier(
                             from_state.final_orchard_tree().clone(),
                             Retention::Checkpoint {
@@ -968,6 +999,11 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
 
                             for (height, checkpoint) in &missing_orchard_checkpoints {
                                 if *height > min_checkpoint_height {
+                                    debug!(
+                                        "Adding missing Orchard checkpoint for height: {:?}: {:?}",
+                                        height,
+                                        checkpoint.position()
+                                    );
                                     orchard_tree
                                         .store_mut()
                                         .add_checkpoint(*height, checkpoint.clone())