Skip to content

An alternative approach to how to make Sqlite storage transactions usable in async context#1976

Merged
ImplOfAnImpl merged 3 commits intomasterfrom
sendable_db_txs_in_wallet
Oct 9, 2025
Merged

An alternative approach to how to make Sqlite storage transactions usable in async context#1976
ImplOfAnImpl merged 3 commits intomasterfrom
sendable_db_txs_in_wallet

Conversation

@ImplOfAnImpl
Copy link
Contributor

An alternative approach to how to make Sqlite storage transactions usable in async context.

The work is based on an earlier version of #1846, so this PR contains both my and Boris's changes. Also, since I had to rebase it on top of master and to make rebasing easier, I squashed all commits into one, so now it's unfortunately non-trivial to understand which changes belong to whom.

The gist of the change is that now Sqlite storage transactions are Send. To make them so I changed sqlite::DbTx to hold a mutex to the underlying connection and make it lock the mutex on each operation, while the previous implementation was locking it only once and kept the lock inside sqlite::DbTx (making it non-Send, because locks are non-Send).

Once consequence of this change is that in the previous implementation it was technically possible to create multiple transaction objects at the same time; if created on different threads, they would wait for the previous ones to close, due to the lock (or, if they were created on the same thread, they would just deadlock). With the new approach this is no longer possible (because the lock is not held all the time, and because Sqlite doesn't support nested transactions). So I've split the Backend trait into 2 - the original Backend is now called SharedBackend (better naming suggestions are welcome) and, as before, it is shallow-clonable and allows to create rw transactions from &self. And the new Backend is no longer ShallowClone and its rw transactions can only be created from &mut self. This approach still allows creating multiple simultaneous ro transaction objects; to make it work, sqlite::DbTx holds a counter of currenctly "open" ro transaction objects and only opens a real transaction if the counter goes from 0 to 1 (and closes it when it goes from 1 to 0).

The fact that wallet's backend is no longer clonable has led to some ugliness in its tests - some of the tests actually used the clonability to reload the wallet from the same storage to check that the wallet is in the same state after the realod. For now I've solved it by introducing the method Sqlite::new_named_in_memory in addition to the previously existing Sqlite::new_in_memory. Unlike "unnamed" ones, a "named" in-memory db can be re-opened, provided that at least one connection to the db with the same name still exists. So, the tests that need to re-open the db now use this new_named_in_memory approach.
The better solution would be to create some wrapper storage implementation that would hold a reference to an existing storage, allowing ro transactions and panicking when an rw one is requested. However this is currently not possible, because BackendImpl has the 'static bound, which doesn't seem to be easy to get rid of.

P.S. I also fixed some warnings coming from Rust/Clippy 1.90 and disabled one of them ("manual_is_multiple_of", see the comment in do_checks.sh). Let me know if you have objections.

@ImplOfAnImpl ImplOfAnImpl marked this pull request as draft October 1, 2025 08:41
@ImplOfAnImpl ImplOfAnImpl marked this pull request as ready for review October 2, 2025 08:35
impl_read_ops!(StoreTxRoUnlocked);
impl_read_ops!(StoreTxRwUnlocked);

impl<T> WalletStorageReadLocked for &mut T
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those trait implementations will not be needed if we change the Signer trait to accept a &mut T instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those trait implementations will not be needed if we change the Signer trait to accept a &mut T instead.

I've looked at it again and I don't think we should change Signer to accept &mut impl WalletStorageReadUnlocked because all WalletStorageReadXXXs methods accept &self, so it would just look strange.
For now I've added the following note:

// Note: the reasons for having this impl are:
// 1) Unlike `&T`, `&mut T` can be `Send` without requiring `T` to be `Sync`, so in async code `&mut db_tx`
// can be passed across an await point but `&db_tx` can't.
// 2) Accepting `&mut impl WalletStorageReadXXX` in a generic async function would look weird, because
// the Read traits themselves don't require mutability. So this impl allows a generic function to accept
// `impl WalletStorageReadXXX` by value, while the caller may pass a `&mut db_tx` to it if needed.

…ckend` trait renamed to `SharedBackend`; new `Backend` is not clonable and requires mut self to create rw tx
@ImplOfAnImpl ImplOfAnImpl force-pushed the sendable_db_txs_in_wallet branch from c4da8c6 to 8795797 Compare October 8, 2025 15:36
@ImplOfAnImpl ImplOfAnImpl merged commit f5ddd6c into master Oct 9, 2025
28 checks passed
@ImplOfAnImpl ImplOfAnImpl deleted the sendable_db_txs_in_wallet branch October 9, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants