zcash / librustzcash

Rust-language assets for Zcash
Other
324 stars 242 forks source link

Adding a second account with an older birthday height effectively changes height for existing account #1436

Open AArnott opened 6 days ago

AArnott commented 6 days ago

Repro steps

  1. Add an account A with a birthday height of x, where transactions exist prior to x that you don't expect to be discovered.
  2. Sync fully. Observe that only transactions >= x are discovered.
  3. Add another account B with birthday height y < x, where y is low enough that more transactions from account A would be discovered if A were scanned at that height.

Expected

No additional transactions for A are found.

Actual

All the transactions between heights y and x are discovered for A and reported.

This screws up balance calculations because A's birthday height is no longer at an acceptable re-birth height.

nuttycom commented 4 days ago

This screws up balance calculations because A's birthday height is no longer at an acceptable re-birth height.

I don't understand this bit; the balance is exclusively the sum of unspent transaction outputs belonging to the wallet. Is the intended behavior here to exclude from the balance any value that was unspent prior to the birthday height?

AArnott commented 4 days ago

Note that another important side effect of ignoring the per-account birthday heights is that trial decryptions will take much longer, as you're testing with keys before they are even expected to work.

the balance is exclusively the sum of unspent transaction outputs belonging to the wallet

You're thinking about current balance, which is all most Zcash wallets show. I'm talking about calculating balance by summing up the net change impact of each transaction. This allows me to present an ordinary ledger like any cash book or financial application would show. But note how in the screenshot below, negative balances are shown, presumably because (at least) the first transaction withdrew funds, which means the birthday height was set wrong since the first transaction must always be a deposit.

image

So long as the ledger's oldest transaction adds ZEC to an empty account as of that block height, all transactions that follow it should compute the right balance assuming they actually understand their own net effect on that balance (which they absolutely should, but I've seen many bugs in this area in my code and librustzcash, as we've talked about).

A perhaps more resilient strategy might be to actually compute the balance for each row based on notes available as of that block height, which to be clear would be very cool to be able to do for a variety of reasons. But it wouldn't replace the need for running balance to be computed based on individual transaction impact when you list more than one transaction in a single block.