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 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe restore flow computes the highest valid trade index across orders and disputes. It then raises the persisted identity trade-key index before returning restore data. Tests cover idempotency, invalid values, persistence, publication, and fresh derivation. ChangesRestore recovery flow
Estimated code review effort: 3 (Moderate) | ~22 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_at_least(floor)
IdentityAPI-->>OrdersAPI: persist and publish updated index
OrdersAPI-->>OrdersAPI: return RestoreSessionInfo
Possibly related issues
Possibly related PRs
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
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
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
|
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. |
7b97e5a to
39c6492
Compare
|
Rebased onto current main now that #225 (the #215 RestoreSession handshake this was stacked on) has merged. Squashed the four iterative review-fix commits into one clean commit. The only conflict was additive test-module overlap in |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rust/src/api/identity.rs (1)
424-433: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist the WebAssembly resynchronization before returning success.
On WebAssembly, Lines 424-433 pass
Noneto the core. The core updates only memory and callspublish_index, which ignoresSender::sendfailures. Tokio broadcast sends only to active receivers, and a successful send does not confirm that a receiver observed the value. (docs.rs)
restore_session()can succeed without a durabletrade_key_index. If the application stops before the Flutter mirror writes the event, a later startup can reuse the old index. Persist this Rust-owned protocol state through IndexedDB before returning success.As per coding guidelines, Rust owns protocol-layer persistence and must use
indexed_db_futureson web.#!/usr/bin/env bash set -euo pipefail # Confirm the resolved Tokio version and locate every durable consumer of this index. rg -n -C 3 '(^tokio\s*=|name = "tokio")' --glob 'Cargo.toml' --glob 'Cargo.lock' . rg -n -C 6 \ 'publish_index|trade_key_index_tx|TradeKeyIndexStream|trade_key_index|indexed_db_futures|save_identity' \ --glob '*.rs' --glob '*.dart' .🤖 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 424 - 433, Update the WebAssembly path around ensure_trade_key_index_at_least_with so the resynchronized trade-key index is durably persisted through indexed_db_futures/IndexedDB before returning success, rather than relying on publish_index or the Flutter mirror. Keep the existing non-WebAssembly flow unchanged, and propagate persistence failures instead of treating the in-memory update as successful.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.
Inline comments:
In `@rust/src/api/identity.rs`:
- Around line 452-465: The resync failure path must preserve the raised
trade_key_index and track that persistence is pending instead of restoring
current. Update the resync logic around save_identity and the same-floor
short-circuit to retry or reconcile pending durability rather than return
success without writing, and prevent derive_trade_key_with from issuing keys
until the floor is persisted or reconciled. Add a targeted failing-store test
covering failure, recovery, and attempted derivation.
- Around line 447-450: Update recovered_max_trade_index to reject u32::MAX
before ensure_trade_key_index_at_least_with can persist it, using the existing
error path and an i64::from(u32::MAX) + 1 limit. Preserve the no-op behavior in
the raised == current branch for valid recovered indices.
---
Duplicate comments:
In `@rust/src/api/identity.rs`:
- Around line 424-433: Update the WebAssembly path around
ensure_trade_key_index_at_least_with so the resynchronized trade-key index is
durably persisted through indexed_db_futures/IndexedDB before returning success,
rather than relying on publish_index or the Flutter mirror. Keep the existing
non-WebAssembly flow unchanged, and propagate persistence failures instead of
treating the in-memory update as successful.
🪄 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: db232fb6-c95d-4ca6-8f92-082a6cb20895
📒 Files selected for processing (2)
rust/src/api/identity.rsrust/src/api/orders.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/api/orders.rs
Refs MostroP2P#217 (sub-issue of MostroP2P#142). After a restore, the local trade_key_index counter is still at its post-install value while recovered trades already occupy higher indexes, so the next order reuses a trade key already bound to a recovered trade — the daemon rejects the reused index with CantDo(InvalidTradeIndex), and two trades would share a key. When a valid RestoreData is processed, raise trade_key_index to the maximum recovered index across orders and disputes. Monotonic (a restore never rewinds the counter) and idempotent. - identity::ensure_trade_key_index_at_least(floor) bumps the counter to max(current, floor) under the identity write lock; persists with the same discipline as derive_trade_key (rolls back the in-memory bump on a persist failure so a bumped-but-unpersisted counter can't regress on restart and reopen the bug), and requires durable storage on native (web exempt, same rationale as derive_trade_key). - orders::recovered_max_trade_index(info) — the max trade_index over restore_orders and restore_disputes; u32::try_from drops negatives and out-of-range values rather than truncating garbage. None when the restore carried no trades. - Wired into restore_session's Restored arm: resync before returning the info. A resync failure fails the restore rather than returning success with a counter that could hand out a reused key.
39c6492 to
e22ef66
Compare
|
Thanks for the thorough pass. Went through each finding against the current code: 1. 2. Rollback on persist failure this is intentional, and I believe correct as written. The trace:
So the alternative (keep the raised value in memory on failure) would introduce the memory/durable divergence this is designed to avoid. A failing-store regression test is a fair ask and I'd like to add one, but there's no failing-store harness in the repo yet (even 3. WASM durability intentionally consistent with |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Adversarial review — feat(#217): resync trade_key_index to the max recovered index
The core logic is right and I verified it rather than trusting the description. What follows is what survived an attempt to break it.
Verified working
- Off-by-one is correct.
trade_key_indexis the last consumed index (derive_trade_key_withcomputescurrent + 1), and the daemon'sRestoredOrdersInfo::trade_indexis the index that order used. Sofloor = max(used)and the next derivation lands atmax + 1. Theafter.index == 51assertion pins it. - Monotonic / idempotent / no spurious publish. The
raised == currentshort-circuit returns before any write orpublish_index, and the tests assert the channel stays silent on both the no-op and the repeat. - No deadlock.
blog_infois a synchronous broadcast send with no re-entry intoidentity_lock, so emitting it under the write guard is safe. - Build claims hold. On this branch:
cargo test --lib→ 238 passed, 0 failed;cargo clippy --lib -- -D warnings→ clean. (--all-targetsdoes error, but every one is a pre-existingawait_holding_lockinescrow*.rstest code, not from this PR.)
1. (high) The restore's own trade key is still drawn from the un-resynced counter
restore_session opens with derive_trade_key() (orders.rs:3114) — before it can possibly know the recovered maximum. On the real recovery path this is not hypothetical:
import_from_mnemonic(recover = true)
-> load_identity_from_mnemonic(words, 0, ...) // counter starts at 0
-> restore_session()
-> derive_trade_key() // hands out index 1
On a fresh install there is no stored row for reconcile_trade_key_index to raise, so the counter really is 0 and the RestoreSession is signed with the trade key of index 1 — a key a recovered trade already owns whenever the user had ≥ 1 trade. The PR closes the window for every subsequent derivation but not for the one the restore itself consumes.
The sharper edge is the failure path: if the restore is rejected or times out, index 1 has been consumed locally and no resync happens (the bump only lives in the Restored arm). The next order then goes out at index 2 — an index the daemon has already bound — which is exactly the CantDo(InvalidTradeIndex) this issue is about. The bug is narrowed, not closed.
Two things worth doing: (a) confirm mostrod actually accepts a first-contact wrap whose sender trade key is already bound to an active order — Message::new_restore(None) carries no trade_index, so the index check is likely skipped, but the key reuse itself should be checked against the daemon; (b) consider sourcing the restore's sender key from outside the trade-index tree (an ephemeral key), which removes the collision and the wasted index in one move. If neither is in scope here, please state the residual window in the PR body — right now the description reads as if #217 is fully closed.
2. (high) The rollback branch — the subtlest code in the PR — has no test
See inline. The doc comment on ensure_trade_key_index_at_least_with says the seam exists "so tests can inject a failing store", and no test injects one.
3. (high) The e2e coverage claim does not hold
The wiring into the
Restoredarm is exercised end-to-end by the merged #225'srestore_e2e_tests
I read both of them on this branch. restore_session_roundtrip asserts is_ok(); trade_then_restore_recovers_order asserts !info.restore_orders.is_empty(). Neither one ever looks at trade_key_index. The wiring — the if let Some(floor) guard and the ? that converts a persist failure into a restore failure — has no assertion anywhere, live daemon or not. One line in trade_then_restore_recovers_order (assert!(get_identity().await?.unwrap().trade_key_index >= max_recovered)) would make the claim true; otherwise please drop it.
4. (low) Not reachable from the app yet
restore_session is #[frb(ignore)] and its only caller is import_from_mnemonic(recover = true), while lib/core/services/identity_service.dart:105 passes recover: false. This is fine as staged work toward #142/#219, but the description reads like a shipped user-facing fix. A sentence saying it lands ahead of the flow that will use it would set the right expectation for whoever bisects this later.
5. (low) Branch is 37 commits behind main
Base is a149b8f (#264); orders.rs has moved ~1000 lines since. GitHub still reports MERGEABLE and I found no semantic conflict with the drift, but a rebase gets CI to validate against the tree this will actually land on.
Nothing here is a correctness defect in the code that was written — the added functions do what they say. #1 is a scope boundary that the description currently overstates, and #2/#3 are the test claims not matching the tests. Happy to see this land once the coverage claims are either backed or corrected.
| #[cfg(target_arch = "wasm32")] | ||
| if db.is_none() { | ||
| log::warn!( | ||
| "[identity] no local store on web — the resynced trade-key counter is durable only through the Flutter mirror" |
There was a problem hiding this comment.
(medium) Broken line continuation — this ships a run of literal spaces into the log.
The \ + newline continuation was lost when this was adapted from derive_trade_key (line 336, which has it right). The literal is currently:
"[identity] no local store on web — the resynced trade-key counter is durable only through the Flutter mirror"
So the emitted line reads ...counter is durable.... Restore the continuation:
log::warn!(
"[identity] no local store on web — the resynced trade-key counter is \
durable only through the Flutter mirror"
);Only reachable on wasm, which is why neither cargo test nor clippy caught it.
| // un-raised and reopening the key-reuse bug this closes. (Unlike | ||
| // derive_trade_key_with, which safely keeps its forward mutation | ||
| // because it has no idempotency short-circuit to defeat.) | ||
| state.identity_info.trade_key_index = current; |
There was a problem hiding this comment.
(high) This rollback is the subtlest logic in the PR and nothing tests it.
The comment right above correctly explains why the rollback is load-bearing: without it the idempotency short-circuit at the top swallows the retry and the durable counter silently stays un-raised. That is a good catch — and it is exactly the kind of reasoning that needs a test pinning it, because a future refactor that "simplifies" the short-circuit or drops the restore-on-failure line will not fail anything.
The doc comment on this function even advertises the seam:
takes an explicit store and publish channel so tests can inject a failing store and a private channel
No test in identity.rs injects a failing store — the only Storage used in tests is temp_store, which succeeds. Please add one (a Storage impl whose save_identity returns Err) asserting the three properties this branch exists for:
- the call returns
Errwith theStorageError:marker; - the in-memory
trade_key_indexis unchanged afterwards (get_identity()still reports the old value); - a retry with the same floor against a working store actually performs the write — i.e. the short-circuit was not poisoned.
(3) is the one that would regress silently, and it is the reason this rollback was written.
| // so a session loss would reload a stale pre-resync index and reopen the | ||
| // key-reuse bug this closes (#249). | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| require_durable_storage(db)?; |
There was a problem hiding this comment.
(low) Unreachable from the only caller — the comment presents it as load-bearing.
restore_session calls derive_trade_key() as its first statement, and that already runs require_durable_storage and returns early on native. So by the time ensure_trade_key_index_at_least runs, a store is guaranteed to exist and this check can never fire in production.
Defence-in-depth is fine and I would keep it, but the comment ("the _with core would otherwise bump and publish the raised index WITHOUT persisting it") describes a path no caller can reach today. Worth a short note that it guards future callers rather than the current one, so a reader does not go looking for the scenario it prevents.
| // in practice, but the floor must never be a value the counter cannot | ||
| // advance past. | ||
| .filter_map(|i| u32::try_from(i).ok()) | ||
| .filter(|&i| i < u32::MAX) |
There was a problem hiding this comment.
(medium) Invalid indexes are dropped silently — that is a silent failure on protocol drift.
A negative trade_index, or one at/beyond u32::MAX, is not just an odd value: it means the daemon sent something this client's model does not cover. Dropping it without a trace means the counter can end up lower than the true recovered maximum and the resulting CantDo(InvalidTradeIndex) on the next order will have no breadcrumb pointing back here.
The filtering decision is right; the silence is not. Suggest counting the drops and emitting blog_warn("restore", ...) when the count is non-zero. The degenerate case matters most: when every index is invalid this returns None, restore_session skips the resync entirely, and the restore reports success — the one path where a log is the only evidence anything happened.
Minor, same expression: the two adapters can collapse into one, which also removes the chance of the second filter drifting from the first —
.filter_map(|i| u32::try_from(i).ok().filter(|&v| v < u32::MAX))| // 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) { |
There was a problem hiding this comment.
(high) The resync cannot cover the index this very function already consumed — see point 1 of the summary.
By the time this line runs, derive_trade_key() at the top of restore_session has already handed out current + 1 from the un-resynced counter. On the actual recovery path (import_from_mnemonic(recover = true) → load_identity_from_mnemonic(words, 0, ...)) that is index 1 on a fresh install — a key a recovered trade already owns.
And because the bump lives only in this arm, the Rejected / timeout paths below leave the counter advanced by one with no resync: the next order then goes out at index 2, which the daemon already bound. That is the failure #217 describes, still reachable.
(medium, same line) Failing the whole restore on a persist error discards info entirely — the oneshot is consumed, so a retry costs another daemon round-trip and another consumed index. The trade-off is argued well in the comment and I would not block on it, but with #219 about to drive this from the UI it is worth deciding now whether the caller should instead receive the recovered trades plus an explicit "resync failed, do not create orders" signal. Surfacing the data and the hazard separately gives the UI something to act on; an opaque Err gives it a retry button and nothing else.
| } | ||
|
|
||
| #[test] | ||
| fn recovered_max_drops_negative_and_out_of_range_indexes() { |
There was a problem hiding this comment.
(low) Test placement. This one is separated from its two siblings (recovered_max_is_none_when_nothing_was_restored, recovered_max_spans_orders_and_disputes) by the unrelated a_dispute_message_without_a_peer_payload_yields_no_solver. Move it up under the ── #217 recovered_max_trade_index ── banner so the section the banner introduces is actually contiguous.
2 participants
Closes #217 (sub-issue of #142).
Problem
After a restore, the local
trade_key_indexcounter is still at its post-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 withCantDo(InvalidTradeIndex), and worse, two trades would share a key).What it does
When a valid
RestoreDatais processed, raisetrade_key_indexto the maximum recovered 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 asderive_trade_key: if the counter moves, the write must succeed or the call fails a bumped-but-unpersisted counter would regress on the next restart and reopen this bug. Native requires durable storage (require_durable_storage); web is exempt for the same reasonderive_trade_keyis (noinit_dbon web, IndexedDBsave_identityis a stub tracked in Web: IndexedDB storage backend is a stub — nothing persists across a reload #233; the Flutter mirror is web's durable record until then).orders::recovered_max_trade_index(info)the maxtrade_indexoverrestore_ordersandrestore_disputes. Indexes arei64on the wire;u32::try_fromdrops negatives and anything ≥u32::MAX(the terminal index is reserved storing it would overflow the nextderive_trade_key's+1). ReturnsNonewhen the restore carried no trades.restore_session'sRestoredarm: resync before returning the info. A resync failure fails the restore rather than returning "success" with a counter that could hand out a reused key.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 a valid restorederive_trade_key()returns a fresh index, not a recovered oneRestoreDatatwice leaves the counter unchangedTests
load_derive_then_delete_identity_lifecycleextended: a floor below current is a no-op (never lowers), a higher floor raises, re-applying the same floor is idempotent, and the nextderive_trade_key()returns a fresh index past the recovered set.recovered_max_trade_index:Nonewhen empty, max spans both orders and disputes, negatives / out-of-range /u32::MAXdropped.Restoredarm is exercised end-to-end by the merged feat(#215): RestoreSession handshake, send, correlate reply, subscribe #225'srestore_e2e_tests(needs a live regtest daemon).cargo test --lib(238) / clippy-D warnings/cargo check --target wasm32/flutter analyzeall clean.