Merge pull request #26 from MyHush/danger

lock while sync, add memos in change zaddr

lib/src/lightclient.rs

@@ -592,20 +592,27 @@ impl LightClient {
             }
         }        
 
-        let mut file_buffer = BufWriter::with_capacity(
-            1_000_000, // 1 MB write buffer
-            File::create(self.config.get_wallet_path()).unwrap());
-        
-            let r = match self.wallet.write().unwrap().write(&mut file_buffer) {
-            Ok(_) => Ok(()),
-            Err(e) => {
-                let err = format!("ERR: {}", e);
-                error!("{}", err);
-                Err(e.to_string())
-            }
-        };
+        let r;
+        {
+            // Prevent any overlapping syncs during save, and don't save in the middle of a sync
+            let _lock = self.sync_lock.lock().unwrap();
+
+            let wallet = self.wallet.write().unwrap();
+            let mut file_buffer = BufWriter::with_capacity(
+                1_000_000, // 1 MB write buffer
+                File::create(self.config.get_wallet_path()).unwrap());
 
-        file_buffer.flush().map_err(|e| format!("{}", e))?;
+                r = match wallet.write(&mut file_buffer) {
+                    Ok(_) => Ok(()),
+                    Err(e) => {
+                        let err = format!("ERR: {}", e);
+                        error!("{}", err);
+                        Err(e.to_string())
+                    }
+                };
+
+                file_buffer.flush().map_err(|e| format!("{}", e))?;
+            }
 
         r
     }

lib/src/lightwallet.rs

@@ -1128,8 +1128,8 @@ impl LightWallet {
                     None => continue,
                 };
 
-                {
-                    info!("A sapling note was sent in {}, getting memo", tx.txid());
+                if memo.to_utf8().is_some() {
+                    info!("A sapling note was sent to wallet in {} that had a memo", tx.txid());
 
                     // Do it in a short scope because of the write lock.   
                     let mut txs = self.txs.write().unwrap();
@@ -1138,7 +1138,10 @@ impl LightWallet {
                        .and_then(|t| {
                            t.notes.iter_mut().find(|nd| nd.note == note)
                        }) {
-                           None => (),
+                        None => {
+                            info!("No txid matched for incoming sapling funds while updating memo"); 
+                            ()
+                        },
                            Some(nd) => {
                                nd.memo = Some(memo)
                            }
@@ -1172,8 +1175,13 @@ impl LightWallet {
                             let address = encode_payment_address(self.config.hrp_sapling_address(), 
                                             &payment_address);
 
-                            // Check if this is a change address
-                            if z_addresses.contains(&address) {
+                             // Check if this is change, and if it also doesn't have a memo, don't add 
+                            // to the outgoing metadata. 
+                            // If this is change (i.e., funds sent to ourself) AND has a memo, then
+                            // presumably the users is writing a memo to themself, so we will add it to 
+                            // the outgoing metadata, even though it might be confusing in the UI, but hopefully
+                            // the user can make sense of it. 
+                            if z_addresses.contains(&address) && memo.to_utf8().is_none() {
                                 continue;
                             }
 

lib/src/lightwallet/tests.rs

@@ -791,6 +791,177 @@ fn test_z_spend_to_z() {
     }
 }
 
+#[test]
+fn test_self_txns_ttoz_withmemo() {
+    let mut rng = OsRng;
+    let secp = Secp256k1::new();
+
+    let (wallet, _txid1, block_hash) = get_test_wallet(0);
+
+    let pk = PublicKey::from_secret_key(&secp, &wallet.tkeys.read().unwrap()[0]);
+    let taddr = wallet.address_from_sk(&wallet.tkeys.read().unwrap()[0]);
+
+    const TAMOUNT1: u64 = 50000;
+
+    let mut tx = FakeTransaction::new(&mut rng);
+    tx.add_t_output(&pk, TAMOUNT1);
+    let txid1 = tx.get_tx().txid();
+
+    wallet.scan_full_tx(&tx.get_tx(), 1, 0); 
+
+    {
+        let txs = wallet.txs.read().unwrap();
+
+        // Now make sure the t addr was recieved
+        assert_eq!(txs[&txid1].utxos.len(), 1);
+        assert_eq!(txs[&txid1].utxos[0].address, taddr);
+        assert_eq!(txs[&txid1].utxos[0].value, TAMOUNT1);
+    }
+
+    // Create a new Tx, spending this taddr
+    const AMOUNT_SENT: u64 = 20;
+
+    let outgoing_memo = "Outgoing Memo".to_string();
+    let zaddr = wallet.add_zaddr();
+
+    let branch_id = u32::from_str_radix("2bb40e60", 16).unwrap();
+    let (ss, so) =get_sapling_params().unwrap();
+
+    // Create a tx and send to address. This should consume both the UTXO and the note
+    let raw_tx = wallet.send_to_address(branch_id, &ss, &so,
+                            vec![(&zaddr, AMOUNT_SENT, Some(outgoing_memo.clone()))]).unwrap();
+
+    let sent_tx = Transaction::read(&raw_tx[..]).unwrap();
+    let sent_txid = sent_tx.txid();
+
+    let mut cb3 = FakeCompactBlock::new(2, block_hash);
+    cb3.add_tx(&sent_tx);
+
+    // Scan the compact block and the full Tx
+    wallet.scan_block(&cb3.as_bytes()).unwrap();
+    wallet.scan_full_tx(&sent_tx, 2, 0);
+
+    {
+        let txs = wallet.txs.read().unwrap();
+
+        // Includes Outgoing meta data, since this is a wallet -> wallet tx with a memo
+        assert_eq!(txs[&sent_txid].outgoing_metadata.len(), 1);
+        assert_eq!(txs[&sent_txid].outgoing_metadata[0].memo.to_utf8().unwrap().unwrap(), outgoing_memo);
+    }
+}
+
+#[test]
+fn test_self_txns_ttoz_nomemo() {
+    let mut rng = OsRng;
+    let secp = Secp256k1::new();
+
+    let (wallet, _txid1, block_hash) = get_test_wallet(0);
+
+    let pk = PublicKey::from_secret_key(&secp, &wallet.tkeys.read().unwrap()[0]);
+    let taddr = wallet.address_from_sk(&wallet.tkeys.read().unwrap()[0]);
+
+    const TAMOUNT1: u64 = 50000;
+
+    let mut tx = FakeTransaction::new(&mut rng);
+    tx.add_t_output(&pk, TAMOUNT1);
+    let txid1 = tx.get_tx().txid();
+
+    wallet.scan_full_tx(&tx.get_tx(), 1, 0); 
+
+    {
+        let txs = wallet.txs.read().unwrap();
+
+        // Now make sure the t addr was recieved
+        assert_eq!(txs[&txid1].utxos.len(), 1);
+        assert_eq!(txs[&txid1].utxos[0].address, taddr);
+        assert_eq!(txs[&txid1].utxos[0].value, TAMOUNT1);
+    }
+
+    // Create a new Tx, spending this taddr
+    const AMOUNT_SENT: u64 = 20;
+
+    let zaddr = wallet.add_zaddr();
+
+    let branch_id = u32::from_str_radix("2bb40e60", 16).unwrap();
+    let (ss, so) =get_sapling_params().unwrap();
+
+    // Create a tx and send to address. This should consume both the UTXO and the note
+    let raw_tx = wallet.send_to_address(branch_id, &ss, &so,
+                            vec![(&zaddr, AMOUNT_SENT, None)]).unwrap();
+
+    let sent_tx = Transaction::read(&raw_tx[..]).unwrap();
+    let sent_txid = sent_tx.txid();
+
+    let mut cb3 = FakeCompactBlock::new(2, block_hash);
+    cb3.add_tx(&sent_tx);
+
+    // Scan the compact block and the full Tx
+    wallet.scan_block(&cb3.as_bytes()).unwrap();
+    wallet.scan_full_tx(&sent_tx, 2, 0);
+
+    {
+        let txs = wallet.txs.read().unwrap();
+
+        // No Outgoing meta data, since this is a wallet -> wallet tx without a memo
+        assert_eq!(txs[&sent_txid].outgoing_metadata.len(), 0);
+    }
+}
+
+#[test]
+fn test_self_txns_ztoz() {
+    const AMOUNT1: u64 = 50000;
+    let (wallet, _txid1, block_hash) = get_test_wallet(AMOUNT1);
+
+    let zaddr2 = wallet.add_zaddr();   // This is acually address #6, since there are 5 initial addresses in the wallet
+
+    const AMOUNT_SENT: u64 = 20;
+
+    let outgoing_memo = "Outgoing Memo".to_string();
+
+    let branch_id = u32::from_str_radix("2bb40e60", 16).unwrap();
+    let (ss, so) =get_sapling_params().unwrap();
+
+    // Create a tx and send to address
+    let raw_tx = wallet.send_to_address(branch_id, &ss, &so,
+                            vec![(&zaddr2, AMOUNT_SENT, Some(outgoing_memo.clone()))]).unwrap();
+
+    let sent_tx = Transaction::read(&raw_tx[..]).unwrap();
+    let sent_txid = sent_tx.txid();
+
+    let mut cb3 = FakeCompactBlock::new(2, block_hash);
+    cb3.add_tx(&sent_tx);
+    wallet.scan_block(&cb3.as_bytes()).unwrap();
+    wallet.scan_full_tx(&sent_tx, 2, 0);
+
+    {
+        let txs = wallet.txs.read().unwrap();
+
+        // Includes Outgoing meta data, since this is a wallet -> wallet tx with a memo
+        assert_eq!(txs[&sent_txid].outgoing_metadata.len(), 1);
+        assert_eq!(txs[&sent_txid].outgoing_metadata[0].memo.to_utf8().unwrap().unwrap(), outgoing_memo);
+    }
+
+    // Another self tx, this time without a memo
+    let raw_tx = wallet.send_to_address(branch_id, &ss, &so,
+        vec![(&zaddr2, AMOUNT_SENT, None)]).unwrap();
+    let sent_tx = Transaction::read(&raw_tx[..]).unwrap();
+    let sent_txid = sent_tx.txid();
+
+    let mut cb4 = FakeCompactBlock::new(3, cb3.hash());
+    cb4.add_tx(&sent_tx);
+    wallet.scan_block(&cb4.as_bytes()).unwrap();
+    wallet.scan_full_tx(&sent_tx, 3, 0);
+
+    {
+        let txs = wallet.txs.read().unwrap();
+
+        // No Outgoing meta data, since this is a wallet -> wallet tx without a memo
+        assert_eq!(txs[&sent_txid].outgoing_metadata.len(), 0);
+    }
+}
+
+
+
 #[test]
 fn test_multi_z() {
     const AMOUNT1: u64 = 50000;
@@ -849,12 +1020,13 @@ fn test_multi_z() {
         assert_eq!(txs[&sent_txid].notes[ext_note_number].is_change, false);
         assert_eq!(txs[&sent_txid].notes[ext_note_number].spent, None);
         assert_eq!(txs[&sent_txid].notes[ext_note_number].unconfirmed_spent, None);
-        assert_eq!(LightWallet::memo_str(&txs[&sent_txid].notes[ext_note_number].memo), Some(outgoing_memo));
+        assert_eq!(LightWallet::memo_str(&txs[&sent_txid].notes[ext_note_number].memo), Some(outgoing_memo.clone()));
 
         assert_eq!(txs[&sent_txid].total_shielded_value_spent, AMOUNT1);
 
-        // No Outgoing meta data, since this is a wallet -> wallet tx
-        assert_eq!(txs[&sent_txid].outgoing_metadata.len(), 0);
+        // Includes Outgoing meta data, since this is a wallet -> wallet tx with a memo
+        assert_eq!(txs[&sent_txid].outgoing_metadata.len(), 1);
+        assert_eq!(txs[&sent_txid].outgoing_metadata[0].memo.to_utf8().unwrap().unwrap(), outgoing_memo);
     }
 
     // Now spend the money, which should pick notes from both addresses