fix: Avoid wrong test results for modified JoinSplits (#6604)

* Run the verifier in a loop

* Remove the `modify_joinsplit` fn

The fn is called only once from a single test, and is short, so I moved
the code from the fn directly to the test.

* Refactor some error messages.

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

zebra-consensus/src/transaction/tests.rs

@@ -2016,30 +2016,52 @@ async fn v4_with_joinsplit_is_rejected_for_modification(
         .rev()
         .filter(|(_, transaction)| !transaction.is_coinbase() && transaction.inputs().is_empty())
         .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some())
-        .expect("No transaction found with Groth16 JoinSplits");
+        .expect("There should be a tx with Groth16 JoinSplits.");
 
-    modify_joinsplit(
+    let expected_error = Err(expected_error);
-        Arc::get_mut(&mut transaction).expect("Transaction only has one active reference"),
+
-        modification,
+    // Modify a JoinSplit in the transaction following the given modification type.
-    );
+    let tx = Arc::get_mut(&mut transaction).expect("The tx should have only one active reference.");
+    match tx {
+        Transaction::V4 {
+            joinsplit_data: Some(ref mut joinsplit_data),
+            ..
+        } => modify_joinsplit_data(joinsplit_data, modification),
+        _ => unreachable!("Transaction should have some JoinSplit shielded data."),
+    }
 
     // Initialize the verifier
     let state_service =
-        service_fn(|_| async { unreachable!("State service should not be called") });
+        service_fn(|_| async { unreachable!("State service should not be called.") });
     let verifier = Verifier::new(network, state_service);
 
-    // Test the transaction verifier
+    // Test the transaction verifier.
-    let result = verifier
+    //
-        .clone()
+    // Note that modifying the JoinSplit data invalidates the tx signatures. The signatures are
-        .oneshot(Request::Block {
+    // checked concurrently with the ZK proofs, and when a signature check finishes before the proof
-            transaction,
+    // check, the verifier reports an invalid signature instead of invalid proof. This race
-            known_utxos: Arc::new(HashMap::new()),
+    // condition happens only occasionaly, so we run the verifier in a loop with a small iteration
-            height,
+    // threshold until it returns the correct error.
-            time: DateTime::<Utc>::MAX_UTC,
+    let mut i = 1;
-        })
+    let result = loop {
-        .await;
+        let result = verifier
+            .clone()
+            .oneshot(Request::Block {
+                transaction: transaction.clone(),
+                known_utxos: Arc::new(HashMap::new()),
+                height,
+                time: DateTime::<Utc>::MAX_UTC,
+            })
+            .await;
 
-    assert_eq!(result, Err(expected_error));
+        if result == expected_error || i >= 10 {
+            break result;
+        }
+
+        i += 1;
+    };
+
+    assert_eq!(result, expected_error);
 }
 
 /// Test if a V4 transaction with Sapling spends is accepted by the verifier.
@@ -2496,17 +2518,6 @@ enum JoinSplitModification {
     ZeroProof,
 }
 
-/// Modify a JoinSplit in the transaction following the given modification type.
-fn modify_joinsplit(transaction: &mut Transaction, modification: JoinSplitModification) {
-    match transaction {
-        Transaction::V4 {
-            joinsplit_data: Some(ref mut joinsplit_data),
-            ..
-        } => modify_joinsplit_data(joinsplit_data, modification),
-        _ => unreachable!("Transaction has no JoinSplit shielded data"),
-    }
-}
-
 /// Modify a [`JoinSplitData`] following the given modification type.
 fn modify_joinsplit_data(
     joinsplit_data: &mut JoinSplitData<Groth16Proof>,