diff --git a/CHANGELOG.md b/CHANGELOG.md index bb7927b40..36b82638e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,12 @@ * [BREAKING][param][rust] `NodeRpcClient` models encrypted submissions: `submit_proven_transaction` now takes `SealedTransactionInputs` instead of `TransactionInputs`, `submit_proven_batch` now takes `Vec` (one per transaction, each sealed against its own transaction ID), and implementations must provide the new `get_transaction_encryption_key` method ([#2341](https://github.com/0xMiden/rust-sdk/pull/2341)). * [BREAKING][removal][cli] Removed the `account --show --with-code` flag. Use `account --inspect --verbose` to view procedure disassembly. ([#2312](https://github.com/0xMiden/rust-sdk/issues/2312)). * [BREAKING][type][rust] Added the `NoteFilter::ScriptRoots` variant, so exhaustive matches on `NoteFilter` in `Store` implementations must handle it ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)). +* [BREAKING][behavior][store] `SqliteStore::new` rejects a database path that is not valid UTF-8. ([#2349](https://github.com/0xMiden/rust-sdk/pull/2349)). * [BREAKING][behavior][store] The SQLite base schema now declares an index on `input_notes(script_root)`. This changes the schema fingerprint, so opening a database created before this change fails with `SchemaHashMismatch` and existing stores must be recreated ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)). ### Enhancements +* [store] Added `SqliteStore::database_filepath`, which returns the backing database path losslessly as a `&Path` ([#2349](https://github.com/0xMiden/rust-sdk/pull/2349)). * [FEATURE][rust] A client that only watches a public account now recovers notes the account consumed authenticated, even when it never tracked them by tag. During sync it reads the note references the node attaches to the account's transactions, fetches each note body by id, and surfaces it through `InputNoteReader`. Requires node `0.15.1` ([#2300](https://github.com/0xMiden/rust-sdk/pull/2300)). * [FEATURE][cli] Added a `--payback-note-type` option to `swap` so the payback note can be created as public or private (defaults to private). Public payback works without any off-band advice now that SWAP derives the payback recipient deterministically ([#2190](https://github.com/0xMiden/rust-sdk/pull/2190)). * [FEATURE][rust] `Client::get_consumable_notes(Some(account_id))` now screens only that account instead of screening every tracked account and discarding the rest, so its cost no longer grows with the number of tracked accounts. Added `NoteScreener::get_batch_consumability_for_account` to screen notes against a single account ([#2338](https://github.com/0xMiden/rust-sdk/pull/2338)). diff --git a/Cargo.lock b/Cargo.lock index 2100c450a..c12477de2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2123,6 +2123,7 @@ dependencies = [ "rusqlite_migration", "thiserror", "tokio", + "tracing", ] [[package]] diff --git a/crates/sqlite-store/Cargo.toml b/crates/sqlite-store/Cargo.toml index 38e4834df..dcdb763cc 100644 --- a/crates/sqlite-store/Cargo.toml +++ b/crates/sqlite-store/Cargo.toml @@ -22,6 +22,7 @@ anyhow = { workspace = true } async-trait = { workspace = true } chrono = { workspace = true } thiserror = { workspace = true } +tracing = { workspace = true } # External dependencies deadpool = { default-features = false, features = ["managed", "rt_tokio_1"], version = "0.12" } @@ -37,3 +38,5 @@ workspace = true # Enable client testing utilities only for tests miden-client = { features = ["testing"], workspace = true } miden-standards = { features = ["testing"], workspace = true } +# `time` is only needed to bound tests that would otherwise hang on a pool regression +tokio = { features = ["time"], workspace = true } diff --git a/crates/sqlite-store/README.md b/crates/sqlite-store/README.md index 0de71a65f..e74ff85b7 100644 --- a/crates/sqlite-store/README.md +++ b/crates/sqlite-store/README.md @@ -1,12 +1,12 @@ # SQLite Store SQLite-backed `Store` implementation for the Miden client. This crate provides a production‑ready -persistence layer for std environments using SQLite (via `rusqlite`) with a small in‑memory -MerkleStore cache for fast proof queries. +persistence layer for std environments using SQLite (via `rusqlite`) with an in‑memory account SMT +forest for fast proof queries. - Persists accounts, notes, transactions, block headers, and MMR nodes - Atomic updates on transaction and state sync paths -- Connection pooling (Deadpool) and bundled SQLite for reproducible builds +- WAL journaling and bundled SQLite for reproducible builds ## Quick Start diff --git a/crates/sqlite-store/src/db_management/pool_manager.rs b/crates/sqlite-store/src/db_management/pool_manager.rs index 3ac0a55ed..b5c3739b6 100644 --- a/crates/sqlite-store/src/db_management/pool_manager.rs +++ b/crates/sqlite-store/src/db_management/pool_manager.rs @@ -1,7 +1,8 @@ use std::path::PathBuf; +use std::time::Duration; use deadpool::Runtime; -use deadpool::managed::{Manager, Metrics, RecycleResult}; +use deadpool::managed::{Manager, Metrics, RecycleError, RecycleResult}; use rusqlite::Connection; use rusqlite::vtab::array; @@ -47,7 +48,11 @@ impl SqlitePoolManager { if path.exists() && let Err(e) = std::fs::set_permissions(&path, perms.clone()) { - eprintln!("Warning: failed to set permissions on {}: {e}", path.display()); + tracing::warn!( + path = %path.display(), + error = %e, + "failed to restrict permissions on the database file" + ); } } } @@ -57,6 +62,21 @@ impl SqlitePoolManager { // queries we want to run array::load_module(&conn)?; + conn.busy_timeout(Duration::from_secs(5))?; + + let journal_mode: String = + conn.pragma_update_and_check(None, "journal_mode", "WAL", |row| row.get(0))?; + if !journal_mode.eq_ignore_ascii_case("wal") { + tracing::warn!( + journal_mode = %journal_mode, + path = %self.database_path.display(), + "database does not support WAL journal mode; commits will be slower and readers \ + will block on writes" + ); + } + + conn.pragma_update(None, "synchronous", "NORMAL")?; + // Enable foreign key checks. conn.pragma_update(None, "foreign_keys", "ON")?; @@ -73,7 +93,216 @@ impl Manager for SqlitePoolManager { deadpool_sync::SyncWrapper::new(RUNTIME, move || conn).await } - async fn recycle(&self, _: &mut Self::Type, _: &Metrics) -> RecycleResult { + async fn recycle(&self, conn: &mut Self::Type, _: &Metrics) -> RecycleResult { + if conn.is_mutex_poisoned() { + return Err(RecycleError::message("sqlite connection mutex is poisoned")); + } + + // A closure that issued a bare `BEGIN` and returned early leaves the transaction open on + // the connection, holding its locks and hiding its writes from the next caller. `rusqlite` + // only rolls back for a dropped `Transaction` guard, so undo it here. + conn.interact(|conn| { + if conn.is_autocommit() { + Ok(()) + } else { + conn.execute_batch("ROLLBACK") + } + }) + .await + .map_err(|_| RecycleError::message("failed to reset the sqlite connection"))??; + + Ok(()) + } +} + +// TESTS +// ================================================================================================ + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use miden_client::EMPTY_WORD; + use miden_client::account::component::{AccountComponent, BasicWallet}; + use miden_client::account::{ + Account, + AccountBuilder, + AccountBuilderSchemaCommitmentExt, + AccountType, + Address, + }; + use miden_client::auth::{AuthSchemeId, AuthSingleSig, PublicKeyCommitment}; + use miden_client::store::{ClientAccountType, Store}; + use miden_client::testing::common::create_test_store_path; + use miden_protocol::account::AccountComponentMetadata; + use miden_standards::account::auth::Approver; + + use crate::SqliteStore; + use crate::sql_error::SqlResultExt; + use crate::tests::create_test_store; + + /// A construction that hangs means the pool is being acquired twice, so bound it rather than + /// let it burn the whole nextest slow-timeout budget. + const OPEN_TIMEOUT: Duration = Duration::from_secs(30); + + /// Builds a minimal account that can be inserted into a store. + fn test_account(init_seed: [u8; 32]) -> anyhow::Result { + let component = AccountComponent::new( + BasicWallet::code().as_library().clone(), + vec![], + AccountComponentMetadata::new("miden::testing::dummy_component"), + )?; + + Ok(AccountBuilder::new(init_seed) + .account_type(AccountType::Private) + .with_auth_component(AuthSingleSig::new(Approver::new( + PublicKeyCommitment::from(EMPTY_WORD), + AuthSchemeId::Falcon512Poseidon2, + ))) + .with_component(component) + .build_with_schema_commitment()?) + } + + #[tokio::test] + async fn connection_pragmas_are_applied() -> anyhow::Result<()> { + let store = create_test_store().await; + + let (journal_mode, synchronous, busy_timeout, foreign_keys) = store + .interact_with_connection(|conn| { + let journal_mode: String = conn + .pragma_query_value(None, "journal_mode", |row| row.get(0)) + .into_store_error()?; + let synchronous: i32 = conn + .pragma_query_value(None, "synchronous", |row| row.get(0)) + .into_store_error()?; + let busy_timeout: i32 = conn + .pragma_query_value(None, "busy_timeout", |row| row.get(0)) + .into_store_error()?; + let foreign_keys: i32 = conn + .pragma_query_value(None, "foreign_keys", |row| row.get(0)) + .into_store_error()?; + + Ok((journal_mode, synchronous, busy_timeout, foreign_keys)) + }) + .await?; + + // Asserted on the connection rather than on the `PRAGMA` return value, because + // `pragma_update` reports success even when SQLite kept the previous journal mode. + assert_eq!(journal_mode.to_lowercase(), "wal"); + // 1 is `NORMAL`. + assert_eq!(synchronous, 1); + assert_eq!(busy_timeout, 5_000); + assert_eq!(foreign_keys, 1); + Ok(()) } + + #[tokio::test] + async fn pool_holds_a_single_connection() -> anyhow::Result<()> { + let store = create_test_store().await; + assert_eq!(store.pool.status().max_size, 1); + + Ok(()) + } + + /// Reopening a database that already holds accounts makes the SMT forest initialization in + /// `SqliteStore::new` acquire a connection after the migrations did, which deadlocks if the + /// migration connection has not been returned to the single-connection pool. + #[tokio::test] + async fn new_does_not_deadlock_on_a_populated_database() -> anyhow::Result<()> { + let path = create_test_store_path(); + + let account = test_account([0; 32])?; + { + let store = + tokio::time::timeout(OPEN_TIMEOUT, SqliteStore::new(path.clone())).await??; + store + .insert_account(&account, Address::new(account.id()), ClientAccountType::Native) + .await?; + } + + let store = tokio::time::timeout(OPEN_TIMEOUT, SqliteStore::new(path)).await??; + assert_eq!(store.get_account_ids().await?, vec![account.id()]); + + Ok(()) + } + + /// A panic inside `interact` poisons the connection's mutex. `recycle` has to drop that + /// connection, otherwise the single-connection pool hands the poisoned one back forever. + #[tokio::test] + async fn poisoned_connection_is_replaced() -> anyhow::Result<()> { + let store = create_test_store().await; + + let panicked = store + .interact_with_connection(|_| -> Result<(), miden_client::store::StoreError> { + panic!("poisoning the connection on purpose") + }) + .await; + assert!(panicked.is_err()); + + store.set_setting("after-panic".to_string(), b"value".to_vec()).await?; + assert_eq!(store.get_setting("after-panic".to_string()).await?, Some(b"value".to_vec())); + + Ok(()) + } + + /// A bare `BEGIN` that is never committed holds the connection's locks and hides its writes + /// from the next caller, so `recycle` has to roll it back. + #[tokio::test] + async fn leaked_transaction_is_rolled_back() -> anyhow::Result<()> { + let store = create_test_store().await; + + store + .interact_with_connection(|conn| { + conn.execute_batch("BEGIN").into_store_error()?; + conn.execute_batch("INSERT INTO settings (name, value) VALUES ('leaked', X'00')") + .into_store_error()?; + Ok(()) + }) + .await?; + + let autocommit = store.interact_with_connection(|conn| Ok(conn.is_autocommit())).await?; + assert!(autocommit, "the leaked transaction was not rolled back"); + assert_eq!(store.get_setting("leaked".to_string()).await?, None); + + Ok(()) + } + + /// Overlapping accessors are unsupported (see [`SqliteStore`]), but they must degrade to + /// waiting on each other's write locks rather than failing with `SQLITE_BUSY`. + #[tokio::test] + async fn overlapping_accessors_wait_instead_of_failing() -> anyhow::Result<()> { + let path = create_test_store_path(); + let first = SqliteStore::new(path.clone()).await?; + let second = SqliteStore::new(path).await?; + + first.set_setting("from-first".to_string(), b"1".to_vec()).await?; + second.set_setting("from-second".to_string(), b"2".to_vec()).await?; + + assert_eq!(second.get_setting("from-first".to_string()).await?, Some(b"1".to_vec())); + assert_eq!(first.get_setting("from-second".to_string()).await?, Some(b"2".to_vec())); + + Ok(()) + } + + /// `Store::identifier` returns `&str`, so a path it cannot represent has to be refused at + /// construction rather than reported as a placeholder every such path would share. + #[cfg(unix)] + #[tokio::test] + async fn non_utf8_database_path_is_rejected() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let mut path = create_test_store_path(); + let mut file_name = + path.file_name().expect("test path has a file name").as_bytes().to_vec(); + file_name.push(0xff); + path.set_file_name(OsStr::from_bytes(&file_name)); + + // `SqliteStore` is not `Debug`, so this cannot go through `expect_err`. + let Err(error) = SqliteStore::new(path).await else { + panic!("a non-UTF-8 path must be rejected"); + }; + assert!(error.to_string().contains("not valid UTF-8"), "unexpected error: {error}"); + } } diff --git a/crates/sqlite-store/src/lib.rs b/crates/sqlite-store/src/lib.rs index 2d6d22ad2..fa9382183 100644 --- a/crates/sqlite-store/src/lib.rs +++ b/crates/sqlite-store/src/lib.rs @@ -6,9 +6,10 @@ use std::boxed::Box; use std::collections::{BTreeMap, BTreeSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::string::{String, ToString}; use std::sync::{Arc, RwLock}; +use std::time::Duration; use std::vec::Vec; use db_management::pool_manager::{Pool, SqlitePoolManager}; @@ -19,6 +20,7 @@ use db_management::utils::{ remove_setting, set_setting, }; +use deadpool::Runtime; use miden_client::Word; use miden_client::account::{ Account, @@ -73,13 +75,18 @@ pub use builder::ClientBuilderSqliteExt; // SQLITE STORE // ================================================================================================ -/// Represents a pool of connections with an `SQLite` database. The pool is used to interact -/// concurrently with the underlying database in a safe and efficient manner. +/// `SQLite`-backed [`Store`] implementation. +/// +/// # Single accessor +/// +/// A database file must be reached through at most one live `SqliteStore` at a time, across every +/// process. The instance keeps account SMT state in memory and only rebuilds it at construction, so +/// another accessor's writes stay invisible to this one and its cached roots go stale. /// /// Current table definitions can be found at `store.sql` migration file. pub struct SqliteStore { pub(crate) pool: Pool, - database_filepath: String, + database_filepath: PathBuf, smt_forest: Arc>, } @@ -89,22 +96,35 @@ impl SqliteStore { /// Returns a new instance of [Store] instantiated with the specified configuration options. pub async fn new(database_filepath: PathBuf) -> Result { - let database_filepath_str = database_filepath.to_string_lossy().into_owned(); - let sqlite_pool_manager = SqlitePoolManager::new(database_filepath); + if database_filepath.to_str().is_none() { + return Err(StoreError::DatabaseError(format!( + "database path is not valid UTF-8: {}", + database_filepath.display() + ))); + } + + let sqlite_pool_manager = SqlitePoolManager::new(database_filepath.clone()); let pool = Pool::builder(sqlite_pool_manager) + .max_size(1) + .wait_timeout(Some(Duration::from_secs(30))) + .runtime(Runtime::Tokio1) .build() .map_err(|e| StoreError::DatabaseError(e.to_string()))?; - let conn = pool.get().await.map_err(|e| StoreError::DatabaseError(e.to_string()))?; + // Scoped so the connection returns to the pool before the SMT forest initialization below + // reaches for it. The pool holds a single connection. + { + let conn = pool.get().await.map_err(|e| StoreError::DatabaseError(e.to_string()))?; - conn.interact(apply_migrations) - .await - .map_err(|e| StoreError::DatabaseError(e.to_string()))? - .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + conn.interact(apply_migrations) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))? + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + } let store = SqliteStore { pool, - database_filepath: database_filepath_str, + database_filepath, smt_forest: Arc::new(RwLock::new(AccountSmtForest::new())), }; @@ -125,6 +145,11 @@ impl SqliteStore { Ok(store) } + /// Returns the path of the database file backing this store. + pub fn database_filepath(&self) -> &Path { + &self.database_filepath + } + /// Interacts with the database by executing the provided function on a connection from the /// pool. /// @@ -153,7 +178,9 @@ impl SqliteStore { #[async_trait::async_trait] impl Store for SqliteStore { fn identifier(&self) -> &str { - &self.database_filepath + self.database_filepath + .to_str() + .expect("rejected by SqliteStore::new when not UTF-8") } fn get_current_timestamp(&self) -> Option {