updated comments and todos

Author: PaulLaux

Cargo.toml

@@ -30,6 +30,8 @@ edition = "2021"
 
 [workspace.dependencies]
 incrementalmerkletree = { version = "0.8.2", features = ["legacy-api"] }
+# TODO: Remove the `temporary-zebra` feature once upstream `orchard` exposes the APIs
+# Zebra needs without this transition feature.
 orchard = { version = "0.12", features = ["zsa-issuance", "temporary-zebra"] }
 sapling-crypto = "0.6"
 zcash_address = "0.10.1"

zebra-chain/src/orchard_zsa/burn.rs

@@ -160,6 +160,9 @@ pub(crate) fn compute_burn_value_commitment(burn: &[BurnItem]) -> ValueCommitmen
         .map(|&BurnItem(asset, amount)| {
             ValueCommitment::new(
                 pallas::Scalar::zero(),
+                // TODO: `amount - NoteValue::from_raw(0)` is an obscure way to widen
+                // `NoteValue` into `ValueSum`. Replace with a direct/explicit conversion
+                // once the upstream Orchard API exposes one.
                 amount - NoteValue::from_raw(0),
                 asset,
             )

zebra-chain/src/transaction/serialize.rs

@@ -438,6 +438,11 @@ impl ZcashSerialize for orchard::ShieldedData<OrchardZSA> {
         zcash_serialize_external_count(&sigs, &mut writer)?;
 
         // Denoted as `valueBalanceOrchard` in the spec.
+        // FIXME: `valueBalanceOrchard` and `bindingSigOrchard` are per-transaction fields,
+        // not per-action-group fields. They are serialized here because today the V6
+        // transaction has exactly one action group. Once multi-action-group support is
+        // added (ZIP-230), move this serialization up to the caller in the V6 branch of
+        // `Transaction::zcash_serialize`.
         self.value_balance.zcash_serialize(&mut writer)?;
 
         // Denoted as `bindingSigOrchard` in the spec.
@@ -868,6 +873,9 @@ impl ZcashSerialize for Transaction {
                 writer.write_u32::<LittleEndian>(expiry_height.0)?;
 
                 // Denoted as `zip233_amount` in the spec.
+                // FIXME: ZIP-230 places this *after* the 8-byte `fee` field which is currently
+                // missing (see TODO on the Transaction::V6 variant). Once `fee` is added, the
+                // byte offset of zip233_amount will shift to match the spec.
                 zip233_amount.zcash_serialize(&mut writer)?;
 
                 // Denoted as `tx_in_count` and `tx_in` in the spec.

zebra-consensus/src/transaction.rs

@@ -1078,6 +1078,10 @@ where
     /// - the prepared `cached_ffi_transaction` used by the script verifier
     /// - the sapling shielded data of the transaction, if any
     /// - the orchard shielded data of the transaction, if any
+    // FIXME: This function performs no V6-specific issuance or burn semantic checks
+    // (ZIP-226 / ZIP-227). Those rules are enforced only in `zebra-state` via
+    // `IssuedAssetChanges::validate_and_get_changes`. Either move that validation here
+    // or document the contract that the state layer cannot be bypassed.
     #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
     fn verify_v6_transaction(
         request: &Request,

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

@@ -1005,7 +1005,11 @@ impl Chain {
     }
 
     #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
-    /// Remove the History tree index at `height`.
+    /// Revert the issued-asset state changes recorded for a block.
+    ///
+    /// At `RevertPosition::Tip`, restores `issued_assets` to the per-asset `old_state`
+    /// captured when the block was applied. At `RevertPosition::Root` this is a no-op
+    /// because finalized issuance state lives in the on-disk column family.
     fn revert_issued_assets(
         &mut self,
         position: RevertPosition,