feat(#215): RestoreSession handshake, send, correlate reply, subscribe - #225
Conversation
…, subscribe Sends a RestoreSession request to the active Mostro daemon and correlates the RestoreData reply. Handshake only — no state reconstruction, no persistence. - actions::restore_session(): builds Message::new_restore(None); Seal signed by the identity key (daemon looks up trades by master key), rumor by a fresh trade key (daemon replies to event.sender). - PendingRequestKind::Restore + take_matching_restore(): correlation by trade pubkey, not request_id (RestoreSession carries no request_id). - Action::RestoreSession arm in dispatch_mostro_message() routes RestoreData to the waiting caller. - Subscribes gift-wraps on the fresh trade key and registers the pending record before publishing, so the reply can't race the map. - Privacy-mode guard: recovery only in Reputation mode; returns PrivacyModeRecoveryUnavailable before any network traffic. - restore_session() is #[frb(ignore)] (internal until UI wiring lands). - #[ignore]d E2E test round-trips against a regtest daemon (run command in the test module). No DB writes, no key-counter changes, no is_mine mutation. Closes MostroP2P#215
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughSession restoration now builds and sends a daemon handshake, correlates ChangesRestore session flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MnemonicImport
participant OrdersAPI
participant MostroDaemon
participant MessageDispatcher
MnemonicImport->>OrdersAPI: Invoke restore_session
OrdersAPI->>MostroDaemon: Publish wrapped RestoreSession
MostroDaemon-->>MessageDispatcher: Return RestoreData
MessageDispatcher->>OrdersAPI: Match reply by trade pubkey
OrdersAPI-->>MnemonicImport: Return RestoreSessionInfo
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/identity.rs`:
- Around line 167-184: Move the recover-and-privacy_mode rejection in
import_from_mnemonic before calling load_identity_from_mnemonic, preventing
identity state from being replaced when recovery is unavailable. Preserve the
existing PrivacyModeRecoveryUnavailable error, then load the identity and keep
restore_session() as a separate post-load step for permitted recovery requests.
In `@rust/src/api/orders.rs`:
- Around line 3439-3443: Update the restore confirmation handling around
dispatch_mostro_message so a DaemonReply::Rejected { reason, message } response
is converted into an error that preserves both rejection details. Keep
successful DaemonReply::Restored(info) handling unchanged, and retain the
generic fallback only for other unexpected reply variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8bdcaf3-24e7-4d30-9416-0a386c70a7f1
📒 Files selected for processing (3)
rust/src/api/identity.rsrust/src/api/orders.rsrust/src/mostro/actions.rs
… restore rejection CodeRabbit review: - import_from_mnemonic: move the privacy-mode guard BEFORE load_identity_from_mnemonic. Previously the identity was swapped before the guard ran, so a Full-Privacy user got PrivacyModeRecoveryUnavailable while their identity was already replaced — and it violated the MostroP2P#215 'reject before any network traffic' criterion. - restore_session dispatch: handle DaemonReply::Rejected by logging reason and returning the message (matching create_order/take_order), instead of collapsing it into a generic 'unexpected restore reply' that dropped the daemon's rejection detail. Refs MostroP2P#215
…r/take_order
Log 'reason — message' and return {message}, matching the existing rejection
arms so the Dart layer surfaces daemon rejections uniformly. Refs MostroP2P#215
|
Both addressed: Moved the privacy-mode guard before load_identity_from_mnemonic good catch. The identity was being swapped before the bail, leaving a Full-Privacy user in a mutated state, and it violated #215's "reject before any network traffic" criterion. |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Actionable comments posted: 4
Scope discipline is good — this really is the handshake and nothing else, and moving the privacy-mode guard ahead of load_identity_from_mnemonic is the right fix from the #204 round. Three things block, and the first is the one worth a design call before merge.
Verification performed on b982d89
| Check | Result |
|---|---|
cargo test --lib |
✅ 110 passed, 0 failed, 8 ignored — matches the description |
cargo clippy --all-targets |
❌ new warning introduced — see blocker 3 |
| Is the flow user-reachable? | ✅ no — identity_service.dart:71 is the only Dart caller and passes recover: false |
| Is the transport correct? | ✅ yes — wrap_message → gift_wrap::wrap_mostro_message, i.e. protocol-v2 NIP-44 signed Kind 14, which is right for a daemon message. The doc comment says otherwise — see inline |
Does derive_trade_key() touch persistent state? |
❌ yes, it writes to the DB — see blocker 1 |
Blocking
1. restore_session() consumes and persists a trade-key index — acceptance criterion 2 is checked but false — severity: high
The description states, ticked:
- No DB writes, no key-counter changes, no
is_minemutation
restore_session() opens with derive_trade_key(), and that function (rust/src/api/identity.rs:245-271) increments state.identity_info.trade_key_index and immediately persists it:
state.identity_info.trade_key_index = candidate_index;
// Persist immediately: an index is consumed the moment it is derived.
db.save_identity(&state.identity_info).await...So every restore attempt writes the identity row and burns an index — including attempts that time out. That rationale is sound for order flows, where the daemon registers every index it sees. Restore is different: it is a read-only query, and nothing binds the consumed index to a trade.
The part that needs a decision before this merges: #217 exists to raise trade_key_index to the maximum recovered index, and #218 builds reconstruction on top of that counter. Having the restore handshake itself advance the counter by one, for reasons unrelated to any trade, is state that both of those issues will have to reason about. Either:
- accept it and say so explicitly in #217 (the resync must be
max(recovered, current), never a plain assignment — worth stating since a naive implementation would rewind), or - draw the restore key from outside the trade-index sequence.
Either way the checkbox and the "no key-counter changes" claim need to come out of the description. Flagging as blocking because two downstream issues inherit the answer, not because the code is wrong on its own.
2. No CI-runnable test covers any of the new logic — severity: high
Both new tests are #[ignore]d behind a live regtest stack, so CI exercises none of this — cargo test --lib reports 110 passed with 8 ignored, exactly as before the PR in terms of what actually runs.
Two pieces are trivially testable without a daemon, and the file already shows how:
actions::restore_session()— the test right above it,new_order_carries_request_id_and_trade_index, builds keys withKeys::generate(), wraps, and asserts on the resultingMessage. The same shape asserts the contract this PR documents as load-bearing: payload isNone(the daemon rejects anything else inMessageKind::verify), seal signed by identity, rumor by the trade key.take_matching_restore()— a pure operation over the pending map: returns the record for aRestoreentry, returnsNonefor a non-Restorekind on the same key, and removes on take.
The nonce-gate asymmetry is the single subtlest thing in this PR, and right now nothing pins it.
3. cargo clippy no longer passes cleanly; "Build clean" is inaccurate — severity: medium
warning: items after test module
--> src/mostro/actions.rs:470
= note: `#[warn(clippy::items_after_test_module)]` on by default
= help: move the items to before the test module was defined
Both new restore_session functions are appended after #[cfg(test)] mod tests, in actions.rs and in orders.rs. I checked the rest of the output so this isn't a false alarm: the other warnings are all await_holding_lock in settings.rs and reputation.rs, pre-existing and unrelated. This one is new, and cargo clippy is a mandated gate in this repo.
Non-blocking
| Severity | Finding | Where |
|---|---|---|
| Medium | An existing doc block now documents the wrong function | inline |
| Medium | Three new comments say "identity pubkey"; correlation is by trade pubkey | inline |
| Medium | restore_session() doc claims NIP-59 Gift Wrap; it is transport v2 Kind 14 |
inline |
| Medium | Restore failure propagates after the identity was already loaded | inline |
| Medium | Poisoned-mutex path registers nothing, publishes anyway, then waits the full 10s | orders.rs:3407 |
| Low | A timed-out restore leaks a pending-map entry permanently | orders.rs:193-201 |
| Low | &trade_pubkey_hex[..8] byte-slices a &str — panics under 8 chars, in the branch reached by unexpected input |
orders.rs, the "no waiting caller" arm |
| Low | Both E2E tests hardcode the regtest pubkey and mutate global state (initialize, set_active_mostro_pubkey); un-ignoring both would race in one process |
restore_e2e_tests |
| Low | orders.rs is now ~3530 lines; #120 tracks the split |
— |
On the poisoned-mutex one: if let Ok(mut map) = pending_requests().lock() { map.insert(...) } silently skips registration on Err, then publishes anyway. The caller then waits the full 10 seconds for a reply that can never be correlated and reports NoDaemonResponse — a lock bug wearing a network bug's clothing. Bail instead.
Yes to making the regtest pubkey env-configurable, as you offered, and a shared setup helper would remove the duplicated 8-line preamble.
Also worth a look
import_from_mnemonic now passes crate::api::reputation::get_privacy_mode() into load_identity_from_mnemonic where it previously passed a hardcoded false. That changes how an imported identity is loaded even when recover == false — which is the only path Dart uses today (identity_service.dart:71). It looks like a genuine fix rather than an accident, but it is behaviour change outside the stated scope of a handshake PR, and no test pins it. Worth either calling it out in the description or splitting it.
| /// `take_matching_request`, there is no request_id gate: the RestoreSession | ||
| /// message carries no request_id, so the daemon's RestoreData reply is | ||
| /// correlated purely by the identity pubkey it is addressed to. | ||
| fn take_matching_restore(pubkey_hex: &str) -> Option<PendingRequest> { |
There was a problem hiding this comment.
take_matching_restore was inserted directly beneath the doc comment that belongs to take_matching_request, with no blank line between them. That block ends:
…relays can replay historical events, and a stale reply must not confirm, reject, or reconcile a live request — the genuine reply (carrying the nonce) arrives later and finds the record.
It is now attached to take_matching_restore — the one function in this file that deliberately has no nonce gate — while take_matching_request, whose rationale it actually describes, is left with no documentation at all. Precisely inverted, and the two functions sit adjacent so the next reader has every reason to trust it.
Add a blank line after the original block and keep the new doc with the new function.
| /// Daemon rejected the request with a CantDo reason. | ||
| Rejected { reason: String, message: String }, | ||
| /// Daemon replied to a RestoreSession with the user's active trades and | ||
| /// disputes. Correlated by identity pubkey (RestoreSession carries no |
There was a problem hiding this comment.
Three new comments claim identity-pubkey correlation:
- this one, on
DaemonReply::Restored; PendingRequestKind::Restore— "Correlated by identity pubkey, not request_id";take_matching_restore's own doc — "correlated purely by the identity pubkey it is addressed to".
The code does the opposite, and restore_session()'s doc gets it right:
the request is sent from a fresh TRADE key (event.sender) … the daemon looks up trades by identity/master key and replies to the trade key … so we subscribe on the trade key and correlate the reply by that pubkey
The map is keyed by trade_pk_hex, and the dispatcher looks up take_matching_restore(trade_pubkey_hex). The identity key's role is the seal — it is what the daemon uses to find the trades, not what the reply is addressed to. Conflating the two is the kind of thing that sends the next person debugging correlation down the wrong path for an afternoon.
| /// (`MessageKind::verify`). The Seal is signed by the identity key (used by | ||
| /// the daemon to look up the user's trades); the rumor is signed by a fresh | ||
| /// trade key, and the daemon's restore reply is addressed to that trade key. | ||
| /// Returns the NIP-59 Gift Wrap event JSON. |
There was a problem hiding this comment.
wrap_message delegates to gift_wrap::wrap_mostro_message, documented at rust/src/nostr/gift_wrap.rs:37 as:
Wrap a
Messagedestined forreceiver(typically the Mostro node) as a protocol-v2 NIP-44 direct event (signed Kind 14, authored by the trade key).
Restore is a daemon message, so v2 is the correct transport and the code is right — only the comment is wrong. NIP-59 / Kind 1059 is peer and dispute chat.
The pre-existing doc on wrap_message (line 341) carries the same error, and line 31 repeats it. Since this PR quotes the claim into a new function, worth correcting all three rather than propagating it further.
| /// Returns the NIP-59 Gift Wrap event JSON. | |
| /// Returns the transport-v2 (NIP-44, signed Kind 14) event JSON. |
| } | ||
| let info = load_identity_from_mnemonic(words, 0, privacy_mode, None).await?; | ||
| if recover { | ||
| crate::api::orders::restore_session().await?; |
There was a problem hiding this comment.
You moved the privacy-mode guard ahead of load_identity_from_mnemonic for exactly the right reason, stated in the comment two lines up:
otherwise the identity is already swapped when we bail, leaving the user in a mutated state
This line has the same problem and did not get the same treatment. By the time it runs, load_identity_from_mnemonic has succeeded and the identity is loaded. A daemon that is slow, unreachable, or simply busy makes restore_session() return Err("NoDaemonResponse") after the 10s timeout, ? propagates it, and import_from_mnemonic reports failure to the caller — while having fully swapped the identity as a side effect. The caller has no way to tell "import failed" from "import succeeded, recovery didn't".
Not reachable today (identity_service.dart:71 passes recover: false), which is exactly what makes it a trap: it will surface when #220 wires the UI and starts passing true, far from this line.
Recovery is best-effort relative to the import — the import genuinely did succeed. Suggest logging the failure and returning Ok(info), with recovery state reported separately, or returning a result that distinguishes the two outcomes. This overlaps #219, which will restructure the waiting anyway; a comment pointing there would do for now if you would rather not decide it here.
…l, doc fixes grunch review round 2: - Add CI-runnable unit tests: restore_session builder (payload None, action RestoreSession, rumor authored by trade key) and take_matching_restore (returns/removes RESTORE, ignores non-RESTORE kinds). - clippy: move both restore_session fns before their test modules (items_after_test_module). - restore_session: bail on a poisoned pending-requests lock instead of publishing an event whose reply could never be correlated. - Guard the two [..8] pubkey slices in the RestoreData dispatch arms. - Docs: correlation is by TRADE pubkey not identity (3 comments); restore wrap is transport v2 (NIP-44 Kind 14) not NIP-59 Gift Wrap (all refs); restore the take_matching_request doc block to its own function. - Note the identity-already-swapped propagation on the recover path (MostroP2P#219). Refs MostroP2P#215
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/src/mostro/actions.rs (1)
483-508: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover both signing roles in the restore regression test.
This test verifies the trade-key rumor sender but not that
identity_keyssigned the Seal/outer identity. A regression in that half of the handshake could still pass this test while preventing the daemon from locating the user’s trades. Add an assertion for the decoded identity/Seal signer alongside the existing trade-key assertion.As per coding guidelines, protocol-handling changes should include targeted tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/mostro/actions.rs` around lines 483 - 508, Extend restore_session_payload_none_action_restore_rumor_by_trade_key to decode the event’s outer identity/Seal and assert its signer is identity_keys, while preserving the existing assertion that the unwrapped rumor sender is trade_keys.public_key(). Ensure the test covers both signing roles in the restore handshake.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust/src/mostro/actions.rs`:
- Around line 483-508: Extend
restore_session_payload_none_action_restore_rumor_by_trade_key to decode the
event’s outer identity/Seal and assert its signer is identity_keys, while
preserving the existing assertion that the unwrapped rumor sender is
trade_keys.public_key(). Ensure the test covers both signing roles in the
restore handshake.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98e37869-3afa-43e3-8698-763896bcc3fc
📒 Files selected for processing (3)
rust/src/api/identity.rsrust/src/api/orders.rsrust/src/mostro/actions.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/src/api/identity.rs
- rust/src/api/orders.rs
CodeRabbit: the restore handshake test verified the trade-key rumor sender but not the identity-key Seal signer — the half the daemon uses to locate the user's trades. Assert both signing roles. Refs MostroP2P#215
|
Good catch added assert_eq!(unwrapped.identity, identity_keys.public_key()) so the test now covers both signing roles: rumor by trade key (daemon's reply target) and Seal by identity key (how the daemon locates the trades). |
|
@grunch addressed the full review: Blockers
Non-blocking
On |
A RestoreSession is nonce-less (Message::new_restore(None)), so its pending record is registered without a request_id and correlated by trade pubkey via take_matching_restore. The Action::CantDo handler only used the nonce-gated take_matching_request, so a genuine rejection of a restore never matched: the caller waited the full 10s timeout and got NoDaemonResponse instead of the actual CantDo reason. Try take_matching_restore first in the CantDo arm, then fall back to the nonce-gated path. take_matching_restore only matches a Restore-kind record, so order requests keep their nonce gate and their replay protection — a stale replayed CantDo (no or foreign request_id) still touches nothing. Test: take_matching_restore matches a Restore record by pubkey alone and refuses a non-restore record. Refs MostroP2P#215
|
Also addressed a restore-rejection correlation bug CodeRabbit surfaced on the stacked #217: the Action::CantDo handler was nonce-gated, so a rejected RestoreSession (which is nonce-less) waited the full 10s and returned NoDaemonResponse instead of the reason. Now tries take_matching_restore first restore rejections surface their reason, order requests keep their nonce gate and replay protection. Regression test added (take_matching_restore_matches_restore_records_only). |
2 participants
Refs #215 RestoreSession handshake (send, correlate reply, subscribe).
Re-lands the handshake from the closed #204, scoped to the handshake only no
reconstruction side effects (those are #217/#218/#219).
What it does
Sends a
RestoreSessionrequest to the active daemon and correlates theRestoreDatareply. The Seal is signed by the identity key (daemon looks uptrades by master key); the rumor is signed by a fresh trade key (daemon replies
to
event.sender). Correlation is by trade pubkey, notrequest_idRestoreSessioncarries norequest_id, sotake_matching_restore()skips thenonce gate.
Scope
mostro::actions::restore_session()Message::new_restore(None)PendingRequestKind::Restore+take_matching_restore()Action::RestoreSessionarm indispatch_mostro_message()PrivacyModeRecoveryUnavailablebefore anynetwork traffic
restore_session()stays#[frb(ignore)](internal until UI wiring)Out of scope (separate sub-issues)
order_id -> trade_indexpersistenceis_mineon cached orderstrade_key_indexresync (Restore: resync trade_key_index to the max recovered index (prevents trade-key reuse) #217)Note on state (corrects an inaccurate claim in an earlier revision)
restore_session()consumes one trade-key index viaderive_trade_key(), whichincrements and persists
trade_key_index. This is inherent to the derivationthe daemon registers every index it sees, so the counter must survive even
timed-out operations. Restore is therefore not zero-side-effect: it burns one
index per attempt. #217's resync must be
max(recovered, current), never a plainassignment, or it would rewind past an index consumed here (noted on #217). No
is_minemutation and no reconstruction, as before.Separately:
import_from_mnemonicnow sources the realprivacy_mode(washardcoded
false, which meant the recovery guard could never fire). This affectsthe
recover: falsepath Dart uses today an intentional fix; happy to split itout if preferred.
Tests
CI-runnable unit tests (no daemon needed):
restore_session_payload_none_action_restore_rumor_by_trade_keyasserts thepayload is
None(daemon rejects anything else inMessageKind::verify), theaction is
RestoreSession, the rumor is authored by the trade key(
event.sender, the daemon's reply target), and the Seal is signed by theidentity key (how the daemon locates the user's trades) both signing roles.
take_matching_restore_returns_restore_and_ignores_othersreturns and removesa pending RESTORE record, ignores non-RESTORE kinds (pins the nonce-gate
asymmetry).
cargo test --lib-> 113 passed, 0 failed (2 E2E tests still#[ignore]d, runcommand documented in the test module).
cargo clippyclean. Build clean, CI green.