Fix double-entry accounting errors in transaction views. by nuttycom · Pull Request #807 · zcash/librustzcash

nuttycom · 2023-04-07T17:54:05Z

No description provided.

pacu

utACK.

I pushed this branch on the iOS FFI repo and pointed it to this branch. It has a small compiler error, could you check it out? I never understood the zcash_address crate and how it worked so I need some help over there to test this out

https://github.com/zcash-hackworks/zcash-light-client-ffi/pull/85

str4d · 2023-04-10T14:20:07Z

What is this EXCEPT necessary for? Do we have a test that exercises it?

This EXCEPT is the critical part that ensures we don't create redundant sent_notes entries.

Ah, I see. The from_account, from_account part was what confused me; it seems overly restrictive since we know that the relevant row will have (tx, :output_pool, output_index) be unique (so that is what we really want to match on). If we need to select values to have this side be compatible with the EXCEPT, we should select whatever is in the to_account column rather than selecting from_account twice.

The problem is that the value in the to_account column may be null, and I'm not sure under what conditions that might have arisen (i.e. to_account could be null because the record was created before that column existed and there is no correct way to populate it because the second account was created on a separate wallet but has not been created here.) However, selecting from_account twice should always give the correct result, I think.

str4d · 2023-04-10T14:31:26Z

Add a check that after the new migration is run, the only change to the data in the database is that sent_notes contains a row corresponding to this change note in received_notes.

…change output indices. Co-authored-by: Jack Grigg <jack@electriccoin.co>

pacu

left a comment, not specifically approving or requesting changes.

pacu · 2023-04-12T14:24:43Z

+                   transactions.txid                 AS txid,
+                   transactions.expiry_height        AS expiry_height,
+                   transactions.raw                  AS raw,
+                   SUM(notes.value)                  AS net_transfer,


@nuttycom I've tested it and I have a doubt. It could be either an error in the query or a misunderstanding.

When I send 20000 Zats I see that the mined transaction is read back from the chain with and the value present in net_transfer is -21000 and fee_paid of 1000.

I would expect that net_transfer is the value of the transaction minus all deductions (in this case the fee), and when I inspect the value I find the "gross" value, which is the whole value transferred, which I consider it to be the value indicated by user intent to be transferred to the recipient, plus the value of the fee (which in this case is 1000 but if could be something else less noticeable).

I see a couple of paths here:
a) this is intentional. The value property of a transaction in the SDK will show the sum total value transferred to recipients and the naming and to show the value transferred to recipients without the fee the wallet must add the fee to the net_transfer column

b) there's a mistake. The value property of a transaction in the SDK will show the value transferred to recipients and to show the whole value of the transaction, wallets must add the fee to the value present in the net_transfer column.

Also, fee value for received transactions is zero

@pacu your answer is (a), this is intentional. The reason for this is, consider this scenario:

Your wallet recovers a transaction that:

Spends a 500 ZAT note belonging to account 0

Spends a 20500 ZAT note belonging to account 1

Has an output of 20000 ZAT that goes to an external address, recovered with account 0's OVK.

Paid a fee of 1000 ZAT

This transaction will result in two entries in v_transactions, one associated with each account. The problem is, where to associate the fee?

Consider further: the ECC wallet only attempts recovery for account 0. Now, you show as one of "your" wallet's transactions... what? The answer we have here is a transfer of -500, a fee of 1000 and 1 sent output.

The v_transactions view shows each transaction's effect on your account balances and there's actually nothing more that we can show at the transaction level.

there is no emoji to reaction to express my "mindblown-ness" 😅

Should wallets display the net_transfer straightforward in any case then? or shall they add or substract the fee?

I think that wallets should show net_transfer directly at the transaction level. You have to drill into the transaction to see the additional detail.

The value property of a transaction in the SDK will show the sum total value transferred to recipients and the naming and to show the value transferred to recipients without the fee the wallet must add the fee to the net_transfer column

Yes. Framing this another way, the net_transfer column is the total amount that the balance of the account increases or decreases by.

Maybe we should rename net_transfer back to net_value? Because that's absolutely what it is.

Or maybe account_net_value?

I think the main naming conflict here is that the "net value" of something is the total value minus all possible deductions which is not quite what's going on here.

pacu

i'm just rejecting because these are new changes that I haven't fully explicitly approved

Also, rename `v_tx_events` to `v_tx_outputs`.

pacu

I think this fixes the bug unstoppable reported plus other ones and paves the way ahead for future changes. I'll point them to the branch of the SDK that implements these changes so they can validate as well.

This change also settles on `account_value_delta` as the name of the column in `v_transactions` that describes the transaction's effect on the value of the associated account.

str4d

utACK. I'm sufficiently satisfied that we can cut a release with this. If the edge cases of the new views turn out to cause issues, we can refine them further in future releases (or replace them with a Rust API-based approach).

Missing changelog updates, but we can do that in the release PR.

str4d · 2023-04-14T22:40:28Z

+            INSERT INTO sent_notes (tx, output_pool, output_index, from_account, to_account, to_address, value, memo)
+            VALUES (1, 2, 1, 0, NULL, 'addrb', 3, X'61');
+            INSERT INTO received_notes (tx, output_index, account, diversifier, value, rcm, nf, is_change)
+            VALUES (1,    2, 0, '', 2, '', 'nf_c', true);").unwrap();


Non-blocking:

Suggested change

VALUES (1, 2, 0, '', 2, '', 'nf_c', true);").unwrap();

VALUES (1, 2, 0, '', 2, '', 'nf_c', true);").unwrap();

This space was actually intended, so that the output indices would line up.

nuttycom force-pushed the bug/v_transactions_net_value branch 2 times, most recently from 30df920 to ef4ecec Compare April 7, 2023 20:12

nuttycom marked this pull request as ready for review April 7, 2023 20:12

nuttycom force-pushed the bug/v_transactions_net_value branch from ef4ecec to f986f9c Compare April 7, 2023 21:24

pacu requested review from ccjernigan, pacu and str4d April 8, 2023 18:41

pacu approved these changes Apr 8, 2023

View reviewed changes

str4d requested changes Apr 10, 2023

View reviewed changes

nuttycom force-pushed the bug/v_transactions_net_value branch from 49de1e9 to fee352b Compare April 10, 2023 16:59

nuttycom commented Apr 10, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs Outdated

nuttycom commented Apr 10, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/init.rs Outdated

nuttycom force-pushed the bug/v_transactions_net_value branch 4 times, most recently from 0bf2d34 to 9f2c516 Compare April 11, 2023 00:57

nuttycom requested review from pacu and str4d April 11, 2023 00:58

nuttycom marked this pull request as draft April 11, 2023 18:49

nuttycom force-pushed the bug/v_transactions_net_value branch from 9f2c516 to 53b47de Compare April 11, 2023 19:46

nuttycom marked this pull request as ready for review April 11, 2023 19:46

nuttycom force-pushed the bug/v_transactions_net_value branch 3 times, most recently from 48e5eb1 to 33ff5ba Compare April 11, 2023 20:59

Fix double-entry accounting errors in transaction views.

1782753

nuttycom force-pushed the bug/v_transactions_net_value branch from 33ff5ba to 1782753 Compare April 11, 2023 20:59

str4d reviewed Apr 11, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/init/migrations/v_transactions_net.rs Outdated

nuttycom force-pushed the bug/v_transactions_net_value branch 2 times, most recently from ad4173c to 095551d Compare April 11, 2023 22:08

Ensure that we don't filter out sent UTXOs that collide with Sapling …

024c858

…change output indices. Co-authored-by: Jack Grigg <jack@electriccoin.co>

nuttycom force-pushed the bug/v_transactions_net_value branch from 095551d to 024c858 Compare April 11, 2023 22:21

nuttycom requested a review from str4d April 11, 2023 22:26

pacu reviewed Apr 12, 2023

View reviewed changes

pacu requested changes Apr 12, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/init.rs Outdated

Account for received utxos in v_transactions.

25b2148

Also, rename `v_tx_events` to `v_tx_outputs`.

pacu approved these changes Apr 13, 2023

View reviewed changes

Fix Rust formatting errors.

b6fd844

nuttycom force-pushed the bug/v_transactions_net_value branch 2 times, most recently from 7bb515b to 7f3aa4a Compare April 14, 2023 20:02

Adds documentation for the v_transactions and v_tx_outputs views.

7d18d1d

This change also settles on `account_value_delta` as the name of the column in `v_transactions` that describes the transaction's effect on the value of the associated account.

nuttycom force-pushed the bug/v_transactions_net_value branch from 7f3aa4a to 7d18d1d Compare April 14, 2023 20:16

str4d approved these changes Apr 14, 2023

View reviewed changes

nuttycom merged commit 93a48a9 into zcash:main Apr 14, 2023

nuttycom deleted the bug/v_transactions_net_value branch April 14, 2023 22:53

nuttycom mentioned this pull request Apr 17, 2023

Product Request: Surface additional unverified transaction states in zcash_client_sqlite #249

Closed

2 tasks

	VALUES (1, 2, 0, '', 2, '', 'nf_c', true);").unwrap();
	VALUES (1, 2, 0, '', 2, '', 'nf_c', true);").unwrap();

Conversation

nuttycom commented Apr 7, 2023

Uh oh!

pacu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pacu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pacu left a comment

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

str4d Apr 10, 2023 •

edited

Loading

nuttycom Apr 12, 2023 •

edited

Loading

nuttycom Apr 12, 2023 •

edited

Loading