Inject module_instance_id as prefix into database operations #1331

m1sterc001guy · 2023-01-15T03:53:17Z

This PR introduces a wrapper struct to wrap the DatabaseTransaction into an IsolatedDatabaseTransaction. IsolatedDatabaseTransaction will now always prepend a 2 byte "module_prefix", which is the ModuleInstanceId, when inserting and reading from the database. The purpose of this is to logically partition the database so modules can use keys that have conflicting prefixes. This also allows us to use the same key for different modules (ex: ModuleDatabaseVersion).

When passing a DatabaseTransaction to a module, it is first wrapped using with_module_prefix, which wraps the existing DatabaseTransaction inside an IsolatedDatabaseTransaction. This means they cannot overwrite keys from other modules. The fedimint server is now responsible for committing the wrapped DatabaseTransactions, because the individual modules cannot commit because they do not own the transaction. This approach also allows the fedimint server to create a DatabaseTransaction that spans over multiple modules and is also atomic.

Original issue

fedimint-api/src/db/mod.rs

dpc · 2023-01-15T06:21:33Z

Amost there, but IMO, we need to do the prefix adding and removal on the lower level APIs. The higher APIs are just a convenience layer.

BTW. We can still pass to the modules the a DatabaseTransaction. It's just that the DatabaseTransaction::tx should now hold &'a mut dyn IDatabasETransaction<'a> to the original ("vanilla") implementation.

The way I see it:

struct ModuleDatabaseTransaction<'a> {
  tx: &'a dyn IDatabaseTransaction<'a>
  prefix: Vec<u8>
}

impl IDatabaseTransaction<'a> for ModuleDatabaseTransaction<'a> {
   /* impl all the raw methods by adding suffix to the key and removing if needed on the way back (like in PrefixIter) */
}

then maybe a helper:

impl<'a> DatabaseTranstion<'a> {
   fn with_module_prefix(self, module_id: Vec<u8>) -> &'a ModuleDatabaseTransaction<'a> {
      ModuleDatabaseTransaction::new(self, module_id_to_db_byte_prefix(module_id)))
   }
}

so that in the code we can quickly turn

for (module_id, module) in self.modules {
   module.some_method(&dbtx.with_module_prefix(module_id), other_params).await?;
}

As you can see tx: &'a dyn IDatabaseTransaction<'a>, to avoid loosing the ownership of the original wrapped transaction, and get it right back after the call has ended.

There might be something I'm missing here, and async/await might get it the way, so please ping me if anything is unclear or you get stuck with some async/await + lifetimes problems.

elsirion

Agree with @dpc and added some of my own nits

client/client-lib/src/ln/mod.rs

fedimint-api/src/core/server.rs

fedimint-api/src/db/mod.rs

elsirion · 2023-01-18T22:02:40Z

fedimint-api/src/db/mod.rs

+        Box::new(raw_prefix.map(|pair| match pair {
+            Ok(kv) => {
+                let key = kv.0;
+                let stripped_key = &key[(self.prefix.len())..];


observation: Assumes prefix-freeness of ModuleInstanceId encodings.

How come? The self.prefix is the only thing we appended, and only thing we should remove. If we nest isolated classess each one will take care of its part of the prefix.

Had we change the defintion of PrefixIter to return &[u8] we might get rid of the to_vec() and since we are probably going to decode to the destination soon anyway, the higher level code would not care.

Is the concern with to_vec just that it is an extra copy? I tried changing this to &[u8] but ran into some lifetime issues, since the returned key/value pairs are temporary values with a lifetime of the existing function. I think I would need to use a Box here if we wanted to return &[u8]?

fedimint-api/src/db/mod.rs

fedimint-api/src/module/mod.rs

elsirion · 2023-01-18T22:08:39Z

modules/fedimint-wallet/src/lib.rs

+        broadcast_pending_tx(
+            db.begin_transaction()
+                .await
+                .with_module_prefix(LEGACY_HARDCODED_INSTANCE_ID_WALLET),


Needs to be fixed. I'm ok with a follow-up, but outside tests and the old client we should not use legacy ids.

Hmm yes I think this is best as a follow up. I think the issue here is that, in order to not use the hardcoded constant, we'd need to pass the module_instance_id all the way down from ModuleGen::init. We can't create a database transaction during ModuleGen::init because this thread is polling the DB and needs to create a new transaction every time. Passing it down all the way through ModuleGen::init though means individual modules are now aware of their ID, which I think we're trying to avoid. Maybe this would also be a good candidate for the notification work @maan2003

@dpc is there an existing issue for removing the legacy harded instance ids or should I make a new one?

Wait, what? This is a module code already. Whatever this module is holding in that db: Database should be already isolated.

If we need to pass modules a whole db handle, we might need a similar wrapper for the whole Database

The module must never get it's unprivileged hands on anything remotely resembling unisolated access to the database. :D

elsirion · 2023-01-18T22:10:59Z

@dpc @jkitman do you want to take another look or can we go ahead and merge?

fedimint-api/src/core/server.rs

fedimint-api/src/db/mod.rs

dpc

Something tells me that it's best to avoid that instance_id in that IServerModule, even if just to keep the typed and untyped version the same. It might come later and make us have to insert some dummy values where otherwise we wouldn't have to, or be a gateway to breaking the module-id obliviousness of the module implementation.

I think it should be calling code's responsibility to do this. I don't think there will be many places doing it, and it's not something that can be easily overlooked.

@elsirion What do you think?

m1sterc001guy · 2023-01-18T22:56:10Z

Something tells me that it's best to avoid that instance_id in that IServerModule, even if just to keep the typed and untyped version the same. It might come later and make us have to insert some dummy values where otherwise we wouldn't have to, or be a gateway to breaking the module-id obliviousness of the module implementation.

I think it should be calling code's responsibility to do this. I don't think there will be many places doing it, and it's not something that can be easily overlooked.

@elsirion What do you think?

I'll make the change, I agree it's not too difficult to do and the calling code should be responsible.

jkitman · 2023-01-19T15:22:32Z

Easier and safer for module writers to not worry about db txs 👍

fedimint-api/src/core/server.rs

elsirion

LGTM

elsirion · 2023-01-19T20:11:31Z

fedimint-api/src/db/mod.rs

@@ -169,10 +175,99 @@ impl Drop for CommitTracker {
    }
 }

+/// IsolatedDatabaseTransaction is a wrapper around DatabaseTransaction that is responsible for


nit: using inline code formatting for type names would look nicer in rendered docs

fedimint-api/src/db/mod.rs

elsirion · 2023-01-19T22:48:15Z

Can we merge @dpc @jkitman ?

dpc

I'm fine with landing already but we need to follow up with doing the same isolation technique for db: Database that we give that wallet.

@m1sterc001guy feel free to smash the Merge button, just making sure you will not miss my commment :)

…_tx, and remove module_instance_id from ServerModule

m1sterc001guy · 2023-01-20T15:58:44Z

I'm fine with landing already but we need to follow up with doing the same isolation technique for db: Database that we give that wallet.

Ah yes, I'll address that next. Will rebase then merge

dpc reviewed Jan 15, 2023

View reviewed changes

fedimint-api/src/db/mod.rs Outdated Show resolved Hide resolved

dpc reviewed Jan 15, 2023

View reviewed changes

fedimint-api/src/db/mod.rs Outdated Show resolved Hide resolved

dpc reviewed Jan 15, 2023

View reviewed changes

fedimint-api/src/db/mod.rs Outdated Show resolved Hide resolved

dpc reviewed Jan 15, 2023

View reviewed changes

fedimint-api/src/db/mod.rs Outdated Show resolved Hide resolved

dpc reviewed Jan 15, 2023

View reviewed changes

fedimint-api/src/db/mod.rs Outdated Show resolved Hide resolved

elsirion reviewed Jan 15, 2023

View reviewed changes

client/client-lib/src/ln/mod.rs Outdated Show resolved Hide resolved

fedimint-api/src/core/server.rs Show resolved Hide resolved

fedimint-api/src/db/mod.rs Outdated Show resolved Hide resolved

m1sterc001guy mentioned this pull request Jan 16, 2023

WIP: Add Module Prefix and Check Database Version #1279

Closed

m1sterc001guy force-pushed the module_prefix branch 3 times, most recently from cb09498 to 268a8fc Compare January 18, 2023 21:52

elsirion previously approved these changes Jan 18, 2023

View reviewed changes