Merge branch 'sync-zcash-v4.1.0-merge-1' into sync-zcash-v4.1.0-merge

Author: dmidem

zebra-chain/src/orchard/arbitrary.rs

@@ -126,7 +126,15 @@ impl Arbitrary for Flags {
     type Parameters = ();
 
     fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
-        (any::<u8>()).prop_map(Self::from_bits_truncate).boxed()
+        (any::<u8>())
+            .prop_map(|byte| {
+                // Clear ENABLE_ZSA: it is only allowed in V6, and this generator is
+                // also used for V5 cases where the flag would make deserialization fail.
+                #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+                let byte = byte & !(Flags::ENABLE_ZSA.bits());
+                Self::from_bits_truncate(byte)
+            })
+            .boxed()
     }
 
     type Strategy = BoxedStrategy<Self>;

zebra-chain/src/orchard/shielded_data.rs

@@ -291,7 +291,7 @@ bitflags! {
     /// # Consensus
     ///
     /// > [NU5 onward] In a version 5 transaction, the reserved bits 2..7 of the flagsOrchard
-    /// > field MUST be zero.
+    /// > field MUST be zero. Bit 2 (ENABLE_ZSA) is introduced in V6 (NU7, ZIP 230).
     ///
     /// <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
     ///
@@ -306,7 +306,7 @@ bitflags! {
         /// Enable creating new non-zero valued Orchard notes.
         const ENABLE_OUTPUTS = 0b00000010;
         /// Enable ZSA transaction (otherwise all notes within actions must use native asset).
-        // FIXME: Should we use this flag explicitly anywhere in Zebra?
+        #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
         const ENABLE_ZSA = 0b00000100;
     }
 }

zebra-chain/src/orchard_zsa/asset_state.rs

@@ -70,9 +70,6 @@ impl AssetState {
     pub fn to_bytes(&self) -> Result<Vec<u8>, io::Error> {
         use std::io::Write;
 
-        // FIXME: Consider writing a leading version byte here so we can change AssetState's
-        // on-disk format without silently mis-parsing old DB entries during upgrades (and fix
-        // from_bytes accordingly).
         let mut bytes = Vec::new();
         bytes.write_all(&self.0.amount.to_bytes())?;
         bytes.write_u8(self.0.is_finalized as u8)?;
@@ -133,6 +130,9 @@ pub enum AssetStateError {
 
     #[error("burn validation failed: {0}")]
     Burn(BurnError),
+
+    #[error("invalid input: {0}")]
+    InvalidInput(String),
 }
 
 /// A map of asset state changes for assets modified in a block or transaction set.
@@ -174,16 +174,14 @@ impl IssuedAssetChanges {
         transaction_sighashes: Option<&[SigHash]>,
         get_state: impl Fn(&AssetBase) -> Option<AssetState>,
     ) -> Result<Self, AssetStateError> {
-        // When sighashes are provided, transactions and sighashes must be equal length by design,
-        // so we use assert instead of returning error.
         if let Some(sighashes) = transaction_sighashes {
-            assert_eq!(
-                transactions.len(),
-                sighashes.len(),
-                "Bug in caller: {} transactions but {} sighashes. Caller must provide one sighash per transaction.",
-                transactions.len(),
-                sighashes.len()
-            );
+            if transactions.len() != sighashes.len() {
+                return Err(AssetStateError::InvalidInput(format!(
+                    "transaction count ({}) does not match sighash count ({})",
+                    transactions.len(),
+                    sighashes.len(),
+                )));
+            }
         }
 
         // Track old and current states - old_state is None for newly created assets
@@ -206,18 +204,17 @@ impl IssuedAssetChanges {
                 // ZIP-0227 defines issued-note rho as DeriveIssuedRho(nf_{0,0}, i_action, i_note),
                 // so we must pass the first Action nullifier (nf_{0,0}). We rely on
                 // `orchard_nullifiers()` preserving Action order, so `.next()` returns nf_{0,0}.
-                let first_nullifier =
-                    // FIXME: For now, the only way to convert Zebra's nullifier type to Orchard's nullifier type
-                    // is via bytes, although they both wrap pallas::Point. Consider a more direct conversion to
-                    // avoid this round-trip, if possible.
-                    &Nullifier::from_bytes(&<[u8; 32]>::from(
-                        *tx.orchard_nullifiers()
-                            .next()
-                            // ZIP-0227 requires an issuance bundle to contain at least one OrchardZSA Action Group.
-                            // `ShieldedData.actions` is `AtLeastOne<...>`, so nf_{0,0} must exist.
-                            .expect("issuance must have at least one nullifier"),
-                    ))
-                    .expect("Bytes can be converted to Nullifier");
+                // Nullifier type conversion via bytes: both types wrap pallas::Point
+                // but lack a direct conversion path in the current orchard API.
+                // TODO: Consider adding a test for the case where a V6 transaction has issuance data
+                // but has no nullifiers (the test may require constructing a proper mock V6 transaction).
+                let raw_nullifier = tx.orchard_nullifiers().next().ok_or_else(|| {
+                    AssetStateError::InvalidInput(
+                        "issuance bundle has no orchard actions".to_string(),
+                    )
+                })?;
+                let first_nullifier = &Nullifier::from_bytes(&<[u8; 32]>::from(*raw_nullifier))
+                    .expect("valid zebra nullifier bytes convert to orchard nullifier");
 
                 let issue_records = match transaction_sighashes {
                     Some(sighashes) => {

zebra-chain/src/orchard_zsa/issuance.rs

@@ -38,10 +38,8 @@ impl IssueData {
     pub(crate) fn note_commitments(&self) -> impl Iterator<Item = pallas::Base> + '_ {
         self.0.actions().iter().flat_map(|action| {
             action.notes().iter().map(|note| {
-                // TODO: FIXME: Make `ExtractedNoteCommitment::inner` public in `orchard` (this would
-                // eliminate the need for the workaround of converting `pallas::Base` from bytes
-                // here), or introduce a new public method in `orchard::issuance::IssueBundle` to
-                // retrieve note commitments directly from `orchard`.
+                // TODO: Replace this workaround with orchard `ExtractedNoteCommitment` if its inner
+                // field is made public, or if a note_commitments() method is added to IssueBundle.
                 pallas::Base::from_repr(ExtractedNoteCommitment::from(note.commitment()).to_bytes())
                     .unwrap()
             })

zebra-chain/src/transaction/serialize.rs

@@ -475,6 +475,14 @@ impl ZcashDeserialize for Option<orchard::ShieldedData<OrchardVanilla>> {
         // in [`Flags::zcash_deserialized`].
         let flags: orchard::Flags = (&mut reader).zcash_deserialize_into()?;
 
+        // `ENABLE_ZSA` is introduced in V6 (ZIP 230) and must be zero in V5.
+        #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+        if flags.contains(orchard::Flags::ENABLE_ZSA) {
+            return Err(SerializationError::Parse(
+                "ENABLE_ZSA is not allowed in V5 transactions",
+            ));
+        }
+
         // Denoted as `valueBalanceOrchard` in the spec.
         let value_balance: Amount = (&mut reader).zcash_deserialize_into()?;
 

zebra-consensus/src/error.rs

@@ -47,6 +47,10 @@ pub enum TransactionError {
     #[error("coinbase transaction MUST NOT have the EnableSpendsOrchard flag set")]
     CoinbaseHasEnableSpendsOrchard,
 
+    #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+    #[error("coinbase transaction MUST NOT have the EnableZSA flag set")]
+    CoinbaseHasEnableZSA,
+
     #[error("coinbase transaction Sapling or Orchard outputs MUST be decryptable with an all-zero outgoing viewing key")]
     CoinbaseOutputsNotDecryptable,
 

zebra-consensus/src/transaction/check.rs

@@ -182,6 +182,13 @@ pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), Tr
             if orchard_flags.contains(Flags::ENABLE_SPENDS) {
                 return Err(TransactionError::CoinbaseHasEnableSpendsOrchard);
             }
+            // ZIP-230: coinbase must not set enableZSA.
+            // TODO: Add V6 coinbase ENABLE_ZSA tests (fails when set, passes when unset),
+            // like v5_coinbase_transaction_without_enable_spends_flag_passes_validation.
+            #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+            if orchard_flags.contains(Flags::ENABLE_ZSA) {
+                return Err(TransactionError::CoinbaseHasEnableZSA);
+            }
         }
     }
 

zebra-rpc/src/methods.rs

@@ -2096,13 +2096,25 @@ where
         let read_state = self.read_state.clone();
         let include_non_finalized = include_non_finalized.unwrap_or(true);
 
-        let asset_base = zebra_chain::orchard_zsa::AssetBase::from_bytes(
-            &hex::decode(asset_base).map_misc_error()?[..]
-                .try_into()
-                .map_misc_error()?,
-        )
-        .into_option()
-        .ok_or_misc_error("invalid asset base")?;
+        if asset_base.len() != 64 {
+            return Err("expected 32 bytes (64 hex chars)")
+                .map_error(server::error::LegacyCode::InvalidParameter);
+        }
+
+        let asset_base_bytes: [u8; 32] = hex::decode(&asset_base)
+            .map_error_with_prefix(
+                server::error::LegacyCode::InvalidParameter,
+                "invalid hex encoding",
+            )?
+            .try_into()
+            .expect("length already checked above");
+
+        let asset_base = zebra_chain::orchard_zsa::AssetBase::from_bytes(&asset_base_bytes)
+            .into_option()
+            .ok_or_error(
+                server::error::LegacyCode::InvalidParameter,
+                "invalid asset base",
+            )?;
 
         let request = zebra_state::ReadRequest::AssetState {
             asset_base,

zebra-state/src/request.rs

@@ -599,7 +599,6 @@ impl SemanticallyVerifiedBlock {
             new_outputs,
             transaction_hashes,
             // Not used in checkpoint paths.
-            // FIXME: Is this correct?
             transaction_sighashes: None,
             deferred_pool_balance_change: None,
         }

zebra-state/src/service/non_finalized_state/chain.rs

@@ -1014,11 +1014,9 @@ impl Chain {
         issued_asset_changes: &IssuedAssetChanges,
     ) {
         if position == RevertPosition::Root {
-            // TODO: Consider evicting issued-asset entries from the non-finalized in-memory cache.
-            // We may add `updated_at_height` to track last update and drop “old” entries occasionally.
-            // Doing that here on root reverts might be too aggressive (happens ~every 100 blocks ≈ 2 hours).
-            // Eviction would only move reads to disk (DB); issuance/burn verification must still work,
-            // just with slightly worse performance due to extra DB reads.
+            // `issued_assets` grows monotonically (no eviction on finalization).
+            // At ~112 bytes per asset this is negligible for realistic asset counts.
+            // Revisit if ZSA adoption reaches hundreds of thousands of unique assets.
         } else {
             trace!(
                 ?position,
@@ -1570,16 +1568,11 @@ impl Chain {
                     *current_state = *new_state;
                 })
                 .or_insert_with(|| {
-                    // FIXME: This assert fails if the asset was removed in revert_issued_assets
-                    // at RevertPosition::Root position. And it looks like it's a legal case
-                    // when the previous state in block is not None but the state item was
-                    // removed during eviction in revert_issued_assets. So we should not use
-                    // this check here?
-                    //assert!(
-                    //    old_state_from_block.is_none(),
-                    //    "issued asset state mismatch for {:?}",
-                    //    asset_base
-                    //);
+                    // When `old_state_from_block` is `Some` but the asset is missing from
+                    // the in-memory map, it means the entry was evicted during finalization
+                    // and lives in the finalized DB. Inserting `new_state` is correct, the
+                    // old state is preserved on disk for rollback via the `IssuedAssetChanges`
+                    // pairs.
                     *new_state
                 });
         }