perf: avoid holding write-lock across prove()

Previously a write-lock was held across `create_transaction()` which
includes a lengthy (potentially minutes) call to `prove`.  This would be
unacceptably bad for concurrency.

This commit makes create_transaction() take &self instead of &mut self.

Only a single mutation was being performed inside add_change() and this
is now moved into `send()` RPC after `create_transaction()` completes.
The write-lock acquisition is like-wise moved, so it should only be
held very briefly while adding a record to wallet's `expected_utxos`.

src/mine_loop.rs

@@ -508,7 +508,7 @@ mod mine_loop_tests {
             coins: four_neptune_coins,
             lock_script_hash: LockScript::anyone_can_spend().hash(),
         };
-        let tx_by_preminer = premine_receiver_global_state
+        let (tx_by_preminer, _) = premine_receiver_global_state
             .create_transaction(
                 vec![
                     (UtxoReceiverData {

src/models/blockchain/block/mod.rs

@@ -623,7 +623,7 @@ mod block_tests {
             sender_randomness: random(),
             utxo: new_utxo,
         };
-        let new_tx = global_state_lock
+        let (new_tx, _) = global_state_lock
             .lock_guard_mut()
             .await
             .create_transaction(

src/models/state/archival_state.rs

@@ -1004,7 +1004,7 @@ mod archival_state_tests {
                 own_receiving_address,
                 rng.gen(),
             );
-            let sender_tx = genesis_receiver_global_state
+            let (sender_tx, _) = genesis_receiver_global_state
                 .create_transaction(
                     vec![UtxoReceiverData {
                         public_announcement: PublicAnnouncement::default(),
@@ -1150,7 +1150,7 @@ mod archival_state_tests {
                 public_announcement: PublicAnnouncement::default(),
             },
         ];
-        let sender_tx = global_state_lock
+        let (sender_tx, _) = global_state_lock
             .lock_guard_mut()
             .await
             .create_transaction(receiver_data, NeptuneCoins::new(4), now + seven_months)
@@ -1284,7 +1284,7 @@ mod archival_state_tests {
                     public_announcement: PublicAnnouncement::default(),
                 },
             ];
-            let sender_tx = global_state
+            let (sender_tx, _) = global_state
                 .create_transaction(receiver_data, NeptuneCoins::new(4), now + seven_months)
                 .await
                 .unwrap();
@@ -1436,7 +1436,7 @@ mod archival_state_tests {
                 lock_script_hash: LockScript::anyone_can_spend().hash(),
             },
         };
-        let sender_tx = global_state_lock
+        let (sender_tx, _) = global_state_lock
             .lock_guard_mut()
             .await
             .create_transaction(vec![receiver_data], one_money, now + seven_months)
@@ -1697,7 +1697,7 @@ mod archival_state_tests {
                 public_announcement: PublicAnnouncement::default(),
             },
         ];
-        let tx_from_alice = alice_state_lock
+        let (tx_from_alice, _) = alice_state_lock
             .lock_guard_mut()
             .await
             .create_transaction(

src/models/state/mempool.rs

@@ -631,7 +631,7 @@ mod tests {
         }
         let mut now = genesis_block.kernel.header.timestamp;
         let seven_months = Timestamp::months(7);
-        let tx_by_preminer = premine_receiver_global_state
+        let (tx_by_preminer, _) = premine_receiver_global_state
             .create_transaction(
                 output_utxos_generated_by_me,
                 NeptuneCoins::new(1),
@@ -657,7 +657,7 @@ mod tests {
             receiver_privacy_digest: other_receiver_address.privacy_digest,
             public_announcement: PublicAnnouncement::default(),
         }];
-        let tx_by_other_original = other_global_state
+        let (tx_by_other_original, _) = other_global_state
             .create_transaction(
                 output_utxo_data_by_miner,
                 NeptuneCoins::new(1),
@@ -802,7 +802,7 @@ mod tests {
             sender_randomness: random(),
             public_announcement: PublicAnnouncement::default(),
         };
-        let tx_by_preminer_low_fee = preminer_state
+        let (tx_by_preminer_low_fee, _) = preminer_state
             .create_transaction(
                 vec![receiver_data.clone()],
                 NeptuneCoins::new(1),
@@ -824,7 +824,7 @@ mod tests {
 
         // Insert a transaction that spends the same UTXO and has a higher fee.
         // Verify that this replaces the previous transaction.
-        let tx_by_preminer_high_fee = preminer_state
+        let (tx_by_preminer_high_fee, _) = preminer_state
             .create_transaction(
                 vec![receiver_data.clone()],
                 NeptuneCoins::new(10),
@@ -843,7 +843,7 @@ mod tests {
 
         // Insert a conflicting transaction with a lower fee and verify that it
         // does *not* replace the existing transaction.
-        let tx_by_preminer_medium_fee = preminer_state
+        let (tx_by_preminer_medium_fee, _) = preminer_state
             .create_transaction(
                 vec![receiver_data],
                 NeptuneCoins::new(4),

src/models/state/mod.rs

@@ -247,6 +247,14 @@ pub struct UtxoReceiverData {
     pub public_announcement: PublicAnnouncement,
 }
 
+#[derive(Debug, Clone)]
+pub struct ChangeUtxoData {
+    pub change_utxo: Utxo,
+    pub change_addition_record: AdditionRecord,
+    pub change_sender_randomness: Digest,
+    pub receiver_preimage: Digest,
+}
+
 impl GlobalState {
     pub fn new(
         wallet_state: WalletState,
@@ -394,7 +402,7 @@ impl GlobalState {
     /// (*i.e.*, synced and never or no longer timelocked) and that sum to
     /// enough funds.
     pub async fn assemble_inputs_for_transaction(
-        &mut self,
+        &self,
         total_spend: NeptuneCoins,
         timestamp: Timestamp,
     ) -> Result<Vec<(Utxo, LockScript, MsMembershipProof)>> {
@@ -440,9 +448,10 @@ impl GlobalState {
     }
 
     /// Generate a change UTXO and transaction output to ensure that the difference
-    /// in input amount and output amount goes back to us. Also, make sure to expect
-    /// the UTXO so that we can synchronize it after it is confirmed.
-    pub async fn add_change(&mut self, change_amount: NeptuneCoins) -> (AdditionRecord, Utxo) {
+    /// in input amount and output amount goes back to us.
+    /// The caller should also notify the wallet to expect the change Utxo
+    /// so that we can synchronize it after it is confirmed.
+    pub async fn add_change(&self, change_amount: NeptuneCoins) -> ChangeUtxoData {
         // generate utxo
         let own_spending_key_for_change = self
             .wallet_state
@@ -468,20 +477,14 @@ impl GlobalState {
             receiver_digest,
         );
 
-        // Add change UTXO to pool of expected incoming UTXOs
         let receiver_preimage = own_spending_key_for_change.privacy_preimage;
-        let _change_addition_record = self
-            .wallet_state
-            .expected_utxos
-            .add_expected_utxo(
-                change_utxo.clone(),
-                change_sender_randomness,
-                receiver_preimage,
-                UtxoNotifier::Myself,
-            )
-            .expect("Adding change UTXO to UTXO notification pool must succeed");
 
-        (change_addition_record, change_utxo)
+        ChangeUtxoData {
+            change_utxo,
+            change_sender_randomness,
+            receiver_preimage,
+            change_addition_record,
+        }
     }
 
     /// Generate a primitive witness for a transaction from various disparate witness data.
@@ -533,13 +536,13 @@ impl GlobalState {
     /// Returns the transaction and a vector containing the sender
     /// randomness for each output UTXO.
     pub async fn create_transaction(
-        &mut self,
+        &self,
         receiver_data: Vec<UtxoReceiverData>,
         fee: NeptuneCoins,
         timestamp: Timestamp,
-    ) -> Result<Transaction> {
+    ) -> Result<(Transaction, Option<ChangeUtxoData>)> {
         // UTXO data: inputs, outputs, and supporting witness data
-        let (inputs, spendable_utxos_and_mps, outputs, output_utxos) = self
+        let (inputs, spendable_utxos_and_mps, outputs, output_utxos, change_utxo_data) = self
             .generate_utxo_data_for_transaction(&receiver_data, fee, timestamp)
             .await?;
 
@@ -564,17 +567,20 @@ impl GlobalState {
             .nth_generation_spending_key(0);
 
         // assemble transaction object
-        Ok(Self::create_transaction_from_data(
-            spending_key,
-            inputs,
-            spendable_utxos_and_mps,
-            outputs,
-            output_utxos,
-            fee,
-            public_announcements,
-            timestamp,
-            mutator_set_accumulator,
-            privacy,
+        Ok((
+            Self::create_transaction_from_data(
+                spending_key,
+                inputs,
+                spendable_utxos_and_mps,
+                outputs,
+                output_utxos,
+                fee,
+                public_announcements,
+                timestamp,
+                mutator_set_accumulator,
+                privacy,
+            ),
+            change_utxo_data,
         ))
     }
 
@@ -583,7 +589,7 @@ impl GlobalState {
     /// and produce a list of removal records, input UTXOs (with lock scripts and
     /// membership proofs), addition records, and output UTXOs.
     async fn generate_utxo_data_for_transaction(
-        &mut self,
+        &self,
         receiver_data: &[UtxoReceiverData],
         fee: NeptuneCoins,
         timestamp: Timestamp,
@@ -592,6 +598,7 @@ impl GlobalState {
         Vec<(Utxo, LockScript, MsMembershipProof)>,
         Vec<AdditionRecord>,
         Vec<Utxo>,
+        Option<ChangeUtxoData>,
     )> {
         // total amount to be spent -- determines how many and which UTXOs to use
         let total_spend: NeptuneCoins = receiver_data
@@ -625,14 +632,22 @@ impl GlobalState {
         let mut output_utxos = receiver_data.iter().map(|rd| rd.utxo.clone()).collect_vec();
 
         // keep track of change (if any)
+        let mut maybe_change_utxo_data = None;
         if total_spend < input_amount {
             let change_amount = input_amount.checked_sub(&total_spend).unwrap();
-            let (change_addition_record, change_utxo) = self.add_change(change_amount).await;
-            outputs.push(change_addition_record);
-            output_utxos.push(change_utxo.clone());
+            let change_utxo_data = self.add_change(change_amount).await;
+            outputs.push(change_utxo_data.change_addition_record);
+            output_utxos.push(change_utxo_data.change_utxo.clone());
+            maybe_change_utxo_data = Some(change_utxo_data);
         }
 
-        Ok((inputs, spendable_utxos_and_mps, outputs, output_utxos))
+        Ok((
+            inputs,
+            spendable_utxos_and_mps,
+            outputs,
+            output_utxos,
+            maybe_change_utxo_data,
+        ))
     }
 
     /// Assembles a transaction kernel and supporting witness or proof(s) from
@@ -1278,7 +1293,7 @@ mod global_state_tests {
         timestamp: Timestamp,
     ) -> Result<Transaction> {
         // UTXO data: inputs, outputs, and supporting witness data
-        let (inputs, spendable_utxos_and_mps, outputs, output_utxos) = global_state_lock
+        let (inputs, spendable_utxos_and_mps, outputs, output_utxos, _) = global_state_lock
             .lock_guard_mut()
             .await
             .generate_utxo_data_for_transaction(receiver_data, fee, timestamp)
@@ -2205,7 +2220,7 @@ mod global_state_tests {
             },
         ];
         let now = genesis_block.kernel.header.timestamp;
-        let tx_from_alice = alice_state_lock
+        let (tx_from_alice, _) = alice_state_lock
             .lock_guard_mut()
             .await
             .create_transaction(receiver_data_from_alice.clone(), NeptuneCoins::new(1), now)
@@ -2240,7 +2255,7 @@ mod global_state_tests {
                 public_announcement: PublicAnnouncement::default(),
             },
         ];
-        let tx_from_bob = bob_state_lock
+        let (tx_from_bob, _) = bob_state_lock
             .lock_guard_mut()
             .await
             .create_transaction(receiver_data_from_bob.clone(), NeptuneCoins::new(2), now)

src/models/state/wallet/mod.rs

@@ -868,7 +868,7 @@ mod wallet_tests {
         };
         let receiver_data_to_other = vec![receiver_data_12_to_other, receiver_data_one_to_other];
         let mut now = genesis_block.kernel.header.timestamp;
-        let valid_tx = premine_receiver_global_state
+        let (valid_tx, _) = premine_receiver_global_state
             .create_transaction(
                 receiver_data_to_other.clone(),
                 NeptuneCoins::new(2),
@@ -1138,7 +1138,7 @@ mod wallet_tests {
             },
             sender_randomness: random(),
         };
-        let tx_from_preminer = premine_receiver_global_state
+        let (tx_from_preminer, _) = premine_receiver_global_state
             .create_transaction(vec![receiver_data_six.clone()], NeptuneCoins::new(4), now)
             .await
             .unwrap();

src/rpc_server.rs

@@ -1,6 +1,7 @@
 use crate::models::blockchain::type_scripts::neptune_coins::NeptuneCoins;
 use crate::models::consensus::timestamp::Timestamp;
 use crate::models::state::wallet::coin_with_possible_timelock::CoinWithPossibleTimeLock;
+use crate::models::state::wallet::utxo_notification_pool::UtxoNotifier;
 use crate::prelude::twenty_first;
 
 use anyhow::Result;
@@ -496,7 +497,7 @@ impl RPC for NeptuneRPCServer {
 
         // note: for future changes:
         // No consensus data should be read within this read-lock.
-        // Else a write lock must be used instead and held until
+        // Else the lock must be held until
         // create_transaction() completes, so entire op is atomic.
         // See: https://github.com/Neptune-Crypto/neptune-core/issues/134
         let state = self.state.lock_guard().await;
@@ -540,23 +541,57 @@ impl RPC for NeptuneRPCServer {
         }
 
         // All cryptographic data must be in relation to a single block
-        // and a write-lock must therefore be held over GlobalState to ensure this.
+        // and a read-lock must therefore be held over GlobalState to ensure this.
+        // Note that create_transaction() does not modify any state.
         let transaction_result = self
             .state
-            .lock_guard_mut()
+            .lock_guard()
             .await
             .create_transaction(receiver_data, fee, now)
             .await;
 
-        let transaction = match transaction_result {
-            Ok(tx) => tx,
+        let (transaction, maybe_change_utxo_data) = match transaction_result {
+            Ok((tx, data)) => (tx, data),
             Err(err) => {
                 tracing::error!("Could not create transaction: {}", err);
                 return None;
             }
         };
 
-        // 2. Send transaction message to main
+        // 2. Inform wallet.
+        //
+        //    If we have a change Utxo, we add it to pool
+        //    of expected incoming UTXOs so that we can
+        //    synchronize it after it is confirmed.
+        //
+        //    Here we modify state, so we briefly acquire write-lock.
+        //    TBD:
+        //      a) is this really necessary?  why can't wallet be
+        //         informed of change UTXO when block is received like
+        //         any other incoming utxo?  If not necessary, we can
+        //         avoid any mutation/write during send().
+        //      b) if it's necessary to notify wallet of incoming change
+        //         which adds to balance, should we not also notify
+        //         of all utxos that could affect wallet's balance?
+        //         In particular, it seems like the wallet balance
+        //         should decrease by the amount spent.
+        if let Some(change_utxo_data) = maybe_change_utxo_data {
+            let _change_addition_record = self
+                .state
+                .lock_guard_mut()
+                .await
+                .wallet_state
+                .expected_utxos
+                .add_expected_utxo(
+                    change_utxo_data.change_utxo,
+                    change_utxo_data.change_sender_randomness,
+                    change_utxo_data.receiver_preimage,
+                    UtxoNotifier::Myself,
+                )
+                .expect("Adding change UTXO to UTXO notification pool must succeed");
+        }
+
+        // 3. Send transaction message to main
         let response: Result<(), SendError<RPCServerToMain>> = self
             .rpc_server_to_main_tx
             .send(RPCServerToMain::Send(Box::new(transaction.clone())))