Add auto-shielding to the data access API

[nuttycom]

This is mostly @pacu's work, now integrated with the data access API.

[gmale]

One thing I'm not yet clear on is what role does the cachedb play here? it seems like we're inserting directly into the wallet_db, bypassing cache.

If so, it seems like the workflow would be:

• wallet downloads UTXOs
• inserts them directly into wallet_db
• marks them spent as a side effect of calling shield_funds
• the corresponding transaction is considered "mined" once it has a block height added

Is that about right?

Update:
After working with the code, I realized that this breaks our convention where the Kotlin/Swift side only ever writes to the cacheDB and reads from the dataDb (and does not write directly to the dataDb) while the Rust side only reads from the cacheDb and writes to the dataDb. Perhaps it is okay to deviate from that original design because it simplifies things and we already have to deviate in other scenarios (like tx cancellation).

[gmale]

Another question I would have would be the cancel flow. Generally, the user has a 10-ish second window before any transaction is submitted to press "cancel." In this case, it seems like we'd need to add a bit of logic to revert the changes that happened while "shielding."

For normal transactions, code for this exists on the Android side. It might be good to migrate that into the data_access_api for both spend_to_address transactions and shielding ones that have been cancelled by the user prior to being submitted to the network.

[nuttycom]

One thing I'm not yet clear on is what role does the cachedb play here? it seems like we're inserting directly into the wallet_db, bypassing cache.

If so, it seems like the workflow would be:

• wallet downloads UTXOs
• inserts them directly into wallet_db
• marks them spent as a side effect of calling shield_funds
• the corresponding transaction is considered "mined" once it has a block height added

Is that about right?

Update:
After working with the code, I realized that this breaks our convention where the Kotlin/Swift side only ever writes to the cacheDB and reads from the dataDb (and does not write directly to the dataDb) while the Rust side only reads from the cacheDb and writes to the dataDb. Perhaps it is okay to deviate from that original design because it simplifies things and we already have to deviate in other scenarios (like tx cancellation).

The thing that I wanted to preserve here was that read access has to be implemented in the WalletRead trait; the existence of the cachedb is kind of an artifact. In fact, in terms of the public API, I don't think that the WalletWrite trait is likely to remain in its current form, because it's very particular to the way that we use the cachedb. I anticipate that a lot of its methods are going to be collapsed down to sending larger amounts of data (a pruned transaction or block at a time) to the backend at once, which can then be decomposed by the particular data storage layer.

The main reason that I didn't put the writes into the cachedb is that we don't have any analog of scanning for this UTXO data; we get it "already scanned for" from lightwalletd, right? Now, that's something that I think is actually a problem with this prototype, because it reveals to the lightwalletd server which transparent addresses are associated with a given wallet, and so you get a lot more information about who is shielding which transactions as potential metadata for an adversary to work from. I mentioned this and tagged @defuse in Slack about this a few days ago.

In any case, I think that the write side of this interface should currently be considered only a prototype. I'm not sure whether I'll get time to work on the broader WalletWrite changes soon, since I have a couple of higher-priority NU5 items that need to get done first, but I'll try to find time for it.

[nuttycom]

Another question I would have would be the cancel flow. Generally, the user has a 10-ish second window before any transaction is submitted to press "cancel." In this case, it seems like we'd need to add a bit of logic to revert the changes that happened while "shielding."

For normal transactions, code for this exists on the Android side. It might be good to migrate that into the data_access_api for both spend_to_address transactions and shielding ones that have been cancelled by the user prior to being submitted to the network.

That's interesting. I think that I'd like to implement that by making the back-end use a "journaled writes" strategy, but I'll have to give it some thought. So instead of a "cancel" flow it'd be more of a "commit" flow - propagate the changes to the tables that are used to query from in the future only once the transaction has been successfully submitted to the network. Then the failure path is that in the future someone's wallet might attempt a double-spend if something had gone wrong in the past, instead of having changes committed to the DB that incorrectly indicate that some UTXOs had already been shielded and are therefore unavailable.

The other nice thing about this approach is that it'd be easy to detect if you have a dirty journal, and that therefore balances might be unreliable. That's not the case if you use a rollback strategy instead.

[codecov]

Codecov Report

Merging #341 (cdd899d) into non-consensus-changes-on-branchid-37519621 (d521717) will decrease coverage by 0.87%.
The diff coverage is 31.96%.

❗ Current head cdd899d differs from pull request most recent head 3699a6d. Consider uploading reports for the commit 3699a6d to get more accurate results

@@                              Coverage Diff                               @@
##           non-consensus-changes-on-branchid-37519621     #341      +/-   ##
==============================================================================
- Coverage                                       50.23%   49.35%   -0.88%     
==============================================================================
  Files                                              88       90       +2     
  Lines                                            7963     8338     +375     
==============================================================================
+ Hits                                             4000     4115     +115     
- Misses                                           3963     4223     +260     

Impacted Files	Coverage Δ
components/zcash_note_encryption/src/lib.rs	26.56% <ø> (ø)
zcash_client_backend/src/data_api.rs	5.88% <0.00%> (ø)
zcash_client_backend/src/data_api/chain.rs	34.11% <ø> (ø)
zcash_client_backend/src/data_api/error.rs	25.71% <0.00%> (-0.76%)	⬇️
zcash_client_backend/src/decrypt.rs	0.00% <ø> (ø)
zcash_client_backend/src/welding_rig.rs	65.18% <ø> (ø)
zcash_client_sqlite/src/error.rs	28.57% <0.00%> (-7.80%)	⬇️
zcash_primitives/src/legacy.rs	78.94% <ø> (ø)
zcash_primitives/src/sapling.rs	83.95% <ø> (ø)
zcash_primitives/src/sapling/keys.rs	46.15% <ø> (-1.61%)	⬇️
... and 32 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d521717...3699a6d. Read the comment docs.

[str4d]

utACK cdd899d 🎉

[daira]

This page is so slow for me now that I cannot even access the review drop-down.

utACK with typo-only suggestions.

[daira]

utACK with typo-only suggestions.