From e22ef667b66c4a2e541a69c966881731e33671c2 Mon Sep 17 00:00:00 2001 From: codaMW Date: Tue, 4 Aug 2026 08:53:25 +0200 Subject: [PATCH] feat(#217): resync trade_key_index to the max recovered index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs #217 (sub-issue of #142). After a restore, the local trade_key_index counter is still at its post-install value while recovered trades already occupy higher indexes, so the next order reuses a trade key already bound to a recovered trade — the daemon rejects the reused index with CantDo(InvalidTradeIndex), and two trades would share a key. When a valid RestoreData is processed, raise trade_key_index to the maximum recovered index across orders and disputes. Monotonic (a restore never rewinds the counter) and idempotent. - identity::ensure_trade_key_index_at_least(floor) bumps the counter to max(current, floor) under the identity write lock; persists with the same discipline as derive_trade_key (rolls back the in-memory bump on a persist failure so a bumped-but-unpersisted counter can't regress on restart and reopen the bug), and requires durable storage on native (web exempt, same rationale as derive_trade_key). - orders::recovered_max_trade_index(info) — the max trade_index over restore_orders and restore_disputes; u32::try_from drops negatives and out-of-range values rather than truncating garbage. None when the restore carried no trades. - Wired into restore_session's Restored arm: resync before returning the info. A resync failure fails the restore rather than returning success with a counter that could hand out a reused key. --- rust/src/api/identity.rs | 104 +++++++++++++++++++++++++++++++++ rust/src/api/orders.rs | 123 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 226 insertions(+), 1 deletion(-) diff --git a/rust/src/api/identity.rs b/rust/src/api/identity.rs index b7e4f60..6db98c1 100644 --- a/rust/src/api/identity.rs +++ b/rust/src/api/identity.rs @@ -399,6 +399,83 @@ async fn derive_trade_key_with( }) } +/// Raise `trade_key_index` to at least `floor`, never lowering it (#217). +/// +/// A restore recovers trades that already occupy trade-key indexes; without +/// this, the next `derive_trade_key()` would hand out an index a recovered +/// trade already owns — reusing a key the daemon has bound. The bump is +/// monotonic: a stale or partial `RestoreData`, or one that arrives after the +/// counter has already advanced, must never rewind it. Idempotent — applying +/// the same recovered set twice changes nothing. +/// +/// Persisted under the same discipline as `derive_trade_key`: if the counter +/// moves, the write must succeed or the call fails, so the advance is durable +/// (a bumped-but-unpersisted counter would regress on the next restart). +pub(crate) async fn ensure_trade_key_index_at_least(floor: u32) -> Result<()> { + let db = crate::db::app_db::db(); + // Same durable-storage precondition as derive_trade_key: on native, refuse + // to advance the counter when there is no store, because the _with core + // would otherwise bump and publish the raised index WITHOUT persisting it + // (the `if let Some(db)` save is skipped) — and publication is best-effort, + // so a session loss would reload a stale pre-resync index and reopen the + // key-reuse bug this closes (#249). + #[cfg(not(target_arch = "wasm32"))] + require_durable_storage(db)?; + // Web is exempt for the same reason derive_trade_key is: `init_db` is never + // called there and IndexedDB has no save_identity yet, so the published + // index is web's durable record via the Flutter mirror until #233 lands. + #[cfg(target_arch = "wasm32")] + if db.is_none() { + log::warn!( + "[identity] no local store on web — the resynced trade-key counter is durable only through the Flutter mirror" + ); + } + ensure_trade_key_index_at_least_with(db, trade_key_index_tx(), floor).await +} + +/// Testable core of [`ensure_trade_key_index_at_least`]: takes an explicit store +/// and publish channel so tests can inject a failing store and a private channel, +/// mirroring `derive_trade_key` / `derive_trade_key_with`. +async fn ensure_trade_key_index_at_least_with( + db: Option<&S>, + tx: &broadcast::Sender, + floor: u32, +) -> Result<()> { + let mut guard = identity_lock().write().await; + let state = guard.as_mut().ok_or_else(|| anyhow!("NoIdentity"))?; + let current = state.identity_info.trade_key_index; + let raised = current.max(floor); + if raised == current { + // Already ahead of (or level with) the recovered set — no-op, no write. + return Ok(()); + } + state.identity_info.trade_key_index = raised; + if let Some(db) = db { + if let Err(e) = db.save_identity(&state.identity_info).await { + // Roll back the in-memory bump on a failed persist. Without this, a + // retried restore with the same floor would see `raised == current`, + // take the no-op short-circuit above, and return Ok(()) WITHOUT ever + // re-attempting the write — silently leaving the durable counter + // un-raised and reopening the key-reuse bug this closes. (Unlike + // derive_trade_key_with, which safely keeps its forward mutation + // because it has no idempotency short-circuit to defeat.) + state.identity_info.trade_key_index = current; + return Err(anyhow!( + "StorageError: failed to persist resynced trade_key_index {raised}: {e}" + )); + } + } + // Only after the primary record is durable: mirror to secure storage the + // same way derive_trade_key_with does, so a later loss of mostro.db still + // reloads the resynced counter rather than a stale pre-restore index (#249). + publish_index(tx, raised); + crate::api::logging::blog_info( + "restore", + format!("trade_key_index resynced {current} -> {raised} from recovered trades"), + ); + Ok(()) +} + /// Re-derive an existing trade key by index. pub async fn get_trade_key(index: u32) -> Result { let guard = identity_lock().read().await; @@ -707,6 +784,33 @@ mod tests { let current = get_identity().await.unwrap().unwrap(); assert_eq!(current.trade_key_index, 22); + // #217 resync — asserted here (not a separate #[tokio::test]) so it + // shares the single identity_lock lifecycle and can't race it. Uses the + // `_with` core so publications land on this test's private channel. + // Never lowers: a floor below current is a no-op — no write, no publish. + ensure_trade_key_index_at_least_with(Some(&db), &tx, 10).await.unwrap(); + assert_eq!(get_identity().await.unwrap().unwrap().trade_key_index, 22); + assert!( + published.rx.try_recv().is_err(), + "a no-op resync must not publish", + ); + // Raises to the recovered max, persists, and publishes to the mirror. + ensure_trade_key_index_at_least_with(Some(&db), &tx, 50).await.unwrap(); + assert_eq!(get_identity().await.unwrap().unwrap().trade_key_index, 50); + assert_eq!(published.next().await.unwrap(), 50); + assert_eq!(db.get_identity().await.unwrap().unwrap().trade_key_index, 50); + // Idempotent: the same floor again changes nothing and publishes nothing. + ensure_trade_key_index_at_least_with(Some(&db), &tx, 50).await.unwrap(); + assert_eq!(get_identity().await.unwrap().unwrap().trade_key_index, 50); + assert!( + published.rx.try_recv().is_err(), + "an idempotent resync must not publish again", + ); + // Regression (the bug #217 fixes): the next derived key is FRESH — + // index 51, past every recovered trade — not a reused recovered index. + let after = derive_trade_key_with(Some(&db), &tx).await.unwrap(); + assert_eq!(after.index, 51); + crate::api::logging::forward_log(log::Level::Info, "identity_probe", "before delete"); delete_identity().await.unwrap(); diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index e7e7629..cc927b2 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -3071,6 +3071,35 @@ pub async fn get_trade_role(order_id: String) -> Result Option { + info.restore_orders + .iter() + .map(|o| o.trade_index) + .chain(info.restore_disputes.iter().map(|d| d.trade_index)) + // `filter_map` with `try_from` drops both negatives and any value + // beyond `u32::MAX` — neither is a real trade index, and truncating + // one into a small `u32` could corrupt the counter this exists to + // protect. `u32::MAX` itself is also dropped: it is reserved as the + // terminal index, because storing it as the counter would make the + // next `derive_trade_key` compute `u32::MAX + 1` and overflow (panic + // in debug, wrap to 0 in release — reissuing index 0, the exact + // key-reuse this resync prevents). 4 billion trades is not reachable + // in practice, but the floor must never be a value the counter cannot + // advance past. + .filter_map(|i| u32::try_from(i).ok()) + .filter(|&i| i < u32::MAX) + .max() +} + /// Send a `RestoreSession` to the active daemon and return the user's active /// trades/disputes. Mirrors create_order's send/await, minus the order payload. /// @@ -3143,7 +3172,18 @@ pub async fn restore_session() -> Result Ok(info), + Ok(Ok(DaemonReply::Restored(info))) => { + // #217: raise trade_key_index past every recovered trade before + // returning, so the next derive_trade_key() can't reuse a key a + // recovered trade already owns. Monotonic and idempotent. A persist + // failure fails the restore: an un-resynced counter reopens the + // key-reuse bug this closes, so silent success would be worse than + // a surfaced error the caller can retry. + if let Some(floor) = recovered_max_trade_index(&info) { + crate::api::identity::ensure_trade_key_index_at_least(floor).await?; + } + Ok(info) + } Ok(Ok(DaemonReply::Rejected { reason, message })) => { crate::api::logging::blog_warn("orders", format!( "restore_session rejected: {reason} — {message}" @@ -3187,6 +3227,55 @@ mod tests { ); } + // ── #217 recovered_max_trade_index ──────────────────────────────────────── + fn restored_order(trade_index: i64) -> mostro_core::message::RestoredOrdersInfo { + mostro_core::message::RestoredOrdersInfo { + order_id: uuid::Uuid::new_v4(), + trade_index, + status: "active".to_string(), + } + } + + fn restored_dispute(trade_index: i64) -> mostro_core::message::RestoredDisputesInfo { + mostro_core::message::RestoredDisputesInfo { + dispute_id: uuid::Uuid::new_v4(), + order_id: uuid::Uuid::new_v4(), + trade_index, + status: "initiated".to_string(), + initiator: None, + solver_pubkey: None, + } + } + + fn restore_info( + orders: Vec, + disputes: Vec, + ) -> mostro_core::message::RestoreSessionInfo { + mostro_core::message::RestoreSessionInfo { + restore_orders: orders.into_iter().map(restored_order).collect(), + restore_disputes: disputes.into_iter().map(restored_dispute).collect(), + } + } + + #[test] + fn recovered_max_is_none_when_nothing_was_restored() { + assert_eq!(recovered_max_trade_index(&restore_info(vec![], vec![])), None); + } + + #[test] + fn recovered_max_spans_orders_and_disputes() { + // Max lives in disputes here — the fn must consider both collections. + assert_eq!( + recovered_max_trade_index(&restore_info(vec![3, 7], vec![12, 5])), + Some(12) + ); + // ...and the other way round. + assert_eq!( + recovered_max_trade_index(&restore_info(vec![40, 9], vec![2])), + Some(40) + ); + } + #[test] fn a_dispute_message_without_a_peer_payload_yields_no_solver() { use mostro_core::message::Payload; @@ -3198,6 +3287,38 @@ mod tests { assert_eq!(admin_pubkey_from_payload(Some(&Payload::Amount(42))), None); } + #[test] + fn recovered_max_drops_negative_and_out_of_range_indexes() { + // A negative index is not a real trade index — dropped, not counted. + assert_eq!( + recovered_max_trade_index(&restore_info(vec![-1, 8], vec![-99])), + Some(8) + ); + // Beyond u32::MAX: dropped rather than truncated into a small counter. + let huge = i64::from(u32::MAX) + 1; + assert_eq!( + recovered_max_trade_index(&restore_info(vec![huge, 4], vec![])), + Some(4) + ); + // u32::MAX itself is dropped — reserved as the terminal index, since + // storing it would make the next derive_trade_key overflow on +1. + let terminal = i64::from(u32::MAX); + assert_eq!( + recovered_max_trade_index(&restore_info(vec![terminal, 4], vec![])), + Some(4) + ); + // Only u32::MAX present -> None (no safe floor to resync to). + assert_eq!( + recovered_max_trade_index(&restore_info(vec![terminal], vec![])), + None + ); + // All invalid -> None (nothing safe to resync to). + assert_eq!( + recovered_max_trade_index(&restore_info(vec![-1], vec![huge])), + None + ); + } + fn insert_pending_create(key: &str, request_id: u64) -> tokio::sync::oneshot::Receiver { let (tx, rx) = tokio::sync::oneshot::channel::(); pending_requests().lock().unwrap().insert(