From abdde307c68f2d0f5e2f7b0399a96711c77b698d Mon Sep 17 00:00:00 2001 From: Cesar Rodas Date: Sat, 19 Apr 2025 02:45:33 -0300 Subject: [PATCH 1/4] Fix race conditions with proof state updates. Add a strict set of updates to prevent incorrect state changes and correct usage. Supporting the transaction at the trait level prevented some cases, but having a strict set of state change flows is better. This bug was found while developing the signatory. The keys are read from memory, triggering race conditions at the database, and some `Pending` states are selected (instead of just selecting `Unspent`). This PR also introduces a set of generic database tests to be executed for all database implementations, this test suite will make sure writing and maintaining new database drivers --- crates/cdk-common/Cargo.toml | 1 + crates/cdk-common/src/database/mint/mod.rs | 3 + crates/cdk-common/src/database/mint/test.rs | 83 +++++++++++++++++++++ crates/cdk-common/src/database/mod.rs | 16 +++- crates/cdk-common/src/error.rs | 15 +++- crates/cdk-common/src/lib.rs | 1 + crates/cdk-common/src/state.rs | 42 +++++++++++ crates/cdk-redb/Cargo.toml | 2 +- crates/cdk-redb/src/mint/mod.rs | 23 ++++-- crates/cdk-sqlite/Cargo.toml | 4 +- crates/cdk-sqlite/src/mint/mod.rs | 15 ++-- crates/cdk-sqlite/src/wallet/mod.rs | 2 +- 12 files changed, 188 insertions(+), 19 deletions(-) create mode 100644 crates/cdk-common/src/database/mint/test.rs create mode 100644 crates/cdk-common/src/state.rs diff --git a/crates/cdk-common/Cargo.toml b/crates/cdk-common/Cargo.toml index 6b96468d..2c9ce391 100644 --- a/crates/cdk-common/Cargo.toml +++ b/crates/cdk-common/Cargo.toml @@ -13,6 +13,7 @@ readme = "README.md" [features] default = ["mint", "wallet"] swagger = ["dep:utoipa", "cashu/swagger"] +test = [] bench = [] wallet = ["cashu/wallet"] mint = ["cashu/mint", "dep:uuid"] diff --git a/crates/cdk-common/src/database/mint/mod.rs b/crates/cdk-common/src/database/mint/mod.rs index 8aa67481..a11e54f9 100644 --- a/crates/cdk-common/src/database/mint/mod.rs +++ b/crates/cdk-common/src/database/mint/mod.rs @@ -17,6 +17,9 @@ use crate::nuts::{ #[cfg(feature = "auth")] mod auth; +#[cfg(feature = "test")] +pub mod test; + #[cfg(feature = "auth")] pub use auth::MintAuthDatabase; diff --git a/crates/cdk-common/src/database/mint/test.rs b/crates/cdk-common/src/database/mint/test.rs new file mode 100644 index 00000000..f5177712 --- /dev/null +++ b/crates/cdk-common/src/database/mint/test.rs @@ -0,0 +1,83 @@ +//! Macro with default tests +//! +//! This set is generic and checks the default and expected behaviour for a mint database +//! implementation +use std::fmt::Debug; +use std::str::FromStr; + +use cashu::secret::Secret; +use cashu::{Amount, CurrencyUnit, SecretKey}; + +use super::*; +use crate::mint::MintKeySetInfo; + +#[inline] +async fn setup_keyset>(db: &DB) -> Id { + let keyset_id = Id::from_str("00916bbf7ef91a36").unwrap(); + let keyset_info = MintKeySetInfo { + id: keyset_id, + unit: CurrencyUnit::Sat, + active: true, + valid_from: 0, + valid_to: None, + derivation_path: bitcoin::bip32::DerivationPath::from_str("m/0'/0'/0'").unwrap(), + derivation_path_index: Some(0), + max_order: 32, + input_fee_ppk: 0, + }; + db.add_keyset_info(keyset_info).await.unwrap(); + keyset_id +} + +/// State transition test +pub async fn state_transition>(db: DB) { + let keyset_id = setup_keyset(&db).await; + + let proofs = vec![ + Proof { + amount: Amount::from(100), + keyset_id, + secret: Secret::generate(), + c: SecretKey::generate().public_key(), + witness: None, + dleq: None, + }, + Proof { + amount: Amount::from(200), + keyset_id, + secret: Secret::generate(), + c: SecretKey::generate().public_key(), + witness: None, + dleq: None, + }, + ]; + + // Add proofs to database + db.add_proofs(proofs.clone(), None).await.unwrap(); + + // Mark one proof as `pending` + assert!(db + .update_proofs_states(&[proofs[0].y().unwrap()], State::Pending) + .await + .is_ok()); + + // Attempt to select the `pending` proof, as `pending` again (which should fail) + assert!(db + .update_proofs_states(&[proofs[0].y().unwrap()], State::Pending) + .await + .is_err()); +} + +/// Unit test that is expected to be passed for a correct database implementation +#[macro_export] +macro_rules! mint_db_test { + ($make_db_fn:ident) => { + mint_db_test!(state_transition, $make_db_fn); + }; + ($name:ident, $make_db_fn:ident) => { + #[tokio::test] + async fn $name() { + cdk_common::database::mint::test::$name($make_db_fn().await).await; + } + }; +} diff --git a/crates/cdk-common/src/database/mod.rs b/crates/cdk-common/src/database/mod.rs index 6dbd1796..c0a67db8 100644 --- a/crates/cdk-common/src/database/mod.rs +++ b/crates/cdk-common/src/database/mod.rs @@ -1,7 +1,7 @@ //! CDK Database #[cfg(feature = "mint")] -mod mint; +pub mod mint; #[cfg(feature = "wallet")] mod wallet; @@ -16,6 +16,8 @@ pub use mint::{ #[cfg(feature = "wallet")] pub use wallet::Database as WalletDatabase; +use crate::state; + /// CDK_database error #[derive(Debug, thiserror::Error)] pub enum Error { @@ -53,4 +55,16 @@ pub enum Error { /// Invalid keyset #[error("Unknown or invalid keyset")] InvalidKeysetId, + /// Invalid state transition + #[error("Invalid state transition")] + InvalidStateTransition(state::Error), +} + +impl From for Error { + fn from(state: state::Error) -> Self { + match state { + state::Error::AlreadySpent => Error::AttemptUpdateSpentProof, + _ => Error::InvalidStateTransition(state), + } + } } diff --git a/crates/cdk-common/src/error.rs b/crates/cdk-common/src/error.rs index bcac4a9b..5e4cd4bc 100644 --- a/crates/cdk-common/src/error.rs +++ b/crates/cdk-common/src/error.rs @@ -317,7 +317,7 @@ pub enum Error { NUT22(#[from] crate::nuts::nut22::Error), /// Database Error #[error(transparent)] - Database(#[from] crate::database::Error), + Database(crate::database::Error), /// Payment Error #[error(transparent)] #[cfg(feature = "mint")] @@ -502,6 +502,19 @@ impl From for ErrorResponse { } } +impl From for Error { + fn from(db_error: crate::database::Error) -> Self { + match db_error { + crate::database::Error::InvalidStateTransition(state) => match state { + crate::state::Error::Pending => Self::TokenPending, + crate::state::Error::AlreadySpent => Self::TokenAlreadySpent, + state => Self::Database(crate::database::Error::InvalidStateTransition(state)), + }, + db_error => Self::Database(db_error), + } + } +} + impl From for Error { fn from(err: ErrorResponse) -> Error { match err.code { diff --git a/crates/cdk-common/src/lib.rs b/crates/cdk-common/src/lib.rs index fdc5f475..e0732165 100644 --- a/crates/cdk-common/src/lib.rs +++ b/crates/cdk-common/src/lib.rs @@ -16,6 +16,7 @@ pub mod mint; #[cfg(feature = "mint")] pub mod payment; pub mod pub_sub; +pub mod state; pub mod subscription; #[cfg(feature = "wallet")] pub mod wallet; diff --git a/crates/cdk-common/src/state.rs b/crates/cdk-common/src/state.rs new file mode 100644 index 00000000..f514808b --- /dev/null +++ b/crates/cdk-common/src/state.rs @@ -0,0 +1,42 @@ +//! State transition rules + +use cashu::State; + +/// State transition Error +#[derive(thiserror::Error, Debug)] +pub enum Error { + /// Pending Token + #[error("Token already pending for another update")] + Pending, + /// Already spent + #[error("Token already spent")] + AlreadySpent, + /// Invalid transition + #[error("Invalid transition: From {0} to {1}")] + InvalidTransition(State, State), +} + +#[inline] +/// Check if the state transition is allowed +pub fn check_state_transition(current_state: State, new_state: State) -> Result<(), Error> { + let is_valid_transition = match current_state { + State::Unspent => matches!( + new_state, + State::Pending | State::Reserved | State::PendingSpent | State::Spent + ), + State::Pending => matches!(new_state, State::Unspent | State::Spent), + State::Reserved => matches!(new_state, State::Pending | State::Unspent), + State::PendingSpent => matches!(new_state, State::Unspent | State::Spent), + State::Spent => false, + }; + + if !is_valid_transition { + Err(match current_state { + State::Pending => Error::Pending, + State::Spent => Error::AlreadySpent, + _ => Error::InvalidTransition(current_state, new_state), + }) + } else { + Ok(()) + } +} diff --git a/crates/cdk-redb/Cargo.toml b/crates/cdk-redb/Cargo.toml index 7454f96d..297ced30 100644 --- a/crates/cdk-redb/Cargo.toml +++ b/crates/cdk-redb/Cargo.toml @@ -19,7 +19,7 @@ auth = ["cdk-common/auth"] [dependencies] async-trait.workspace = true -cdk-common.workspace = true +cdk-common = { workspace = true, features = ["test"] } redb = "2.4.0" thiserror.workspace = true tracing.workspace = true diff --git a/crates/cdk-redb/src/mint/mod.rs b/crates/cdk-redb/src/mint/mod.rs index 6b563909..c537affe 100644 --- a/crates/cdk-redb/src/mint/mod.rs +++ b/crates/cdk-redb/src/mint/mod.rs @@ -15,6 +15,7 @@ use cdk_common::database::{ use cdk_common::dhke::hash_to_curve; use cdk_common::mint::{self, MintKeySetInfo, MintQuote}; use cdk_common::nut00::ProofsMethods; +use cdk_common::state::check_state_transition; use cdk_common::util::unix_time; use cdk_common::{ BlindSignature, CurrencyUnit, Id, MeltBolt11Request, MeltQuoteState, MintInfo, MintQuoteState, @@ -787,21 +788,19 @@ impl MintProofsDatabase for MintRedbDatabase { for y in ys { let current_state = match table.get(y.to_bytes()).map_err(Error::from)? { Some(state) => { - Some(serde_json::from_str(state.value()).map_err(Error::from)?) + let current_state = + serde_json::from_str(state.value()).map_err(Error::from)?; + check_state_transition(current_state, proofs_state)?; + Some(current_state) } None => None, }; + states.push(current_state); } } } - // Check if any proofs are spent - if states.contains(&Some(State::Spent)) { - write_txn.abort().map_err(Error::from)?; - return Err(database::Error::AttemptUpdateSpentProof); - } - { let mut table = write_txn .open_table(PROOFS_STATE_TABLE) @@ -1007,7 +1006,7 @@ impl MintDatabase for MintRedbDatabase { #[cfg(test)] mod tests { use cdk_common::secret::Secret; - use cdk_common::{Amount, SecretKey}; + use cdk_common::{mint_db_test, Amount, SecretKey}; use tempfile::tempdir; use super::*; @@ -1136,4 +1135,12 @@ mod tests { assert_eq!(states[0], Some(State::Spent)); assert_eq!(states[1], Some(State::Unspent)); } + + async fn provide_db() -> MintRedbDatabase { + let tmp_dir = tempdir().unwrap(); + + MintRedbDatabase::new(&tmp_dir.path().join("mint.redb")).unwrap() + } + + mint_db_test!(provide_db); } diff --git a/crates/cdk-sqlite/Cargo.toml b/crates/cdk-sqlite/Cargo.toml index ccdd3018..9a2d6f42 100644 --- a/crates/cdk-sqlite/Cargo.toml +++ b/crates/cdk-sqlite/Cargo.toml @@ -20,11 +20,11 @@ sqlcipher = ["libsqlite3-sys"] [dependencies] async-trait.workspace = true -cdk-common.workspace = true +cdk-common = { workspace = true, features = ["test"] } bitcoin.workspace = true sqlx = { version = "0.7.4", default-features = false, features = [ "runtime-tokio-rustls", - "sqlite", + "sqlite", "macros", "migrate", "uuid", diff --git a/crates/cdk-sqlite/src/mint/mod.rs b/crates/cdk-sqlite/src/mint/mod.rs index 6732b7ed..746d29b9 100644 --- a/crates/cdk-sqlite/src/mint/mod.rs +++ b/crates/cdk-sqlite/src/mint/mod.rs @@ -15,6 +15,7 @@ use cdk_common::mint::{self, MintKeySetInfo, MintQuote}; use cdk_common::nut00::ProofsMethods; use cdk_common::nut05::QuoteState; use cdk_common::secret::Secret; +use cdk_common::state::check_state_transition; use cdk_common::util::unix_time; use cdk_common::{ Amount, BlindSignature, BlindSignatureDleq, CurrencyUnit, Id, MeltBolt11Request, @@ -1311,10 +1312,8 @@ WHERE keyset_id=?; let states = current_states.values().collect::>(); - if states.contains(&State::Spent) { - transaction.rollback().await.map_err(Error::from)?; - tracing::warn!("Attempted to update state of spent proof"); - return Err(database::Error::AttemptUpdateSpentProof); + for state in states { + check_state_transition(*state, proofs_state)?; } // If no proofs are spent, proceed with update @@ -1843,7 +1842,7 @@ fn sqlite_row_to_melt_request( #[cfg(test)] mod tests { use cdk_common::mint::MintKeySetInfo; - use cdk_common::Amount; + use cdk_common::{mint_db_test, Amount}; use super::*; @@ -1985,4 +1984,10 @@ mod tests { assert_eq!(states[0], Some(State::Spent)); assert_eq!(states[1], Some(State::Unspent)); } + + async fn provide_db() -> MintSqliteDatabase { + memory::empty().await.unwrap() + } + + mint_db_test!(provide_db); } diff --git a/crates/cdk-sqlite/src/wallet/mod.rs b/crates/cdk-sqlite/src/wallet/mod.rs index 2d7460a6..b5a1199f 100644 --- a/crates/cdk-sqlite/src/wallet/mod.rs +++ b/crates/cdk-sqlite/src/wallet/mod.rs @@ -1116,7 +1116,7 @@ mod tests { .await .unwrap(); - db.migrate().await; + db.migrate().await.unwrap(); let mint_info = MintInfo::new().description("test"); let mint_url = MintUrl::from_str("https://mint.xyz").unwrap(); From 81a6d68ef3075f2753664b110c9ffbc5f332f539 Mon Sep 17 00:00:00 2001 From: Cesar Rodas Date: Sun, 20 Apr 2025 11:17:03 -0300 Subject: [PATCH 2/4] Remove wallet states from `check_state_transition` --- crates/cdk-common/src/state.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/cdk-common/src/state.rs b/crates/cdk-common/src/state.rs index f514808b..3c0ac6fb 100644 --- a/crates/cdk-common/src/state.rs +++ b/crates/cdk-common/src/state.rs @@ -25,9 +25,9 @@ pub fn check_state_transition(current_state: State, new_state: State) -> Result< State::Pending | State::Reserved | State::PendingSpent | State::Spent ), State::Pending => matches!(new_state, State::Unspent | State::Spent), - State::Reserved => matches!(new_state, State::Pending | State::Unspent), - State::PendingSpent => matches!(new_state, State::Unspent | State::Spent), - State::Spent => false, + // Any other state shouldn't be updated by the mint, and the wallet does not use this + // function + _ => false, }; if !is_valid_transition { From 5505f0b5d70b8afb118eb3ff7cacb4603be2983d Mon Sep 17 00:00:00 2001 From: C Date: Tue, 22 Apr 2025 09:30:21 -0400 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: thesimplekid --- crates/cdk-common/src/lib.rs | 1 + crates/cdk-common/src/state.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/cdk-common/src/lib.rs b/crates/cdk-common/src/lib.rs index e0732165..3dec0a8d 100644 --- a/crates/cdk-common/src/lib.rs +++ b/crates/cdk-common/src/lib.rs @@ -16,6 +16,7 @@ pub mod mint; #[cfg(feature = "mint")] pub mod payment; pub mod pub_sub; +#[cfg(feature = "mint")] pub mod state; pub mod subscription; #[cfg(feature = "wallet")] diff --git a/crates/cdk-common/src/state.rs b/crates/cdk-common/src/state.rs index 3c0ac6fb..4833ec04 100644 --- a/crates/cdk-common/src/state.rs +++ b/crates/cdk-common/src/state.rs @@ -22,7 +22,7 @@ pub fn check_state_transition(current_state: State, new_state: State) -> Result< let is_valid_transition = match current_state { State::Unspent => matches!( new_state, - State::Pending | State::Reserved | State::PendingSpent | State::Spent + State::Pending | State::Spent ), State::Pending => matches!(new_state, State::Unspent | State::Spent), // Any other state shouldn't be updated by the mint, and the wallet does not use this From 25fad98aa896e187b72ab786e8c634ed21ab0404 Mon Sep 17 00:00:00 2001 From: Cesar Rodas Date: Tue, 22 Apr 2025 10:36:58 -0300 Subject: [PATCH 4/4] Fix formatting --- crates/cdk-common/src/database/mod.rs | 12 ++++++------ crates/cdk-common/src/error.rs | 8 ++++++++ crates/cdk-common/src/state.rs | 5 +---- crates/cdk-sqlite/src/mint/mod.rs | 5 +---- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/crates/cdk-common/src/database/mod.rs b/crates/cdk-common/src/database/mod.rs index c0a67db8..745c8ef3 100644 --- a/crates/cdk-common/src/database/mod.rs +++ b/crates/cdk-common/src/database/mod.rs @@ -16,8 +16,6 @@ pub use mint::{ #[cfg(feature = "wallet")] pub use wallet::Database as WalletDatabase; -use crate::state; - /// CDK_database error #[derive(Debug, thiserror::Error)] pub enum Error { @@ -55,15 +53,17 @@ pub enum Error { /// Invalid keyset #[error("Unknown or invalid keyset")] InvalidKeysetId, + #[cfg(feature = "mint")] /// Invalid state transition #[error("Invalid state transition")] - InvalidStateTransition(state::Error), + InvalidStateTransition(crate::state::Error), } -impl From for Error { - fn from(state: state::Error) -> Self { +#[cfg(feature = "mint")] +impl From for Error { + fn from(state: crate::state::Error) -> Self { match state { - state::Error::AlreadySpent => Error::AttemptUpdateSpentProof, + crate::state::Error::AlreadySpent => Error::AttemptUpdateSpentProof, _ => Error::InvalidStateTransition(state), } } diff --git a/crates/cdk-common/src/error.rs b/crates/cdk-common/src/error.rs index 5e4cd4bc..ea2843c4 100644 --- a/crates/cdk-common/src/error.rs +++ b/crates/cdk-common/src/error.rs @@ -502,6 +502,7 @@ impl From for ErrorResponse { } } +#[cfg(feature = "mint")] impl From for Error { fn from(db_error: crate::database::Error) -> Self { match db_error { @@ -515,6 +516,13 @@ impl From for Error { } } +#[cfg(not(feature = "mint"))] +impl From for Error { + fn from(db_error: crate::database::Error) -> Self { + Self::Database(db_error) + } +} + impl From for Error { fn from(err: ErrorResponse) -> Error { match err.code { diff --git a/crates/cdk-common/src/state.rs b/crates/cdk-common/src/state.rs index 4833ec04..be080001 100644 --- a/crates/cdk-common/src/state.rs +++ b/crates/cdk-common/src/state.rs @@ -20,10 +20,7 @@ pub enum Error { /// Check if the state transition is allowed pub fn check_state_transition(current_state: State, new_state: State) -> Result<(), Error> { let is_valid_transition = match current_state { - State::Unspent => matches!( - new_state, - State::Pending | State::Spent - ), + State::Unspent => matches!(new_state, State::Pending | State::Spent), State::Pending => matches!(new_state, State::Unspent | State::Spent), // Any other state shouldn't be updated by the mint, and the wallet does not use this // function diff --git a/crates/cdk-sqlite/src/mint/mod.rs b/crates/cdk-sqlite/src/mint/mod.rs index 746d29b9..6b106b8c 100644 --- a/crates/cdk-sqlite/src/mint/mod.rs +++ b/crates/cdk-sqlite/src/mint/mod.rs @@ -1962,10 +1962,7 @@ mod tests { // Try to update both proofs - should fail because one is spent let result = db - .update_proofs_states( - &[proofs[0].y().unwrap(), proofs[1].y().unwrap()], - State::Reserved, - ) + .update_proofs_states(&[proofs[0].y().unwrap()], State::Unspent) .await; assert!(result.is_err());