Give zcash_client_sqlite its own account ID #1175

AArnott · 2024-02-08T18:39:04Z

This is work toward supporting arbitrary imported keys, which may not be based on seeds (as far as we know) and may consist only of a full viewing key or incoming viewing key.

Closes #1218.

Additional work

New tests?
Bring in UnifiedIncomingViewingKey as a type and then a UnifiedViewingKey enum that will allow code to start preparing for the possibility of UIVKs being in the accounts table instead of it always being UFVKs.
Update the CHANGELOGs
Are there other SQL queries for which no automated test exists and therefore may still need to be discovered and updated given the new db schema?
Either update the ASCII art graph in migrations.rs or merge in Use mermaid for migrations graph #1186 and then update the migrations.md file.

Interesting things of note

Several tests needed to be updated because they assumed the result of create_account was a ZIP-32 account index where it was really only documented to return an account id or that the first account id would be 0. In fact in the sqlite implementation these two values are no longer the same. I've updated the docs and CHANGELOG to call out this behavior.
In addition to adding a few columns to the accounts table, I also renamed a couple of existing columns to avoid their being used with their old semantics. I also added indexes for the FK columns of each table that had to be migrated.
The add_utxo_account migration was improperly relying on internal crate code to query the database as part of migration. The problem with this is the internal crate code assumes the latest db schema, whereas the migration must be able to run with an earlier schema. With this PR, a later migration occurs that breaks the schema in ways that required internal crate updates, and a migration that runs before those updates then failed when calling into the 'newer' crate code. The fix is to remove the dependency: migrations should never make indirect sql calls through non-migration code. I copied the two functions from the regular code into the migration and reverted the changes I had made to them so that the migration works again. This code in the migration may be trimmable to something smaller to fit what the migration needs, but I didn't want to rewrite it.
The new db schema renames the accounts.ufvk column to uvk to allow for UFVKs today, and UIVKs tomorrow. Code remains in the crate itself that only tries to parse the value of this column as a UFVK because no UIVK type exists yet. But I've tried to model the APIs to allow for adding support for that with no further breaking changes.

This contributes toward #1075.

zcash_client_backend/src/data_api.rs

zcash_client_backend/src/scanning.rs

codecov · 2024-02-12T14:32:36Z

Codecov Report

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

Project coverage is 64.20%. Comparing base (8130359) to head (cc6e715).
Report is 2 commits behind head on main.

Files	Patch %	Lines
zcash_client_sqlite/src/wallet.rs	72.25%	48 Missing ⚠️
...ite/src/wallet/init/migrations/add_utxo_account.rs	70.96%	18 Missing ⚠️
...ite/src/wallet/init/migrations/full_account_ids.rs	86.84%	10 Missing ⚠️
zcash_client_sqlite/src/error.rs	0.00%	3 Missing ⚠️
zcash_client_sqlite/src/lib.rs	93.33%	2 Missing ⚠️
zcash_client_sqlite/src/wallet/sapling.rs	66.66%	2 Missing ⚠️
...lite/src/wallet/init/migrations/addresses_table.rs	91.66%	1 Missing ⚠️
zcash_keys/src/keys.rs	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
+ Coverage   63.89%   64.20%   +0.30%     
==========================================
  Files         115      116       +1     
  Lines       11930    12194     +264     
==========================================
+ Hits         7623     7829     +206     
- Misses       4307     4365      +58

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

zcash_client_backend/src/data_api/chain.rs

zcash_client_backend/src/data_api/error.rs

zcash_client_backend/src/decrypt.rs

zcash_client_backend/src/scanning.rs

zcash_client_backend/src/wallet.rs

zcash_client_sqlite/src/lib.rs

AArnott · 2024-02-14T01:39:25Z

I'll have to do a rebase to deal with the rebase in #1185. Assuming your policy is to merge PRs (which I like), I'll rebase not only to resolve the stale merge but to clean up my commits here too, many of which have junk messages. I'll probably just make one commit of all of them as that'll be simpler. Any concerns?

This is work toward supporting arbitrary imported keys, which may not be based on seeds (as far as we know) and may consist only of a full viewing key or incoming viewing key. - Several tests needed to be updated because they assumed the result of `create_account` was a ZIP-32 account index where it was really only documented to return an account id or that the first account id would be 0. In fact in the sqlite implementation these two values are no longer the same. I've updated the docs and CHANGELOG to call out this behavior. - In addition to adding a few columns to the `accounts` table, I also *renamed* a couple of existing columns to avoid their being used with their old semantics. I also added indexes for the FK columns of each table that had to be migrated. - The `add_utxo_account` migration was improperly relying on internal crate code to query the database as part of migration. The problem with this is the internal crate code assumes the _latest_ db schema, whereas the migration must be able to run with an earlier schema. With this PR, a later migration occurs that breaks the schema in ways that required internal crate updates, and a migration that runs _before_ those updates then failed when calling into the 'newer' crate code. The fix is to remove the dependency: migrations should never make indirect sql calls through non-migration code. I copied the two functions from the regular code into the migration and reverted the changes I had made to them so that the migration works again. This code in the migration may be trimmable to something smaller to fit what the migration needs, but I didn't want to rewrite it. - The new db schema renames the `accounts.ufvk` column to `uvk` to allow for UFVKs today, and UIVKs tomorrow. Code remains in the crate itself that only tries to parse the value of this column as a UFVK because no UIVK type exists yet. But I've tried to model the APIs to allow for adding support for that with no further breaking changes.

AArnott · 2024-02-14T04:22:30Z

I've pushed the rebase.

zcash_client_backend/src/data_api.rs

nuttycom

My review isn't yet complete, but I'm flushing comments now as what I've requested thus far is a good chunk of work.

zcash_client_backend/src/data_api.rs

zcash_client_backend/src/wallet.rs

zcash_client_sqlite/src/wallet/init.rs

zcash_client_sqlite/CHANGELOG.md

zcash_client_sqlite/src/wallet.rs

zcash_client_sqlite/src/wallet/init.rs

str4d · 2024-03-07T17:07:22Z

I'll have to do a rebase to deal with the rebase in #1185. Assuming your policy is to merge PRs (which I like), I'll rebase not only to resolve the stale merge but to clean up my commits here too, many of which have junk messages. I'll probably just make one commit of all of them as that'll be simpler. Any concerns?

Yes, this makes sense to do. However, for reviewability it helps if you are able to separate the rebase (and merge conflict resolution) force push, from the force push that adjusts commit history. (I post comments like #1173 (comment) and #1135 (comment) when doing so, to help reviewers keep track.)

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

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

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

nuttycom · 2024-03-07T21:31:45Z

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

@@ -64,9 +62,8 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {



@daira the entire migration takes place inside a single transaction.

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

zcash_keys/src/keys.rs

daira

utACK modulo comments.

- Also verify that the migration-provided seed matches the ufvk's already in the table.

str4d

Reviewed as of cc6e715.

zcash_client_sqlite/src/wallet.rs

str4d · 2024-03-08T18:01:24Z

zcash_client_sqlite/src/wallet.rs

+        ivks.push(Ivk::P2pkh(transparent.serialize().try_into().map_err(
+            |_| {
+                SqliteClientError::BadAccountData(
+                    "Unable to serialize transparent IVK.".to_string(),
+                )
+            },
+        )?));


Bug: This is serializing the transparent FVK, not the external transparent IVK.

It's funny you bring this up, because in an earlier iteration I had the code first get the external transparent IVK. But that caused tests to fail when they took the UIVK's transparent component and repeated the external IVK derivation. Maybe I 'fixed' the test in the wrong place. I don't think of transparent keys offering an IVK option, so I just stored the whole public key in there.
But now that you mention it, I look at ZIP-316 and find this:

For a Transparent P2PKH FVK, the corresponding Transparent P2PKH IVK is obtained by deriving the child key with non-hardened index 0 as described in 32.

That seems to verify your expectation. I'll make the change.

str4d · 2024-03-08T18:10:34Z

zcash_client_sqlite/src/wallet.rs

    })
 }

+pub(crate) fn ufvk_to_uivk<P: consensus::Parameters>(


This method should either be documented as always producing the external UIVK, or take a zip32::Scope argument. Probably the former, given the apparent invariant that if ufvk != NULL, then uivk MUST be the external UIVK.

str4d · 2024-03-08T18:15:13Z

zcash_client_sqlite/src/wallet.rs

+        for item in uivk.items() {
+            if let Ivk::P2pkh(ivk) = item {
+                let pubkey = AccountPubKey::deserialize(&ivk)?;
+                return Ok(Some(pubkey.derive_external_ivk()?.default_address()));


Bug: the UIVK contains the transparent external IVK. This is related to the earlier bug; for UFVKs this line "cancels out" the earlier bug, but for imported UIVKs this will derive the wrong address. The old code that this is replacing used a UFVK here, so this derivation path was correct (only) in that context.

str4d · 2024-03-08T18:32:48Z

zcash_client_sqlite/src/wallet.rs

+            let ufvk = if let Some(ufvk_str) = ufvk_str {
+                Some(
+                    UnifiedFullViewingKey::decode(&db.params, &ufvk_str[..])
+                        .map_err(SqliteClientError::BadAccountData)?,
+                )
+            } else {
+                None
+            };


Rust note: this is more clean and less brittle as:

Suggested change

let ufvk = if let Some(ufvk_str) = ufvk_str {

Some(

UnifiedFullViewingKey::decode(&db.params, &ufvk_str[..])

.map_err(SqliteClientError::BadAccountData)?,

)

} else {

None

};

let ufvk = ufvk_str.map(|ufvk_str| {

UnifiedFullViewingKey::decode(&db.params, &ufvk_str[..])

.map_err(SqliteClientError::BadAccountData)?,

});

str4d · 2024-03-08T18:59:25Z

I've opened #1235 which is a squashed version of this PR. I will fix the bugs in that PR (as I cannot push to this PR).

nuttycom

Flushing comments.

nuttycom · 2024-03-08T16:30:14Z

zcash_client_sqlite/src/wallet.rs

+    params: &P,
+) -> Result<String, SqliteClientError> {
+    let mut ivks: Vec<Ivk> = Vec::new();
+    if let Some(orchard) = ufvk.orchard() {


This will need to be behind #[cfg(feature = "orchard")]

This is not currently necessary because we always enable zcash_keys/orchard for backwards-compatibility. Once we finish Orchard support in zcash_client_sqlite it will become necessary, but for now it's fine.

WIP: abstract account ID

1064be1

nuttycom reviewed Feb 8, 2024

View reviewed changes

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

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

AArnott force-pushed the diverse_accounts branch from b5e03f1 to 059f84b Compare February 10, 2024 14:27

Merge remote-tracking branch 'upstream/main' into diverse_accounts

9956662

AArnott force-pushed the diverse_accounts branch 3 times, most recently from bed9551 to c06baed Compare February 11, 2024 22:00

AArnott marked this pull request as ready for review February 12, 2024 16:02

nuttycom reviewed Feb 12, 2024

View reviewed changes

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

nuttycom reviewed Feb 12, 2024

View reviewed changes

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

nuttycom reviewed Feb 12, 2024

View reviewed changes

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

nuttycom reviewed Feb 12, 2024

View reviewed changes

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

nuttycom reviewed Feb 12, 2024

View reviewed changes

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

nuttycom reviewed Feb 12, 2024

View reviewed changes

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

AArnott mentioned this pull request Feb 14, 2024

Declare and use NonHardenedChildIndex for transparent #1185

Merged

AArnott added 2 commits February 13, 2024 21:18

Merge branch 'nonhardenedchildindex' into diverse_accounts

e6e40ee

AArnott force-pushed the diverse_accounts branch from a3afd81 to 862efdb Compare February 14, 2024 04:21

Merge remote-tracking branch 'upstream/main' into diverse_accounts

3b7d670

nuttycom reviewed Feb 15, 2024

View reviewed changes

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

nuttycom reviewed Feb 15, 2024

View reviewed changes

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

nuttycom reviewed Feb 15, 2024

View reviewed changes

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

nuttycom reviewed Feb 15, 2024

View reviewed changes

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

nuttycom reviewed Feb 15, 2024

View reviewed changes

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

nuttycom reviewed Feb 15, 2024

View reviewed changes

AArnott added 2 commits February 15, 2024 13:50

Move HDSeedFingerprint to zcash_keys

5c83f0a

Drop Copy constraint on WalletRead::AccountId

3791bf1

AArnott force-pushed the diverse_accounts branch from a319684 to 5e0e9a3 Compare March 7, 2024 13:55

Remove unnecessary qualifier

2253127