zingolabs / zingolib

An API and test-app that exposes zcash functionality for app consumption
MIT License
15 stars 22 forks source link

zingocli: summaries crash with funded viewkey #553

Closed fluidvanadium closed 10 months ago

fluidvanadium commented 11 months ago

summaries thread '' panicked at 'attempt to subtract with overflow', zingolib/src/wallet/data.rs:948:9 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace Error executing command summaries: receiving on a closed channel 2023-10-02T20:43:06.458196Z ERROR zingo_cli: Error executing command summaries: receiving on a closed channel thread 'main' panicked at 'called Result::unwrap() on an Err value: SendError { .. }', zingocli/src/lib.rs:130:55

fluidvanadium commented 11 months ago

data.rs:948

pub fn get_transaction_fee(&self) -> u64 {
    self.total_value_spent() - (self.value_outgoing() + self.total_change_returned())
}
AArnott commented 11 months ago

This happens to me as well in a wallet that includes the spending key.

AArnott commented 11 months ago

The calculation seems suspicious in that it may assume that the transaction is an outbound one. e.g. if it's inbound, one cannot calculate the total_change_returned.

AArnott commented 11 months ago

I wonder if this is related to #567, which reports one spend twice. I could see that causing the fee to be negative.

AArnott commented 11 months ago

Ultimately, in order to support incoming transactions, isn't the only reliable way to calculate fee to look at the turnstiles and transparent elements of a transaction to find out how much ZEC wasn't accounted for?

Something like: fee = orchard_pool_decrease + sapling_pool_decrease + transparent_pool_utxos - transparent_pool_sends

AloeareV commented 11 months ago

565 should fix this, once it's finished.

self.total_value_spent() - (self.value_outgoing() + self.total_change_returned()) makes no sense, and only works due to us having conflicting definitions of what change is.

fluidvanadium commented 11 months ago

trying to catch this in a test. it doesnt happen in the basic modern test case... hmm

fluidvanadium commented 11 months ago

zingocli treasury viewkey

list_transactions ... TXID: f88... amount: -15_000_010_000 outgoing_metadata: [ { address: u1blahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblah value: 15_000_000_000 memo: "Septmeber!" }, { address: u1blahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblah value: 15_000_000_000 memo: "Septmeber!" }, ]

summaries ... TXID f88... spent 18_063_706_992 received 3_063_696_992 sent out 30_000_000_000 underflow panic at zingolib/src/wallet/data.rs:963 <

please note that sent out 30_000_000_000 = 15_000_000_000 + 15_000_000_000 this suggests that the unified address and the orchard address are being summed, improperly doubling the apparent send out. the root cause is confusing the unified address, a data point we want to store somewhere, with the on chain orchard address which an appropriate sole source of truth for a transaction history

AloeareV commented 11 months ago

Ultimately, in order to support incoming transactions, isn't the only reliable way to calculate fee to look at the turnstiles and transparent elements of a transaction to find out how much ZEC wasn't accounted for?

Something like: fee = orchard_pool_decrease + sapling_pool_decrease + transparent_pool_utxos - transparent_pool_sends

Generally, we don't care about the fee for an incoming-only transaction, and it's not strictly necessary for the outgoing case. I've looked into chasing down the transparent part of the turnstile before and it's not as easy as I would expect it to be, but given the amount of bugs we've been dealing with in order to avoid it it's probably the right solution at this point, rather than continuing to jump through hoops to avoid it.

fluidvanadium commented 11 months ago

1697822196

another, address independent, duplicate "outgoing_metadata"