Repository navigation
perf(platform-wallet)!: build the Orchard proving key once off the async runtime and prove off tokio workers - #5278
PastaPastaPasta wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit 797c1aa) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 27afa59. The new proof await exposes stranded note reservations on cancellation, and four preparation/API/test issues also remain; these are client-side suggestions under the supplied severity policy, not consensus blockers. This was a static review: the supplied CI snapshot reports Rust workspace and Kotlin checks passing, dedicated Rust wallet tests skipped, and Swift build/tests still pending.
🟡 5 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes in shielded/prover.rs alter cryptographic proving-key initialization, sharing and panic recovery, while shielded/operations.rs moves spend-authority-bearing proof construction across thread boundaries, directly changing key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-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/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1843-1844: Make pre-broadcast note reservations cancellation-safe at the new proof await
The new await can suspend after reserve_unspent_notes marks the selected notes pending but before arm_pending_release attaches their anchor. If a Rust caller times out, drops the send future through select!, or aborts its task during preparation/proving, the outer result match is dropped too, so cancel_pending never runs. The blocking proof can finish after cancellation, but its closure owns neither reservation cleanup nor broadcasting. These reservations retain anchor: None: store.rs excludes them from spendable balance and note selection, while stale_pending_spends deliberately omits them, so normal sync cannot release them. The same newly exposed window exists in unshield, withdrawal, and identity top-up. Give pre-broadcast reservations cancellation-safe ownership in the wallet operations layer, then transfer ownership to conservative recovery before broadcasting. Add a gated operation-level cancellation test that verifies the notes become selectable again; the existing prepare-waiter cancellation test only verifies cache behavior.
In `packages/rs-platform-wallet/src/wallet/shielded/prover.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/prover.rs:118-120: Preserve preparation's panic-to-error contract outside Tokio
The supported no-runtime fallback calls get_or_build_blocking directly. An unwinding build panic therefore escapes prepare instead of returning ShieldedBuildError, unlike the Tokio path, which converts the blocking job's JoinError. This contradicts the preparation method's documented error behavior and makes failure handling depend on the executor. Catch unwinding build panics in this fallback and add a futures::executor::block_on test asserting that the first attempt returns Err, the cache remains empty, and a later attempt succeeds. This recovery applies only to unwind-enabled builds, not panic=abort profiles.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:633-635: isShieldedProverReady should be nonisolated like warmUp/prepare
PlatformWalletManager is @MainActor, so this static getter remains main-actor isolated even though the PR explicitly says readiness polling becomes nonisolated. Background callers still need an actor hop to read the indicator in Swift 6, unlike the newly nonisolated warm-up and preparation methods. The getter only reads the Rust process-global OnceLock and accesses no manager state. Mark it nonisolated to complete the stated API change.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:177-180: Guard the new preparation export against outer runtime panics
Preparation already converts a panic in the blocking key-build job into an error, but the surrounding block_on_worker bridge remains unguarded. runtime() can panic during runtime creation, and Runtime::block_on panics when invoked from an existing Tokio async execution context; block_on_worker also expects its worker join to succeed. An unwind from these paths reaches the extern "C" frame and aborts the host rather than returning PlatformWalletFFIResult. Wrap the new export with the existing catch_panic_to_code helper and a definitive wallet-operation error, since preparation cannot broadcast a spend. This protects unwind-enabled builds only; the iOS panic=abort profiles remain unrecoverable.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:2337-2341: Ignored prepare test asserts a 100ms wall-clock bound
The cached preparation call still goes through block_on_worker, including task scheduling and a JoinHandle round-trip. Scheduler delays can exceed 100ms without rebuilding the key or violating preparation correctness, so explicitly running this ignored test can fail solely because of machine load. Remove the elapsed-time assertion and verify successful repeated preparation and readiness instead. The gated cell tests already check build counts without using a latency threshold.
Out-of-scope follow-up suggestions (4)
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.
- Complete panic containment for legacy shielded spend exports — The transfer, unshield, and withdrawal C exports still call block_on_worker without catch_spend_panic. On unwind-enabled builds, a worker panic is re-raised inside extern "C" and terminates the host before JNI or Swift can translate it. The same unguarded calls exist before this PR's four commits, so this is a concrete legacy host-crash issue rather than a new blocker.
- Follow-up: Track a separate issue or maintainer-requested PR to apply the existing ambiguous-outcome panic guards to these legacy spend exports.
- Unify proving execution for signer-dependent send builders — Outside scope: the PR explicitly limits proof offloading to transfer, unshield, withdrawal, and identity top-up. Shield, shield-from-identity, and identity-create retain their existing inline proof execution while gaining shared preparation. Completing the intentionally deferred migration is not necessary for this change to work.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Finish off-runtime proving for the signer-dependent send paths — Outside scope and duplicates the intentionally deferred signer-dependent migration. The synchronous DPP builder execution predates this PR and is not worsened by awaiting preparation.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- shield / shield-from-identity / identity-create still prove inline — Outside scope: retained inline proving is consistent with the PR description's explicit distinction between four offloaded spend builders and signer-dependent paths that only await shared preparation.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| }) | ||
| .await? |
There was a problem hiding this comment.
🟡 Suggestion: Make pre-broadcast note reservations cancellation-safe at the new proof await
The new await can suspend after reserve_unspent_notes marks the selected notes pending but before arm_pending_release attaches their anchor. If a Rust caller times out, drops the send future through select!, or aborts its task during preparation/proving, the outer result match is dropped too, so cancel_pending never runs. The blocking proof can finish after cancellation, but its closure owns neither reservation cleanup nor broadcasting. These reservations retain anchor: None: store.rs excludes them from spendable balance and note selection, while stale_pending_spends deliberately omits them, so normal sync cannot release them. The same newly exposed window exists in unshield, withdrawal, and identity top-up. Give pre-broadcast reservations cancellation-safe ownership in the wallet operations layer, then transfer ownership to conservative recovery before broadcasting. Add a gated operation-level cancellation test that verifies the notes become selectable again; the existing prepare-waiter cancellation test only verifies cache behavior.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Declining this one as out of scope. The cancellation window isn't new in this PR. In transfer, unshield, withdraw and identity_top_up_from_pool_with_metadata, the notes are already reserved before extract_spends_and_anchor(...).await, which does a network round-trip (ShieldedAnchors::fetch_current(sdk).await), and before record_pending_activity(...).await. All of that comes before arm_pending_release. So a caller that drops the send future could already strand anchor: None reservations before this change. Moving the proof onto the blocking pool makes that window longer but doesn't create it. The FFI hosts drive these futures through block_on_worker to completion, so they never drop them mid-flight. Making pre-broadcast reservations cancellation-safe needs a reservation ownership/recovery change across the operations layer, and that belongs in its own PR.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Deferred (re-reviewed at 797c1aae): Your scope argument is supported by the baseline: all four operations already reserve notes before the anchor-fetch and activity-recording awaits, and the FFI bridge drives the send future to completion. The new proof await extends that existing window without broadcasting from detached work, so reservation ownership can remain a separate operations-layer follow-up.
| if tokio::runtime::Handle::try_current().is_err() { | ||
| return Ok(self.get_or_build_blocking()); | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
797c1aae; see the replies below.
🟡 Suggestion: Preserve preparation's panic-to-error contract outside Tokio
The supported no-runtime fallback calls get_or_build_blocking directly. An unwinding build panic therefore escapes prepare instead of returning ShieldedBuildError, unlike the Tokio path, which converts the blocking job's JoinError. This contradicts the preparation method's documented error behavior and makes failure handling depend on the executor. Catch unwinding build panics in this fallback and add a futures::executor::block_on test asserting that the first attempt returns Err, the cache remains empty, and a later attempt succeeds. This recovery applies only to unwind-enabled builds, not panic=abort profiles.
| if tokio::runtime::Handle::try_current().is_err() { | |
| return Ok(self.get_or_build_blocking()); | |
| } | |
| if tokio::runtime::Handle::try_current().is_err() { | |
| return std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | |
| self.get_or_build_blocking() | |
| })) | |
| .map_err(|_| { | |
| PlatformWalletError::ShieldedBuildError( | |
| "Orchard proving key preparation failed: the build panicked".to_string(), | |
| ) | |
| }); | |
| } |
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 180b997. The no-runtime fallback now catches an unwinding build panic and returns ShieldedBuildError, matching the blocking-pool path. Added panicking_build_without_runtime_reports_error_and_a_later_call_retries, which uses futures::executor::block_on and checks that the first attempt errors, the cell stays empty, and the retry succeeds.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 797c1aae): Your no-runtime fallback now converts an unwinding build panic into ShieldedBuildError. The added regression test checks the failed attempt leaves the cell empty and a later attempt succeeds; I verified those assertions statically.
| public static var isShieldedProverReady: Bool { | ||
| platform_wallet_shielded_prover_is_ready() | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
797c1aae; see the replies below.
🟡 Suggestion: isShieldedProverReady should be nonisolated like warmUp/prepare
PlatformWalletManager is @mainactor, so this static getter remains main-actor isolated even though the PR explicitly says readiness polling becomes nonisolated. Background callers still need an actor hop to read the indicator in Swift 6, unlike the newly nonisolated warm-up and preparation methods. The getter only reads the Rust process-global OnceLock and accesses no manager state. Mark it nonisolated to complete the stated API change.
| public static var isShieldedProverReady: Bool { | |
| platform_wallet_shielded_prover_is_ready() | |
| } | |
| nonisolated public static var isShieldedProverReady: Bool { | |
| platform_wallet_shielded_prover_is_ready() | |
| } |
source: muse-spark-1.3-contributor (phase1-reviewer: general)
There was a problem hiding this comment.
Fixed in 180b997. isShieldedProverReady is now nonisolated, as the PR description says.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 797c1aae): Your readiness getter is now explicitly nonisolated, matching warm-up and preparation. It reads only the Rust process-global readiness state, so background polling no longer requires a main-actor hop.
| match block_on_worker(CachedOrchardProver::prepare()) { | ||
| Ok(()) => PlatformWalletFFIResult::ok(), | ||
| Err(e) => e.into(), | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
797c1aae; see the replies below.
🟡 Suggestion: Guard the new preparation export against outer runtime panics
Preparation already converts a panic in the blocking key-build job into an error, but the surrounding block_on_worker bridge remains unguarded. runtime() can panic during runtime creation, and Runtime::block_on panics when invoked from an existing Tokio async execution context; block_on_worker also expects its worker join to succeed. An unwind from these paths reaches the extern "C" frame and aborts the host rather than returning PlatformWalletFFIResult. Wrap the new export with the existing catch_panic_to_code helper and a definitive wallet-operation error, since preparation cannot broadcast a spend. This protects unwind-enabled builds only; the iOS panic=abort profiles remain unrecoverable.
| match block_on_worker(CachedOrchardProver::prepare()) { | |
| Ok(()) => PlatformWalletFFIResult::ok(), | |
| Err(e) => e.into(), | |
| } | |
| catch_panic_to_code( | |
| "shielded prover preparation", | |
| PlatformWalletFFIResultCode::ErrorWalletOperation, | |
| "No spend was broadcast.", | |
| || match block_on_worker(CachedOrchardProver::prepare()) { | |
| Ok(()) => PlatformWalletFFIResult::ok(), | |
| Err(e) => e.into(), | |
| }, | |
| ) |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
Fixed in 180b997. The export is now wrapped in catch_panic_to_code with ErrorWalletOperation and "No spend was broadcast." Preparation never spends, so a definitive failure code is correct here.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 797c1aae): Your guard encloses the entire runtime bridge and result conversion, returning ErrorWalletOperation with "No spend was broadcast." This addresses the identified unwinding paths without implying recovery under panic=abort.
| // Ready: a second call is immediate. | ||
| let start = std::time::Instant::now(); | ||
| let result = platform_wallet_shielded_prepare_prover(); | ||
| assert_eq!(result.code, PlatformWalletFFIResultCode::Success); | ||
| assert!(start.elapsed() < std::time::Duration::from_millis(100)); |
There was a problem hiding this comment.
✅ Resolved at
797c1aae; see the replies below.
🟡 Suggestion: Ignored prepare test asserts a 100ms wall-clock bound
The cached preparation call still goes through block_on_worker, including task scheduling and a JoinHandle round-trip. Scheduler delays can exceed 100ms without rebuilding the key or violating preparation correctness, so explicitly running this ignored test can fail solely because of machine load. Remove the elapsed-time assertion and verify successful repeated preparation and readiness instead. The gated cell tests already check build counts without using a latency threshold.
| // Ready: a second call is immediate. | |
| let start = std::time::Instant::now(); | |
| let result = platform_wallet_shielded_prepare_prover(); | |
| assert_eq!(result.code, PlatformWalletFFIResultCode::Success); | |
| assert!(start.elapsed() < std::time::Duration::from_millis(100)); | |
| // Ready: repeated preparation succeeds and preserves readiness. | |
| let result = platform_wallet_shielded_prepare_prover(); | |
| assert_eq!(result.code, PlatformWalletFFIResultCode::Success); | |
| assert!(platform_wallet_shielded_prover_is_ready()); |
source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 180b997. Removed the 100 ms bound. The test now checks that repeated preparation succeeds and readiness still holds.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 797c1aae): Your fix is present at the current head: repeated preparation checks success and readiness, and the 100 ms assertion is gone. The remaining elapsed-time output cannot fail the test because of scheduler load.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
Waiting for bot review — coderabbitai skipped after its own rate limit · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
github-actions
Bot
added
the
bot-review-skipped
|
@thepastaclaw review No review for |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
… off the async runtime The Halo 2 proving key lived in a `OnceLock` filled lazily inside `OrchardProver::proving_key`, so a send that arrived while the key was still building blocked a tokio worker in `get_or_init` for up to the whole build (~30 s debug, ~1.4 s release), and every proof then ran synchronously on a tokio worker, competing with networking and shielded sync. - `ProvingKeyCell`: one build per process. Concurrent async callers coalesce on a `tokio::sync::OnceCell` and wait on one blocking-pool job; the sync `warm_up` path shares the same `OnceLock`, so the two never build twice. A panicked build leaves the cell empty for a retry. - `CachedOrchardProver::prepare()` (async) and the new `ShieldedProver` trait: send operations await `ready()` (the shared preparation) and get a `'static` `ReadyOrchardProver`. - transfer, unshield, withdraw and identity top-up build their transitions in `prove_on_blocking_thread`; the spend keyset moves in by `Arc`, so the spend authority is never copied. Shield, shield-from-identity and identity-create await the shared preparation. - On Apple targets key builds and proofs run on a dedicated rayon pool whose threads request USER_INITIATED QoS (8 MiB stacks). Darwin threads inherit their creator's QoS, so a warm-up started from a background-priority host task could otherwise leave the build, and the lazily created global rayon pool halo2 uses, throttled. `PlatformWallet`'s shielded send methods now take `P: ShieldedProver` (implemented by `CachedOrchardProver` and `&CachedOrchardProver`) instead of any `OrchardProver`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`platform_wallet_shielded_prepare_prover()` blocks the calling thread until the shared Halo 2 proving-key build finishes (starting it if needed) and returns a `PlatformWalletFFIResult`, so hosts can wait for readiness instead of polling. `platform_wallet_shielded_warm_up_prover` stays fire-and-forget but now joins the same shared preparation, and `platform_wallet_shielded_prover_is_ready` is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`PlatformWalletManager.prepareShieldedProver() async throws` completes only once the proving key is built. The blocking FFI wait runs on a global user-initiated dispatch queue through a checked continuation, so it never pins a Swift cooperative-pool thread. Document that `warmUpShieldedProver()` only schedules the build. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rover tests Review follow-ups for the shared proving-key preparation: - Top-level jobs enter the Apple proving pool one at a time. With two proofs in flight, a worker blocked in a `join` inside proof A could steal proof B's whole top-level job and run it on A's stack (A then finishes after A + B, sharing one 8 MiB stack). A call made from a pool thread runs inline instead of re-taking the gate. - Document the invariant that the key is never lazily built from a rayon worker (deadlock), why proving awaits `ready()` first, and that a persistently failing build is retried once per queued waiter. - Replace wall-clock thresholds in the prover tests with gated builds and channels, add a test that aborting the waiter that started the build does not trigger a second build, and a test for the pool gate. - Swift: `prepareShieldedProver()` and `warmUpShieldedProver()` are `nonisolated` (no main-actor hop for the wait); document that prepare ignores task cancellation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- ProvingKeyCell::prepare: catch an unwinding build panic in the no-runtime fallback too, so it returns ShieldedBuildError (and a later call retries) exactly like the blocking-pool path. - platform_wallet_shielded_prepare_prover: wrap the export in catch_panic_to_code so a panic in the runtime bridge returns ErrorWalletOperation instead of unwinding across the C ABI. - Swift: make isShieldedProverReady nonisolated, as the PR states. - Drop the 100 ms wall-clock assertion from the ignored FFI prepare test; assert repeated success and readiness instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
180b997 to
797c1aa
Compare
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 797c1aa: no in-scope blocking defects were identified, and four prior findings are fixed. One non-blocking suggestion remains: report Apple proving-pool initialization failures before permanently caching the inline fallback. Validation was static only; Rust workspace tests were pending in the supplied CI snapshot, the Kotlin failure log confirms an action-download timeout before compilation, and Swift linking/runtime validation remains unverified.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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:
criticalbygpt-6.1-sol(effort low) — The intricate changes in shielded/prover.rs and shielded/operations.rs alter proving-key initialization, panic recovery, concurrent proof execution, and spend-authority ownership during funds-moving transition construction. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-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/src/wallet/shielded/prover.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/prover.rs:223-224: Log the permanent fallback when the Apple proving pool cannot initialize
The inline fallback preserves functionality, but `.ok()` discards the pool initialization error and the process-global `OnceLock` permanently caches `None`. Even a transient thread-creation failure therefore disables the dedicated proving pool for every subsequent job, while preparation can still succeed and readiness reports true. Since isolating proving from inherited background QoS is a stated goal of this change, log the source error before caching the fallback so persistent latency regressions can be diagnosed.
Out-of-scope follow-up suggestions (2)
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.
- Make pre-broadcast shielded note reservations cancellation-safe — The baseline already reserves notes before awaiting the recorded-anchor network query and activity recording, with cleanup performed only by a later result match. Dropping a native Rust send future can leave anchor-less reservations excluded from note selection and spendable balance; stale-pending recovery skips those reservations. The new proof await extends this existing cancellation window, but its closure does not broadcast and the synchronous FFI bridge drives sends to completion.
- Follow-up: Track cancellation-safe reservation ownership separately, with an explicit handoff to conservative recovery before broadcasting and an operation-level cancellation test.
- Guard the legacy shielded C spend exports against unwinding panics — The transfer, unshield, and withdrawal exports at shielded_send.rs lines 411, 489, and 563 call block_on_worker without an enclosing panic guard. On unwind-enabled builds, a worker panic reaches the bridge's join expect and then the extern "C" boundary, aborting the host before Swift or JNI can handle it. These exports are identically unguarded in the assigned baseline, and the new proof helper preserves their existing panic propagation.
- Follow-up: Track boundary hardening separately: guard these exports with conservative spend-outcome codes and add unwind-enabled FFI panic coverage.
| .build() | ||
| .ok() |
There was a problem hiding this comment.
🟡 Suggestion: Log the permanent fallback when the Apple proving pool cannot initialize
The inline fallback preserves functionality, but .ok() discards the pool initialization error and the process-global OnceLock permanently caches None. Even a transient thread-creation failure therefore disables the dedicated proving pool for every subsequent job, while preparation can still succeed and readiness reports true. Since isolating proving from inherited background QoS is a stated goal of this change, log the source error before caching the fallback so persistent latency regressions can be diagnosed.
| .build() | |
| .ok() | |
| .build() | |
| .inspect_err(|error| { | |
| tracing::warn!( | |
| %error, | |
| "could not create Orchard proving pool; using inline work with inherited QoS for the process lifetime" | |
| ); | |
| }) | |
| .ok() |
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
Closing: after measuring, this PR doesn't give the app a speedup a user can see, and its main premise doesn't hold. Darwin QoS is not inherited by new threads. A pthread created from a background-QoS pthread, or from a background or user-initiated GCD worker, starts at The other claims, measured (M4 Max, release, real proving key and a 2-action Shield proof, 8 interleaved old/new runs; desktop numbers):
The cost is +994/-140 lines and a breaking Rust API change ( 🤖 Posted autonomously by Claude on behalf of pasta. |
The awaitable prepare API proposed in dashpay/platform#5278 was closed, so polling `isShieldedProverReady` after `warmUpShieldedProver()` is the supported way to wait for the key, not a stopgap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The awaitable prepare API proposed in dashpay/platform#5278 was closed, so polling `isShieldedProverReady` after `warmUpShieldedProver()` is the supported way to wait for the key, not a stopgap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The awaitable prepare API proposed in dashpay/platform#5278 was closed, so polling `isShieldedProverReady` after `warmUpShieldedProver()` is the supported way to wait for the key, not a stopgap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue being fixed or feature implemented
Shielded sends had avoidable latency, and proving starved the async runtime:
OnceLock::get_or_init(~1.4 s release, ~30 s debug). A send that arrived mid-build parked a tokio worker for the rest of the build.warmUpShieldedProver()is fire-and-forget, so the only readiness signal was pollingisShieldedProverReady.Part of a series cutting Dash shielded send latency:
What was done?
rs-platform-wallet/src/wallet/shielded/prover.rs):warm_upjoins it.CachedOrchardProver::prepare(), and aShieldedProvertrait (ready()) that the send operations await before proving.Arc, so the spend authority is never copied. Shield, shield-from-identity and identity-create await the shared preparation.pthread_set_qos_class_self_np), 8 MiB stacks;rayon-core);platform_wallet_shielded_prepare_prover(), which returns once the key is ready. The existing warm-up entry point now joins the shared preparation.PlatformWalletManager.prepareShieldedProver() async throws(nonisolated). It uses a checked continuation on a user-initiated dispatch queue, so it never holds a cooperative-pool thread.warmUpShieldedProver()andisShieldedProverReadyare unchanged apart from becomingnonisolated.The proof itself isn't faster here; that's the halo2/orchard PRs. What this PR fixes:
How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-ffi --features platform-wallet/shielded,platform-wallet-ffi/shielded: 1401 + 426 lib tests plus integration suites pass. The prover tests passed 9 times in a row, including with--test-threads=64.-D warnings(wallet, FFI) andcargo fmt --checkare clean.27afa59735.xcodebuild, Swift 6 mode) against a header with the new declaration. It was not linked or run, and the XCFramework was not rebuilt.Breaking Changes
The
PlatformWalletshielded send methods (transfer, unshield, withdraw, identity top-up) now take:P: ShieldedProverinstead of anyOrchardProver(implemented byCachedOrchardProverand&CachedOrchardProver, so existing callers compile unchanged);keys: &Arc<OrchardKeySet>.All in-repo callers (FFI, JNI via FFI symbols) are updated.
Not changed here (follow-up):
fund_from_asset_lock/seed_poolstill prove inline throughOrchardProver.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
797c1aa/self-reviewedCargo.lock) — QuantumExplorer or shumkovrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/shielded_send.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/Cargo.toml,packages/rs-platform-wallet/src/wallet/platform_wallet.rs,packages/rs-platform-wallet/src/wallet/shielded/mod.rsand 2 more) — HashEngineering or ZocoLini or llbartekll or romchornyiswift-sdk(packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.