zcash_client_sqlite: Align handling of transparent UTXOs with that of shielded notes. #1402

nuttycom · 2024-05-31T20:01:36Z

This PR is best reviewed hiding whitespace changes.

codecov · 2024-05-31T20:20:02Z

Codecov Report

Attention: Patch coverage is 85.57692% with 30 lines in your changes missing coverage. Please review.

Project coverage is 63.20%. Comparing base (26c6b82) to head (9716617).

Files	Patch %	Lines
zcash_client_sqlite/src/wallet/transparent.rs	86.62%	21 Missing ⚠️
...sqlite/src/wallet/init/migrations/utxos_to_txos.rs	60.00%	4 Missing ⚠️
zcash_client_sqlite/src/wallet.rs	81.25%	3 Missing ⚠️
zcash_client_backend/src/data_api.rs	0.00%	1 Missing ⚠️
zcash_client_sqlite/src/lib.rs	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1402      +/-   ##
==========================================
+ Coverage   63.13%   63.20%   +0.06%     
==========================================
  Files         127      128       +1     
  Lines       14864    14869       +5     
==========================================
+ Hits         9385     9398      +13     
+ Misses       5479     5471       -8

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

zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs

zcash_client_sqlite/src/wallet.rs

zcash_client_sqlite/src/wallet/init.rs

zcash_client_sqlite/src/wallet.rs

zcash_client_sqlite/src/wallet/transparent.rs

zcash_client_sqlite/src/wallet/init.rs

zcash_client_sqlite/src/wallet/init/migrations/utxos_to_txos.rs

…`wallet::transparent` module.

… shielded notes. Co-authored-by: Daira-Emma Hopwood <[email protected]> Co-authored-by: Jack Grigg <[email protected]>

daira · 2024-06-24T12:57:22Z

zcash_client_sqlite/src/lib.rs

@@ -525,7 +523,12 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
        account: AccountId,
        max_height: BlockHeight,
    ) -> Result<HashMap<TransparentAddress, NonNegativeAmount>, Self::Error> {
-        wallet::get_transparent_balances(self.conn.borrow(), &self.params, account, max_height)
+        wallet::transparent::get_transparent_address_balances(


Why is this renaming the function in wallet::transparent so that it doesn't match the trait method? I think that for all of the other functions there that correspond to trait methods, the names do match.

(Currently I'm renaming it back in #1257, because I hadn't previously noticed that the change was made here.)

When moving the methods, I got confused about the meaning of get_transparent_balances vs get_transparent_account_balance and I wanted to clarify. I didn't want to change the top-level trait method, but since this is crate-private I thought that it was a good start.

Do you want me to undo renaming it back?

I don't feel strongly about it.

zcash_client_sqlite/src/wallet.rs

daira · 2024-06-24T16:44:53Z

zcash_client_sqlite/src/wallet/db.rs

 /// Originally this table only stored the current UTXO set (as of latest refresh), and the
 /// table was cleared prior to loading in the latest UTXO set. We now upsert instead of
 /// insert into the database, meaning that spent outputs are left in the database. This
-/// makes it similar to the `*_received_notes` tables in that it can store history, but
-/// has several downsides:
-/// - The table has incomplete contents for recovered-from-seed wallets.
-/// - The table can have inconsistent contents for seeds loaded into multiple wallets
+/// makes it similar to the `*_received_notes` tables in that it can store history.


"Originally this table, when it was called utxos, ..." (Non-blocking.)

zcash_client_sqlite/src/wallet/db.rs

daira

utACK

str4d

utACK 10258b8 with nit about tracing spans. I did not review the codebase outside of what is already changed in this PR to look for other places in e.g. the SQL queries that might need to be altered.

zcash_client_sqlite/src/wallet/transparent.rs

str4d · 2024-06-24T17:36:04Z

zcash_client_sqlite/src/wallet/transparent.rs

+         WHERE (
+            t.mined_height < :mempool_height -- tx is mined
+            OR t.expiry_height = 0 -- tx will not expire
+            OR t.expiry_height >= :mempool_height
+         )


Observation: The other minconf-0 instances of this pattern use t.mined_height <= SOME_HEIGHT OR t.expiry_height >= SOME_HEIGHT. I think it's okay to use < :mempool_height here however, solely because we never actually encounter the off-by-one here (because we encode mempool heights in the database as NULL).

zcash_client_sqlite/src/wallet/transparent.rs

str4d

Re-utACK 5c8f585

nuttycom · 2024-06-24T18:09:16Z

zcash_client_sqlite/src/wallet/db.rs

+       ro.account_id                AS to_account_id,
+       sent_notes.to_address        AS to_address,
+       sent_notes.value             AS value,
+       0                            AS is_change,


It just occurs to me, will this be correct behavior after ZIP 320 integration?

I don't see why not, but we should test it. Note that put_transparent_output is called from store_sent_tx for ephemeral outputs.

Co-authored-by: Jack Grigg <[email protected]> Co-authored-by: Daira-Emma Hopwood <[email protected]>

str4d

utACK 9716617

daira reviewed Jun 6, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs Outdated Show resolved Hide resolved

nuttycom commented Jun 11, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

nuttycom force-pushed the transparent_txos_migration branch 4 times, most recently from 77fe74d to baa2ad3 Compare June 11, 2024 19:59

nuttycom marked this pull request as draft June 11, 2024 19:59

nuttycom force-pushed the transparent_txos_migration branch 5 times, most recently from acedca8 to bf86c08 Compare June 12, 2024 17:22

daira reviewed Jun 12, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved

daira reviewed Jun 12, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved

daira reviewed Jun 12, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved

daira reviewed Jun 12, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved

nuttycom force-pushed the transparent_txos_migration branch from bf86c08 to 648df30 Compare June 14, 2024 16:45

nuttycom marked this pull request as ready for review June 14, 2024 16:45

daira reviewed Jun 14, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved

nuttycom mentioned this pull request Jun 14, 2024

Add support for transparent-source-only (TEX) addresses #1257

Merged

daira reviewed Jun 15, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

daira reviewed Jun 15, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

daira reviewed Jun 15, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

daira reviewed Jun 17, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/transparent.rs Outdated Show resolved Hide resolved

daira mentioned this pull request Jun 17, 2024

zcash_client_sqlite wallet will never expire outputs from a tx that has no expiry height, even if it cannot be mined ("conflicted" in zcashd terminology) #1426

Open

nuttycom force-pushed the transparent_txos_migration branch from ae834bc to 0eb0b3f Compare June 17, 2024 17:50

daira reviewed Jun 18, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved

daira reviewed Jun 18, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init/migrations/utxos_to_txos.rs Outdated Show resolved Hide resolved

nuttycom force-pushed the transparent_txos_migration branch from 0eb0b3f to ddbe993 Compare June 18, 2024 21:01

nuttycom force-pushed the transparent_txos_migration branch from 3e28b19 to 4ba438c Compare June 22, 2024 00:49

nuttycom and others added 3 commits June 21, 2024 19:13

zcash_client_sqlite: Move transparent-inputs wallet methods into the …

d92bf27

…`wallet::transparent` module.

zcash_client_sqlite: Align handling of transparent UTXOs with that of…

72d8df8

… shielded notes. Co-authored-by: Daira-Emma Hopwood <[email protected]> Co-authored-by: Jack Grigg <[email protected]>

zcash_client_sqlite: Address comments from code review.

10258b8

nuttycom mentioned this pull request Jun 22, 2024

Ensure symmetry of transparent output handling between put_blocks, store_decrypted_tx and store_sent_tx #1434

Closed

nuttycom force-pushed the transparent_txos_migration branch from 4ba438c to 10258b8 Compare June 22, 2024 01:19

daira reviewed Jun 24, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

daira mentioned this pull request Jun 24, 2024

Don't create new WalletDb wrappers outside WalletDb::transactionally: take &mut WalletDb<SqlTransaction<'_>, P> as argument instead #1435

Open

nuttycom requested review from str4d and daira June 24, 2024 16:13

daira reviewed Jun 24, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/db.rs Show resolved Hide resolved

daira previously approved these changes Jun 24, 2024

View reviewed changes

str4d previously approved these changes Jun 24, 2024

View reviewed changes

nuttycom dismissed stale reviews from str4d and daira via 5c8f585 June 24, 2024 17:40

str4d previously approved these changes Jun 24, 2024

View reviewed changes

nuttycom commented Jun 24, 2024

View reviewed changes

Apply suggestions from code review

9716617

Co-authored-by: Jack Grigg <[email protected]> Co-authored-by: Daira-Emma Hopwood <[email protected]>

nuttycom dismissed str4d’s stale review via 9716617 June 24, 2024 20:51

nuttycom force-pushed the transparent_txos_migration branch from 5c8f585 to 9716617 Compare June 24, 2024 20:51

str4d approved these changes Jun 25, 2024

View reviewed changes

nuttycom merged commit 5bd911f into zcash:main Jun 25, 2024
27 checks passed

nuttycom deleted the transparent_txos_migration branch June 25, 2024 02:34

str4d mentioned this pull request Jun 30, 2024

zcash_client_sqlite: Add migration to link utxo table to transactions #503

Closed

str4d mentioned this pull request Jul 31, 2024

zcash_client_sqlite: v_tx_outputs describes one of two tx outputs as not from the account #1330

Open

nuttycom mentioned this pull request Aug 12, 2024

zcash_client_backend: Ensure that enhancement of tx containing received transparent UTXOs does not lead to wallet database inconsistency. #1029

Closed

nuttycom mentioned this pull request Sep 4, 2024

zcash_client_backend: Add support for full P2PKH history tracking #1379

Closed

nuttycom mentioned this pull request Oct 24, 2024

sent_notes table is not fully populated after a recovery from seed #936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zcash_client_sqlite: Align handling of transparent UTXOs with that of shielded notes. #1402

zcash_client_sqlite: Align handling of transparent UTXOs with that of shielded notes. #1402

nuttycom commented May 31, 2024 •

edited

Loading

codecov bot commented May 31, 2024 •

edited

Loading

daira Jun 24, 2024 •

edited

Loading

nuttycom Jun 24, 2024

daira Jun 24, 2024

nuttycom Jun 24, 2024

daira Jun 24, 2024 •

edited

Loading

daira left a comment

str4d left a comment

str4d Jun 24, 2024

str4d left a comment

nuttycom Jun 24, 2024

daira Jun 24, 2024

str4d left a comment

zcash_client_sqlite: Align handling of transparent UTXOs with that of shielded notes. #1402

zcash_client_sqlite: Align handling of transparent UTXOs with that of shielded notes. #1402

Conversation

nuttycom commented May 31, 2024 • edited Loading

codecov bot commented May 31, 2024 • edited Loading

Codecov Report

daira Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

nuttycom Jun 24, 2024

Choose a reason for hiding this comment

daira Jun 24, 2024

Choose a reason for hiding this comment

nuttycom Jun 24, 2024

Choose a reason for hiding this comment

daira Jun 24, 2024 • edited Loading

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

str4d Jun 24, 2024

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

nuttycom Jun 24, 2024

Choose a reason for hiding this comment

daira Jun 24, 2024

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

nuttycom commented May 31, 2024 •

edited

Loading

codecov bot commented May 31, 2024 •

edited

Loading

daira Jun 24, 2024 •

edited

Loading

daira Jun 24, 2024 •

edited

Loading