diff --git a/src/app/trade_pubkey.rs b/src/app/trade_pubkey.rs index 7c932639..5e8d4860 100644 --- a/src/app/trade_pubkey.rs +++ b/src/app/trade_pubkey.rs @@ -1,7 +1,7 @@ use crate::app::context::AppContext; +use crate::db::cas_rotate_maker_trade_pubkey; use crate::util::{enqueue_order_msg, get_order}; -use mostro_core::db::Crud; use mostro_core::order::Kind as OrderKind; use mostro_core::prelude::*; @@ -14,6 +14,9 @@ use mostro_core::prelude::*; /// maker-side trade pubkey and `creator_pubkey` (which tracks the maker's /// current trade key) are set to `event.sender`. Anything else is rejected /// with [`ServiceError::InvalidPubkey`] and the order is left untouched. +/// +/// The rotation is persisted with a status-guarded compare-and-swap and the +/// confirmation is sent only once that write lands. pub async fn trade_pubkey_action( ctx: &AppContext, msg: Message, @@ -23,7 +26,7 @@ pub async fn trade_pubkey_action( // Get request id let request_id = msg.get_inner_message_kind().request_id; // Get order - let mut order = get_order(&msg, pool).await?; + let order = get_order(&msg, pool).await?; // Phase 1.5: accept both `Pending` and `WaitingTakerBond` as // pre-trade entry points. The trade pubkey is a maker-only piece of @@ -56,15 +59,37 @@ pub async fn trade_pubkey_action( if maker_master_key != event.identity { return Err(MostroInternalErr(ServiceError::InvalidPubkey)); } - match kind { - OrderKind::Sell => order.seller_pubkey = Some(event.sender.to_string()), - OrderKind::Buy => order.buyer_pubkey = Some(event.sender.to_string()), + // Persist through the pre-trade compare-and-swap (#866): it moves the + // maker-side trade pubkey and `creator_pubkey` — the maker is the order + // creator, and `creator_pubkey` must never move for anyone else — and + // nothing else, only while the order is still pre-trade. + // + // A full-row `Crud::update` here would write this handler's snapshot + // over whatever committed since the read at the top. The window is + // real: the post-bond resume keeps the order pre-trade across an LND + // `create_hold_invoice` round trip, so a rotation racing it could + // revert the committed take to `pending` with `hash`/`preimage` + // NULLed, orphaning a hold invoice the seller had already paid. + let rotated = + cas_rotate_maker_trade_pubkey(pool, order.id, kind, &event.sender.to_string()).await?; + if !rotated { + // The order left the pre-trade window while we were validating. + // Log it: this is the only trace the race leaves behind, and it is + // what tells us in production that a rotation and a take collided. + // `NotAllowedByStatus` matches the other CAS-miss sites + // (`take_sell.rs`, `show_hold_invoice`, `show_cashu_escrow_request`); + // the pre-check above keeps `InvalidOrderStatus`, the reason every + // pre-check in the repo reports for a status it can see up front. + tracing::info!( + "trade pubkey rotation: order {} left the pre-trade window before the CAS — refusing the rotation", + order.id + ); + return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); } - // The maker is the order creator: `creator_pubkey` tracks the maker's - // current trade key and must never move for anyone else. - order.creator_pubkey = event.sender.to_string(); - // We a message to the seller + // Confirm only once the rotation is durable (#811): announcing it + // ahead of the write leaves the maker signing with a key the daemon + // never stored. enqueue_order_msg( request_id, Some(order.id), @@ -75,11 +100,6 @@ pub async fn trade_pubkey_action( ) .await; - order - .update(pool) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - Ok(()) } @@ -87,6 +107,7 @@ pub async fn trade_pubkey_action( mod tests { use super::*; use crate::app::context::test_utils::{test_settings, TestContextBuilder}; + use mostro_core::db::Crud; use nostr_sdk::prelude::{Keys, PublicKey, Timestamp}; use sqlx::SqlitePool; use std::sync::Arc; @@ -353,4 +374,135 @@ mod tests { assert_eq!(after.buyer_pubkey, Some(maker.public_key().to_string())); assert_eq!(after.seller_pubkey, Some(taker.public_key().to_string())); } + + /// #811: the confirmation must never precede the persist. A rotation + /// the handler rejects announces nothing to the caller. + /// + /// This is the *pre-check* rejection: the status is already out of the + /// pre-trade window when the handler reads the order, so it never + /// reaches the CAS. The CAS-miss branch is covered by + /// `trade_pubkey_action_does_not_confirm_a_cas_miss` below. + #[tokio::test] + async fn trade_pubkey_action_does_not_confirm_a_rejected_rotation() { + let ctx = setup_ctx().await; + let maker = Keys::generate(); + let sender = Keys::generate().public_key(); + + // Past the pre-trade window: the status gate rejects. + let order = maker_order(OrderKind::Sell, Status::WaitingPayment, &maker); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = trade_pubkey_event(sender, maker.public_key()); + let result = trade_pubkey_action(&ctx, trade_pubkey_msg(order.id), &event).await; + + assert!(matches!( + result, + Err(MostroCantDo(CantDoReason::InvalidOrderStatus)) + )); + // The queue is process-global; filter by this test's unique key. + let confirmations = crate::config::MESSAGE_QUEUES + .queue_order_msg + .read() + .await + .iter() + .filter(|(m, pk)| { + *pk == sender && m.get_inner_message_kind().action == Action::TradePubkey + }) + .count(); + assert_eq!( + confirmations, 0, + "a rejected rotation must not be confirmed" + ); + } + + /// The other half of #811, and the only handler-level coverage of the + /// `!rotated` branch: the order is still pre-trade when the handler + /// reads it, so validation passes and the CAS runs — but a concurrent + /// writer has moved the row out of the pre-trade window in between, so + /// the guarded `UPDATE` matches nothing. + /// + /// That interleaving has no injection point between `get_order` and the + /// CAS, so a `BEFORE UPDATE` trigger stands in for the racing writer: + /// `RAISE(IGNORE)` skips the row, which is exactly what the handler sees + /// from a status guard that no longer matches — `rows_affected() == 0`. + /// The handler must reject and stay silent; the row must be untouched. + #[tokio::test] + async fn trade_pubkey_action_does_not_confirm_a_cas_miss() { + let ctx = setup_ctx().await; + let maker = Keys::generate(); + let sender = Keys::generate().public_key(); + + let order = maker_order(OrderKind::Sell, Status::Pending, &maker); + let order = order.create(ctx.pool()).await.unwrap(); + + sqlx::query( + "CREATE TRIGGER rotation_loses_the_cas BEFORE UPDATE ON orders \ + WHEN NEW.creator_pubkey <> OLD.creator_pubkey \ + BEGIN SELECT RAISE(IGNORE); END", + ) + .execute(ctx.pool()) + .await + .unwrap(); + + let event = trade_pubkey_event(sender, maker.public_key()); + let result = trade_pubkey_action(&ctx, trade_pubkey_msg(order.id), &event).await; + + assert!( + matches!(result, Err(MostroCantDo(CantDoReason::NotAllowedByStatus))), + "a CAS miss must be reported as NotAllowedByStatus: {result:?}" + ); + + // Nothing moved, and nothing was announced. + let after = order_by_id(ctx.pool(), order.id).await; + assert_eq!(after.creator_pubkey, maker.public_key().to_string()); + assert_eq!(after.seller_pubkey, Some(maker.public_key().to_string())); + // The queue is process-global; filter by this test's unique key. + let confirmations = crate::config::MESSAGE_QUEUES + .queue_order_msg + .read() + .await + .iter() + .filter(|(m, pk)| { + *pk == sender && m.get_inner_message_kind().action == Action::TradePubkey + }) + .count(); + assert_eq!(confirmations, 0, "a CAS miss must not be confirmed"); + } + + /// The rotation is a targeted write: a pre-trade order carrying a + /// promoted taker context and trade escrow material keeps both. The + /// full-row write this replaced would have NULLed them from the + /// handler's own snapshot whenever that snapshot was stale. + #[tokio::test] + async fn trade_pubkey_action_leaves_taker_context_and_escrow_material_untouched() { + let ctx = setup_ctx().await; + let maker = Keys::generate(); + let taker = Keys::generate(); + let new_trade_key = Keys::generate().public_key(); + + let mut order = maker_order(OrderKind::Sell, Status::WaitingTakerBond, &maker); + order.buyer_pubkey = Some(taker.public_key().to_string()); + order.master_buyer_pubkey = Some(taker.public_key().to_string()); + order.hash = Some("aa".repeat(32)); + order.preimage = Some("bb".repeat(32)); + order.taken_at = 1_700_000_000; + let order = order.create(ctx.pool()).await.unwrap(); + + let event = trade_pubkey_event(new_trade_key, maker.public_key()); + let result = trade_pubkey_action(&ctx, trade_pubkey_msg(order.id), &event).await; + assert!(result.is_ok(), "maker rotation must succeed: {result:?}"); + + let after = order_by_id(ctx.pool(), order.id).await; + assert_eq!(after.seller_pubkey, Some(new_trade_key.to_string())); + assert_eq!(after.creator_pubkey, new_trade_key.to_string()); + assert_eq!(after.status, Status::WaitingTakerBond.to_string()); + assert_eq!(after.buyer_pubkey, Some(taker.public_key().to_string())); + assert_eq!( + after.master_buyer_pubkey, + Some(taker.public_key().to_string()) + ); + assert_eq!(after.hash, Some("aa".repeat(32))); + assert_eq!(after.preimage, Some("bb".repeat(32))); + assert_eq!(after.taken_at, 1_700_000_000); + } } diff --git a/src/db.rs b/src/db.rs index 1b3fd117..6df2a29d 100644 --- a/src/db.rs +++ b/src/db.rs @@ -762,6 +762,10 @@ pub async fn cas_promote_taker_context( /// the hold invoice was created) and the status/event pair — in one /// guarded write. /// +/// The maker side is deliberately excluded: only the taker's pubkey, +/// master key and trade index are written, selected by order kind exactly +/// as `cas_promote_taker_context` does. +/// /// The `WHERE` guard always covers the pre-trade statuses /// (`pending` / `waiting-taker-bond`). `allow_waiting_buyer_invoice` /// adds that status for the one caller that legitimately arrives from @@ -784,15 +788,27 @@ pub async fn cas_complete_pretrade_take( } else { "" }; + // Only the taker side, selected by kind — same rule as + // `cas_promote_taker_context`. The maker's pubkey, master key and trade + // index are persisted at order creation and a take never learns new + // values for them, so writing them back from the caller's snapshot can + // only undo someone else's committed write: a maker trade-pubkey + // rotation landing during the `create_hold_invoice` round trip used to + // be reverted here, and only halfway, since `creator_pubkey` was left + // alone — the resulting mismatch fails the + // `seller_pubkey == creator_pubkey` gate in `handle_child_order`. + let kind = order.get_order_kind().map_err(MostroInternalErr)?; + let taker_columns = match kind { + OrderKind::Sell => "buyer_pubkey = ?4, master_buyer_pubkey = ?6, trade_index_buyer = ?8", + OrderKind::Buy => "seller_pubkey = ?5, master_seller_pubkey = ?7, trade_index_seller = ?9", + }; let query = format!( - "UPDATE orders SET status = ?2, event_id = ?3, \ - buyer_pubkey = ?4, seller_pubkey = ?5, \ - master_buyer_pubkey = ?6, master_seller_pubkey = ?7, \ - trade_index_buyer = ?8, trade_index_seller = ?9, \ + "UPDATE orders SET status = ?2, event_id = ?3, {taker_columns}, \ fiat_amount = ?10, amount = ?11, fee = ?12, dev_fee = ?13, created_at = ?14, \ buyer_invoice = ?15, preimage = ?16, hash = ?17, taken_at = ?18 \ WHERE id = ?1 AND status IN ({PRETRADE_STATUSES}{extra_status})" ); + // The slots the unselected side would have used stay bound but unreferenced. let result = sqlx::query(AssertSqlSafe(query.as_str())) .bind(order.id) .bind(order.status.clone()) @@ -819,6 +835,56 @@ pub async fn cas_complete_pretrade_take( Ok(result.rows_affected() > 0) } +/// Compare-and-swap the maker's per-trade pubkey rotation +/// (see `trade_pubkey_action`). Writes only the maker-side trade pubkey +/// and `creator_pubkey` — never the status, the taker context or the +/// escrow material — and only while the order is still pre-trade. +/// +/// The rotation handler reads the order, validates maker ownership and +/// then persists; a take (or the post-bond resume, whose +/// `create_hold_invoice` round trip keeps the order pre-trade for +/// seconds) can commit in between. A full-row write from the handler's +/// stale snapshot would revert that take — status back to `pending`, +/// `hash`/`preimage` NULLed, the seller's paid hold invoice orphaned. +/// Guarding on the pre-trade statuses makes the rotation lose cleanly +/// instead. +/// +/// `maker_trade_pubkey` lands on the maker side fixed by the order kind: +/// the seller of a sell order, the buyer of a buy order. Returns `false` +/// when the CAS missed. +/// +/// Caveat: surviving the row is not the same as surviving the flow. A take +/// already in flight when the rotation commits keeps addressing the maker +/// with the pre-rotation key it read at the top — `PayInvoice`, +/// `WaitingSellerToPay` — because those messages are built from its own +/// snapshot, not re-read after this write. The row ends up correct; the +/// in-flight conversation does not. Tracked in #911. +pub async fn cas_rotate_maker_trade_pubkey( + pool: &SqlitePool, + order_id: Uuid, + kind: OrderKind, + maker_trade_pubkey: &str, +) -> Result { + let maker_column = match kind { + OrderKind::Sell => "seller_pubkey", + OrderKind::Buy => "buyer_pubkey", + }; + // `creator_pubkey` tracks the maker's current trade key, so both + // columns take the same value: bind it once and reuse `?2`. + let query = format!( + "UPDATE orders SET {maker_column} = ?2, creator_pubkey = ?2 \ + WHERE id = ?1 AND status IN ({PRETRADE_STATUSES})" + ); + let result = sqlx::query(AssertSqlSafe(query.as_str())) + .bind(order_id) + .bind(maker_trade_pubkey) + .execute(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + + Ok(result.rows_affected() > 0) +} + /// Atomically persist a validated Cashu escrow and advance the order status /// (Cashu foundation CF-4, `docs/cashu/01-fundamentals.md` §6). /// @@ -2614,6 +2680,102 @@ mod tests { } } + /// The take snapshots the order before its `create_hold_invoice` round + /// trip. A maker trade-pubkey rotation committing inside that window + /// must survive the take: the write covers the taker side only. + #[tokio::test] + async fn cas_complete_pretrade_take_preserves_a_maker_key_rotated_meanwhile() { + use mostro_core::db::Crud; + let pool = migrated_pool().await; + // Sell order → the maker is the seller side. + let stale = insert_pretrade_order(&pool, "pending").await; + + let rotated = "ff".repeat(32); + let won = + super::cas_rotate_maker_trade_pubkey(&pool, stale.id, super::OrderKind::Sell, &rotated) + .await + .unwrap(); + assert!( + won, + "the rotation commits while the order is still pre-trade" + ); + + // The take now commits from the snapshot it took before the + // rotation — it still carries the old maker key. + let mut committing = stale.clone(); + committing.status = super::Status::WaitingPayment.to_string(); + committing.event_id = "ev-escrow".to_string(); + committing.buyer_pubkey = Some("bb".repeat(32)); + committing.master_buyer_pubkey = Some("bb".repeat(32)); + committing.trade_index_buyer = Some(3); + committing.hash = Some("ee".repeat(32)); + committing.taken_at = 1_700_000_500; + let won = super::cas_complete_pretrade_take(&pool, &committing, false) + .await + .unwrap(); + assert!(won, "the take still wins the pre-trade window"); + + let after = super::Order::by_id(&pool, stale.id).await.unwrap().unwrap(); + // The take landed in full... + assert_eq!(after.status, "waiting-payment"); + assert_eq!(after.buyer_pubkey, Some("bb".repeat(32))); + assert_eq!(after.master_buyer_pubkey, Some("bb".repeat(32))); + assert_eq!(after.trade_index_buyer, Some(3)); + assert_eq!(after.hash, Some("ee".repeat(32))); + // ...and the rotation survived it, `creator_pubkey` included, so + // the `seller_pubkey == creator_pubkey` gate still holds. + assert_eq!(after.seller_pubkey, Some(rotated.clone())); + assert_eq!(after.creator_pubkey, rotated); + } + + /// Mirror of the above on a buy order, where the maker is the buyer + /// and the take writes the seller side. + #[tokio::test] + async fn cas_complete_pretrade_take_preserves_a_rotated_buy_maker_key() { + use mostro_core::db::Crud; + let pool = migrated_pool().await; + let stale = super::Order { + id: uuid::Uuid::new_v4(), + kind: "buy".to_string(), + status: "pending".to_string(), + creator_pubkey: "aa".repeat(32), + buyer_pubkey: Some("aa".repeat(32)), + master_buyer_pubkey: Some("aa".repeat(32)), + fiat_code: "USD".to_string(), + payment_method: "bank".to_string(), + amount: 100_000, + fiat_amount: 100, + ..Default::default() + }; + let stale = stale.create(&pool).await.unwrap(); + + let rotated = "ff".repeat(32); + assert!(super::cas_rotate_maker_trade_pubkey( + &pool, + stale.id, + super::OrderKind::Buy, + &rotated + ) + .await + .unwrap()); + + let mut committing = stale.clone(); + committing.status = super::Status::WaitingPayment.to_string(); + committing.event_id = "ev-escrow".to_string(); + committing.seller_pubkey = Some("bb".repeat(32)); + committing.master_seller_pubkey = Some("bb".repeat(32)); + assert!(super::cas_complete_pretrade_take(&pool, &committing, false) + .await + .unwrap()); + + let after = super::Order::by_id(&pool, stale.id).await.unwrap().unwrap(); + assert_eq!(after.status, "waiting-payment"); + assert_eq!(after.seller_pubkey, Some("bb".repeat(32))); + assert_eq!(after.master_seller_pubkey, Some("bb".repeat(32))); + assert_eq!(after.buyer_pubkey, Some(rotated.clone())); + assert_eq!(after.creator_pubkey, rotated); + } + #[tokio::test] async fn cas_promote_taker_context_persists_taker_context_and_take_time() { let pool = migrated_pool().await; @@ -2688,6 +2850,100 @@ mod tests { assert_ne!(after.event_id, "ev-escrow"); } + #[tokio::test] + async fn cas_rotate_maker_trade_pubkey_moves_only_the_seller_side_and_creator() { + use mostro_core::db::Crud; + let pool = migrated_pool().await; + // Both pre-trade entry points the rotation handler accepts. + for status in ["pending", "waiting-taker-bond"] { + let mut order = insert_pretrade_order(&pool, status).await; + // A taker context the rotation must leave alone: the bond flow + // can promote it onto a still-pre-trade row. + order.buyer_pubkey = Some("bb".repeat(32)); + order.master_buyer_pubkey = Some("bb".repeat(32)); + order.taken_at = 1_700_000_500; + let order = order.update(&pool).await.unwrap(); + + let fresh = "ff".repeat(32); + let won = super::cas_rotate_maker_trade_pubkey( + &pool, + order.id, + super::OrderKind::Sell, + &fresh, + ) + .await + .unwrap(); + assert!(won, "{status} is inside the pre-trade window"); + + let after = super::Order::by_id(&pool, order.id).await.unwrap().unwrap(); + assert_eq!(after.seller_pubkey, Some(fresh.clone())); + assert_eq!(after.creator_pubkey, fresh); + // Everything else is untouched — above all the escrow material + // and the taker context a full-row write used to clobber. + assert_eq!(after.status, status); + assert_eq!(after.buyer_pubkey, Some("bb".repeat(32))); + assert_eq!(after.master_buyer_pubkey, Some("bb".repeat(32))); + assert_eq!(after.hash, Some("cc".repeat(32))); + assert_eq!(after.preimage, Some("dd".repeat(32))); + assert_eq!(after.taken_at, 1_700_000_500); + } + } + + #[tokio::test] + async fn cas_rotate_maker_trade_pubkey_moves_the_buyer_side_on_a_buy_order() { + use mostro_core::db::Crud; + let pool = migrated_pool().await; + let order = super::Order { + id: uuid::Uuid::new_v4(), + kind: "buy".to_string(), + status: "pending".to_string(), + creator_pubkey: "aa".repeat(32), + buyer_pubkey: Some("aa".repeat(32)), + fiat_code: "USD".to_string(), + payment_method: "bank".to_string(), + amount: 100_000, + fiat_amount: 100, + ..Default::default() + }; + let order = order.create(&pool).await.unwrap(); + + let fresh = "ff".repeat(32); + let won = + super::cas_rotate_maker_trade_pubkey(&pool, order.id, super::OrderKind::Buy, &fresh) + .await + .unwrap(); + assert!(won); + + let after = super::Order::by_id(&pool, order.id).await.unwrap().unwrap(); + assert_eq!(after.buyer_pubkey, Some(fresh.clone())); + assert_eq!(after.creator_pubkey, fresh); + assert_eq!(after.seller_pubkey, None); + } + + #[tokio::test] + async fn cas_rotate_maker_trade_pubkey_loses_once_the_take_committed() { + use mostro_core::db::Crud; + let pool = migrated_pool().await; + // The window the finding turns on: the take committed + // `waiting-payment` with the escrow material while the rotation was + // in flight. The stale rotation must miss, not revert it. + let order = insert_pretrade_order(&pool, "waiting-payment").await; + + let fresh = "ff".repeat(32); + let won = + super::cas_rotate_maker_trade_pubkey(&pool, order.id, super::OrderKind::Sell, &fresh) + .await + .unwrap(); + assert!(!won, "waiting-payment is past the pre-trade window"); + + let after = super::Order::by_id(&pool, order.id).await.unwrap().unwrap(); + assert_eq!(after.status, "waiting-payment"); + assert_eq!(after.seller_pubkey, Some("aa".repeat(32))); + assert_eq!(after.creator_pubkey, "aa".repeat(32)); + assert_eq!(after.hash, Some("cc".repeat(32))); + assert_eq!(after.preimage, Some("dd".repeat(32))); + } + #[tokio::test] async fn test_find_escrow_deadline_orders_selects_only_due_escrows() { async fn insert_order(pool: &SqlitePool, status: &str, held_at: i64, with_hash: bool) {