Add support for importing accounts by UFVK

[AArnott]

Add import_account_hd and import_account_ufvk methods to the WalletWrite trait and implements them for the zcash_client_sqlite crate.

Also fix wallet::add_account to not fail when the UFVK contains fewer keys than librustzcash was compiled to support. For example, a UFVK that lacks a transparent key will no longer be rejected.

Partially fulfills #1075

Open questions

• The spending_key_available parameter declared by import_account_ufvk function isn't used at present. It's a future-proofing idea for an optimization that we'll surely want to leverage down the road. The open question is should we go to the trouble to record its value in the accounts table now, or only when we do the additional work to actually optimize that path?
• Eventually I'd like to add an API to import an account with only an UIVK. With that in mind, should we add another function later to import such an account, or should we promote the sqlite crate's ViewingKey enum for use by a more general import account function that takes the ViewingKey enum? Consider also that we may want the ability to import accounts by pool-specific keys (e.g. transparent private key, sapling private key, sapling viewing key, etc.) eventually too.
• Not entirely related to this PR, but it came up in my review: when zcash_client_sqlite::wallet::add_account rewrites scan ranges for the new account, could it possibly make the range to scan smaller than it already was? Say, for example, one account was fully synced. Then two new accounts were added together, the first with an old birthday and the second with a young birthday height. Would the older birthday with the broader scan range survive such that the next sync would scan enough blocks for the longer of the two?
• Whichever merges later (this or #1135), the error raised by code added in this PR if no UA can be created should be updated to not return a code indicating that a shielded receiver is required.

TODO

• Add tests
  • [x] Success: importing an account with a UFVK that does not conflict
  • [x] Failure: importing an account with an index that is already defined in the database.
  • [x] Failure: importing an account with a viewing key that collides with an existing view-only account
  • [x] Failure: importing an account with a viewing key that collides with an existing HD derived account

[AArnott]

I've manually tested this in my app and it works. I'd prefer to add unit tests to this PR only after I get a thumbs up on the API changes so that I know I'm testing the right thing.

[AArnott]

I rebased my commits and removed the merges (which brought back merge conflicts) so that I can use these same commits in my fork without bringing in the #1431 regression. Once approved, I can do a final merge with main to resolve conflicts.

[AArnott]

Merge pushed.

[nuttycom]

There's a clippy lint that needs to be fixed (just a duplicate import); also, please check the intra-doc linking error. I have the fix locally, but since nerdcash is an organization I'm unable to push the fix onto your branch.

[nuttycom]

Also it appears that there are some test failures that need to be resolved.

[AArnott]

I'll take a look. I ran cargo test at the root and that passed, but I'll see what the command line is to run tests that are failing. And run clippy.

[AArnott]

It looks like the test failure was really just a compile failure due to the duplicate import.

[nuttycom]

I'll take a look. I ran cargo test at the root and that passed, but I'll see what the command line is to run tests that are failing. And run clippy.

Ah, yeah, usually you want cargo test --release --all-features for the final test run.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 85.71429% with 16 lines in your changes missing coverage. Please review.

Project coverage is 60.71%. Comparing base (b9e37d0) to head (31f8e64). Report is 2 commits behind head on main.

Files	Patch %	Lines
zcash_client_sqlite/src/lib.rs	86.48%	5 Missing :warning:
.../src/wallet/init/migrations/spend_key_available.rs	60.00%	4 Missing :warning:
zcash_client_sqlite/src/wallet.rs	91.17%	3 Missing :warning:
zcash_client_backend/src/data_api.rs	0.00%	2 Missing :warning:
zcash_client_sqlite/src/error.rs	0.00%	1 Missing :warning:
zcash_client_sqlite/src/testing.rs	93.33%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1425      +/-   ##
==========================================
+ Coverage   60.57%   60.71%   +0.13%     
==========================================
  Files         139      140       +1     
  Lines       16362    16448      +86     
==========================================
+ Hits         9911     9986      +75     
- Misses       6451     6462      +11     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[str4d]

Re: the other open questions:

• Eventually I'd like to add an API to import an account with only an UIVK. With that in mind, should we add another function later to import such an account, or should we promote the sqlite crate's ViewingKey enum for use by a more general import account function that takes the ViewingKey enum? Consider also that we may want the ability to import accounts by pool-specific keys (e.g. transparent private key, sapling private key, sapling viewing key, etc.) eventually too.

I do not want to make this decision at this time; we can alter WalletWrite later if we decide it becomes necessary. I will note however that the Data Access APIs are intended to be granular, with backend-agnostic logic living in zcash_client_backend. The more high-level we make WalletWrite APIs, the more logic needs to be duplicated across alternative backends (and kept in sync as future changes to the Data Access API are made.

• Not entirely related to this PR, but it came up in my review: when zcash_client_sqlite::wallet::add_account rewrites scan ranges for the new account, could it possibly make the range to scan smaller than it already was? Say, for example, one account was fully synced. Then two new accounts were added together, the first with an old birthday and the second with a young birthday height. Would the older birthday with the broader scan range survive such that the next sync would scan enough blocks for the longer of the two?

The scan queue priority logic is implemented such that new ranges won't incorrectly clobber existing ones. When these two new accounts are added:

• The first's older birthday would introduce a Historic priority scan range that replaces any existing Scanned priority range covering it from its birthday.
• The second's newer birthday would attempt to introduce an Ignored priority range extending prior to its birthday, that would not replace the Historic priority that is currently there.

[AArnott]

There is one test failure: wallet::init::migrations::ephemeral_addresses::tests::initialize_table

This test fails because it executes the create_account function in the product, but with a database that hasn't had the new migration applied (since it's a migration test). In general, it's incorrect to run the latest product as part of a migration test which executes code in a db state that isn't as current as the product. Should we copy the older version of the product functions into the migration test? The failure occurs here:

        // Creating a new account should initialize `ephemeral_addresses` for that account.
        let seed1 = vec![0x01; 32];
        let (account1_id, _usk) = db_data
            .create_account(&Secret::new(seed1), &birthday)
            .unwrap();
        assert_ne!(account0_id, account1_id);
        check(&db_data, account1_id);

I wonder based on the comment if the migration test is testing a little too much. It appears that it's testing product functionality beyond the migration itself. Maybe this part of the test should be moved to an ordinary non-migration test so that it always runs against the current db schema. If you agree, perhaps @daira as the test owner can push an update? (or rather, since this PR comes from an org instead of a personal account, either push a commit that fixes the test as an independent PR or push the commit on top of this branch somewhere I can fetch it and push it to my source branch to update this PR).

[nuttycom]

There is one test failure: wallet::init::migrations::ephemeral_addresses::tests::initialize_table

This test fails because it executes the create_account function in the product, but with a database that hasn't had the new migration applied (since it's a migration test). In general, it's incorrect to run the latest product as part of a migration test which executes code in a db state that isn't as current as the product. Should we copy the older version of the product functions into the migration test?

The normal way we handle this is that we copy the source of the method that is used in the migration test, as of the creation of that migration, into the migration test sources itself. We do this incrementally when we discover breakage; it's often a good canary for other things that one needs to look at when a migration test fails in this fashion.

[nuttycom]

@AArnott I have opened #1459 with the fix for your test errors; I apparently can't even open a PR against the nerdcash/librustzcash repo. If you could merge or cherry-pick these changes into your branch, I would like to get this in.

[nuttycom]

Alternately we can just merge #1459 if that's more convenient for you; that will close this PR as well.

[AArnott]

Thanks for fixing the failing test, @nuttycom. I've merged it into this PR.

[AArnott]

I apparently can't even open a PR against the nerdcash/librustzcash repo.

That's very odd.

[nuttycom]

Argh, looks like there's one more clippy error.

[nuttycom]

@AArnott I have added the clippy fix to my branch, if you want to pull it across.