Use `rusqlite::Connection::prepare_cached` instead of manual statement caching. by nuttycom · Pull Request #857 · zcash/librustzcash

nuttycom · 2023-06-09T17:08:35Z

rusqlite includes a mechanism for creating prepared statements that
automatically caches them and reuses the caches when possible. This
means that it's unnecessary for us to do our own caching, and also
offers a minor performance improvement in that we don't need to eagerly
prepare statements that we may not execute in the lifetime of a given
WalletDb object. It also improves code locality, because the prepared
statements are now adjacent in the code to the parameter assignment
blocks that correspond to those statements.

This also updates a number of put_x methods to use sqlite upsert
functionality via the ON CONFLICT clause, instead of having to perform
separate inserts and updates.

codecov · 2023-06-09T17:42:22Z

Codecov Report

Patch coverage: 69.93% and project coverage change: -0.13 ⚠️

Comparison is base (d2f105e) 70.09% compared to head (48434bb) 69.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #857      +/-   ##
==========================================
- Coverage   70.09%   69.97%   -0.13%     
==========================================
  Files         125      124       -1     
  Lines       11862    11551     -311     
==========================================
- Hits         8315     8083     -232     
+ Misses       3547     3468      -79

Impacted Files	Coverage Δ
zcash_client_sqlite/src/chain.rs	`51.48% <ø> (ø)`
zcash_client_sqlite/src/lib.rs	`56.21% <49.39%> (+2.17%)`	⬆️
zcash_client_sqlite/src/wallet.rs	`75.74% <80.85%> (+4.00%)`	⬆️
zcash_client_sqlite/src/wallet/sapling.rs	`79.22% <82.69%> (-0.66%)`	⬇️
zcash_client_sqlite/src/wallet/init.rs	`71.92% <100.00%> (+1.02%)`	⬆️
...ite/src/wallet/init/migrations/add_utxo_account.rs	`62.50% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

daira · 2023-06-09T18:56:22Z

 ) -> Result<Vec<(AccountId, Nullifier)>, SqliteClientError> {
    // Get the nullifiers for the notes we are tracking
-    let mut stmt_fetch_nullifiers = wdb.conn.prepare(
+    let mut stmt_fetch_nullifiers = conn.prepare(


Why prepare rather than prepare_cached here?

The only cases where I changed to use prepare_cached were those where we were previously caching prepared statements. It might be reasonable to cache here as well; I haven't looked closely to determine whether it would make sense to do so.

There appears to be no down-side. This isn't a method that is only used at startup or anything like that.

daira · 2023-06-09T19:05:21Z

+            value = :value,
+            rcm = :rcm,
+            nf = IFNULL(:nf, nf),
+            memo = IFNULL(:memo, memo),


This doesn't appear to do the same thing that the previous code did. If I'm reading correctly, when the memo is MemoBytes::empty() and an entry for (tx, output_index) already exists, the previous code would write an SQL NULL, whereas this code will leave the memo field unchanged.

This is a direct transcription of https://github.com/zcash/librustzcash/pull/857/files#diff-1e7d5d95a00896e1916695c2509d888591b4eb647ec38b7e750855e0b72c609dL223-L233, as it is used in https://github.com/zcash/librustzcash/pull/857/files#diff-1e7d5d95a00896e1916695c2509d888591b4eb647ec38b7e750855e0b72c609dL743-L746 via https://github.com/zcash/librustzcash/pull/857/files#diff-9cea07a2b90189d927a2737a964494dfea6ab1bc5a51342e481210b93586716eL378-L404

This sort of thing is part of the reason for this consolidation; having this logic about how parameters are transformed scattered across (literally, when you consider inserts) 5 different pieces of code is a disaster for understandability.

You're technically correct (the best kind of correct) about what the existing code did, but the point is that if the caller passes output with output.memo() as MemoBytes::empty(), then leaving the existing memo unchanged is wrong. Please file an issue (also for https://github.com/zcash/librustzcash/pull/857/files#r1224668332) if it isn't to be fixed in this PR.

The problem here is that we use SQL NULL both to represent the absence of a memo and to represent the empty memo. If we want an explicit operation to remove the memo from an output record, we should add that, but I think we shouldn't make a special case that sending the empty memo removes the existing value of the memo field, overwriting whatever was previously stored there. Under normal circumstances the memo value for an output should never change between calls.

(Sorry, I missed your reply earlier.)

My suggestion is to remove a special case. Currently, memo_repr converts an empty memo to a None option. memo_repr is called here and below. Therefore, in the statements that use IFNULL(:memo, memo), :memo will be NULL when the memo passed into the function is MemoBytes::empty(). So when Some(new_memo) is passed into the function, new_memo will replace the existing one except when new_memo == MemoBytes::empty().

This is propagating the implementation detail that MemoBytes::empty() is represented as an SQL NULL to the visible behaviour of the Rust functions in a way that isn't necessitated by that representation. No-one using this API would expect that if you have stored the memo "foo" and overwrite it with the empty memo, you still get "foo". You should get the same thing as if you'd written the empty memo in the first place (which happens to be not distinguishable from a missing memo, but that's a separate issue).

I take the point that the memo value for an output should not change between calls. It's still irregular and error-prone.

Hm. Perhaps a better solution all around would be to store the empty memo as a single 0xf6 byte, and special-case that for both serialization and parsing.

Filed as #866.

daira · 2023-06-09T19:13:57Z

+            to_address = :to_address,
+            to_account = :to_account,
+            value = :value,
+            memo = IFNULL(:memo, memo)",


This appears to have the same issue as https://github.com/zcash/librustzcash/pull/857/files#r1224660086

daira · 2023-06-09T19:25:31Z

@@ -32,11 +32,9 @@
 // Catch documentation errors caused by code changes.
 #![deny(rustdoc::broken_intra_doc_links)]



Codecov seems to be complaining that this file has poor test coverage.

daira · 2023-06-09T19:32:34Z

I'm unconvinced by the changes to take conn: &rusqlite::Connection (and sometimes params: &P) instead of wdb: &WalletDb<P>. Aren't these logically operations on the WalletDb, so that it's proper for the callee rather than the caller to be extracting the connection and parameters from the WalletDb?

daira

Partial review.

nuttycom · 2023-06-09T20:00:10Z

I'm unconvinced by the changes to take conn: &rusqlite::Connection (and sometimes params: &P) instead of wdb: &WalletDb<P>. Aren't these logically operations on the WalletDb, so that it's proper for the callee rather than the caller to be extracting the connection and parameters from the WalletDb?

These are all crate-private methods; taking &rusqlite::Connection is useful in circumstances where we want to access these methods directly and don't have a WalletDb in scope, such as in a database migration (where all that we have is a &rusqlite::Transaction and we can't construct a WalletDb<rusqlite::Connection, P> because we don't have an owned rusqlite::Connection value at all.)

daira · 2023-06-15T17:39:42Z

Nit for preexisting code: the code that checks for nvalid addresses is duplicated between here and get_current_address.

daira

utACK modulo comments.

… caching. `rusqlite` includes a mechanism for creating prepared statements that automatically caches them and reuses the caches when possible. This means that it's unnecessary for us to do our own caching, and also offers a minor performance improvement in that we don't need to eagerly prepare statements that we may not execute in the lifetime of a given `WalletDb` object. It also improves code locality, because the prepared statements are now adjacent in the code to the parameter assignment blocks that correspond to those statements. This also updates a number of `put_x` methods to use sqlite upsert functionality via the `ON CONFLICT` clause, instead of having to perform separate inserts and updates.

…ments for `put_received_note`

…ments for `put_sent_output`

…TXO operations.

…ency

Also address a minor naming issue from code review.

…f stored memos.

str4d

Post-hoc utACK with notes on missing documentation.

str4d · 2023-06-19T13:37:45Z

 };

-mod prepared;
-pub use prepared::DataConnStmtCache;


This removal should be documented in the changelog.

str4d · 2023-06-19T13:38:26Z

 /// A wrapper for the SQLite connection to the wallet database.
-pub struct WalletDb<P> {
-    conn: Connection,
+pub struct WalletDb<C, P> {


This change should be documented in the changelog.

str4d · 2023-06-19T13:38:45Z


-impl<P: consensus::Parameters> WalletDb<P> {
+/// A wrapper for a SQLite transaction affecting the wallet database.
+pub struct SqlTransaction<'conn>(pub(crate) rusqlite::Transaction<'conn>);


This addition should be documented in the changelog.

str4d · 2023-06-19T13:40:04Z

-    /// prepared statements that are used in write operations.
-    pub fn get_update_ops(&self) -> Result<DataConnStmtCache<'_, P>, SqliteClientError> {
-        DataConnStmtCache::new(self)
+    pub fn transactionally<F, A>(&mut self, f: F) -> Result<A, SqliteClientError>


This method needs documentation and a changelog addition entry, unless it was accidental that it was exposed in the public API (DataConnStmtCache::transactionally was private), in which case remove the pub. And even then it should probably still be documented.

str4d · 2023-06-19T13:43:43Z

-}
-
-impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> {
+impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P> {


This addition (of impl WalletWrite for WalletDb) should be documented in the changelog.

Also, a consistency nit:

Suggested change

impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P> {

impl<P: consensus::Parameters> WalletWrite for WalletDb<Connection, P> {

i.e. pick one or the other, but usually just follow what the original code does. The original code used Connection directly, whereas most (but not all) of the changes in this PR use the full import.

str4d · 2023-06-19T13:58:19Z

 /// [`create_spend_to_address`]: zcash_client_backend::data_api::wallet::create_spend_to_address
 pub fn init_accounts_table<P: consensus::Parameters>(
-    wdb: &WalletDb<P>,
+    wallet_db: &mut WalletDb<rusqlite::Connection, P>,


The earlier changes to public methods in this file don't need to be explicitly documented in the changelog as long as the additional generic parameter on WalletDb is (as I noted above). But this change does need to be documented in the changelog, because it is changing the mutability requirements (to fix a bug introduced in #307, so this should probably actually be documented in the "Fixed" section of the changelog).

str4d · 2023-06-19T13:59:20Z

-pub fn init_blocks_table<P>(
-    wdb: &WalletDb<P>,
+pub fn init_blocks_table<P: consensus::Parameters>(
+    wallet_db: &mut WalletDb<rusqlite::Connection, P>,


str4d · 2023-06-19T14:20:09Z

-) -> Result<(), SqliteClientError> {
-    stmts.stmt_mark_sapling_note_spent(tx_ref, nf)?;
-    Ok(())
+) -> Result<bool, SqliteClientError> {


The return value documentation that previously existed on DataConnStmtCache::stmt_mark_sapling_note_spent should be copied here.

I also looked back to try and figure out why the return value has never been used, and got stuck in the mires of the auto-shielding PoC PR. I added the explicit return value in 12e8c53 so that it wasn't simply dropped as the original PoC PR was doing, but we appear to have never propagated the return value 🤷

str4d · 2023-06-19T22:19:29Z

+        ":to_address": &to_address,
+        ":to_account": &to_account,
+        ":value": &i64::from(output.value()),
+        ":memo": output.memo().filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()),


Suggested change

":memo": output.memo().filter(|m| *m != &MemoBytes::empty()).map(|m| m.as_slice()),

":memo": memo_repr(output.memo()),

nuttycom force-pushed the wallet/sqlite_cached_statements branch from 8696d28 to ff6291b Compare June 9, 2023 18:00

daira reviewed Jun 9, 2023

View reviewed changes