diff --git a/Cargo.lock b/Cargo.lock index 7636f355..596496dc 100644 --- a/Cargo.lock +++ b/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", diff --git a/zebra-consensus/src/script.rs b/zebra-consensus/src/script.rs index 3a69cd19..6b5a9821 100644 --- a/zebra-consensus/src/script.rs +++ b/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, + pub cached_ffi_transaction: Arc, 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 { - zebra_script::is_valid( - transaction, - 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" - ); - } + cached_ffi_transaction + .is_valid(branch_id, (input_index as u32, utxo.output))?; + tracing::trace!("script verification succeeded"); Ok(()) } diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 5fdd0869..a06e7758 100644 --- a/zebra-consensus/src/transaction.rs +++ b/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, }); diff --git a/zebra-script/Cargo.toml b/zebra-script/Cargo.toml index a97ed0eb..566448a7 100644 --- a/zebra-script/Cargo.toml +++ b/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 "] 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" diff --git a/zebra-script/src/lib.rs b/zebra-script/src/lib.rs index 1ec45769..1bdb9d68 100644 --- a/zebra-script/src/lib.rs +++ b/zebra-script/src/lib.rs @@ -56,85 +56,129 @@ impl From for Error { } } -/// Thin safe wrapper around ffi interface provided by libzcash_script -fn verify_script( - script_pub_key: impl AsRef<[u8]>, - amount: i64, - tx_to: impl AsRef<[u8]>, - n_in: u32, - consensus_branch_id: u32, -) -> Result<(), Error> { - let script_pub_key = script_pub_key.as_ref(); - let tx_to = tx_to.as_ref(); +/// A preprocessed Transction which can be used to verify scripts within said +/// Transaction. +#[derive(Debug)] +pub struct CachedFfiTransaction { + transaction: Arc, + precomputed: *mut std::ffi::c_void, +} - let script_ptr = script_pub_key.as_ptr(); - let script_len = script_pub_key.len(); - let tx_to_ptr = tx_to.as_ptr(); - let tx_to_len = tx_to.len(); - let mut err = 0; +impl CachedFfiTransaction { + /// Construct a `PrecomputedTransaction` from a `Transaction`. + pub fn new(transaction: Arc) -> Self { + let tx_to = transaction + .zcash_serialize_to_vec() + .expect("serialization into a vec is infallible"); - let flags = zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_P2SH - | zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY; + let tx_to_ptr = tx_to.as_ptr(); + let tx_to_len = tx_to.len() as u32; + let mut err = 0; - let ret = unsafe { - zcash_script::zcash_script_verify( - 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, - ) - }; + let precomputed = unsafe { + zcash_script::zcash_script_new_precomputed_tx(tx_to_ptr, tx_to_len, &mut err) + }; - if ret == 1 { - Ok(()) - } else { - Err(Error::from(err)) + Self { + transaction, + precomputed, + } + } + + 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 -/// `transparent::Output` it is spending and the `ConsensusBranchId` of the block -/// containing the transaction. -/// -/// # Details -/// -/// input index corresponds to the index of the `TransparentInput` which in -/// `transaction` used to identify the `previous_output` -pub fn is_valid( - transaction: Arc, - branch_id: ConsensusBranchId, - (input_index, previous_output): (u32, transparent::Output), -) -> Result<(), Error> { - assert!((input_index as usize) < transaction.inputs().len()); +// # SAFETY +// +// ## Justification +// +// `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 +// data to be mutated from different threads if copied. +// +// CachedFFiTransaction needs to be Send and Sync to be stored within a `Box`. In `zebra_consensus/src/transaction.rs`, an +// async block owns a `CachedFfiTransaction`, and holds it across an await +// point, while the transaction verifier is spawning all of the script verifier +// futures. The service readiness check requires this await between each task +// spawn. Each `script` future needs a copy of the +// `Arc` 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 - .zcash_serialize_to_vec() - .expect("serialization into a vec is infallible"); - - let transparent::Output { value, lock_script } = previous_output; - - verify_script( - &lock_script.0, - value.into(), - &tx_to, - input_index as _, - branch_id.into(), - )?; - - Ok(()) +impl Drop for CachedFfiTransaction { + fn drop(&mut self) { + unsafe { zcash_script::zcash_script_free_precomputed_tx(self.precomputed) }; + } } #[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 script_pub_key = &*SCRIPT_PUBKEY; - let amount = 212 * coin; - let tx_to = &*SCRIPT_TX; - let n_in = 0; - let branch_id = 0x2bb40e60; + let transaction = + SCRIPT_TX.zcash_deserialize_into::>()?; + let coin = u64::pow(10, 8); + let amount = 211 * coin; + let output = transparent::Output { + 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 script_pub_key = &*SCRIPT_PUBKEY; - let amount = 212 * coin; - let tx_to = &*SCRIPT_TX; - let n_in = 0; - let branch_id = 0x2bb40e61; + let coin = u64::pow(10, 8); + let transaction = + SCRIPT_TX.zcash_deserialize_into::>()?; - 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::>()?; + + 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::>()?; + + 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::>()?; + + 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(()) }