chore: Migrate `DatabaseVersion` to global namespace #4192

m1sterc001guy · 2024-01-31T18:54:58Z

In developing the state machine migrations, I have found it annoying to work with DatabaseVersion that is within the module's namespace (since states for state machines are not kept in the module's namespace). Given that this is an existing issue, I decided to fix this first.

Adds database migration for DatabaseVersion. Since this is a migration that needs to be applied to clients and server, and is migrating keys from modules to the global namespace, I wrote some custom migration code for this. IMO this should be the only time this is necessary, since it should not be common to move keys from modules to the global namespace.
This PR also modifies the migration tests to use isolated databases, which matches what the client and server will do in production. Because of this, all of the database backups needed to be re-generated.
Has a simple test in the dummy client that verifies 0x50 prefix cannot be read after the migration, which should verify that the key collision issue is fixed.

modules/fedimint-ln-tests/tests/tests.rs

fedimint-core/src/db/mod.rs

fedimint-testing/src/db.rs

fedimint-core/src/db/mod.rs

modules/fedimint-wallet-tests/tests/tests.rs

dpc

🚀

codecov · 2024-02-01T21:12:42Z

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (718bf5e) 57.93% compared to head (95474db) 58.04%.
Report is 23 commits behind head on master.

Files	Patch %	Lines
fedimint-testing/src/db.rs	79.64%	23 Missing ⚠️
fedimint-core/src/db/mod.rs	91.57%	8 Missing ⚠️
fedimint-server/src/db.rs	62.50%	6 Missing ⚠️
modules/fedimint-dummy-client/src/lib.rs	62.50%	3 Missing ⚠️
fedimint-dbtool/src/dump.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4192      +/-   ##
==========================================
+ Coverage   57.93%   58.04%   +0.10%     
==========================================
  Files         197      197              
  Lines       43425    43546     +121     
==========================================
+ Hits        25160    25276     +116     
- Misses      18265    18270       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

modules/fedimint-wallet-tests/tests/tests.rs

elsirion

Looks good, just some nits

fedimint-core/src/module/mod.rs

elsirion · 2024-02-06T12:01:03Z

fedimint-core/src/db/mod.rs

+    // First check if the module has a `DatabaseVersion` written to
+    // `DatabaseVersionKey` If `DatabaseVersion` already exists, there is
+    // nothing to do.
+    if global_dbtx
+        .get_value(&DatabaseVersionKey(key_module_instance_id))
+        .await
+        .is_none()
+    {


nit: early return might be slightly more readable

elsirion · 2024-02-06T12:03:32Z

fedimint-core/src/db/mod.rs

+        let current_version_in_module = if let Some(module_instance_id) = module_instance_id {
+            remove_current_db_version_if_exists(
+                &mut global_dbtx
+                    .to_ref_with_prefix_module_id(module_instance_id)
+                    .into_nc(),
+                target_db_version,
+            )
+            .await
+        } else {
+            remove_current_db_version_if_exists(
+                &mut global_dbtx.to_ref().into_nc(),
+                target_db_version,
+            )
+            .await
+        };


nit: The actual fn call could be factored out, only the DB argument needs to change depending on the instance id being set or not.

m1sterc001guy force-pushed the database_version_migration branch 2 times, most recently from 238a503 to c7de29a Compare January 31, 2024 22:59

m1sterc001guy marked this pull request as ready for review January 31, 2024 23:31

m1sterc001guy requested review from a team as code owners January 31, 2024 23:31

m1sterc001guy marked this pull request as draft January 31, 2024 23:32