zcash_client_sqlite crate #246

str4d · 2020-06-25T06:30:01Z

My note-spending-v7 branch is pretty stable at this point, and now being depended on more widely (after the ECC wallet was recently open-sourced). I rebased my branch on current master to fix merge conflicts (due to recent refactors), but it's otherwise unaltered.

Closes #71.

Needed because SQLite internally stores BOOLEAN as INTEGER anyway, but this causes problems with newer versions of Room on Android.

This provides a way to expose a more fine grained measure of scan progress. For example, by scanning in batches of 100 blocks, rather than everything that is pending.

Fixes a bug where rewinding a block that contained a received note would cause a constraint violation.

gmale

Does this still have the commit to set activation heights for the Heartwood network upgrade? I'm not seeing that commit in the history. Nor do I see the related changes in this PR but it could be all this new-fangled github UI throwing me off.

Perhaps that needs to be (or has been) applied to the zcash_primitives crate as a separate PR?

str4d · 2020-06-25T20:48:44Z

@gmale that commit was separately applied in #239.

Requires bumping the MSRV to 1.40.0 because libsqlite3-sys uses features introduced in that version. remove_dir_all can similarly be unpinned.

Includes an example that exposes the API as a binary.

codecov · 2020-06-26T13:03:58Z

Codecov Report

Merging #246 into master will decrease coverage by 0.26%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
- Coverage   34.71%   34.45%   -0.27%     
==========================================
  Files          95       97       +2     
  Lines       11457    11544      +87     
==========================================
  Hits         3977     3977              
- Misses       7480     7567      +87

Impacted Files	Coverage Δ
zcash_client_sqlite/src/address.rs	`0.00% <0.00%> (ø)`
zcash_client_sqlite/src/error.rs	`0.00% <0.00%> (ø)`
zcash_primitives/src/transaction/builder.rs	`51.42% <0.00%> (-1.87%)`	⬇️
zcash_proofs/src/prover.rs	`0.00% <0.00%> (ø)`
zcash_primitives/src/primitives.rs	`47.32% <0.00%> (-0.90%)`	⬇️
zcash_primitives/src/pedersen_hash.rs	`97.91% <0.00%> (-0.05%)`	⬇️
bellman/src/gadgets/boolean.rs	`32.68% <0.00%> (ø)`
zcash_primitives/src/note_encryption.rs	`79.52% <0.00%> (ø)`
... and 2 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 7134ab8...8c250ca. Read the comment docs.

gmale

Tested ACK

daira · 2020-06-29T07:04:27Z

zcash_client_sqlite/src/init.rs

+/// let db_data = data_file.path();
+/// init_data_database(&db_data).unwrap();
+/// ```
+pub fn init_data_database<P: AsRef<Path>>(db_data: P) -> Result<(), Error> {


This should require specifying privacy-relevant policy such as whether ovks are stored (explicitly, with no defaults).

Alternatively (or in addition), transact::create_to_address could require specifying a per-transaction policy; this is where the policy would be enforced.

Specifically, here is where the current default policy is implemented: https://github.com/zcash/librustzcash/pull/246/files#diff-0db70545aba62e450ec1e1634b293f05R104

daira

Blocking request to require specifying whether ovks are stored when opening the database. This will require a version bump according to semver. I haven't reviewed the rest of the PR.

daira · 2020-06-29T07:24:44Z

There should also be an API to delete data on transactions held by the wallet, e.g. ovks and other viewing keys, and any data cached from them. As @str4d said on Discord:

Without ovk you lose the non-change recipients and memos [if you restore from the backup seed]. You still learn how much was spent and when.

The use case here is that a wallet can have a button that someone might choose to use if they are about to go into a potentially coercive environment where data on their device could be seized (the assumption being that their backup seed is not available in the coercive environment).

imichaelmiers · 2020-06-29T15:14:42Z

I agree with all of this, but we should be clear on which does what. I could easily see down stream devs confusing what does what. This might result in end users deleting history but not OVK and thinking they've erased a past sensitive purchase. Or that deleting OVK erases past purchases.

Ideally there should be some policy switch with clear labels:

no history
(if we support this) device lcoal history file but no on chain storage.
history stored on chain encrypted and undeletable , but only accessible with off device backup key. Erasing this key may lose access to money
history stored on chain encrypted and undeletable, accessible from keys stored on the device.
Erasing this key may lose access to money

gmale · 2020-06-29T17:26:11Z

This all touches on a much larger issue of whether librustzcash should store any state. I would propose that we not bandaid key storage and instead take a thoughtful approach to implementing #142 which returns control of data storage to the wallet.

I agree with all of this, but we should be clear on which does what.

I strongly agree with that statement. We seem to be discussing features and decisions that probably should be made at the wallet level, rather than within librustzcash. The root issue seems to be that, with this PR, librustzcash will officially manage state and we could avoid that entirely and let wallets manage state, instead.

gmale · 2020-06-29T17:31:04Z

The discussion here relates to the "Hide Transaction" feature request on the wallet.

pacu

Tested. Ok!

This enables an SQLite light client to specify whether recipient history can be recovered from the block chain (and by what outgoing viewing key) with per-transaction granularity.

str4d · 2020-07-09T11:56:00Z

Blocking request to require specifying whether ovks are stored when opening the database.

I've implemented an outgoing viewing key usage policy for transact::create_to_transaction, which provides per-transaction granularity. Previous behaviour can be retained by using OvkPolicy::Sender.

This will require a version bump according to semver.

zcash_client_sqlite is internally still version 0.0.0, and has thus far only been used by pinning a git commit. The version bump will occur inherently when 0.1.0 is released.

There should also be an API to delete data on transactions held by the wallet, e.g. ovks and other viewing keys, and any data cached from them.

Let's implement this in a subsequent PR.

daira

utACK (with particular attention to 8188fae, but I also reviewed the rest of the PR lightly).

daira · 2020-07-09T12:16:34Z

zcash_client_sqlite/src/init.rs

+            value INTEGER NOT NULL,
+            rcm BLOB NOT NULL,
+            nf BLOB NOT NULL UNIQUE,
+            is_change INTEGER NOT NULL,


https://www.sqlite.org/datatype3.html#boolean_datatype

SQLite does not have a separate Boolean storage class. Instead, Boolean values are stored as integers 0 (false) and 1 (true).

😢 (not a problem, but sigh).

daira · 2020-07-09T12:19:56Z

zcash_client_sqlite/src/init.rs

+            nf BLOB NOT NULL UNIQUE,
+            is_change INTEGER NOT NULL,
+            memo BLOB,
+            spent INTEGER,


Is this also supposed to be a boolean? Why no NOT NULL?

No, this is a foreign key to the transactions database, pointing at the transaction this note is spent in. NULL represents the "not spent" case.

zcash_client_sqlite/src/query.rs

daira · 2020-07-09T12:32:17Z

zcash_client_sqlite/src/query.rs

+    }
+}
+
+/// Returns the memo for a received note, if it is known and a valid UTF-8 string.


Consider filing a ticket to add APIs that get the raw memo without checking that it is valid UTF-8.

The way this crate is designed (specifically that db_data is read-only outside the crate), it's expected that the caller can query the database for that information themselves.

daira · 2020-07-09T13:02:08Z

zcash_client_sqlite/src/transact.rs

+    // get re-marked as spent).
+    //
+    // Assumes that create_to_address() will never be called in parallel, which is a
+    // reasonable assumption for a light client such as a mobile phone.


This assumption should be documented in the function comment.

It is:

Do not call this multiple times in parallel, or you will generate transactions that double-spend the same notes.

str4d requested review from pacu, gmale and ebfull June 25, 2020 06:30

str4d force-pushed the zcash_client_sqlite branch from 898586d to 27fbcea Compare June 25, 2020 06:44

str4d and others added 15 commits June 25, 2020 18:54

SQLite database structure and initialisation

c0cf55c

zcash_client_sqlite::query::get_address()

c8b70e5

zcash_client_sqlite::query::{get_balance, get_verified_balance}

72dd76e

zcash_client_sqlite::scan::scan_cached_blocks()

3ef03f2

zcash_client_sqlite::transact::create_to_address()

34725df

zcash_client_sqlite::query::get_*_memo_as_utf8()

02324fb

Add security disclaimer to README

a2de5d7

Add mainnet support to zcash_client_sqlite via a feature flag

f0ce0c5

Chain validity and reorg handling

380c2f7

zcash_client_sqlite: Support sending to t-addrs

4cfdace

zcash_client_sqlite: Store is_change as INTEGER instead of BOOLEAN

c8074d4

Needed because SQLite internally stores BOOLEAN as INTEGER anyway, but this causes problems with newer versions of Room on Android.

Added a limit value for scanning.

9363ec3

This provides a way to expose a more fine grained measure of scan progress. For example, by scanning in batches of 100 blocks, rather than everything that is pending.

zcash_client_sqlite: Update received note during scan if present

3036064

Fixes a bug where rewinding a block that contained a received note would cause a constraint violation.

zcash_client_backend: Add account to DecryptedOutput

b1a2e81

zcash_client_sqlite::scan::decrypt_and_store_transaction

131e00e

str4d force-pushed the zcash_client_sqlite branch from 27fbcea to 131e00e Compare June 25, 2020 06:55

gmale reviewed Jun 25, 2020

View reviewed changes

str4d force-pushed the zcash_client_sqlite branch 7 times, most recently from edfa9c1 to dbd3122 Compare June 26, 2020 12:17

Upgrade to rusqlite 0.23

0a61db0

Requires bumping the MSRV to 1.40.0 because libsqlite3-sys uses features introduced in that version. remove_dir_all can similarly be unpinned.

str4d added 3 commits June 27, 2020 00:45

zcash_proofs: Extract default params folder logic

e910788

zcash_proofs: Add API for downloading the Sapling parameters

9012672

Includes an example that exposes the API as a binary.

CI: Fetch and cache Sapling parameters for tests

bbc3ec5

str4d force-pushed the zcash_client_sqlite branch from dbd3122 to bbc3ec5 Compare June 26, 2020 12:48

gmale approved these changes Jun 26, 2020

View reviewed changes

daira reviewed Jun 29, 2020

View reviewed changes

daira requested changes Jun 29, 2020

View reviewed changes

gmale mentioned this pull request Jun 29, 2020

Data Access API #142

Closed

pacu approved these changes Jul 2, 2020

View reviewed changes

zcash_client_sqlite: Implement outgoing viewing key policies

8188fae

This enables an SQLite light client to specify whether recipient history can be recovered from the block chain (and by what outgoing viewing key) with per-transaction granularity.

daira approved these changes Jul 9, 2020

View reviewed changes

zcash_sqlite: Add an unreliability warning to query::get_balance docs

8c250ca

str4d merged commit d380a8c into zcash:master Jul 9, 2020

str4d deleted the zcash_client_sqlite branch July 9, 2020 22:22

str4d mentioned this pull request Jan 19, 2023

Raise MSRV for zcash_primitives and its dependents #759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zcash_client_sqlite crate #246

zcash_client_sqlite crate #246

str4d commented Jun 25, 2020 •

edited

gmale left a comment •

edited

str4d commented Jun 25, 2020 •

edited

codecov bot commented Jun 26, 2020 •

edited

gmale left a comment

daira Jun 29, 2020 •

edited

str4d Jun 29, 2020

str4d Jun 29, 2020

daira left a comment

daira commented Jun 29, 2020 •

edited

imichaelmiers commented Jun 29, 2020

gmale commented Jun 29, 2020

gmale commented Jun 29, 2020

pacu left a comment

str4d commented Jul 9, 2020

daira left a comment

daira Jul 9, 2020

daira Jul 9, 2020

str4d Jul 9, 2020 •

edited

daira Jul 9, 2020 •

edited

str4d Jul 9, 2020

daira Jul 9, 2020

str4d Jul 9, 2020

zcash_client_sqlite crate #246

zcash_client_sqlite crate #246

Conversation

str4d commented Jun 25, 2020 • edited

gmale left a comment • edited

Choose a reason for hiding this comment

str4d commented Jun 25, 2020 • edited

codecov bot commented Jun 26, 2020 • edited

Codecov Report

gmale left a comment

Choose a reason for hiding this comment

daira Jun 29, 2020 • edited

Choose a reason for hiding this comment

str4d Jun 29, 2020

Choose a reason for hiding this comment

str4d Jun 29, 2020

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

daira commented Jun 29, 2020 • edited

imichaelmiers commented Jun 29, 2020

gmale commented Jun 29, 2020

gmale commented Jun 29, 2020

pacu left a comment

Choose a reason for hiding this comment

str4d commented Jul 9, 2020

daira left a comment

Choose a reason for hiding this comment

daira Jul 9, 2020

Choose a reason for hiding this comment

daira Jul 9, 2020

Choose a reason for hiding this comment

str4d Jul 9, 2020 • edited

Choose a reason for hiding this comment

daira Jul 9, 2020 • edited

Choose a reason for hiding this comment

str4d Jul 9, 2020

Choose a reason for hiding this comment

daira Jul 9, 2020

Choose a reason for hiding this comment

str4d Jul 9, 2020

Choose a reason for hiding this comment

str4d commented Jun 25, 2020 •

edited

gmale left a comment •

edited

str4d commented Jun 25, 2020 •

edited

codecov bot commented Jun 26, 2020 •

edited

daira Jun 29, 2020 •

edited

daira commented Jun 29, 2020 •

edited

str4d Jul 9, 2020 •

edited

daira Jul 9, 2020 •

edited