zcash_client_sqlite: Use consistent criteria for detecting what accounts fund a transaction. #1306

nuttycom · 2024-03-23T18:26:44Z

nuttycom · 2024-03-23T18:31:19Z

zcash_client_sqlite/src/wallet/orchard.rs

+    let mut account_q = conn.prepare_cached(
+        "SELECT rn.account_id
+        FROM orchard_received_notes rn
+        WHERE rn.nf IN :nf_ptr",


@AArnott one question about this: should an account be detected as funding the transaction even if it's an imported account? Or should this only apply to derived accounts? Same question for Sapling and transparent.

My initial answer is "yes, of course", which makes me wonder if there's a downside that you see that I'm missing. Isn't scanning the same in only requiring the UFVK whether the account is derived or imported? I would expect downloaded or transmitted transactions to be recorded in the db exactly the same whether the account derivation information is in the db or not (as in the imported case).

For imported UFVK accounts, we definitely should do this tracking, because it's just metadata between accounts for which we have complete information. I don't believe we will ever select imported UIVK accounts here, as we don't have nullifier-deriving keys for those accounts, and thus can never populate rn.nf.

We should separately ensure that we set the "to account" correctly on the send side for any source account that we would be fetching here (to ensure symmetry in the reported data), but that's likely going to require a migration to the sent_notes table (as we currently use the account column there as a proxy for "account-internal send", which would be invalid in this context), so that does not need to happen in this PR (or even in the next release).

codecov · 2024-03-23T18:44:25Z

Codecov Report

Attention: Patch coverage is 45.26316% with 104 lines in your changes are missing coverage. Please review.

Project coverage is 63.73%. Comparing base (22f90bc) to head (1719b01).
Report is 6 commits behind head on main.

Files	Patch %	Lines
zcash_client_sqlite/src/lib.rs	28.20%	84 Missing ⚠️
zcash_client_sqlite/src/wallet/orchard.rs	0.00%	7 Missing ⚠️
zcash_client_sqlite/src/wallet/transparent.rs	45.45%	6 Missing ⚠️
zcash_client_backend/src/data_api/wallet.rs	50.00%	4 Missing ⚠️
zcash_client_backend/src/wallet.rs	84.61%	2 Missing ⚠️
zcash_client_sqlite/src/wallet.rs	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
+ Coverage   63.44%   63.73%   +0.29%     
==========================================
  Files         121      122       +1     
  Lines       14142    14258     +116     
==========================================
+ Hits         8972     9087     +115     
- Misses       5170     5171       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AArnott · 2024-03-23T19:06:24Z

I applied this change to my branch (combined with your tree panic fix), and early in sync, it failed with:

near ":nf_ptr": syntax error in SELECT rn.account_id
        FROM sapling_received_notes rn
        WHERE rn.nf IN :nf_ptr at offset 83

nuttycom · 2024-03-23T23:02:04Z

I applied this change to my branch (combined with your tree panic fix), and early in sync, it failed with:
near ":nf_ptr": syntax error in SELECT rn.account_id
        FROM sapling_received_notes rn
        WHERE rn.nf IN :nf_ptr at offset 83

Oops, fixed, thanks.

AArnott · 2024-03-23T23:24:48Z

That's much better. Everything in the sqlite table looks correct now. There is one remaining error in the balance impact calculation for the send-to-self transaction, but I believe that is in my own code.

AArnott · 2024-03-23T23:38:26Z

I just confirmed the remaining discrepancy was in my code and I have a fix for that too. Thanks for fixing this!

str4d

Reviewed 9b4e9ea. The sent_notes table migration is blocking, because the data inconsistency introduced in this PR will affect Zashi 1.0; the other comments would not because Zashi 1.0 will not support multiple accounts.

If the sent_notes table migration is not feasible on the Zashi 1.0 timeline, then for this PR to be merged as part of Zashi 1.0 the new WalletInternal parts that insert into sent_notes must be gated on the wallet only containing a single account (which ensures that the cross-account data inconsistency cannot occur). This restriction could then be lifted in a subsequent post-Zashi-1.0 crate release when the necessary migrations are introduced.

str4d · 2024-03-25T03:57:00Z

zcash_client_sqlite/src/testing/pool.rs

+    assert_matches!(
+        decrypt_and_store_transaction(&st.network(), st.wallet_mut(), &tx),
+        Ok(_)
+    );


I cannot determine what purpose this addition serves, given the lack of subsequent assertions to check the resulting funding account state. The changes in this PR to store_decrypted_transaction do not contain any new error states, only warnings; is this addition left over from debugging?

Please either document what this assertion is verifying, or add additional subsequent assertions that make relevant checks.

It's here to ensure that the view query is valid. When I have more time I will verify the return values.

Opened #1315

zcash_client_sqlite/src/lib.rs

str4d · 2024-03-25T04:20:43Z

zcash_client_sqlite/src/lib.rs

                        wallet::sapling::put_received_note(wdb.conn.0, output, tx_ref, None)?;
+
+                        if let Some(account_id) = funding_account {
+                            // Even if the recipient address is external, record the send as internal.


Blocking:

This is the exact misuse of the sent_notes account column that I mentioned earlier. The enum is named WalletInternal, but prior to now AFAICT this has always been used as "account-internal". Given that any account in the wallet can be the funding account here, "wallet-internal" is ill-defined; the two accounts may have no common controller of spend authority (e.g. not be derived from the same seed). For example, it would make no sense for a note sent from a wallet account to ZecPages to be considered a "wallet-internal" transaction just because the ZecPages full viewing key is imported.

As an additional datapoint for this column currently meaning "account-internal", if we detect a note as going to an external address, and then later call create_account such that the external address is learned to actually be a wallet address, we do not update the sent_notes table to correct the information.

If we are going to make this change to how funding accounts are tracked, we need to do so in a way that ensures data consistency. That means either writing a migration to distinguish "account-internal" from "wallet-internal" in the sent_notes table, or writing a migration to redefine the to_account_id column as "amongst the accounts derived within or imported into this wallet database" and add code to rescan the entire wallet to update relationships whenever a new account is created or imported.

The easy fix here is to restrict this check to verify that the funding account is the receiving account. When I refer to "external" here I'm simply meaning the zip32 external scope.

Wait a minute though, this is just populating the "from_account" column of the sent_notes table correctly; the to_account column should already be getting the correct data. So I don't think there is a problem here at all.

The recipient value being constructed in this fashion ensures that the to_account field gets set correctly. You're correct though that reusing the "WalletInternal" variant in this fashion is a change in semantics. It may be that we just need an additional Recipient variant, that can carry both the address (for external addresses) and the recipient account ID.

zcash_client_sqlite/src/lib.rs

str4d · 2024-03-25T04:28:33Z

zcash_client_sqlite/src/lib.rs

+                if let Some(account_id) = funding_account {
+                    if funding_accounts.len() > 1 {
+                        warn!(
+                            "More than one wallet account detected as funding transaction {:?}, selecting {:?}",


There's a slight difference in the new logic: before if a transaction spent Sapling funds from one account and Orchard funds from another, the correct data would be recorded. Now, one of the accounts will be used for both. This is probably fine for the same reason as above: a future migration could insert the other accounts after changing the tables.

daira

utACK; no additional comments beyond @str4d's. Please at least file tickets for remaining issues.

…n output is recorded as sent.

…actions. Previously, if the funding account for a received transaction output was determined to be an account known to the wallet, the output was recorded as though it were sent to an internal (change) address of the wallet.

str4d

Reviewed as of 151e6e5

str4d · 2024-03-25T16:18:51Z

zcash_client_sqlite/src/lib.rs

-                            )
+                            Recipient::InternalAccount {
+                                receiving_account: *output.account(),
+                                external_address: None,


How can we ensure this is set correctly for cross-account transfers instead of setting it to None?

This is correct because of the meaning of TransferType::Outgoing and TransferType::WalletInternal (and the corresponding TransferType::Incoming branch guarantees that the transfer was decrypted with the wallet's external IVK).

What I mean is, don't we need a separate TransferType case to distinguish between wallet-internal (cross-account) and account-internal?

Oh, I guess that for correct cross-account transfers (i.e. correctly treated as separate accounts, so that each account's external FVK sees the correct view), they should show up here as TransferType::Outgoing. In that case, we should have external_address available already for cross-account transfers and can set it here?

Ah, and now going back into the PR diff, we're currently in the TransferType::WalletInternal branch. Okay, so the change I think is missing here is that the other side of this branch should be tracking the receiving account for cross-account transfers.

Discussion from offline: the received_notes tables would then duplicate exactly information that's available from the sent_notes table, and such duplication would be a potential source of error. So this should be fine.

When we scan, we scan with all of the available IVKs, so in the case of a cross-account transfer it will always be detected as TransferType::Incoming and will get handled by that branch, which will correctly find and set the source account. And the Outgoing case is already handled correctly.

There's nothing that guarantees that (from the perspective of an implementation of the WalletWrite trait), but I agree that's what the current code does in zcash_client_backend which is what callers are most likely using, as I said in my subsequent review. It's just brittle.

zcash_client_sqlite/src/lib.rs

…ed_tx`

str4d

Reviewed 4ba7fbd

str4d · 2024-03-25T18:17:37Z

zcash_client_sqlite/src/lib.rs

+                            *output.account(),
+                            tx_ref,
+                            output.index(),
+                            &recipient,


Okay, the 3-way match now makes clear that there's an asymmetry currently between TransferType::Outgoing and TransferType::Incoming when it comes to cross-account transfers. Both will put an entry into sent_notes, but only Incoming fills in both the funding and receiving accounts; Outgoing just sets the funding account but not the receiving account.

In general I think we are "probably okay" here, because we scan with IVKs before OVKs, so in the case of a cross-account transfer we will see Incoming here instead of Outgoing. However, in the future case of actually supporting imported accounts, there will be a bug here depending on the order in which accounts are imported. wallet::put_sent_output overwrites the contents of the to_account_id column, so if the receiving account gets handled before the sending account, then the collected data will be erased.

This is not simple to recover from: we cannot fix the data inconsistency in a migration if we don't have the raw transaction (to re-decrypt), and we are not guaranteed to have that if the caller is not running enhancement. So I think there are two possible options here:

Track the receiving account in TransferType::Outgoing, as I had previously suggested.

Change wallet::put_sent_output to not overwrite known to_account_id values.

I prefer the latter solution, because in general for outgoing transfers we don't know anything about the recipient (and it would be a pain to recover that in the send flow), whereas our usual upsert functionality is to not overwrite known data with nulls.

The main issue here is that our usual upsert functionality is avoiding overwriting stable known data; account IDs are not stable. But as long as we are aware of this hazard, then it should be fine.

Within the lifetime of an a wallet database, I can't see how account IDs could ever become unstable; if you swapped them around, you'd have to update all the foreign keys everywhere and this is no exception.

…sent_output`.

str4d

utACK 1719b01. Please open issues for the remaining multi-account comments.

nuttycom mentioned this pull request Mar 23, 2024

v_tx_outputs from_account_id column left NULL for notes sent to self #1304

Closed

nuttycom commented Mar 23, 2024

View reviewed changes

nuttycom force-pushed the detect_spending_accounts branch from 9278aab to 6f876e5 Compare March 23, 2024 22:58

nuttycom mentioned this pull request Mar 24, 2024

zcash_client_sqlite: Use account birthday subtree sizes for progress #1307

Merged

nuttycom force-pushed the detect_spending_accounts branch from 6f876e5 to 9b4e9ea Compare March 24, 2024 23:00

nuttycom added this to the Librustzcash Zashi 1.0 milestone Mar 24, 2024

str4d requested changes Mar 25, 2024

View reviewed changes

daira previously approved these changes Mar 25, 2024

View reviewed changes

nuttycom dismissed daira’s stale review via ab1f9a2 March 25, 2024 13:55

nuttycom added 3 commits March 25, 2024 07:59

zcash_client_sqlite: Add get_funding_accounts method.

7e7dba8

zcash_client_sqlite: Use get_funding_account to determine whether a…

404132b

…n output is recorded as sent.

nuttycom force-pushed the detect_spending_accounts branch from ab1f9a2 to 151e6e5 Compare March 25, 2024 14:18

nuttycom requested a review from str4d March 25, 2024 14:26

str4d reviewed Mar 25, 2024

View reviewed changes

nuttycom force-pushed the detect_spending_accounts branch from 531d21b to de0c7a2 Compare March 25, 2024 17:54

zcash_client_sqlite: Clarify TransferType matches in `store_decrypt…

4ba7fbd

…ed_tx`

nuttycom force-pushed the detect_spending_accounts branch from de0c7a2 to 4ba7fbd Compare March 25, 2024 17:55

nuttycom requested a review from str4d March 25, 2024 18:14

str4d requested changes Mar 25, 2024

View reviewed changes

nuttycom requested a review from str4d March 25, 2024 18:37

zcash_client_sqlite: Don't overwrite account_id with nulls in `put_…

1719b01

…sent_output`.

str4d approved these changes Mar 25, 2024

View reviewed changes

nuttycom mentioned this pull request Mar 25, 2024

zcash_client_{backend,sqlite}: Correctly track accounts that fund each transaction output. #1305

Open

nuttycom merged commit 895e8e3 into main Mar 25, 2024
25 of 26 checks passed

nuttycom deleted the detect_spending_accounts branch March 25, 2024 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zcash_client_sqlite: Use consistent criteria for detecting what accounts fund a transaction. #1306

zcash_client_sqlite: Use consistent criteria for detecting what accounts fund a transaction. #1306

nuttycom commented Mar 23, 2024 •

edited

Loading

nuttycom Mar 23, 2024

AArnott Mar 23, 2024

str4d Mar 25, 2024

codecov bot commented Mar 23, 2024 •

edited

Loading

AArnott commented Mar 23, 2024

nuttycom commented Mar 23, 2024

AArnott commented Mar 23, 2024

AArnott commented Mar 23, 2024

str4d left a comment •

edited

Loading

str4d Mar 25, 2024

nuttycom Mar 25, 2024

nuttycom Mar 25, 2024

str4d Mar 25, 2024

nuttycom Mar 25, 2024 •

edited

Loading

nuttycom Mar 25, 2024

nuttycom Mar 25, 2024

str4d Mar 25, 2024

daira left a comment

str4d left a comment

str4d Mar 25, 2024

nuttycom Mar 25, 2024

str4d Mar 25, 2024

str4d Mar 25, 2024

str4d Mar 25, 2024

nuttycom Mar 25, 2024

nuttycom Mar 25, 2024

str4d Mar 25, 2024

str4d left a comment

str4d Mar 25, 2024

nuttycom Mar 25, 2024

str4d Mar 25, 2024

nuttycom Mar 25, 2024

str4d left a comment

zcash_client_sqlite: Use consistent criteria for detecting what accounts fund a transaction. #1306

zcash_client_sqlite: Use consistent criteria for detecting what accounts fund a transaction. #1306

Conversation

nuttycom commented Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2024 • edited Loading

Codecov Report

AArnott commented Mar 23, 2024

nuttycom commented Mar 23, 2024

AArnott commented Mar 23, 2024

AArnott commented Mar 23, 2024

str4d left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

nuttycom commented Mar 23, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading

str4d left a comment •

edited

Loading

nuttycom Mar 25, 2024 •

edited

Loading