zcash_client_backend: Add account birthday management to the Data Access API #907

nuttycom · 2023-08-22T14:45:17Z

Closes #897.

codecov · 2023-08-22T15:28:43Z

Codecov Report

Patch coverage is 66.97% of modified lines.

❗ Current head b845947 differs from pull request most recent head 5b3f544. Consider uploading reports for the commit 5b3f544 to get more accurate results

Files Changed	Coverage
zcash_client_backend/src/proto.rs	`0.00%`
zcash_client_sqlite/src/chain.rs	`ø`
zcash_client_sqlite/src/error.rs	`0.00%`
zcash_client_sqlite/src/wallet/init.rs	`ø`
zcash_client_sqlite/src/lib.rs	`20.00%`
zcash_client_backend/src/data_api.rs	`31.57%`
zcash_client_sqlite/src/wallet/sapling.rs	`36.36%`
zcash_client_sqlite/src/wallet/scanning.rs	`70.42%`
zcash_client_sqlite/src/wallet.rs	`75.00%`
...rc/wallet/init/migrations/add_account_birthdays.rs	`85.29%`
... and 3 more

📢 Thoughts on this report? Let us know!.

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

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

zcash_client_sqlite/src/wallet.rs

HonzaR

From the Android SDK consumer view, it looks good. Thank you! The only question I have is that I see both init_blocks_table and init_accounts_table are replaced by create_account, which expects AccountBirthday, which is optional. Can you please describe what we should use in case of new or restored wallets for this parameter? Or should we leave it empty?

nuttycom · 2023-08-28T14:44:18Z

From the Android SDK consumer view, it looks good. Thank you! The only question I have is that I see both init_blocks_table and init_accounts_table are replaced by create_account, which expects AccountBirthday, which is optional. Can you please describe what we should use in case of new or restored wallets for this parameter? Or should we leave it empty?

There are two options:
(1) is to first call updateChainTip and then call this without specifying an AccountBirthday.
(2) call this with the arguments that you would have previously provided to init_blocks_table (block height and note commitment tree frontier at the chain tip).

In the Zashi wallet, you will usually want option (2) because the wallet only supports a single account. In the future, though, the SDK may support the creation of multiple accounts, and also we'll have to figure out something for when the wallets add Orchard support; I'm not sure whether we'll want to handle that by creating a new account, or by creating separate per-protocol birthdays.

str4d

Flushing my review for the first two commits (up to 12ddc7d).

zcash_client_sqlite/src/wallet/scanning.rs

zcash_client_backend/src/data_api.rs

zcash_client_sqlite/CHANGELOG.md

zcash_client_sqlite/src/chain.rs

str4d · 2023-08-29T22:02:48Z

zcash_client_sqlite/src/wallet/init.rs

-                priority_code(&ScanPriority::Scanned),
-            ),
+            "CREATE VIEW v_sapling_shard_unscanned_ranges AS
+            WITH wallet_birthday AS (SELECT MIN(birthday_height) AS height FROM accounts)


After the change to AccountBirthday, we are effectively changing the definition of the wallet_birthday column (to match its semantics for new accounts, i.e. last height we don't need to scan). Check that this works for the view, and adjust if necessary.

zcash_client_sqlite/src/wallet/init.rs

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

…te_account` This also removes the zcash_client_sqlite-specific database initialization procedures in favor of a standardized approach using the methods available via the data access API.

…ble via the data access API

…t in note selection

zcash_client_backend/CHANGELOG.md

zcash_client_backend/src/data_api.rs

str4d

Re-utACK 3a1d4a4

zcash_client_backend/src/data_api.rs

zcash_client_sqlite/CHANGELOG.md

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

str4d

Re-utACK 5b3f544.

daira

Flushing comments.

zcash_client_sqlite/src/error.rs

zcash_client_sqlite/src/wallet.rs

daira · 2023-09-01T17:29:07Z

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

+            u32::from(
+                self.params
+                    .activation_height(NetworkUpgrade::Sapling)
+                    .unwrap()
+            )


I know this is guaranteed to be an integer, nevertheless I would have used a named parameter.

I did this mostly for the sake of using execute_batch; as far as I know that is the only mechanism available for executing multiple SQL statements in a single call.

daira · 2023-09-01T17:43:43Z

zcash_client_sqlite/src/wallet/sapling.rs

@@ -186,6 +198,15 @@ pub(crate) fn select_spendable_sapling_notes(
    anchor_height: BlockHeight,
    exclude: &[ReceivedNoteId],
 ) -> Result<Vec<ReceivedSaplingNote<ReceivedNoteId>>, SqliteClientError> {


The partially duplicated code between this and get_spendable_sapling_notes seems like a maintenance hazard. I see that the SQL statement is different and has an extra :target_value parameter; can the rest of these methods be abstracted out?

The fact that we have two separate such methods is a historical artifact. We should clean it up some day.

zcash_client_sqlite/src/wallet/scanning.rs

daira

Post-hoc ACK with comments.

Address unresovled code review comments from #907

nuttycom force-pushed the sbs/wallet_birthday branch 4 times, most recently from e04f345 to c2b1e41 Compare August 23, 2023 18:41

nuttycom commented Aug 23, 2023

View reviewed changes

zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs Show resolved Hide resolved

nuttycom commented Aug 23, 2023

View reviewed changes

zcash_client_sqlite/src/wallet/init/migrations/v_sapling_shard_unscanned_ranges.rs Show resolved Hide resolved

nuttycom force-pushed the sbs/wallet_birthday branch 4 times, most recently from 5811b8c to 578dc57 Compare August 24, 2023 17:55

nuttycom changed the title ~~WIP: Add account birthdays.~~ zcash_client_backend: Add account birthday management to the Data Access API Aug 24, 2023

nuttycom marked this pull request as ready for review August 24, 2023 17:57

nuttycom requested a review from str4d August 24, 2023 19:21

nuttycom commented Aug 24, 2023

View reviewed changes

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

nuttycom force-pushed the sbs/wallet_birthday branch from 578dc57 to 9163d7d Compare August 24, 2023 20:56

This was referenced Aug 24, 2023

Anchor height not available; scan required #904

Closed

Too wide suggested ranges for Testnet #897

Closed

Add get_wallet_summary to WalletRead #914

Merged

nuttycom force-pushed the sbs/wallet_birthday branch 2 times, most recently from 3ee4507 to 4f8eab8 Compare August 25, 2023 23:03

HonzaR reviewed Aug 26, 2023

View reviewed changes

nuttycom added this to the Spend-before-Sync milestone Aug 28, 2023

nuttycom force-pushed the sbs/wallet_birthday branch 2 times, most recently from 588f5c9 to 1c00f75 Compare August 29, 2023 02:26

str4d reviewed Aug 29, 2023

View reviewed changes

nuttycom mentioned this pull request Aug 29, 2023

Test refactorings in preparation for #907 #915

Merged

nuttycom marked this pull request as draft August 29, 2023 23:28

nuttycom force-pushed the sbs/wallet_birthday branch from 1c00f75 to cf9cd72 Compare August 29, 2023 23:32

nuttycom added 3 commits September 1, 2023 10:08

zcash_client_backend: Accept account birthday as a parameter to `crea…

459dc49

…te_account` This also removes the zcash_client_sqlite-specific database initialization procedures in favor of a standardized approach using the methods available via the data access API.

zcash_client_backend: Make wallet and account birthday heights availa…

0b0274c

…ble via the data access API

zcash_client_sqlite: ignore unscanned ranges below the birthday heigh…

a517128

…t in note selection

nuttycom force-pushed the sbs/wallet_birthday branch from bc94a63 to 3a1d4a4 Compare September 1, 2023 16:20

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_backend/CHANGELOG.md Show resolved Hide resolved

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_backend/src/data_api.rs Outdated Show resolved Hide resolved

str4d previously approved these changes Sep 1, 2023

View reviewed changes

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_backend/src/data_api.rs Outdated Show resolved Hide resolved

nuttycom dismissed str4d’s stale review via 363dea9 September 1, 2023 16:25

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_backend/src/data_api.rs Show resolved Hide resolved

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_backend/src/data_api.rs Outdated Show resolved Hide resolved

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_sqlite/CHANGELOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

5b3f544

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

nuttycom force-pushed the sbs/wallet_birthday branch from 727cddd to 5b3f544 Compare September 1, 2023 16:38

str4d approved these changes Sep 1, 2023

View reviewed changes

str4d merged commit 229f6e8 into zcash:main Sep 1, 2023
8 of 10 checks passed

nuttycom deleted the sbs/wallet_birthday branch September 1, 2023 17:11

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_sqlite/src/error.rs Show resolved Hide resolved

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

daira reviewed Sep 1, 2023

View reviewed changes

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

daira reviewed Sep 1, 2023

View reviewed changes

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

daira reviewed Sep 1, 2023

View reviewed changes

daira mentioned this pull request Sep 2, 2023

Fix update_chain_tip bugs #932

Merged

nuttycom mentioned this pull request Sep 5, 2023

zcash_client_backend: Add an "account birthday" parameter to the create_account API. #901

Closed

nuttycom added a commit to nuttycom/librustzcash that referenced this pull request Sep 5, 2023

Address unresovled code review comments from zcash#907

440e0e9

nuttycom added a commit to nuttycom/librustzcash that referenced this pull request Sep 5, 2023

Address unresovled code review comments from zcash#907

dd60f51

nuttycom added a commit that referenced this pull request Sep 5, 2023

Merge pull request #935 from nuttycom/sbs/post_907_cleanup

6478113

Address unresovled code review comments from #907

nuttycom mentioned this pull request Sep 6, 2023

Send fails: Tree does not contain a root at address #898

Closed

str4d mentioned this pull request Nov 7, 2023

Create a Dependency that can load checkpoints from bundle Electric-Coin-Company/zcash-swift-wallet-sdk#1316

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zcash_client_backend: Add account birthday management to the Data Access API #907

zcash_client_backend: Add account birthday management to the Data Access API #907

nuttycom commented Aug 22, 2023 •

edited by str4d

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

HonzaR left a comment

nuttycom commented Aug 28, 2023 •

edited

Loading

str4d left a comment

str4d Aug 29, 2023

str4d left a comment

str4d left a comment

daira left a comment

daira Sep 1, 2023

nuttycom Sep 1, 2023

daira Sep 1, 2023

nuttycom Sep 1, 2023

daira left a comment

zcash_client_backend: Add account birthday management to the Data Access API #907

zcash_client_backend: Add account birthday management to the Data Access API #907

Conversation

nuttycom commented Aug 22, 2023 • edited by str4d Loading

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

HonzaR left a comment

Choose a reason for hiding this comment

nuttycom commented Aug 28, 2023 • edited Loading

str4d left a comment

Choose a reason for hiding this comment

str4d Aug 29, 2023

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

daira Sep 1, 2023

Choose a reason for hiding this comment

nuttycom Sep 1, 2023

Choose a reason for hiding this comment

daira Sep 1, 2023

Choose a reason for hiding this comment

nuttycom Sep 1, 2023

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

nuttycom commented Aug 22, 2023 •

edited by str4d

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

nuttycom commented Aug 28, 2023 •

edited

Loading