Transactional MlsProvider

[insipx]

There are a few key changes in this PR. There are also some things that definitely could be better, but overall I think it's good. I'm happy to accept feedback if there are things that should be better

• Most of our calls to EncryptedMessageStore which interact with the database don't need '&self', and carrying this around pollutes our structs/method calls even though the types are not tightly coupled. Removing this cleaned some things up, as well as makes those calls clearer. The stores purpose now is mostly to hand out connections to things that need it
• SqlKeyStore now carries around a &mut DbConnection. We need this to be mutable b/c of diesels transaction design, which strictly requires a mutable reference. Transactions are started with XmtpOpenMlsProvider, which provides a provider that is able to access the underlying connection with provider.conn().borrow_mut()
• The underlying connection is wrapped in a RefCell. This is mostly for performance/remain close to the DbConnection !Sync bound, since we're doing mostly single-threaded work. I thought it was OK to make it a RefCell at first anyway, since making it a Mutex isn't so different.
• The transaction that is started is the default one on the Connection trait. We can control where/how the database locks to some extent, however, through more specific Sqlite Transaction methods: methods

In a follow up PR, it might be good to:

• Restrict usage of conn() method to only be available within a transaction via a XmtpOpenMlsProvider wrapper type. This makes it much more difficult to run into borrow errors resulting from the underlying RefCell type. And, if using a mutex, run into deadlocks. For now we should be extra careful about borrowing the underlying connection.
• We could add some level of error handling to our transaction method to clean up matching/continuing on the errors we see where the transaction code would go into today (I put a todo in for this)

[neekolas]

I'm perfectly fine with making all the store methods static, given how few of them ever need to call another store method.

There's probably a case to make that these should just be methods on the models Groups::find instead of EncryptedMessageStore::find_groups. Probably more a matter of taste. Having them all be methods on one big store does limit the number of imports needed, which is simpler.

Good to leave as is setup in this PR if everyone else feels the same way.

[insipx]

I don't have a problem reverting back if there are strong feelings around it, but it does clean up some of the fields in structs as well

I'm not sure how much more the store will grow, but considering we already have most of the methods within their model files, it shouldn't be difficult to refactor if we decide that pulling in all the methods is too much down the line

[neekolas]

Good to leave it for now, and we can re-open this discussion if the EncryptedMessageStore keeps growing out of control and becomes unwieldy.

[richardhuaaa]

I was actually wondering if these made sense as non-static methods on the connection. Every DB operation requires a connection anyway. But happy with this too

[neekolas]

That's what Wire does and the ergonomics actually seem pretty nice to me

[insipx]

Essentially we would then just be wrapping diesel in a NewType and abstracting it away which I would generally be in favor of. I wasn't trying to re-do the design too much here, just figured making these methods static makes it easier to think of the store as just something that provides connections and makes db operations, without having to worry about where to access it

[insipx]

I can exp with this tonight

[neekolas]

I could see this being very useful. Smart addition.

[neekolas]

Restrict usage of conn() method to only be available within a transaction via a XmtpOpenMlsProvider wrapper type. This makes it much more difficult to run into borrow errors resulting from the underlying RefCell type. And, if using a mutex, run into deadlocks. For now we should be extra careful about borrowing the underlying connection.

This sounds fine. My feeling is that we are going to want anything that touches the SqlKeyStore to be transactional anyways, since openmls does so much deleting of key material under the hood. There are a few cases like load_mls_group that are read only, but that feels like the exception and not the rule.

[richardhuaaa]

LGTM! I'm curious about what the tradeoff is of doing it this way with dispensable keystores versus having a long-lived keystore that always uses the same connection, but this works

[richardhuaaa]

Restrict usage of conn() method to only be available within a transaction via a XmtpOpenMlsProvider wrapper type.

1. Are we basically trying to enforce there is only ever one DB connection at a time? Or do we envision multiple XmtpOpenMlsProvider existing simultaneously?
2. Spitballing, one possible way is to move all of the DB methods find_groups, etc into class methods on a struct that holds a connection inside of itself. And only create/return that struct from XmtpOpenMlsProvider

[insipx]

I'm curious about what the tradeoff is of doing it this way with dispensable keystores versus having a long-lived keystore that always uses the same connection, but this works

I think either way, we'd have to have some function that starts a transaction with a mutable connection in a key store in order to remain scoped to one conn/provider. Quickly thinking through it, but we could definitely add a layer that hands out transactional key stores, if we decide it would be helpful. If we're dealing with one &mut reference, the lifetimes and borrow errors should be easier to deal with if we don't require things to live for too long. It's still possible to do, the tricky part is just satisfying diesels transaction reqs.

[insipx]

Are we basically trying to enforce there is only ever one DB connection at a time? Or do we envision multiple XmtpOpenMlsProvider existing simultaneously?

Not necessarily one db connection, but just one connection in one transaction.

Currently, it's easy to use the connection on provider rather than requesting a new connection from the pool if there's any provider type around. If there is lots of nested code using this conn, then it's easy to run into BorrowMut Errors at runtime b/c of the RefCell. In order to enforce only using the connection from provider when in the transaction, I was testing out adding a new struct, TransactionalMlsProvider that wraps XmtpOpenMlsProvider, but with the conn fn on it that allows accessing the underlying connection. then the transaction callback would pass this wrapper rather than Self. However, this might warrant some redesign in methods we already have that use the provider

I ran into some issues in Identity::new this morning for instance, where the tests only passed when using the same connection as provider. If i tried changing StoredIdentity::from to use a new conn from store, I would get a "this is not a db file" error. Not completely sure where it came from, but might just be an issue with the test database setup

Spitballing, one possible way is to move all of the DB methods find_groups, etc into class methods on a struct that holds a connection inside of itself. And only create/return that struct from XmtpOpenMlsProvider

I do think it would be worth looking into this, it might make things much cleaner and easier to handle w.r.t to the lifetime of the DbConnection. Might have to be careful of pulling too many DbConnections at once. Although i'm not sure what you mean by returning the struct from XmtpOpenMlsProvider. Would this create an interface encapsulating each database item? i.e

provider.groups().find_groups()

Or something else?

[richardhuaaa]

the tests only passed when using the same connection as provider

Do you know if the tests were using an ephemeral or persistent DB store? For ephemeral store, there is a pool size of 1, because any additional connections seem to make a new in-memory store rather than writing to the same one.

Would this create an interface encapsulating each database item?

Something like this - it might be a struct called something like ValidatedConnection, it has one field which is the connection. It has multiple methods like find_groups, get_last_synced_topic_timestamp, etc - no connection param needed on these methods. ValidatedConnection is provided to the callback used in the transaction() method in XmtpOpenMlsProvider and is not available anywhere else