zcash_client_backend: Fix off-by-one in tree heights at NU activation.

zcash_client_backend/CHANGELOG.md

@@ -26,8 +26,12 @@ and this library adheres to Rust's notion of
 
 ### Changed
 - `zcash_client_backend::data_api`:
+  - Arguments to `BlockMetadata::from_parts` have changed to include Orchard.
   - `BlockMetadata::sapling_tree_size` now returns an `Option<u32>` instead of
     a `u32` for consistency with Orchard.
+  - Arguments to `ScannedBlock::from_parts` have changed.
+  - `ScannedBlock::metadata` has been renamed to `to_block_metadata` and now
+    returns an owned value rather than a reference.
   - `ShieldedProtocol` has a new variant for `Orchard`, allowing for better
     reporting to callers trying to perform actions using `Orchard` before it is
     fully supported.
@@ -38,8 +42,6 @@ and this library adheres to Rust's notion of
       backend-specific note identifier. The related `NoteRef` type parameter has
       been removed from `error::Error`.
     - A new variant `UnsupportedPoolType` has been added.
-  - `ScannedBlock::metadata` has been renamed to `to_block_metadata` and now
-    returns an owned value rather than a reference.
   - `wallet::shield_transparent_funds` no longer
     takes a `memo` argument; instead, memos to be associated with the shielded
     outputs should be specified in the construction of the value of the
@@ -108,6 +110,7 @@ and this library adheres to Rust's notion of
   - The fields of `ReceivedSaplingNote` are now private. Use
     `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods
     are provided for each previously public field.
+- `zcash_client_backend::scanning::ScanError` has a new variant, `TreeSizeInvalid`.
 - The following fields now have type `NonNegativeAmount` instead of `Amount`:
   - `zcash_client_backend::data_api`:
     - `error::Error::InsufficientFunds.{available, required}`

zcash_client_backend/src/data_api.rs

@@ -440,13 +440,13 @@ impl BlockMetadata {
     }
 
     /// Returns the size of the Sapling note commitment tree for the final treestate of the block
-    /// that this [`BlockMetadata`] describes.
+    /// that this [`BlockMetadata`] describes, if available.
     pub fn sapling_tree_size(&self) -> Option<u32> {
         self.sapling_tree_size
     }
 
     /// Returns the size of the Orchard note commitment tree for the final treestate of the block
-    /// that this [`BlockMetadata`] describes.
+    /// that this [`BlockMetadata`] describes, if available.
     pub fn orchard_tree_size(&self) -> Option<u32> {
         self.orchard_tree_size
     }

zcash_client_backend/src/scanning.rs

@@ -167,6 +167,14 @@ pub enum ScanError {
         protocol: ShieldedProtocol,
         at_height: BlockHeight,
     },
+
+    /// We were provided chain metadata for a block containing note commitment tree metadata
+    /// that is invalidated by the data in the block itself. This may be caused by the presence
+    /// of default values in the chain metadata.
+    TreeSizeInvalid {
+        protocol: ShieldedProtocol,
+        at_height: BlockHeight,
+    },
 }
 
 impl ScanError {
@@ -178,6 +186,7 @@ impl ScanError {
             BlockHeightDiscontinuity { .. } => true,
             TreeSizeMismatch { .. } => true,
             TreeSizeUnknown { .. } => false,
+            TreeSizeInvalid { .. } => false,
         }
     }
 
@@ -189,6 +198,7 @@ impl ScanError {
             BlockHeightDiscontinuity { new_height, .. } => *new_height,
             TreeSizeMismatch { at_height, .. } => *at_height,
             TreeSizeUnknown { at_height, .. } => *at_height,
+            TreeSizeInvalid { at_height, .. } => *at_height,
         }
     }
 }
@@ -211,6 +221,9 @@ impl fmt::Display for ScanError {
             TreeSizeUnknown { protocol, at_height } => {
                 write!(f, "Unable to determine {:?} note commitment tree size at height {}", protocol, at_height)
             }
+            TreeSizeInvalid { protocol, at_height } => {
+                write!(f, "Received invalid (potentially default) {:?} note commitment tree size metadata at height {}", protocol, at_height)
+            }
         }
     }
 }
@@ -335,70 +348,98 @@ pub(crate) fn scan_block_with_runner<
     let cur_hash = block.hash();
 
     let initial_sapling_tree_size = prior_block_metadata.and_then(|m| m.sapling_tree_size());
-    let mut sapling_commitment_tree_size = initial_sapling_tree_size
+    let mut sapling_commitment_tree_size = initial_sapling_tree_size.map_or_else(
-        .or_else(|| {
+        || {
-            block
+            block.chain_metadata.as_ref().map_or_else(
-                .chain_metadata
+                || {
-                .as_ref()
+                    // If we're below Sapling activation, or Sapling activation is not set, the tree size is zero
-                .and_then(|m| {
+                    params
-                    if m.sapling_commitment_tree_size == 0 {
+                        .activation_height(NetworkUpgrade::Sapling)
-                        None
+                        .map_or_else(
+                            || Ok(0),
+                            |sapling_activation| {
+                                if cur_height < sapling_activation {
+                                    Ok(0)
                                 } else {
-                        let block_output_count: u32 = block
+                                    Err(ScanError::TreeSizeUnknown {
+                                        protocol: ShieldedProtocol::Sapling,
+                                        at_height: cur_height,
+                                    })
+                                }
+                            },
+                        )
+                },
+                |m| {
+                    let sapling_output_count: u32 = block
                         .vtx
                         .iter()
                         .map(|tx| tx.outputs.len())
                         .sum::<usize>()
                         .try_into()
                         .expect("Sapling output count cannot exceed a u32");
-                        Some(m.sapling_commitment_tree_size - block_output_count)
+
-                    }
+                    if m.sapling_commitment_tree_size >= sapling_output_count {
-                })
+                        Ok(m.sapling_commitment_tree_size - sapling_output_count)
-                .or_else(|| {
+                    } else {
-                    // If we're below Sapling activation, or Sapling activation is not set, the
+                        // The default for m.sapling_commitment_tree_size is zero, so we need to check
-                    // tree size is zero
+                        // that the subtraction will not underflow; if it would do so, we given invalid
-                    params
+                        // chain metadata for a block with Sapling outputs.
-                        .activation_height(NetworkUpgrade::Sapling)
+                        Err(ScanError::TreeSizeInvalid {
-                        .map_or(Some(0), |h| if h > cur_height { Some(0) } else { None })
-                })
-        })
-        .ok_or(ScanError::TreeSizeUnknown {
                             protocol: ShieldedProtocol::Sapling,
                             at_height: cur_height,
-        })?;
+                        })
+                    }
+                },
+            )
+        },
+        Ok,
+    )?;
 
     let initial_orchard_tree_size = prior_block_metadata.and_then(|m| m.orchard_tree_size());
-    let mut orchard_commitment_tree_size = initial_orchard_tree_size
+    let mut orchard_commitment_tree_size = initial_orchard_tree_size.map_or_else(
-        .or_else(|| {
+        || {
-            block
+            block.chain_metadata.as_ref().map_or_else(
-                .chain_metadata
+                || {
-                .as_ref()
+                    // If we're below Orchard activation, or Orchard activation is not set, the tree size is zero
-                .and_then(|m| {
+                    params.activation_height(NetworkUpgrade::Nu5).map_or_else(
-                    if m.orchard_commitment_tree_size == 0 {
+                        || Ok(0),
-                        None
+                        |orchard_activation| {
+                            if cur_height < orchard_activation {
+                                Ok(0)
                             } else {
-                        let block_action_count: u32 = block
+                                Err(ScanError::TreeSizeUnknown {
+                                    protocol: ShieldedProtocol::Orchard,
+                                    at_height: cur_height,
+                                })
+                            }
+                        },
+                    )
+                },
+                |m| {
+                    let orchard_action_count: u32 = block
                         .vtx
                         .iter()
                         .map(|tx| tx.actions.len())
                         .sum::<usize>()
                         .try_into()
                         .expect("Orchard action count cannot exceed a u32");
-                        Some(m.orchard_commitment_tree_size - block_action_count)
+
-                    }
+                    // The default for m.orchard_commitment_tree_size is zero, so we need to check
-                })
+                    // that the subtraction will not underflow; if it would do so, we given invalid
-                .or_else(|| {
+                    // chain metadata for a block with Orchard actions.
-                    // If we're below Nu5 activation, or Nu5 activation is not set, the tree size
+                    if m.orchard_commitment_tree_size >= orchard_action_count {
-                    // is zero
+                        Ok(m.orchard_commitment_tree_size - orchard_action_count)
-                    params
+                    } else {
-                        .activation_height(NetworkUpgrade::Nu5)
+                        Err(ScanError::TreeSizeInvalid {
-                        .map_or(Some(0), |h| if h > cur_height { Some(0) } else { None })
-                })
-        })
-        .ok_or(ScanError::TreeSizeUnknown {
                             protocol: ShieldedProtocol::Orchard,
                             at_height: cur_height,
-        })?;
+                        })
+                    }
+                },
+            )
+        },
+        Ok,
+    )?;
 
     let compact_block_tx_count = block.vtx.len();
     let mut wtxs: Vec<WalletTx<K::Nf>> = vec![];