refactor: address comments from #3415: Prepare for changes in ZIP-244

zebra-chain/src/transaction/tests/vectors.rs

@@ -32,6 +32,16 @@ lazy_static! {
     };
 }
 
+/// Build a mock output list for pre-V5 transactions, with (index+1)
+/// copies of `output`, which is used to computed the sighash.
+///
+/// Pre-V5, the entire output list is not used; only the output for the
+/// given index is read. Therefore, we just need a list where `array[index]`
+/// is the given `output`.
+fn mock_pre_v5_output_list(output: transparent::Output, index: usize) -> Vec<transparent::Output> {
+    iter::repeat(output).take(index + 1).collect()
+}
+
 #[test]
 fn transactionhash_struct_from_str_roundtrip() {
     zebra_test::init();
@@ -612,7 +622,7 @@ fn test_vec143_1() -> Result<()> {
         &transaction,
         HashType::ALL,
         NetworkUpgrade::Overwinter,
-        Default::default(),
+        &[],
         None,
     );
 
@@ -641,13 +651,12 @@ fn test_vec143_2() -> Result<()> {
     let lock_script = Script::new(&hex::decode("53")?);
     let input_ind = 1;
     let output = transparent::Output { value, lock_script };
-    let all_previous_outputs = vec![output.clone(), output];
+    let all_previous_outputs = mock_pre_v5_output_list(output, input_ind);
 
     let hasher = SigHasher::new(
         &transaction,
         HashType::SINGLE,
         NetworkUpgrade::Overwinter,
-        // Pre-V5, only the matching output matters, so just use clones for the rest
         &all_previous_outputs,
         Some(input_ind),
     );
@@ -678,7 +687,7 @@ fn test_vec243_1() -> Result<()> {
         &transaction,
         HashType::ALL,
         NetworkUpgrade::Sapling,
-        Default::default(),
+        &[],
         None,
     );
 
@@ -717,13 +726,12 @@ fn test_vec243_2() -> Result<()> {
     let lock_script = Script::new(&[]);
     let input_ind = 1;
     let output = transparent::Output { value, lock_script };
-    let all_previous_outputs = vec![output.clone(), output];
+    let all_previous_outputs = mock_pre_v5_output_list(output, input_ind);
 
     let hasher = SigHasher::new(
         &transaction,
         HashType::NONE,
         NetworkUpgrade::Sapling,
-        // Pre-V5, only the matching output matters, so just use clones for the rest
         &all_previous_outputs,
         Some(input_ind),
     );
@@ -743,13 +751,13 @@ fn test_vec243_2() -> Result<()> {
     let lock_script = Script::new(&[]);
     let prevout = transparent::Output { value, lock_script };
     let index = input_ind as usize;
+    let all_previous_outputs = mock_pre_v5_output_list(prevout, input_ind);
 
     let alt_sighash = crate::primitives::zcash_primitives::sighash(
         &transaction,
         HashType::NONE,
         NetworkUpgrade::Sapling,
-        // Pre-V5, only the matching output matters, so just use clones for the rest
-        &[prevout.clone(), prevout],
+        &all_previous_outputs,
         Some(index),
     );
     let result = hex::encode(alt_sighash);
@@ -826,9 +834,8 @@ fn zip143_sighash() -> Result<()> {
             ),
             None => (None, None),
         };
-        // Pre-V5, only the matching output matters, so just use clones for the rest
         let all_previous_outputs: Vec<_> = match output {
-            Some(output) => (0..=input_index.unwrap()).map(|_| output.clone()).collect(),
+            Some(output) => mock_pre_v5_output_list(output, input_index.unwrap()),
             None => vec![],
         };
         let result = hex::encode(
@@ -863,9 +870,8 @@ fn zip243_sighash() -> Result<()> {
             ),
             None => (None, None),
         };
-        // Pre-V5, only the matching output matters, so just use clones for the rest
         let all_previous_outputs: Vec<_> = match output {
-            Some(output) => (0..=input_index.unwrap()).map(|_| output.clone()).collect(),
+            Some(output) => mock_pre_v5_output_list(output, input_index.unwrap()),
             None => vec![],
         };
         let result = hex::encode(
@@ -908,9 +914,8 @@ fn zip244_sighash() -> Result<()> {
             ),
             None => (None, None),
         };
-        // Pre-V5, only the matching output matters, so just use clones for the rest
         let all_previous_outputs: Vec<_> = match output {
-            Some(output) => (0..=input_index.unwrap()).map(|_| output.clone()).collect(),
+            Some(output) => mock_pre_v5_output_list(output, input_index.unwrap()),
             None => vec![],
         };
         let result = hex::encode(transaction.sighash(
@@ -955,8 +960,7 @@ fn binding_signatures_for_network(network: Network) {
                     ..
                 } => {
                     if let Some(sapling_shielded_data) = sapling_shielded_data {
-                        let shielded_sighash =
-                            tx.sighash(upgrade, HashType::ALL, Default::default(), None);
+                        let shielded_sighash = tx.sighash(upgrade, HashType::ALL, &[], None);
 
                         let bvk = redjubjub::VerificationKey::try_from(
                             sapling_shielded_data.binding_verification_key(),
@@ -975,8 +979,7 @@ fn binding_signatures_for_network(network: Network) {
                     ..
                 } => {
                     if let Some(sapling_shielded_data) = sapling_shielded_data {
-                        let shielded_sighash =
-                            tx.sighash(upgrade, HashType::ALL, Default::default(), None);
+                        let shielded_sighash = tx.sighash(upgrade, HashType::ALL, &[], None);
 
                         let bvk = redjubjub::VerificationKey::try_from(
                             sapling_shielded_data.binding_verification_key(),

zebra-consensus/src/block/tests.rs

@@ -22,7 +22,7 @@ use zebra_chain::{
 use zebra_script::CachedFfiTransaction;
 use zebra_test::transcript::{ExpectedTranscriptError, Transcript};
 
-use crate::{parameters::SLOW_START_SHIFT, script, transaction};
+use crate::{parameters::SLOW_START_SHIFT, transaction};
 
 use super::*;
 
@@ -125,8 +125,7 @@ async fn check_transcripts() -> Result<(), Report> {
     let network = Network::Mainnet;
     let state_service = zebra_state::init_test(network);
 
-    let script = script::Verifier::new();
-    let transaction = transaction::Verifier::new(network, state_service.clone(), script);
+    let transaction = transaction::Verifier::new(network, state_service.clone());
     let transaction = Buffer::new(BoxService::new(transaction), 1);
     let block_verifier = Buffer::new(
         BlockVerifier::new(network, state_service.clone(), transaction),

zebra-consensus/src/chain.rs

@@ -38,7 +38,7 @@ use crate::{
     block::VerifyBlockError,
     checkpoint::{CheckpointList, CheckpointVerifier, VerifyCheckpointError},
     error::TransactionError,
-    script, transaction, BoxError, Config,
+    transaction, BoxError, Config,
 };
 
 #[cfg(test)]
@@ -229,8 +229,7 @@ where
 
     // transaction verification
 
-    let script = script::Verifier::new();
-    let transaction = transaction::Verifier::new(network, state_service.clone(), script);
+    let transaction = transaction::Verifier::new(network, state_service.clone());
     let transaction = Buffer::new(BoxService::new(transaction), VERIFIER_BUFFER_BOUND);
 
     // block verification

zebra-consensus/src/script.rs

@@ -18,14 +18,8 @@ use crate::BoxError;
 /// The asynchronous script verification design is documented in [RFC4].
 ///
 /// [RFC4]: https://zebra.zfnd.org/dev/rfcs/0004-asynchronous-script-verification.html
-#[derive(Debug, Clone, Default)]
-pub struct Verifier {}
-
-impl Verifier {
-    pub fn new() -> Self {
-        Self {}
-    }
-}
+#[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
+pub struct Verifier;
 
 /// A script verification request.
 #[derive(Debug)]

zebra-consensus/src/transaction.rs

@@ -13,7 +13,7 @@ use std::{
 use chrono::{DateTime, Utc};
 use futures::{
     stream::{FuturesUnordered, StreamExt},
-    FutureExt, TryFutureExt,
+    FutureExt,
 };
 use tower::{timeout::Timeout, Service, ServiceExt};
 use tracing::Instrument;
@@ -73,11 +73,11 @@ where
     ZS::Future: Send + 'static,
 {
     /// Create a new transaction verifier.
-    pub fn new(network: Network, state: ZS, script_verifier: script::Verifier) -> Self {
+    pub fn new(network: Network, state: ZS) -> Self {
         Self {
             network,
             state: Timeout::new(state, UTXO_LOOKUP_TIMEOUT),
-            script_verifier,
+            script_verifier: script::Verifier::default(),
         }
     }
 }
@@ -294,7 +294,7 @@ where
 
     // TODO: break up each chunk into its own method
     fn call(&mut self, req: Request) -> Self::Future {
-        let script_verifier = self.script_verifier.clone();
+        let script_verifier = self.script_verifier;
         let network = self.network;
         let state = self.state.clone();
 
@@ -457,31 +457,26 @@ where
         let mut spent_utxos = HashMap::new();
         let mut spent_outputs = Vec::new();
         for input in inputs {
-            match input {
-                transparent::Input::PrevOut {
-                    outpoint,
-                    unlock_script: _,
-                    sequence: _,
-                } => {
-                    tracing::trace!("awaiting outpoint lookup");
-                    let utxo = if let Some(output) = known_utxos.get(outpoint) {
-                        tracing::trace!("UXTO in known_utxos, discarding query");
-                        output.utxo.clone()
+            if let transparent::Input::PrevOut { outpoint, .. } = input {
+                tracing::trace!("awaiting outpoint lookup");
+                let utxo = if let Some(output) = known_utxos.get(outpoint) {
+                    tracing::trace!("UXTO in known_utxos, discarding query");
+                    output.utxo.clone()
+                } else {
+                    let query = state
+                        .clone()
+                        .oneshot(zebra_state::Request::AwaitUtxo(*outpoint));
+                    if let zebra_state::Response::Utxo(utxo) = query.await? {
+                        utxo
                     } else {
-                        let query = state
-                            .clone()
-                            .oneshot(zebra_state::Request::AwaitUtxo(*outpoint));
-                        if let zebra_state::Response::Utxo(utxo) = query.await? {
-                            utxo
-                        } else {
-                            unreachable!("AwaitUtxo always responds with Utxo")
-                        }
-                    };
-                    tracing::trace!(?utxo, "got UTXO");
-                    spent_outputs.push(utxo.output.clone());
-                    spent_utxos.insert(*outpoint, utxo);
-                }
-                transparent::Input::Coinbase { .. } => continue,
+                        unreachable!("AwaitUtxo always responds with Utxo")
+                    }
+                };
+                tracing::trace!(?utxo, "got UTXO");
+                spent_outputs.push(utxo.output.clone());
+                spent_utxos.insert(*outpoint, utxo);
+            } else {
+                continue;
             }
         }
         Ok((spent_utxos, spent_outputs))
@@ -689,7 +684,7 @@ where
                         input_index,
                     };
 
-                    script_verifier.clone().oneshot(request).map_ok(|_r| {})
+                    script_verifier.oneshot(request)
                 })
                 .collect();