Open daira opened 6 days ago
@nuttycom wrote:
SqlTransaction
was added to the codebase much later on, to deal with some problems around how to implement theWalletCommitmentTrees
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 inWalletWrite
as inWalletCommitmentTrees.
I wrote at https://github.com/zcash/librustzcash/pull/1402#discussion_r1651118779_ :
Aside: I don't understand why we're creating a new
WalletDb
wrapper when we could instead pass inwdb: &mut WalletDb<SqlTransaction<'_>, P>
.If I understand correctly, the point of the
SqlTransaction
wrapper is to mark that it was constructed byWalletDb::transactionally
. This does several useful things:rusqlite::Transaction
hasDEFERRED
behaviour, becauseWalletDb::transactionally
callsself.conn.transaction()
which is implemented to callrusqlite::Transaction::new(conn, TransactionBehavior::Deferred)
. The meaning ofDEFERRED
is that the transaction rolls back unlesscommit()
is called explicitly (given that we never callrusqlite::Connection::set_drop_behavior
).WalletDb::transactionally
callscommit()
iff the closure returnsOk(_)
, which is the proper behaviour of a Rust database API that we should always want.f
that needs to be transactional were responsible for callingcommit()
, that would be a hazard because we couldn't reusef
in a context that also needs to be transactional:f
would callcommit()
too early. This is avoided with thetransactionally
API because the transactional code never needs to callcommit()
.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 theTransaction::new_unchecked
escape hatch), but it can't prevent user code from committing too early.commit()
.Here we don't call
commit()
inzcash_client_sqlite::wallet::truncate_to_height
, which is correct because we take arusqlite::Transaction
as argument, but that means:conn
came from;commit()
.Please let's refactor (not in this PR) so that we never need to explicitly create a
WalletDb
wrapper like this outsideWalletDb::transactionally
.