Reject duplicate Sapling and Orchard nullifiers (#2497)

* Add sapling and orchard duplicate nullifier errors

* Reject duplicate finalized sapling and orchard nullifiers

Reject duplicate sapling and orchard nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is already in the finalized state.

* Reject duplicate non-finalized sapling and orchard nullifiers

Reject duplicate sapling and orchard nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is in:
* the same shielded data,
* the same transaction,
* the same block, or
* an earlier block in the non-finalized chain.

* Refactor sprout nullifier tests to remove common code

* Add sapling nullifier tests

Test that the state rejects duplicate sapling nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is in:
* the same shielded data,
* the same transaction,
* the same block,
* an earlier block in the non-finalized chain, or
* the finalized state.

* Add orchard nullifier tests

Test that the state rejects duplicate orchard nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is in:
* the same shielded data,
* the same transaction,
* the same block,
* an earlier block in the non-finalized chain, or
* the finalized state.

* Check for specific nullifiers in the state in tests

* Replace slices with vectors in arguments

* Remove redundant code and variables

* Simplify sapling TransferData tests

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

* Remove an extra :

* Remove redundant vec!

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

zebra-state/src/error.rs

@@ -3,7 +3,7 @@ use std::sync::Arc;
 use chrono::{DateTime, Utc};
 use thiserror::Error;
 
-use zebra_chain::{block, sprout, work::difficulty::CompactDifficulty};
+use zebra_chain::{block, orchard, sapling, sprout, work::difficulty::CompactDifficulty};
 
 /// A wrapper for type erased errors that is itself clonable and implements the
 /// Error trait
@@ -81,6 +81,20 @@ pub enum ValidateContextError {
         nullifier: sprout::Nullifier,
         in_finalized_state: bool,
     },
+
+    #[error("sapling double-spend: duplicate nullifier: {nullifier:?}, in finalized state: {in_finalized_state:?}")]
+    #[non_exhaustive]
+    DuplicateSaplingNullifier {
+        nullifier: sapling::Nullifier,
+        in_finalized_state: bool,
+    },
+
+    #[error("orchard double-spend: duplicate nullifier: {nullifier:?}, in finalized state: {in_finalized_state:?}")]
+    #[non_exhaustive]
+    DuplicateOrchardNullifier {
+        nullifier: orchard::Nullifier,
+        in_finalized_state: bool,
+    },
 }
 
 /// Trait for creating the corresponding duplicate nullifier error from a nullifier.
@@ -97,3 +111,21 @@ impl DuplicateNullifierError for sprout::Nullifier {
         }
     }
 }
+
+impl DuplicateNullifierError for sapling::Nullifier {
+    fn duplicate_nullifier_error(&self, in_finalized_state: bool) -> ValidateContextError {
+        ValidateContextError::DuplicateSaplingNullifier {
+            nullifier: *self,
+            in_finalized_state,
+        }
+    }
+}
+
+impl DuplicateNullifierError for orchard::Nullifier {
+    fn duplicate_nullifier_error(&self, in_finalized_state: bool) -> ValidateContextError {
+        ValidateContextError::DuplicateOrchardNullifier {
+            nullifier: *self,
+            in_finalized_state,
+        }
+    }
+}

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

@@ -37,7 +37,17 @@ pub(crate) fn no_duplicates_in_finalized_chain(
         }
     }
 
-    // TODO: sapling and orchard nullifiers (#2231)
+    for nullifier in prepared.block.sapling_nullifiers() {
+        if finalized_state.contains_sapling_nullifier(nullifier) {
+            Err(nullifier.duplicate_nullifier_error(true))?;
+        }
+    }
+
+    for nullifier in prepared.block.orchard_nullifiers() {
+        if finalized_state.contains_orchard_nullifier(nullifier) {
+            Err(nullifier.duplicate_nullifier_error(true))?;
+        }
+    }
 
     Ok(())
 }

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

@@ -1,4 +1,4 @@
-//! Randomised property tests for state contextual validation nullifier: (), in_finalized_state: ()  nullifier: (), in_finalized_state: ()  checks.
+//! Randomised property tests for state contextual validation
 
 use std::{convert::TryInto, sync::Arc};
 
@@ -8,16 +8,23 @@ use proptest::prelude::*;
 use zebra_chain::{
     block::{Block, Height},
     fmt::TypeNameToDebug,
-    parameters::Network::*,
+    orchard,
+    parameters::{Network::*, NetworkUpgrade::Nu5},
     primitives::Groth16Proof,
+    sapling::{self, FieldNotPresent, PerSpendAnchor, TransferData::*},
     serialization::ZcashDeserializeInto,
-    sprout::{self, JoinSplit},
+    sprout::JoinSplit,
     transaction::{JoinSplitData, LockTime, Transaction},
 };
 
 use crate::{
-    config::Config, service::StateService, tests::Prepare, FinalizedBlock,
+    config::Config,
-    ValidateContextError::DuplicateSproutNullifier,
+    service::StateService,
+    tests::Prepare,
+    FinalizedBlock,
+    ValidateContextError::{
+        DuplicateOrchardNullifier, DuplicateSaplingNullifier, DuplicateSproutNullifier,
+    },
 };
 
 // These tests use the `Arbitrary` trait to easily generate complex types,
@@ -27,6 +34,9 @@ use crate::{
 // but the differences shouldn't matter,
 // because we're only interested in spend validation,
 // (and passing various other state checks).
+
+// sprout
+
 proptest! {
     /// Make sure an arbitrary sprout nullifier is accepted by state contextual validation.
     ///
@@ -35,7 +45,7 @@ proptest! {
     #[test]
     fn accept_distinct_arbitrary_sprout_nullifiers(
         mut joinsplit in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
         use_finalized_state in any::<bool>(),
     ) {
         zebra_test::init();
@@ -45,12 +55,9 @@ proptest! {
             .expect("block should deserialize");
 
         make_distinct_nullifiers(&mut joinsplit.nullifiers);
+        let expected_nullifiers = joinsplit.nullifiers;
 
-        // make sure there are no other nullifiers
+        let transaction = transaction_v4_with_joinsplit_data(joinsplit_data.0, [joinsplit.0]);
-        joinsplit_data.first = joinsplit.0;
-        joinsplit_data.rest = Vec::new();
-
-        let transaction = transaction_v4_with_joinsplit_data(joinsplit_data.0);
 
         // convert the coinbase transaction to a version that the non-finalized state will accept
         block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
@@ -67,17 +74,32 @@ proptest! {
             let block1 = FinalizedBlock::from(Arc::new(block1));
             let commit_result = state.disk.commit_finalized_direct(block1.clone(), "test");
 
+            // the block was committed
             prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
             prop_assert!(commit_result.is_ok());
+
+            // the non-finalized state didn't change
             prop_assert!(state.mem.eq_internal_state(&previous_mem));
+
+            // the finalized state has the nullifiers
+            prop_assert!(state.disk.contains_sprout_nullifier(&expected_nullifiers[0]));
+            prop_assert!(state.disk.contains_sprout_nullifier(&expected_nullifiers[1]));
         } else {
             let block1 = Arc::new(block1).prepare();
             let commit_result =
                 state.validate_and_commit(block1.clone());
 
+            // the block was committed
             prop_assert_eq!(commit_result, Ok(()));
             prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+
+            // the block data is in the non-finalized state
             prop_assert!(!state.mem.eq_internal_state(&previous_mem));
+
+            // the non-finalized state has the nullifiers
+            prop_assert_eq!(state.mem.chain_set.len(), 1);
+            prop_assert!(state.mem.best_contains_sprout_nullifier(&expected_nullifiers[0]));
+            prop_assert!(state.mem.best_contains_sprout_nullifier(&expected_nullifiers[1]));
         }
     }
 
@@ -86,7 +108,7 @@ proptest! {
     #[test]
     fn reject_duplicate_sprout_nullifiers_in_joinsplit(
         mut joinsplit in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
     ) {
         zebra_test::init();
 
@@ -99,10 +121,7 @@ proptest! {
         let duplicate_nullifier = joinsplit.nullifiers[0];
         joinsplit.nullifiers[1] = duplicate_nullifier;
 
-        joinsplit_data.first = joinsplit.0;
+        let transaction = transaction_v4_with_joinsplit_data(joinsplit_data.0, [joinsplit.0]);
-        joinsplit_data.rest = Vec::new();
-
-        let transaction = transaction_v4_with_joinsplit_data(joinsplit_data.0);
 
         block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
 
@@ -139,7 +158,7 @@ proptest! {
     fn reject_duplicate_sprout_nullifiers_in_transaction(
         mut joinsplit1 in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
         mut joinsplit2 in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
     ) {
         zebra_test::init();
 
@@ -153,11 +172,10 @@ proptest! {
         let duplicate_nullifier = joinsplit1.nullifiers[0];
         joinsplit2.nullifiers[0] = duplicate_nullifier;
 
-        // make sure there are no other nullifiers
+        let transaction = transaction_v4_with_joinsplit_data(
-        joinsplit_data.first = joinsplit1.0;
+            joinsplit_data.0,
-        joinsplit_data.rest = vec![joinsplit2.0];
+            [joinsplit1.0, joinsplit2.0]
-
+        );
-        let transaction = transaction_v4_with_joinsplit_data(joinsplit_data.0);
 
         block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
 
@@ -191,8 +209,8 @@ proptest! {
     fn reject_duplicate_sprout_nullifiers_in_block(
         mut joinsplit1 in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
         mut joinsplit2 in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data1 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data1 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data2 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data2 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
     ) {
         zebra_test::init();
 
@@ -206,15 +224,8 @@ proptest! {
         let duplicate_nullifier = joinsplit1.nullifiers[0];
         joinsplit2.nullifiers[0] = duplicate_nullifier;
 
-        // make sure there are no other nullifiers
+        let transaction1 = transaction_v4_with_joinsplit_data(joinsplit_data1.0, [joinsplit1.0]);
-        joinsplit_data1.first = joinsplit1.0;
+        let transaction2 = transaction_v4_with_joinsplit_data(joinsplit_data2.0, [joinsplit2.0]);
-        joinsplit_data1.rest = Vec::new();
-
-        joinsplit_data2.first = joinsplit2.0;
-        joinsplit_data2.rest = Vec::new();
-
-        let transaction1 = transaction_v4_with_joinsplit_data(joinsplit_data1.0);
-        let transaction2 = transaction_v4_with_joinsplit_data(joinsplit_data2.0);
 
         block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
 
@@ -251,8 +262,8 @@ proptest! {
     fn reject_duplicate_sprout_nullifiers_in_chain(
         mut joinsplit1 in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
         mut joinsplit2 in TypeNameToDebug::<JoinSplit::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data1 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data1 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
-        mut joinsplit_data2 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
+        joinsplit_data2 in TypeNameToDebug::<JoinSplitData::<Groth16Proof>>::arbitrary(),
         duplicate_in_finalized_state in any::<bool>(),
     ) {
         zebra_test::init();
@@ -265,20 +276,14 @@ proptest! {
             .expect("block should deserialize");
 
         make_distinct_nullifiers(&mut joinsplit1.nullifiers.iter_mut().chain(joinsplit2.nullifiers.iter_mut()));
+        let expected_nullifiers = joinsplit1.nullifiers;
 
         // create a double-spend across two blocks
         let duplicate_nullifier = joinsplit1.nullifiers[0];
         joinsplit2.nullifiers[0] = duplicate_nullifier;
 
-        // make sure there are no other nullifiers
+        let transaction1 = transaction_v4_with_joinsplit_data(joinsplit_data1.0, [joinsplit1.0]);
-        joinsplit_data1.first = joinsplit1.0;
+        let transaction2 = transaction_v4_with_joinsplit_data(joinsplit_data2.0, [joinsplit2.0]);
-        joinsplit_data1.rest = Vec::new();
-
-        joinsplit_data2.first = joinsplit2.0;
-        joinsplit_data2.rest = Vec::new();
-
-        let transaction1 = transaction_v4_with_joinsplit_data(joinsplit_data1.0);
-        let transaction2 = transaction_v4_with_joinsplit_data(joinsplit_data2.0);
 
         block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
         block2.transactions[0] = transaction_v4_from_coinbase(&block2.transactions[0]).into();
@@ -302,6 +307,8 @@ proptest! {
             prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
             prop_assert!(commit_result.is_ok());
             prop_assert!(state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.disk.contains_sprout_nullifier(&expected_nullifiers[0]));
+            prop_assert!(state.disk.contains_sprout_nullifier(&expected_nullifiers[1]));
 
             block1_hash = block1.hash;
         } else {
@@ -312,6 +319,8 @@ proptest! {
             prop_assert_eq!(commit_result, Ok(()));
             prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
             prop_assert!(!state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.mem.best_contains_sprout_nullifier(&expected_nullifiers[0]));
+            prop_assert!(state.mem.best_contains_sprout_nullifier(&expected_nullifiers[1]));
 
             block1_hash = block1.hash;
             previous_mem = state.mem.clone();
@@ -335,6 +344,510 @@ proptest! {
     }
 }
 
+// sapling
+
+proptest! {
+    /// Make sure an arbitrary sapling nullifier is accepted by state contextual validation.
+    ///
+    /// This test makes sure there are no spurious rejections that might hide bugs in the other tests.
+    /// (And that the test infrastructure generally works.)
+    #[test]
+    fn accept_distinct_arbitrary_sapling_nullifiers(
+        spend in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        sapling_shielded_data in TypeNameToDebug::<sapling::ShieldedData::<PerSpendAnchor>>::arbitrary(),
+        use_finalized_state in any::<bool>(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        let expected_nullifier = spend.nullifier;
+
+        let transaction = transaction_v4_with_sapling_shielded_data(
+            sapling_shielded_data.0,
+            [spend.0]
+        );
+
+        // convert the coinbase transaction to a version that the non-finalized state will accept
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction.into());
+
+        let (mut state, _genesis) = new_state_with_mainnet_genesis();
+        let previous_mem = state.mem.clone();
+
+        // randomly choose to commit the block to the finalized or non-finalized state
+        if use_finalized_state {
+            let block1 = FinalizedBlock::from(Arc::new(block1));
+            let commit_result = state.disk.commit_finalized_direct(block1.clone(), "test");
+
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(commit_result.is_ok());
+            prop_assert!(state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.disk.contains_sapling_nullifier(&expected_nullifier));
+        } else {
+            let block1 = Arc::new(block1).prepare();
+            let commit_result =
+                state.validate_and_commit(block1.clone());
+
+            prop_assert_eq!(commit_result, Ok(()));
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(!state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.mem.best_contains_sapling_nullifier(&expected_nullifier));
+        }
+    }
+
+    /// Make sure duplicate sapling nullifiers are rejected by state contextual validation,
+    /// if they come from different Spends in the same sapling::ShieldedData/Transaction.
+    #[test]
+    fn reject_duplicate_sapling_nullifiers_in_transaction(
+        spend1 in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        mut spend2 in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        sapling_shielded_data in TypeNameToDebug::<sapling::ShieldedData::<PerSpendAnchor>>::arbitrary(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        // create a double-spend across two spends
+        let duplicate_nullifier = spend1.nullifier;
+        spend2.nullifier = duplicate_nullifier;
+
+        let transaction = transaction_v4_with_sapling_shielded_data(
+            sapling_shielded_data.0,
+            [spend1.0, spend2.0],
+        );
+
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction.into());
+
+        let (mut state, genesis) = new_state_with_mainnet_genesis();
+        let previous_mem = state.mem.clone();
+
+        let block1 = Arc::new(block1).prepare();
+        let commit_result =
+            state.validate_and_commit(block1);
+
+        prop_assert_eq!(
+            commit_result,
+            Err(
+                DuplicateSaplingNullifier {
+                    nullifier: duplicate_nullifier,
+                    in_finalized_state: false,
+                }.into()
+            )
+        );
+        prop_assert_eq!(Some((Height(0), genesis.hash)), state.best_tip());
+        prop_assert!(state.mem.eq_internal_state(&previous_mem));
+    }
+
+    /// Make sure duplicate sapling nullifiers are rejected by state contextual validation,
+    /// if they come from different transactions in the same block.
+    #[test]
+    fn reject_duplicate_sapling_nullifiers_in_block(
+        spend1 in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        mut spend2 in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        sapling_shielded_data1 in TypeNameToDebug::<sapling::ShieldedData::<PerSpendAnchor>>::arbitrary(),
+        sapling_shielded_data2 in TypeNameToDebug::<sapling::ShieldedData::<PerSpendAnchor>>::arbitrary(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        // create a double-spend across two transactions
+        let duplicate_nullifier = spend1.nullifier;
+        spend2.nullifier = duplicate_nullifier;
+
+        let transaction1 = transaction_v4_with_sapling_shielded_data(
+            sapling_shielded_data1.0,
+            [spend1.0]
+        );
+        let transaction2 = transaction_v4_with_sapling_shielded_data(
+            sapling_shielded_data2.0,
+            [spend2.0]
+        );
+
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction1.into());
+        block1
+            .transactions
+            .push(transaction2.into());
+
+        let (mut state, genesis) = new_state_with_mainnet_genesis();
+        let previous_mem = state.mem.clone();
+
+        let block1 = Arc::new(block1).prepare();
+        let commit_result =
+            state.validate_and_commit(block1);
+
+        prop_assert_eq!(
+            commit_result,
+            Err(
+                DuplicateSaplingNullifier {
+                    nullifier: duplicate_nullifier,
+                    in_finalized_state: false,
+                }.into()
+            )
+        );
+        prop_assert_eq!(Some((Height(0), genesis.hash)), state.best_tip());
+        prop_assert!(state.mem.eq_internal_state(&previous_mem));
+    }
+
+    /// Make sure duplicate sapling nullifiers are rejected by state contextual validation,
+    /// if they come from different blocks in the same chain.
+    #[test]
+    fn reject_duplicate_sapling_nullifiers_in_chain(
+        spend1 in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        mut spend2 in TypeNameToDebug::<sapling::Spend::<PerSpendAnchor>>::arbitrary(),
+        sapling_shielded_data1 in TypeNameToDebug::<sapling::ShieldedData::<PerSpendAnchor>>::arbitrary(),
+        sapling_shielded_data2 in TypeNameToDebug::<sapling::ShieldedData::<PerSpendAnchor>>::arbitrary(),
+        duplicate_in_finalized_state in any::<bool>(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+        let mut block2 = zebra_test::vectors::BLOCK_MAINNET_2_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        // create a double-spend across two blocks
+        let duplicate_nullifier = spend1.nullifier;
+        spend2.nullifier = duplicate_nullifier;
+
+        let transaction1 = transaction_v4_with_sapling_shielded_data(
+            sapling_shielded_data1.0,
+            [spend1.0]
+        );
+        let transaction2 = transaction_v4_with_sapling_shielded_data(
+            sapling_shielded_data2.0,
+            [spend2.0]
+        );
+
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+        block2.transactions[0] = transaction_v4_from_coinbase(&block2.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction1.into());
+        block2
+            .transactions
+            .push(transaction2.into());
+
+        let (mut state, _genesis) = new_state_with_mainnet_genesis();
+        let mut previous_mem = state.mem.clone();
+
+        let block1_hash;
+        // randomly choose to commit the next block to the finalized or non-finalized state
+        if duplicate_in_finalized_state {
+            let block1 = FinalizedBlock::from(Arc::new(block1));
+            let commit_result = state.disk.commit_finalized_direct(block1.clone(), "test");
+
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(commit_result.is_ok());
+            prop_assert!(state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.disk.contains_sapling_nullifier(&duplicate_nullifier));
+
+            block1_hash = block1.hash;
+        } else {
+            let block1 = Arc::new(block1).prepare();
+            let commit_result =
+                state.validate_and_commit(block1.clone());
+
+            prop_assert_eq!(commit_result, Ok(()));
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(!state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.mem.best_contains_sapling_nullifier(&duplicate_nullifier));
+
+            block1_hash = block1.hash;
+            previous_mem = state.mem.clone();
+        }
+
+        let block2 = Arc::new(block2).prepare();
+        let commit_result =
+            state.validate_and_commit(block2);
+
+        prop_assert_eq!(
+            commit_result,
+            Err(
+                DuplicateSaplingNullifier {
+                    nullifier: duplicate_nullifier,
+                    in_finalized_state: duplicate_in_finalized_state,
+                }.into()
+            )
+        );
+        prop_assert_eq!(Some((Height(1), block1_hash)), state.best_tip());
+        prop_assert!(state.mem.eq_internal_state(&previous_mem));
+    }
+}
+
+// orchard
+
+proptest! {
+    /// Make sure an arbitrary orchard nullifier is accepted by state contextual validation.
+    ///
+    /// This test makes sure there are no spurious rejections that might hide bugs in the other tests.
+    /// (And that the test infrastructure generally works.)
+    #[test]
+    fn accept_distinct_arbitrary_orchard_nullifiers(
+        authorized_action in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        orchard_shielded_data in TypeNameToDebug::<orchard::ShieldedData>::arbitrary(),
+        use_finalized_state in any::<bool>(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        let expected_nullifier = authorized_action.action.nullifier;
+
+        let transaction = transaction_v5_with_orchard_shielded_data(
+            orchard_shielded_data.0,
+            [authorized_action.0]
+        );
+
+        // convert the coinbase transaction to a version that the non-finalized state will accept
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction.into());
+
+        let (mut state, _genesis) = new_state_with_mainnet_genesis();
+        let previous_mem = state.mem.clone();
+
+        // randomly choose to commit the block to the finalized or non-finalized state
+        if use_finalized_state {
+            let block1 = FinalizedBlock::from(Arc::new(block1));
+            let commit_result = state.disk.commit_finalized_direct(block1.clone(), "test");
+
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(commit_result.is_ok());
+            prop_assert!(state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.disk.contains_orchard_nullifier(&expected_nullifier));
+        } else {
+            let block1 = Arc::new(block1).prepare();
+            let commit_result =
+                state.validate_and_commit(block1.clone());
+
+            prop_assert_eq!(commit_result, Ok(()));
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(!state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.mem.best_contains_orchard_nullifier(&expected_nullifier));
+        }
+    }
+
+    /// Make sure duplicate orchard nullifiers are rejected by state contextual validation,
+    /// if they come from different AuthorizedActions in the same orchard::ShieldedData/Transaction.
+    #[test]
+    fn reject_duplicate_orchard_nullifiers_in_transaction(
+        authorized_action1 in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        mut authorized_action2 in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        orchard_shielded_data in TypeNameToDebug::<orchard::ShieldedData>::arbitrary(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        // create a double-spend across two authorized_actions
+        let duplicate_nullifier = authorized_action1.action.nullifier;
+        authorized_action2.action.nullifier = duplicate_nullifier;
+
+        let transaction = transaction_v5_with_orchard_shielded_data(
+            orchard_shielded_data.0,
+            [authorized_action1.0, authorized_action2.0],
+        );
+
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction.into());
+
+        let (mut state, genesis) = new_state_with_mainnet_genesis();
+        let previous_mem = state.mem.clone();
+
+        let block1 = Arc::new(block1).prepare();
+        let commit_result =
+            state.validate_and_commit(block1);
+
+        prop_assert_eq!(
+            commit_result,
+            Err(
+                DuplicateOrchardNullifier {
+                    nullifier: duplicate_nullifier,
+                    in_finalized_state: false,
+                }.into()
+            )
+        );
+        prop_assert_eq!(Some((Height(0), genesis.hash)), state.best_tip());
+        prop_assert!(state.mem.eq_internal_state(&previous_mem));
+    }
+
+    /// Make sure duplicate orchard nullifiers are rejected by state contextual validation,
+    /// if they come from different transactions in the same block.
+    #[test]
+    fn reject_duplicate_orchard_nullifiers_in_block(
+        authorized_action1 in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        mut authorized_action2 in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        orchard_shielded_data1 in TypeNameToDebug::<orchard::ShieldedData>::arbitrary(),
+        orchard_shielded_data2 in TypeNameToDebug::<orchard::ShieldedData>::arbitrary(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        // create a double-spend across two transactions
+        let duplicate_nullifier = authorized_action1.action.nullifier;
+        authorized_action2.action.nullifier = duplicate_nullifier;
+
+        let transaction1 = transaction_v5_with_orchard_shielded_data(
+            orchard_shielded_data1.0,
+            [authorized_action1.0]
+        );
+        let transaction2 = transaction_v5_with_orchard_shielded_data(
+            orchard_shielded_data2.0,
+            [authorized_action2.0]
+        );
+
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction1.into());
+        block1
+            .transactions
+            .push(transaction2.into());
+
+        let (mut state, genesis) = new_state_with_mainnet_genesis();
+        let previous_mem = state.mem.clone();
+
+        let block1 = Arc::new(block1).prepare();
+        let commit_result =
+            state.validate_and_commit(block1);
+
+        prop_assert_eq!(
+            commit_result,
+            Err(
+                DuplicateOrchardNullifier {
+                    nullifier: duplicate_nullifier,
+                    in_finalized_state: false,
+                }.into()
+            )
+        );
+        prop_assert_eq!(Some((Height(0), genesis.hash)), state.best_tip());
+        prop_assert!(state.mem.eq_internal_state(&previous_mem));
+    }
+
+    /// Make sure duplicate orchard nullifiers are rejected by state contextual validation,
+    /// if they come from different blocks in the same chain.
+    #[test]
+    fn reject_duplicate_orchard_nullifiers_in_chain(
+        authorized_action1 in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        mut authorized_action2 in TypeNameToDebug::<orchard::AuthorizedAction>::arbitrary(),
+        orchard_shielded_data1 in TypeNameToDebug::<orchard::ShieldedData>::arbitrary(),
+        orchard_shielded_data2 in TypeNameToDebug::<orchard::ShieldedData>::arbitrary(),
+        duplicate_in_finalized_state in any::<bool>(),
+    ) {
+        zebra_test::init();
+
+        let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+        let mut block2 = zebra_test::vectors::BLOCK_MAINNET_2_BYTES
+            .zcash_deserialize_into::<Block>()
+            .expect("block should deserialize");
+
+        // create a double-spend across two blocks
+        let duplicate_nullifier = authorized_action1.action.nullifier;
+        authorized_action2.action.nullifier = duplicate_nullifier;
+
+        let transaction1 = transaction_v5_with_orchard_shielded_data(
+            orchard_shielded_data1.0,
+            [authorized_action1.0]
+        );
+        let transaction2 = transaction_v5_with_orchard_shielded_data(
+            orchard_shielded_data2.0,
+            [authorized_action2.0]
+        );
+
+        block1.transactions[0] = transaction_v4_from_coinbase(&block1.transactions[0]).into();
+        block2.transactions[0] = transaction_v4_from_coinbase(&block2.transactions[0]).into();
+
+        block1
+            .transactions
+            .push(transaction1.into());
+        block2
+            .transactions
+            .push(transaction2.into());
+
+        let (mut state, _genesis) = new_state_with_mainnet_genesis();
+        let mut previous_mem = state.mem.clone();
+
+        let block1_hash;
+        // randomly choose to commit the next block to the finalized or non-finalized state
+        if duplicate_in_finalized_state {
+            let block1 = FinalizedBlock::from(Arc::new(block1));
+            let commit_result = state.disk.commit_finalized_direct(block1.clone(), "test");
+
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(commit_result.is_ok());
+            prop_assert!(state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.disk.contains_orchard_nullifier(&duplicate_nullifier));
+
+            block1_hash = block1.hash;
+        } else {
+            let block1 = Arc::new(block1).prepare();
+            let commit_result =
+                state.validate_and_commit(block1.clone());
+
+            prop_assert_eq!(commit_result, Ok(()));
+            prop_assert_eq!(Some((Height(1), block1.hash)), state.best_tip());
+            prop_assert!(!state.mem.eq_internal_state(&previous_mem));
+            prop_assert!(state.mem.best_contains_orchard_nullifier(&duplicate_nullifier));
+
+            block1_hash = block1.hash;
+            previous_mem = state.mem.clone();
+        }
+
+        let block2 = Arc::new(block2).prepare();
+        let commit_result =
+            state.validate_and_commit(block2);
+
+        prop_assert_eq!(
+            commit_result,
+            Err(
+                DuplicateOrchardNullifier {
+                    nullifier: duplicate_nullifier,
+                    in_finalized_state: duplicate_in_finalized_state,
+                }.into()
+            )
+        );
+        prop_assert_eq!(Some((Height(1), block1_hash)), state.best_tip());
+        prop_assert!(state.mem.eq_internal_state(&previous_mem));
+    }
+}
+
 /// Return a new `StateService` containing the mainnet genesis block.
 /// Also returns the finalized genesis block itself.
 fn new_state_with_mainnet_genesis() -> (StateService, FinalizedBlock) {
@@ -358,15 +871,21 @@ fn new_state_with_mainnet_genesis() -> (StateService, FinalizedBlock) {
 }
 
 /// Make sure the supplied nullifiers are distinct, modifying them if necessary.
-fn make_distinct_nullifiers<'joinsplit>(
+fn make_distinct_nullifiers<'until_modified, NullifierT>(
-    nullifiers: impl IntoIterator<Item = &'joinsplit mut sprout::Nullifier>,
+    nullifiers: impl IntoIterator<Item = &'until_modified mut NullifierT>,
-) {
+) where
+    NullifierT: Into<[u8; 32]> + Clone + Eq + std::hash::Hash + 'until_modified,
+    [u8; 32]: Into<NullifierT>,
+{
     let nullifiers: Vec<_> = nullifiers.into_iter().collect();
 
     if nullifiers.iter().unique().count() < nullifiers.len() {
         let mut tweak: u8 = 0x00;
         for nullifier in nullifiers {
-            nullifier.0[0] = tweak;
+            let mut nullifier_bytes: [u8; 32] = nullifier.clone().into();
+            nullifier_bytes[0] = tweak;
+            *nullifier = nullifier_bytes.into();
+
             tweak = tweak
                 .checked_add(0x01)
                 .expect("unexpectedly large nullifier list");
@@ -374,16 +893,30 @@ fn make_distinct_nullifiers<'joinsplit>(
     }
 }
 
-/// Return a `Transaction::V4` containing `joinsplit_data`.
+/// Return a `Transaction::V4` containing `joinsplit_data`,
+/// with its `JoinSplit`s replaced by `joinsplits`.
 ///
 /// Other fields have empty or default values.
+///
+/// # Panics
+///
+/// If there are no `JoinSplit`s in `joinsplits`.
 fn transaction_v4_with_joinsplit_data(
     joinsplit_data: impl Into<Option<JoinSplitData<Groth16Proof>>>,
+    joinsplits: impl IntoIterator<Item = JoinSplit<Groth16Proof>>,
 ) -> Transaction {
     let mut joinsplit_data = joinsplit_data.into();
+    let joinsplits: Vec<_> = joinsplits.into_iter().collect();
+
+    if let Some(ref mut joinsplit_data) = joinsplit_data {
+        // make sure there are no other nullifiers, by replacing all the joinsplits
+        let (first, rest) = joinsplits
+            .split_first()
+            .expect("unexpected empty joinsplits");
+        joinsplit_data.first = first.clone();
+        joinsplit_data.rest = rest.to_vec();
 
         // set value balance to 0 to pass the chain value pool checks
-    if let Some(ref mut joinsplit_data) = joinsplit_data {
         let zero_amount = 0.try_into().expect("unexpected invalid zero amount");
 
         joinsplit_data.first.vpub_old = zero_amount;
@@ -405,6 +938,111 @@ fn transaction_v4_with_joinsplit_data(
     }
 }
 
+/// Return a `Transaction::V4` containing `sapling_shielded_data`.
+/// with its `Spend`s replaced by `spends`.
+///
+/// Other fields have empty or default values.
+///
+/// Note: since sapling nullifiers in V5 transactions are identical to V4 transactions,
+/// we just use V4 transactions in the tests.
+///
+/// # Panics
+///
+/// If there are no `Spend`s in `spends`, and no `Output`s in `sapling_shielded_data`.
+fn transaction_v4_with_sapling_shielded_data(
+    sapling_shielded_data: impl Into<Option<sapling::ShieldedData<PerSpendAnchor>>>,
+    spends: impl IntoIterator<Item = sapling::Spend<PerSpendAnchor>>,
+) -> Transaction {
+    let mut sapling_shielded_data = sapling_shielded_data.into();
+    let spends: Vec<_> = spends.into_iter().collect();
+
+    if let Some(ref mut sapling_shielded_data) = sapling_shielded_data {
+        // make sure there are no other nullifiers, by replacing all the spends
+        sapling_shielded_data.transfers = match (
+            sapling_shielded_data.transfers.clone(),
+            spends.try_into().ok(),
+        ) {
+            // old and new spends: replace spends
+            (
+                SpendsAndMaybeOutputs {
+                    shared_anchor,
+                    maybe_outputs,
+                    ..
+                },
+                Some(spends),
+            ) => SpendsAndMaybeOutputs {
+                shared_anchor,
+                spends,
+                maybe_outputs,
+            },
+            // old spends, but no new spends: delete spends, panic if no outputs
+            (SpendsAndMaybeOutputs { maybe_outputs, .. }, None) => JustOutputs {
+                outputs: maybe_outputs.try_into().expect(
+                    "unexpected invalid TransferData: must have at least one spend or one output",
+                ),
+            },
+            // no old spends, but new spends: add spends
+            (JustOutputs { outputs, .. }, Some(spends)) => SpendsAndMaybeOutputs {
+                shared_anchor: FieldNotPresent,
+                spends,
+                maybe_outputs: outputs.into(),
+            },
+            // no old and no new spends: do nothing
+            (just_outputs @ JustOutputs { .. }, None) => just_outputs,
+        };
+
+        // set value balance to 0 to pass the chain value pool checks
+        let zero_amount = 0.try_into().expect("unexpected invalid zero amount");
+        sapling_shielded_data.value_balance = zero_amount;
+    }
+
+    Transaction::V4 {
+        inputs: Vec::new(),
+        outputs: Vec::new(),
+        lock_time: LockTime::min_lock_time(),
+        expiry_height: Height(0),
+        joinsplit_data: None,
+        sapling_shielded_data,
+    }
+}
+
+/// Return a `Transaction::V5` containing `orchard_shielded_data`.
+/// with its `AuthorizedAction`s replaced by `authorized_actions`.
+///
+/// Other fields have empty or default values.
+///
+/// # Panics
+///
+/// If there are no `AuthorizedAction`s in `authorized_actions`.
+fn transaction_v5_with_orchard_shielded_data(
+    orchard_shielded_data: impl Into<Option<orchard::ShieldedData>>,
+    authorized_actions: impl IntoIterator<Item = orchard::AuthorizedAction>,
+) -> Transaction {
+    let mut orchard_shielded_data = orchard_shielded_data.into();
+    let authorized_actions: Vec<_> = authorized_actions.into_iter().collect();
+
+    if let Some(ref mut orchard_shielded_data) = orchard_shielded_data {
+        // make sure there are no other nullifiers, by replacing all the authorized_actions
+        orchard_shielded_data.actions = authorized_actions.try_into().expect(
+            "unexpected invalid orchard::ShieldedData: must have at least one AuthorizedAction",
+        );
+
+        // set value balance to 0 to pass the chain value pool checks
+        let zero_amount = 0.try_into().expect("unexpected invalid zero amount");
+        orchard_shielded_data.value_balance = zero_amount;
+    }
+
+    Transaction::V5 {
+        network_upgrade: Nu5,
+        inputs: Vec::new(),
+        outputs: Vec::new(),
+        lock_time: LockTime::min_lock_time(),
+        expiry_height: Height(0),
+        sapling_shielded_data: None,
+        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.

zebra-state/src/service/finalized_state.rs

@@ -9,8 +9,9 @@ use std::{collections::HashMap, convert::TryInto, path::Path, sync::Arc};
 
 use zebra_chain::{
     block::{self, Block},
+    orchard,
     parameters::{Network, GENESIS_PREVIOUS_BLOCK_HASH},
-    sprout,
+    sapling, sprout,
     transaction::{self, Transaction},
     transparent,
 };
@@ -375,6 +376,18 @@ impl FinalizedState {
         self.db.zs_contains(sprout_nullifiers, &sprout_nullifier)
     }
 
+    /// Returns `true` if the finalized state contains `sapling_nullifier`.
+    pub fn contains_sapling_nullifier(&self, sapling_nullifier: &sapling::Nullifier) -> bool {
+        let sapling_nullifiers = self.db.cf_handle("sapling_nullifiers").unwrap();
+        self.db.zs_contains(sapling_nullifiers, &sapling_nullifier)
+    }
+
+    /// Returns `true` if the finalized state contains `orchard_nullifier`.
+    pub fn contains_orchard_nullifier(&self, orchard_nullifier: &orchard::Nullifier) -> bool {
+        let orchard_nullifiers = self.db.cf_handle("orchard_nullifiers").unwrap();
+        self.db.zs_contains(orchard_nullifiers, &orchard_nullifier)
+    }
+
     /// Returns the finalized hash for a given `block::Height` if it is present.
     pub fn hash(&self, height: block::Height) -> Option<block::Hash> {
         let hash_by_height = self.db.cf_handle("hash_by_height").unwrap();

zebra-state/src/service/non_finalized_state.rs

@@ -14,7 +14,9 @@ use std::{collections::BTreeSet, mem, ops::Deref, sync::Arc};
 
 use zebra_chain::{
     block::{self, Block},
+    orchard,
     parameters::Network,
+    sapling, sprout,
     transaction::{self, Transaction},
     transparent,
 };
@@ -301,6 +303,30 @@ impl NonFinalizedState {
             .map(|(height, index)| best_chain.blocks[height].block.transactions[*index].clone())
     }
 
+    /// Returns `true` if the best chain contains `sprout_nullifier`.
+    #[allow(dead_code)]
+    pub fn best_contains_sprout_nullifier(&self, sprout_nullifier: &sprout::Nullifier) -> bool {
+        self.best_chain()
+            .map(|best_chain| best_chain.sprout_nullifiers.contains(sprout_nullifier))
+            .unwrap_or(false)
+    }
+
+    /// Returns `true` if the best chain contains `sapling_nullifier`.
+    #[allow(dead_code)]
+    pub fn best_contains_sapling_nullifier(&self, sapling_nullifier: &sapling::Nullifier) -> bool {
+        self.best_chain()
+            .map(|best_chain| best_chain.sapling_nullifiers.contains(sapling_nullifier))
+            .unwrap_or(false)
+    }
+
+    /// Returns `true` if the best chain contains `orchard_nullifier`.
+    #[allow(dead_code)]
+    pub fn best_contains_orchard_nullifier(&self, orchard_nullifier: &orchard::Nullifier) -> bool {
+        self.best_chain()
+            .map(|best_chain| best_chain.orchard_nullifiers.contains(orchard_nullifier))
+            .unwrap_or(false)
+    }
+
     /// Return the non-finalized portion of the current best chain
     fn best_chain(&self) -> Option<&Chain> {
         self.chain_set

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

@@ -457,60 +457,68 @@ impl<AnchorV> UpdateWith<Option<sapling::ShieldedData<AnchorV>>> for Chain
 where
     AnchorV: sapling::AnchorVariant + Clone,
 {
+    #[instrument(skip(self, sapling_shielded_data))]
     fn update_chain_state_with(
         &mut self,
         sapling_shielded_data: &Option<sapling::ShieldedData<AnchorV>>,
     ) -> Result<(), ValidateContextError> {
         if let Some(sapling_shielded_data) = sapling_shielded_data {
-            for nullifier in sapling_shielded_data.nullifiers() {
+            check::nullifier::add_to_non_finalized_chain_unique(
-                // TODO: check sapling nullifiers are unique (#2231)
+                &mut self.sapling_nullifiers,
-                self.sapling_nullifiers.insert(*nullifier);
+                sapling_shielded_data.nullifiers(),
-            }
+            )?;
         }
         Ok(())
     }
 
+    /// # Panics
+    ///
+    /// Panics if any nullifier is missing from the chain when we try to remove it.
+    ///
+    /// See [`check::nullifier::remove_from_non_finalized_chain`] for details.
+    #[instrument(skip(self, sapling_shielded_data))]
     fn revert_chain_state_with(
         &mut self,
         sapling_shielded_data: &Option<sapling::ShieldedData<AnchorV>>,
     ) {
         if let Some(sapling_shielded_data) = sapling_shielded_data {
-            for nullifier in sapling_shielded_data.nullifiers() {
+            check::nullifier::remove_from_non_finalized_chain(
-                // TODO: refactor using generic assert function (#2231)
+                &mut self.sapling_nullifiers,
-                assert!(
+                sapling_shielded_data.nullifiers(),
-                    self.sapling_nullifiers.remove(nullifier),
-                    "nullifier must be present if block was added to chain"
             );
         }
     }
-    }
 }
 
 impl UpdateWith<Option<orchard::ShieldedData>> for Chain {
+    #[instrument(skip(self, orchard_shielded_data))]
     fn update_chain_state_with(
         &mut self,
         orchard_shielded_data: &Option<orchard::ShieldedData>,
     ) -> Result<(), ValidateContextError> {
         if let Some(orchard_shielded_data) = orchard_shielded_data {
-            // TODO: check orchard nullifiers are unique (#2231)
+            check::nullifier::add_to_non_finalized_chain_unique(
-            for nullifier in orchard_shielded_data.nullifiers() {
+                &mut self.orchard_nullifiers,
-                self.orchard_nullifiers.insert(*nullifier);
+                orchard_shielded_data.nullifiers(),
-            }
+            )?;
         }
         Ok(())
     }
 
+    /// # Panics
+    ///
+    /// Panics if any nullifier is missing from the chain when we try to remove it.
+    ///
+    /// See [`check::nullifier::remove_from_non_finalized_chain`] for details.
+    #[instrument(skip(self, orchard_shielded_data))]
     fn revert_chain_state_with(&mut self, orchard_shielded_data: &Option<orchard::ShieldedData>) {
         if let Some(orchard_shielded_data) = orchard_shielded_data {
-            for nullifier in orchard_shielded_data.nullifiers() {
+            check::nullifier::remove_from_non_finalized_chain(
-                // TODO: refactor using generic assert function (#2231)
+                &mut self.orchard_nullifiers,
-                assert!(
+                orchard_shielded_data.nullifiers(),
-                    self.orchard_nullifiers.remove(nullifier),
-                    "nullifier must be present if block was added to chain"
             );
         }
     }
-    }
 }
 
 impl Ord for Chain {