From fe46cd30aed5d1769f8fd854d8a537af96687f12 Mon Sep 17 00:00:00 2001 From: Forte11Cuba Date: Fri, 28 Aug 2026 00:45:44 -0600 Subject: [PATCH 1/6] feat(#328): source restore trade-key resync from Action::LastTradeIndex --- rust/src/api/orders.rs | 236 +++++++++++++++++++++++++++++++++---- rust/src/mostro/actions.rs | 23 +++- 2 files changed, 237 insertions(+), 22 deletions(-) diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index 562f241e..4e8cee49 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -3398,23 +3398,33 @@ pub async fn get_trade_role(order_id: String) -> Result= u32::MAX` is not a +/// real trade index: negatives are nonsense, and `u32::MAX` is the reserved +/// terminal index — 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 the resync +/// prevents). Such a value is dropped rather than truncated into the counter. +fn sanitize_trade_index(i: i64) -> Option { + u32::try_from(i).ok().filter(|&v| v < u32::MAX) +} + /// Highest trade-key index across all recovered orders and disputes (#217). /// /// The counter must be raised to this so the next `derive_trade_key()` cannot /// hand out an index a recovered trade already owns. Returns `None` when the -/// restore carried no trades (nothing to resync to). Indexes are `i64` on the -/// wire; a value that is negative or beyond `u32::MAX` is not a real trade -/// index, so it is dropped rather than truncated into the counter. +/// restore carried no trades (nothing to resync to). +/// +/// NOTE (#328): this is only a *lower bound* of the daemon's real counter — +/// the restore payload lists only non-finalized orders, so a finalized trade +/// holding a higher index is invisible here. `restore_session` sources its +/// resync floor from `last_trade_index()` (authoritative) and falls back to +/// this only when the daemon does not answer. fn recovered_max_trade_index( info: &mostro_core::message::RestoreSessionInfo, ) -> Option { - // A single adapter drops both negatives and any value >= 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 dropped: it is the - // reserved terminal index, and 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). - // Collapsed into one filter_map so the two conditions can't drift apart. let all: Vec = info .restore_orders .iter() @@ -3422,10 +3432,7 @@ fn recovered_max_trade_index( .chain(info.restore_disputes.iter().map(|d| d.trade_index)) .collect(); let total = all.len(); - let valid: Vec = all - .into_iter() - .filter_map(|i| u32::try_from(i).ok().filter(|&v| v < u32::MAX)) - .collect(); + let valid: Vec = all.iter().copied().filter_map(sanitize_trade_index).collect(); // A dropped index is not just an odd value: it means the daemon sent // something this client's model does not cover, and a silently-lowered // floor produces a later CantDo(InvalidTradeIndex) with no breadcrumb. Warn @@ -3445,6 +3452,140 @@ fn recovered_max_trade_index( valid.into_iter().max() } +/// Pick the resync floor from the authoritative daemon counter, falling back to +/// the restore-payload maximum (#328). +/// +/// The daemon's `LastTradeIndex` answer (`daemon_counter`) wins whenever it is +/// present — it is the real high-water mark, including finalized trades. The +/// payload maximum is only a lower bound (the restore lists non-finalized +/// orders only) and is used solely when the daemon did not answer. Raising is +/// monotonic downstream (`ensure_trade_key_index_at_least`), so a low daemon +/// answer can never lower the counter. +fn resync_floor( + daemon_counter: Option, + info: &mostro_core::message::RestoreSessionInfo, +) -> Option { + daemon_counter.or_else(|| recovered_max_trade_index(info)) +} + +/// Ask the daemon for its authoritative last-known trade index (#328). +/// +/// `Action::LastTradeIndex` is account-scoped: the request is signed with the +/// identity keys for BOTH the Seal and the rumor (see +/// `actions::last_trade_index`), so it derives NO trade key and the daemon +/// resolves the account by sender pubkey. The reply is a kind-14 addressed to +/// the identity pubkey, carrying the counter in `MessageKind::trade_index`. +/// +/// Returns `Ok(Some(idx))` with the sanitized counter, or `Ok(None)` when the +/// daemon does not answer within the timeout or the reply carries no usable +/// index — the caller then falls back to `recovered_max_trade_index`. +/// +/// This is a self-contained request/reply (own subscription + inline wait, like +/// mostro-cli's `wait_for_dm`) rather than a reuse of the per-trade +/// subscription/dispatch path: that path derives its recipient keys from a +/// trade index and is coupled to trade keys, but this reply is encrypted to the +/// identity key. +async fn last_trade_index() -> Result> { + use nostr_sdk::RelayPoolNotification; + use crate::rt::time::{timeout, Duration}; + + let identity_keys = crate::api::identity::get_active_keys().await?; + let identity_pk = identity_keys.public_key(); + let identity_pk_hex = identity_pk.to_hex(); + let mostro_pubkey = nostr_sdk::PublicKey::from_hex(&active_mostro_pubkey())?; + + let pool = crate::api::nostr::get_pool()?; + let client = pool.client(); + + // Grab the notifications receiver BEFORE subscribing so the reply can't + // arrive in the gap between subscribe and the first recv. + let mut rx = client.notifications(); + + // limit(0): live-only, same rationale as subscribe_daemon_messages — the + // reply is published after we subscribe, and we never want a replayed + // historical LastTradeIndex to resolve this request. + let filter = nostr_sdk::Filter::new() + .kind(nostr_sdk::Kind::PrivateDirectMessage) + .author(mostro_pubkey) + .pubkey(identity_pk) + .limit(0); + if let Err(e) = client.subscribe(filter, None).await { + log::warn!("[orders] last_trade_index subscribe failed: {e}"); + return Ok(None); + } + + let event_json = actions::last_trade_index(&identity_keys, &mostro_pubkey).await?; + publish_event_json(&event_json).await?; + crate::api::logging::blog_info( + "restore", + "LastTradeIndex published — waiting for daemon".to_string(), + ); + + // Wait for the reply. 10s matches restore_session's timeout. + let deadline = Duration::from_secs(10); + let start = crate::rt::time::Instant::now(); + loop { + let remaining = deadline.saturating_sub(start.elapsed()); + if remaining.is_zero() { + break; + } + match timeout(remaining, rx.recv()).await { + Ok(Ok(RelayPoolNotification::Event { event, .. })) => { + if event.kind != nostr_sdk::Kind::PrivateDirectMessage + || event.pubkey != mostro_pubkey + { + continue; + } + let is_for_us = event.tags.iter().any(|t| { + let s = t.as_slice(); + s.first().map(|v| v.as_str()) == Some("p") + && s.get(1).map(|v| v.as_str()) == Some(identity_pk_hex.as_str()) + }); + if !is_for_us { + continue; + } + match crate::nostr::transport::unwrap_mostro_message(&identity_keys, &event).await { + Ok(Some(unwrapped)) => { + // Authenticate: the kind-14 author must be the node. + if unwrapped.sender != mostro_pubkey { + continue; + } + let kind = unwrapped.message.get_inner_message_kind(); + if kind.action != mostro_core::message::Action::LastTradeIndex { + continue; + } + let idx = kind.trade_index.and_then(sanitize_trade_index); + crate::api::logging::blog_info("restore", format!( + "LastTradeIndex reply: trade_index={:?} -> floor={idx:?}", + kind.trade_index + )); + return Ok(idx); + } + Ok(None) => continue, + Err(e) => { + log::warn!("[orders] last_trade_index decrypt failed: {e}"); + continue; + } + } + } + Ok(Ok(RelayPoolNotification::Shutdown)) => break, + Ok(Err(broadcast::error::RecvError::Lagged(n))) => { + log::warn!("[orders] last_trade_index lagged by {n} messages"); + continue; + } + Ok(Err(broadcast::error::RecvError::Closed)) => break, + Err(_) => break, // timeout + Ok(Ok(_)) => continue, + } + } + crate::api::logging::blog_warn( + "restore", + "LastTradeIndex: no usable daemon reply — falling back to restore payload max" + .to_string(), + ); + Ok(None) +} + /// 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. /// @@ -3518,13 +3659,29 @@ pub async fn restore_session() -> Result { - // #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) { + // Raise trade_key_index 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. + // + // #328: the authoritative floor is the daemon's LastTradeIndex + // counter. The restore payload lists only non-finalized orders, so + // recovered_max_trade_index is a lower bound (a finalized trade + // holding a higher index is invisible) — kept only as a fallback + // for when the daemon does not answer. + let daemon_counter = match last_trade_index().await { + Ok(idx) => idx, + Err(e) => { + crate::api::logging::blog_warn("restore", format!( + "LastTradeIndex request errored ({e}); \ + falling back to restore payload max" + )); + None + } + }; + if let Some(floor) = resync_floor(daemon_counter, &info) { crate::api::identity::ensure_trade_key_index_at_least(floor).await?; } Ok(info) @@ -3654,6 +3811,43 @@ mod tests { ); } + // ── #328 sanitize_trade_index / resync_floor ───────────────────────────── + #[test] + fn sanitize_trade_index_drops_negative_and_out_of_range() { + assert_eq!(sanitize_trade_index(0), Some(0)); + assert_eq!(sanitize_trade_index(42), Some(42)); + assert_eq!(sanitize_trade_index(-1), None); + // u32::MAX is the reserved terminal index (dropped to avoid +1 overflow). + assert_eq!(sanitize_trade_index(i64::from(u32::MAX)), None); + assert_eq!(sanitize_trade_index(i64::from(u32::MAX) - 1), Some(u32::MAX - 1)); + assert_eq!(sanitize_trade_index(i64::from(u32::MAX) + 1), None); + } + + #[test] + fn resync_floor_prefers_the_daemon_counter_over_the_payload_max() { + // The #328 scenario: order X open at index 1 (the only non-finalized + // trade the restore returns), order Y canceled at index 2. The payload + // max is 1, but the daemon's LastTradeIndex counter is 2 — the real + // high-water mark — and must win, or the first new order collides. + let info = restore_info(vec![1], vec![]); + assert_eq!(resync_floor(Some(2), &info), Some(2)); + // The daemon answer wins even when it is lower than the payload max — + // monotonic raising downstream (ensure_trade_key_index_at_least) makes + // this safe, and the daemon is authoritative. + assert_eq!(resync_floor(Some(1), &restore_info(vec![5], vec![])), Some(1)); + // ...and even when the payload carried nothing at all. + assert_eq!(resync_floor(Some(7), &restore_info(vec![], vec![])), Some(7)); + } + + #[test] + fn resync_floor_falls_back_to_payload_max_when_daemon_is_silent() { + // No LastTradeIndex answer (timeout / error): the restore-payload + // maximum is the best available lower bound. + assert_eq!(resync_floor(None, &restore_info(vec![3, 9], vec![4])), Some(9)); + // Nothing anywhere -> no resync (None). + assert_eq!(resync_floor(None, &restore_info(vec![], vec![])), 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( diff --git a/rust/src/mostro/actions.rs b/rust/src/mostro/actions.rs index ff7e31a5..6f98c65e 100644 --- a/rust/src/mostro/actions.rs +++ b/rust/src/mostro/actions.rs @@ -13,7 +13,7 @@ /// arguments — see `api::identity::get_transport_identity_keys`, which /// applies the runtime privacy toggle. use anyhow::Result; -use mostro_core::message::{Action, Message, Payload}; +use mostro_core::message::{Action, Message, MessageKind, Payload}; use nostr_sdk::prelude::*; use uuid::Uuid; @@ -419,6 +419,27 @@ pub async fn restore_session( wrap_message_first_contact(identity_keys, trade_keys, mostro_pubkey, &msg).await } +/// Build and wrap a `LastTradeIndex` request (#328). +/// +/// Account-scoped: `identity_keys` sign BOTH the Seal and the rumor, so the +/// daemon resolves the account by sender pubkey and NO trade key is derived. +/// The reply carries the counter in `MessageKind::trade_index`. Payload must be +/// `None` (enforced by mostro-core). Mirrors `mostro-cli`'s +/// `execute_last_trade_index`, which signs with `identity_keys` for both. +pub async fn last_trade_index( + identity_keys: &Keys, + mostro_pubkey: &PublicKey, +) -> Result { + let msg = Message::Restore(MessageKind::new( + None, + None, + None, + Action::LastTradeIndex, + None, + )); + wrap_message_first_contact(identity_keys, identity_keys, mostro_pubkey, &msg).await +} + #[cfg(test)] mod tests { use super::*; From 17d3a823003fca517000e74aea63cfd84b618c2e Mon Sep 17 00:00:00 2001 From: Forte11Cuba Date: Fri, 28 Aug 2026 01:06:30 -0600 Subject: [PATCH 2/6] feat(#328): source restore trade-key resync from Action::LastTradeIndex --- rust/src/api/orders.rs | 45 ++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index 4e8cee49..5ade8f79 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -3452,20 +3452,31 @@ fn recovered_max_trade_index( valid.into_iter().max() } -/// Pick the resync floor from the authoritative daemon counter, falling back to -/// the restore-payload maximum (#328). +/// Pick the resync floor as the highest of the daemon counter and the +/// restore-payload maximum (#328). /// -/// The daemon's `LastTradeIndex` answer (`daemon_counter`) wins whenever it is -/// present — it is the real high-water mark, including finalized trades. The -/// payload maximum is only a lower bound (the restore lists non-finalized -/// orders only) and is used solely when the daemon did not answer. Raising is -/// monotonic downstream (`ensure_trade_key_index_at_least`), so a low daemon -/// answer can never lower the counter. +/// The daemon's `LastTradeIndex` answer (`daemon_counter`) is authoritative and +/// is the real high-water mark, including finalized trades. The payload maximum +/// is a proven lower bound — every recovered order carries its own index. +/// +/// Against a consistent daemon the counter is always `>=` the payload maximum: +/// the daemon raises `last_trade_index` to every index it accepts +/// (`update_user_trade_index`) and rejects any index it has already seen, so an +/// order it still returns in the restore payload was necessarily seen at or +/// below the counter. Taking the max is therefore a no-op in practice — kept as +/// cheap defense-in-depth so the floor stays correct independent of that +/// invariant: a stale/partial reply or a daemon bug can never make us resync +/// below a recovered trade's own index and reuse its key. Returns `None` only +/// when neither source has a usable index. fn resync_floor( daemon_counter: Option, info: &mostro_core::message::RestoreSessionInfo, ) -> Option { - daemon_counter.or_else(|| recovered_max_trade_index(info)) + let payload_max = recovered_max_trade_index(info); + match (daemon_counter, payload_max) { + (Some(daemon), Some(payload)) => Some(daemon.max(payload)), + (daemon, payload) => daemon.or(payload), + } } /// Ask the daemon for its authoritative last-known trade index (#328). @@ -3824,18 +3835,18 @@ mod tests { } #[test] - fn resync_floor_prefers_the_daemon_counter_over_the_payload_max() { + fn resync_floor_takes_the_higher_of_daemon_counter_and_payload_max() { // The #328 scenario: order X open at index 1 (the only non-finalized // trade the restore returns), order Y canceled at index 2. The payload // max is 1, but the daemon's LastTradeIndex counter is 2 — the real // high-water mark — and must win, or the first new order collides. - let info = restore_info(vec![1], vec![]); - assert_eq!(resync_floor(Some(2), &info), Some(2)); - // The daemon answer wins even when it is lower than the payload max — - // monotonic raising downstream (ensure_trade_key_index_at_least) makes - // this safe, and the daemon is authoritative. - assert_eq!(resync_floor(Some(1), &restore_info(vec![5], vec![])), Some(1)); - // ...and even when the payload carried nothing at all. + assert_eq!(resync_floor(Some(2), &restore_info(vec![1], vec![])), Some(2)); + // Defense-in-depth: never resync below a recovered trade's own index. + // A consistent daemon cannot answer below an index it still tracks (it + // raised last_trade_index when it accepted that order), so this only + // guards a stale/partial reply — the payload lower bound then wins. + assert_eq!(resync_floor(Some(1), &restore_info(vec![5], vec![])), Some(5)); + // Daemon present, payload empty -> the daemon value. assert_eq!(resync_floor(Some(7), &restore_info(vec![], vec![])), Some(7)); } From 57d0893f388af1c2b12e0ccf5e00b7d13fa06d5d Mon Sep 17 00:00:00 2001 From: Forte11Cuba Date: Fri, 28 Aug 2026 13:00:30 -0600 Subject: [PATCH 3/6] fix(#328): auto-close LastTradeIndex subscription, 5s timeout --- rust/src/api/orders.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index 5ade8f79..e5410795 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -3512,6 +3512,12 @@ async fn last_trade_index() -> Result> { // arrive in the gap between subscribe and the first recv. let mut rx = client.notifications(); + // This query, unlike the restore itself, has a fallback (the payload + // maximum), so a shorter wait halves the worst-case restore latency + // against a silent daemon. Shared by the relay-side auto-close and the + // outer wait loop so the two can't drift. + const REPLY_TIMEOUT: Duration = Duration::from_secs(5); + // limit(0): live-only, same rationale as subscribe_daemon_messages — the // reply is published after we subscribe, and we never want a replayed // historical LastTradeIndex to resolve this request. @@ -3520,7 +3526,17 @@ async fn last_trade_index() -> Result> { .author(mostro_pubkey) .pubkey(identity_pk) .limit(0); - if let Err(e) = client.subscribe(filter, None).await { + // Auto-close the relay-side subscription — this is a one-shot request/reply + // (mostro-cli's wait_for_dm shape), not a long-lived watcher, so the library + // issues the CLOSE on every path: after the reply (WaitForEventsAfterEOSE(1)) + // and, if the daemon never answers, on the timeout. Leaving it to manual + // bookkeeping is the leak class #182/#255 address. Auto-close subs are + // deliberately excluded from reconnect re-subscription (correct here: if the + // socket drops mid-request we fall back to the payload maximum by design). + let close_opts = nostr_sdk::prelude::SubscribeAutoCloseOptions::default() + .exit_policy(nostr_sdk::prelude::ReqExitPolicy::WaitForEventsAfterEOSE(1)) + .timeout(Some(REPLY_TIMEOUT)); + if let Err(e) = client.subscribe(filter, Some(close_opts)).await { log::warn!("[orders] last_trade_index subscribe failed: {e}"); return Ok(None); } @@ -3532,8 +3548,9 @@ async fn last_trade_index() -> Result> { "LastTradeIndex published — waiting for daemon".to_string(), ); - // Wait for the reply. 10s matches restore_session's timeout. - let deadline = Duration::from_secs(10); + // Wait for the reply. Bounded by REPLY_TIMEOUT — the same budget the + // relay-side auto-close uses, so both give up together. + let deadline = REPLY_TIMEOUT; let start = crate::rt::time::Instant::now(); loop { let remaining = deadline.saturating_sub(start.elapsed()); From a0205f529deae7a40e309213eaefe831c681bc19 Mon Sep 17 00:00:00 2001 From: Forte11Cuba Date: Sat, 29 Aug 2026 12:46:34 -0600 Subject: [PATCH 4/6] fix(#328): bind LastTradeIndex replies to their request via request_id --- rust/src/api/orders.rs | 52 ++++++++++++++++++++++++++++++++++++-- rust/src/mostro/actions.rs | 7 ++++- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index fdf98233..10c7f085 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -3638,6 +3638,23 @@ fn resync_floor( } } +/// True only for the reply to THIS `LastTradeIndex` request: the action +/// matches and the daemon echoed our correlation nonce +/// (`mostro/src/app/last_trade_index.rs` copies `request_id` into the reply). +/// +/// A replayed reply from an earlier request carries a different nonce — or +/// none, since this client's own pre-hardening requests sent no id — so +/// accepting `None` would readmit exactly the replays this guards against. +/// Strict matching means a daemon that does not echo falls back to the +/// restore-payload maximum, the same designed path as a silent daemon. +fn is_matching_last_trade_index_reply( + kind: &mostro_core::message::MessageKind, + request_id: u64, +) -> bool { + kind.action == mostro_core::message::Action::LastTradeIndex + && kind.request_id == Some(request_id) +} + /// Ask the daemon for its authoritative last-known trade index (#328). /// /// `Action::LastTradeIndex` is account-scoped: the request is signed with the @@ -3700,7 +3717,18 @@ async fn last_trade_index() -> Result> { return Ok(None); } - let event_json = actions::last_trade_index(&identity_keys, &mostro_pubkey).await?; + // Correlation nonce, echoed by the daemon in its reply. Without it any + // authenticated LastTradeIndex reply resolves this request, so a malicious + // relay could replay an old one. Monotonicity caps the damage (the max in + // resync_floor means a stale counter degrades to the payload fallback, the + // same as a silent daemon) — but the daemon echoes request_id, so binding + // the reply to this request costs nothing. + let request_id: u64 = { + use rand::RngCore; + rand::rngs::OsRng.next_u64().max(1) // 0 is indistinguishable from "unset" + }; + let event_json = + actions::last_trade_index(&identity_keys, &mostro_pubkey, request_id).await?; publish_event_json(&event_json).await?; crate::api::logging::blog_info( "restore", @@ -3738,7 +3766,7 @@ async fn last_trade_index() -> Result> { continue; } let kind = unwrapped.message.get_inner_message_kind(); - if kind.action != mostro_core::message::Action::LastTradeIndex { + if !is_matching_last_trade_index_reply(kind, request_id) { continue; } let idx = kind.trade_index.and_then(sanitize_trade_index); @@ -4026,6 +4054,26 @@ mod tests { assert_eq!(resync_floor(Some(7), &restore_info(vec![], vec![])), Some(7)); } + #[test] + fn a_replayed_last_trade_index_reply_is_rejected() { + use mostro_core::message::{Action, MessageKind}; + + let reply = |request_id: Option| { + MessageKind::new(None, request_id, Some(7), Action::LastTradeIndex, None) + }; + // The genuine reply echoes our nonce. + assert!(is_matching_last_trade_index_reply(&reply(Some(42)), 42)); + // A replay of an earlier request's reply carries a different nonce... + assert!(!is_matching_last_trade_index_reply(&reply(Some(41)), 42)); + // ...or none at all — this client's own earlier requests sent no id, + // so their stored replies are exactly the replay material to reject. + assert!(!is_matching_last_trade_index_reply(&reply(None), 42)); + // A different action never matches, even with the right nonce. + let other = + MessageKind::new(None, Some(42), Some(7), Action::RestoreSession, None); + assert!(!is_matching_last_trade_index_reply(&other, 42)); + } + #[test] fn resync_floor_falls_back_to_payload_max_when_daemon_is_silent() { // No LastTradeIndex answer (timeout / error): the restore-payload diff --git a/rust/src/mostro/actions.rs b/rust/src/mostro/actions.rs index 6f98c65e..459af572 100644 --- a/rust/src/mostro/actions.rs +++ b/rust/src/mostro/actions.rs @@ -426,13 +426,18 @@ pub async fn restore_session( /// The reply carries the counter in `MessageKind::trade_index`. Payload must be /// `None` (enforced by mostro-core). Mirrors `mostro-cli`'s /// `execute_last_trade_index`, which signs with `identity_keys` for both. +/// +/// `request_id` is the correlation nonce the daemon echoes in its reply +/// (`mostro/src/app/last_trade_index.rs`) — the caller uses it to reject +/// replayed replies from earlier requests. pub async fn last_trade_index( identity_keys: &Keys, mostro_pubkey: &PublicKey, + request_id: u64, ) -> Result { let msg = Message::Restore(MessageKind::new( None, - None, + Some(request_id), None, Action::LastTradeIndex, None, From 88e26168f20efce5e67144c673e65910d6a37582 Mon Sep 17 00:00:00 2001 From: Forte11Cuba Date: Wed, 2 Sep 2026 16:27:36 -0600 Subject: [PATCH 5/6] =?UTF-8?q?fix(#328):=20review=20round=201=20=E2=80=94?= =?UTF-8?q?=20trade-key-authored=20request,=20aligned=20timers,=20CantDo?= =?UTF-8?q?=20fast-path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- rust/src/api/orders.rs | 213 +++++++++++++----- rust/src/mostro/actions.rs | 23 +- .../contracts/identity.md | 9 +- 3 files changed, 177 insertions(+), 68 deletions(-) diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index 10c7f085..e929e39e 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -3633,7 +3633,25 @@ fn resync_floor( ) -> Option { let payload_max = recovered_max_trade_index(info); match (daemon_counter, payload_max) { - (Some(daemon), Some(payload)) => Some(daemon.max(payload)), + (Some(daemon), Some(payload)) => { + if payload > daemon { + // The invariant argued above says this cannot happen against a + // consistent daemon — so seeing it means a stale/partial reply + // or a daemon bug, the same "the daemon sent something this + // client's model does not cover" class + // recovered_max_trade_index already warns about. The behaviour + // (take the payload bound) is right; the silence would not be. + crate::api::logging::blog_warn( + "restore", + format!( + "LastTradeIndex counter {daemon} is below the restore \ + payload max {payload} — inconsistent daemon reply; \ + resyncing to the payload bound" + ), + ); + } + Some(daemon.max(payload)) + } (daemon, payload) => daemon.or(payload), } } @@ -3643,10 +3661,12 @@ fn resync_floor( /// (`mostro/src/app/last_trade_index.rs` copies `request_id` into the reply). /// /// A replayed reply from an earlier request carries a different nonce — or -/// none, since this client's own pre-hardening requests sent no id — so -/// accepting `None` would readmit exactly the replays this guards against. -/// Strict matching means a daemon that does not echo falls back to the -/// restore-payload maximum, the same designed path as a silent daemon. +/// none: mostro-cli sends this action with `request_id: None` +/// (`src/cli/last_trade_index.rs`), so nonce-less replies for the same account +/// exist in the wild wherever the user also runs the CLI. Accepting `None` +/// would readmit exactly those replays. Strict matching means a daemon that +/// does not echo falls back to the restore-payload maximum, the same designed +/// path as a silent daemon. fn is_matching_last_trade_index_reply( kind: &mostro_core::message::MessageKind, request_id: u64, @@ -3655,31 +3675,79 @@ fn is_matching_last_trade_index_reply( && kind.request_id == Some(request_id) } +/// True for the daemon's refusal of THIS request: `CantDo` echoing our nonce. +/// +/// The daemon answers `LastTradeIndex` with `CantDo(NotFound)` when the +/// account is unknown — an identity with no trade history on this node, and +/// every privacy-mode request, since without an identity proof there is no +/// account to look up — and `CantDo(InvalidTradeIndex)` when the stored +/// counter is 0. Both echo `request_id` (`mostro/src/app.rs` routes +/// `MostroCantDo` through `enqueue_cant_do_msg` with the request's id). +/// Treating them as terminal turns a full REPLY_TIMEOUT stall on those paths +/// into an immediate, logged fallback. +fn is_matching_cant_do_refusal( + kind: &mostro_core::message::MessageKind, + request_id: u64, +) -> bool { + kind.action == mostro_core::message::Action::CantDo + && kind.request_id == Some(request_id) +} + /// Ask the daemon for its authoritative last-known trade index (#328). /// -/// `Action::LastTradeIndex` is account-scoped: the request is signed with the -/// identity keys for BOTH the Seal and the rumor (see -/// `actions::last_trade_index`), so it derives NO trade key and the daemon -/// resolves the account by sender pubkey. The reply is a kind-14 addressed to -/// the identity pubkey, carrying the counter in `MessageKind::trade_index`. +/// The rumor is authored by `sender_keys` — the fresh trade key the caller +/// (`restore_session`) already derived — like every other daemon-bound event: +/// the outer kind-14 must never be authored by the master identity pubkey, +/// which would publish a permanent identity→Mostro link on every relay. The +/// daemon resolves the account from the encrypted identity proof +/// (`event.identity`) and replies to the rumor author (`event.sender`), so the +/// reply is a kind-14 addressed to the trade key, carrying the counter in +/// `MessageKind::trade_index`. The identity keys come from +/// `get_transport_identity_keys` — the privacy-toggle gate: in full-privacy +/// mode no proof is attached, the daemon finds no account and refuses with +/// `CantDo(NotFound)`, and the caller takes the payload fallback (privacy mode +/// has no stable account to ask about). /// /// Returns `Ok(Some(idx))` with the sanitized counter, or `Ok(None)` when the -/// daemon does not answer within the timeout or the reply carries no usable -/// index — the caller then falls back to `recovered_max_trade_index`. +/// daemon refuses (`CantDo`), does not answer within the timeout, or the reply +/// carries no usable index — the caller then falls back to +/// `recovered_max_trade_index`. /// /// This is a self-contained request/reply (own subscription + inline wait, like -/// mostro-cli's `wait_for_dm`) rather than a reuse of the per-trade -/// subscription/dispatch path: that path derives its recipient keys from a -/// trade index and is coupled to trade keys, but this reply is encrypted to the -/// identity key. -async fn last_trade_index() -> Result> { +/// mostro-cli's `wait_for_dm`) rather than a `pending_requests` record: the +/// reply also reaches the global dispatch path (the trade key is in the bulk +/// coverage), which ignores it — the restore's pending record was already +/// consumed — while this loop correlates by its own nonce. +async fn last_trade_index(sender_keys: &nostr_sdk::Keys) -> Result> { use nostr_sdk::RelayPoolNotification; use crate::rt::time::{timeout, Duration}; - let identity_keys = crate::api::identity::get_active_keys().await?; - let identity_pk = identity_keys.public_key(); - let identity_pk_hex = identity_pk.to_hex(); + let trade_pk = sender_keys.public_key(); + let trade_pk_hex = trade_pk.to_hex(); let mostro_pubkey = nostr_sdk::PublicKey::from_hex(&active_mostro_pubkey())?; + let identity_keys = + crate::api::identity::get_transport_identity_keys(sender_keys).await?; + + // Correlation nonce, echoed by the daemon in its reply. Without it any + // authenticated LastTradeIndex reply resolves this request, so a malicious + // relay could replay an old one. Monotonicity caps the damage (the max in + // resync_floor means a stale counter degrades to the payload fallback, the + // same as a silent daemon) — but the daemon echoes request_id, so binding + // the reply to this request costs nothing. + let request_id: u64 = { + use rand::RngCore; + rand::rngs::OsRng.next_u64().max(1) // 0 is indistinguishable from "unset" + }; + + // Build the event BEFORE subscribing: wrap_message_first_contact awaits the + // PoW capability snapshot and mines the PoW synchronously, so building + // after subscribe would start the relay-side auto-close early and shrink + // the usable reply window by the PoW + publish cost — at a high + // pow_first_contact on a slow device the relay could CLOSE before the + // request is even published. + let event_json = + actions::last_trade_index(&identity_keys, sender_keys, &mostro_pubkey, request_id) + .await?; let pool = crate::api::nostr::get_pool()?; let client = pool.client(); @@ -3691,7 +3759,8 @@ async fn last_trade_index() -> Result> { // This query, unlike the restore itself, has a fallback (the payload // maximum), so a shorter wait halves the worst-case restore latency // against a silent daemon. Shared by the relay-side auto-close and the - // outer wait loop so the two can't drift. + // outer wait loop — both started at subscribe below, so the two budgets + // actually run together. const REPLY_TIMEOUT: Duration = Duration::from_secs(5); // limit(0): live-only, same rationale as subscribe_daemon_messages — the @@ -3700,47 +3769,40 @@ async fn last_trade_index() -> Result> { let filter = nostr_sdk::Filter::new() .kind(nostr_sdk::Kind::PrivateDirectMessage) .author(mostro_pubkey) - .pubkey(identity_pk) + .pubkey(trade_pk) .limit(0); // Auto-close the relay-side subscription — this is a one-shot request/reply - // (mostro-cli's wait_for_dm shape), not a long-lived watcher, so the library - // issues the CLOSE on every path: after the reply (WaitForEventsAfterEOSE(1)) - // and, if the daemon never answers, on the timeout. Leaving it to manual - // bookkeeping is the leak class #182/#255 address. Auto-close subs are - // deliberately excluded from reconnect re-subscription (correct here: if the - // socket drops mid-request we fall back to the payload maximum by design). + // (mostro-cli's wait_for_dm shape), not a long-lived watcher. Leaving the + // CLOSE to manual bookkeeping is the leak class #182/#255 address. + // WaitDurationAfterEOSE, not WaitForEventsAfterEOSE(1): the recipient is + // the restore's trade key, so a late-propagating duplicate of the restore + // reply matches this filter too and would consume a one-event budget before + // the LastTradeIndex reply arrives. Holding the subscription open for the + // full reply window closes it deterministically on every path without that + // race. Auto-close subs are deliberately excluded from reconnect + // re-subscription (correct here: if the socket drops mid-request we fall + // back to the payload maximum by design). let close_opts = nostr_sdk::prelude::SubscribeAutoCloseOptions::default() - .exit_policy(nostr_sdk::prelude::ReqExitPolicy::WaitForEventsAfterEOSE(1)) + .exit_policy(nostr_sdk::prelude::ReqExitPolicy::WaitDurationAfterEOSE( + REPLY_TIMEOUT, + )) .timeout(Some(REPLY_TIMEOUT)); if let Err(e) = client.subscribe(filter, Some(close_opts)).await { log::warn!("[orders] last_trade_index subscribe failed: {e}"); return Ok(None); } + // Client-side deadline, started at subscribe time — the same instant the + // relay-side auto-close starts — so both give up together. + let start = crate::rt::time::Instant::now(); - // Correlation nonce, echoed by the daemon in its reply. Without it any - // authenticated LastTradeIndex reply resolves this request, so a malicious - // relay could replay an old one. Monotonicity caps the damage (the max in - // resync_floor means a stale counter degrades to the payload fallback, the - // same as a silent daemon) — but the daemon echoes request_id, so binding - // the reply to this request costs nothing. - let request_id: u64 = { - use rand::RngCore; - rand::rngs::OsRng.next_u64().max(1) // 0 is indistinguishable from "unset" - }; - let event_json = - actions::last_trade_index(&identity_keys, &mostro_pubkey, request_id).await?; publish_event_json(&event_json).await?; crate::api::logging::blog_info( "restore", "LastTradeIndex published — waiting for daemon".to_string(), ); - // Wait for the reply. Bounded by REPLY_TIMEOUT — the same budget the - // relay-side auto-close uses, so both give up together. - let deadline = REPLY_TIMEOUT; - let start = crate::rt::time::Instant::now(); loop { - let remaining = deadline.saturating_sub(start.elapsed()); + let remaining = REPLY_TIMEOUT.saturating_sub(start.elapsed()); if remaining.is_zero() { break; } @@ -3754,27 +3816,40 @@ async fn last_trade_index() -> Result> { let is_for_us = event.tags.iter().any(|t| { let s = t.as_slice(); s.first().map(|v| v.as_str()) == Some("p") - && s.get(1).map(|v| v.as_str()) == Some(identity_pk_hex.as_str()) + && s.get(1).map(|v| v.as_str()) == Some(trade_pk_hex.as_str()) }); if !is_for_us { continue; } - match crate::nostr::transport::unwrap_mostro_message(&identity_keys, &event).await { + match crate::nostr::transport::unwrap_mostro_message(sender_keys, &event).await { Ok(Some(unwrapped)) => { // Authenticate: the kind-14 author must be the node. if unwrapped.sender != mostro_pubkey { continue; } let kind = unwrapped.message.get_inner_message_kind(); - if !is_matching_last_trade_index_reply(kind, request_id) { - continue; + if is_matching_last_trade_index_reply(kind, request_id) { + let idx = kind.trade_index.and_then(sanitize_trade_index); + crate::api::logging::blog_info("restore", format!( + "LastTradeIndex reply: trade_index={:?} -> floor={idx:?}", + kind.trade_index + )); + return Ok(idx); } - let idx = kind.trade_index.and_then(sanitize_trade_index); - crate::api::logging::blog_info("restore", format!( - "LastTradeIndex reply: trade_index={:?} -> floor={idx:?}", - kind.trade_index - )); - return Ok(idx); + if is_matching_cant_do_refusal(kind, request_id) { + let reason = match &kind.payload { + Some(mostro_core::message::Payload::CantDo(Some(r))) => { + format!("{r:?}") + } + _ => "unspecified".to_string(), + }; + crate::api::logging::blog_warn("restore", format!( + "LastTradeIndex refused: CantDo({reason}) — \ + falling back to restore payload max" + )); + return Ok(None); + } + continue; } Ok(None) => continue, Err(e) => { @@ -3886,7 +3961,7 @@ pub async fn restore_session() -> Result idx, Err(e) => { crate::api::logging::blog_warn("restore", format!( @@ -4065,8 +4140,9 @@ mod tests { assert!(is_matching_last_trade_index_reply(&reply(Some(42)), 42)); // A replay of an earlier request's reply carries a different nonce... assert!(!is_matching_last_trade_index_reply(&reply(Some(41)), 42)); - // ...or none at all — this client's own earlier requests sent no id, - // so their stored replies are exactly the replay material to reject. + // ...or none at all — mostro-cli sends this action with no request_id, + // so nonce-less replies for the same account exist in the wild and are + // exactly the replay material to reject. assert!(!is_matching_last_trade_index_reply(&reply(None), 42)); // A different action never matches, even with the right nonce. let other = @@ -4074,6 +4150,25 @@ mod tests { assert!(!is_matching_last_trade_index_reply(&other, 42)); } + #[test] + fn a_cant_do_refusal_matches_only_our_nonce() { + use mostro_core::message::{Action, MessageKind}; + + let refusal = |request_id: Option| { + MessageKind::new(None, request_id, None, Action::CantDo, None) + }; + // The daemon's refusal of THIS request echoes our nonce and is + // terminal — the caller falls back immediately instead of stalling. + assert!(is_matching_cant_do_refusal(&refusal(Some(42)), 42)); + // A replayed or foreign CantDo does not resolve this request. + assert!(!is_matching_cant_do_refusal(&refusal(Some(41)), 42)); + assert!(!is_matching_cant_do_refusal(&refusal(None), 42)); + // The genuine counter reply is not a refusal. + let counter = + MessageKind::new(None, Some(42), Some(7), Action::LastTradeIndex, None); + assert!(!is_matching_cant_do_refusal(&counter, 42)); + } + #[test] fn resync_floor_falls_back_to_payload_max_when_daemon_is_silent() { // No LastTradeIndex answer (timeout / error): the restore-payload diff --git a/rust/src/mostro/actions.rs b/rust/src/mostro/actions.rs index 459af572..30c376d4 100644 --- a/rust/src/mostro/actions.rs +++ b/rust/src/mostro/actions.rs @@ -421,17 +421,24 @@ pub async fn restore_session( /// Build and wrap a `LastTradeIndex` request (#328). /// -/// Account-scoped: `identity_keys` sign BOTH the Seal and the rumor, so the -/// daemon resolves the account by sender pubkey and NO trade key is derived. +/// The daemon resolves the account from `event.identity` — the proven pubkey +/// inside the encrypted identity proof — and uses the rumor author +/// (`event.sender`) only as the reply address +/// (`mostro/src/app/last_trade_index.rs`). So the rumor is signed by an +/// ephemeral trade key, like every other daemon-bound event from this client: +/// the outer kind-14 must never be authored by the master identity pubkey, +/// which would publish a permanent identity→Mostro link on every relay. +/// (mostro-cli signs both with the identity keys and its comment claims the +/// daemon resolves by sender pubkey — the daemon source says otherwise.) +/// /// The reply carries the counter in `MessageKind::trade_index`. Payload must be -/// `None` (enforced by mostro-core). Mirrors `mostro-cli`'s -/// `execute_last_trade_index`, which signs with `identity_keys` for both. +/// `None` (enforced by mostro-core). /// -/// `request_id` is the correlation nonce the daemon echoes in its reply -/// (`mostro/src/app/last_trade_index.rs`) — the caller uses it to reject -/// replayed replies from earlier requests. +/// `request_id` is the correlation nonce the daemon echoes in its reply — the +/// caller uses it to reject replayed replies from earlier requests. pub async fn last_trade_index( identity_keys: &Keys, + trade_keys: &Keys, mostro_pubkey: &PublicKey, request_id: u64, ) -> Result { @@ -442,7 +449,7 @@ pub async fn last_trade_index( Action::LastTradeIndex, None, )); - wrap_message_first_contact(identity_keys, identity_keys, mostro_pubkey, &msg).await + wrap_message_first_contact(identity_keys, trade_keys, mostro_pubkey, &msg).await } #[cfg(test)] diff --git a/specs/004-mostro-p2p-client/contracts/identity.md b/specs/004-mostro-p2p-client/contracts/identity.md index 0b7eec73..3a87a0d6 100644 --- a/specs/004-mostro-p2p-client/contracts/identity.md +++ b/specs/004-mostro-p2p-client/contracts/identity.md @@ -38,7 +38,14 @@ disputes. 2. Send `Action.restore` to Mostro daemon via NIP-44 (Kind 14). 3. Receive list of order IDs + dispute IDs. 4. Request details for each order/dispute. -5. Sync trade key index. +5. Sync trade key index: send `Action.last_trade_index` (rumor authored by a + trade key; the daemon resolves the account from the identity proof) and + raise the local counter to the reply's `trade_index` — the daemon's + authoritative high-water mark, which includes finalized trades the restore + payload omits (#328). If the daemon refuses (`CantDo`) or does not answer, + fall back to the maximum trade index in the restore payload (a lower + bound: the payload lists only non-finalized orders). The counter is only + ever raised, never lowered; re-asking is idempotent. 6. Reconstruct local DB from daemon responses. **Note**: Recovery only works if identity is NOT in privacy mode. From 6a58ddf7cf802d57b2c7ff320b4032aaa25a7b15 Mon Sep 17 00:00:00 2001 From: Forte11Cuba Date: Wed, 2 Sep 2026 16:48:29 -0600 Subject: [PATCH 6/6] test(#328): regtest e2e for the finalized-top-index restore boundary --- rust/src/api/orders.rs | 100 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/rust/src/api/orders.rs b/rust/src/api/orders.rs index f3587c13..b138fe60 100644 --- a/rust/src/api/orders.rs +++ b/rust/src/api/orders.rs @@ -6035,4 +6035,104 @@ mod restore_e2e_tests { ); } } + + /// #328 e2e: the highest-index trade is already finalized, so the restore + /// payload's maximum is a lower bound and only LastTradeIndex carries the + /// real counter. Mirrors the probe in the issue: order A open at index 1, + /// order B canceled at index 2 — a fresh install that restores must end + /// with the counter at the daemon's high-water mark (2) and get its first + /// new order accepted (index 3), where the payload-only resync of #239 + /// left the counter at 1 and the daemon refused the next order with + /// CantDo(InvalidTradeIndex). + #[tokio::test] + #[ignore = "requires live regtest daemon + relay on ws://localhost:7000"] + async fn restore_with_finalized_top_index_resyncs_from_last_trade_index() { + crate::api::nostr::initialize(Some(vec!["ws://localhost:7000".to_string()])) + .await + .expect("relay pool init"); + crate::config::set_active_mostro_pubkey(Some( + "bae71ea2566771ed45b1d267dc0c0753028fe960a7bc4aeee08a44da0cb91520".to_string(), + )); + + let id = crate::api::identity::create_identity().await.expect("create identity"); + let words = id.mnemonic_words.clone(); + println!("[test] identity pubkey={}", id.public_key); + tokio::time::sleep(std::time::Duration::from_secs(3)).await; + + let params = |fiat: f64| crate::api::types::NewOrderParams { + kind: crate::api::types::OrderKind::Sell, + fiat_amount: Some(fiat), + fiat_amount_min: None, + fiat_amount_max: None, + fiat_code: "USD".to_string(), + payment_method: "cash".to_string(), + premium: 0.0, + amount_sats: None, + }; + // Distinct fiat amounts so the two orders never share a content + // fingerprint slot (see bridge_fingerprint_trade_index). + println!("[test] creating order A (index 1)..."); + let order_a = create_order(params(100.0)).await.expect("create order A"); + println!("[test] order A id={}", order_a.id); + println!("[test] creating order B (index 2)..."); + let order_b = create_order(params(200.0)).await.expect("create order B"); + println!("[test] order B id={}", order_b.id); + + // Finalize the top-index trade: cancel B. cancel_order publishes and + // returns without waiting for the daemon, so give it time to settle — + // the restore below must not see B as pending. + cancel_order(order_b.id.clone()).await.expect("cancel order B"); + tokio::time::sleep(std::time::Duration::from_secs(5)).await; + + // Fresh install: same mnemonic, counter back to zero. + crate::api::identity::delete_identity().await.expect("delete identity"); + crate::api::identity::import_from_mnemonic(words, false) + .await + .expect("re-import identity"); + tokio::time::sleep(std::time::Duration::from_secs(3)).await; + + println!("[test] calling restore_session()..."); + let info = restore_session().await.expect("restore round-trip"); + for o in &info.restore_orders { + println!( + "[test] order_id={} status={} index={}", + o.order_id, o.status, o.trade_index + ); + } + + // The canceled order B must be invisible here — that is exactly what + // makes the payload maximum (1) a lower bound of the daemon counter (2). + let payload_max = recovered_max_trade_index(&info); + assert_eq!( + payload_max, + Some(1), + "restore payload should carry only the open order A" + ); + + // The resync floor must have come from LastTradeIndex: with the + // payload fallback alone the counter would sit at 1. + let idx = crate::api::identity::get_identity() + .await + .expect("get_identity") + .expect("identity present after restore") + .trade_key_index; + assert!( + idx >= 2, + "counter ({idx}) must be >= 2 — the LastTradeIndex floor, not the payload bound" + ); + + // And the point of #328: the first post-restore order must be ACCEPTED. + println!("[test] creating first post-restore order..."); + let order_c = create_order(params(300.0)).await.expect( + "first post-restore order must be accepted \ + (was CantDo(InvalidTradeIndex) before #328)", + ); + println!("[test] ✓ post-restore order accepted id={}", order_c.id); + let idx_after = crate::api::identity::get_identity() + .await + .expect("get_identity") + .expect("identity present") + .trade_key_index; + assert_eq!(idx_after, 3, "the post-restore order should consume index 3"); + } }