feat(#217): resync trade_key_index to the max recovered index - #239
feat(#217): resync trade_key_index to the max recovered index#239codaMW wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 27 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 (1)
WalkthroughThe restore flow now computes the highest valid recovered trade index across orders and disputes, then monotonically resynchronizes the persisted identity trade-key index before completing. Tests cover idempotency, invalid values, persistence, publication, and fresh derivation. ChangesRestore recovery flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OrdersAPI
participant DaemonReply
participant IdentityAPI
OrdersAPI->>DaemonReply: receive restored orders and disputes
OrdersAPI->>OrdersAPI: compute maximum valid trade_index
OrdersAPI->>IdentityAPI: ensure trade-key index reaches recovered floor
IdentityAPI-->>OrdersAPI: persist and publish updated index
OrdersAPI-->>OrdersAPI: 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: 1
🧹 Nitpick comments (2)
rust/src/api/orders.rs (1)
2922-2953: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment for
restore_sessionis misattached torecovered_max_trade_index.Lines 2922-2929 describe
restore_session's send/await behavior and key-correlation design, but there's no blank line before Line 2930, so the whole block (2922-2936) becomes one contiguous rustdoc comment attached tofn recovered_max_trade_index(Line 2937) instead.pub async fn restore_session()(Line 2953) ends up with no doc comment of its own.♻️ Proposed fix
-/// 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. -/// -/// Correlation: the request is sent from a fresh TRADE key (event.sender) while -/// the Seal carries the IDENTITY key (event.identity). The daemon looks up -/// trades by identity/master key and replies to the trade key -/// (mostro restore_session.rs: master_key = event.identity, reply -> event.sender), -/// so we subscribe on the trade key and correlate the reply by that pubkey. -/// Highest trade-key index across all recovered orders and disputes (`#217`). +/// 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. fn recovered_max_trade_index( info: &mostro_core::message::RestoreSessionInfo, ) -> Option<u32> {And restore the removed lines as the doc comment directly above
pub async fn restore_session()(Line 2952-2953).🤖 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/api/orders.rs` around lines 2922 - 2953, Separate the restore_session-specific rustdoc from recovered_max_trade_index by ending it before the helper’s documentation, then restore that documentation immediately above pub async fn restore_session. Keep the recovered_max_trade_index explanation attached only to recovered_max_trade_index and preserve the existing restore_session send/await and key-correlation details on the public function.rust/src/api/identity.rs (1)
297-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
ensure_trade_key_index_at_leastout of the Dart-callable API.This setter is only used by the ignore-marked
orders::restore_session()path and by tests; make it non-pubinstead ofpub(crate)and regenerate FRB bindings if needed.♻️ Proposed fix
-pub async fn ensure_trade_key_index_at_least(floor: u32) -> Result<()> { +async fn ensure_trade_key_index_at_least(floor: u32) -> Result<()> {🤖 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/api/identity.rs` around lines 297 - 317, Make ensure_trade_key_index_at_least private by removing its public visibility, since it is only used internally by orders::restore_session() and tests. Confirm the change does not expose it through Dart-callable or generated FRB bindings, and regenerate bindings only if required.Source: Path instructions
🤖 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/orders.rs`:
- Around line 2952-3033: Update the Action::CantDo rejection handling to
correlate restore requests with take_matching_restore(trade_pubkey_hex) instead
of the nonce-based take_matching_request path when the request kind is Restore.
Ensure the matched Restore waiter receives the rejection so restore_session()
returns the actual reason immediately, while preserving nonce-based correlation
for other request kinds.
---
Nitpick comments:
In `@rust/src/api/identity.rs`:
- Around line 297-317: Make ensure_trade_key_index_at_least private by removing
its public visibility, since it is only used internally by
orders::restore_session() and tests. Confirm the change does not expose it
through Dart-callable or generated FRB bindings, and regenerate bindings only if
required.
In `@rust/src/api/orders.rs`:
- Around line 2922-2953: Separate the restore_session-specific rustdoc from
recovered_max_trade_index by ending it before the helper’s documentation, then
restore that documentation immediately above pub async fn restore_session. Keep
the recovered_max_trade_index explanation attached only to
recovered_max_trade_index and preserve the existing restore_session send/await
and key-correlation details on the public function.
🪄 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: e7ce263d-b70c-4593-98a1-8b519886f74c
📒 Files selected for processing (3)
rust/src/api/identity.rsrust/src/api/orders.rsrust/src/mostro/actions.rs
After a restore, the local trade_key_index still sits at its post-install value while recovered trades already occupy higher indexes — so the next order would reuse a trade key already bound to a recovered trade. On a valid RestoreData, raise the counter to the max recovered index across orders and disputes. - identity::ensure_trade_key_index_at_least(floor): monotonic bump under the identity lock, persisted with the same fail-on-error discipline as derive_trade_key. No-op when already ahead (idempotent), never lowers. - orders::recovered_max_trade_index(info): max trade_index over recovered orders and disputes; drops negative / out-of-range values via u32::try_from rather than truncating a garbage index into the counter. - Wired into restore_session's Restored arm: resync before returning; a persist failure fails the restore (an un-resynced counter reopens the reuse bug). Tests: lifecycle test extended to assert raise / never-lower / idempotent and a fresh derive past the recovered set; pure tests for recovered_max_trade_index (spans both collections, drops negatives/out-of-range, None when empty). The resync wiring into the Restored arm is exercised by MostroP2P#215's E2E restore suite. Stacked on MostroP2P#215 (consumes its RestoreData handling). Refs MostroP2P#217
…lity - Move the restore_session rustdoc back onto restore_session; inserting recovered_max_trade_index between the doc and the fn had reattached it to the helper. Each doc now sits with its own function. - ensure_trade_key_index_at_least: pub -> pub(crate). It's only called from orders::restore_session and tests, so it should not reach the FRB/Dart surface. (pub(crate) rather than a plain private fn, since the call crosses the identity/orders module boundary.) Refs MostroP2P#217
8b4ef14 to
e7471ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/api/identity.rs (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUn-rolled-back persist failure in
ensure_trade_key_index_at_leastlets a retried restore silently report success without ever persisting the raised counter. The root cause is inidentity.rs;orders.rsonly surfaces the downstream effect.
rust/src/api/identity.rs#L414-434:state.identity_info.trade_key_index = raisedis set beforedb.save_identityand never rolled back on failure; combined with theraised == currentno-op short-circuit, a retry with the same floor silently returnsOk(())without retrying the persist. Also missing apublish_indexcall so the secure-storage mirror (issue#249) never learns of the raised index. Roll backtrade_key_indextocurrenton save failure and callpublish_index(trade_key_index_tx(), raised)after a successful save.rust/src/api/orders.rs#L3040-3051: because of the above, a retriedrestore_session()call can returnOk(info)even though the DB'strade_key_indexwas never durably raised — no local change needed once the identity.rs fix lands, but flagging so the contract is understood.🤖 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/api/identity.rs` at line 1, Update ensure_trade_key_index_at_least to restore state.identity_info.trade_key_index to current when db.save_identity fails, allowing retries to persist the requested floor instead of taking the raised == current no-op path; after a successful save, call publish_index(trade_key_index_tx(), raised) to update the secure-storage mirror.
🤖 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 414-434: Update ensure_trade_key_index_at_least to restore the
in-memory index if save_identity fails, allowing a retry with the same floor to
persist it again. After a successful persistence, publish the raised index
through the existing trade-key index channel, such as
publish_index/trade_key_index_tx, and preserve the no-op behavior when the
current index already meets the floor. Extend the lifecycle test to assert
publication and add coverage for persistence failure followed by a successful
retry.
In `@rust/src/api/orders.rs`:
- Around line 3040-3051: The restore path in the match arm handling
DaemonReply::Restored depends on ensure_trade_key_index_at_least retrying
persistence when the stored counter is already high enough. Update
ensure_trade_key_index_at_least in identity.rs so each call verifies or retries
the durable counter update instead of returning early solely because the
in-memory/current value meets the floor, while preserving monotonicity and
propagating persistence failures to the restore caller.
---
Outside diff comments:
In `@rust/src/api/identity.rs`:
- Line 1: Update ensure_trade_key_index_at_least to restore
state.identity_info.trade_key_index to current when db.save_identity fails,
allowing retries to persist the requested floor instead of taking the raised ==
current no-op path; after a successful save, call
publish_index(trade_key_index_tx(), raised) to update the secure-storage mirror.
🪄 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: b4e4403f-d7c9-4a2d-abc8-afb81a0494a7
📒 Files selected for processing (2)
rust/src/api/identity.rsrust/src/api/orders.rs
| 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) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Depends on identity.rs fix.
This correctly derives the floor and fails the restore (?) if the resync can't be persisted — good, matches the documented "silent success would be worse" intent. However this relies on ensure_trade_key_index_at_least actually retrying the persist on a retried call, which it currently does not (see the identity.rs L414-434 comment) — a retried restore could return Ok(info) here while the DB counter was never durably raised.
🤖 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/api/orders.rs` around lines 3040 - 3051, The restore path in the
match arm handling DaemonReply::Restored depends on
ensure_trade_key_index_at_least retrying persistence when the stored counter is
already high enough. Update ensure_trade_key_index_at_least in identity.rs so
each call verifies or retries the durable counter update instead of returning
early solely because the in-memory/current value meets the floor, while
preserving monotonicity and propagating persistence failures to the restore
caller.
…e, publish to mirror CodeRabbit caught two real durability gaps in ensure_trade_key_index_at_least: - No rollback on persist failure: the in-memory index was raised before save_identity and never reverted on error, so a retried restore hit the raised == current no-op and returned Ok() without re-persisting — silently leaving the durable counter un-raised and reopening the key-reuse bug MostroP2P#217 closes. Now rolls back to current on failure so a retry re-attempts the write. - Missing publish_index: the resync never mirrored to secure storage (MostroP2P#249), unlike derive_trade_key_with, so a later loss of mostro.db would reload a stale pre-restore index. Now publishes the raised index after a durable save. Refactored into an injectable ensure_trade_key_index_at_least_with(db, tx, floor) core + thin global wrapper (mirroring derive_trade_key / derive_trade_key_with), and extended the lifecycle test to assert the raise publishes, the no-op and idempotent calls publish nothing, and the counter is durable in the DB.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 414-415: Update ensure_trade_key_index_at_least to require a
durable database handle from app_db::db() before calling
ensure_trade_key_index_at_least_with; return an error when storage is
unavailable so neither the in-memory trade_key_index nor its publisher is
changed. Add coverage verifying the None-storage path preserves both states
unchanged.
🪄 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: 292f1b3b-80a6-45f8-97d0-377a30a34947
📒 Files selected for processing (1)
rust/src/api/identity.rs
…ch derive_trade_key) CodeRabbit: ensure_trade_key_index_at_least passed app_db::db() straight to the _with core, which can be None. On native that would bump and publish the raised index without persisting (the save is skipped), and since publication is best-effort a session loss would reload a stale pre-resync index — reopening the key-reuse bug this closes. Mirror derive_trade_key exactly: require_durable_storage(db)? on native, web exempt (init_db is never called there and IndexedDB has no save_identity yet, so the Flutter mirror is web's durable record until MostroP2P#233). The None-refuses path is covered by the existing require_durable_storage test that derive_trade_key relies on.
|
Fixed, mirroring derive_trade_key rather than the proposed one-liner, since require_durable_storage returns () (a guard), not a handle, and the unconditional version would break web. Now require_durable_storage(db)? on native (refuses the resync when there's no store, so we never bump+publish without persisting), web exempt with the same rationale as derive_trade_key (no init_db on web, IndexedDB has no save_identity yet, so the Flutter mirror is web's durable record until #233). The None-refuses path is covered by the existing require_durable_storage test that derive_trade_key also relies on. |
1 participant
Refs #217 sub-issue of #142. Stacked on #215 (consumes the
RestoreDatahandling that PR introduces).
Problem
After a restore, the local
trade_key_indexcounter is still at itspost-install value (0/1) while the recovered trades already occupy higher
indexes. The next order the user creates therefore reuses a trade key already
bound to a recovered trade a correctness bug (the daemon rejects the reused
index with
CantDo(InvalidTradeIndex), and worse, two trades would share a key).What it does
When a valid
RestoreDatais processed, raisetrade_key_indexto the maximumrecovered index across both orders and disputes. Monotonic a restore never
rewinds the counter.
identity::ensure_trade_key_index_at_least(floor)bumps the counter tomax(current, floor)under the identity write lock. No-op when already ahead(idempotent). Persists with the same discipline as
derive_trade_key: if thecounter moves, the write must succeed or the call fails a bumped-but-
unpersisted counter would regress on the next restart and reopen this bug.
orders::recovered_max_trade_index(info)the maxtrade_indexoverrestore_ordersandrestore_disputes. Indexes arei64on the wire;u32::try_fromdrops negatives and anything beyondu32::MAXrather thantruncating a garbage value into the counter. Returns
Nonewhen the restorecarried no trades (nothing to resync to).
restore_session'sRestoredarm: resync before returning theinfo. A resync failure fails the restore rather than returning "success" with
a counter that could hand out a reused key.
Lands on the payload shape available today (
trade_indexis already present)does not wait for the snapshot contract in #216.
Acceptance criteria
trade_key_indexraised tomax(order indexes, dispute indexes)on avalid restore
derive_trade_key()returns a fresh index, not a recovered one
RestoreDatatwice leaves the counterunchanged
Tests
load_derive_then_delete_identity_lifecycleextended (kept in the onestateful test so parallel threads never race the
identity_locksingleton):a floor below current is a no-op (never lowers), a higher floor raises,
re-applying the same floor is idempotent, and the next
derive_trade_key()returns a fresh index past the recovered set.
recovered_max_trade_index:Nonewhen empty, max spans both orders anddisputes, negatives and out-of-range values dropped.
Restoredarm is exercised end-to-end by Restore: RestoreSession handshake (send, correlate reply, subscribe) #215'srestore_e2e_tests(needs a live regtest daemon).cargo test --lib115 passed / clippy-D warningsclean /flutter analyzeclean.
Merge order
Stacked on #215 depends on its
RestoreDatahandling. Since #215 isn't inmain yet, this PR's diff currently includes #215's commits; the resync itself is
commit
aa7409b(identity.rs+orders.rs, ~153 lines). Once #215 merges,I'll rebase this onto main and the diff will show only the resync.
Summary by CodeRabbit
Bug Fixes
Tests