From 874d6b608b827c0cf2e628cfe949d150663bd3e7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sun, 24 Mar 2024 11:41:02 -0600 Subject: [PATCH] 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 | 2 +- zcash_client_backend/src/scanning.rs | 39 ++++++++++++---- zcash_client_sqlite/src/lib.rs | 52 ++++++++++++++++++---- 3 files changed, 75 insertions(+), 18 deletions(-) diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index 21e136d71..8741fed52 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/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( params: &ParamsT, diff --git a/zcash_client_backend/src/scanning.rs b/zcash_client_backend/src/scanning.rs index 33df8a938..78b585283 100644 --- a/zcash_client_backend/src/scanning.rs +++ b/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 { 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::(); #[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::(); - let compact_block_tx_count = block.vtx.len(); let mut wtxs: Vec> = vec![]; let mut sapling_nullifier_map = Vec::with_capacity(block.vtx.len()); let mut sapling_note_commitments: Vec<(sapling::Node, Retention)> = vec![]; @@ -793,7 +814,7 @@ where #[cfg(feature = "orchard")] let mut orchard_note_commitments: Vec<(MerkleHashOrchard, Retention)> = 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, spent_from_accounts: &HashSet, @@ -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, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 1ffe417e7..45181ef33 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/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 WalletWrite for WalletDb 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::>() + ); + #[cfg(feature = "orchard")] + trace!( + "Orchard commitments for {:?}: {:?}", + last_scanned_height, + block_commitments + .orchard + .iter() + .map(|(_, r)| *r) + .collect::>() + ); + 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 WalletWrite for WalletDb { 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 WalletWrite for WalletDb { 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 WalletWrite for WalletDb 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())