From c9744cb15b08186f9020bf83c5d6b28194e61b54 Mon Sep 17 00:00:00 2001 From: SantiagoPittella Date: Mon, 10 Aug 2026 17:53:24 -0300 Subject: [PATCH] chore(sqlite-framework): add missing types to codec --- Cargo.lock | 1 + crates/db/Cargo.toml | 9 +- crates/db/src/sqlite/codec.rs | 277 ++++++++++++++++++++ crates/db/src/sqlite/mod.rs | 2 + crates/db/src/sqlite/pool.rs | 2 +- crates/db/src/sqlite/testing.rs | 145 ++++++++++ crates/store/Cargo.toml | 1 + crates/store/src/db/migrations.rs | 32 ++- crates/store/src/db/migrations/tests/mod.rs | 28 ++ 9 files changed, 487 insertions(+), 10 deletions(-) create mode 100644 crates/db/src/sqlite/testing.rs diff --git a/Cargo.lock b/Cargo.lock index fb39dfae82..0538715b0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3632,6 +3632,7 @@ name = "miden-node-db" version = "0.16.0-alpha.3" dependencies = [ "anyhow", + "assert_matches", "build-rs", "codegen", "deadpool", diff --git a/crates/db/Cargo.toml b/crates/db/Cargo.toml index e9138c4935..0c4b216a85 100644 --- a/crates/db/Cargo.toml +++ b/crates/db/Cargo.toml @@ -29,9 +29,14 @@ sha2 = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } +[features] +# Exposes `sqlite::testing`, a synchronous single-connection handle for testing query functions. +testing = [] + [dev-dependencies] -tempfile = { workspace = true } -tokio = { features = ["macros", "rt-multi-thread"], workspace = true } +assert_matches = { workspace = true } +tempfile = { workspace = true } +tokio = { features = ["macros", "rt-multi-thread"], workspace = true } [lib] doctest = false diff --git a/crates/db/src/sqlite/codec.rs b/crates/db/src/sqlite/codec.rs index 1e77e37934..ca0b4c66ac 100644 --- a/crates/db/src/sqlite/codec.rs +++ b/crates/db/src/sqlite/codec.rs @@ -8,9 +8,18 @@ //! [`impl_blob_codec!`](crate::impl_blob_codec) macro generates both traits for such a type. Scalar //! types map onto an SQLite `INTEGER`/`TEXT` and implement the traits directly (see the impls ported //! from the legacy `SqlTypeConvert` below). +//! +//! Integer primitives read back range-checked rather than cast, so a column holding a value outside +//! the target type's range errors instead of silently truncating. The one place a lossy conversion +//! is deliberate is where the stored encoding itself is a bit-pattern wrap (`NoteTag`, `Felt`); +//! those are documented at the impl. use std::rc::Rc; +use miden_protocol::Felt; +use miden_protocol::account::StorageSlotName; +use miden_protocol::block::BlockNumber; +use miden_protocol::note::NoteTag; use rusqlite::ToSql; use rusqlite::types::{ToSqlOutput, Value, ValueRef}; @@ -142,6 +151,45 @@ impl FromSqlValue for i64 { } } +// The unsigned integers widen losslessly on the write side and are range-checked on the read side, +// so a column holding a value outside the type's range errors instead of silently truncating. + +impl ToSqlValue for u8 { + fn to_sql_value(&self) -> DbValue { + DbValue::integer(i64::from(*self)) + } +} + +impl FromSqlValue for u8 { + fn from_sql_value(value: DbValueRef<'_>) -> Result { + Self::try_from(value.as_i64()?).map_err(|err| DatabaseError::deserialization("u8", err)) + } +} + +impl ToSqlValue for u16 { + fn to_sql_value(&self) -> DbValue { + DbValue::integer(i64::from(*self)) + } +} + +impl FromSqlValue for u16 { + fn from_sql_value(value: DbValueRef<'_>) -> Result { + Self::try_from(value.as_i64()?).map_err(|err| DatabaseError::deserialization("u16", err)) + } +} + +impl ToSqlValue for u32 { + fn to_sql_value(&self) -> DbValue { + DbValue::integer(i64::from(*self)) + } +} + +impl FromSqlValue for u32 { + fn from_sql_value(value: DbValueRef<'_>) -> Result { + Self::try_from(value.as_i64()?).map_err(|err| DatabaseError::deserialization("u32", err)) + } +} + impl ToSqlValue for bool { fn to_sql_value(&self) -> DbValue { DbValue::integer(i64::from(*self)) @@ -203,6 +251,74 @@ impl FromSqlValue for Option { } } +// DOMAIN SCALAR IMPLS +// ================================================================================================= +// +// Domain types stored in an `INTEGER`/`TEXT` column rather than as a BLOB. + +impl ToSqlValue for BlockNumber { + fn to_sql_value(&self) -> DbValue { + DbValue::integer(i64::from(self.as_u32())) + } +} + +impl FromSqlValue for BlockNumber { + fn from_sql_value(value: DbValueRef<'_>) -> Result { + u32::from_sql_value(value).map(BlockNumber::from) + } +} + +impl ToSqlValue for NoteTag { + #[expect( + clippy::cast_possible_wrap, + reason = "tags occupy the full u32 range and are stored as the wrapped i32 bit pattern" + )] + fn to_sql_value(&self) -> DbValue { + DbValue::integer(i64::from(self.as_u32() as i32)) + } +} + +impl FromSqlValue for NoteTag { + #[expect(clippy::cast_sign_loss, reason = "reverses the u32 -> i32 wrap applied on write")] + fn from_sql_value(value: DbValueRef<'_>) -> Result { + let raw = value.as_i64()?; + let raw = + i32::try_from(raw).map_err(|err| DatabaseError::deserialization("NoteTag", err))?; + Ok(NoteTag::new(raw as u32)) + } +} + +impl ToSqlValue for StorageSlotName { + fn to_sql_value(&self) -> DbValue { + DbValue::text(self.as_str().to_owned()) + } +} + +impl FromSqlValue for StorageSlotName { + fn from_sql_value(value: DbValueRef<'_>) -> Result { + StorageSlotName::new(value.as_str()?) + .map_err(|err| DatabaseError::deserialization("StorageSlotName", err)) + } +} + +/// A field element is stored as the bit reinterpretation of its canonical `u64`. +impl ToSqlValue for Felt { + #[expect( + clippy::cast_possible_wrap, + reason = "canonical field elements are stored as the wrapped i64 bit pattern" + )] + fn to_sql_value(&self) -> DbValue { + DbValue::integer(self.as_canonical_u64() as i64) + } +} + +impl FromSqlValue for Felt { + #[expect(clippy::cast_sign_loss, reason = "reverses the u64 -> i64 wrap applied on write")] + fn from_sql_value(value: DbValueRef<'_>) -> Result { + Felt::new(value.as_i64()? as u64).map_err(|err| DatabaseError::deserialization("Felt", err)) + } +} + // BLOB CODEC MACRO // ================================================================================================= @@ -243,14 +359,175 @@ macro_rules! impl_blob_codec { // rule does not force each consumer to redeclare them. impl_blob_codec!( miden_protocol::block::BlockHeader, + miden_protocol::block::BlockSignatures, miden_protocol::block::ValidatorKeys, miden_protocol::account::Account, + miden_protocol::account::AccountCode, miden_protocol::account::AccountId, + miden_protocol::account::AccountStorageHeader, + miden_protocol::account::StorageMapKey, + miden_protocol::asset::Asset, miden_protocol::transaction::TransactionId, miden_protocol::note::Note, + miden_protocol::note::NoteAssets, + miden_protocol::note::NoteAttachments, miden_protocol::note::NoteId, miden_protocol::note::NoteScript, + miden_protocol::note::NoteStorage, miden_protocol::note::Nullifier, + miden_protocol::crypto::merkle::SparseMerklePath, miden_protocol::crypto::merkle::mmr::PartialMmr, miden_protocol::Word, ); + +// TESTS +// ================================================================================================= + +#[cfg(test)] +mod tests { + use miden_protocol::Word; + use miden_protocol::block::BlockNumber; + use rusqlite::types::{Value, ValueRef}; + + use super::*; + use crate::SqlTypeConvert; + + /// Returns the `i64` a value binds to, failing the test for non-integer values. + fn bound_integer(value: &impl ToSqlValue) -> i64 { + match value.to_sql_value() { + DbValue::Single(Value::Integer(raw)) => raw, + other => panic!("expected an INTEGER binding, got {other:?}"), + } + } + + /// Reads a value back from the `i64` a column holds. + fn read_integer(raw: i64) -> Result { + T::from_sql_value(DbValueRef::new(ValueRef::Integer(raw))) + } + + // ENCODING PARITY WITH `SqlTypeConvert` + // --------------------------------------------------------------------------------------------- + // These are the load-bearing tests of this module: the codec must write and read exactly the + // bytes the diesel-era `SqlTypeConvert` impls did, or it silently misreads existing databases. + + #[test] + fn block_number_matches_sql_type_convert() { + for block_num in [ + BlockNumber::GENESIS, + BlockNumber::from(1), + BlockNumber::from(u32::MAX - 1), + BlockNumber::from(u32::MAX), + ] { + let raw = bound_integer(&block_num); + assert_eq!(raw, block_num.to_raw_sql(), "write side diverged for {block_num}"); + assert_eq!( + read_integer::(raw).unwrap(), + BlockNumber::from_raw_sql(raw).unwrap(), + "read side diverged for {block_num}", + ); + } + } + + #[test] + fn note_tag_matches_sql_type_convert() { + // The tags above `i32::MAX` are the interesting ones: they are stored as a negative + // integer, and a range-checked (rather than wrapping) read would reject them. + for tag in [ + NoteTag::new(0), + NoteTag::new(1), + NoteTag::new(i32::MAX as u32), + NoteTag::new(1 << 31), + NoteTag::new(u32::MAX), + ] { + let raw = bound_integer(&tag); + assert_eq!(raw, i64::from(tag.to_raw_sql()), "write side diverged for {tag:?}"); + assert_eq!( + read_integer::(raw).unwrap(), + tag, + "read side diverged for {tag:?}" + ); + } + } + + #[test] + fn felt_matches_sql_type_convert() { + // `Felt::MAX` is the largest canonical element; it exceeds `i64::MAX` and is therefore + // stored as a negative integer. + for felt in [Felt::ZERO, Felt::ONE, Felt::from_u32(u32::MAX), Felt::MAX] { + let raw = bound_integer(&felt); + #[expect(clippy::cast_possible_wrap, reason = "mirrors the legacy nonce encoding")] + let legacy = felt.as_canonical_u64() as i64; + assert_eq!(raw, legacy, "write side diverged for {felt}"); + assert_eq!(read_integer::(raw).unwrap(), felt, "read side diverged for {felt}"); + } + } + + #[test] + fn storage_slot_name_round_trips_as_text() { + let name = StorageSlotName::new("some_component::some_slot").unwrap(); + let DbValue::Single(Value::Text(text)) = name.to_sql_value() else { + panic!("storage slot names are stored as TEXT"); + }; + assert_eq!(text, String::from(name.clone())); + assert_eq!( + StorageSlotName::from_sql_value(DbValueRef::new(ValueRef::Text(text.as_bytes()))) + .unwrap(), + name, + ); + } + + // RANGE CHECKING + // --------------------------------------------------------------------------------------------- + + #[test] + fn unsigned_ints_round_trip_at_their_bounds() { + assert_eq!(read_integer::(bound_integer(&u8::MAX)).unwrap(), u8::MAX); + assert_eq!(read_integer::(bound_integer(&u16::MAX)).unwrap(), u16::MAX); + assert_eq!(read_integer::(bound_integer(&u32::MAX)).unwrap(), u32::MAX); + assert_eq!(read_integer::(0).unwrap(), 0); + } + + #[test] + fn out_of_range_ints_error_instead_of_truncating() { + // A cast would have yielded 0, 0, and `u32::MAX` respectively. + assert_matches::assert_matches!( + read_integer::(256), + Err(DatabaseError::ConversionSqlToRust { to: "u8", .. }) + ); + assert_matches::assert_matches!( + read_integer::(65_536), + Err(DatabaseError::ConversionSqlToRust { to: "u16", .. }) + ); + assert_matches::assert_matches!( + read_integer::(-1), + Err(DatabaseError::ConversionSqlToRust { to: "u32", .. }) + ); + } + + #[test] + fn out_of_range_block_number_errors() { + // `BlockNumber` is a u32 on the wire; a wider column value is corruption, not a wrap. + assert_matches::assert_matches!( + read_integer::(i64::from(u32::MAX) + 1), + Err(DatabaseError::ConversionSqlToRust { to: "u32", .. }) + ); + assert_matches::assert_matches!( + read_integer::(-1), + Err(DatabaseError::ConversionSqlToRust { to: "u32", .. }) + ); + } + + #[test] + fn blob_codec_round_trips_and_reports_context_on_failure() { + let word = Word::from([1u32, 2, 3, 4]); + let DbValue::Single(Value::Blob(bytes)) = word.to_sql_value() else { + panic!("words are stored as BLOBs"); + }; + assert_eq!(Word::from_sql_value(DbValueRef::new(ValueRef::Blob(&bytes))).unwrap(), word); + + assert_matches::assert_matches!( + Word::from_sql_value(DbValueRef::new(ValueRef::Blob(&[0xff]))), + Err(DatabaseError::ConversionSqlToRust { to: "miden_protocol::Word", .. }) + ); + } +} diff --git a/crates/db/src/sqlite/mod.rs b/crates/db/src/sqlite/mod.rs index 6e93aa3a8a..d5032850f3 100644 --- a/crates/db/src/sqlite/mod.rs +++ b/crates/db/src/sqlite/mod.rs @@ -3,6 +3,8 @@ mod codec; mod in_list; mod pool; +#[cfg(any(test, feature = "testing"))] +pub mod testing; mod tx; pub use codec::{DbValue, DbValueRef, FromSqlValue, ToSqlValue}; diff --git a/crates/db/src/sqlite/pool.rs b/crates/db/src/sqlite/pool.rs index e7b7cad62f..18f960c1f2 100644 --- a/crates/db/src/sqlite/pool.rs +++ b/crates/db/src/sqlite/pool.rs @@ -92,7 +92,7 @@ impl Manager for SqliteManager { /// Both pools open the file `READ_WRITE`; reader connections are made read-only at runtime with /// `PRAGMA query_only = ON` (which, unlike opening `READ_ONLY`, still lets them create the WAL /// `-shm` file and read a WAL database). -fn configure_connection(conn: &Connection, read_only: bool) -> rusqlite::Result<()> { +pub(crate) fn configure_connection(conn: &Connection, read_only: bool) -> rusqlite::Result<()> { // busy_timeout makes concurrent writers wait instead of failing immediately; foreign keys // enforce referential integrity. if read_only { diff --git a/crates/db/src/sqlite/testing.rs b/crates/db/src/sqlite/testing.rs new file mode 100644 index 0000000000..0c0aa36fb6 --- /dev/null +++ b/crates/db/src/sqlite/testing.rs @@ -0,0 +1,145 @@ +//! A synchronous, single-connection handle for testing query functions. +//! +//! Query functions take a [`ReadTx`]/[`WriteTx`], which the async pool only ever hands out inside a +//! `read`/`write` closure. That is the right shape for production code, but it forces every test of +//! a query function to be `async` and to route through the owning component's database wrapper. + +use std::path::Path; + +use rusqlite::{Connection, OpenFlags}; + +use crate::DatabaseError; +use crate::sqlite::pool::configure_connection; +use crate::sqlite::tx::{ReadTx, WriteTx}; + +/// A single SQLite connection that hands out transaction handles synchronously, for tests. +/// +/// The connection is configured identically to a pooled writer connection (WAL, foreign keys, +/// `busy_timeout`, statement cache, and the `array` module used by `rarray(?)` IN-lists), so query +/// functions behave here as they do in production. +pub struct TestConnection { + conn: Connection, +} + +impl TestConnection { + /// Opens a connection to an existing database file. + /// + /// The caller is expected to have created the schema already (via its own migrator), just as the + /// production `open` path expects a migrated file. + pub fn open(database_filepath: &Path) -> Result { + let conn = + Connection::open_with_flags(database_filepath, OpenFlags::SQLITE_OPEN_READ_WRITE)?; + configure_connection(&conn, false)?; + Ok(Self { conn }) + } + + /// Opens a connection to a private in-memory database. + /// + /// Useful for exercising the framework itself; component tests want [`open`](Self::open) so the + /// schema comes from their migrations. + pub fn open_in_memory() -> Result { + let conn = Connection::open_in_memory()?; + configure_connection(&conn, false)?; + Ok(Self { conn }) + } + + /// Returns a read handle. No transaction is opened; see the module docs. + pub fn read(&self) -> ReadTx<'_> { + ReadTx::new(&self.conn) + } + + /// Returns a write handle. No transaction is opened, so each statement autocommits; see the + /// module docs. + pub fn write(&self) -> WriteTx<'_> { + WriteTx::new(&self.conn) + } + + /// Runs `work` inside a single `IMMEDIATE` transaction, committing if it returns `Ok` and + /// rolling back otherwise. + pub fn transact(&self, work: impl FnOnce(&WriteTx<'_>) -> Result) -> Result + where + E: From, + { + self.conn.execute_batch("BEGIN IMMEDIATE").map_err(DatabaseError::from)?; + match work(&self.write()) { + Ok(value) => { + self.conn.execute_batch("COMMIT").map_err(DatabaseError::from)?; + Ok(value) + }, + Err(err) => { + // Preserve the original error: a failing rollback would only mask it. + let _ = self.conn.execute_batch("ROLLBACK"); + Err(err) + }, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Creates a single-column table to write to. + fn setup() -> TestConnection { + let db = TestConnection::open_in_memory().expect("in-memory database should open"); + db.conn + .execute_batch("CREATE TABLE items (id INTEGER PRIMARY KEY, name TEXT NOT NULL)") + .expect("table should be created"); + db + } + + fn insert(db: &TestConnection, id: u32, name: &str) -> Result { + db.write() + .execute("INSERT INTO items (id, name) VALUES (?1, ?2)", &[&id, &name]) + } + + fn names(db: &TestConnection) -> Vec { + db.read() + .query("SELECT name FROM items ORDER BY id", &[], |row| row.get::(0)) + .expect("select should succeed") + } + + #[test] + fn writes_autocommit_and_are_visible_to_the_next_read() { + let db = setup(); + insert(&db, 1, "first").expect("insert should succeed"); + assert_eq!(names(&db), vec!["first".to_string()]); + } + + #[test] + fn transact_commits_on_ok() { + let db = setup(); + db.transact::<_, DatabaseError>(|tx| { + tx.execute("INSERT INTO items (id, name) VALUES (?1, ?2)", &[&1_u32, &"first"])?; + tx.execute("INSERT INTO items (id, name) VALUES (?1, ?2)", &[&2_u32, &"second"]) + }) + .expect("transaction should commit"); + + assert_eq!(names(&db), vec!["first".to_string(), "second".to_string()]); + } + + #[test] + fn transact_rolls_back_on_error() { + let db = setup(); + let err = db + .transact::<(), DatabaseError>(|tx| { + tx.execute("INSERT INTO items (id, name) VALUES (?1, ?2)", &[&1_u32, &"first"])?; + // Duplicate primary key: the whole transaction must roll back. + tx.execute("INSERT INTO items (id, name) VALUES (?1, ?2)", &[&1_u32, &"again"])?; + Ok(()) + }) + .expect_err("duplicate primary key should fail the transaction"); + + assert_matches::assert_matches!(err, DatabaseError::Rusqlite(_)); + assert!(names(&db).is_empty(), "the successful insert must not have been committed"); + } + + #[test] + fn read_handle_cannot_be_used_after_the_connection_is_dropped() { + // A compile-time property rather than a runtime one, asserted here so it is not lost in a + // later refactor: `read`/`write` borrow the connection, so no handle can outlive it. + let db = setup(); + insert(&db, 1, "first").expect("insert should succeed"); + drop(db); + } +} diff --git a/crates/store/Cargo.toml b/crates/store/Cargo.toml index 5d37456ad8..747b7cea48 100644 --- a/crates/store/Cargo.toml +++ b/crates/store/Cargo.toml @@ -56,6 +56,7 @@ miden-standards = { workspace = true } assert_matches = { workspace = true } criterion = "0.8" fs-err = { workspace = true } +miden-node-db = { features = ["testing"], workspace = true } miden-node-test-macro = { workspace = true } miden-node-utils = { features = ["testing", "tracing-forest"], workspace = true } miden-protocol = { default-features = true, features = ["testing"], workspace = true } diff --git a/crates/store/src/db/migrations.rs b/crates/store/src/db/migrations.rs index 213b6df53c..ec778064de 100644 --- a/crates/store/src/db/migrations.rs +++ b/crates/store/src/db/migrations.rs @@ -63,20 +63,38 @@ pub fn verify_latest_schema(database_filepath: &Path) -> std::result::Result<(), Ok(()) } +/// Bootstraps a throwaway database and returns its path. +/// +/// The temporary directory is intentionally leaked so the file outlives the returned path; these are +/// test databases in the OS temp directory. #[cfg(test)] -pub(crate) fn test_connection() -> diesel::SqliteConnection { - use diesel::{Connection, SqliteConnection}; - +fn bootstrapped_test_database() -> std::path::PathBuf { let temp_dir = tempfile::tempdir().expect("failed to create temp directory"); let database_filepath = temp_dir.path().join("test.sqlite3"); bootstrap_database(&database_filepath).expect("database should bootstrap"); + let _kept_dir = temp_dir.keep(); + database_filepath +} - let conn = SqliteConnection::establish( +#[cfg(test)] +pub(crate) fn test_connection() -> diesel::SqliteConnection { + use diesel::{Connection, SqliteConnection}; + + let database_filepath = bootstrapped_test_database(); + SqliteConnection::establish( database_filepath.to_str().expect("temp database path should be valid UTF-8"), ) - .expect("temp file sqlite should always work"); - let _kept_dir = temp_dir.keep(); - conn + .expect("temp file sqlite should always work") +} + +/// Bootstraps a throwaway database and returns a framework connection to it. +/// +/// The counterpart of [`test_connection`] for query functions that have moved onto the +/// `miden-node-db` SQLite framework; see [`TestConnection`](miden_node_db::sqlite::testing::TestConnection). +#[cfg(test)] +pub(crate) fn test_framework_connection() -> miden_node_db::sqlite::testing::TestConnection { + miden_node_db::sqlite::testing::TestConnection::open(&bootstrapped_test_database()) + .expect("temp file sqlite should always work") } #[cfg(test)] diff --git a/crates/store/src/db/migrations/tests/mod.rs b/crates/store/src/db/migrations/tests/mod.rs index ddb9c034ea..1e43ea3716 100644 --- a/crates/store/src/db/migrations/tests/mod.rs +++ b/crates/store/src/db/migrations/tests/mod.rs @@ -112,6 +112,34 @@ fn migration_004_validity_intervals_backfills_valid_until() -> Result<()> { Ok(()) } +/// Smoke test for the framework test harness against the real bootstrapped schema: a write through +/// a [`TestConnection`](miden_node_db::sqlite::testing::TestConnection) autocommits and is visible +/// to the following read, and the codec round-trips the column types on the way through. +#[test] +fn framework_test_connection_reads_back_what_it_writes() -> Result<()> { + use miden_protocol::block::BlockNumber; + + let db = test_framework_connection(); + let block_num = BlockNumber::from(7); + + db.write().execute( + "INSERT INTO accounts \ + (account_id, network_account_type, block_num, account_commitment, created_at_block, \ + valid_until) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6)", + &[&vec![0xaa_u8], &0_u32, &block_num, &vec![0x01_u8], &block_num, &VALID_FOREVER], + )?; + + let stored: Vec<(BlockNumber, u32)> = + db.read() + .query("SELECT block_num, network_account_type FROM accounts", &[], |row| { + Ok((row.get::(0)?, row.get::(1)?)) + })?; + + pretty_assertions::assert_eq!(stored, vec![(block_num, 0)]); + Ok(()) +} + #[test] #[ignore = "requires diesel CLI; CI runs this in the diesel-schema job"] fn diesel_schema_is_in_sync_with_migrations() -> Result<()> {