export new precompute api in zebra-script (#1493)

* export new precompute api in zebra-script
* remove old API in favor of precompute API
* add multi use test cases and bump version
* update implementation to actually match henry's design
* Add safety comment for zebra-script

Co-authored-by: teor <teor@riseup.net>

Cargo.lock

@@ -3829,9 +3829,9 @@ dependencies = [
 
 [[package]]
 name = "zcash_script"
-version = "0.1.4"
+version = "0.1.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8bf1995ddb0c827f160357922a96c0cd34c6dd759757cb2b799128d6f420c91e"
+checksum = "381b15f5e9b000121834a4fcf4351e020e42ed1b23620da3cf5c650f896d7a6a"
 dependencies = [
  "bindgen",
  "blake2b_simd",
@@ -3946,7 +3946,7 @@ version = "1.0.0-alpha.0"
 
 [[package]]
 name = "zebra-script"
-version = "1.0.0-alpha.0"
+version = "1.0.0-alpha.1"
 dependencies = [
  "displaydoc",
  "hex",

zebra-consensus/src/script.rs

@@ -3,7 +3,8 @@ use std::{collections::HashMap, future::Future, pin::Pin, sync::Arc};
 use tower::timeout::Timeout;
 use tracing::Instrument;
 
-use zebra_chain::{parameters::NetworkUpgrade, transaction::Transaction, transparent};
+use zebra_chain::{parameters::NetworkUpgrade, transparent};
+use zebra_script::CachedFfiTransaction;
 use zebra_state::Utxo;
 
 use crate::BoxError;
@@ -55,7 +56,7 @@ pub struct Request {
     ///
     /// This causes quadratic script verification behavior, so
     /// at some future point, we need to reform this data.
-    pub transaction: Arc<Transaction>,
+    pub cached_ffi_transaction: Arc<CachedFfiTransaction>,
     pub input_index: usize,
     /// A set of additional UTXOs known in the context of this verification request.
     ///
@@ -89,12 +90,12 @@ where
         use futures_util::FutureExt;
 
         let Request {
-            transaction,
+            cached_ffi_transaction,
             input_index,
             known_utxos,
             upgrade,
         } = req;
-        let input = &transaction.inputs()[input_index];
+        let input = &cached_ffi_transaction.inputs()[input_index];
         let branch_id = upgrade
             .branch_id()
             .expect("post-Sapling NUs have a consensus branch ID");
@@ -119,19 +120,9 @@ where
                     };
                     tracing::trace!(?utxo, "got UTXO");
 
-                    if transaction.inputs().len() < 20 {
+                    cached_ffi_transaction
-                        zebra_script::is_valid(
+                        .is_valid(branch_id, (input_index as u32, utxo.output))?;
-                            transaction,
+                    tracing::trace!("script verification succeeded");
-                            branch_id,
-                            (input_index as u32, utxo.output),
-                        )?;
-                        tracing::trace!("script verification succeeded");
-                    } else {
-                        tracing::debug!(
-                            inputs.len = transaction.inputs().len(),
-                            "skipping verification of script with many inputs to avoid quadratic work until we fix zebra_script/zcash_script interface"
-                        );
-                    }
 
                     Ok(())
                 }

zebra-consensus/src/transaction.rs

@@ -20,6 +20,7 @@ use zebra_chain::{
     transparent,
 };
 
+use zebra_script::CachedFfiTransaction;
 use zebra_state as zs;
 
 use crate::{error::TransactionError, script, BoxError};
@@ -150,11 +151,14 @@ where
                     } else {
                         // otherwise, check no coinbase inputs
                         // feed all of the inputs to the script verifier
+                        let cached_ffi_transaction =
+                            Arc::new(CachedFfiTransaction::new(tx.clone()));
+
                         for input_index in 0..inputs.len() {
                             let rsp = script_verifier.ready_and().await?.call(script::Request {
                                 upgrade,
                                 known_utxos: known_utxos.clone(),
-                                transaction: tx.clone(),
+                                cached_ffi_transaction: cached_ffi_transaction.clone(),
                                 input_index,
                             });
 

zebra-script/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "zebra-script"
-version = "1.0.0-alpha.0"
+version = "1.0.0-alpha.1"
 authors = ["Zcash Foundation <zebra@zfnd.org>"]
 license = "MIT OR Apache-2.0"
 edition = "2018"
@@ -8,7 +8,7 @@ edition = "2018"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-zcash_script = "0.1.4"
+zcash_script = "0.1.5"
 zebra-chain = { path = "../zebra-chain" }
 thiserror = "1.0.22"
 displaydoc = "0.1.7"

zebra-script/src/lib.rs

@@ -56,85 +56,129 @@ impl From<zcash_script_error_t> for Error {
     }
 }
 
-/// Thin safe wrapper around ffi interface provided by libzcash_script
+/// A preprocessed Transction which can be used to verify scripts within said
-fn verify_script(
+/// Transaction.
-    script_pub_key: impl AsRef<[u8]>,
+#[derive(Debug)]
-    amount: i64,
+pub struct CachedFfiTransaction {
-    tx_to: impl AsRef<[u8]>,
+    transaction: Arc<Transaction>,
-    n_in: u32,
+    precomputed: *mut std::ffi::c_void,
-    consensus_branch_id: u32,
+}
-) -> Result<(), Error> {
-    let script_pub_key = script_pub_key.as_ref();
-    let tx_to = tx_to.as_ref();
 
-    let script_ptr = script_pub_key.as_ptr();
+impl CachedFfiTransaction {
-    let script_len = script_pub_key.len();
+    /// Construct a `PrecomputedTransaction` from a `Transaction`.
-    let tx_to_ptr = tx_to.as_ptr();
+    pub fn new(transaction: Arc<Transaction>) -> Self {
-    let tx_to_len = tx_to.len();
+        let tx_to = transaction
-    let mut err = 0;
+            .zcash_serialize_to_vec()
+            .expect("serialization into a vec is infallible");
 
-    let flags = zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_P2SH
+        let tx_to_ptr = tx_to.as_ptr();
-        | zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY;
+        let tx_to_len = tx_to.len() as u32;
+        let mut err = 0;
 
-    let ret = unsafe {
+        let precomputed = unsafe {
-        zcash_script::zcash_script_verify(
+            zcash_script::zcash_script_new_precomputed_tx(tx_to_ptr, tx_to_len, &mut err)
-            script_ptr,
+        };
-            script_len as u32,
-            amount,
-            tx_to_ptr,
-            tx_to_len as u32,
-            n_in,
-            #[cfg(not(windows))]
-            flags,
-            #[cfg(windows)]
-            flags.try_into().expect("why bindgen whyyy"),
-            consensus_branch_id,
-            &mut err,
-        )
-    };
 
-    if ret == 1 {
+        Self {
-        Ok(())
+            transaction,
-    } else {
+            precomputed,
-        Err(Error::from(err))
+        }
+    }
+
+    pub fn inputs(&self) -> &[transparent::Input] {
+        self.transaction.inputs()
+    }
+
+    /// Verify a script within a transaction given the corresponding
+    /// `transparent::Output` it is spending and the `ConsensusBranchId` of the block
+    /// containing the transaction.
+    ///
+    /// # Details
+    ///
+    /// The `input_index` corresponds to the index of the `TransparentInput` which in
+    /// `transaction` used to identify the `previous_output`.
+    pub fn is_valid(
+        &self,
+        branch_id: ConsensusBranchId,
+        (input_index, previous_output): (u32, transparent::Output),
+    ) -> Result<(), Error> {
+        let transparent::Output { value, lock_script } = previous_output;
+        let script_pub_key: &[u8] = lock_script.0.as_ref();
+        let n_in = input_index as _;
+
+        let script_ptr = script_pub_key.as_ptr();
+        let script_len = script_pub_key.len();
+
+        let amount = value.into();
+        let flags = zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_P2SH
+            | zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY;
+
+        let consensus_branch_id = branch_id.into();
+
+        let mut err = 0;
+
+        let ret = unsafe {
+            zcash_script::zcash_script_verify_precomputed(
+                self.precomputed,
+                n_in,
+                script_ptr,
+                script_len as u32,
+                amount,
+                #[cfg(not(windows))]
+                flags,
+                #[cfg(windows)]
+                flags
+                    .try_into()
+                    .expect("zcash_script_SCRIPT_FLAGS_VERIFY_* enum values fit in a c_uint"),
+                consensus_branch_id,
+                &mut err,
+            )
+        };
+
+        if ret == 1 {
+            Ok(())
+        } else {
+            Err(Error::from(err))
+        }
     }
 }
 
-/// Verify a script within a transaction given the corresponding
+// # SAFETY
-/// `transparent::Output` it is spending and the `ConsensusBranchId` of the block
+//
-/// containing the transaction.
+// ## Justification
-///
+//
-/// # Details
+// `CachedFfiTransaction` is not `Send` and `Sync` by default because of the
-///
+// `*mut c_void` it contains. This is because raw pointers could allow the same
-/// input index corresponds to the index of the `TransparentInput` which in
+// data to be mutated from different threads if copied.
-/// `transaction` used to identify the `previous_output`
+//
-pub fn is_valid(
+// CachedFFiTransaction needs to be Send and Sync to be stored within a `Box<dyn
-    transaction: Arc<Transaction>,
+// Future + Send + Sync + static>`. In `zebra_consensus/src/transaction.rs`, an
-    branch_id: ConsensusBranchId,
+// async block owns a `CachedFfiTransaction`, and holds it across an await
-    (input_index, previous_output): (u32, transparent::Output),
+// point, while the transaction verifier is spawning all of the script verifier
-) -> Result<(), Error> {
+// futures. The service readiness check requires this await between each task
-    assert!((input_index as usize) < transaction.inputs().len());
+// spawn. Each `script` future needs a copy of the
+// `Arc<CachedFfiTransaction>` so that it can simultaniously verify inputs
+// without cloning the c++ allocated type unnecessarily.
+//
+// ## Explanation
+//
+// It is safe for us to mark this as `Send` and `Sync` because the data pointed
+// to by `precomputed` is never modified after it is constructed and points to
+// heap memory with a stable memory location. The function
+// `zcash_script::zcash_script_verify_precomputed` only reads from the
+// precomputed context while verifying inputs, which makes it safe to treat this
+// pointer like a shared reference (given that is how it is used).
+unsafe impl Send for CachedFfiTransaction {}
+unsafe impl Sync for CachedFfiTransaction {}
 
-    let tx_to = transaction
+impl Drop for CachedFfiTransaction {
-        .zcash_serialize_to_vec()
+    fn drop(&mut self) {
-        .expect("serialization into a vec is infallible");
+        unsafe { zcash_script::zcash_script_free_precomputed_tx(self.precomputed) };
-
+    }
-    let transparent::Output { value, lock_script } = previous_output;
-
-    verify_script(
-        &lock_script.0,
-        value.into(),
-        &tx_to,
-        input_index as _,
-        branch_id.into(),
-    )?;
-
-    Ok(())
 }
 
 #[cfg(test)]
 mod tests {
-    use super::*;
     use hex::FromHex;
     use std::convert::TryInto;
     use std::sync::Arc;
@@ -151,7 +195,7 @@ mod tests {
     }
 
     #[test]
-    fn verify_valid_script_parsed() -> Result<()> {
+    fn verify_valid_script() -> Result<()> {
         zebra_test::init();
 
         let transaction =
@@ -167,39 +211,169 @@ mod tests {
             .branch_id()
             .expect("Blossom has a ConsensusBranchId");
 
-        is_valid(transaction, branch_id, (input_index, output))?;
+        let verifier = super::CachedFfiTransaction::new(transaction);
+        verifier.is_valid(branch_id, (input_index, output))?;
 
         Ok(())
     }
 
     #[test]
-    fn verify_valid_script() -> Result<()> {
+    fn fail_invalid_script() -> Result<()> {
         zebra_test::init();
 
-        let coin = i64::pow(10, 8);
+        let transaction =
-        let script_pub_key = &*SCRIPT_PUBKEY;
+            SCRIPT_TX.zcash_deserialize_into::<Arc<zebra_chain::transaction::Transaction>>()?;
-        let amount = 212 * coin;
+        let coin = u64::pow(10, 8);
-        let tx_to = &*SCRIPT_TX;
+        let amount = 211 * coin;
-        let n_in = 0;
+        let output = transparent::Output {
-        let branch_id = 0x2bb40e60;
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        let input_index = 0;
+        let branch_id = Blossom
+            .branch_id()
+            .expect("Blossom has a ConsensusBranchId");
 
-        verify_script(script_pub_key, amount, tx_to, n_in, branch_id)?;
+        let verifier = super::CachedFfiTransaction::new(transaction);
+        verifier
+            .is_valid(branch_id, (input_index, output))
+            .unwrap_err();
 
         Ok(())
     }
 
     #[test]
-    fn dont_verify_invalid_script() -> Result<()> {
+    fn reuse_script_verifier_pass_pass() -> Result<()> {
         zebra_test::init();
 
-        let coin = i64::pow(10, 8);
+        let coin = u64::pow(10, 8);
-        let script_pub_key = &*SCRIPT_PUBKEY;
+        let transaction =
-        let amount = 212 * coin;
+            SCRIPT_TX.zcash_deserialize_into::<Arc<zebra_chain::transaction::Transaction>>()?;
-        let tx_to = &*SCRIPT_TX;
-        let n_in = 0;
-        let branch_id = 0x2bb40e61;
 
-        verify_script(script_pub_key, amount, tx_to, n_in, branch_id).unwrap_err();
+        let verifier = super::CachedFfiTransaction::new(transaction);
+
+        let input_index = 0;
+        let branch_id = Blossom
+            .branch_id()
+            .expect("Blossom has a ConsensusBranchId");
+
+        let amount = 212 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier.is_valid(branch_id, (input_index, output))?;
+
+        let amount = 212 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier.is_valid(branch_id, (input_index, output))?;
+
+        Ok(())
+    }
+
+    #[test]
+    fn reuse_script_verifier_pass_fail() -> Result<()> {
+        zebra_test::init();
+
+        let coin = u64::pow(10, 8);
+        let transaction =
+            SCRIPT_TX.zcash_deserialize_into::<Arc<zebra_chain::transaction::Transaction>>()?;
+
+        let verifier = super::CachedFfiTransaction::new(transaction);
+
+        let input_index = 0;
+        let branch_id = Blossom
+            .branch_id()
+            .expect("Blossom has a ConsensusBranchId");
+
+        let amount = 212 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier.is_valid(branch_id, (input_index, output))?;
+
+        let amount = 211 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier
+            .is_valid(branch_id, (input_index, output))
+            .unwrap_err();
+
+        Ok(())
+    }
+
+    #[test]
+    fn reuse_script_verifier_fail_pass() -> Result<()> {
+        zebra_test::init();
+
+        let coin = u64::pow(10, 8);
+        let transaction =
+            SCRIPT_TX.zcash_deserialize_into::<Arc<zebra_chain::transaction::Transaction>>()?;
+
+        let verifier = super::CachedFfiTransaction::new(transaction);
+
+        let input_index = 0;
+        let branch_id = Blossom
+            .branch_id()
+            .expect("Blossom has a ConsensusBranchId");
+
+        let amount = 211 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier
+            .is_valid(branch_id, (input_index, output))
+            .unwrap_err();
+
+        let amount = 212 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier.is_valid(branch_id, (input_index, output))?;
+
+        Ok(())
+    }
+
+    #[test]
+    fn reuse_script_verifier_fail_fail() -> Result<()> {
+        zebra_test::init();
+
+        let coin = u64::pow(10, 8);
+        let transaction =
+            SCRIPT_TX.zcash_deserialize_into::<Arc<zebra_chain::transaction::Transaction>>()?;
+
+        let verifier = super::CachedFfiTransaction::new(transaction);
+
+        let input_index = 0;
+        let branch_id = Blossom
+            .branch_id()
+            .expect("Blossom has a ConsensusBranchId");
+
+        let amount = 211 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier
+            .is_valid(branch_id, (input_index, output))
+            .unwrap_err();
+
+        let amount = 210 * coin;
+        let output = transparent::Output {
+            value: amount.try_into()?,
+            lock_script: transparent::Script(SCRIPT_PUBKEY.clone()),
+        };
+        verifier
+            .is_valid(branch_id, (input_index, output))
+            .unwrap_err();
 
         Ok(())
     }