Improve fee and change logic

zingo-testutils/src/data.rs

@@ -16,6 +16,7 @@ pub mod seeds {
      unfold vocal weird milk scale social vessel identify \
      crowd hospital control album rib bulb path oven civil tank";
 }
+pub const BLOCK_REWARD: u64 = 625_000_000u64;
 pub const REGSAP_ADDR_FROM_ABANDONART: &str =
     "zregtestsapling1fmq2ufux3gm0v8qf7x585wj56le4wjfsqsj27zprjghntrerntggg507hxh2ydcdkn7sx8kya7p";
 pub mod config_template_fillers {

zingocli/tests/integration_tests.rs

@@ -5,7 +5,11 @@ use orchard::tree::MerkleHashOrchard;
 use shardtree::store::memory::MemoryShardStore;
 use shardtree::ShardTree;
 use std::{fs::File, path::Path, str::FromStr};
-use zingo_testutils::{self, build_fvk_client, data, BASE_HEIGHT};
+use zingo_testutils::{
+    self, build_fvk_client,
+    data::{self, BLOCK_REWARD},
+    BASE_HEIGHT,
+};
 
 use bip0039::Mnemonic;
 use data::seeds::HOSPITAL_MUSEUM_SEED;
@@ -687,10 +691,11 @@ async fn send_mined_sapling_to_orchard() {
     // We send change to orchard now, so we should have the full value of the note
     // we spent, minus the transaction fee
     assert_eq!(balance.unverified_orchard_balance, Some(0));
-    assert_eq!(
-        balance.verified_orchard_balance.unwrap(),
-        625_000_000 - u64::from(MINIMUM_FEE)
-    );
+    let expected_orchard = BLOCK_REWARD - u64::from(MINIMUM_FEE);
+    assert_eq!(balance.verified_orchard_balance.unwrap(), expected_orchard);
+    // Important to remember that the block reward includes not just the base 6.25 ZEC,
+    // but also all fees paid.
+    check_client_balances!(faucet, o: expected_orchard s: {(BLOCK_REWARD * 2) + u64::from(MINIMUM_FEE)} t: 0);
 }
 
 fn extract_value_as_u64(input: &JsonValue) -> u64 {
@@ -961,7 +966,6 @@ async fn from_t_z_o_tz_to_zo_tzo_to_orchard() {
 async fn send_orchard_back_and_forth() {
     // setup
     let (regtest_manager, _cph, faucet, recipient) = scenarios::two_wallet_one_miner_fund().await;
-    let block_reward = 625_000_000u64;
     let faucet_to_recipient_amount = 20_000u64;
     let recipient_to_faucet_amount = 5_000u64;
     // check start state
@@ -971,7 +975,7 @@ async fn send_orchard_back_and_forth() {
         wallet_height.as_fixed_point_u64(0).unwrap(),
         BASE_HEIGHT as u64
     );
-    let three_blocks_reward = block_reward.checked_mul(BASE_HEIGHT as u64).unwrap();
+    let three_blocks_reward = BLOCK_REWARD.checked_mul(BASE_HEIGHT as u64).unwrap();
     check_client_balances!(faucet, o: 0 s: three_blocks_reward  t: 0);
 
     // post transfer to recipient, and verify
@@ -983,7 +987,7 @@ async fn send_orchard_back_and_forth() {
         )])
         .await
         .unwrap();
-    let orch_change = block_reward - (faucet_to_recipient_amount + u64::from(MINIMUM_FEE));
+    let orch_change = BLOCK_REWARD - (faucet_to_recipient_amount + u64::from(MINIMUM_FEE));
     let reward_and_fee = three_blocks_reward + u64::from(MINIMUM_FEE);
     zingo_testutils::increase_height_and_wait_for_client(&regtest_manager, &recipient, 1)
         .await
@@ -1019,7 +1023,7 @@ async fn send_orchard_back_and_forth() {
     let recipient_final_orch =
         faucet_to_recipient_amount - (u64::from(MINIMUM_FEE) + recipient_to_faucet_amount);
     let faucet_final_orch = orch_change + recipient_to_faucet_amount;
-    let faucet_final_block = 4 * block_reward + u64::from(MINIMUM_FEE) * 2;
+    let faucet_final_block = 4 * BLOCK_REWARD + u64::from(MINIMUM_FEE) * 2;
     check_client_balances!(
         faucet,
         o: faucet_final_orch s: faucet_final_block t: 0

zingolib/src/lightclient.rs

@@ -824,13 +824,14 @@ impl LightClient {
             .iter()
         {
             LightClient::tx_summary_matcher(&mut summaries, *txid, transaction_md);
-            let tx_fee = transaction_md.get_transaction_fee();
-            let (block_height, datetime, price) = (
-                transaction_md.block_height,
-                transaction_md.datetime,
-                transaction_md.price,
-            );
-            if transaction_md.is_outgoing_transaction() {
+            if let Some(tx_fee) = transaction_md
+                .get_transaction_fee(&self.wallet.wallet_capability(), &self.config().chain)
+            {
+                let (block_height, datetime, price) = (
+                    transaction_md.block_height,
+                    transaction_md.datetime,
+                    transaction_md.price,
+                );
                 summaries.push(ValueTransfer {
                     block_height,
                     datetime,

zingolib/src/wallet/data.rs

@@ -14,10 +14,11 @@ use shardtree::ShardTree;
 use std::convert::TryFrom;
 use std::io::{self, Read, Write};
 use std::usize;
+use zcash_client_backend::address::RecipientAddress;
 use zcash_client_backend::serialization::shardtree::{read_shard, write_shard};
 use zcash_encoding::{Optional, Vector};
 use zcash_note_encryption::Domain;
-use zcash_primitives::consensus::BlockHeight;
+use zcash_primitives::consensus::{BlockHeight, Parameters};
 use zcash_primitives::memo::MemoBytes;
 use zcash_primitives::merkle_tree::{read_commitment_tree, write_commitment_tree, HashSer};
 use zcash_primitives::sapling::note_encryption::SaplingDomain;
@@ -28,7 +29,7 @@ use zcash_primitives::{
 };
 use zingoconfig::{ChainType, MAX_REORG};
 
-use super::keys::unified::WalletCapability;
+use super::keys::unified::{orchard_viewkey, sapling_viewkey, WalletCapability};
 use super::traits::{self, DomainWalletExt, ReadableWriteable, ToBytes};
 
 pub const COMMITMENT_TREE_LEVELS: u8 = 32;
@@ -944,8 +945,27 @@ impl TransactionMetadata {
         }
     }
 
-    pub fn get_transaction_fee(&self) -> u64 {
-        self.total_value_spent() - (self.value_outgoing() + self.total_change_returned())
+    /// Returns Some(fee) for outgoing transactions, and None for non-outgoing
+    pub fn get_transaction_fee<P: Parameters>(
+        &self,
+        wc: &WalletCapability,
+        params: &P,
+    ) -> Option<u64> {
+        if self.is_outgoing_transaction() {
+            println!(
+                "TXID {} spent {} and received {}",
+                self.txid,
+                self.total_value_spent(),
+                self.total_value_received()
+            );
+            Some(
+                self.total_value_spent()
+                    - self.total_value_received()
+                    - self.value_outgoing(wc, params),
+            )
+        } else {
+            None
+        }
     }
 
     // TODO: This is incorrect in the edge case where where we have a send-to-self with
@@ -1145,9 +1165,54 @@ impl TransactionMetadata {
         self.value_spent_by_pool().iter().sum()
     }
 
-    pub fn value_outgoing(&self) -> u64 {
+    pub fn value_outgoing<P: Parameters>(&self, wc: &WalletCapability, params: &P) -> u64 {
+        let viewkey = wc.ufvk().unwrap();
         self.outgoing_tx_data
             .iter()
+            // Filter out sends-to-self, by checking to see if are capable of generating the address
+            // we sent to from our viewing key(s)
+            .filter(
+                |outgoing| match RecipientAddress::decode(params, &outgoing.to_address) {
+                    Some(RecipientAddress::Shielded(sapling_addr)) => sapling_viewkey(&viewkey)
+                        .map_or(true, |sapling_viewkey| {
+                            sapling_viewkey.decrypt_diversifier(&sapling_addr).is_none()
+                        }),
+                    //pubkey_to_address is deprecated, but as of yet there isn't a good alternative
+                    #[allow(deprecated)]
+                    Some(RecipientAddress::Transparent(taddr)) => wc
+                        .transparent_child_keys()
+                        .map_or(true, |child_pubkey_vec| {
+                            child_pubkey_vec.iter().any(|(_diversifier_index, pubkey)| {
+                                zcash_primitives::legacy::keys::pubkey_to_address(pubkey) != taddr
+                            })
+                        }),
+                    Some(RecipientAddress::Unified(ua)) => {
+                        if let Some(orchard_addr) = ua.orchard() {
+                            orchard_viewkey(&viewkey).map_or(true, |orchard_viewkey| {
+                                orchard_viewkey.scope_for_address(orchard_addr).is_none()
+                            })
+                        } else if let Some(sapling_addr) = ua.sapling() {
+                            sapling_viewkey(&viewkey).map_or(true, |sapling_viewkey| {
+                                sapling_viewkey.decrypt_diversifier(&sapling_addr).is_none()
+                            })
+                        } else {
+                            // If the UA we sent to has no sapling or orchard components,
+                            // this means this is a send to a future pool that did not
+                            // exist at the time of this code being written.
+                            // This is probably the the least wrong thing to do here.
+                            true
+                        }
+                    }
+                    // This is situation that should never occur.
+                    // If this is none, it means we can't parse the address we sent to
+                    // This is either some future address type (exceedingly unlikely, as UAs are
+                    // future-compatible), or we somehow sent to an invalid address
+                    // (Maybe there's a way to burn funds by sending to a bad address?)
+                    None => {
+                        unreachable!("Sent to an address we can't parse: {}", outgoing.to_address)
+                    }
+                },
+            )
             .fold(0, |running_total, tx_data| tx_data.value + running_total)
     }
 

zingolib/src/wallet/keys/unified.rs

@@ -70,7 +70,7 @@ pub struct WalletCapability {
     >,
     pub orchard: Capability<orchard::keys::FullViewingKey, orchard::keys::SpendingKey>,
 
-    transparent_child_keys: append_only_vec::AppendOnlyVec<(usize, secp256k1::SecretKey)>,
+    transparent_child_keys: append_only_vec::AppendOnlyVec<(usize, secp256k1::PublicKey)>,
     addresses: append_only_vec::AppendOnlyVec<UnifiedAddress>,
     // Not all diversifier indexes produce valid sapling addresses.
     // Because of this, the index isn't necessarily equal to addresses.len()
@@ -147,11 +147,11 @@ impl WalletCapability {
 
     pub fn transparent_child_keys(
         &self,
-    ) -> Result<&AppendOnlyVec<(usize, secp256k1::SecretKey)>, String> {
-        if self.transparent.can_spend() {
+    ) -> Result<&AppendOnlyVec<(usize, secp256k1::PublicKey)>, String> {
+        if self.transparent.can_view() {
             Ok(&self.transparent_child_keys)
         } else {
-            Err("The wallet is not capable of spending transparent funds.".to_string())
+            Err("The wallet is not capable of viewing transparent funds.".to_string())
         }
     }
 
@@ -245,7 +245,7 @@ impl WalletCapability {
                     let secp = secp256k1::Secp256k1::new();
                     let child_pk = secp256k1::PublicKey::from_secret_key(&secp, &child_sk);
                     self.transparent_child_keys
-                        .push((self.addresses.len(), child_sk));
+                        .push((self.addresses.len(), child_pk));
                     Some(child_pk)
                 }
                 Capability::View(ext_pk) => {
@@ -257,6 +257,8 @@ impl WalletCapability {
                         }
                         Ok(res) => res.public_key,
                     };
+                    self.transparent_child_keys
+                        .push((self.addresses.len(), child_pk));
                     Some(child_pk)
                 }
                 Capability::None => None,
@@ -297,7 +299,7 @@ impl WalletCapability {
         &self,
         config: &ZingoConfig,
     ) -> Result<HashMap<String, secp256k1::SecretKey>, String> {
-        if self.transparent.can_spend() {
+        if let Capability::Spend(ref ext_sk) = self.transparent {
             Ok(self
                 .addresses
                 .iter()
@@ -310,13 +312,19 @@ impl WalletCapability {
                     )
                 })
                 .map(|(taddr, key)| {
-                    let hash = match taddr {
-                        TransparentAddress::PublicKey(hash) => hash,
-                        TransparentAddress::Script(hash) => hash,
-                    };
                     (
-                        hash.to_base58check(&config.base58_pubkey_address(), &[]),
-                        key.1,
+                        match taddr {
+                            TransparentAddress::PublicKey(hash) => {
+                                hash.to_base58check(&config.base58_pubkey_address(), &[])
+                            }
+                            TransparentAddress::Script(hash) => {
+                                hash.to_base58check(&config.base58_script_address(), &[])
+                            }
+                        },
+                        ext_sk
+                            .derive_private_key(KeyIndex::from_index(key.0 as u32).unwrap())
+                            .unwrap()
+                            .private_key,
                     )
                 })
                 .collect())
@@ -480,6 +488,38 @@ impl WalletCapability {
     }
 }
 
+pub fn orchard_viewkey(ufvk: &Ufvk) -> Option<orchard::keys::FullViewingKey> {
+    ufvk.items().iter().find_map(|receiver| {
+        if let Fvk::Orchard(fvk_bytes) = receiver {
+            orchard::keys::FullViewingKey::from_bytes(fvk_bytes)
+        } else {
+            None
+        }
+    })
+}
+pub fn sapling_viewkey(
+    ufvk: &Ufvk,
+) -> Option<zcash_primitives::zip32::sapling::DiversifiableFullViewingKey> {
+    ufvk.items().iter().find_map(|receiver| {
+        if let Fvk::Sapling(fvk_bytes) = receiver {
+            zcash_primitives::zip32::sapling::DiversifiableFullViewingKey::from_bytes(fvk_bytes)
+        } else {
+            None
+        }
+    })
+}
+
+pub fn transparent_viewkey(ufvk: &Ufvk) -> Option<zcash_primitives::legacy::keys::ExternalIvk> {
+    use zcash_primitives::legacy::keys::IncomingViewingKey as _;
+    ufvk.items().iter().find_map(|receiver| {
+        if let Fvk::P2pkh(fvk_bytes) = receiver {
+            zcash_primitives::legacy::keys::ExternalIvk::deserialize(fvk_bytes).ok()
+        } else {
+            None
+        }
+    })
+}
+
 /// Reads a transparent ExtendedPrivKey from a buffer that has a 32 byte private key and 32 byte chain code.
 fn transparent_key_from_bytes(bytes: &[u8]) -> Result<ExtendedPrivKey, std::io::Error> {
     let mut reader = std::io::Cursor::new(bytes);
@@ -731,10 +771,14 @@ pub async fn get_transparent_secretkey_pubkey_taddr(
             let child_ext_pk = ext_pk.derive_public_key(KeyIndex::Normal(0)).ok();
             (None, child_ext_pk.map(|x| x.public_key))
         }
-        Capability::Spend(_) => {
-            let sk = wc.transparent_child_keys[0].1;
-            let secp = secp256k1::Secp256k1::new();
-            let pk = secp256k1::PublicKey::from_secret_key(&secp, &sk);
+        Capability::Spend(ext_sk) => {
+            let pk = wc.transparent_child_keys[0].1;
+            let sk = ext_sk
+                .derive_private_key(
+                    KeyIndex::from_index(wc.transparent_child_keys[0].0 as u32).unwrap(),
+                )
+                .unwrap()
+                .private_key;
             (Some(sk), Some(pk))
         }
     };

zingolib/src/wallet/transactions.rs

@@ -41,13 +41,6 @@ impl TransactionMetadataSet {
         22
     }
 
-    pub fn get_fee_by_txid(&self, txid: &TxId) -> u64 {
-        self.current
-            .get(txid)
-            .expect("To have the requested txid")
-            .get_transaction_fee()
-    }
-
     pub fn read_old<R: Read>(
         mut reader: R,
         wallet_capability: &WalletCapability,
@@ -469,8 +462,12 @@ impl TransactionMetadataSet {
     fn mark_notes_as_change_for_pool<Note: ReceivedNoteAndMetadata>(notes: &mut [Note]) {
         notes.iter_mut().for_each(|n| {
             *n.is_change_mut() = match n.memo() {
-                Some(Memo::Text(_)) => false,
-                Some(Memo::Empty | Memo::Arbitrary(_) | Memo::Future(_)) | None => true,
+                Some(Memo::Arbitrary(arb_memo))
+                    if zingo_memo::parse_zingo_memo(**arb_memo).is_ok() =>
+                {
+                    true
+                }
+                _ => false,
             }
         });
     }