From 58e5ea4946532aaf1d0e6c08014e08b62a5d922c Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Mon, 21 Oct 2024 19:40:02 -0300 Subject: [PATCH 1/7] add output_index to transaction metadata --- libtonode-tests/tests/concrete.rs | 9 +++++++-- zingolib/src/wallet/data.rs | 11 ++++++++++- zingolib/src/wallet/transaction_context.rs | 6 ++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/libtonode-tests/tests/concrete.rs b/libtonode-tests/tests/concrete.rs index 93677ead9..6d67af44e 100644 --- a/libtonode-tests/tests/concrete.rs +++ b/libtonode-tests/tests/concrete.rs @@ -1277,7 +1277,8 @@ mod slow { recipient_address: "zregtestsapling1fmq2ufux3gm0v8qf7x585wj56le4wjfsqsj27zprjghntrerntggg507hxh2ydcdkn7sx8kya7p".to_string(), value: first_send_to_sapling, memo: Memo::Empty, - recipient_ua: None + recipient_ua: None, + output_index: None, }]) .build() .unwrap(); @@ -1307,6 +1308,7 @@ mod slow { value: first_send_to_transparent, memo: Memo::Empty, recipient_ua: None, + output_index: None, }]) .build() .unwrap(); @@ -1420,6 +1422,7 @@ mod slow { value: second_send_to_transparent, memo: Memo::Empty, recipient_ua: None, + output_index: None, }]) .build() .unwrap(); @@ -1457,7 +1460,8 @@ mod slow { recipient_address: "zregtestsapling1fmq2ufux3gm0v8qf7x585wj56le4wjfsqsj27zprjghntrerntggg507hxh2ydcdkn7sx8kya7p".to_string(), value: second_send_to_sapling, memo: Memo::Empty, - recipient_ua: None + recipient_ua: None, + output_index: None, }]) .build() .unwrap(); @@ -1499,6 +1503,7 @@ mod slow { value: external_transparent_3, memo: Memo::Empty, recipient_ua: None, + output_index: None, }]) .build() .unwrap(); diff --git a/zingolib/src/wallet/data.rs b/zingolib/src/wallet/data.rs index 41af97d7a..9c0f6f708 100644 --- a/zingolib/src/wallet/data.rs +++ b/zingolib/src/wallet/data.rs @@ -291,6 +291,8 @@ pub struct OutgoingTxData { /// What if it wasn't provided? How does this relate to /// recipient_address? pub recipient_ua: Option, + /// This output's pool-specific index in its containing transaction + pub output_index: Option, } impl std::fmt::Display for OutgoingTxData { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -370,6 +372,7 @@ impl OutgoingTxData { value, memo, recipient_ua: None, + output_index: None, }) } @@ -2046,6 +2049,7 @@ pub(crate) mod mocks { value: Option, memo: Option, recipient_ua: Option>, + output_index: Option>, } impl OutgoingTxDataBuilder { @@ -2055,6 +2059,7 @@ pub(crate) mod mocks { value: None, memo: None, recipient_ua: None, + output_index: None, } } @@ -2063,6 +2068,7 @@ pub(crate) mod mocks { build_method!(value, u64); build_method!(memo, Memo); build_method!(recipient_ua, Option); + build_method!(output_index, Option); pub(crate) fn build(&self) -> OutgoingTxData { OutgoingTxData { @@ -2070,6 +2076,7 @@ pub(crate) mod mocks { value: self.value.unwrap(), memo: self.memo.clone().unwrap(), recipient_ua: self.recipient_ua.clone().unwrap(), + output_index: self.output_index.unwrap(), } } } @@ -2081,7 +2088,9 @@ pub(crate) mod mocks { .recipient_address("default_address".to_string()) .value(50_000) .memo(Memo::default()) - .recipient_ua(None); + .recipient_ua(None) + .output_index(None); + builder } } diff --git a/zingolib/src/wallet/transaction_context.rs b/zingolib/src/wallet/transaction_context.rs index 1c10ac84a..6f9ee2507 100644 --- a/zingolib/src/wallet/transaction_context.rs +++ b/zingolib/src/wallet/transaction_context.rs @@ -556,7 +556,7 @@ mod decrypt_transaction { // 1. There's more than one way to be "spent". // 2. It's possible for a "nullifier" to be in the wallet's spent list, but never in the global ledger. // - for (_domain, output) in domain_tagged_outputs { + for (i, (_domain, output)) in domain_tagged_outputs.iter().enumerate() { outgoing_metadatas.extend( match try_output_recovery_with_ovk::< D, @@ -612,6 +612,7 @@ mod decrypt_transaction { value: D::WalletNote::value_from_note(¬e), memo, recipient_ua: None, + output_index: Some(i as u64), }) } } @@ -643,7 +644,7 @@ mod decrypt_transaction { .transaction_kind(transaction_record, &self.config.chain) { if let Some(t_bundle) = transaction.transparent_bundle() { - for vout in &t_bundle.vout { + for (i, vout) in t_bundle.vout.iter().enumerate() { if let Some(taddr) = vout.recipient_address().map(|raw_taddr| { match sent_to_tex { false => address_from_pubkeyhash(&self.config, raw_taddr), @@ -662,6 +663,7 @@ mod decrypt_transaction { value: u64::from(vout.value), memo: Memo::Empty, recipient_ua: None, + output_index: Some(i as u64), }); } } From b657ee72fa90501228a31f15fce51eca154b7a82 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Tue, 29 Oct 2024 14:54:30 -0300 Subject: [PATCH 2/7] sort attempt --- zingolib/src/lightclient/describe.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/zingolib/src/lightclient/describe.rs b/zingolib/src/lightclient/describe.rs index 09e7feb10..66b598e00 100644 --- a/zingolib/src/lightclient/describe.rs +++ b/zingolib/src/lightclient/describe.rs @@ -590,6 +590,24 @@ impl LightClient { }; } + value_transfers.sort_by(|t1, t2| match t1.txid().cmp(&t2.txid()) { + Ordering::Equal => match match (t1.pool_received(), t2.pool_received()) { + (None, None) => Ordering::Equal, + (None, Some(_)) => Ordering::Less, + (Some(_), None) => Ordering::Greater, + (Some(pool1), Some(pool2)) if pool1 == pool2 => Ordering::Greater, + (Some("transparent"), Some(_)) => Ordering::Less, + (Some(_), Some("transparent")) => Ordering::Greater, + (Some("sapling"), Some(_)) => Ordering::Less, + (Some(_), Some("sapling")) => Ordering::Greater, + (Some(pool1), Some(pool2)) => panic!("invalid pool variants"), + } { + Ordering::Equal => todo!(), + nonequal => nonequal, + }, + nonequal => nonequal, + }); + ValueTransfers(value_transfers) } From f80e8fecd495a5318805056616e73aab4d46e728 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Tue, 29 Oct 2024 17:05:44 -0300 Subject: [PATCH 3/7] remove hacky post-hoc sort in favor of pushing value transfers in sorted order --- zingolib/src/lightclient/describe.rs | 26 +++++----------------- zingolib/src/wallet/data.rs | 2 +- zingolib/src/wallet/traits.rs | 19 ++++++++++++++++ zingolib/src/wallet/transaction_context.rs | 4 +++- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/zingolib/src/lightclient/describe.rs b/zingolib/src/lightclient/describe.rs index 66b598e00..1214d96b1 100644 --- a/zingolib/src/lightclient/describe.rs +++ b/zingolib/src/lightclient/describe.rs @@ -282,7 +282,7 @@ impl LightClient { transaction_summary: &TransactionSummary, ) { let mut addresses = - HashSet::with_capacity(transaction_summary.outgoing_tx_data().len()); + HashMap::with_capacity(transaction_summary.outgoing_tx_data().len()); transaction_summary .outgoing_tx_data() .iter() @@ -293,9 +293,11 @@ impl LightClient { outgoing_tx_data.recipient_address.clone() }; // hash set is used to create unique list of addresses as duplicates are not inserted twice - addresses.insert(address); + addresses.insert(address, outgoing_tx_data.output_index); }); - addresses.iter().for_each(|address| { + let mut addresses_vec = addresses.into_iter().collect::>(); + addresses_vec.sort_by_key(|x| x.1); + addresses_vec.iter().for_each(|(address, _output_index)| { let outgoing_data_to_address: Vec = transaction_summary .outgoing_tx_data() .iter() @@ -590,24 +592,6 @@ impl LightClient { }; } - value_transfers.sort_by(|t1, t2| match t1.txid().cmp(&t2.txid()) { - Ordering::Equal => match match (t1.pool_received(), t2.pool_received()) { - (None, None) => Ordering::Equal, - (None, Some(_)) => Ordering::Less, - (Some(_), None) => Ordering::Greater, - (Some(pool1), Some(pool2)) if pool1 == pool2 => Ordering::Greater, - (Some("transparent"), Some(_)) => Ordering::Less, - (Some(_), Some("transparent")) => Ordering::Greater, - (Some("sapling"), Some(_)) => Ordering::Less, - (Some(_), Some("sapling")) => Ordering::Greater, - (Some(pool1), Some(pool2)) => panic!("invalid pool variants"), - } { - Ordering::Equal => todo!(), - nonequal => nonequal, - }, - nonequal => nonequal, - }); - ValueTransfers(value_transfers) } diff --git a/zingolib/src/wallet/data.rs b/zingolib/src/wallet/data.rs index b54fe18aa..3c843ff79 100644 --- a/zingolib/src/wallet/data.rs +++ b/zingolib/src/wallet/data.rs @@ -291,7 +291,7 @@ pub struct OutgoingTxData { /// What if it wasn't provided? How does this relate to /// recipient_address? pub recipient_ua: Option, - /// This output's pool-specific index in its containing transaction + /// This output's index in its containing transaction pub output_index: Option, } impl std::fmt::Display for OutgoingTxData { diff --git a/zingolib/src/wallet/traits.rs b/zingolib/src/wallet/traits.rs index 95d1ec1dd..b55ccb194 100644 --- a/zingolib/src/wallet/traits.rs +++ b/zingolib/src/wallet/traits.rs @@ -537,6 +537,10 @@ pub trait DomainWalletExt: /// TODO: Add Doc Comment Here! fn get_tree(tree_state: &TreeState) -> &String; + /// Counts the number of outputs in earlier pools, to allow for + // the output index to be a single source for output order + fn output_index_offset(transaction: &Transaction) -> u64; + /// TODO: Add Doc Comment Here! fn ua_from_contained_receiver<'a>( unified_spend_auth: &'a WalletCapability, @@ -596,6 +600,13 @@ impl DomainWalletExt for SaplingDomain { &tree_state.sapling_tree } + fn output_index_offset(transaction: &Transaction) -> u64 { + transaction + .transparent_bundle() + .map(|tbundle| tbundle.vout.len() as u64) + .unwrap_or(0) + } + fn ua_from_contained_receiver<'a>( unified_spend_auth: &'a WalletCapability, receiver: &Self::Recipient, @@ -660,6 +671,14 @@ impl DomainWalletExt for OrchardDomain { &tree_state.orchard_tree } + fn output_index_offset(transaction: &Transaction) -> u64 { + SaplingDomain::output_index_offset(transaction) + + transaction + .sapling_bundle() + .map(|sbundle| sbundle.shielded_outputs().len() as u64) + .unwrap_or(0) + } + fn ua_from_contained_receiver<'a>( unified_spend_capability: &'a WalletCapability, receiver: &Self::Recipient, diff --git a/zingolib/src/wallet/transaction_context.rs b/zingolib/src/wallet/transaction_context.rs index a63cf9574..8454cb7bf 100644 --- a/zingolib/src/wallet/transaction_context.rs +++ b/zingolib/src/wallet/transaction_context.rs @@ -611,7 +611,9 @@ mod decrypt_transaction { value: D::WalletNote::value_from_note(¬e), memo, recipient_ua: None, - output_index: Some(i as u64), + output_index: Some( + D::output_index_offset(&transaction) + i as u64, + ), }) } } From 53f0a85838568e4e44773eed7b559754a824392e Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Wed, 30 Oct 2024 17:12:39 -0300 Subject: [PATCH 4/7] add output index to read/write --- zingolib/src/lightclient/describe.rs | 7 +--- zingolib/src/wallet/data.rs | 47 +++++++++++++++++++++-- zingolib/src/wallet/transaction_record.rs | 8 ++-- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/zingolib/src/lightclient/describe.rs b/zingolib/src/lightclient/describe.rs index 1214d96b1..0c2fb6170 100644 --- a/zingolib/src/lightclient/describe.rs +++ b/zingolib/src/lightclient/describe.rs @@ -2,10 +2,7 @@ use ::orchard::note_encryption::OrchardDomain; use json::{object, JsonValue}; use sapling_crypto::note_encryption::SaplingDomain; -use std::{ - cmp::Ordering, - collections::{HashMap, HashSet}, -}; +use std::{cmp::Ordering, collections::HashMap}; use tokio::runtime::Runtime; use zcash_client_backend::{encoding::encode_payment_address, PoolType, ShieldedProtocol}; @@ -292,7 +289,7 @@ impl LightClient { } else { outgoing_tx_data.recipient_address.clone() }; - // hash set is used to create unique list of addresses as duplicates are not inserted twice + // hash map is used to create unique list of addresses as duplicates are not inserted twice addresses.insert(address, outgoing_tx_data.output_index); }); let mut addresses_vec = addresses.into_iter().collect::>(); diff --git a/zingolib/src/wallet/data.rs b/zingolib/src/wallet/data.rs index 3c843ff79..85283fa0d 100644 --- a/zingolib/src/wallet/data.rs +++ b/zingolib/src/wallet/data.rs @@ -15,7 +15,7 @@ use zcash_client_backend::{ proto::compact_formats::CompactBlock, wallet::TransparentAddressMetadata, }; -use zcash_encoding::{Optional, Vector}; +use zcash_encoding::{CompactSize, Optional, Vector}; use zcash_primitives::merkle_tree::{read_commitment_tree, write_commitment_tree}; use zcash_primitives::{legacy::TransparentAddress, memo::MemoBytes}; @@ -345,8 +345,9 @@ impl PartialEq for OutgoingTxData { } } impl OutgoingTxData { - /// TODO: Add Doc Comment Here! - pub fn read(mut reader: R) -> io::Result { + const VERSION: u64 = 0; + /// Before version 0, OutgoingTxData didn't have a version field + pub fn read_old(mut reader: R) -> io::Result { let address_len = reader.read_u64::()?; let mut address_bytes = vec![0; address_len as usize]; reader.read_exact(&mut address_bytes)?; @@ -376,8 +377,43 @@ impl OutgoingTxData { }) } + /// Read an OutgoingTxData from its serialized + /// representation + pub fn read(mut reader: R) -> io::Result { + let _external_version = CompactSize::read(&mut reader); + let address_len = reader.read_u64::()?; + let mut address_bytes = vec![0; address_len as usize]; + reader.read_exact(&mut address_bytes)?; + let address = String::from_utf8(address_bytes).unwrap(); + + let value = reader.read_u64::()?; + + let mut memo_bytes = [0u8; 512]; + reader.read_exact(&mut memo_bytes)?; + let memo = match MemoBytes::from_bytes(&memo_bytes) { + Ok(mb) => match Memo::try_from(mb.clone()) { + Ok(m) => Ok(m), + Err(_) => Ok(Memo::Future(mb)), + }, + Err(e) => Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("Couldn't create memo: {}", e), + )), + }?; + let output_index = Optional::read(&mut reader, CompactSize::read)?; + + Ok(OutgoingTxData { + recipient_address: address, + value, + memo, + recipient_ua: None, + output_index, + }) + } + /// TODO: Add Doc Comment Here! pub fn write(&self, mut writer: W) -> io::Result<()> { + CompactSize::write(&mut writer, Self::VERSION as usize)?; // Strings are written as len + utf8 match &self.recipient_ua { None => { @@ -390,7 +426,10 @@ impl OutgoingTxData { } } writer.write_u64::(self.value)?; - writer.write_all(self.memo.encode().as_array()) + writer.write_all(self.memo.encode().as_array())?; + Optional::write(&mut writer, self.output_index, |w, output_index| { + CompactSize::write(w, output_index as usize) + }) } } diff --git a/zingolib/src/wallet/transaction_record.rs b/zingolib/src/wallet/transaction_record.rs index ec2f7aa8e..6dcfffe71 100644 --- a/zingolib/src/wallet/transaction_record.rs +++ b/zingolib/src/wallet/transaction_record.rs @@ -445,10 +445,10 @@ impl TransactionRecord { 0 }; - // Outgoing metadata was only added in version 2 - let outgoing_metadata = - zcash_encoding::Vector::read(&mut reader, |r| OutgoingTxData::read(r))?; - + let outgoing_metadata = match version { + ..24 => zcash_encoding::Vector::read(&mut reader, |r| OutgoingTxData::read_old(r))?, + 24.. => zcash_encoding::Vector::read(&mut reader, |r| OutgoingTxData::read(r))?, + }; let _full_tx_scanned = reader.read_u8()? > 0; let zec_price = if version <= 4 { From 64b374dcb4304f350b1792eb40b4961fbe594019 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Wed, 30 Oct 2024 22:10:36 +0000 Subject: [PATCH 5/7] cargo clippy --fix --tests --all-features --- zingolib/src/wallet/transaction_context.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zingolib/src/wallet/transaction_context.rs b/zingolib/src/wallet/transaction_context.rs index 8454cb7bf..b348c7414 100644 --- a/zingolib/src/wallet/transaction_context.rs +++ b/zingolib/src/wallet/transaction_context.rs @@ -563,7 +563,7 @@ mod decrypt_transaction { >( &output.domain(status.get_height(), self.config.chain), &ovk.ovk, - &output, + output, &output.value_commitment(), &output.out_ciphertext(), ) { @@ -612,7 +612,7 @@ mod decrypt_transaction { memo, recipient_ua: None, output_index: Some( - D::output_index_offset(&transaction) + i as u64, + D::output_index_offset(transaction) + i as u64, ), }) } From f512bd336ad1f8fe812695e8d0cdd398b92dc996 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 31 Oct 2024 12:25:15 -0300 Subject: [PATCH 6/7] fix unused result --- zingolib/src/wallet/data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zingolib/src/wallet/data.rs b/zingolib/src/wallet/data.rs index 85283fa0d..133305b44 100644 --- a/zingolib/src/wallet/data.rs +++ b/zingolib/src/wallet/data.rs @@ -380,7 +380,7 @@ impl OutgoingTxData { /// Read an OutgoingTxData from its serialized /// representation pub fn read(mut reader: R) -> io::Result { - let _external_version = CompactSize::read(&mut reader); + let _external_version = CompactSize::read(&mut reader)?; let address_len = reader.read_u64::()?; let mut address_bytes = vec![0; address_len as usize]; reader.read_exact(&mut address_bytes)?; From f7be359e19aa7eefaf82ad49044afabd469265a1 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 31 Oct 2024 12:29:51 -0300 Subject: [PATCH 7/7] actually bump version number in write --- zingolib/src/wallet/transaction_record.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_record.rs b/zingolib/src/wallet/transaction_record.rs index 6dcfffe71..d147f1c99 100644 --- a/zingolib/src/wallet/transaction_record.rs +++ b/zingolib/src/wallet/transaction_record.rs @@ -496,7 +496,7 @@ impl TransactionRecord { /// TODO: Add Doc Comment Here! pub fn serialized_version() -> u64 { - 23 + 24 } /// TODO: Add Doc Comment Here!