feat(db-arch): ctx functions and use of global db #2378

mariocynicys · 2025-02-28T17:14:53Z

The first PR in the series of PRs related to database architecture changes. Changes here won't have effect on builds since they are guarded with a new-db-arch feature flag.

This adds some utility methods in the context to get the global & wallet & address databases and data directories.

Also adds some concrete usages of the global db.

mariocynicys · 2025-02-28T17:19:53Z

mm2src/mm2_core/src/mm_ctx.rs

+    pub fn global_dir(&self) -> Result<PathBuf, String> {
+        let path = path_to_db_root(self.conf["dbdir"].as_str()).join("global");
+        if !path.exists() {
+            std::fs::create_dir_all(&path).map_err(|e| format!("Failed to create global directory: {}", e))?;


i opted for this io to be sync to avoid the ripple effect it will cause if it's async (it is A LOT esp in address db usage).

The path will probably always exist (for global and wallet dirs) anyways unless this is the first run.
For address dirs, "first runs" happen more frequently, but still, by no means a lot.

borngraced

Welldone!

borngraced · 2025-03-01T02:47:57Z

mm2src/mm2_core/src/mm_ctx.rs

+        if !path.exists() {
+            std::fs::create_dir_all(&path).map_err(|e| format!("Failed to create wallet directory: {}", e))?;
+        }
+        Ok(path)


we probably should add a new field to MmCtx e.tg rmd160_hex and initialize once same place rmd160 is initialized.

This will enable us to avoid constructing hex::encode(self.rmd160().as_slice()) every time we need to call wallet_dir and similar functions

aha agreed. there is a lot of room for optimizations really (e.g. cache connections and return arcs to them). but i didn't wanna optimize early on to not make the review harder.
we def can add this now. or else wait till the skeleton is ready and target unoptimized code then (it should be abstracted behind funcs like these, so the optimization then wouldn't really bloat the review).

by caching the connection, we don't re-calculate the hex every call to wallet DB.
01b7085

borngraced · 2025-03-01T03:23:13Z

mm2src/mm2_main/src/database/stats_nodes.rs

+    // TODO: Can't use `conn` in the return statement because it's a mutex borrow, and also clippy complains when assigning the result into a temporary `result`.
+    #[allow(clippy::let_and_return)]
+    let result = conn
        .prepare(SELECT_PEERS_NAMES)?
        .query_map([], |row| Ok((row.get(0)?, row.get(1)?)))?
-        .collect::<SqlResult<HashMap<String, String>>>()
+        .collect::<SqlResult<HashMap<String, String>>>();
+    result


conn is not dropped at the right place!. In this case, we need to explicitly drop conn after we're done with it.

pub fn select_peers_names(ctx: &MmArc) -> SqlResult<HashMap<String, String>, SqlError> { #[cfg(not(feature = "new-db-arch"))] let conn = ctx.sqlite_connection(); #[cfg(feature = "new-db-arch")] let conn = ctx.global_db().unwrap(); let peers_names = conn .prepare(SELECT_PEERS_NAMES)? .query_map([], |row| Ok((row.get(0)?, row.get(1)?)))? .collect::<SqlResult<HashMap<String, String>>>(); drop(conn); peers_names }

ummmm... no it should be dropped since peer_names is owned.

well, compiler doesn't think so.

conn dropped here while still borrowed | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type std::ops::ControlFlow<std::result::Result<std::convert::Infallible, db_common::sqlite::rusqlite::Error>, db_common::sqlite::rusqlite::Statement<'_>>

borngraced · 2025-03-01T03:30:34Z

mm2src/mm2_core/src/mm_ctx.rs

+    #[cfg(all(feature = "new-db-arch", not(target_arch = "wasm32")))]
+    pub fn global_db(&self) -> Result<Connection, String> {
+        // TODO: Initialize this DB on startup and cache it in the context to avoid having this function return `Result`.
+        //       Note that you are using `unwrap`s in the calls to `global_db`.
+        let path = self.global_dir()?.join("global.db");
+        log_sqlite_file_open_attempt(&path);
+        let connection = try_s!(Connection::open(path));
+        Ok(connection)
+    }


/ TODO: Initialize this DB on startup and cache it in the context to avoid having this function return Result.
// Note that you are using unwraps in the calls to global_db.

in addition to doing this, it will help us avoid performing these operations every time the function is called. There similar functions too e.g wallet_dir, async_wallet_db

mm2src/coins/utxo/utxo_block_header_storage/mod.rs

onur-ozkan · 2025-03-04T08:19:30Z

mm2src/mm2_core/src/mm_ctx.rs

+    #[cfg(all(feature = "new-db-arch", not(target_arch = "wasm32")))]
+    pub fn wallet_db(&self) -> Result<Connection, String> {
+        // TODO: Initialize this DB on startup and cache it in the context to avoid having this function return `Result`.
+        let path = self.wallet_dir()?.join("MM2-shared.db");
+        log_sqlite_file_open_attempt(&path);
+        let connection = try_s!(Connection::open(path));
+        Ok(connection)
+    }
+
+    /// Returns an AsyncSQL connection to the shared wallet database.
+    ///
+    /// This replaces `self.wallet_db()` and should be used for new implementations.
+    #[cfg(all(feature = "new-db-arch", not(target_arch = "wasm32")))]
+    pub async fn async_wallet_db(&self) -> Result<AsyncConnection, String> {
+        let path = self.wallet_dir()?.join("KOMODEFI.db");
+        log_sqlite_file_open_attempt(&path);
+        let connection = try_s!(AsyncConnection::open(path).await);
+        Ok(connection)
+    }
+
+    /// Returns a SQL connection to the address database.
+    #[cfg(all(feature = "new-db-arch", not(target_arch = "wasm32")))]
+    pub fn address_db(&self, address: &str) -> Result<Connection, String> {
+        let path = self.address_dir(address)?.join("MM2.db");
+        log_sqlite_file_open_attempt(&path);
+        let connection = try_s!(Connection::open(path));
+        Ok(connection)
+    }


We should probably prefer strictly typed errors instead of Strings on these to be able to handle various error situations in the incoming implementations.

mm2src/mm2_main/src/database/stats_nodes.rs

onur-ozkan

I am confident that our tests would fail if there was a problem with using an incorrect DB path/connection/etc.

LGTM other than my previous notes.

this avoids re-computations that will result in the same output. this also removes the fallible method annotation from global and wallet db getters, since we know for sure they are initialized and have a copy of them. note that the name MM2-Shared.db for the wallet db was changed to a simple wallet.db. Also for the async wallet db (KOMODEFI.db), it changes to wallet.db as well for simplicity (merging them/their tables together in the same DB file).

mariocynicys added 2 commits February 28, 2025 18:11

ctx funcs for global/wallet/address directories and dbs

11f1475

use global db in stats and block header dbs

2f3c41c

mariocynicys commented Feb 28, 2025

View reviewed changes

mariocynicys added status: pending review improvement: database labels Feb 28, 2025

borngraced requested changes Mar 1, 2025

View reviewed changes

onur-ozkan reviewed Mar 4, 2025

View reviewed changes

onur-ozkan previously approved these changes Mar 6, 2025

View reviewed changes

mariocynicys added 3 commits March 6, 2025 13:06

review(onur): don't import Arc & Mutex

95d0ded

review(onur): structure the errors instead of returning strings

d4b9084

mariocynicys dismissed onur-ozkan’s stale review via d4b9084 March 6, 2025 14:47

import sql stuff inside cfg_native!

8369891

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(db-arch): ctx functions and use of global db #2378

feat(db-arch): ctx functions and use of global db #2378

mariocynicys commented Feb 28, 2025

mariocynicys Feb 28, 2025

borngraced left a comment

borngraced Mar 1, 2025 •

edited

Loading

mariocynicys Mar 4, 2025

mariocynicys Mar 6, 2025

borngraced Mar 1, 2025 •

edited

Loading

mariocynicys Mar 4, 2025

borngraced Mar 4, 2025 •

edited

Loading

borngraced Mar 1, 2025 •

edited

Loading

mariocynicys Mar 6, 2025

onur-ozkan Mar 4, 2025

mariocynicys Mar 6, 2025

onur-ozkan left a comment

feat(db-arch): ctx functions and use of global db #2378

Are you sure you want to change the base?

feat(db-arch): ctx functions and use of global db #2378

Conversation

mariocynicys commented Feb 28, 2025

mariocynicys Feb 28, 2025

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

borngraced Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

mariocynicys Mar 4, 2025

Choose a reason for hiding this comment

mariocynicys Mar 6, 2025

Choose a reason for hiding this comment

borngraced Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

mariocynicys Mar 4, 2025

Choose a reason for hiding this comment

borngraced Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

borngraced Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

mariocynicys Mar 6, 2025

Choose a reason for hiding this comment

onur-ozkan Mar 4, 2025

Choose a reason for hiding this comment

mariocynicys Mar 6, 2025

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

borngraced Mar 1, 2025 •

edited

Loading

borngraced Mar 1, 2025 •

edited

Loading

borngraced Mar 4, 2025 •

edited

Loading

borngraced Mar 1, 2025 •

edited

Loading