From 24e8c82546b5ac3d65201c6b915118e98613debe Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 11 Sep 2023 15:18:17 -0600 Subject: [PATCH] zcash_client_backend: Get commitment tree depth for a given number of confirmations from the database. This fixes the following bug: Due to complexities related to non-linear scanning, checkpoints are only added to the wallet's commitment tree in cases where there are notes discovered within a scanned block. At present, the `shardtree` API only makes it possible to add multiple checkpoints of the same tree state when adding checkpoints at the chain tip, and this functionality is not used by `zcash_client_backend` because we perform checkpoint insertion in batches that may contain gaps in the case that multiple blocks contain no Sapling notes. While it would be possible to fix this by altering the `shardtree` API to permit explicit insertion of multiple checkpoints of the same tree state at a given note position, this fix takes a simpler approach. Instead of ensuring that a checkpoint exists at every block and computing the required checkpoint depth directly from the minimum number of confirmations required when attempting a spend, we alter the `WalletCommitmentTrees` API to allow internal information of the note commitment tree to be used to determine this checkpoint depth, given the minimum number of commitments as an argument. This allows us to select a usable checkpoint from the sparse checkpoint set that resulted from the sparse insertion of checkpoints described above. --- zcash_client_backend/src/data_api.rs | 17 ++++++++++ zcash_client_backend/src/data_api/wallet.rs | 4 ++- zcash_client_sqlite/src/lib.rs | 28 +++++++++++++-- .../src/wallet/commitment_tree.rs | 34 +++++++++++++++++-- 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index b20ced7ae..098ad9e61 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -867,6 +867,16 @@ pub trait WalletCommitmentTrees { Error = Self::Error, >; + /// Returns the depth of the checkpoint in the tree that can be used to create a witness at the + /// anchor having the given number of confirmations. + /// + /// This assumes that at any time a note is added to the tree, a checkpoint is created for the + /// end of the block in which that note was discovered. + fn get_checkpoint_depth( + &self, + min_confirmations: NonZeroU32, + ) -> Result>; + fn with_sapling_tree_mut(&mut self, callback: F) -> Result where for<'a> F: FnMut( @@ -1182,5 +1192,12 @@ pub mod testing { Ok(()) } + + fn get_checkpoint_depth( + &self, + min_confirmations: NonZeroU32, + ) -> Result> { + Ok(usize::try_from(u32::from(min_confirmations) - 1).unwrap()) + } } } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index da820e729..71d050369 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -488,6 +488,8 @@ where // are no possible transparent inputs, so we ignore those let mut builder = Builder::new(params.clone(), proposal.min_target_height(), None); + let checkpoint_depth = wallet_db.get_checkpoint_depth(min_confirmations)?; + wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _, _>>(|sapling_tree| { for selected in proposal.sapling_inputs() { let (note, key, merkle_path) = select_key_for_note( @@ -495,7 +497,7 @@ where selected, usk.sapling(), &dfvk, - usize::try_from(u32::from(min_confirmations) - 1).unwrap(), + checkpoint_depth, )? .ok_or(Error::NoteMismatch(selected.note_id))?; diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 22f0ff8c6..3c30f3f6b 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -44,7 +44,10 @@ use std::{ }; use incrementalmerkletree::Position; -use shardtree::{error::ShardTreeError, ShardTree}; +use shardtree::{ + error::{QueryError, ShardTreeError}, + ShardTree, +}; use zcash_primitives::{ block::BlockHash, consensus::{self, BlockHeight}, @@ -89,7 +92,7 @@ pub mod serialization; pub mod wallet; use wallet::{ - commitment_tree::{self, put_shard_roots}, + commitment_tree::{self, get_checkpoint_depth, put_shard_roots}, SubtreeScanProgress, }; @@ -795,6 +798,15 @@ impl WalletCommitmentTrees for WalletDb Result> { + get_checkpoint_depth(&self.conn, SAPLING_TABLES_PREFIX, min_confirmations) + .map_err(|e| ShardTreeError::Storage(commitment_tree::Error::Query(e)))? + .ok_or(ShardTreeError::Query(QueryError::CheckpointPruned)) + } } impl<'conn, P: consensus::Parameters> WalletCommitmentTrees for WalletDb, P> { @@ -835,6 +847,18 @@ impl<'conn, P: consensus::Parameters> WalletCommitmentTrees for WalletDb Result> { + get_checkpoint_depth(self.conn.0, SAPLING_TABLES_PREFIX, min_confirmations) + .map_err(|e| ShardTreeError::Storage(commitment_tree::Error::Query(e)))? + // `CheckpointPruned` is perhaps a little misleading; in this case it's that + // the chain tip is unknown, but if that were the case we should never have been + // calling this anyway. + .ok_or(ShardTreeError::Query(QueryError::CheckpointPruned)) + } } /// A handle for the SQLite block source. diff --git a/zcash_client_sqlite/src/wallet/commitment_tree.rs b/zcash_client_sqlite/src/wallet/commitment_tree.rs index f90b135be..f05ab043c 100644 --- a/zcash_client_sqlite/src/wallet/commitment_tree.rs +++ b/zcash_client_sqlite/src/wallet/commitment_tree.rs @@ -4,6 +4,7 @@ use std::{ error, fmt, io::{self, Cursor}, marker::PhantomData, + num::NonZeroU32, ops::Range, sync::Arc, }; @@ -20,6 +21,8 @@ use zcash_primitives::{consensus::BlockHeight, merkle_tree::HashSer}; use crate::serialization::{read_shard, write_shard}; +use super::scan_queue_extrema; + /// Errors that can appear in SQLite-back [`ShardStore`] implementation operations. #[derive(Debug)] pub enum Error { @@ -724,8 +727,8 @@ pub(crate) fn get_checkpoint( .query_row( &format!( "SELECT position - FROM {}_tree_checkpoints - WHERE checkpoint_id = ?", + FROM {}_tree_checkpoints + WHERE checkpoint_id = ?", table_prefix ), [u32::from(checkpoint_id)], @@ -747,6 +750,33 @@ pub(crate) fn get_checkpoint( .transpose() } +pub(crate) fn get_checkpoint_depth( + conn: &rusqlite::Connection, + table_prefix: &'static str, + min_confirmations: NonZeroU32, +) -> Result, rusqlite::Error> { + scan_queue_extrema(conn)? + .map(|(_, max)| max) + .map(|chain_tip| { + let max_checkpoint_height = + u32::from(chain_tip).saturating_sub(u32::from(min_confirmations) - 1); + + // We exclude from consideration all checkpoints having heights greater than the maximum + // checkpoint height. The checkpoint depth is the number of excluded checkpoints + 1. + conn.query_row( + &format!( + "SELECT COUNT(*) + FROM {}_tree_checkpoints + WHERE checkpoint_id > :max_checkpoint_height", + table_prefix + ), + named_params![":max_checkpoint_height": max_checkpoint_height], + |row| row.get::<_, usize>(0).map(|s| s + 1), + ) + }) + .transpose() +} + pub(crate) fn get_checkpoint_at_depth( conn: &rusqlite::Connection, table_prefix: &'static str,