ZSA integration (step 8): Merge zsa-issued-assets into zsa-integration-consensus with bug fixes

zebra-chain/src/orchard/orchard_flavor_ext.rs

@@ -11,12 +11,11 @@ use orchard::{note_encryption::OrchardDomainCommon, orchard_flavor};
 
 use crate::{
     orchard::ValueCommitment,
-    orchard_zsa,
     serialization::{ZcashDeserialize, ZcashSerialize},
 };
 
 #[cfg(feature = "tx-v6")]
-use crate::orchard_zsa::{Burn, NoBurn};
+use crate::orchard_zsa::{Burn, BurnItem, NoBurn};
 
 use super::note;
 
@@ -59,8 +58,9 @@ pub trait OrchardFlavorExt: Clone + Debug {
         + Default
         + ZcashDeserialize
         + ZcashSerialize
+        // FIXME: consider using AsRef instead of Into, to avoid a redundancy
         + Into<ValueCommitment>
-        + AsRef<[orchard_zsa::BurnItem]>
+        + AsRef<[BurnItem]>
         + TestArbitrary;
 }
 

zebra-chain/src/orchard_zsa/arbitrary.rs

@@ -19,13 +19,9 @@ impl Arbitrary for BurnItem {
         // FIXME: move arb_asset_to_burn out of BundleArb in orchard
         // as it does not depend on flavor (we pinned it here OrchardVanilla
         // just for certainty, as there's no difference, which flavor to use)
-        // FIXME: consider to use BurnItem(asset_base, value.try_into().expect("Invalid value for Amount"))
-        // instead of filtering non-convertable values
         // FIXME: should we filter/protect from including native assets into burn here?
         BundleArb::<orchard::orchard_flavor::OrchardVanilla>::arb_asset_to_burn()
-            .prop_filter_map("Conversion to Amount failed", |(asset_base, value)| {
-                BurnItem::try_from((asset_base, value)).ok()
-            })
+            .prop_map(|(asset_base, value)| BurnItem::from((asset_base, value)))
             .boxed()
     }
 

zebra-chain/src/orchard_zsa/asset_state.rs

@@ -28,6 +28,7 @@ pub struct AssetState {
 pub struct AssetStateChange {
     /// Whether the asset should be finalized such that no more of it can be issued.
     pub should_finalize: bool,
+    // FIXME: is this a correct comment?
     /// Whether the asset should be finalized such that no more of it can be issued.
     pub includes_issuance: bool,
     /// The change in supply from newly issued assets or burned assets, if any.
@@ -50,6 +51,7 @@ impl Default for SupplyChange {
     }
 }
 
+// FIXME: can we reuse some functions from orchard crate?s
 impl SupplyChange {
     /// Applies `self` to a provided `total_supply` of an asset.
     ///
@@ -81,7 +83,8 @@ impl SupplyChange {
                 // Burn amounts MUST not be 0
                 // TODO: Reference ZIP
                 0.. => signed.try_into().ok().map(Self::Issuance),
-                ..0 => signed.try_into().ok().map(Self::Burn),
+                // FIXME: (-signed) - is this a correct fix?
+                ..0 => (-signed).try_into().ok().map(Self::Burn),
             })
         {
             *self = result;
@@ -208,6 +211,7 @@ impl AssetStateChange {
             })
             .unwrap_or_default()
             .into_iter()
+            // FIXME: We use 0 as a value - is that correct?
             .map(|asset_base| Self::new(asset_base, SupplyChange::Issuance(0), true));
 
         supply_changes.chain(finalize_changes)
@@ -238,7 +242,7 @@ impl AssetStateChange {
     /// Accepts an [`BurnItem`] and returns an iterator of asset bases and issued asset state changes
     /// that should be applied to those asset bases when committing the provided burn to the chain state.
     fn from_burn(burn: &BurnItem) -> (AssetBase, Self) {
-        Self::new(burn.asset(), SupplyChange::Burn(burn.amount()), false)
+        Self::new(burn.asset(), SupplyChange::Burn(burn.raw_amount()), false)
     }
 
     /// Updates and returns self with the provided [`AssetStateChange`] if

zebra-chain/src/orchard_zsa/burn.rs

@@ -5,6 +5,7 @@ use std::io;
 use halo2::pasta::pallas;
 
 use crate::{
+    amount::Amount,
     block::MAX_BLOCK_BYTES,
     orchard::ValueCommitment,
     serialization::{
@@ -36,28 +37,27 @@ const AMOUNT_SIZE: u64 = 8;
 // FIXME: is this a correct way to calculate (simple sum of sizes of components)?
 const BURN_ITEM_SIZE: u64 = ASSET_BASE_SIZE + AMOUNT_SIZE;
 
+// FIXME: Define BurnItem (or, even Burn/NoBurn) in Orchard and reuse it here?
 /// Orchard ZSA burn item.
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
-pub struct BurnItem(AssetBase, u64);
+pub struct BurnItem(AssetBase, NoteValue);
 
 impl BurnItem {
     /// Returns [`AssetBase`] being burned.
     pub fn asset(&self) -> AssetBase {
         self.0
     }
 
-    /// Returns [`u64`] representing amount being burned.
-    pub fn amount(&self) -> u64 {
-        self.1
+    /// Returns the raw [`u64`] amount being burned.
+    pub fn raw_amount(&self) -> u64 {
+        self.1.inner()
     }
 }
 
 // Convert from burn item type used in `orchard` crate
-impl TryFrom<(AssetBase, NoteValue)> for BurnItem {
-    type Error = crate::amount::Error;
-
-    fn try_from(item: (AssetBase, NoteValue)) -> Result<Self, Self::Error> {
-        Ok(Self(item.0, item.1.inner()))
+impl From<(AssetBase, NoteValue)> for BurnItem {
+    fn from(item: (AssetBase, NoteValue)) -> Self {
+        Self(item.0, item.1)
     }
 }
 
@@ -66,20 +66,18 @@ impl ZcashSerialize for BurnItem {
         let BurnItem(asset_base, amount) = self;
 
         asset_base.zcash_serialize(&mut writer)?;
-        writer.write_all(&amount.to_be_bytes())?;
+        writer.write_all(&amount.to_bytes())?;
 
         Ok(())
     }
 }
 
 impl ZcashDeserialize for BurnItem {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        let asset_base = AssetBase::zcash_deserialize(&mut reader)?;
         let mut amount_bytes = [0; 8];
         reader.read_exact(&mut amount_bytes)?;
-        Ok(Self(
-            AssetBase::zcash_deserialize(&mut reader)?,
-            u64::from_be_bytes(amount_bytes),
-        ))
+        Ok(Self(asset_base, NoteValue::from_bytes(amount_bytes)))
     }
 }
 
@@ -98,7 +96,7 @@ impl serde::Serialize for BurnItem {
         S: serde::Serializer,
     {
         // FIXME: return a custom error with a meaningful description?
-        (self.0.to_bytes(), &self.1).serialize(serializer)
+        (self.0.to_bytes(), &self.1.inner()).serialize(serializer)
     }
 }
 
@@ -114,7 +112,7 @@ impl<'de> serde::Deserialize<'de> for BurnItem {
             // FIXME: duplicates the body of AssetBase::zcash_deserialize?
             Option::from(AssetBase::from_bytes(&asset_base_bytes))
                 .ok_or_else(|| serde::de::Error::custom("Invalid orchard_zsa AssetBase"))?,
-            amount,
+            NoteValue::from_raw(amount),
         ))
     }
 }
@@ -166,7 +164,15 @@ impl From<Burn> for ValueCommitment {
         burn.0
             .into_iter()
             .map(|BurnItem(asset, amount)| {
-                ValueCommitment::with_asset(pallas::Scalar::zero(), amount, &asset)
+                ValueCommitment::with_asset(
+                    pallas::Scalar::zero(),
+                    // FIXME: consider to use TryFrom and return an error instead of using "expect"
+                    amount
+                        .inner()
+                        .try_into()
+                        .expect("should convert Burn amount to i64"),
+                    &asset,
+                )
             })
             .sum()
     }

zebra-chain/src/orchard_zsa/issuance.rs

@@ -55,8 +55,8 @@ impl IssueData {
     }
 
     /// Returns issuance actions
-    pub fn actions(&self) -> &NonEmpty<IssueAction> {
-        self.0.actions()
+    pub fn actions(&self) -> impl Iterator<Item = &IssueAction> {
+        self.0.actions().iter()
     }
 }
 

zebra-state/src/service/check/issuance.rs

@@ -29,6 +29,8 @@ pub fn valid_burns_and_issuance(
 ) -> Result<IssuedAssets, ValidateContextError> {
     let mut issued_assets = HashMap::new();
 
+    // FIXME: Do all checks (for duplication, existence etc.) need to be performed per tranaction, not per
+    // the entire block?
     for (issued_assets_change, transaction) in semantically_verified
         .issued_assets_changes
         .iter()
@@ -41,7 +43,11 @@ pub fn valid_burns_and_issuance(
                 asset_state(finalized_state, parent_chain, &issued_assets, &asset_base)
                     .ok_or(ValidateContextError::InvalidBurn)?;
 
-            if asset_state.total_supply < burn.amount() {
+            // FIXME: The check that we can't burn an asset before we issued it is implicit -
+            // through the check it total_supply < burn.amount (the total supply is zero if the
+            // asset is not issued). May be validation functions from the orcharfd crate need to be
+            // reused in a some way?
+            if asset_state.total_supply < burn.raw_amount() {
                 return Err(ValidateContextError::InvalidBurn);
             } else {
                 // Any burned asset bases in the transaction will also be present in the issued assets change,
@@ -50,6 +56,13 @@ pub fn valid_burns_and_issuance(
             }
         }
 
+        // FIXME: Not sure: it looks like semantically_verified.issued_assets_changes is already
+        // filled with burn and issuance items in zebra-consensus, see Verifier::call function in
+        // zebra-consensus/src/transaction.rs (it uses from_burn and from_issue_action AssetStateChange
+        // methods from ebra-chain/src/orchard_zsa/asset_state.rs). Can't it cause a duplication?
+        // Can we collect all change items here, not in zebra-consensus (and so we don't need
+        // SemanticallyVerifiedBlock::issued_assets_changes at all), or performing part of the
+        // checks in zebra-consensus is important for the consensus checks order in a some way?
         for (asset_base, change) in issued_assets_change.iter() {
             let asset_state =
                 asset_state(finalized_state, parent_chain, &issued_assets, &asset_base)
@@ -59,9 +72,13 @@ pub fn valid_burns_and_issuance(
                 .apply_change(change)
                 .ok_or(ValidateContextError::InvalidIssuance)?;
 
-            issued_assets
-                .insert(asset_base, updated_asset_state)
-                .expect("transactions must have only one burn item per asset base");
+            // FIXME: Is it correct to do nothing if the issued_assets aready has asset_base? Now it'd be
+            // replaced with updated_asset_state in this case (where the duplicated value is added to
+            // the supply). Block may have two burn records for the same asset but in different
+            // transactions - it's allowed, that's why the check has been removed. On the other
+            // hand, there needs to be a check that denies duplicated burn records for the same
+            // asset inside the same transaction.
+            issued_assets.insert(asset_base, updated_asset_state);
         }
     }