Don't create new WalletDb wrappers outside WalletDb::transactionally: take &mut WalletDb<SqlTransaction<'_>, P> as argument instead

[daira]

I wrote at #1402 (comment)_ :

---

Aside: I don't understand why we're creating a new WalletDb wrapper when we could instead pass in wdb: &mut WalletDb<SqlTransaction<'_>, P>.

If I understand correctly, the point of the SqlTransaction wrapper is to mark that it was constructed by WalletDb::transactionally. This does several useful things:

• It guarantees that the rusqlite::Transaction has DEFERRED behaviour, because WalletDb::transactionally calls self.conn.transaction() which is implemented to call rusqlite::Transaction::new(conn, TransactionBehavior::Deferred). The meaning of DEFERRED is that the transaction rolls back unless commit() is called explicitly (given that we never call rusqlite::Connection::set_drop_behavior). WalletDb::transactionally calls commit() iff the closure returns Ok(_), which is the proper behaviour of a Rust database API that we should always want.
• It also guarantees that we can't accidentally commit earlier than we intended to. SQLite doesn't support nested transactions (it does support savepoints which can be used to simulate them, but we don't use that). If some called function f that needs to be transactional were responsible for calling commit(), that would be a hazard because we couldn't reuse f in a context that also needs to be transactional: f would call commit() too early. This is avoided with the transactionally API because the transactional code never needs to call commit().
  • Note that the fact that rusqlite::Connection::transaction takes a &mut reference does not prevent this hazard: it prevents creating nested transactions at the SQLite API layer (if you don't use the Transaction::new_unchecked escape hatch), but it can't prevent user code from committing too early.
• It enforces single-threaded use of the connection when it is in a transaction, which (even if SQLite didn't internally use locks in a way that effectively prevents any useful concurrency) is necessary for the transaction API to even make sense: you have to do all the things and then call commit().

Here we don't call commit() in zcash_client_sqlite::wallet::truncate_to_height, which is correct because we take a rusqlite::Transaction as argument, but that means:

• we don't know what the transaction behaviour is set to without tracing where conn came from;
• we don't have any guarantee that the caller has taken responsibility for calling commit().

Please let's refactor (not in this PR) so that we never need to explicitly create a WalletDb wrapper like this outside WalletDb::transactionally.

[daira]

@nuttycom wrote:

SqlTransaction was added to the codebase much later on, to deal with some problems around how to implement the WalletCommitmentTrees trait. I can't recall all the problems that I ran into, but I remember spending several days and multiple pairings with @str4d figuring out an approach that could work here; there's some problem if the connection type is the same in WalletWrite as in WalletCommitmentTrees.