Reject UTXO double spends (#2511)

* Reject transparent output double-spends

Check that transparent spends use unspent outputs from:
* earlier transaction in the same block,
* earlier blocks in the parent non-finalized chain, or
* the finalized state.

* Fixup UTXOs in proptests

* Add a comment

* Clarify a consensus rule implementation

* Fix an incorrect comment

* Fix an incorrect error message

* Clarify a comment

* Document `unspent_utxos`

* Simplify the UTXO check

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

* Further simplify and fix the UTXO check

- split each error case into a separate check
- combine `contains` and `insert`
- add a missing check against the non-finalized unspent UTXOs
- rename arguments and edit error strings for clarity

* Share test methods between check test modules

* Make some chain fields available to tests

* Make error field names consistent with transparent::Input

* WIP: Add tests for UTXO double-spends

- accept output and spend in the same block
- accept output and spend in a later block
- reject output and double-spend all in the same block
- reject output then double-spend in a later block
- reject output, spend, then double-spend all in different blocks

* Use Extend rather than multiple pushes

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

* Use Extend for more pushes

* Limit the number of proptest cases, to speed up tests

* Test rejection of UTXOs that were never in the chain

* Test rejection of spends of later transactions in the same block

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

zebra-chain/src/block/arbitrary.rs

@@ -3,17 +3,19 @@ use proptest::{
     prelude::*,
 };
 
-use std::sync::Arc;
+use std::{collections::HashSet, convert::TryInto, sync::Arc};
 
 use crate::{
     block,
     fmt::SummaryDebug,
+    orchard,
     parameters::{
         Network,
         NetworkUpgrade::{self, *},
         GENESIS_PREVIOUS_BLOCK_HASH,
     },
     serialization,
+    transparent::Input::*,
     work::{difficulty::CompactDifficulty, equihash},
 };
 
@@ -326,14 +328,79 @@ impl Block {
             current.height.0 += 1;
         }
 
-        // after the vec strategy generates blocks, update the previous block hashes
+        // after the vec strategy generates blocks, fixup invalid parts of the blocks
         vec.prop_map(|mut vec| {
             let mut previous_block_hash = None;
+            let mut utxos = HashSet::<transparent::OutPoint>::new();
+
             for block in vec.iter_mut() {
+                // fixup the previous block hash
                 if let Some(previous_block_hash) = previous_block_hash {
                     block.header.previous_block_hash = previous_block_hash;
                 }
                 previous_block_hash = Some(block.hash());
+
+                // fixup the transparent spends
+                let mut new_transactions = Vec::new();
+                for transaction in block.transactions.drain(..) {
+                    let mut transaction = (*transaction).clone();
+                    let mut new_inputs = Vec::new();
+
+                    for mut input in transaction.inputs_mut().drain(..) {
+                        if let PrevOut {
+                            ref mut outpoint, ..
+                        } = input
+                        {
+                            // take a UTXO if available
+                            if utxos.remove(outpoint) {
+                                new_inputs.push(input);
+                            } else if let Some(arbitrary_utxo) = utxos.clone().iter().next() {
+                                *outpoint = *arbitrary_utxo;
+                                utxos.remove(arbitrary_utxo);
+                                new_inputs.push(input);
+                            }
+                            // otherwise, drop the invalid input, it has no UTXOs to spend
+                        } else {
+                            // preserve coinbase inputs
+                            new_inputs.push(input);
+                        }
+                    }
+
+                    // delete invalid inputs
+                    *transaction.inputs_mut() = new_inputs;
+
+                    // keep transactions with valid input counts
+                    // coinbase transactions will never fail this check
+                    // this is the input check from `has_inputs_and_outputs`
+                    if !transaction.inputs().is_empty()
+                        || transaction.joinsplit_count() > 0
+                        || transaction.sapling_spends_per_anchor().count() > 0
+                        || (transaction.orchard_actions().count() > 0
+                            && transaction
+                                .orchard_flags()
+                                .unwrap_or_else(orchard::Flags::empty)
+                                .contains(orchard::Flags::ENABLE_SPENDS))
+                    {
+                        // add the created UTXOs
+                        // these outputs can be spent from the next transaction in this block onwards
+                        // see `new_outputs` for details
+                        let hash = transaction.hash();
+                        for output_index_in_transaction in 0..transaction.outputs().len() {
+                            utxos.insert(transparent::OutPoint {
+                                hash,
+                                index: output_index_in_transaction.try_into().unwrap(),
+                            });
+                        }
+
+                        // and keep the transaction
+                        new_transactions.push(Arc::new(transaction));
+                    }
+                }
+
+                // delete invalid transactions
+                block.transactions = new_transactions;
+
+                // TODO: fixup the history and authorizing data commitments, if needed
             }
             SummaryDebug(vec.into_iter().map(Arc::new).collect())
         })

zebra-chain/src/transaction.rs

@@ -223,6 +223,18 @@ impl Transaction {
         }
     }
 
+    /// Modify the transparent inputs of this transaction, regardless of version.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn inputs_mut(&mut self) -> &mut Vec<transparent::Input> {
+        match self {
+            Transaction::V1 { ref mut inputs, .. } => inputs,
+            Transaction::V2 { ref mut inputs, .. } => inputs,
+            Transaction::V3 { ref mut inputs, .. } => inputs,
+            Transaction::V4 { ref mut inputs, .. } => inputs,
+            Transaction::V5 { ref mut inputs, .. } => inputs,
+        }
+    }
+
     /// Access the transparent outputs of this transaction, regardless of version.
     pub fn outputs(&self) -> &[transparent::Output] {
         match self {

zebra-chain/src/transparent.rs

@@ -103,6 +103,37 @@ pub enum Input {
     },
 }
 
+impl Input {
+    /// If this is a `PrevOut` input, returns this input's outpoint.
+    /// Otherwise, returns `None`.
+    pub fn outpoint(&self) -> Option<OutPoint> {
+        if let Input::PrevOut { outpoint, .. } = self {
+            Some(*outpoint)
+        } else {
+            None
+        }
+    }
+
+    /// Set this input's outpoint.
+    ///
+    /// Should only be called on `PrevOut` inputs.
+    ///
+    /// # Panics
+    ///
+    /// If `self` is a coinbase input.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn set_outpoint(&mut self, new_outpoint: OutPoint) {
+        if let Input::PrevOut {
+            ref mut outpoint, ..
+        } = self
+        {
+            *outpoint = new_outpoint;
+        } else {
+            unreachable!("unexpected variant: Coinbase Inputs do not have OutPoints");
+        }
+    }
+}
+
 /// A transparent output from a transaction.
 ///
 /// The most fundamental building block of a transaction is a

zebra-state/src/error.rs

@@ -3,7 +3,9 @@ use std::sync::Arc;
 use chrono::{DateTime, Utc};
 use thiserror::Error;
 
-use zebra_chain::{block, orchard, sapling, sprout, work::difficulty::CompactDifficulty};
+use zebra_chain::{
+    block, orchard, sapling, sprout, transparent, work::difficulty::CompactDifficulty,
+};
 
 /// A wrapper for type erased errors that is itself clonable and implements the
 /// Error trait
@@ -75,6 +77,24 @@ pub enum ValidateContextError {
         expected_difficulty: CompactDifficulty,
     },
 
+    #[error("transparent double-spend: {outpoint:?} is spent twice in {location:?}")]
+    #[non_exhaustive]
+    DuplicateTransparentSpend {
+        outpoint: transparent::OutPoint,
+        location: &'static str,
+    },
+
+    #[error("missing transparent output: possible double-spend of {outpoint:?} in {location:?}")]
+    #[non_exhaustive]
+    MissingTransparentOutput {
+        outpoint: transparent::OutPoint,
+        location: &'static str,
+    },
+
+    #[error("out-of-order transparent spend: {outpoint:?} is created by a later transaction in the same block")]
+    #[non_exhaustive]
+    EarlyTransparentSpend { outpoint: transparent::OutPoint },
+
     #[error("sprout double-spend: duplicate nullifier: {nullifier:?}, in finalized state: {in_finalized_state:?}")]
     #[non_exhaustive]
     DuplicateSproutNullifier {

zebra-state/src/service/arbitrary.rs

@@ -1,17 +1,19 @@
+use std::sync::Arc;
+
 use proptest::{
     num::usize::BinarySearch,
+    prelude::*,
     strategy::{NewTree, ValueTree},
     test_runner::TestRunner,
 };
-use std::sync::Arc;
 
 use zebra_chain::{
     block::{Block, Height},
     fmt::SummaryDebug,
-    parameters::NetworkUpgrade,
+    parameters::{Network::*, NetworkUpgrade},
+    serialization::ZcashDeserializeInto,
     LedgerState,
 };
-use zebra_test::prelude::*;
 
 use crate::tests::Prepare;
 
@@ -146,3 +148,46 @@ pub(crate) fn partial_nu5_chain_strategy(
             .prop_map(move |partial_chain| (network, nu_activation, partial_chain))
         })
 }
+
+/// Return a new `StateService` containing the mainnet genesis block.
+/// Also returns the finalized genesis block itself.
+pub(super) fn new_state_with_mainnet_genesis() -> (StateService, FinalizedBlock) {
+    let genesis = zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES
+        .zcash_deserialize_into::<Arc<Block>>()
+        .expect("block should deserialize");
+
+    let mut state = StateService::new(Config::ephemeral(), Mainnet);
+
+    assert_eq!(None, state.best_tip());
+
+    let genesis = FinalizedBlock::from(genesis);
+    state
+        .disk
+        .commit_finalized_direct(genesis.clone(), "test")
+        .expect("unexpected invalid genesis block test vector");
+
+    assert_eq!(Some((Height(0), genesis.hash)), state.best_tip());
+
+    (state, genesis)
+}
+
+/// Return a `Transaction::V4` with the coinbase data from `coinbase`.
+///
+/// Used to convert a coinbase transaction to a version that the non-finalized state will accept.
+pub(super) fn transaction_v4_from_coinbase(coinbase: &Transaction) -> Transaction {
+    assert!(
+        !coinbase.has_sapling_shielded_data(),
+        "conversion assumes sapling shielded data is None"
+    );
+
+    Transaction::V4 {
+        inputs: coinbase.inputs().to_vec(),
+        outputs: coinbase.outputs().to_vec(),
+        lock_time: coinbase.lock_time(),
+        // `Height(0)` means that the expiry height is ignored
+        expiry_height: coinbase.expiry_height().unwrap_or(Height(0)),
+        // invalid for coinbase transactions
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    }
+}

zebra-state/src/service/check.rs

@@ -19,6 +19,7 @@ use difficulty::{AdjustedDifficulty, POW_MEDIAN_BLOCK_SPAN};
 
 pub(crate) mod difficulty;
 pub(crate) mod nullifier;
+pub(crate) mod utxo;
 
 #[cfg(test)]
 mod tests;

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

@@ -1,4 +1,5 @@
 //! Tests for state contextual validation checks.
 
 mod nullifier;
+mod utxo;
 mod vectors;

zebra-state/src/service/check/tests/nullifier.rs

@@ -1,4 +1,4 @@
-//! Randomised property tests for state contextual validation
+//! Randomised property tests for nullifier contextual validation
 
 use std::{convert::TryInto, sync::Arc};
 
@@ -9,7 +9,7 @@ use zebra_chain::{
     block::{Block, Height},
     fmt::TypeNameToDebug,
     orchard,
-    parameters::{Network::*, NetworkUpgrade::Nu5},
+    parameters::NetworkUpgrade::Nu5,
     primitives::Groth16Proof,
     sapling::{self, FieldNotPresent, PerSpendAnchor, TransferData::*},
     serialization::ZcashDeserializeInto,
@@ -18,8 +18,7 @@ use zebra_chain::{
 };
 
 use crate::{
-    config::Config,
-    service::StateService,
+    service::arbitrary::{new_state_with_mainnet_genesis, transaction_v4_from_coinbase},
     tests::Prepare,
     FinalizedBlock,
     ValidateContextError::{
@@ -848,28 +847,6 @@ proptest! {
     }
 }
 
-/// Return a new `StateService` containing the mainnet genesis block.
-/// Also returns the finalized genesis block itself.
-fn new_state_with_mainnet_genesis() -> (StateService, FinalizedBlock) {
-    let genesis = zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES
-        .zcash_deserialize_into::<Arc<Block>>()
-        .expect("block should deserialize");
-
-    let mut state = StateService::new(Config::ephemeral(), Mainnet);
-
-    assert_eq!(None, state.best_tip());
-
-    let genesis = FinalizedBlock::from(genesis);
-    state
-        .disk
-        .commit_finalized_direct(genesis.clone(), "test")
-        .expect("unexpected invalid genesis block test vector");
-
-    assert_eq!(Some((Height(0), genesis.hash)), state.best_tip());
-
-    (state, genesis)
-}
-
 /// Make sure the supplied nullifiers are distinct, modifying them if necessary.
 fn make_distinct_nullifiers<'until_modified, NullifierT>(
     nullifiers: impl IntoIterator<Item = &'until_modified mut NullifierT>,
@@ -1042,24 +1019,3 @@ fn transaction_v5_with_orchard_shielded_data(
         orchard_shielded_data,
     }
 }
-
-/// Return a `Transaction::V4` with the coinbase data from `coinbase`.
-///
-/// Used to convert a coinbase transaction to a version that the non-finalized state will accept.
-fn transaction_v4_from_coinbase(coinbase: &Transaction) -> Transaction {
-    assert!(
-        !coinbase.has_sapling_shielded_data(),
-        "conversion assumes sapling shielded data is None"
-    );
-
-    Transaction::V4 {
-        inputs: coinbase.inputs().to_vec(),
-        outputs: coinbase.outputs().to_vec(),
-        lock_time: coinbase.lock_time(),
-        // `Height(0)` means that the expiry height is ignored
-        expiry_height: coinbase.expiry_height().unwrap_or(Height(0)),
-        // invalid for coinbase transactions
-        joinsplit_data: None,
-        sapling_shielded_data: None,
-    }
-}