diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c3c9fa..db574de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,12 @@ Skip the changelog only when the PR contains no runtime-affecting changes (docs, CI, tooling, tests). In that case the hook will tell you to apply the `no changelog` label instead. +## Unreleased + +### Changes + +- Removed support for in-memory databases. The --database-url CLI option is now required, the service won't start without explicitly specifying the database file path ([#113](https://github.com/0xMiden/note-transport-service/pull/113)). + ## v0.4.1 (2026-06-17) ### Features @@ -56,4 +62,4 @@ Released before this changelog was started. See [`git log v0.2..v0.3.0`](https:/ ## v0.2 (2026-01-24) -First tagged release. Earlier history available via `git log v0.2`. \ No newline at end of file +First tagged release. Earlier history available via `git log v0.2`. diff --git a/Cargo.lock b/Cargo.lock index db178f3..ae67d3d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1920,6 +1920,7 @@ dependencies = [ "prost-types", "rand 0.9.5", "serial_test", + "tempfile", "thiserror", "tokio", "tonic 0.14.6", diff --git a/Cargo.toml b/Cargo.toml index 443326d..6e253c8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ rand = { version = "0.9" } serde = { features = ["derive"], version = "1.0" } serde_json = { version = "1.0" } serial_test = { version = "3.2" } +tempfile = { version = "3" } thiserror = { default-features = false, version = "2.0" } tokio = { features = ["macros", "net", "rt-multi-thread"], version = "1.48" } tonic = { default-features = false, features = ["codegen", "transport"], version = "0.14" } diff --git a/bin/node/src/main.rs b/bin/node/src/main.rs index ec9804d..a14d29d 100644 --- a/bin/node/src/main.rs +++ b/bin/node/src/main.rs @@ -18,7 +18,7 @@ struct Args { port: u16, /// Database URL - #[arg(long, default_value = ":memory:")] + #[arg(long)] database_url: String, /// Retention period in days @@ -82,3 +82,19 @@ async fn main() -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use clap::error::ErrorKind; + + use super::*; + + #[test] + fn database_url_is_required() { + let Err(error) = Args::try_parse_from(["miden-note-transport-node"]) else { + panic!("database URL should be required"); + }; + + assert_eq!(error.kind(), ErrorKind::MissingRequiredArgument); + } +} diff --git a/crates/node/Cargo.toml b/crates/node/Cargo.toml index fa059d2..b2f96d1 100644 --- a/crates/node/Cargo.toml +++ b/crates/node/Cargo.toml @@ -69,3 +69,4 @@ rand = { workspace = true } [dev-dependencies] serial_test = { workspace = true } +tempfile = { workspace = true } diff --git a/crates/node/src/database/maintenance.rs b/crates/node/src/database/maintenance.rs index 0c88da7..796f016 100644 --- a/crates/node/src/database/maintenance.rs +++ b/crates/node/src/database/maintenance.rs @@ -70,7 +70,7 @@ mod tests { use super::*; use crate::metrics::Metrics; - use crate::test_utils::test_note_header; + use crate::test_utils::{temp_database_config, test_note_header}; use crate::types::StoredNote; fn note_at(age: Duration) -> StoredNote { @@ -86,7 +86,8 @@ mod tests { #[tokio::test] #[serial] async fn test_cleanup_old_notes_no_retention() { - let config = DatabaseConfig { retention_days: 0, ..Default::default() }; + let (mut config, _temp_dir) = temp_database_config(); + config.retention_days = 0; let db = Arc::new(Database::connect(config.clone(), Metrics::default().db).await.unwrap()); db.store_note(¬e_at(Duration::from_secs(30))).await.unwrap(); @@ -102,7 +103,8 @@ mod tests { #[tokio::test] #[serial] async fn test_cleanup_old_notes_retention() { - let config = DatabaseConfig { retention_days: 7, ..Default::default() }; + let (mut config, _temp_dir) = temp_database_config(); + config.retention_days = 7; let db = Arc::new(Database::connect(config.clone(), Metrics::default().db).await.unwrap()); db.store_note(¬e_at(Duration::from_secs(30))).await.unwrap(); @@ -118,7 +120,8 @@ mod tests { #[tokio::test] #[serial] async fn test_cleanup_old_notes_mixed_ages() { - let config = DatabaseConfig { retention_days: 1, ..Default::default() }; + let (mut config, _temp_dir) = temp_database_config(); + config.retention_days = 1; let db = Arc::new(Database::connect(config.clone(), Metrics::default().db).await.unwrap()); db.store_note(¬e_at(Duration::from_secs(30))).await.unwrap(); diff --git a/crates/node/src/database/mod.rs b/crates/node/src/database/mod.rs index c0e2203..7e9c108 100644 --- a/crates/node/src/database/mod.rs +++ b/crates/node/src/database/mod.rs @@ -64,15 +64,6 @@ pub struct DatabaseConfig { pub retention_days: u32, } -impl Default for DatabaseConfig { - fn default() -> Self { - Self { - url: ":memory:".to_string(), - retention_days: 30, - } - } -} - impl Database { /// Connect to a database (with `SQLite` backend) pub async fn connect( @@ -126,16 +117,43 @@ impl Database { #[cfg(test)] mod tests { use chrono::Utc; + use tempfile::TempDir; use super::*; use crate::metrics::Metrics; - use crate::test_utils::{TAG_LOCAL_ANY, test_note_header, test_note_header_with_tag}; + use crate::test_utils::{ + TAG_LOCAL_ANY, + temp_database_config, + test_note_header, + test_note_header_with_tag, + }; + + async fn test_database() -> (Database, TempDir) { + let (config, temp_dir) = temp_database_config(); + let database = Database::connect(config, Metrics::default().db).await.unwrap(); + (database, temp_dir) + } + + #[tokio::test] + async fn test_in_memory_databases_are_rejected() { + for url in [":memory:", "file::memory:?cache=shared", "file:notes?mode=memory&cache=shared"] + { + let result = Database::connect( + DatabaseConfig { url: url.to_string(), retention_days: 30 }, + Metrics::default().db, + ) + .await; + + assert!( + matches!(result, Err(DatabaseError::Configuration(message)) if message.contains("in-memory")), + "expected an in-memory database configuration error for {url}" + ); + } + } #[tokio::test] async fn test_sqlite_database() { - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; let note = StoredNote { header: test_note_header(), @@ -164,9 +182,7 @@ mod tests { #[tokio::test] async fn test_seq_assigned_monotonically_in_insert_order() { - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; let first = StoredNote { header: test_note_header(), @@ -207,14 +223,6 @@ mod tests { #[tokio::test] async fn test_concurrent_store_fetch_sees_all_rows() { - // Regression test for the `:memory:` pool-isolation bug: when the pool - // had max_size>1 and the URL was `:memory:`, writes and reads could - // land on different connections and each connection had its own - // isolated in-memory DB. Result: writes silently split across pool - // connections, fetches only saw a fraction of the actual data. - // - // With the pool clamped to size=1 for `:memory:`, all ops go to the - // same connection and see the same DB. use std::sync::Arc; use tokio::task::JoinSet; @@ -222,14 +230,10 @@ mod tests { const TAG_A: u32 = 0x3d9c_0000; const TAG_B: u32 = 0x47ac_0000; - let db = Arc::new( - Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(), - ); + let (db, _temp_dir) = test_database().await; + let db = Arc::new(db); - // Spawn many concurrent writers — more than the old max_size=16 — so - // that the bug would have fragmented writes across connections. + // Spawn more concurrent writers than the connection pool can service at once. let mut writers = JoinSet::new(); for i in 0..40u32 { let db = db.clone(); @@ -262,9 +266,7 @@ mod tests { #[tokio::test] async fn test_fetch_notes_seq_cursor_filtering() { - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; let note = StoredNote { header: test_note_header(), @@ -298,9 +300,7 @@ mod tests { /// distinct monotonic id and both are reachable. #[tokio::test] async fn test_seq_cursor_survives_identical_created_at() { - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; let t = Utc::now(); let note1 = StoredNote { @@ -357,9 +357,7 @@ mod tests { const TAG_A: u32 = 0x3d9c_0000; const TAG_B: u32 = 0x47ac_0000; - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; // Seed: one pre-existing tag A note. db.store_note(&StoredNote { @@ -449,9 +447,7 @@ mod tests { /// Cursors above `LEGACY_CURSOR_THRESHOLD` are treated as 0. #[tokio::test] async fn test_fetch_notes_resets_legacy_cursor() { - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; let note = StoredNote { header: test_note_header(), @@ -485,9 +481,7 @@ mod tests { async fn test_fetch_notes_paginates_at_batch_limit() { use crate::database::sqlite::FETCH_NOTES_BATCH_SIZE; - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; // Insert BATCH_SIZE + extra notes for the same tag. let extra: usize = 7; @@ -538,9 +532,7 @@ mod tests { async fn test_block_context_round_trips_through_store_and_fetch() { use miden_note_transport_proto::miden_note_transport::TransportNote; - let db = Database::connect(DatabaseConfig::default(), Metrics::default().db) - .await - .unwrap(); + let (db, _temp_dir) = test_database().await; // Store a note with a typical after_block_num value. let note = StoredNote { diff --git a/crates/node/src/database/sqlite/connection_manager.rs b/crates/node/src/database/sqlite/connection_manager.rs index d34683c..bd1aea5 100644 --- a/crates/node/src/database/sqlite/connection_manager.rs +++ b/crates/node/src/database/sqlite/connection_manager.rs @@ -1,10 +1,9 @@ -//! A minimal connection manager wrapper -//! -//! Only required to setup connection parameters, specifically `WAL`. +//! A minimal connection manager wrapper for per-connection SQLite parameters. use deadpool_sync::InteractError; use diesel::prelude::*; +use crate::database::DatabaseError; use crate::database::sqlite::migrations; /// Connection manager error types @@ -80,13 +79,6 @@ impl deadpool::managed::Manager for ConnectionManager { pub fn configure_connection_on_creation( conn: &mut SqliteConnection, ) -> Result<(), ConnectionManagerError> { - // Enable the WAL mode. This allows concurrent reads while the transaction is being written, - // this is required for proper synchronization of the servers in-memory and on-disk - // representations (see [State::apply_block]) - diesel::sql_query("PRAGMA journal_mode=WAL") - .execute(conn) - .map_err(ConnectionManagerError::ConnectionParamSetup)?; - // Enable foreign key checks. diesel::sql_query("PRAGMA foreign_keys=ON") .execute(conn) @@ -97,13 +89,16 @@ pub fn configure_connection_on_creation( .execute(conn) .map_err(ConnectionManagerError::ConnectionParamSetup)?; - // Apply migrations on each connection to ensure schema is up to date - migrations::apply_migrations(conn).map_err(|e| { - ConnectionManagerError::ConnectionParamSetup(diesel::result::Error::DatabaseError( - diesel::result::DatabaseErrorKind::UnableToSendCommand, - Box::new(format!("Migration failed: {e}")), - )) - })?; - Ok(()) } + +/// Initializes settings and schema shared by all connections to a database file. +pub fn initialize_database(conn: &mut SqliteConnection) -> Result<(), DatabaseError> { + // WAL mode is persistent for the database file and must be enabled once, before the pool can + // create connections concurrently. + diesel::sql_query("PRAGMA journal_mode=WAL") + .execute(conn) + .map_err(|e| DatabaseError::Configuration(format!("Failed to enable WAL mode: {e}")))?; + + migrations::apply_migrations(conn) +} diff --git a/crates/node/src/database/sqlite/migrations.rs b/crates/node/src/database/sqlite/migrations.rs index 5f3ddcc..6953ebe 100644 --- a/crates/node/src/database/sqlite/migrations.rs +++ b/crates/node/src/database/sqlite/migrations.rs @@ -10,7 +10,7 @@ pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("src/database/sqlit #[instrument(level = "debug", skip_all, err)] pub fn apply_migrations(conn: &mut SqliteConnection) -> std::result::Result<(), DatabaseError> { - let migrations = conn.pending_migrations(MIGRATIONS).expect("In memory migrations never fail"); + let migrations = conn.pending_migrations(MIGRATIONS).expect("embedded migrations are valid"); tracing::info!("Applying {} migration(s)", migrations.len()); if let Err(e) = conn.run_pending_migrations(MIGRATIONS) { diff --git a/crates/node/src/database/sqlite/mod.rs b/crates/node/src/database/sqlite/mod.rs index 56b3766..54eb90c 100644 --- a/crates/node/src/database/sqlite/mod.rs +++ b/crates/node/src/database/sqlite/mod.rs @@ -31,6 +31,16 @@ pub(crate) const FETCH_NOTES_BATCH_SIZE: i64 = 500; /// and two orders of magnitude below any microsecond timestamp this decade. const LEGACY_CURSOR_THRESHOLD: u64 = 1_000_000_000_000; +fn is_in_memory_url(url: &str) -> bool { + url == ":memory:" + || url.starts_with("file::memory:") + || url.strip_prefix("file:").is_some_and(|uri| { + uri.split_once('?').is_some_and(|(_, query)| { + query.split('&').any(|parameter| parameter.eq_ignore_ascii_case("mode=memory")) + }) + }) +} + /// `SQLite` implementation of the database backend pub struct SqliteDatabase { pool: deadpool_diesel::Pool>, @@ -83,35 +93,34 @@ impl DatabaseBackend for SqliteDatabase { config: DatabaseConfig, metrics: MetricsDatabase, ) -> Result { - if !std::path::Path::new(&config.url).exists() && !config.url.contains(":memory:") { + if is_in_memory_url(&config.url) { + return Err(DatabaseError::Configuration( + "SQLite in-memory databases are not supported; provide a database file path" + .to_string(), + )); + } + + if !std::path::Path::new(&config.url).exists() { std::fs::File::create(&config.url).map_err(|e| { DatabaseError::Configuration(format!("Failed to create database file: {e}")) })?; } - // SQLite `:memory:` DBs are per-connection-isolated — two connections - // pointing at `:memory:` see two different databases. With a pool of N - // connections, writes splinter across N isolated DBs and most reads - // return a partial view, which silently loses note data under load. - // - // Two ways to fix for an in-memory DB: - // 1. `file::memory:?cache=shared` — SQLite URI syntax that makes all connections share - // the SAME in-memory DB via shared cache. - // 2. Pool with `max_size=1` so only one connection exists. - // - // We pick #2 for simplicity and portability (URI mode requires the - // `SQLITE_OPEN_URI` flag to be set on connection open, which is not the - // driver default). For file-backed URLs, a large pool is appropriate - // since all connections open the same file. - let is_in_memory = config.url == ":memory:" || config.url.starts_with("file::memory:"); - let max_size = if is_in_memory { 1 } else { 16 }; - let manager = ConnectionManager::new(&config.url); let pool = deadpool_diesel::Pool::builder(manager) - .max_size(max_size) + .max_size(16) .build() .map_err(|e| DatabaseError::Pool(format!("Failed to create connection pool: {e}")))?; + // Initialize database-wide settings and migrations before exposing the pool. Otherwise, + // concurrent first requests can race while lazily creating connections. + let conn: deadpool::managed::Object = pool.get().await.map_err(|e| { + DatabaseError::Connection(format!("Failed to get initialization connection: {e}")) + })?; + conn.interact(connection_manager::initialize_database).await.map_err(|e| { + DatabaseError::Connection(format!("Failed to initialize database: {e}")) + })??; + Ok(Self { pool, metrics }) } diff --git a/crates/node/src/node/grpc/mod.rs b/crates/node/src/node/grpc/mod.rs index f8873d6..9bb8762 100644 --- a/crates/node/src/node/grpc/mod.rs +++ b/crates/node/src/node/grpc/mod.rs @@ -347,15 +347,15 @@ mod tests { use miden_note_transport_proto::miden_note_transport::miden_note_transport_server::MidenNoteTransport; use super::*; - use crate::database::{Database, DatabaseConfig}; + use crate::database::Database; use crate::metrics::Metrics; + use crate::test_utils::temp_database_config; - async fn test_server() -> GrpcServer { + async fn test_server() -> (GrpcServer, tempfile::TempDir) { let metrics = Metrics::default(); - let db = Arc::new( - Database::connect(DatabaseConfig::default(), metrics.db.clone()).await.unwrap(), - ); - GrpcServer::new(db, GrpcServerConfig::default(), metrics.grpc) + let (config, temp_dir) = temp_database_config(); + let db = Arc::new(Database::connect(config, metrics.db.clone()).await.unwrap()); + (GrpcServer::new(db, GrpcServerConfig::default(), metrics.grpc), temp_dir) } /// A client sending more tags than `MAX_TAGS_PER_FETCH_REQUEST` is rejected @@ -364,7 +364,7 @@ mod tests { /// ceiling. #[tokio::test] async fn test_fetch_notes_rejects_too_many_tags() { - let server = test_server().await; + let (server, _temp_dir) = test_server().await; let tags = vec![0u32; MAX_TAGS_PER_FETCH_REQUEST + 1]; let request = tonic::Request::new(FetchNotesRequest { tags, cursor: 0 }); @@ -384,7 +384,7 @@ mod tests { /// `BTreeSet` before issuing the query.) #[tokio::test] async fn test_fetch_notes_accepts_max_tags_at_limit() { - let server = test_server().await; + let (server, _temp_dir) = test_server().await; let tags = vec![0u32; MAX_TAGS_PER_FETCH_REQUEST]; let request = tonic::Request::new(FetchNotesRequest { tags, cursor: 0 }); diff --git a/crates/node/src/node/mod.rs b/crates/node/src/node/mod.rs index 67ec6fd..ef4d2b4 100644 --- a/crates/node/src/node/mod.rs +++ b/crates/node/src/node/mod.rs @@ -24,7 +24,7 @@ pub struct Node { } /// Node configuration -#[derive(Debug, Default, Clone)] +#[derive(Debug, Clone)] pub struct NodeConfig { /// gRPC server configuration pub grpc: GrpcServerConfig, diff --git a/crates/node/src/test_utils.rs b/crates/node/src/test_utils.rs index 55a5560..ecfcfcc 100644 --- a/crates/node/src/test_utils.rs +++ b/crates/node/src/test_utils.rs @@ -12,6 +12,17 @@ use miden_protocol::testing::account_id::ACCOUNT_ID_MAX_ZEROES; use miden_protocol::{Felt, Word}; use rand::Rng; +#[cfg(test)] +use crate::database::DatabaseConfig; + +/// Creates a database configuration backed by a temporary SQLite file. +#[cfg(test)] +pub fn temp_database_config() -> (DatabaseConfig, tempfile::TempDir) { + let temp_dir = tempfile::tempdir().expect("failed to create temporary database directory"); + let url = temp_dir.path().join("test.db").to_string_lossy().into_owned(); + (DatabaseConfig { url, retention_days: 30 }, temp_dir) +} + /// Generate a random [`NoteDetailsCommitment`] pub fn random_note_details_commitment() -> NoteDetailsCommitment { let mut rng = rand::rng(); diff --git a/docs/external/src/design.md b/docs/external/src/design.md index 22bc0a9..abb9f9a 100644 --- a/docs/external/src/design.md +++ b/docs/external/src/design.md @@ -84,7 +84,7 @@ The current server implementation does not use the request cursor to initialize ## Storage and retention -The node uses SQLite and embedded migrations. File-backed databases use a larger connection pool. In-memory databases use a single connection because SQLite `:memory:` databases are isolated per connection. +The node uses a file-backed SQLite database and embedded migrations. Notes older than the configured retention period are removed by a maintenance task. diff --git a/docs/external/src/operators.md b/docs/external/src/operators.md index 380a424..8eead9a 100644 --- a/docs/external/src/operators.md +++ b/docs/external/src/operators.md @@ -19,10 +19,10 @@ This installs the `miden-note-transport-node` binary. ## Run the node -The default configuration binds to localhost and stores notes in an in-memory SQLite database: +The database file path is required: ```bash -miden-note-transport-node +miden-note-transport-node --database-url mtln.db ``` For a reachable node with persistent storage: @@ -41,7 +41,7 @@ miden-note-transport-node \ | --- | --- | --- | | `--host` | `127.0.0.1` | Address to bind to. | | `--port` | `57292` | gRPC port. | -| `--database-url` | `:memory:` | SQLite database URL or file path. Use a file path for persistence. | +| `--database-url` | Required | SQLite database file path. | | `--retention-days` | `30` | How long to retain notes before cleanup. | | `--max-note-size` | `512000` | Maximum note details size in bytes. | | `--max-connections` | `4096` | Maximum concurrent gRPC connections. | @@ -111,7 +111,7 @@ The note transport node exposes gRPC health through the same gRPC server, not a ## Database behavior -Use a file-backed SQLite path for production-like deployments. The default `:memory:` database is useful for local testing but loses all notes on restart. +The node only supports file-backed SQLite databases and requires `--database-url` at startup. Relative paths are resolved from the node's working directory; use an absolute path for predictable production deployments. The node runs embedded migrations at startup. The current schema stores note IDs with a uniqueness constraint and uses a monotonic `seq` column for pagination.