Apply changelog, documentation & style suggestions from code review

Co-authored-by: str4d <thestr4d@gmail.com>

zcash_client_backend/CHANGELOG.md

@@ -7,30 +7,73 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 ### Added
-- `impl Eq for address::RecipientAddress`
+- `impl Eq for zcash_client_backend::address::RecipientAddress`
-- `impl Eq for zip321::{Payment, TransactionRequest}`
+- `impl Eq for zcash_client_backend::zip321::{Payment, TransactionRequest}`
-- `data_api::NullifierQuery` for use with `WalletRead::get_sapling_nullifiers`
+- `impl Debug` for `zcash_client_backend::{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}`
+- `zcash_client_backend::data_api`:
+  - `WalletRead::{fully_scanned_height, suggest_scan_ranges}`
   - `WalletWrite::put_block`
-- `impl Debug` for `{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}
+  - `WalletCommitmentTrees`
+  - `testing::MockWalletDb::new`
+  - `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers`
+  - `wallet::input_sellection::Proposal::{min_target_height, min_anchor_height}`:
+- `zcash_client_backend::wallet::WalletSaplingOutput::note_commitment_tree_position`
+- `zcash_client_backend::welding_rig::SyncError`
 
 ### Changed
 - MSRV is now 1.65.0.
 - Bumped dependencies to `hdwallet 0.4`, `zcash_primitives 0.12`, `zcash_note_encryption 0.4`,
   `incrementalmerkletree 0.4`, `orchard 0.5`, `bs58 0.5`
+- `zcash_client_backend::data_api`:
   - `WalletRead::get_memo` now returns `Result<Option<Memo>, Self::Error>`
     instead of `Result<Memo, Self::Error>` in order to make representable
     wallet states where the full note plaintext is not available.
   - `WalletRead::get_nullifiers` has been renamed to `WalletRead::get_sapling_nullifiers`
     and its signature has changed; it now subsumes the removed `WalletRead::get_all_nullifiers`.
-- `wallet::SpendableNote` has been renamed to `wallet::ReceivedSaplingNote`.
+  - `chain::scan_cached_blocks` now takes a `from_height` argument that
-- `data_api::chain::scan_cached_blocks` now takes a `from_height` argument that
     permits the caller to control the starting position of the scan range.
-- `WalletWrite::advance_by_block` has been replaced by `WalletWrite::put_block`
+  - A new `CommitmentTree` variant has been added to `data_api::error::Error`
-  to reflect the semantic change that scanning is no longer a linear operation.
+  - `data_api::wallet::{create_spend_to_address, create_proposed_transaction,
+    shield_transparent_funds}` all now require that `WalletCommitmentTrees` be
+    implemented for the type passed to them for the `wallet_db` parameter.
+  - `data_api::wallet::create_proposed_transaction` now takes an additional 
+    `min_confirmations` argument.
+  - A new `Sync` variant has been added to `data_api::chain::error::Error`.
+  - A new `SyncRequired` variant has been added to `data_api::wallet::input_selection::InputSelectorError`.
+- `zcash_client_backend::wallet`:
+  - `SpendableNote` has been renamed to `ReceivedSaplingNote`.
+  - Arguments to `WalletSaplingOutput::from_parts` have changed.
+- `zcash_client_backend::data_api::wallet::input_selection::InputSelector`:
+  - Arguments to `{propose_transaction, propose_shielding}` have changed. 
+- `zcash_client_backend::wallet::ReceivedSaplingNote::note_commitment_tree_position`
+  has replaced the `witness` field in the same struct.
+- `zcash_client_backend::welding_rig::ScanningKey::sapling_nf` has been changed to
+  take a note position instead of an incremental witness for the note.
+- Arguments to `zcash_client_backend::welding_rig::scan_block` have changed. This
+  method now takes an optional `CommitmentTreeMeta` argument instead of a base commitment
+  tree and incremental witnesses for each previously-known note.
+
 
 ### Removed
+- `zcash_client_backend::data_api`:
   - `WalletRead::get_all_nullifiers`
-- `WalletWrite::advance_by_block` 
+  - `WalletRead::{get_commitment_tree, get_witnesses}` (use
+    `WalletRead::fully_scanned_height` instead).
+  - `WalletWrite::advance_by_block` (use `WalletWrite::put_block` instead).
+  - The `commitment_tree` and `transactions` properties of the `PrunedBlock`
+    struct are now owned values instead of references, making this a fully owned type.
+    It is now parameterized by the nullifier type instead of by a lifetime for the 
+    fields that were previously references. In addition, two new properties,
+    `sapling_commitment_tree_size` and `sapling_commitments` have been added.
+  - `testing::MockWalletDb`, which is available under the `test-dependencies`
+    feature flag, has been modified by the addition of a `sapling_tree` property. 
+  - `wallet::input_selection`:
+    - `Proposal::target_height` (use `Proposal::min_target_height` instead).
+- `zcash_client_backend::wallet::WalletSaplingOutput::{witness, witness_mut}`
+  have been removed as individual incremental witnesses are no longer tracked on a
+  per-note basis. The global note commitment tree for the wallet should be used
+  to obtain witnesses for spend operations instead.
+
 
 ## [0.9.0] - 2023-04-28
 ### Added

zcash_client_backend/src/data_api.rs

@@ -438,6 +438,10 @@ pub trait WalletWrite: WalletRead {
     ) -> Result<Self::UtxoRef, Self::Error>;
 }
 
+/// This trait describes a capability for manipulating wallet note commitment trees.
+///
+/// At present, this only serves the Sapling protocol, but it will be modified to
+/// also provide operations related to Orchard note commitment trees in the future.
 pub trait WalletCommitmentTrees {
     type Error;
     type SaplingShardStore<'a>: ShardStore<

zcash_client_backend/src/data_api/chain.rs

@@ -101,16 +101,20 @@ use crate::{
 pub mod error;
 use error::{ChainError, Error};
 
+/// Metadata describing the sizes of the zcash note commitment trees as of a particular block.
 pub struct CommitmentTreeMeta {
     sapling_tree_size: u64,
     //TODO: orchard_tree_size: u64
 }
 
 impl CommitmentTreeMeta {
+    /// Constructs a new [`CommitmentTreeMeta`] value from its constituent parts.
     pub fn from_parts(sapling_tree_size: u64) -> Self {
         Self { sapling_tree_size }
     }
 
+    /// Returns the size of the Sapling note commitment tree as of the block that this
+    /// [`CommitmentTreeMeta`] describes.
     pub fn sapling_tree_size(&self) -> u64 {
         self.sapling_tree_size
     }
@@ -199,23 +203,19 @@ where
 /// Scans at most `limit` new blocks added to the block source for any transactions received by the
 /// tracked accounts.
 ///
+/// If the `from_height` argument is not `None`, then the block source will begin requesting blocks
+/// from the provided block source at the specified height; if `from_height` is `None then this
+/// will begin scanning at first block after the position to which the wallet has previously
+/// fully scanned the chain, thereby beginning or continuing a linear scan over all blocks.
+///
 /// This function will return without error after scanning at most `limit` new blocks, to enable
-/// the caller to update their UI with scanning progress. Repeatedly calling this function will
+/// the caller to update their UI with scanning progress. Repeatedly calling this function with
-/// process sequential ranges of blocks, and is equivalent to calling `scan_cached_blocks` and
+/// `from_height == None` will process sequential ranges of blocks.
-/// passing `None` for the optional `limit` value.
 ///
-/// This function pays attention only to cached blocks with heights greater than the highest
+/// For brand-new light client databases, if `from_height == None` this function starts scanning
-/// scanned block in `data`. Cached blocks with lower heights are not verified against
+/// from the Sapling activation height. This height can be fast-forwarded to a more recent block by
-/// previously-scanned blocks. In particular, this function **assumes** that the caller is handling
+/// initializing the client database with a starting block (for example, calling
-/// rollbacks.
+/// `init_blocks_table` before this function if using `zcash_client_sqlite`).
-///
-/// For brand-new light client databases, this function starts scanning from the Sapling activation
-/// height. This height can be fast-forwarded to a more recent block by initializing the client
-/// database with a starting block (for example, calling `init_blocks_table` before this function
-/// if using `zcash_client_sqlite`).
-///
-/// Scanned blocks are required to be height-sequential. If a block is missing from the block
-/// source, an error will be returned with cause [`error::Cause::BlockHeightDiscontinuity`].
 #[tracing::instrument(skip(params, block_source, data_db))]
 #[allow(clippy::type_complexity)]
 pub fn scan_cached_blocks<ParamsT, DbT, BlockSourceT>(
@@ -260,7 +260,7 @@ where
     );
 
     // Start at either the provided height, or where we synced up to previously.
-    let (from_height, commitment_tree_meta) = from_height.map_or_else(
+    let (scan_from, commitment_tree_meta) = from_height.map_or_else(
         || {
             data_db.fully_scanned_height().map_or_else(
                 |e| Err(Error::Wallet(e)),
@@ -273,7 +273,7 @@ where
     )?;
 
     block_source.with_blocks::<_, DbT::Error, DbT::NoteRef>(
-        from_height,
+        scan_from,
         limit,
         |block: CompactBlock| {
             add_block_to_runner(params, block, &mut batch_runner);
@@ -283,10 +283,22 @@ where
 
     batch_runner.flush();
 
+    let mut last_scanned = None;
     block_source.with_blocks::<_, DbT::Error, DbT::NoteRef>(
-        from_height,
+        scan_from,
         limit,
         |block: CompactBlock| {
+            // block heights should be sequential within a single scan range
+            let block_height = block.height();
+            if let Some(h) = last_scanned {
+                if block_height != h + 1 {
+                    return Err(Error::Chain(ChainError::block_height_discontinuity(
+                        block.height(),
+                        h,
+                    )));
+                }
+            }
+
             let pruned_block = scan_block_with_runner(
                 params,
                 block,
@@ -312,6 +324,7 @@ where
 
             data_db.put_block(pruned_block).map_err(Error::Wallet)?;
 
+            last_scanned = Some(block_height);
             Ok(())
         },
     )?;

zcash_client_backend/src/data_api/wallet.rs

@@ -122,6 +122,10 @@ where
 ///   spent. A value of 10 confirmations is recommended and 0-conf transactions are
 ///   not supported.
 ///
+/// # Panics
+///
+/// Panics if `min_confirmations == 0`; 0-conf transactions are not supported.
+///
 /// # Examples
 ///
 /// ```

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -62,7 +62,9 @@ impl<DE: fmt::Display, SE: fmt::Display> fmt::Display for InputSelectorError<DE,
                 i64::from(*available),
                 i64::from(*required)
             ),
-            InputSelectorError::SyncRequired => write!(f, "No chain data is available."),
+            InputSelectorError::SyncRequired => {
+                write!(f, "Insufficient chain data is available, sync required.")
+            }
         }
     }
 }
@@ -135,7 +137,7 @@ impl<FeeRuleT, NoteRef> Debug for Proposal<FeeRuleT, NoteRef> {
             .field("min_target_height", &self.min_target_height)
             .field("min_anchor_height", &self.min_anchor_height)
             .field("is_shielding", &self.is_shielding)
-            .finish()
+            .finish_non_exhaustive()
     }
 }
 

zcash_client_backend/src/welding_rig.rs

@@ -672,6 +672,22 @@ mod tests {
             assert_eq!(tx.sapling_outputs[0].index(), 0);
             assert_eq!(tx.sapling_outputs[0].account(), AccountId::from(0));
             assert_eq!(tx.sapling_outputs[0].note().value().inner(), 5);
+
+            assert_eq!(
+                pruned_block
+                    .sapling_commitments
+                    .iter()
+                    .map(|(_, retention)| *retention)
+                    .collect::<Vec<_>>(),
+                vec![
+                    Retention::Ephemeral,
+                    Retention::Marked,
+                    Retention::Checkpoint {
+                        id: pruned_block.block_height,
+                        is_marked: false
+                    }
+                ]
+            );
         }
 
         go(false);
@@ -714,5 +730,20 @@ mod tests {
         assert_eq!(tx.sapling_spends[0].index(), 0);
         assert_eq!(tx.sapling_spends[0].nf(), &nf);
         assert_eq!(tx.sapling_spends[0].account(), account);
+
+        assert_eq!(
+            pruned_block
+                .sapling_commitments
+                .iter()
+                .map(|(_, retention)| *retention)
+                .collect::<Vec<_>>(),
+            vec![
+                Retention::Ephemeral,
+                Retention::Checkpoint {
+                    id: pruned_block.block_height,
+                    is_marked: false
+                }
+            ]
+        );
     }
 }

zcash_client_sqlite/CHANGELOG.md

@@ -6,10 +6,19 @@ and this library adheres to Rust's notion of
 [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Added
+- `zcash_client_sqlite::serialization` Serialization formats for data stored
+  as SQLite BLOBs in the wallet database.
+
 ### Changed
 - MSRV is now 1.65.0.
 - Bumped dependencies to `hdwallet 0.4`, `incrementalmerkletree 0.4`, `bs58 0.5`,
   `zcash_primitives 0.12`
+- A `CommitmentTree` variant has been added to `zcash_client_sqlite::wallet::init::WalletMigrationError`
+- `min_confirmations` parameter values are now more strongly enforced. Previously,
+  a note could be spent with fewer than `min_confirmations` confirmations if the
+  wallet did not contain enough observed blocks to satisfy the `min_confirmations`
+  value specified; this situation is now treated as an error.
 
 ### Removed
 - The empty `wallet::transact` module has been removed.

zcash_client_sqlite/src/error.rs

@@ -78,7 +78,8 @@ pub enum SqliteClientError {
     #[cfg(feature = "transparent-inputs")]
     AddressNotRecognized(TransparentAddress),
 
-    /// An error occurred in inserting data into one of the wallet's note commitment trees.
+    /// An error occurred in inserting data into or accessing data from one of the wallet's note
+    /// commitment trees.
     CommitmentTree(ShardTreeError<Either<io::Error, rusqlite::Error>>),
 }
 

zcash_client_sqlite/src/lib.rs

@@ -85,6 +85,8 @@ pub mod wallet;
 /// this delta from the chain tip to be pruned.
 pub(crate) const PRUNING_HEIGHT: u32 = 100;
 
+pub(crate) const SAPLING_TABLES_PREFIX: &'static str = "sapling";
+
 /// A newtype wrapper for sqlite primary key values for the notes
 /// table.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
@@ -396,10 +398,9 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
     ) -> Result<Vec<Self::NoteRef>, Self::Error> {
         self.transactionally(|wdb| {
             // Insert the block into the database.
-            let block_height = block.block_height;
             wallet::put_block(
                 wdb.conn.0,
-                block_height,
+                block.block_height,
                 block.block_hash,
                 block.block_time,
                 block.sapling_commitment_tree_size.map(|s| s.into()),
@@ -423,18 +424,19 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
                 }
             }
 
+            let sapling_commitments_len = block.sapling_commitments.len();
             let mut sapling_commitments = block.sapling_commitments.into_iter();
             wdb.with_sapling_tree_mut::<_, _, SqliteClientError>(move |sapling_tree| {
                 if let Some(sapling_tree_size) = block.sapling_commitment_tree_size {
                     let start_position = Position::from(u64::from(sapling_tree_size))
-                        - u64::try_from(sapling_commitments.len()).unwrap();
+                        - u64::try_from(sapling_commitments_len).unwrap();
                     sapling_tree.batch_insert(start_position, &mut sapling_commitments)?;
                 }
                 Ok(())
             })?;
 
             // Update now-expired transactions that didn't get mined.
-            wallet::update_expired_notes(wdb.conn.0, block_height)?;
+            wallet::update_expired_notes(wdb.conn.0, block.block_height)?;
 
             Ok(wallet_note_ids)
         })
@@ -633,10 +635,10 @@ impl<P: consensus::Parameters> WalletCommitmentTrees for WalletDb<rusqlite::Conn
             .conn
             .transaction()
             .map_err(|e| ShardTreeError::Storage(Either::Right(e)))?;
-        let shard_store = SqliteShardStore::from_connection(&tx, "sapling")
+        let shard_store = SqliteShardStore::from_connection(&tx, SAPLING_TABLES_PREFIX)
             .map_err(|e| ShardTreeError::Storage(Either::Right(e)))?;
         let result = {
-            let mut shardtree = ShardTree::new(shard_store, 100);
+            let mut shardtree = ShardTree::new(shard_store, PRUNING_HEIGHT.try_into().unwrap());
             callback(&mut shardtree)?
         };
         tx.commit()
@@ -662,9 +664,9 @@ impl<'conn, P: consensus::Parameters> WalletCommitmentTrees for WalletDb<SqlTran
         E: From<ShardTreeError<Either<io::Error, rusqlite::Error>>>,
     {
         let mut shardtree = ShardTree::new(
-            SqliteShardStore::from_connection(self.conn.0, "sapling")
+            SqliteShardStore::from_connection(self.conn.0, SAPLING_TABLES_PREFIX)
                 .map_err(|e| ShardTreeError::Storage(Either::Right(e)))?,
-            100,
+            PRUNING_HEIGHT.try_into().unwrap(),
         );
         let result = callback(&mut shardtree)?;
 

zcash_client_sqlite/src/serialization.rs

@@ -14,10 +14,11 @@ const NIL_TAG: u8 = 0;
 const LEAF_TAG: u8 = 1;
 const PARENT_TAG: u8 = 2;
 
-pub fn write_shard_v1<H: HashSer, W: Write>(
+/// Writes a [`PrunableTree`] to the provided [`Write`] instance.
-    writer: &mut W,
+///
-    tree: &PrunableTree<H>,
+/// This is the primary method used for ShardTree shard persistence. It writes a version identifier
-) -> io::Result<()> {
+/// for the most-current serialized form, followed by the tree data.
+pub fn write_shard<H: HashSer, W: Write>(writer: &mut W, tree: &PrunableTree<H>) -> io::Result<()> {
     fn write_inner<H: HashSer, W: Write>(
         mut writer: &mut W,
         tree: &PrunableTree<H>,
@@ -80,6 +81,11 @@ fn read_shard_v1<H: HashSer, R: Read>(mut reader: &mut R) -> io::Result<Prunable
     }
 }
 
+/// Reads a [`PrunableTree`] from the provided [`Read`] instance.
+///
+/// This function operates by first parsing a 1-byte version identifier, and then dispatching to
+/// the correct deserialization function for the observed version, or returns an
+/// [`io::ErrorKind::InvalidData`] error in the case that the version is not recognized.
 pub fn read_shard<H: HashSer, R: Read>(mut reader: R) -> io::Result<PrunableTree<H>> {
     match reader.read_u8()? {
         SER_V1 => read_shard_v1(&mut reader),
@@ -97,7 +103,7 @@ mod tests {
     use shardtree::testing::arb_prunable_tree;
     use std::io::Cursor;
 
-    use super::{read_shard, write_shard_v1};
+    use super::{read_shard, write_shard};
 
     proptest! {
         #[test]
@@ -105,7 +111,7 @@ mod tests {
             tree in arb_prunable_tree(arb_test_node(), 8, 32)
         ) {
             let mut tree_data = vec![];
-            write_shard_v1(&mut tree_data, &tree).unwrap();
+            write_shard(&mut tree_data, &tree).unwrap();
             let cursor = Cursor::new(tree_data);
             let tree_result = read_shard::<TestNode, _>(cursor).unwrap();
             assert_eq!(tree, tree_result);

zcash_client_sqlite/src/wallet.rs

@@ -640,7 +640,7 @@ pub(crate) fn get_min_unspent_height(
 /// block, this function does nothing.
 ///
 /// This should only be executed inside a transactional context.
-pub(crate) fn truncate_to_height<P: consensus::Parameters + Clone>(
+pub(crate) fn truncate_to_height<P: consensus::Parameters>(
     conn: &rusqlite::Transaction,
     params: &P,
     block_height: BlockHeight,

zcash_client_sqlite/src/wallet/commitment_tree.rs

@@ -11,7 +11,7 @@ use shardtree::{Checkpoint, LocatedPrunableTree, PrunableTree, ShardStore, TreeS
 
 use zcash_primitives::{consensus::BlockHeight, merkle_tree::HashSer};
 
-use crate::serialization::{read_shard, write_shard_v1};
+use crate::serialization::{read_shard, write_shard};
 
 pub struct SqliteShardStore<C, H, const SHARD_HEIGHT: u8> {
     pub(crate) conn: C,
@@ -327,7 +327,7 @@ pub(crate) fn put_shard<H: HashSer>(
         .map_err(Either::Left)?;
 
     let mut subtree_data = vec![];
-    write_shard_v1(&mut subtree_data, subtree.root()).map_err(Either::Left)?;
+    write_shard(&mut subtree_data, subtree.root()).map_err(Either::Left)?;
 
     let mut stmt_put_shard = conn
         .prepare_cached(&format!(
@@ -423,7 +423,7 @@ pub(crate) fn put_cap<H: HashSer>(
         .map_err(Either::Right)?;
 
     let mut cap_data = vec![];
-    write_shard_v1(&mut cap_data, &cap).map_err(Either::Left)?;
+    write_shard(&mut cap_data, &cap).map_err(Either::Left)?;
     stmt.execute([cap_data]).map_err(Either::Right)?;
 
     Ok(())

zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs

@@ -18,10 +18,10 @@ use zcash_primitives::{
     sapling,
 };
 
-use crate::wallet::{
+use crate::{wallet::{
     commitment_tree::SqliteShardStore,
     init::{migrations::received_notes_nullable_nf, WalletMigrationError},
-};
+}, SAPLING_TABLES_PREFIX};
 
 pub(super) const MIGRATION_ID: Uuid = Uuid::from_fields(
     0x7da6489d,
@@ -94,7 +94,7 @@ impl RusqliteMigration for Migration {
         let shard_store =
             SqliteShardStore::<_, sapling::Node, SAPLING_SHARD_HEIGHT>::from_connection(
                 transaction,
-                "sapling",
+                SAPLING_TABLES_PREFIX,
             )?;
         let mut shard_tree: ShardTree<
             _,
@@ -187,14 +187,8 @@ impl RusqliteMigration for Migration {
         Ok(())
     }
 
-    fn down(&self, transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> {
+    fn down(&self, _transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> {
-        transaction.execute_batch(
+        // TODO: something better than just panic?
-            "DROP TABLE sapling_tree_checkpoint_marks_removed;
+        panic!("Cannot revert this migration.");
-            DROP TABLE sapling_tree_checkpoints;
-            DROP TABLE sapling_tree_cap;
-            DROP TABLE sapling_tree_shards;",
-        )?;
-
-        Ok(())
     }
 }

zcash_client_sqlite/src/wallet/sapling.rs

@@ -425,7 +425,7 @@ pub(crate) mod tests {
         },
     };
 
-    pub fn test_prover() -> impl TxProver {
+    pub(crate) fn test_prover() -> impl TxProver {
         match LocalTxProver::with_default_location() {
             Some(tx_prover) => tx_prover,
             None => {

zcash_primitives/CHANGELOG.md

@@ -12,6 +12,9 @@ and this library adheres to Rust's notion of
   - `Builder::add_orchard_spend`
   - `Builder::add_orchard_output`
 - `zcash_primitives::transaction::components::orchard::builder` module
+- `impl HashSer for String` is provided under the `test-dependencies` feature
+  flag. This is a test-only impl; the identity leaf value is `_` and the combining
+  operation is concatenation.
 
 ### Changed
 - `zcash_primitives::transaction`: