[llbartekll]

Issue being fixed or feature implemented

PlatformWalletManager.startSpv(config:) was a synchronous wrapper around platform_wallet_manager_spv_start, which block_ons the Rust SPV start. That start opens the dash-spv disk store, and a store that needs a one-time storage migration (the v1 → v2 segment re-split from rust-dashcore #1084/#1092) blocks for many seconds. dashwallet-ios calls the wrapper from the main actor, so the first launch of 9.1.4 (12) froze the UI for 13.9 s on an iPhone (and 31.9 s on a simulator with an old store). Same class of bug as #4994 fixed for the stop.

What was done?

• startSpv(config:) is now async throws: the native start runs on the manager's SPV lifecycle queue (the former spvStopQueue, renamed spvLifecycleQueue since start and stop share it) and the caller suspends; the main actor stays free. The blocking overload is removed (dashwallet-ios is the only consumer).
• spvStartsInFlight + ensureNoSpvStartInFlight: a second startSpv, the blocking stopSpv() and clearSpvStorage() are refused while a start is in flight (they would park the caller behind it on the FFI registry guard). The async stopSpv() is not refused: it queues behind the start on the shared serial queue and stops the client the start brought up.
• The start is admitted like the other async native entry points (admitNativeOp), so shutdown() waits for it and a start after shutdown throws invalidHandle before any native work.
• spv_start_completed lifecycle event with duration_ms, so a slow store open shows up in swift/run.log.
• PlatformSpvStartConfig: Sendable; the native start is injected through PlatformWalletNativeTeardownCalls.spvStart (defaulted, so the existing test tables are unchanged).
• Callers moved to try await: CoreSpvLauncher in SwiftExampleApp (seam type is now async throws), MayaDepositVerificationIntegrationTests, and the start refusal in PlatformWalletStopSpvTests.
• platform_wallet_manager_spv_start (review follow-ups): the registry read guard is no longer held across the native start. The runtime is looked up under the guard, then SpvRuntime::start_and_spawn(config, publish) builds the client, runs the FFI's handle-registry check and spawns the run loop under one hold of the runtime's shutdown lock, so a concurrent stop can neither park a registry writer behind the store open nor slip between construction and publication. An owner already gone when the lock is taken bails out before the store is opened (no lock file held for a gone owner); one destroyed during the open stops the built client under that lock. Either way the call returns ErrorInvalidHandle. The owner check reads a per-runtime flag that destroy sets after removing the manager from the registry — never the registry itself, which a native stop holds while waiting for the shutdown lock (a registry read there could cycle with a queued registry writer). Without this, platform_wallet_manager_create (a registry write) and, through parking_lot's writer preference, every other registry reader queued behind a multi-second store open once the Swift caller stopped parking on it.
• SwiftExampleApp WalletManagerStore.stopSpvCancellingPendingStarts (review follow-ups): the generation is still invalidated synchronously, but the stop goes through the async stopSpv() so a Pause during an admitted start queues behind it instead of being refused under try?; the pending stop is tracked (isStoppingSpv, awaitPendingSpvStop()), the Start button is disabled on it and the start actions await it. New testStopIssuedDuringAnAdmittedStartRunsAfterTheNativeStart and testStartAfterPauseWaitsForTheStopStillTearingDown.

How Has This Been Tested?

• New PlatformWalletStartSpvTests (8): start runs off main while the main actor keeps running; handle + config reach the native call; start refused during an async stop; second start / blocking stop / clear refused during an async start; async stop issued during a start runs after it; shutdown() waits for an in-flight start; start after shutdown → invalidHandle; native failure mapped and the in-flight count released.
• swift test: 873 tests, 0 failures (integration target compiles with -warnings-as-errors), rerun against the rebuilt xcframework.
• cargo test -p platform-wallet-ffi: 427 + 26 + 6 passed, 0 failed. New spv_start_registry_tests (3) cover the registry release during a suspended start, the destroyed-handle branch (real offline client, handle removed inside the locked owner check) and native-failure reporting.
• cargo test -p platform-wallet: 1320 unit + integration + 4 doctests passed. New runtime tests cover a stop racing the run-loop publication (the stop future is polled once under the lock and asserted pending, then completed after publication) and an owner gone at publication; a new FFI test establishes the start / native-stop / queued-registry-writer cycle explicitly (stop signals under its registry guard, writer queued until a fresh try_read is refused) and fails by timeout against a registry-reading owner check; it runs in a child process the parent kills on timeout, so a regression cannot wedge the rest of the suite.
• SwiftExampleApp full unit bundle locally: 213 passed, 0 failed (the CI failure at ca0662b was the simulator refusing to launch the test runner before any test ran).
• SwiftExampleApp CoreSpvLauncherStartTests + WalletManagerStoreStopSpvTests on iPhone 17 simulator: passed.
• dashwallet-ios (dashpay/dashwallet-ios PR follows) on an iPhone 17 simulator whose mainnet SPV store still needed the v1 → v2 migration: spv_start_completed duration_ms=31900, and during that window the PIN unlock and the home screen rendered normally (the app log shows the timeline rebuild at +24 s into the migration). A mainnet → testnet switch afterwards: shutdown 44 ms off-main, start 27 ms, switch ready in 7.4 s.

Breaking Changes

The synchronous startSpv(config:) is removed; call try await startSpv(config:) instead. CoreSpvLauncher.start(network:stillCurrent:startSpv:)'s startSpv closure is now async throws.

Checklist:

• I have performed a self-review of my own code
• My Rust, FFI, WASM, Swift or Kotlin changes follow the coding conventions
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed
• If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 95df921

• Bots — coderabbitai skipped by llbartekll · thepastaclaw ✓
• Self-review — posted; again after any push
• Build green
• Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Source mode: Auto
• Repository: dashpay/platform
• Branch: v5.1-dev
• File: .coderabbit.yaml
• Commit: 033ecae
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1461a855-46bf-4b26-a438-bebedab6176d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

✅ Final review complete — no blockers (commit 95df921)

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The async start wrapper preserves native-result ownership and correctly participates in lifecycle ordering and shutdown admission. Two issues remain: the example app can silently discard a stop overlapping startup, and constructing another manager can still block the main actor behind startup's process-wide registry lock. Static verification confirmed both paths; current CI reports successful Swift SDK build/tests and schema checks, with only PR Hygiene pending.

🔴 2 blocking

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a substantial Swift concurrency and lifecycle change involving asynchronous FFI invocation, shutdown coordination, and race prevention, but it does not itself alter consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migration logic.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:383-386: Migrate the app's stop helper to the async stop overload
  The new startup guard turns an existing stop path into a silently discarded request. `WalletManagerStore.stopSpvCancellingPendingStarts(_:)` still calls `try? manager.stopSpv()` inside a synchronous closure (WalletManagerStore.swift:96–108), selecting this blocking overload. If the helper runs before the start's MainActor epilogue releases `spvStartsInFlight`, the guard throws and the helper suppresses the error without queuing a stop. Incrementing `spvStartGeneration` cannot cancel an admitted start: `CoreSpvLauncher` checks the generation only before awaiting `startSpv`. Both the Pause action and the global sync indicator use this helper; the overlap includes the completion race where polling exposes the running client before the start continuation completes. Preserve synchronous generation invalidation, then route the real stop through the async overload so it follows startup on the lifecycle queue. Add a test through the app helper that verifies the native stop executes after an overlapping start.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:309-312: Prevent second-manager configuration from blocking the main actor during startup
  Moving this call to a background queue leaves a concrete route for the same multi-second migration to freeze the main actor. `platform_wallet_manager_spv_start` holds `PLATFORM_WALLET_MANAGER_STORAGE.with_item` across `block_on_worker(spv.start(...))` (rs-platform-wallet-ffi/src/spv.rs:519–574). `HandleStorage.with_item` retains the process-wide map's read lock throughout that closure, while manager creation requires its write lock at `PLATFORM_WALLET_MANAGER_STORAGE.insert(manager)` (manager.rs:384). `PlatformWalletManager.configure` still performs that creation synchronously on MainActor. During the newly nonblocking startup, importing a wallet onto an uncached network or enabling another network calls `backgroundManager(for:)` → `activate` → `configure`, parking the UI until the first manager's migration finishes. Make native manager construction nonblocking to MainActor with its ownership guarantees preserved, or replace the long global-registry hold with per-manager lifetime protection. Cover creation of a second manager during a blocked native start; the injected single-manager tests do not exercise this lock.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Both prior blocking findings are fixed, and the complete diff introduces no additional blocking defect. The remaining findings are missing native concurrency regression coverage and stale registry-lock documentation. This was a static review without builds or test execution; current CI reports passing Rust wallet, Swift SDK, Kotlin SDK, and frozen-schema checks, with PR Hygiene still pending.

🟡 1 suggestion(s) | 💬 1 nitpick(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

💬 Nitpick: Update guard comments that still cite the former startup pattern
packages/rs-platform-wallet-ffi/src/spv.rs:598-599

The stop comment still cites platform_wallet_manager_spv_start to explain holding the registry guard across blocking work, and clear_storage repeats that reference at lines 672–673. Startup now deliberately does the opposite: it retains an owned runtime and releases the guard during construction. Replace these cross-references with the actual lifetime rationale for stop and clear so the comments do not imply that startup still demonstrates a long-held registry guard.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)}

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change involves intricate Swift/Rust lifecycle concurrency and registry locking, but does not itself modify storage migrations or another qualifying critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:561-565: Cover native registry liveness and destruction during startup
  The native follow-up now clones the runtime under the registry guard, constructs the client outside it, and revalidates the handle before publishing the run loop or stopping the orphaned client. The added Swift concurrency tests inject the entire native start, so they bypass both this lock scope and the destroyed-handle branch at lines 575–584; they would still pass if the long registry hold returned. Add deterministic Rust/FFI tests with startup suspended behind a gate: verify that another manager can be inserted while construction remains suspended, then separately remove the starting handle before releasing the gate and verify ErrorInvalidHandle, no run-loop publication, and eventual client/store cleanup. This directly protects the responsiveness fix and its new lifetime boundary without requiring a real migration or network access.
- [NITPICK] packages/rs-platform-wallet-ffi/src/spv.rs:598-599: Update guard comments that still cite the former startup pattern
  The stop comment still cites platform_wallet_manager_spv_start to explain holding the registry guard across blocking work, and clear_storage repeats that reference at lines 672–673. Startup now deliberately does the opposite: it retains an owned runtime and releases the guard during construction. Replace these cross-references with the actual lifetime rationale for stop and clear so the comments do not imply that startup still demonstrates a long-held registry guard.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Stop and storage clear retain the process-wide registry during blocking work — The unchanged native stop and clear_storage entry points still execute their blocking lifecycle work inside HandleStorage.with_item. A concurrent manager insertion therefore waits for that work, and a synchronous MainActor configuration can still freeze during a slow stop or clear. This is a concrete residual cross-manager stall, separate from the startup lock hold repaired here.
  • Follow-up: Track a separate native lifecycle change to shorten the stop/clear registry scopes while preserving concurrent-destruction safety.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The PR correctly moves blocking SPV startup off the main actor, releases the native registry during slow client construction, and preserves lifecycle admission and shutdown ordering. No blocking correctness issue was found, but the destroyed-handle test still does not exercise cleanup of an actually started client; three additional non-blocking documentation/diagnostic improvements remain. Relevant Swift, Rust wallet, and Kotlin checks passed; PR Hygiene is still pending.

🟡 2 suggestion(s) | 💬 2 nitpick(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a substantial cross-language concurrency and lifecycle change spanning Swift async APIs, Rust FFI registry coordination, shutdown behavior, and storage-start execution, but it does not itself modify a critical surface such as consensus, funds movement, cryptography, key handling, or storage migration logic.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [NITPICK] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:823-827: Update the lifecycle-queue comment after removing registry-guard serialization
  The `spvLifecycleQueue` comment still says that the FFI serializes start and stop on the registry guard. The revised native start releases that guard during client construction, so direct FFI start and stop calls are instead serialized by `SpvRuntime`'s shutdown lock; the Swift queue serializes the SDK's asynchronous callers. The behavior is correct, but the comment now describes a mechanism that no longer applies.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [NITPICK] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:457-461: Remove the stale registry-guard explanation from start refusal
  `ensureNoSpvStartInFlight` says that a concurrent operation would park behind the in-flight start on the FFI registry guard. The native start now releases that guard around the slow store open, so this refusal protects against overlapping lifecycle work rather than a registry-guard wait. Update the comment to describe the in-flight native lifecycle operation without naming the obsolete lock mechanism.

In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:595-597: Report failure when stopping an orphaned client
  When the manager disappears during startup, the new branch stops the started client but discards the stop result. A `ShutdownIncomplete` or `SpvProcessRestartRequired` result is the only observable indication that orphan teardown did not complete, because the manager has already been removed and cannot be stopped again through the registry. Preserve the existing `ErrorInvalidHandle` return, but log the cleanup failure so the orphaned lifecycle problem is diagnosable.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:724-728: Cover native registry liveness and destruction during startup
  (existing thread: https://github.com/dashpay/platform/pull/5512#discussion_r4337137721)
  The new test verifies that removing the handle produces `ErrorInvalidHandle`, but its injected `run_start` ignores the runtime and returns `Ok(())` without creating a client. Consequently, `spv.is_started()` is already false before the destroyed-handle cleanup runs, so deleting `spv.stop().await` would still leave the test passing. Exercise this branch with an offline, temporary-store-backed client or an observable cleanup/publication seam: assert that the runtime is started before removal and that the orphaned client is stopped afterward.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Static verification of head 30036e8 confirms that all seven prior findings are fixed and that blocking startup no longer runs on the main actor or retains the global registry during construction. The rewritten native startup path still carries a start/publication race with concurrent stops; the example app also exposes Start before asynchronous stopping finishes, and one test comment retains obsolete locking terminology. Current Rust wallet, Kotlin, Swift build/tests, and frozen-schema checks pass; no local builds or tests were run.

🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change involves intricate Swift/Rust lifecycle concurrency and registry locking, but does not itself alter consensus, funds movement, cryptography, network deserialization, or storage migration logic.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/spv.rs:589-594: Serialize run-loop publication with concurrent native stops
  The second registry lookup protects handle lifetime, but client construction and run-loop publication remain separate lifecycle operations. After `run_start` returns and releases `SpvRuntime::start`'s shutdown mutex, a concurrent native stop can acquire that mutex, remove and stop the client, and finish before this closure executes. `spawn_run_loop()` then acquires the idle mutex and installs a task without checking that a client exists; `run()` immediately reports "SPV Client not started", the FFI start returns Success, and the finished task remains in the slot, causing subsequent starts to return ShutdownIncomplete until another stop joins it. Kotlin start and stop dispatch independently on Dispatchers.IO, so the Swift queue does not protect this route. The old registry guard was also a shared read lock, not start/stop serialization: this is a lifecycle gap carried into the extracted startup helper, rather than a newly introduced exclusive-lock regression. Make construction and publication one coordinated operation in SpvRuntime, with handle revalidation preserved, and add a deterministic stop-between-construction-and-publication test.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift:114-122: Do not expose a new start before the asynchronous pause has completed
  The helper now returns while its untracked stop task is still running. Native stop removes the client before awaiting teardown, so polling can publish `spvIsRunning == false` while `spvStopsInFlight` is still nonzero. CoreContentView uses only `spvIsRunning` to switch its enabled button from Pause to Start; tapping Start during this interval reaches `ensureNoSpvStopInFlight`, prints "Sync failed", and drops the request without retrying. Previously the synchronous helper prevented that interaction until stop returned. Track the pending stop through completion and disable Start while stopping, or have the start action await the tracked stop. Add a Pause → Start test with teardown held behind a gate.

In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletStartSpvTests.swift`:
- [NITPICK] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletStartSpvTests.swift:264-267: Stale registry-guard wording in start-refusal test comment
  Native startup now releases the registry guard during client construction, so this comment no longer describes the reason for refusing overlapping lifecycle calls. Update it to match the corrected production comments.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The change correctly moves SPV startup off the main thread and fixes the previously identified lifecycle, ownership, and teardown races. However, the new owner revalidation callback can deadlock the process-wide manager registry when startup overlaps a native stop and any registry writer; the touched Swift, Rust wallet, Kotlin, and schema checks are green, while PR Hygiene remains pending.

🔴 1 blocking

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change intricately coordinates asynchronous Swift SPV lifecycle operations and Rust registry/runtime locking, but does not itself alter storage migrations or another qualifying critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/spv.rs:596-599: Avoid reacquiring the registry while holding the SPV shutdown lock
  The `still_owned` callback reacquires the global manager registry from inside `SpvRuntime::start_and_spawn`, while that method holds the runtime shutdown mutex at `runtime.rs:172-175`. The native stop path retains a registry read guard while awaiting the same shutdown mutex at `spv.rs:820-823`. A deadlock is possible when startup holds `shutdown`, a concurrent native stop holds the registry read guard and waits for `shutdown`, and a manager creation or destruction queues a registry writer. Because `parking_lot::RwLock` gives preference to the queued writer, the callback's new registry read waits behind that writer; startup cannot release `shutdown`, the writer cannot acquire the registry, and stop cannot release its read guard. The stop timeout does not recover this cycle because it starts only after the shutdown mutex has been acquired. This is reachable from Kotlin/JNI, whose start and stop calls can be dispatched independently, and it wedges registry-backed operations for every manager. Preserve atomic construction/publication and owner validation, but perform the publication check through a per-manager liveness token or another mechanism that does not reacquire the global registry while holding `shutdown`; add an FFI regression covering overlapping start, native stop, and a queued registry writer.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Static verification of the complete diff at ca0662b confirms that all eleven prior findings are fixed; no blocking production defect remains, but the new deadlock regression needs stronger synchronization. Current Rust wallet and Kotlin CI checks pass; the Swift check failed when the simulator could not launch the example-app test runner, after SDK unit tests completed without failures, which appears to be an infrastructure failure rather than a defect in this change.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a large, cross-language lifecycle and concurrency change around asynchronous SPV startup, shutdown coordination, and storage access, but it does not itself implement or alter a storage migration, consensus, funds, cryptography, or key-handling logic.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:832-845: Synchronize the deadlock regression with the actual lock contenders
  This setup does not establish the interleaving needed to detect the former registry/shutdown deadlock. `is_read_locked()` observes any reader of the process-wide registry, including another parallel test, and the writer constructs `mock_manager()` before attempting the write lock. The 100 ms sleep therefore does not prove that the intended stop holds its read guard or that the writer is queued when `still_owned()` runs. A registry-reading ownership callback can consequently pass this regression without encountering the lock cycle. Signal explicitly after the stop acquires its registry read guard, construct the second manager before spawning its insertion, and wait with a deadline until a fresh registry `try_read` is refused while that known reader remains held before invoking the ownership check.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Static verification of the complete diff at b5ae6d0 found no remaining actionable findings, and all 15 prior findings are fixed. Native construction and publication share the runtime shutdown lock, ownership validation avoids the registry lock cycle, and Swift startup and pause sequencing preserve admission and teardown ordering. No builds or tests were run locally; current CI shows Kotlin build/tests passing, but changed-package detection remains pending and Rust/Swift validation at this head is not yet confirmed.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change is intricate and cross-language, but it modifies SPV lifecycle scheduling, locking and shutdown coordination rather than storage migration logic or another qualifying critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[llbartekll]

/self-reviewed

[llbartekll]

/skip-bots

[github-actions]

Policy satisfied — this can merge.
Full checklist in the description.

[romchornyi]

Reviewed at b5ae6d0f. The direction is right: the freeze came from the synchronous block_on behind startSpv(config:) on the main actor, not from dash-spv. The v1 → v2 migration in rust-dashcore is already async (spawn_blocking per segment worker), and its duration is inherent I/O, so fixing it at the platform/Swift layer is the right place. A useful complement on the rust-dashcore side would be a migration progress/state event, so apps can show "upgrading storage" instead of a silent wait — not a replacement for this PR.

One inline comment (major; it affects FFI hosts other than Swift). Everything below is a non-blocking recommendation — none of it needs to land before merge.

Non-blocking recommendations

1. platform_wallet_manager_spv_stop / spv_clear_storage (rs-platform-wallet-ffi/src/spv.rs:975, :1052) still hold the registry read guard while waiting for the shutdown lock, which the start now holds for the whole store open. A native stop overlapping a slow start (Kotlin's blocking spvStop) therefore parks a queued registry writer, and through parking_lot's writer preference every later registry reader, for the rest of the migration. That was already the case before the PR, so it is not a regression, but it is the same stall the start path now avoids; the same treatment (resolve the runtime under the guard, stop outside it, owner flag for destroy) would finish the job.
2. Blocking stopSpv() / clearSpvStorage() now throw while a start is in flight. A caller using try? (background / wipe / reset paths) silently turns into a no-op and the start then publishes a run loop that was supposed to be stopped. Worth a doc note on both, or routing them through the lifecycle queue like the async stop.
3. SwiftExampleApp CoreContentView.swift:188: the Start/Pause toggle is disabled only on isStoppingSpv. During a slow start spvIsRunning is still false, so the button reads "Start", Pause is unreachable, and each tap is refused by the SDK and logs "Sync failed"; Clear sync data is likewise enabled and refused.
4. SwiftExampleApp WalletManagerStore.swift:116: isStoppingSpv / pendingSpvStop are store-wide, so a Pause on one network's manager disables and delays the start of another network's (independent) runtime.
5. Comment drift, spv.rs:982: "it releases the guard … and re-validates the handle afterwards" — the start deliberately does not re-check the registry, it reads the runtime's owner_released flag. Someone trusting the comment could reintroduce the registry lookup under shutdown, i.e. exactly the cycle the child-process test guards against.
6. Policy in the wrapper: spvStartsInFlight and the refusals of a second start / blocking stop / clear live in Swift. The Kotlin SDK has none of them, so the two SDKs behave differently over the same FFI, and every new SPV entry point has to remember ensureNoSpvStartInFlight. A Rust-side fail-fast (try-lock on shutdown, mapped to an error code) would give both the same rule, in line with "wrappers marshal rather than decide".
7. Small cleanups: mock_manager() in the new spv_start_registry_tests is a copy of the shielded_sync.rs fixture (share it in a #[cfg(test)] module); the four new FFI tests don't follow the should … naming rule that the new runtime.rs tests do; spvStart now lives in PlatformWalletNativeTeardownCalls (not teardown), and start_and_spawn repeats start()'s pre-lock checks.

🤖 Reviewed with Claude Code

[github-actions]

Bots are done — your move: address romchornyi left a review thread unresolved, then post /self-reviewed.
Full checklist in the description.

[llbartekll]

@romchornyi thanks — the major is fixed in 95df921 (owner checked right after the lock is taken, before the store open; reply in the thread). On the non-blocking recommendations, taken in the same commit:

• 5 comment drift at the stop's guard: now says the start reads the runtime's owner flag and never re-reads the registry under shutdown.
• 2 docs on the blocking stopSpv() / clearSpvStorage(): what a try? at the call site does with the refusal, and to use the async stop (queues behind the start) when a stop must land during one.
• 7 partly: the FFI SPV start tests follow the should … naming, mock_manager() is shared with the shielded-sync tests via test_fixtures, and start / start_and_spawn share their pre-lock checks (ensure_startable). spvStart still sits in PlatformWalletNativeTeardownCalls — renaming that table touches a dozen test files, so I left it for a follow-up.

Left for follow-ups, to keep this PR to the freeze: 1 spv_stop / spv_clear_storage outside the registry guard with the same owner-flag treatment (pre-existing stall, as you note); 3 disabling Start/Pause and Clear during an in-flight start in the example app; 4 per-network isStoppingSpv / pending stop; 6 a Rust-side fail-fast on shutdown so Kotlin gets the same rule as Swift. Also +1 on a migration progress/state event in rust-dashcore so hosts can show "upgrading storage".

🤖 Generated with Claude Code

[llbartekll]

/self-reviewed

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[romchornyi]

Re-reviewed at 95df9210a7. The major from my previous review is fixed as intended: the owner flag is now checked right after the shutdown lock is taken, before the store is opened, and the post-construction check still covers a destroy that lands during the open. should_not_open_the_store_when_its_owner_is_gone_before_the_lock pins it. The follow-ups taken in the same commit (stop guard comment, try? docs, should … test names, shared mock_manager, ensure_startable) look good. Agreed on deferring the native spv_stop / clear_storage registry hold, the example-app Start/Clear gating and per-manager pending stop, and the Rust-side start/stop admission rule to follow-ups.

Approving. A few non-blocking notes, none needed before merge:

1. publish in start_and_spawn is redundant in production. The only production caller passes !owner.owner_released() on the same runtime, and the method now checks self.owner_released() itself. Dropping the parameter and checking the flag at both points would remove the "the predicate must not take a registry lock" contract (and the chance that a future caller passes one that does); the race tests could use a cfg(test) hook instead.
2. start_spv_outside_registry (FFI) mostly exists to make the native step injectable. The "owner gone → ErrorInvalidHandle" decision could live in SpvRuntime as a typed error that the FFI just maps, which keeps the FFI to resolve-call-marshal.
3. Swift test scaffolding (EventLog, StopRecorder, TimedOut, waitUntil) gets two more private copies here, on top of the existing ones in the Stop/Load/Create tests. Pre-existing pattern, so lowest priority; a shared test-support file would stop them drifting.

🤖 Reviewed with Claude Code

[llbartekll]

/self-reviewed

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The current head correctly coordinates asynchronous SPV startup, native ownership, publication, and teardown, and all 15 prior findings are fixed. Two non-blocking issues remain in the example-app lifecycle surface: Clear can be pressed during an in-flight start or stop, and lifecycle admission policy is implemented only in the Swift wrapper rather than the runtime. Rust, Swift, Kotlin, and other completed checks passed; the broader test-suite and browser checks were still in progress.

🟡 2 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Gate Clear across asynchronous SPV start and stop windows
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift:201-210

The Clear button is disabled only when isSpvRunning is true. During an asynchronous Pause, the native stop removes the client before teardown completes, so isSpvRunning becomes false while isStoppingSpv remains true. During an asynchronous start, isSpvRunning is also false while the SDK's spvStartsInFlight guard remains active. In either window, clearSyncData() calls the blocking clearSpvStorage() synchronously; the SDK rejects it and the method only logs the error, leaving the user with no indication that Clear was ignored. Disable Clear for both lifecycle windows or route it through an operation that waits for the relevant lifecycle work, and add coverage for both cases. The PR author identified this as a follow-up but no tracking issue is linked.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)}

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The cross-language SPV lifecycle changes involve intricate concurrency and shutdown ordering, but the diff changes startup orchestration rather than storage migration logic or another listed critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift:201-210: Gate Clear across asynchronous SPV start and stop windows
  The Clear button is disabled only when `isSpvRunning` is true. During an asynchronous Pause, the native stop removes the client before teardown completes, so `isSpvRunning` becomes false while `isStoppingSpv` remains true. During an asynchronous start, `isSpvRunning` is also false while the SDK's `spvStartsInFlight` guard remains active. In either window, `clearSyncData()` calls the blocking `clearSpvStorage()` synchronously; the SDK rejects it and the method only logs the error, leaving the user with no indication that Clear was ignored. Disable Clear for both lifecycle windows or route it through an operation that waits for the relevant lifecycle work, and add coverage for both cases. The PR author identified this as a follow-up but no tracking issue is linked.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:295-303: Centralize fail-fast SPV lifecycle admission in the runtime
  The new `spvStartsInFlight` checks establish a lifecycle admission policy in the Swift SDK: Swift refuses blocking stop and clear while startup is admitted, while Kotlin/JNI and other native callers reach `SpvRuntime` directly and wait or observe different outcomes. `SpvRuntime::start_and_spawn` serializes construction and publication once startup has acquired its shutdown lock, but it does not expose the same reservation or typed busy state to callers that race before that point. Move the desired fail-fast reservation/result into `rs-platform-wallet` and expose it through the FFI, leaving Swift responsible for queueing, marshalling, and native-operation accounting. This avoids duplicating lifecycle policy in one mobile wrapper and gives all callers the same contract. The PR author lists this as a follow-up, but no tracking issue is linked.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Release the registry guard before native stop and storage clear — The existing native stop and clear implementations still retain a manager-registry read guard while waiting for runtime shutdown and teardown. A queued registry writer can therefore delay unrelated manager operations. This predates the reviewed startup change and is separate from the corrected startup lock cycle, so it should be handled in a dedicated lifecycle follow-up.
  • Follow-up: Open a separate issue or PR to apply per-runtime ownership protection and release the global registry guard before waiting on stop or clear.

[llbartekll]

/skip-bots

[github-actions]

Policy satisfied — this can merge.
Full checklist in the description.