feat(swift-sdk): use SPV-synced quorums for Platform proof verification#3417
Open
QuantumExplorer wants to merge 22 commits into
Open
feat(swift-sdk): use SPV-synced quorums for Platform proof verification#3417QuantumExplorer wants to merge 22 commits into
QuantumExplorer wants to merge 22 commits into
Conversation
Bridge the SPV client's locally synced masternode list data to the Platform SDK's context provider, replacing the dependency on a trusted HTTP quorum endpoint. When an SPV client handle is available, the SDK is created with callback-based quorum lookups that call through to ffi_dash_spv_get_quorum_public_key and ffi_dash_spv_get_platform_activation_height. A "Fallback to Trusted Quorums" toggle (default: ON) in Settings lets users fall back to the HTTP provider when SPV data is not yet synced. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SPV-backed SDK creation using a client handle, introduces Rust quorum resolution through a feature-gated context provider, and adds configurable trusted-quorum fallback behavior to the Swift example app. A specification documents the planned reintegration architecture and validation requirements. ChangesSPV quorum integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppState
participant SDK
participant dash_sdk_create_with_spv_context
participant SpvContextProvider
AppState->>SDK: initialize with SPV client handle
SDK->>dash_sdk_create_with_spv_context: pass network and configured context
dash_sdk_create_with_spv_context->>SpvContextProvider: resolve quorum public key
SpvContextProvider-->>dash_sdk_create_with_spv_context: return quorum key or error
dash_sdk_create_with_spv_context-->>SDK: return SDK handle or error
SDK-->>AppState: use SPV SDK or trusted fallback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit 4961a0e) |
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "93f085d7f665b623b4cc170feb6c8fc04da6702b601f887c52f8e5181e471994"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- Around line 45-49: The published property useTrustedQuorumFallback currently
only persists the flag; update its didSet to immediately reapply the SDK
proof-verification mode so the running app reflects the change. After
UserDefaults.standard.set(...), invoke the function that configures the SDK
trust mode (for example the AppState method that initializes or configures the
SDK client—if you have a method like applySDKMode(), reconfigureClient(), or
setTrustedQuorumFallback(on:)), passing the new useTrustedQuorumFallback value;
if no such function exists, call the code path that reconnects or reinitializes
the SDK client so proof verification is updated immediately. Ensure you
reference AppState and useTrustedQuorumFallback when wiring this change.
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/UnifiedAppState.swift`:
- Around line 126-133: The current network-switch flow lets
walletService.switchNetwork(to:) destroy the SPV client while platformState.sdk
still holds callbacks to the old core handle; update
UnifiedAppState.handleNetworkSwitch to explicitly detach and destroy the old SDK
before tearing down the SPV client: capture the existing platformState.sdk (or
call a teardown method like
platformState.sdk?.destroy()/platformState.clearSDK()), set platformState.sdk =
nil (or call the SDK's close/unbind) on the MainActor to remove callbacks, then
call walletService.switchNetwork(to:) and only after obtaining
walletService.spvClientHandle recreate and assign the new SDK via
platformState.updateSPVClientHandle/new SDK creation — ensuring the old SDK is
fully unbound before SPV teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed48fb63-d5a9-41ff-b965-9df30eae9aab
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVContextProvider.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/UnifiedAppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift
The quorum/activation-height bridging no longer round-trips through Swift callbacks. Instead, rs-sdk-ffi now depends on dash-spv-ffi directly and implements SpvContextProvider in Rust, calling ffi_dash_spv_get_quorum_public_key and ffi_dash_spv_get_platform_activation_height without leaving Rust. A new FFI function dash_sdk_create_with_spv_context(config, spv_client) replaces the Swift-side ContextProviderCallbacks setup. Swift just passes the raw FFIDashSpvClient pointer. - Add dash-spv-ffi dependency to rs-sdk-ffi - Add spv_context_provider.rs implementing ContextProvider trait - Add dash_sdk_create_with_spv_context FFI function - Remove SPVContextProvider.swift (no longer needed) - Update SDK.swift to call dash_sdk_create_with_spv_context Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3417 +/- ##
=============================================
- Coverage 87.40% 64.77% -22.63%
=============================================
Files 2639 24 -2615
Lines 333066 2657 -330409
=============================================
- Hits 291103 1721 -289382
+ Misses 41963 936 -41027
🚀 New features to boost your workflow:
|
Add SpvContextProvider to platform-wallet behind the `spv-context` feature flag. It holds Arc<RwLock<MasternodeListEngine>> + Network and implements the ContextProvider trait by reading quorum data directly from the in-memory masternode list — zero FFI involvement. The rs-sdk-ffi bridge (which calls dash-spv-ffi functions as Rust calls in the same binary) remains as the pragmatic adapter until dash-spv-ffi exposes a public accessor for FFIDashSpvClient's MasternodeListEngine (currently pub(crate)). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/Cargo.toml`:
- Line 25: The tokio dependency must enable its sync feature for the spv-context
feature and the feature list must use the modern dep: syntax; update Cargo.toml
so the tokio dependency becomes optional with features = ["sync"] (e.g. tokio =
{ version = "1.41", optional = true, features = ["sync"] }) and update the
spv-context feature entry to include "dep:tokio/sync" (instead of the old name)
so spv_context_provider.rs can safely use tokio::sync::RwLock.
In `@packages/rs-platform-wallet/src/spv_context_provider.rs`:
- Around line 115-120: The match that sets `height` currently falls through to
`_ => 0`, silently mapping unsupported `self.network` to 0; replace that
fallback with an explicit error return (e.g., return Err(...) or propagate a
suitable error) so callers are notified of an unsupported Network variant.
Modify the block that computes `height` (the match on `self.network` referencing
`Network::Mainnet`, `Network::Testnet`, `Network::Devnet`) to return a
Result<u64, E> (or use the crate's existing error type) and produce a clear
error for the unsupported branch instead of using 0. Ensure callers of the
enclosing function are updated to handle the Result if necessary.
- Around line 79-80: The call to self.masternode_engine.blocking_read() inside
get_quorum_public_key (which is invoked from async
parse_proof_with_metadata_and_proof) can panic; replace blocking_read() with
try_read() on the Tokio RwLock (self.masternode_engine.try_read()) and handle
the None case gracefully by returning an appropriate error or early failure from
get_quorum_public_key (or falling back to a safe default) instead of unwrapping.
Ensure you reference and use the masternode_lists_around_height result from the
acquired guard when present, and propagate a clear error back to the async proof
verification path so it doesn’t panic.
- Around line 76-77: Validate and reject out-of-range quorum_type before
converting to LLMQType: replace the lossy cast "(quorum_type as u8).into()" with
a fallible conversion using TryInto (e.g., let llmq_type: LLMQType =
(quorum_type.try_into()?) or map the TryInto error to a meaningful error) so
values >255 are handled instead of wrapping; keep
QuorumHash::from_byte_array(quorum_hash) as-is and ensure any error returned
propagates to the caller before using ml.quorums.get(&llmq_type).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b462740-73cb-43f3-9a1b-7653a611fe5e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
packages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/spv_context_provider.rspackages/rs-sdk-ffi/src/spv_context_provider.rs
✅ Files skipped from review due to trivial changes (1)
- packages/rs-platform-wallet/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk-ffi/src/spv_context_provider.rs
- AppState: trigger SDK reconnect when useTrustedQuorumFallback changes so the running app reflects the new setting immediately - UnifiedAppState: nil out old SDK before destroying SPV client during network switch to prevent use-after-free on the context provider's core_handle pointer - Cargo.toml: explicitly enable tokio "sync" feature for blocking_read() support - spv_context_provider.rs: replace lossy `as u8` cast with try_from + error, add comment explaining why blocking_read is safe, return error for unsupported networks instead of silent 0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bump rust-dashcore to 3f65002 which includes the FFIDashSpvClient::masternode_list_engine() accessor (PR #608). dash_sdk_create_with_spv_context now: 1. Casts the void* to FFIDashSpvClient 2. Extracts Arc<RwLock<MasternodeListEngine>> directly 3. Creates platform_wallet::SpvContextProvider (pure Rust) The FFI bridge module (rs-sdk-ffi/src/spv_context_provider.rs) is deleted — quorum lookups go directly from the Platform SDK through the pure-Rust provider to the in-memory masternode list, with zero FFI involvement in the data path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
All review feedback addressed. Additionally, bumped rust-dashcore to |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift (1)
215-267:⚠️ Potential issue | 🟠 MajorMake the network switch commit atomically.
The new SPV-first failure paths can throw after the collections are cleared and
dataManager.currentNetworkis advanced, but beforesdkis replaced. On that path the UI says “new network” while platform calls still go through the old SDK instance. IfcurrentNetworkshould also be authoritative, it needs the same success-only commit behavior.♻️ Possible fix
func switchNetwork(to network: AppNetwork) async { guard let modelContext = modelContext else { return } - - // Clear current data - identities.removeAll() - contracts.removeAll() - documents.removeAll() - tokens.removeAll() - - // Update DataManager's current network - dataManager?.currentNetwork = network // Re-initialize SDK with new network do { isLoading = true @@ - sdk = newSDK + identities.removeAll() + contracts.removeAll() + documents.removeAll() + tokens.removeAll() + dataManager?.currentNetwork = network + sdk = newSDK🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift` around lines 215 - 267, The switchNetwork method mutates state (clearing identities/contracts/documents/tokens and advancing dataManager?.currentNetwork) before potentially failing during SDK creation and load; make the switch atomic by first creating and initializing the new SDK and loading contracts/data (use local temporaries like let newSDK and temp collections), and only after loadKnownContractsIntoSDK and loadPersistedData succeed assign sdk = newSDK, set dataManager?.currentNetwork = network, and replace identities/contracts/documents/tokens with the newly loaded data; keep all error-throwing work (SDK(network:), loadKnownContractsIntoSDK, loadPersistedData) before mutating shared state so failures leave the existing sdk and currentNetwork unchanged.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift (1)
104-124: Extract the SPV/trusted SDK creation path into one helper.The same fallback tree now lives in both
initializeSDKandswitchNetwork. The next change to error handling or provider selection is easy to land in one path and miss in the other.Also applies to: 234-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift` around lines 104 - 124, Extract the SPV/trusted SDK creation logic used in initializeSDK and switchNetwork into a single helper function (e.g., makeSDKForNetwork(network:sdkNetwork, spvHandle:spvClientHandle?, useFallback:useTrustedQuorumFallback) -> SDK) that encapsulates the fallback tree: try constructing SDK(network: , spvClientHandle:) if spvHandle != nil, on failure fall back to SDK(network:) only if useFallback is true, otherwise rethrow; if spvHandle is nil create SDK(network:) when useFallback is true or throw SDKError.invalidState when false. Replace the duplicated blocks in initializeSDK and switchNetwork to call this new helper and preserve the same NSLog messages and error propagation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- Around line 143-146: The race occurs because updateSPVClientHandle(_:) runs in
handleNetworkSwitch() while switchNetwork(...) can be started asynchronously
from the currentNetwork didSet in OptionsView; modify handleNetworkSwitch() so
that after calling updateSPVClientHandle(_:) it immediately triggers/awaits the
SDK rebuild (call switchNetwork(...) directly or await the spawned Task) to
ensure the SDK is rebuilt with the new spvClientHandle before returning;
reference updateSPVClientHandle(_:), handleNetworkSwitch(), switchNetwork(...),
currentNetwork didSet/OptionsView to locate and change the code paths so the
handle update is always followed synchronously by the SDK rebuild.
---
Outside diff comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- Around line 215-267: The switchNetwork method mutates state (clearing
identities/contracts/documents/tokens and advancing dataManager?.currentNetwork)
before potentially failing during SDK creation and load; make the switch atomic
by first creating and initializing the new SDK and loading contracts/data (use
local temporaries like let newSDK and temp collections), and only after
loadKnownContractsIntoSDK and loadPersistedData succeed assign sdk = newSDK, set
dataManager?.currentNetwork = network, and replace
identities/contracts/documents/tokens with the newly loaded data; keep all
error-throwing work (SDK(network:), loadKnownContractsIntoSDK,
loadPersistedData) before mutating shared state so failures leave the existing
sdk and currentNetwork unchanged.
---
Nitpick comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- Around line 104-124: Extract the SPV/trusted SDK creation logic used in
initializeSDK and switchNetwork into a single helper function (e.g.,
makeSDKForNetwork(network:sdkNetwork, spvHandle:spvClientHandle?,
useFallback:useTrustedQuorumFallback) -> SDK) that encapsulates the fallback
tree: try constructing SDK(network: , spvClientHandle:) if spvHandle != nil, on
failure fall back to SDK(network:) only if useFallback is true, otherwise
rethrow; if spvHandle is nil create SDK(network:) when useFallback is true or
throw SDKError.invalidState when false. Replace the duplicated blocks in
initializeSDK and switchNetwork to call this new helper and preserve the same
NSLog messages and error propagation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d056420-6f71-49b1-ad49-3434618ac7b3
📒 Files selected for processing (4)
packages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/spv_context_provider.rspackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/UnifiedAppState.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-platform-wallet/Cargo.toml
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/UnifiedAppState.swift
- packages/rs-platform-wallet/src/spv_context_provider.rs
Remove the switchNetwork() call from currentNetwork.didSet to prevent it racing with UnifiedAppState.handleNetworkSwitch(). The didSet now only persists the preference. handleNetworkSwitch() is the single coordinator: it nils the old SDK, switches the wallet, updates the SPV handle, then calls switchNetwork() to rebuild the SDK. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace blocking_read() with try_read() in SpvContextProvider to avoid panicking when called from within a Tokio async context (proof verification runs inside async tasks) - Use dep: prefix syntax in spv-context feature to suppress implicit feature creation for optional dependencies (Cargo 2021 edition) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
…ovider Brings PR #3417 (SPV-synced Platform proof verification) up to date with v4.1-dev (~827 commits) and re-implements its obsolete integration surface against the restructured PlatformWalletManager architecture. Re-integration: - Delegate quorum lookups to SpvRuntime::get_quorum_public_key via a thin SpvContextProvider { Arc<SpvRuntime>, Network }, bridging the sync trait method to the async lookup with block_in_place + Handle::try_current. - New FFI entry platform_wallet_manager_create_sdk_with_spv_context in platform-wallet-ffi (rs-sdk-ffi cannot reach the manager handle without a dependency cycle). Make rs_sdk_ffi::ContextProviderWrapper public; delete the now-dead dash_sdk_create_with_spv_context and drop the dash-spv-ffi dep. - Swift: SDK.init(network:walletManager:) (@mainactor; captures the Sendable Handle before the nonisolated C-string closure); AppState builds the SDK SPV-first with trusted fallback; accept base's deletion of SPVClient / WalletService / UnifiedAppState. Correctness fixes: - Remove the erroneous `.reverse()` in SpvRuntime::get_quorum_public_key. quorum_hash is internal byte order end-to-end (drive-abci emits it via to_byte_array, proven by the consensus-equality check in finalize_block_proposal; the masternode engine keys its quorum map in the same order), so reversing missed every quorum -> fail-closed rejection, silently masked by the trusted-quorum fallback (SPV never actually used). Added a regression test pinning the byte order (internal-order hits, reversed misses). - Correct get_platform_activation_height to the shipped trusted-provider values (Mainnet 2_132_092, Testnet 1_090_319; were dash-spv-ffi "needs verification" placeholders). - Reclaim the ContextProviderWrapper box after dash_sdk_create_extended (which only borrows + Arc-clones it) to avoid leaking it per SDK creation. Merge breakages resolved: mergiraf-duplicated [workspace.dependencies] with conflicting rust-dashcore revs; platform-wallet-ffi <-> rs-sdk-ffi dependency cycle; deleted `manager` feature; redundant spv-context feature gating. Verified: 3 Rust crates compile + clippy-clean + fmt; byte-order regression test passes; SwiftExampleApp (simulator) BUILD SUCCEEDED; cbindgen header emits the new FFI function correctly. Not verified: end-to-end proof verification against live SPV-synced quorums (integration/on-device only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/SPV_CONTEXT_PROVIDER_REINTEGRATION_SPEC.md`:
- Line 3: Update the document status line in
SPV_CONTEXT_PROVIDER_REINTEGRATION_SPEC.md to describe PR `#3417` as open and
proposed for reintegration, removing the claim that it was merged onto v4.1-dev.
- Around line 83-87: Update get_platform_activation_height to use the
authoritative verification-plan activation heights: Mainnet 2,132,092 and
Testnet 1,090,319. Keep Devnet and Regtest returning 1 and preserve the existing
error behavior for unsupported networks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce81946-ef62-4086-8be3-e3322b91db5b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
docs/SPV_CONTEXT_PROVIDER_REINTEGRATION_SPEC.md
thepastaclaw
suggested changes
Jul 11, 2026
Collaborator
There was a problem hiding this comment.
Code Review
This PR's core Rust quorum byte-order fix is real and correctly reasoned, and the FFI ownership/memory-safety work is solid, but I verified against source that the shipped iOS integration never actually exercises SPV-backed proof verification: bootstrap wires the new constructor to an unconfigured placeholder wallet manager, network switching permanently reverts to the trusted-only path, and the same constructor falls back to SdkBuilder::new_mock() for ordinary mainnet/testnet use — all confirmed by reading the exact call chains. I also verified from the pinned dash-spv revision that get_quorum_at_height only filters Invalid quorums, so Unknown/Skipped entries can be trusted as a proof-verification root, undermining the 'trustless' claim, and confirmed empirically-reasoned that the new regression test never calls the function it claims to guard. 2 nitpicks (a confirmed cargo fmt failure and an FFI safety-doc gap) were cut for budget.
🔴 6 blocking | 🟡 4 suggestion(s)
5 additional finding(s) omitted (not in diff).
🤖 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/SwiftExampleAppApp.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:319-320: Bootstrap passes the unconfigured placeholder wallet manager to the new SPV initializer
Verified: `WalletManagerStore.activeManager` is initialized in its `init` as a bare `PlatformWalletManager()` placeholder (`WalletManagerStore.swift:53,81`) held until `activate(...)` replaces it later. The `walletManager` computed property in `SwiftExampleAppApp.swift:54-55` returns exactly that placeholder, and `bootstrap()` passes it into `platformState.initializeSDK(...)` at line 319-320 *before* `walletManagerStore.activate(...)` runs at line 332. `AppState.initializeSDK` stores this handle and `makeSDK` calls `SDK(network:walletManager:)` with it, which forwards a null/placeholder manager handle to Rust — `platform_wallet_manager_create_sdk_with_spv_context` looks it up in `PLATFORM_WALLET_MANAGER_STORAGE` and returns `"Invalid wallet manager handle"` (`rs-platform-wallet-ffi/src/spv.rs:527-534`). With the default `useTrustedQuorumFallback = true` (`AppState.swift:86-90`), this failure is silently swallowed and a trusted-HTTP SDK is built instead (`AppState.swift:142-147`) — so the feature this PR reintroduces is never actually activated on a normal app launch. With the toggle off, bootstrap fails outright and the real manager is never configured. The real per-network manager created by `activate(...)` afterward is never fed back into a fresh `SDK(network:walletManager:)` call.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:160-177: Network switch always rebuilds a trusted-only SDK, permanently disabling SPV verification and ignoring the user's opt-out toggle
Verified: `initializeSDK` builds the first SDK through `makeSDK(for:)`, which prefers `SDK(network:walletManager:)` (the new SPV path) and only falls back to `SDK(network:)` if that throws and `useTrustedQuorumFallback` is true (`AppState.swift:136-153`). `switchNetwork(to:)`, however, calls `try SDK(network: network)` directly at line 176, bypassing `makeSDK` entirely — so *every* runtime network switch (and every devnet rebuild) reverts proof verification to the centralized HTTP quorum provider, regardless of whether `useTrustedQuorumFallback` is off and the UI explicitly promises 'require SPV-synced quorums for proof verification' (`OptionsView.swift:351`). This is a genuine trust-policy violation, not just a routing bug: a compromised or malicious quorum HTTP endpoint, combined with a colluding DAPI response, can substitute an attacker-controlled BLS key and have it accepted as valid — exactly the class of attack SPV verification is meant to prevent — silently, after an action (switching networks) that gives no indication proof verification just weakened. A correct fix requires coordinating with the correct per-network `PlatformWalletManager` (not just swapping in `makeSDK`, which could use a stale/wrong-network manager) before rebuilding through the SPV initializer.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/sdk.rs:216-218: SPV-backed SDK constructor silently builds an offline mock SDK for ordinary mainnet/testnet use
Verified end-to-end: for plain mainnet/testnet, `SDK.init(network:walletManager:)` deliberately leaves `overrideAddresses` nil (`SDK.swift:407-414`), so `dapi_addresses` is a null pointer in the config passed to `platform_wallet_manager_create_sdk_with_spv_context`, which forwards it unchanged into `dash_sdk_create_extended` (`rs-platform-wallet-ffi/src/spv.rs:543-557`). In `dash_sdk_create_extended`, a null `dapi_addresses` takes the `SdkBuilder::new_mock().with_network(network)` branch (`sdk.rs:216-218`) — there is no real-network branch here at all. This is inconsistent with the sibling `dash_sdk_create_trusted`, which for the exact same null-address case maps `Network::Testnet`/`Network::Mainnet` to `SdkBuilder::new_testnet()`/`new_mainnet()` and only falls through to an error for devnet/regtest (`sdk.rs:415-420`). Once bootstrap wiring (see the linked finding) is fixed and a real manager handle reaches this path, every ordinary SPV-backed SDK creation would silently return a `MockDapiClient`-backed handle that looks successful but can never reach real DAPI nodes.
In `packages/rs-platform-wallet/src/spv_context_provider.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv_context_provider.rs:98-115: SpvContextProvider replaces the entire ContextProvider, unconditionally dropping contract/token lookups and breaking document operations
Installing `SpvContextProvider` swaps out the SDK's *whole* `ContextProvider`, not just quorum-key resolution, yet `get_data_contract`/`get_token_configuration` always return `Ok(None)` (lines 98-115). The extended FFI constructor also always sets `SDKWrapper { trusted_provider: None }` (`rs-sdk-ffi/src/sdk.rs:287`). I verified `wrapper.trusted_provider` is a hard dependency across the FFI surface: `dash_sdk_add_known_contracts` rejects with `InvalidState` when it's `None` (`sdk.rs:703-711`), and document `put`/`replace`/`transfer`/`price` operations return `"No trusted context provider configured"` in that case (e.g. `document/put.rs:82-99`, confirmed identically in `transfer.rs`, `price.rs`, `replace.rs`). So an SPV-created SDK can never load the contracts `AppState.loadKnownContractsIntoSDK` supplies, and every document write/read-with-contract-resolution operation breaks — the SPV path trades away core SDK functionality that has nothing to do with quorum verification.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:58-116: SpvContextProvider — the production ContextProvider implementation — has zero test coverage
The only test this PR adds (the tautological BTreeMap test above) doesn't touch this file. The actual `ContextProvider` impl that runs in production — including the `Handle::try_current()` + `block_in_place` sync/async bridge, the `ContextProviderError` mapping, and the `get_platform_activation_height` match — has no test constructing an `SpvContextProvider` and exercising it. Given this is new bridging code (nested-runtime handling) directly on the proof-verification path, a test with a mock/started `SpvRuntime` covering at least `get_quorum_public_key` success/failure and `get_platform_activation_height` would materially improve confidence in the change.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:71-83: block_in_place bridge trusts whatever runtime is ambient, and diverges from the checked-in design spec
`get_quorum_public_key` resolves `Handle::try_current()` fresh on every call and wraps `handle.block_on(...)` in `tokio::task::block_in_place`. `try_current()` only guards against 'no runtime at all' — it doesn't check runtime *flavor*; `block_in_place` panics if invoked from a `current_thread` runtime (e.g. a future FFI/query path, or a `#[tokio::test]` default-flavor test, that doesn't route through `BigStackRuntime`'s multi-thread runtime). Today this is safe only because every existing call path happens to enter `BigStackRuntime::block_on` (`new_multi_thread()`, confirmed in `rs-sdk-ffi/src/runtime.rs:60-65`) first, and neither FFI crate wraps `extern "C"` entry points in `catch_unwind`, so a future flavor mismatch would unwind across the FFI boundary. Separately, the checked-in spec doc's Layer 1 pseudocode says the `Handle` is 'captured at construction' specifically so the bridge works 'even if a future caller invokes us off the runtime thread' — the shipped code instead re-resolves `try_current()` per call (arguably more robust, since it fails closed rather than trusting a possibly-stale handle, but it means the spec and implementation now disagree, which could mislead future readers of that doc).
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:155-181: Quorum lookup trusts Unknown/Skipped quorum entries as a proof-verification root, not just Verified ones
I traced this against the exact pinned `dash-spv` revision (`1860089eddea27f66e5b3e637d18a73ae138478c`, present in the local cargo git checkout cache). `get_quorum_at_height` (`dash-spv/src/client/queries.rs:56-60`) calls `quorum_entry_for_hash_at_or_before_height`, which filters candidates with `!matches!(quorum.verified, LLMQEntryVerificationStatus::Invalid(_))` (`dash/src/sml/masternode_list_engine/helpers.rs:82-84`) — i.e. it accepts `Unknown` (the `#[default]` status before any validation attempt) and `Skipped(...)` (deferred validation due to missing supporting data, which can persist indefinitely if the required infra never arrives) just as readily as `Verified`. `SpvContextProvider::get_quorum_public_key` (this PR's new code) hands whatever key comes back straight to `verify_tenderdash_proof` as the trust root, with no status check of its own. This PR is what newly activates this lookup as a live proof-verification root (the prior `.reverse()` version made it dead-on-arrival); a quorum entry that a peer supplied but that was never (or not yet) cryptographically validated can therefore authenticate a proof. The PR's own spec doc explicitly defers this ('`verified`-status gate on quorum entries... track separately', `docs/SPV_CONTEXT_PROVIDER_REINTEGRATION_SPEC.md:228-229`), but since the whole feature's 'trustless' claim depends on it and this PR is what wires the dependency up, it belongs in scope rather than as a pure follow-up.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:483-543: "Regression test" for the quorum byte-order fix never calls the function it claims to protect
`quorum_hash_lookup_uses_internal_order_not_reversed` builds its own standalone `BTreeMap<QuorumHash, [u8;48]>`, inserts a key, and asserts the same expression finds it while a `.reverse()`d key doesn't — it never calls `SpvRuntime::get_quorum_public_key`, `SpvClient`, or the masternode engine. It's a tautology: `BTreeMap::get` distinguishing a byte-reversed key from the original is true no matter what `get_quorum_public_key` actually does. Re-introducing the removed `.reverse()` at `runtime.rs:173` would not fail this test. This matters because the PR frames this exact bug as 'the feature-killer' and states in its own checklist that the test 'Must be RED against the `.reverse()` version and GREEN after removal — this is the proof the fix is real, not a tautology' — the delivered test is precisely the tautology it disclaims being, so the PR's stated safety net for its headline fix doesn't exist.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:155-181: Quorum lookup now blocks (fail-slow) on SPV writer contention instead of failing fast
`get_quorum_public_key` takes `self.client.read().await`, which per the PR's own spec doc (§4, risk #1) now blocks until any concurrent writer (a large QRINFO `apply_diff` during masternode-list sync, or `SpvRuntime::run()`/`stop()`'s `write().await`) releases the lock, instead of failing fast the way the previous `try_read()` design did. A remote peer influences the pace/size of masternode-list diffs the SPV client processes, so a slow or adversarial peer could prolong the write-lock hold and stall every proof verification routed through `SpvContextProvider` on the shared single-worker `BigStackRuntime` for the duration. Verification still fails closed eventually rather than accepting anything wrong, so this is an availability concern, not a correctness break — but a bounded `tokio::time::timeout` around the lookup would prevent an unbounded stall.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:393-414: DAPI-address/quorum-URL resolution logic is duplicated verbatim between the two SDK initializers
`init(network:walletManager:)` (lines 393-414) duplicates the devnet-auto-discovery / regtest-and-`useDockerSetup`-override / plain-mainnet-testnet-passthrough logic that already exists in `init(network:)` just above it (lines ~298-338) — the comment even says 'identically to init(network:)'. This encodes several subtle, non-obvious per-network rules (documented at length in the original); having two copies risks them silently drifting apart the next time either one changes.
Source: orchestrator openai/gpt-5.6-sol (high, orchestration-only; not reviewer/verifier); reviewers gpt-5.6-sol (general, security-auditor, ffi-engineer) and claude-sonnet-5 (general, security-auditor, ffi-engineer); verifier claude-sonnet-5; experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3 (not Opus-sampled).
…on spec - rustfmt line-wrap of the wrapper-reclaim call in the SPV SDK-create FFI function (fixes the `Check formatting` CI step; `cargo fmt --all --check` clean). - Remove docs/SPV_CONTEXT_PROVIDER_REINTEGRATION_SPEC.md — an internal design doc that doesn't belong in the PR (its content lives in the merge commit's history). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 11, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers codex/general=gpt-5.6-sol(completed), sonnet5/general=claude-sonnet-5(completed), codex/security-auditor=gpt-5.6-sol(completed), sonnet5/security-auditor=claude-sonnet-5(completed), codex/ffi-engineer=gpt-5.6-sol(completed), sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (orchestration-only, not a reviewer/verifier).
Prior Reconciliation
All 10 prior findings were independently re-verified against the actual source at HEAD 05fd41d6c6fe7f0dc3cec3daecd491678cbc666b (not against the public GitHub thread claims, which describe code that does not exist in this tree). All 10 remain STILL VALID. None were fixed. codex-ffi-engineer's classification of prior-5 and prior-9 as INTENTIONALLY DEFERRED is overridden here to STILL VALID: that classification was based on a since-deleted internal spec doc (docs/SPV_CONTEXT_PROVIDER_REINTEGRATION_SPEC.md, removed in this very commit), not a maintainer decision recorded anywhere in the PR conversation — a deleted design doc cannot constitute an intentional deferral. Several coderabbitai/QuantumExplorer threads claim specific fixes landed (a didSet-triggered switchNetwork, a unified handleNetworkSwitch(), u8::try_from(quorum_type), try_read() replacing blocking_read(), an activation-height error path) — every one of these was checked directly against the current file/line and disproven; the code at those locations is unchanged from what the findings originally described.
Carried-Forward Prior Findings
prior-1 through prior-10 are all carried forward unchanged in severity, each re-verified line-by-line this cycle (see prior_findings_reconciliation). Notably, prior-4 was strengthened during this cycle: since SDKWrapper.trusted_provider is unconditionally None on the SPV construction path (rs-sdk-ffi/src/sdk.rs:282-291), and every document write FFI (put.rs, replace.rs, transfer.rs, price.rs, purchase.rs, delete.rs, queries/fetch.rs) unconditionally returns FFIError::InternalError("No trusted context provider configured") when trusted_provider is None, SPV-mode SDKs cannot perform any document operation at all — not degraded, but hard-broken.
New Findings In Latest Delta
The literal delta (6f678702f5..05fd41d6c6) is a pure rustfmt line-wrap of one drop(Box::from_raw(...)) call in rs-platform-wallet-ffi/src/spv.rs plus the deletion of the 230-line internal spec doc. Zero functional changes. No new findings from this delta; all 6 lanes agree.
Additional Cumulative Findings
One cumulative-only observation (useTrustedQuorumFallback toggle has no runtime effect) was folded into prior-2 below rather than listed separately, since it's the same root defect (nothing outside initializeSDK ever re-derives SDK trust mode). No other new cumulative findings beyond the original 10 were substantiated on re-inspection.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3, trigger new_push, prior head 6f67870): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-3417-1783776729=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 6 blocking | 🟡 4 suggestion(s)
Prior Reconciliation
prior-1: STILL VALID — WalletManagerStore.swift:53/81 confirms activeManager starts as a bare unconfigured placeholder; SwiftExampleAppApp.swift:319-320 confirms initializeSDK runs against that placeholder before activate() at line 332 replaces it. No code change addresses this ordering.prior-2: STILL VALID — AppState.swift:160-194 switchNetwork still unconditionally calls trusted-onlySDK(network:); useTrustedQuorumFallback's didSet (lines 45-51) still only persists to UserDefaults. The coderabbitai-thread-claimed didSet-triggered switchNetwork / handleNetworkSwitch() function does not exist anywhere in AppState.swift.prior-3: STILL VALID — rs-sdk-ffi/src/sdk.rs:191-294 dash_sdk_create_extended still falls back to SdkBuilder::new_mock() on null dapi_addresses; dash_sdk_create_trusted (307-484) still resolves real network seeds for the same case. Asymmetry unchanged.prior-4: STILL VALID — sdk.rs:282-291 confirms trusted_provider: None unconditionally on the SPV path; verified this cycle that every document-write FFI (put/replace/transfer/price/purchase/delete/fetch) hard-errors when trusted_provider is None, so SPV SDKs cannot perform any document operation at all. spv_context_provider.rs:98-115 get_data_contract/get_token_configuration still unconditionally Ok(None).prior-5: STILL VALID — runtime.rs:155-181 get_quorum_public_key still returns the quorum public key unconditionally with no verification-status check; grep across packages/rs-platform-wallet/src/spv/ for verified/verification_status/LLMQEntryVerificationStatus returns zero matches. Overriding codex-ffi-engineer's INTENTIONALLY DEFERRED classification: that was based on a spec doc deleted in this very commit, not a real maintainer decision recorded in the PR thread.prior-6: STILL VALID — runtime.rs:469-511 regression test still only exercises a standalone BTreeMap<QuorumHash,[u8;48]>, never constructs SpvRuntime or calls get_quorum_public_key. Tautological, does not guard the production code path.prior-7: STILL VALID — spv_context_provider.rs (117 lines total) still has no #[cfg(test)] module of any kind.prior-8: STILL VALID — SDK.swift:381-438 still independently re-derives useOverrideAddresses/overrideQuorumURL/overrideAddresses network-selection logic, duplicated against decisions made elsewhere rather than resolved via one Rust-side call, per the Swift-SDK CLAUDE.md architectural rule.prior-9: STILL VALID — runtime.rs:162 still usesself.client.read().await(blocking), nottry_read(). The coderabbitai/QuantumExplorer thread claiming try_read() was substituted does not match the code at this line. Overriding codex-ffi-engineer's INTENTIONALLY DEFERRED classification for the same reason as prior-5 — no real maintainer deferral exists, only a stale thread claim.prior-10: STILL VALID — spv_context_provider.rs:71-84 still pairs Handle::try_current() with unconditional block_in_place; try_current() cannot distinguish current_thread vs multi_thread runtime flavor, so the panic risk on current_thread callers is unchanged and undocumented beyond the general call-time-resolution note in the module doc.
Carried-Forward Prior Findings
blocking: WalletManagerStore bootstraps SDK against a placeholder, unconfigured wallet manager before the real per-network manager exists
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:319-320
WalletManagerStore.activeManager is initialized to a bare, unconfigured PlatformWalletManager() placeholder (WalletManagerStore.swift:81, doc comment at 48-53 confirms this is intentional-but-temporary). bootstrap() calls platformState.initializeSDK(modelContext:, walletManager: walletManager) at line 319-320 using this placeholder — then only at line 332, after a Task.sleep(for: .milliseconds(500)), does it call walletManagerStore.activate(network:sdk:) to materialize the real, configured manager. The SDK built during initializeSDK is never rebuilt against the real manager, so the app boots with an SDK bound to a manager that has no wallet state, no SPV runtime, and no persistence handler wired up.
blocking: switchNetwork() always rebuilds a trusted-only SDK, permanently bypassing SPV even when a wallet manager is available; the SPV/trusted toggle is inert
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:160-177
switchNetwork(to:) (lines 160-194) calls try SDK(network: network) unconditionally — the trusted-only initializer — and never calls makeSDK(for:), the SPV-preferring helper that actually reads self.walletManager. Every network switch after initial boot silently drops to the trusted HTTP quorum provider regardless of SPV availability. Compounding this, useTrustedQuorumFallback's didSet (lines 45-51) only writes to UserDefaults — it never triggers a rebuild via switchNetwork or any other path, so flipping the toggle in Settings has zero runtime effect until the next full app relaunch. Public threads claiming a didSet { Task { await switchNetwork(...) } } fix or a unified handleNetworkSwitch() do not match the code at these lines.
blocking: dash_sdk_create_extended silently falls back to an offline mock SDK when DAPI addresses are null, instead of resolving canonical network seeds like its trusted-path sibling
packages/rs-sdk-ffi/src/sdk.rs:216-218
In dash_sdk_create_extended (SPV path), base_config.dapi_addresses.is_null() routes to SdkBuilder::new_mock().with_network(network) — a fully offline mock SDK with no real network connectivity. The sibling dash_sdk_create_trusted (lines 307-484) handles the identical null-address case correctly at lines 415-430 by resolving SdkBuilder::new_testnet()/new_mainnet() for the well-known networks and only erroring for genuinely unsupported ones. Swift's SDK.swift:381-438 passes overrideAddresses = nil for plain mainnet/testnet SPV construction (only regtest/devnet/docker-override paths set explicit addresses), so any plain mainnet/testnet SPV SDK silently becomes a non-functional mock instead of failing loudly or resolving real seeds.
blocking: SPV-constructed SDKWrapper.trusted_provider is always None, which hard-fails every document write operation (put/replace/transfer/price/purchase/delete/fetch)
packages/rs-sdk-ffi/src/sdk.rs:282-291
dash_sdk_create_extended constructs SDKWrapper { sdk, runtime, trusted_provider: None } unconditionally (lines 282-291) — there is no code path that populates trusted_provider from the SPV context provider. Every document-mutation FFI function checks if let Some(ref provider) = wrapper.trusted_provider { ... } else { return Err(FFIError::InternalError("No trusted context provider configured")) } (verified in document/put.rs:83-100, replace.rs:81, transfer.rs:114/285, price.rs:87/229, purchase.rs:103/275, delete.rs:100/285, document/queries/fetch.rs:57). This means an SPV-mode SDK — the entire point of this PR — cannot put, replace, transfer, price, purchase, delete, or fetch-with-contract-resolution any document; it hard-errors on all of them. This is compounded by SpvContextProvider::get_data_contract and get_token_configuration (spv_context_provider.rs:98-115) both unconditionally returning Ok(None), so even a hypothetical alternate resolution path through the ContextProvider trait itself would also come up empty.
blocking: get_quorum_public_key trusts quorum public keys as the proof-verification root without checking verification status
packages/rs-platform-wallet/src/spv/runtime.rs:155-181
SpvRuntime::get_quorum_public_key (lines 155-181) fetches a quorum via client.get_quorum_at_height(...) and returns quorum.quorum_entry.quorum_public_key unconditionally — there is no gate on LLMQEntryVerificationStatus (Unknown/Verified/Skipped/Invalid) anywhere in this file or in spv_context_provider.rs (confirmed via grep — zero matches for verified/verification_status/LLMQEntryVerificationStatus in packages/rs-platform-wallet/src/spv/). Since this public key is the trust anchor the SDK uses to verify platform proofs, an Unknown or Skipped (unverified) quorum entry is trusted exactly the same as a cryptographically Verified one, defeating the security purpose of proof verification. The public thread claiming a fix here refers to code that isn't present.
blocking: The regression test for the quorum-hash byte-order fix never calls the production function it claims to protect
packages/rs-platform-wallet/src/spv/runtime.rs:469-511
quorum_hash_lookup_uses_internal_order_not_reversed (lines 469-511) builds a standalone BTreeMap<QuorumHash, [u8;48]>, inserts a key, and asserts basic BTreeMap::get lookup semantics for reversed vs. non-reversed QuorumHash values. It never constructs an SpvRuntime, never calls SpvRuntime::get_quorum_public_key (the actual function that previously had the byte-order bug, lines 155-181), and never exercises any SPV client or masternode-list state. This is a tautological test about QuorumHash/BTreeMap ordering, not a regression guard on the function whose bug it's named after — a future re-introduction of the reversed-hash bug in get_quorum_public_key would not be caught by this test.
suggestion: SpvContextProvider has zero unit test coverage
packages/rs-platform-wallet/src/spv_context_provider.rs:58-116
The full 117-line file has no #[cfg(test)] module. None of get_quorum_public_key's sync/async bridging (block_in_place + try_current), the get_platform_activation_height per-network match, or the always-Ok(None) get_data_contract/get_token_configuration stubs have any direct test coverage. Given this struct is the trust root the whole SPV proof-verification path depends on, this is a meaningful gap even though it's not (yet) a functional bug in itself.
suggestion: Network-resolution / override-address logic is duplicated between SDK.swift and AppState.swift instead of living in one place
packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:381-438
The SPV-path initializer (SDK.swift:381-438) independently re-derives useOverrideAddresses (regtest/devnet/useDockerSetup UserDefaults check), overrideQuorumURL, and overrideAddresses resolution logic that overlaps with network-selection decisions made elsewhere in AppState.makeSDK(for:). Per this repo's Swift-SDK architectural rule (packages/swift-sdk/CLAUDE.md), decisions like "which addresses/quorum URL for which network" should be resolved by a single Rust-side call, not duplicated Swift logic in two places that can drift out of sync.
suggestion: Quorum lookup takes a blocking read lock instead of fail-fast try_read, risking long stalls on contention
packages/rs-platform-wallet/src/spv/runtime.rs:155-181
get_quorum_public_key acquires self.client.read().await (line 162) — a blocking wait for the write-lock holder to release — rather than try_read() with a fail-fast error. Since this function is invoked synchronously from ContextProvider::get_quorum_public_key via block_in_place + handle.block_on(...) (spv_context_provider.rs:71-84), a write-lock holder anywhere else in the SPV client stalls proof verification indefinitely rather than surfacing a clear, retryable error. The public thread claiming try_read() replaced this is incorrect — the blocking .read().await is still present verbatim.
suggestion: block_in_place + Handle::try_current only confirms a runtime is active, not that it's multi-threaded — a current_thread caller panics
packages/rs-platform-wallet/src/spv_context_provider.rs:71-84
get_quorum_public_key calls Handle::try_current() to confirm it's inside a Tokio runtime, then unconditionally wraps the blocking call in tokio::task::block_in_place (lines 71-84). block_in_place panics if the ambient runtime was built with #[tokio::main(flavor = "current_thread")] or new_current_thread() — try_current() cannot distinguish runtime flavor, so any caller running on a current-thread executor (a plausible embedding scenario given this is a general-purpose synchronous ContextProvider trait implementation) will panic here instead of erroring cleanly. This is documented as an intentional call-time resolution strategy in the module doc comment (lines 13-20), but the flavor-mismatch panic risk itself is not called out or guarded against.
New Findings In Latest Delta
None.
Additional Cumulative Findings
None.
🤖 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.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:319-320: WalletManagerStore bootstraps SDK against a placeholder, unconfigured wallet manager before the real per-network manager exists
`WalletManagerStore.activeManager` is initialized to a bare, unconfigured `PlatformWalletManager()` placeholder (`WalletManagerStore.swift:81`, doc comment at 48-53 confirms this is intentional-but-temporary). `bootstrap()` calls `platformState.initializeSDK(modelContext:, walletManager: walletManager)` at line 319-320 using this placeholder — then only at line 332, after a `Task.sleep(for: .milliseconds(500))`, does it call `walletManagerStore.activate(network:sdk:)` to materialize the real, configured manager. The SDK built during `initializeSDK` is never rebuilt against the real manager, so the app boots with an SDK bound to a manager that has no wallet state, no SPV runtime, and no persistence handler wired up.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:160-177: switchNetwork() always rebuilds a trusted-only SDK, permanently bypassing SPV even when a wallet manager is available; the SPV/trusted toggle is inert
`switchNetwork(to:)` (lines 160-194) calls `try SDK(network: network)` unconditionally — the trusted-only initializer — and never calls `makeSDK(for:)`, the SPV-preferring helper that actually reads `self.walletManager`. Every network switch after initial boot silently drops to the trusted HTTP quorum provider regardless of SPV availability. Compounding this, `useTrustedQuorumFallback`'s `didSet` (lines 45-51) only writes to `UserDefaults` — it never triggers a rebuild via `switchNetwork` or any other path, so flipping the toggle in Settings has zero runtime effect until the next full app relaunch. Public threads claiming a `didSet { Task { await switchNetwork(...) } }` fix or a unified `handleNetworkSwitch()` do not match the code at these lines.
- [BLOCKING] packages/rs-sdk-ffi/src/sdk.rs:216-218: dash_sdk_create_extended silently falls back to an offline mock SDK when DAPI addresses are null, instead of resolving canonical network seeds like its trusted-path sibling
In `dash_sdk_create_extended` (SPV path), `base_config.dapi_addresses.is_null()` routes to `SdkBuilder::new_mock().with_network(network)` — a fully offline mock SDK with no real network connectivity. The sibling `dash_sdk_create_trusted` (lines 307-484) handles the identical null-address case correctly at lines 415-430 by resolving `SdkBuilder::new_testnet()`/`new_mainnet()` for the well-known networks and only erroring for genuinely unsupported ones. Swift's `SDK.swift:381-438` passes `overrideAddresses = nil` for plain mainnet/testnet SPV construction (only regtest/devnet/docker-override paths set explicit addresses), so any plain mainnet/testnet SPV SDK silently becomes a non-functional mock instead of failing loudly or resolving real seeds.
- [BLOCKING] packages/rs-sdk-ffi/src/sdk.rs:282-291: SPV-constructed SDKWrapper.trusted_provider is always None, which hard-fails every document write operation (put/replace/transfer/price/purchase/delete/fetch)
`dash_sdk_create_extended` constructs `SDKWrapper { sdk, runtime, trusted_provider: None }` unconditionally (lines 282-291) — there is no code path that populates `trusted_provider` from the SPV context provider. Every document-mutation FFI function checks `if let Some(ref provider) = wrapper.trusted_provider { ... } else { return Err(FFIError::InternalError("No trusted context provider configured")) }` (verified in `document/put.rs:83-100`, `replace.rs:81`, `transfer.rs:114/285`, `price.rs:87/229`, `purchase.rs:103/275`, `delete.rs:100/285`, `document/queries/fetch.rs:57`). This means an SPV-mode SDK — the entire point of this PR — cannot put, replace, transfer, price, purchase, delete, or fetch-with-contract-resolution any document; it hard-errors on all of them. This is compounded by `SpvContextProvider::get_data_contract` and `get_token_configuration` (`spv_context_provider.rs:98-115`) both unconditionally returning `Ok(None)`, so even a hypothetical alternate resolution path through the `ContextProvider` trait itself would also come up empty.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:155-181: get_quorum_public_key trusts quorum public keys as the proof-verification root without checking verification status
`SpvRuntime::get_quorum_public_key` (lines 155-181) fetches a quorum via `client.get_quorum_at_height(...)` and returns `quorum.quorum_entry.quorum_public_key` unconditionally — there is no gate on `LLMQEntryVerificationStatus` (`Unknown`/`Verified`/`Skipped`/`Invalid`) anywhere in this file or in `spv_context_provider.rs` (confirmed via grep — zero matches for `verified`/`verification_status`/`LLMQEntryVerificationStatus` in `packages/rs-platform-wallet/src/spv/`). Since this public key is the trust anchor the SDK uses to verify platform proofs, an `Unknown` or `Skipped` (unverified) quorum entry is trusted exactly the same as a cryptographically `Verified` one, defeating the security purpose of proof verification. The public thread claiming a fix here refers to code that isn't present.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:469-511: The regression test for the quorum-hash byte-order fix never calls the production function it claims to protect
`quorum_hash_lookup_uses_internal_order_not_reversed` (lines 469-511) builds a standalone `BTreeMap<QuorumHash, [u8;48]>`, inserts a key, and asserts basic `BTreeMap::get` lookup semantics for reversed vs. non-reversed `QuorumHash` values. It never constructs an `SpvRuntime`, never calls `SpvRuntime::get_quorum_public_key` (the actual function that previously had the byte-order bug, lines 155-181), and never exercises any SPV client or masternode-list state. This is a tautological test about `QuorumHash`/`BTreeMap` ordering, not a regression guard on the function whose bug it's named after — a future re-introduction of the reversed-hash bug in `get_quorum_public_key` would not be caught by this test.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:58-116: SpvContextProvider has zero unit test coverage
The full 117-line file has no `#[cfg(test)]` module. None of `get_quorum_public_key`'s sync/async bridging (`block_in_place` + `try_current`), the `get_platform_activation_height` per-network match, or the always-`Ok(None)` `get_data_contract`/`get_token_configuration` stubs have any direct test coverage. Given this struct is the trust root the whole SPV proof-verification path depends on, this is a meaningful gap even though it's not (yet) a functional bug in itself.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:381-438: Network-resolution / override-address logic is duplicated between SDK.swift and AppState.swift instead of living in one place
The SPV-path initializer (`SDK.swift:381-438`) independently re-derives `useOverrideAddresses` (regtest/devnet/`useDockerSetup` UserDefaults check), `overrideQuorumURL`, and `overrideAddresses` resolution logic that overlaps with network-selection decisions made elsewhere in `AppState.makeSDK(for:)`. Per this repo's Swift-SDK architectural rule (`packages/swift-sdk/CLAUDE.md`), decisions like "which addresses/quorum URL for which network" should be resolved by a single Rust-side call, not duplicated Swift logic in two places that can drift out of sync.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:155-181: Quorum lookup takes a blocking read lock instead of fail-fast try_read, risking long stalls on contention
`get_quorum_public_key` acquires `self.client.read().await` (line 162) — a blocking wait for the write-lock holder to release — rather than `try_read()` with a fail-fast error. Since this function is invoked synchronously from `ContextProvider::get_quorum_public_key` via `block_in_place` + `handle.block_on(...)` (`spv_context_provider.rs:71-84`), a write-lock holder anywhere else in the SPV client stalls proof verification indefinitely rather than surfacing a clear, retryable error. The public thread claiming `try_read()` replaced this is incorrect — the blocking `.read().await` is still present verbatim.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:71-84: block_in_place + Handle::try_current only confirms a runtime is active, not that it's multi-threaded — a current_thread caller panics
`get_quorum_public_key` calls `Handle::try_current()` to confirm it's inside a Tokio runtime, then unconditionally wraps the blocking call in `tokio::task::block_in_place` (lines 71-84). `block_in_place` panics if the ambient runtime was built with `#[tokio::main(flavor = "current_thread")]` or `new_current_thread()` — `try_current()` cannot distinguish runtime flavor, so any caller running on a current-thread executor (a plausible embedding scenario given this is a general-purpose synchronous `ContextProvider` trait implementation) will panic here instead of erroring cleanly. This is documented as an intentional call-time resolution strategy in the module doc comment (lines 13-20), but the flavor-mismatch panic risk itself is not called out or guarded against.
…uild + indicator The example app never actually verified proofs against SPV data: the manager is constructed FROM the platform SDK (`manager.configure(sdk)` -> `platform_wallet_manager_create(sdk_ptr)` -> `PlatformWalletManager::new(sdk)` builds the SpvRuntime), while the SPV SDK needed the manager's SpvRuntime — circular. `SDK.init(network:walletManager:)` therefore always got the empty default manager (SPV not started) and silently fell back to trusted, so SPV never engaged. Replace build-from-manager with swap-after-build: - rs-sdk-ffi: `dash_sdk_install_context_provider(sdk, provider)` swaps a live SDK's context provider via `Sdk::set_context_provider`. Takes ownership of the `ContextProviderWrapper` box and reclaims it exactly once on every return path. - platform-wallet-ffi: `platform_wallet_manager_attach_spv_context(manager, sdk, network)` builds `SpvContextProvider` from the manager's `SpvRuntime` and installs it (never reclaims — ownership moves to install). Replaces the dead `create_sdk_with_spv_context`. - swift-sdk: `SDK.attachSpvQuorums(from:)` + `SDK.QuorumSource`; removed the dead `init(network:walletManager:)`. - example app: `AppState` builds trusted, then `attachSpvIfReady` swaps to SPV once the active manager's SPV headers+masternodes are `.synced` (fallback OFF = attach once merely running, no trusted net). App observes `walletManager.spvProgress`. Options shows a Trusted/SPV "Proof Quorum Source" badge that flips automatically when SPV attaches. v1 scope: attaches to the app's query SDK (what user proof-verified fetches use); the manager's own cloned SDK stays trusted (`Sdk::clone` snapshots the provider slot, sdk.rs:204) — v2 follow-up. Verified: rs fmt + clippy clean; SwiftExampleApp (simulator) BUILD SUCCEEDED. Behavioral verification (badge flips to SPV + a fetch verifies via SPV on a synced testnet) is on-device only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The attach-after-build redesign fixes five prior findings, but five cumulative findings remain valid and the latest delta introduces two blocking regressions plus one defensive nit. The canonical result is REQUEST_CHANGES with 4 blockers, 3 suggestions, and 1 nitpick.
Source: reviewers — Sol (gpt-5.6-sol), Sonnet (claude-sonnet-5), and sampled Opus (claude-opus-4-8) for general, security-auditor, and ffi-engineer; final verifier — Sonnet (claude-sonnet-5). The Sol orchestrator (openai/gpt-5.6-sol, high reasoning) was orchestration-only and is not reviewer/verifier evidence.
Prior finding reconciliation
- FIXED: finding-1 (placeholder wallet-manager bootstrap), finding-2 (permanent trusted-only network switch), finding-3 (SPV path falling into the offline mock SDK), finding-4 (missing
trusted_providerbreaking document operations), and finding-8 (duplicated Swift address/network resolution). - STILL VALID: finding-5, finding-6, finding-7, finding-9, and finding-10. None is outdated or intentionally deferred.
Carried-forward prior findings
- BLOCKING — finding-5: Unverified quorum entries remain valid proof-verification trust roots
- BLOCKING — finding-6: The byte-order regression test never calls the production function it claims to guard
- SUGGESTION — finding-7: SpvContextProvider has zero unit test coverage
- SUGGESTION — finding-9: Quorum lookup blocks on a contended read lock instead of failing fast
- SUGGESTION — finding-10: block_in_place + Handle::try_current can panic on a current-thread runtime
New findings in latest delta
- BLOCKING: Full context-provider swap breaks proofs that require contract or token configuration
- BLOCKING: Disabling trusted fallback does not stop trusted proof verification until the user separately starts SPV
- NITPICK: attach_spv_context does not cross-check the caller-supplied network against the SDK's actual network
🔴 4 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:155-181: Unverified quorum entries remain valid proof-verification trust roots
`SpvRuntime::get_quorum_public_key` fetches a quorum via `client.get_quorum_at_height(...)` and returns `quorum.quorum_entry.quorum_public_key` unconditionally (line 180) — there is no gate on the entry's verification status anywhere in this file or in `spv_context_provider.rs` (independently confirmed: zero matches for `verified`/`verification_status`/`LLMQEntryVerificationStatus` in `packages/rs-platform-wallet/src/spv/`). This key is the sole trust anchor the SDK uses to authenticate every Platform proof once SPV is attached. An `Unknown` or `Skipped` (i.e., not cryptographically verified) quorum entry is trusted identically to a `Verified` one, so a malicious or MITM SPV peer that gets an unverified entry into the local masternode-list state — combined with a colluding DAPI node signing with the corresponding key — can have forged Platform state accepted as proof-verified. This is now directly load-bearing: this delta makes the SPV path genuinely reachable in normal app operation for the first time (previously masked by the now-fixed prior-3/prior-4 breakage).
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:469-511: The byte-order regression test never calls the production function it claims to guard
`quorum_hash_lookup_uses_internal_order_not_reversed` builds a standalone `BTreeMap<QuorumHash, [u8;48]>`, inserts one key, and asserts plain `BTreeMap::get` semantics. It never constructs `SpvRuntime`, never calls `SpvRuntime::get_quorum_public_key` (the function that actually had the `.reverse()` bug), and never touches any SPV client or masternode-list state — independently confirmed by reading the full test body. The docstring's claim ('Re-introducing the reversal fails this test') is false for the real function: reintroducing `.reverse()` inside `get_quorum_public_key` would leave this test green, silently removing the only stated regression guard on a security-critical lookup.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:161-162: Quorum lookup blocks on a contended read lock instead of failing fast
`get_quorum_public_key` acquires `self.client.read().await` (line 162) — an unbounded wait for any write-lock holder — rather than `try_read()`. Invoked synchronously via `block_in_place` + `handle.block_on(...)` from `SpvContextProvider::get_quorum_public_key`, so an SPV writer (e.g. during sync or `stop()`) stalls proof verification instead of surfacing a clear, retryable error. The module doc in `spv_context_provider.rs:68-70` explicitly acknowledges this as a deliberate fail-slow tradeoff, so this may be a documented WONTFIX rather than an oversight — carried forward per the no-silent-erase rule since the code itself is unchanged.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/sdk.rs:557-600: Full context-provider swap breaks proofs that require contract or token configuration
`dash_sdk_install_context_provider` calls `sdk_wrapper.sdk.set_context_provider(...)`, whose own doc comment states 'this will overwrite any previous context provider' — confirmed in `rs-sdk/src/sdk.rs:582-590`. The installed `SpvContextProvider::get_data_contract`/`get_token_configuration` (`spv_context_provider.rs:98-115`) unconditionally return `Ok(None)`. Traced to a concrete, reachable break: `FromProof` for `RewardDistributionMoment` (`rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs:84-114`) calls `provider.get_token_configuration(...)?` and, on `None`, returns `Error::RequestError("Token distribution type not found...")` *before* `verify_tenderdash_proof` is ever called (line 106) — the query fails deterministically regardless of proof validity, with an error message that reads like 'no distribution configured' rather than 'provider doesn't support this lookup'. This is reachable end-to-end: FFI `dash_sdk_token_get_perpetual_distribution_last_claim` → Swift `getTokenPerpetualDistributionLastClaim` (`PlatformQueryExtensions.swift:1794`) → exercised from `DiagnosticsView.swift:415` and `PlatformQueriesView.swift:227`. Document-history proof verification (`proof.rs:1408-1410`, same `get_data_contract`-with-no-fallback pattern) would fail identically once wired to an FFI, but no such wrapper currently exists, so that specific path is not yet reachable. DPNS/DashPay contract resolution (`dashpay/mod.rs:56-65`, `dpns_usernames/mod.rs:206-214`) degrade gracefully to a network fetch on `None`, so those are not broken, only slower. The `SDKWrapper.trusted_provider` field retained by document-write FFIs does not help here because these proof-verifier call sites read `sdk.context_provider()`, the slot this function just swapped. Fix: install a composite provider that delegates `get_quorum_public_key`/`get_platform_activation_height` to SPV while forwarding `get_data_contract`/`get_token_configuration` to the retained trusted provider.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:97-170: Disabling trusted fallback does not stop trusted proof verification until the user separately starts SPV
Every SDK build (`initializeSDK`, `switchNetwork`) starts with `quorumSource = .trusted` and immediately fires `refreshProtocolVersion(for: newSDK)`, which drives a real proof-verified `getEpochsInfo` query over the trusted HTTP provider (per its own doc comment, confirmed in code). `attachSpvIfReady` only ever runs reactively off `.onChange(of: walletManager.spvProgress)` (`SwiftExampleAppApp.swift:177-179`) — and SPV sync itself is not auto-started; it requires an explicit user action from `CoreContentView.startSpv` (confirmed: `startSpv` is the only call site of the underlying start function, on the Core tab). So for any user who disables 'Fallback to Trusted Quorums' but has not separately gone to the Core tab and started SPV sync — the default state on every fresh launch — every proof-verified query, not just fallback scenarios, continues to authenticate against the centralized trusted HTTP service indefinitely, with `quorumSource` never flipping and no user-facing signal that the disabled toggle isn't yet in effect. The OptionsView help text ('switch to SPV as soon as it is running... proofs fail until synced') documents behavior *after* SPV is running, not this indefinite pre-start trusted window, so the gap is not clearly disclosed to the user disabling the toggle expecting trustless verification.
In `packages/rs-platform-wallet/src/spv_context_provider.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:71-84: block_in_place + Handle::try_current can panic on a current-thread runtime
`get_quorum_public_key` calls `Handle::try_current()` (confirms a runtime exists) then unconditionally wraps the lookup in `tokio::task::block_in_place` (independently read at lines 71-84). `block_in_place` panics on a `current_thread`-flavor runtime, and `try_current()` cannot distinguish flavor. The module doc now asserts proof verification always runs on the SDK's multi-threaded runtime, which holds for the current shipped caller, but this is a general-purpose `ContextProvider` impl installable via the new `dash_sdk_install_context_provider` on any SDK, and nothing guards the panic for a future/alternate embedder running on a current-thread executor. Panicking across the `extern "C"` boundary is undefined behavior.
…usted quorum picker Manager SDK on SPV: make `dash_sdk::Sdk` share its context-provider slot across clones — `context_provider: Arc<ArcSwapOption<..>>` and `Clone` shares the `Arc` (was a per-clone snapshot). A `set_context_provider` on the app's SDK now also flips the wallet manager's cloned SDK, so both the app's proof-verified queries and the manager's background wallet/DashPay sync verify against the same SPV-synced quorums. Adversarially reviewed: nothing relied on clone-provider isolation (every other Sdk field was already Arc-shared); the arc-swap read path is lock-free with no torn read / use-after-free. Live quorum-source control: replace the fallback toggle with an Auto/SPV/Trusted picker, applied live via provider swaps on the shared slot (no SDK rebuild): - rs-sdk-ffi: `dash_sdk_restore_trusted_context_provider` re-installs the trusted provider captured on the SDK wrapper — symmetric with the existing `dash_sdk_install_context_provider`. - swift-sdk: `SDK.restoreTrustedQuorums()`. - example app: `AppState.quorumMode` + `applyQuorumMode` (Auto = SPV once fully synced, trusted until then; SPV = force SPV once running; Trusted = force trusted). `OptionsView` segmented picker; the "Proof Quorum Source" badge shows what is actually installed. Verified: `cargo check --workspace` + clippy + fmt clean; SwiftExampleApp (simulator) clean BUILD SUCCEEDED; on-device — Auto auto-attaches to SPV once synced, and the picker flips Trusted<->SPV live (badge + attach/restore FFIs) on a synced testnet node. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…restore FFIs - rs-sdk: pin the shared context-provider slot — a `set_context_provider` on an `Sdk` clone is observed by the original (the v2 behavior the wallet-manager SPV switch relies on; previously untested). - rs-sdk-ffi: cover `dash_sdk_install_context_provider` (swap is observable + both null-pointer guards) and `dash_sdk_restore_trusted_context_provider` (null guard + errors when the SDK has no stored trusted provider). Raises coverage on the new FFI context-provider paths added for the SPV proof-verification switch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 9a2289a, five blocking defects remain in the SPV proof-verification path, alongside four suggestions and one nitpick. All eight prior findings remain valid; the latest delta adds a Swift provider-state race but resolves none of them. One additional valid test-coverage suggestion was omitted from the canonical findings because of the 10-comment budget.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 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/spv_context_provider.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv_context_provider.rs:59-83: Proofs can substitute a lower-threshold non-Platform quorum
Cumulative PR finding; not introduced by the latest delta. The proof's untrusted `quorum_type` is forwarded without checking it against the network's Platform quorum type. `SpvRuntime` converts any byte-sized value to `LLMQType`, and the SPV engine stores multiple quorum families. Because the verifier also incorporates the attacker-selected type into the signature digest, a malicious DAPI endpoint and a compromised threshold of a valid lower-threshold quorum can authenticate forged Platform state. For example, mainnet Platform uses LLMQ 100/67 while LLMQ 50/60 requires only 30 signatures. Reject every type other than `self.network.platform_type()` before performing the lookup.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:58-115: [Carried forward: prior-5] SpvContextProvider has no direct unit coverage
No test constructs `SpvContextProvider` or exercises its synchronous-to-async bridge, runtime error mapping, network activation heights, quorum lookup behavior, or contract/token methods. The new tests use generic sentinel providers and therefore do not cover the production object serving as the SPV proof trust root.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:71-83: [Carried forward: prior-7] Current-thread Tokio callers can panic during quorum lookup
`Handle::try_current()` proves only that a Tokio runtime exists, after which the code unconditionally calls `block_in_place`. Tokio panics when `block_in_place` runs on a current-thread runtime. The shipped SDK FFI uses a multi-thread runtime, but this public provider has no runtime-flavor guard, so alternate embeddings can crash instead of receiving `ContextProviderError`. Check `Handle::runtime_flavor()` or use the repository's existing runtime bridge.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:175-180: [Carried forward: prior-1] Unverified quorum entries remain proof-verification trust roots
`get_quorum_public_key` returns the selected entry's public key without requiring `LLMQEntryVerificationStatus::Verified`. In the pinned rust-dashcore revision, entries default to `Skipped(NotMarkedForVerification)`, and `quorum_entry_for_hash_at_or_before_height` excludes only `Invalid` entries. Consequently, `Unknown` and `Skipped` entries can become the trust anchor for Platform proofs. Require an explicitly `Verified` entry before returning its key.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:469-511: [Carried forward: prior-2] Byte-order regression test bypasses the production lookup
The test constructs a standalone `BTreeMap` and asserts ordinary reversed-versus-unreversed key behavior. It never constructs `SpvRuntime`, populates the production masternode engine, or calls `SpvRuntime::get_quorum_public_key`. Reintroducing `.reverse()` in the production function would therefore leave this stated regression guard green. Exercise the real lookup path with controlled SPV quorum state.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:162: [Carried forward: prior-6] Synchronous proof verification can stall on SPV lock contention
`get_quorum_public_key` awaits the client read lock without a timeout while the synchronous provider blocks a Tokio worker around the lookup. Tokio's fair lock queue means a waiting writer can block this read behind existing long-lived readers, causing an FFI proof query to stall rather than return a retryable error. The implementation documents this fail-slow tradeoff, but it remains an unbounded caller-visible wait.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/sdk.rs:595-599: [Carried forward: prior-3] Full provider replacement removes token and contract resolution
`dash_sdk_install_context_provider` replaces the SDK's complete provider with `SpvContextProvider`, whose contract and token methods return `Ok(None)`. This breaks a concrete proof-backed query: the perpetual-distribution last-claim verifier requires token configuration and returns `Token distribution type not found` before Tenderdash signature verification when it receives `None`. Swift exposes and exercises that query. The clone-sharing change broadens the deficient provider to the wallet manager's SDK clone. Install a composite provider that uses SPV for quorum keys and delegates contract and token lookups to the retained trusted provider.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:183-198: [Carried forward: prior-4] Forced-SPV mode still retains or restores trusted verification
The `.spv` mode computes `wantSpv = running`, so selecting SPV while the client is stopped leaves the trusted HTTP provider installed. If SPV later stops or errors, the same branch actively restores that trusted provider. This contradicts the UI promise to force SPV immediately with no trusted fallback. Installing the SPV provider before startup is safe because its lookup already fails closed with `SPV Client not started`.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:378-424: Concurrent restoration and attachment can desynchronize Swift state from the Rust provider
New in the latest delta. `SDK` is `@unchecked Sendable`, `attachSpvQuorums` is main-actor isolated, but the newly added `restoreTrustedQuorums` and ordinary mutable `quorumSource` property are not. A background restore can interleave its ArcSwap update and Swift assignment with a main-actor attachment, leaving Rust on the trusted provider while Swift reports `.spv`, or vice versa; the Swift property accesses also form a data race. Isolate `quorumSource` and `restoreTrustedQuorums` to `MainActor`, matching attachment.
…V lookup The SPV masternode engine keys its quorum map in internal byte order, but the SDK proof verifier supplies quorum_hash in display (reversed) order — the same order the trusted HTTP provider matches against. Building the lookup key without reversing missed every real quorum, so every Platform proof verification failed and, in SPV mode, platform/shielded sync failed. Verified on a synced testnet node: the engine holds 696 type-6 (llmq_25_67) Platform quorums, and an in-lookup probe showed direct_hit=false / reversed_hit=true (requested 0000006b…, stored 1a3680f9…2d000000). After reversing: served=36, missed=0, zero platform/shielded sync failures (was served=0, all misses). An earlier commit on this branch removed the base's `.reverse()` on the mistaken assumption that quorum_hash was internal-order end-to-end; that was the regression. This restores it. Test would have caught this in CI: the renamed regression test `quorum_hash_reversed_to_internal_order_before_lookup` asserts a display-order lookup misses and a reversed lookup hits — ✖ before fix, ✔ after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… Platform quorum type Addresses two review blockers on PR #3417: - Byte-order regression test now exercises the production transform. The reversal lives in `quorum_lookup_key`, which `get_quorum_public_key` calls, and the test asserts against that function — so dropping the reversal fails the test. Previously the test asserted standalone `BTreeMap` semantics that could not detect a change in the real lookup. - Reject any `quorum_type` other than `network.platform_type()` before the SPV lookup. The proof's `quorum_type` is attacker-controlled and folded into the signature digest; without this check a malicious DAPI endpoint plus a compromised threshold of a lower-threshold quorum (e.g. LLMQ 50/60, which needs only 30 signers, vs Platform's 100/67) could authenticate forged Platform state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses a review blocker on PR #3417: installing the SPV context provider replaced the SDK's entire ContextProvider, so get_data_contract and get_token_configuration returned None and broke every proof that references a contract or token configuration (e.g. token perpetual-distribution verification, which resolves the token config before checking the Tenderdash signature). dash_sdk_install_context_provider now wraps the SPV provider in a CompositeContextProvider that serves the quorum public key and platform activation height from SPV-synced state while delegating contract and token lookups to the SDK's retained trusted provider. When no trusted provider was retained the SPV provider is installed alone (verifies only proofs that need neither). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n state Addresses a review blocker on PR #3417: the Quorum Source picker's `.spv` case computed `wantSpv` from the SPV client's run state, so selecting SPV while the client was stopped left the trusted HTTP provider installed, and a later SPV stop/error actively restored trusted — contradicting the UI's promise to force SPV with no trusted fallback. `.spv` now installs the SPV provider unconditionally. This is safe because the SPV lookup fails closed ("SPV Client not started") rather than silently falling back, so proofs simply fail until sync completes instead of being verified against trusted quorums. Auto and Trusted modes are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 14, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The current head still has two blocking trust-boundary defects: SPV proof verification accepts quorum entries that were not cryptographically verified, and a persisted strict-SPV policy is not enforced during startup. The reviewer-reported format check succeeded, but compilation was not established.
Prior reconciliation
- STILL VALID: finding-5, finding-fallback-off-still-trusted, finding-7, finding-9, finding-10, finding-attach-network-mismatch.
- FIXED: finding-6, finding-context-provider-swap.
- OUTDATED: none.
- INTENTIONALLY DEFERRED: none.
Carried-forward prior findings
- Two blockers, three suggestions, and one nitpick remain. The strict-SPV startup blocker is compounded by the new preference implementation ignoring the legacy
useTrustedQuorumFallback=falsesetting and defaulting those upgrades to Auto.
Genuinely new delta findings
- The composite-provider and successful trusted-provider restoration paths are not exercised by the new regression tests.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s) omitted (not in diff).
🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:188-193: Unverified quorum entries remain valid proof-verification trust roots
The returned `QualifiedQuorumEntry` exposes its verification status, but this code discards it and returns the public key unconditionally. In the pinned rust-dashcore revision, `quorum_entry_for_hash_at_or_before_height` excludes only `LLMQEntryVerificationStatus::Invalid`; `Unknown` and `Skipped` entries remain eligible. Those states mean the commitment was not cryptographically verified, yet the key becomes the trust root for Platform proofs. Require `quorum.verified` to be `LLMQEntryVerificationStatus::Verified` before returning its key.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:180: Quorum lookup still waits on outer lock contention
The lookup awaits `self.client.read()`, so proof verification waits behind any writer rather than returning a retryable busy error. This wait is driven synchronously through `block_in_place`, tying up the verification caller for the duration of the contention. A fail-fast read would make contention explicit, although this changes the currently documented fail-slow tradeoff.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:346-374: Strict SPV mode is not enforced during startup
Bootstrap activates the wallet manager but never calls `platformState.applyQuorumMode`. Its only call sites are non-initial `.onChange` handlers, so a persisted `.spv` selection launches with the trusted HTTP provider and remains there until SPV progress or the picker changes. `AppState.initializeSDK` also starts the proof-verified protocol-version refresh on the trusted SDK before the manager exists. Upgrades compound this: `AppState.init` no longer reads the legacy `useTrustedQuorumFallback` key, so users who persisted `false` silently default to Auto and regain trusted verification. Apply or migrate the saved policy before any proof-verified startup work, then apply it immediately after manager activation.
In `packages/rs-platform-wallet/src/spv_context_provider.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:71-145: SpvContextProvider remains untested
No test directly constructs `SpvContextProvider` or exercises its network-specific Platform-quorum rejection, synchronous-to-asynchronous runtime bridge, error mapping, or activation-height selection. At minimum, test the wrong-quorum rejection and all four network activation-height cases; these are deterministic and do not require network access.
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:99-110: Current-thread Tokio callers can still panic in block_in_place
`Handle::try_current` proves only that a Tokio runtime exists; it does not prove that the runtime is multi-threaded. `tokio::task::block_in_place` panics on a current-thread runtime. The shipped FFI runtime is multi-threaded, but `SpvContextProvider` is public and does not enforce that invariant, so alternate callers can panic instead of receiving `ContextProviderError`.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/sdk.rs:607-615: Regression tests never exercise the composite-provider path
The prior provider-swap defect is fixed by the `Some(trusted)` branch, but `install_swaps_provider_and_guards_nulls` uses `create_mock_sdk_handle`, whose `trusted_provider` is `None`; it exercises only the SPV-only branch. The restore test likewise covers only failure without a trusted provider. Add tests proving quorum and activation requests reach the SPV provider, contract and token requests reach the auxiliary provider, successful restoration reinstalls the trusted provider, and the changes are visible through SDK clones.
…ad runtime Addresses a review suggestion on PR #3417: `SpvContextProvider::get_quorum_public_key` used `tokio::task::block_in_place`, which panics if the ambient runtime is a current-thread runtime. Proof verification runs on the SDK's multi-threaded runtime today, but a current-thread caller would panic. The lookup now branches on `Handle::runtime_flavor()`: the multi-threaded path keeps `block_in_place` (no extra thread), while a current-thread runtime drives the in-memory lookup on a short-lived current-thread runtime on a dedicated thread. SPV state uses runtime-agnostic `tokio::sync` primitives, so this is safe — it never panics and works on both runtime flavors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 15, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: prior-1 and prior-2 remain blocking; prior-5 is fixed; prior-3, prior-4, prior-6, and prior-7 are intentionally deferred. New findings in the latest delta: the current-thread workaround removes the prior panic but can deadlock when SPV synchronization yields while holding the masternode-engine write lock. The cumulative public Swift API also permits provider-swap races, so the PR still requires changes for the two trust-root failures.
Source: Codex reviewers gpt-5.6-sol (general, security-auditor, FFI-engineer); verifier gpt-5.6-sol. Sonnet was not run because the Codex blocker gate remains closed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed_before_output),gpt-5.6-sol— general (failed_before_session),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (failed_before_output),gpt-5.6-sol— security-auditor (failed_before_session),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (failed_before_output),gpt-5.6-sol— ffi-engineer (failed_before_session),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:188-193: Unverified quorum entries remain valid proof-verification trust roots
`get_quorum_at_height` returns a `QualifiedQuorumEntry`, but this code returns its public key without checking `verified`. In pinned rust-dashcore revision `1860089`, the lookup helper excludes only `Invalid`, while incremental `MnListDiff` processing initially applies entries with quorum verification disabled. `Unknown` and `Skipped` entries can therefore become the trust root for Platform proofs. A malicious SPV peer and colluding DAPI endpoint could use an attacker-selected quorum key to authenticate forged state. Require an explicit `Verified` status before returning the key.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:334-374: Strict SPV mode is not enforced during startup
Startup creates an SDK using the trusted HTTP provider and immediately launches the proof-verified protocol-version refresh. Bootstrap later activates the wallet manager but never calls `applyQuorumMode`. Its only call sites are non-initial `onChange` handlers, so loading a persisted `.spv` value does not install the SPV provider; the app can remain trusted until SPV progress or the picker changes. Apply the saved mode immediately after activation and do not perform proof-backed startup queries through the trusted provider when strict SPV mode is selected.
In `packages/rs-platform-wallet/src/spv_context_provider.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv_context_provider.rs:105-131: Current-thread runtime bridge can deadlock SPV proof verification
The new current-thread branch synchronously joins a helper thread whose runtime awaits the masternode-engine read lock. Pinned dash-spv acquires the same engine's write lock and then awaits header-storage operations in `feed_qrinfo_heights_to_engine`. If that writer yields on the original current-thread runtime, proof verification blocks its only executor in `join()` while the helper waits for the read lock; the writer can never resume to release it. The shipped FFI runtimes are multi-threaded, which limits exposure, but the new public current-thread path does not work safely. Return an unsupported-runtime error or make the lookup genuinely synchronous and fail-fast.
… helper-thread bridge Addresses a review follow-up on PR #3417: the previous current-thread branch bridged the SPV lookup via a helper thread, which can deadlock — proof verification would block the current-thread runtime's only executor in `join()` while the SPV masternode-engine write lock is held by a task on the same runtime, so that writer could never resume to release it. Since the SDK verifies proofs on a multi-threaded runtime, `get_quorum_public_key` now returns an unsupported-runtime error on a current-thread runtime, avoiding both the `block_in_place` panic and the helper-thread deadlock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 15, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings
Two blocking findings remain at exact head 6fc782f: unverified SPV quorum entries can become proof trust roots, and persisted strict-SPV mode is not applied during startup; prior-3 is fixed and prior-4 remains intentionally deferred.
New findings in the latest delta
No new defect survives verification. The latest delta fixes the current-thread deadlock, and the quorum-hash reversal claim is refuted by the end-to-end byte-representation trace.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
2 additional finding(s) omitted (not in diff).
🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:188-193: Unverified quorum entries remain valid proof-verification trust roots
`get_quorum_at_height` returns a `QualifiedQuorumEntry`, but this code exposes its public key without checking `verified`. In pinned rust-dashcore revision `1860089`, dash-spv's incremental handler calls `engine.apply_diff(..., false, ...)`, newly inserted quorums receive `Skipped(NotMarkedForVerification)`, and the lookup helper excludes only `Invalid` entries. That incremental path also does not validate the supplied quorum-list Merkle root before insertion. A malicious SPV peer can therefore supply an attacker-controlled Platform quorum entry, and a colluding DAPI endpoint can sign forged state with the corresponding key. Reject every status except `Verified` and ensure synchronization cryptographically verifies quorums before they become usable.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:334-374: Strict SPV mode is not enforced during startup
`AppState` loads the persisted quorum mode, but `initializeSDK` always creates a trusted-provider SDK and immediately starts a proven protocol-version refresh. Bootstrap then activates the wallet manager without calling `applyQuorumMode`; that method is reached only by later, non-initial change handlers. A relaunch with `.spv` selected therefore performs proof verification through the centralized provider and can remain there until SPV progress or the picker changes, violating the mode's explicit no-fallback policy. Apply the saved mode immediately after manager activation and do not launch proof-backed startup queries through the trusted provider when strict SPV mode is selected.
thepastaclaw
suggested changes
Jul 15, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward priors: both remain blocking on the current head. Quorum lookup still accepts entries not marked Verified, and persisted strict SPV mode still launches an initial proof-backed refresh through the default trusted provider, although the older indefinite-fallback behavior is fixed. New latest-delta blockers: the one-shot SPV source breaks wallet-manager recreation and sharing, while fixed provider ownership removes the released public Sdk::set_context_provider API despite the PR claiming no breaking changes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:226-231: Unverified quorum entries remain valid proof-verification trust roots
get_quorum_at_height returns a QualifiedQuorumEntry, but this code returns its public key without requiring a Verified status. In pinned rust-dashcore 1860089, missing validation infrastructure is classified as Skipped, verify_non_rotating_masternode_list_quorums still returns Ok after recording that status, and the lookup excludes only Invalid entries. The new readiness flag only checks that header and masternode synchronization report Synced, so it does not guarantee that the selected quorum was cryptographically verified. An unverified peer-supplied quorum key can therefore become the trust root used to authenticate Platform proofs.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:136-144: Strict SPV mode is still bypassed by the startup refresh
A new SDK's adaptive provider starts in Auto mode and routes quorum lookups through the trusted provider until SPV is ready. These lines schedule refreshProtocolVersion, which performs a proven query, before bootstrap creates the wallet manager and applies the persisted quorum mode after its 500 ms delay. A persisted strict .spv selection can therefore perform an initial proof-backed request through the trusted HTTP provider. Network switching repeats the ordering at lines 212-219. Apply the saved mode immediately after assigning the SDK and before scheduling any proof-backed query; strict mode already fails closed while its SPV source is absent or unready.
In `packages/rs-sdk-ffi/src/context_provider.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/context_provider.rs:104-118: Exactly-once SPV source prevents wallet-manager restart
This permanent compare-and-swap rejects every second SPV source registered with an SDK. Every PlatformWalletManager(sdk:) construction now calls platform_wallet_manager_create_with_sdk, which treats that rejection as construction failure. Checked-in integration flows restart or reset a manager against the retained SDK, and SpvStartErrorIntegrationTests intentionally creates two managers to test data-directory locking; all now fail first with “SPV context source is already populated.” Destroying the original manager cannot recover because the SDK-owned adaptive provider retains its source and exposes no clear or replacement lifecycle.
In `packages/rs-sdk/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk/src/sdk.rs:157-158: Fixed provider ownership removes a public dash-sdk API
Changing the provider to permanently fixed ownership also deletes the public Sdk::set_context_provider method present in both the merge base and the v4.0.0 release. The delta had to rewrite packages/rs-sdk/examples/read_contract.rs and packages/dash-platform-balance-checker/src/main_trusted.rs, directly demonstrating the source incompatibility; external consumers using the released setter will no longer compile. This contradicts the PR description's “None vs the release base” breaking-change declaration. Preserve a compatible API or explicitly version and document the change as breaking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The iOS app can verify Platform proofs with quorum public keys synced by its SPV runtime instead of relying exclusively on the centralized quorum HTTP service. The SDK must support explicit trusted and strict-SPV provider choices, while the example app needs an adaptive choice that can switch quorum sources without replacing the SDK context provider.
What was done?
Auto,SPV, orTrusted); it does not swap SDK context providers.AppState.applyQuorumModeis now a single mode-setting call.Security and correctness
network.platform_type()before lookup.Verified.SyncProgressfor readiness; no parallel readiness flag is maintained.How has this been tested?
cargo clippy --workspace --all-featurescargo fmt --all -- --checkplatform-wallet-ffilibrary tests./build_ios.sh --target sim --profile devxcodebuildwith warnings treated as errorsAn on-device pass against a synced testnet node is still pending because no physical device is connected to the available development host.
Breaking changes
Sdk::set_context_providermutation API. Context providers are now fixed at SDK construction.dash_sdk_install_context_provideranddash_sdk_restore_trusted_context_provider; consumers select Trusted, SPV, or Adaptive when creating the SDK.Checklist