Repository navigation
feat(swift-sdk): use SPV-synced quorums for Platform proof verification - #3417
Open
QuantumExplorer wants to merge 49 commits into
Open
QuantumExplorer wants to merge 49 commits into
QuantumExplorer wants to merge 49 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:
Walkthrough
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: using SPV-synced quorums for Platform proof verification in the Swift SDK. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
Finishing Touches
-
Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
feat/ios-spv-quorums
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Collaborator
|
⛔ Final review complete — 2 blocking finding(s) (commit 1ab26d6) · triage: critical |
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 @@
## v5.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>
…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>
shumkov
marked this pull request as ready for review
July 11, 2026 11:50
…ter the wallet manager moved onto it `rebuildSdkAndMoveWalletManager` published the target network (and endpoint settings) before the fallible SDK build and manager move, and `AppState.switchNetwork` moved the DataManager scope and cleared `sdk` on failure. A failed build or move left the selected network out of step with the active manager; for example, after a failed switch to devnet the app showed devnet while the testnet manager stayed active, and a Core start could pair the devnet configuration with that manager. Publication is now transactional rather than gated: - The sequence stops SPV, re-checks the network, runs `prepare` (settings the build reads, e.g. the Docker flag), builds the SDK without publishing it, moves the manager onto it, then `publish`es the network and the SDK together and bumps the rebind tick. If the build or the move fails, `rollBack` undoes `prepare` and nothing is published, so the selected network, data scope, published SDK and active manager still match. Outcomes: stopFailed / networkChanged / failed / rebuilt. - `AppState.switchNetwork(to:)` is split into `buildSdk(for:)` (builds, applies the quorum mode, loads stored contracts, publishes nothing) and `publish(_:for:)` (DataManager scope, `currentNetwork`, `sdk`, quorum source, protocol-version refresh, quorum mode). `currentNetwork` changes only with the SDK. - The picker turns Docker off in `prepare` when leaving Local and back on in `rollBack`; the Docker toggle writes and restores its flag the same way; the service resets move to just before a successful publish; a failed devnet rebuild stays pending for the next Options close. - `WalletManagerStore.activate` keeps the stale cached manager until the replacement's `configure` succeeds, as its doc already said, so a failed move leaves the store tracking the manager that stays active. - A failed switch to Devnet (missing or unreachable Quorum URL) used to land on devnet, where the devnet fields are; it now stays on the old network, so the fields show there after such a failure until the next completed switch or until Options closes. - Error messages say SPV stays stopped until restarted. Gating wallet and SPV operations until a matching manager is active was rejected: it needs a guard at every manager consumer, and the mismatched state would still exist. Addresses thepastaclaw's blocking finding on a30f2b6 (SdkRebuildSequence.swift:47-51). Test would have caught this: SdkRebuildSequenceTests run locally with a stand-in XCTest module (no Xcode on the dev host; Swift 6.1.2, -warnings-as-errors). ✖ before the fix, a picker-shaped run of the old sequence (publish in `apply`, then a failed build / failed move): currentNetwork was devnet instead of testnet, and after a failed move the published SDK had changed too. ✔ after the fix: testFailedBuildDuringANetworkSwitchKeepsTheSelectedNetwork and testFailedActivationDuringANetworkSwitchKeepsTheSelectedNetwork pass, with the other four (6/6). Not tested: the WalletManagerStore cache change (configure needs a real SDK; no injection seam) and the devnet-fields UI. CI's Swift job is the only real build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dk_get_status
`dash_sdk_get_status` derived `mode` from `trusted_provider.is_some()`, so
an adaptive SDK, which also holds a trusted provider, always reported
"trusted" and the trusted cache's quorum count, even while its proofs
were verified with SPV quorums. Callers that supplied their own provider
reported "spv".
The status JSON is now `{version, network, mode, quorumSource,
quorumCount}`:
- `mode`: "adaptive", "trusted", "spv", or "custom" for a provider this
crate did not build (caller-supplied or mock);
- `quorumSource`: the source answering quorum lookups now, "trusted" or
"spv"; an adaptive SDK reports its live routing choice; null for custom;
- `quorumCount`: quorums in the trusted provider's cache, whichever source
answers; 0 without one.
A shared `quorum_source` helper backs both the status and
`dash_sdk_get_context_provider_source`, whose C return value is
unchanged. Swift `SDKStatus` gains `quorumSource: String?` (decodes old
and new payloads), and the example app labels the count "Trusted Quorums
Cached". The example app's Masternodes sync row, hidden for "trusted",
now shows for adaptive SDKs, which sync masternode lists for SPV routing.
Addresses thepastaclaw's suggestion on a30f2b6 (status.rs:33-43).
Test would have caught this in CI:
should_report_an_adaptive_sdk_and_the_source_it_routes_to and
should_report_the_fixed_sources_and_a_caller_supplied_provider
✖ before the fix: the adaptive SDK reported mode "trusted"; no status
carried a quorum source
✔ after the fix (cargo test -p rs-sdk-ffi --all-features --lib).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
suggested changes
Oct 22, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 615bcbf, nine prior findings are fixed, but two security blockers remain: the pinned dash-spv dependency does not enforce network-required header difficulty or authenticate the list state underlying its quorum keys. The documented upstream merge gates identify the correct owners, but they do not yet change the verification behavior of this head. This was static verification only; current CI has JS build/tests, Kotlin tests, and the Swift frozen-schema check passing, while Rust workspace tests and Swift SDK build/tests remain pending.
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, cross-cutting change that alters quorum key sourcing, proof-verification trust boundaries, asynchronous SPV synchronization, and wallet manager ownership, including cryptographic key handling and concurrent lease coordination in files such as packages/rs-platform-wallet/src/spv_context_provider.rs and packages/rs-context-provider/src/routing.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:362-368: Require network-difficulty validation before trusting SPV headers
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r4351202371)
This readiness gate promotes synchronized SPV state into the Platform proof trust root, including automatically in Adaptive/Auto mode. Cargo.toml and Cargo.lock still select rust-dashcore aca020263990a86a15cc1d97ae2cbf965d488db5. In that revision, BlockHeadersManager::store_headers invokes the network-unaware BlockHeaderValidator::new(); its validation checks continuity and whether each header satisfies its own advertised target, but not whether that target is permitted by the network. A malicious SPV peer can therefore supply a cheaply mined tail after the last trusted checkpoint, before an independently authenticated anchor excludes it. Coinbase/list consistency checks alone would only authenticate commitments against that artificial tail. The supplied upstream default retains the same validator, and rust-dashcore#1161 remains open. Your expanded trust-root gate is appropriate, but this integration must remain blocked until the actual dependency pin includes the required header-authentication stack, or SPV proof verification remains unavailable. Implement the difficulty checks in dash-spv's header-acceptance owner, not as duplicate Platform-side validation.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:493-497: Authenticate SPV quorum state against Core headers before trusting its keys
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r3962587472)
The Verified status accepted here authenticates quorum signatures against the engine's supplied membership, not that the membership and commitment belong to the authenticated Core chain. In pinned rust-dashcore aca020263990a86a15cc1d97ae2cbf965d488db5, MnListDiff handling calls engine.apply_diff after request/hash matching, while QRInfo handling calls engine.feed_qr_info. The underlying list construction and updates do not authenticate coinbase inclusion against the stored header or compare both reconstructed list roots with authenticated coinbase commitments. Persisted message replay invokes the same engine entry points. Signature verification subsequently derives members from those unauthenticated lists, so a malicious SPV peer and DAPI endpoint can supply attacker-controlled membership and quorum keys that authenticate fabricated Platform results without compromising a genuine quorum. The current Platform-quorum-type guard does not repair this membership-authentication gap. The linked upstream work is the correct repair, but Platform must pin the completed authenticated ingestion and replay implementation before using these keys as a merged proof trust root; the open #1077/#1206 stack is not yet present in this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Keep contract Data buffers borrowed throughout the native preload call — SDK.loadKnownContracts in packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift collects pointers inside individual Data.withUnsafeBytes closures and passes them to dash_sdk_add_known_contracts after those closures return. Rust dereferences them with slice::from_raw_parts, outside their guaranteed borrow lifetimes. The same implementation exists in base 4545b88; this is a concrete pre-existing memory-safety concern, not a regression introduced by the provider integration.
- Follow-up: Track a separate FFI-safety fix that keeps all Data borrows active through the synchronous native call or copies the inputs into explicitly owned temporary buffers.
Contributor
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Conflict in packages/rs-sdk/src/sdk.rs: kept this branch's non-optional `context_provider` field and took v5.1-dev's new `enforce_protocol_gate_on_mock` test field. Removed should_refuse_without_asking_when_answers_cannot_be_verified from protocol_gate_tests.rs. It clears the provider with `context_provider.store(None)`, but on this branch an Sdk always has a provider (the builder refuses to build without one), so the state it tests cannot occur. The gate's no-provider branch is still covered by its decision-table test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
suggested changes
Oct 28, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The SPV routing and SDK rebuild changes are substantially implemented, and the changed Rust, Swift, Kotlin, and JavaScript checks reported by CI are successful. However, the PR still exposes quorum keys before the pinned SPV dependency authenticates network difficulty and Core masternode commitments, and Swift can replace a wallet manager before its asynchronous teardown completes; these remain blocking correctness and security issues.
🔴 4 blocking
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Keep contract Data buffers alive through the preload FFI call
packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:492-496
loadKnownContracts stores pointers returned from each Data.withUnsafeBytes closure in contractDataPointers, then invokes dash_sdk_add_known_contracts after all those closures have returned. Swift only guarantees those pointers for the duration of the corresponding closure; retaining the Data values in contracts does not extend the validity of escaped buffer pointers because Foundation may use temporary or relocated storage. The Rust FFI function immediately dereferences each pointer while deserializing the contracts at rs-sdk-ffi/src/sdk.rs:959-965. This can cause invalid reads or preload failures, preventing strict-SPV contract-dependent proof verification from working reliably. Keep every withUnsafeBytes scope open until the FFI call returns.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate diff that changes quorum public-key sourcing and proof-verification trust boundaries, including cryptographic key handling, SPV verification readiness, concurrent source leasing, and FFI ownership/lifecycle paths in files such as packages/rs-platform-wallet/src/spv_context_provider.rs and packages/rs-sdk-ffi/src/sdk.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:492-496: Keep contract Data buffers alive through the preload FFI call
`loadKnownContracts` stores pointers returned from each `Data.withUnsafeBytes` closure in `contractDataPointers`, then invokes `dash_sdk_add_known_contracts` after all those closures have returned. Swift only guarantees those pointers for the duration of the corresponding closure; retaining the `Data` values in `contracts` does not extend the validity of escaped buffer pointers because Foundation may use temporary or relocated storage. The Rust FFI function immediately dereferences each pointer while deserializing the contracts at `rs-sdk-ffi/src/sdk.rs:959-965`. This can cause invalid reads or preload failures, preventing strict-SPV contract-dependent proof verification from working reliably. Keep every `withUnsafeBytes` scope open until the FFI call returns.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift:225-231: Tear down stale managers before replacing their SDK
When `activate` detects that a cached manager was configured against a different SDK, it creates and stores the replacement without awaiting shutdown of the stale manager. `PlatformWalletManager.deinit` only schedules native teardown asynchronously, while the old manager can still be retained by SwiftUI views, wallet-scoped services, or in-flight operations. The replacement is then configured and published against the same persistence scope before the old manager is quiescent, allowing concurrent callbacks, reads, writes, or SPV-related teardown against shared state. Await the old manager's `shutdown()` before configuring or publishing its replacement, while retaining the old manager if replacement setup fails.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:358-369: Require network-difficulty validation before trusting SPV headers
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r4351202371)
The readiness gate treats dash-spv's `SyncState::Synced` headers as sufficient before the manager publishes SPV quorum lookups. The pinned rust-dashcore revision `aca020263990a86a15cc1d97ae2cbf965d488db5` validates continuity and whether each header satisfies the target encoded in that header, but its `BlockHeaderValidator` does not validate the network-required Dark Gravity Wave difficulty. The current rust-dashcore default branch contains a network-aware validator that derives required difficulty from network parameters and header history, but that implementation is not present in the pinned revision. A peer can therefore extend a trusted checkpoint with a cheaply generated, internally consistent header tail and make the SPV source appear ready. Keep the SPV quorum source unavailable until the dependency pin includes the network-difficulty and related chain-selection fixes.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:548-556: Authenticate SPV quorum state against Core headers before trusting its keys
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r3962587472)
`get_quorum_public_key_with_status` returns a key once dash-spv reports the quorum as `Verified`, but the pinned dependency's masternode synchronization applies `MnListDiff` and QRInfo data without authenticating the corresponding coinbase Merkle proof or comparing reconstructed masternode-list and quorum roots with Core's coinbase commitments. Its `Verified` status therefore establishes consistency between quorum signatures and the locally constructed masternode entries, not that those entries came from the authenticated Core chain. The runtime then uses that key as the Platform proof trust root. An attacker controlling the relevant SPV responses and Platform proof endpoint could provide a fabricated list and attacker-controlled quorum key that passes this gate. The dependency pin must remain gated on the upstream coinbase, commitment, and ChainLock anchoring work before SPV keys are exposed.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Validate the SPV network against the SDK network — The SPV attachment ABI accepts an independent network argument even though the SDK already has a configured network. The shipped Swift caller passes matching values, and this API predates the reviewed change, so it does not block this PR; however, another FFI caller could install activation-height and quorum-family rules for the wrong network.
- Follow-up: Create a separate rs-platform-wallet-ffi hardening change that derives the SPV network from the SDK or rejects mismatched values.
`SDK.loadKnownContracts` returned each contract's pointer out of its `Data.withUnsafeBytes` closure, then passed it to `dash_sdk_add_known_contracts`, which dereferences it immediately (`slice::from_raw_parts`, then deserialize). A `withUnsafeBytes` pointer is valid only inside its closure. Real contracts are larger than inline `Data` (14 bytes), so their pointers landed in heap storage that the `contracts` argument kept alive, and the code worked by accident. For inline `Data` the pointer is a dead stack copy. Strict-SPV proof verification depends on this preload. `SDK.withUnsafeBytePointers(to:_:)` copies the buffers into one allocation that it owns for the duration of the body and frees afterwards. Every pointer is non-nil, including an empty buffer's. Code review rejected nesting each buffer's `withUnsafeBytes` instead: stack depth grows with the contract count (it overflowed at about 1,000 buffers on a 512 KiB stack), and empty `Data` still yields a nil pointer. Ids, lengths, behaviour and error mapping are unchanged. Tests (SwiftDashSDKTests): - testShouldKeepEveryBufferReadableUntilTheBodyReturns: inline and heap buffers read back correctly through the helper's pointers. - testShouldReportAContractRustCannotDeserialize: undecodable bytes on a regtest strict-SPV SDK surface as `SDKError.internalError` carrying Rust's reason. Red -> green: the helper is new, so the first test's body ran in a standalone swiftc harness against the same signature, once with the old body (pointers returned out of `withUnsafeBytes`) and once with the new one. - Old body: failed in 6 of 6 runs at -Onone and at -O. Every inline buffer read the same reused stack slot, e.g. [[176,141,87],[176,141,87,4],[176]]. - New body: passed in 6 of 6, plus empty, sliced, 4 KiB and 100,000-buffer inputs. No end-to-end test can go red, because no valid contract fits in inline `Data`. Verified locally (no Xcode or XCTest on this machine): - the harness above; - the verbatim `loadKnownContracts` body and helper type-check under Swift 6 with -warnings-as-errors against the generated DashSDKFFI headers of a release-ios aarch64-apple-darwin build; - the test file type-checks against that module plus an XCTest signature stub; - `cargo clippy -D warnings` is clean for rs-sdk-ffi and rs-unified-sdk-ffi on aarch64-apple-darwin. `swift test` runs only in CI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s replacement loads A network's cached manager can be configured against an SDK that the app has since rebuilt. In that case `WalletManagerStore.activate` configured a replacement, restored its wallets and cached it, but never shut the old manager down. `PlatformWalletManager.deinit` only schedules native teardown for whenever the last reference goes, and views and background callers can hold one. Until then the old manager's platform-address, shielded, DashPay and DPNS workers (and SPV, if it was still running) kept writing the network's persisted wallets while the replacement loaded and wrote the same scope. `activate` now: 1. Configures the replacement. If that throws, it returns with the cache, the active manager and the old manager untouched, so the transactional rebuild still keeps the old manager and the old SDK. Creating a native manager reads and writes no persisted state and starts no sync: it clones the SDK, builds an in-memory WalletManager and claims the new SDK's SPV source slot. 2. Bumps the SPV start generation if the old manager is the active one, because shutting it down stops the active manager's SPV. 3. Awaits the old manager's `shutdown()`. 4. Restores wallets synchronously, as before, through a private `restoreWallets`. From the async body, `loadFromPersistor()` would resolve to its off-main overload. Then it caches and swaps in the replacement. A replaced active manager is always swapped out, even with `makeActive == false`, so a shut-down manager cannot stay active. `activate` and `backgroundManager(for:)` are now async; all their callers already ran in async contexts. `rebuildSdkAndMoveWalletManager` takes an async `activateManager`. The swap still comes after activate's last suspension, so the swap and the SDK publish happen in one main-actor turn. During the shutdown the old manager stays cached and active, but its handle is already revoked, so calls on it fail with `invalidHandle`. Tests (SwiftExampleAppTests, using real regtest SDKs and native managers, no network): - testShouldFinishShuttingDownTheReplacedManagerBeforeItsReplacementTakesOver wraps the old manager's native destroy, observes `$activeManager`, and asserts that destroy comes before the swap. Expected red -> green from reading the code: the old code never shuts the manager down, so the log is ["swap"] and the manager stays configured; the fix makes it pass. It does not pin shutdown-before-restore: the sync loader has no seam. - testShouldKeepTheCurrentManagerWhenItsReplacementFailsToConfigure: a rebuilt SDK whose SPV slot another manager holds makes configure throw. The current manager stays cached, active and configured, and a retry replaces it once the slot is free. This also passes on the old code; it guards against shutting down before configuring. Neither test has run: the app's tests need Xcode and run only in CI. Verified locally (no Xcode or XCTest on this machine): - WalletManagerStore.swift and SdkRebuildSequence.swift, verbatim, plus the new test, type-check under Swift 6 with -warnings-as-errors. They were checked against a stub SwiftDashSDK module with the real signatures, including both `loadFromPersistor` overloads, and the generated DashSDKFFI headers. The stub catches the overload error when the restore is inlined back into `activate`. - Harnesses confirm that `rebuildSdkAndMoveWalletManager` accepts both sync and async closures. - A harness confirms that returning from an async @mainactor callee lets no other main-actor job run before the caller continues: 0 of 200 runs, versus 197 of 200 with a deliberate yield. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
suggested changes
Oct 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The Rust and Swift implementation checks pass, but the SPV provider still exposes quorum keys before the pinned dependency authenticates network difficulty and Core coinbase commitments. The manager replacement path also ignores non-clean shutdown results, and SPV lease arbitration is implemented in the FFI layer despite the repository's rule that mobile orchestration belongs in the wallet crate. The PR Hygiene policy check is failing, but the code-review blockers are independent of that policy result.
🔴 4 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large cross-language change that alters quorum public-key sourcing and Platform proof-verification trust boundaries, directly affecting cryptographic key handling and security-sensitive verification paths. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift:243-264: Do not replace a manager after a non-clean shutdown
`await stale.shutdown()` returns metrics but the replacement path ignores whether native shutdown was clean. The Rust manager reports a non-clean shutdown when a coordinator, event adapter, SPV teardown, or other worker exceeds its bounded join budget; `platform_wallet_manager_destroy` still returns success and leaves straggling workers alive while retaining their persistence callbacks. `activate()` then restores the replacement and publishes it for the same network, allowing the old worker and the new manager to access the same persisted wallet state concurrently. The shutdown contract must surface `all_clean()` (or otherwise serialize persistence until every worker has terminated), and the replacement must refuse to proceed when the old manager has live workers.
In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/manager.rs:216-243: Keep SPV lease ownership in the wallet crate
The FFI constructor directly calls `SpvSourceSlot::claim()` and carries the resulting lifecycle lease into `PlatformWalletManager::serve_spv_quorums()`. This is orchestration logic enforcing the invariant that one wallet manager owns an SDK's SPV source, not FFI marshalling. The repository's coding conventions require mobile orchestration to live in `rs-platform-wallet`; otherwise native wallet callers must reproduce the claim/install protocol manually and can construct a manager without the same exclusivity and teardown invariant. Move claim and lease installation behind an owner-level wallet-manager constructor or attach operation, leaving this FFI function responsible only for converting the SDK handle and callback tables.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:353-369: Require network-difficulty validation before trusting SPV headers
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r4351202371)
`is_ready()` treats dash-spv as ready once header and masternode synchronization report `SyncState::Synced`. The pinned rust-dashcore revision `aca020263990` validates header continuity and whether each header satisfies the target encoded in that header, but does not validate `bits` against the network's required Dark Gravity Wave difficulty. The current upstream implementation adds this network-aware check, but it is not present in the pinned revision. A peer can therefore extend a trusted checkpoint with a cheap artificial header tail and then provide matching attacker-controlled masternode and quorum state; this code will publish those quorum keys as a Platform proof trust root. Keep SPV quorum serving unavailable until the dependency pin includes the network-difficulty validation tracked by rust-dashcore#1161.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:471-510: Authenticate SPV quorum state against Core headers before trusting its keys
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r3962587472)
`get_quorum_public_key()` accepts a quorum whenever dash-spv reports `LLMQEntryVerificationStatus::Verified`; that status only verifies the quorum against the masternode data currently held by the engine. In the pinned rust-dashcore revision, MnListDiff and QRInfo processing does not authenticate the coinbase Merkle proof against the stored header or compare the reconstructed masternode-list and quorum roots with the CbTx commitments. The current upstream tree contains these validation calls, but the Platform pin does not. A peer able to influence the SPV list-diff inputs can consequently supply attacker-controlled Platform quorum keys that pass the local `Verified` check and are then used to authenticate forged Platform proofs. Do not expose this source as a proof trust root until the pinned dependency includes the authenticated coinbase and commitment checks tracked by rust-dashcore#1077 and its dependent fixes.
Comment on lines
243
to
+264
| let manager = PlatformWalletManager() | ||
| try manager.configure(sdk: sdk, modelContainer: modelContainer) | ||
| // Best-effort: a fresh manager comes up with no wallets in | ||
| // memory; restore from the network-scoped persistence | ||
| // handler so reopening the app on this network resurfaces | ||
| // the wallets the user already created here. Failures here | ||
| // are non-fatal — the user can still create / import | ||
| // wallets, the in-memory set just starts empty. | ||
| do { | ||
| _ = try manager.loadFromPersistor() | ||
| } catch { | ||
| SDKLogger.event( | ||
| "manager_restore_failed", | ||
| category: .lifecycle, | ||
| severity: .error, | ||
| fields: ["network": .publicText(network.displayName)], | ||
| error: error | ||
| ) | ||
| // A replaced active manager is shut down below, so its replacement | ||
| // must become active even if the caller did not ask for that. | ||
| let becomesActive = makeActive || managers[network] === activeManager | ||
| if let stale = managers[network] { | ||
| // Nothing below can fail the replacement, so the stale manager | ||
| // goes now, before the fresh one loads this network's wallets. | ||
| // Its sync workers keep writing them until its native teardown | ||
| // ends, and `deinit` only schedules that teardown for whenever | ||
| // the last reference is released. Configuring first leaves it | ||
| // usable if `configure` throws: creating a native manager reads | ||
| // and writes no persisted state and starts no sync. | ||
| if stale === activeManager { | ||
| // Shutting it down stops the active manager's SPV, so a | ||
| // start suspended in peer resolution must bail instead of | ||
| // resuming against the torn-down manager. | ||
| invalidatePendingSpvStarts() | ||
| } | ||
| await stale.shutdown() | ||
| } | ||
| restoreWallets(into: manager, network: network) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Do not replace a manager after a non-clean shutdown
await stale.shutdown() returns metrics but the replacement path ignores whether native shutdown was clean. The Rust manager reports a non-clean shutdown when a coordinator, event adapter, SPV teardown, or other worker exceeds its bounded join budget; platform_wallet_manager_destroy still returns success and leaves straggling workers alive while retaining their persistence callbacks. activate() then restores the replacement and publishes it for the same network, allowing the old worker and the new manager to access the same persisted wallet state concurrently. The shutdown contract must surface all_clean() (or otherwise serialize persistence until every worker has terminated), and the replacement must refuse to proceed when the old manager has live workers.
source: gpt-6.1-sol (phase2-reviewer: general)
Comment on lines
+216
to
243
| // Claimed before the manager exists, so a refusal never takes ownership | ||
| // of the host's callback contexts. | ||
| let spv_quorum_lease = match rs_sdk_ffi::dash_sdk_spv_source_slot(sdk_handle) { | ||
| Ok(Some(slot)) => match slot.claim() { | ||
| Ok(lease) => Some(lease), | ||
| Err(error) => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| error.to_string(), | ||
| ); | ||
| } | ||
| }, | ||
| Ok(None) => None, | ||
| Err(message) => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| message, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| let declaration = persistence_capabilities_declaration(&*persistence_capabilities); | ||
| let persistence_extensions = persistence_extension_callbacks(persistence_extension); | ||
| let dpns_event_callback = event_extension_dpns_callback(event_extension); | ||
| platform_wallet_manager_create_impl( | ||
| sdk_ptr, | ||
| spv_quorum_lease, | ||
| persistence, |
Collaborator
There was a problem hiding this comment.
✅ Withdrawn at
1ab26d6c; see the replies below.
🔴 Blocking: Keep SPV lease ownership in the wallet crate
The FFI constructor directly calls SpvSourceSlot::claim() and carries the resulting lifecycle lease into PlatformWalletManager::serve_spv_quorums(). This is orchestration logic enforcing the invariant that one wallet manager owns an SDK's SPV source, not FFI marshalling. The repository's coding conventions require mobile orchestration to live in rs-platform-wallet; otherwise native wallet callers must reproduce the claim/install protocol manually and can construct a manager without the same exclusivity and teardown invariant. Move claim and lease installation behind an owner-level wallet-manager constructor or attach operation, leaving this FFI function responsible only for converting the SDK handle and callback tables.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
Collaborator
There was a problem hiding this comment.
Withdrawn (re-reviewed at 1ab26d6c): Your current implementation keeps exclusivity in SpvSourceSlot::claim and publication, retention and withdrawal in PlatformWalletManager; the binding only acquires that library resource before transferring callback ownership. I withdraw the placement objection because this adapter sequence does not duplicate a lifecycle policy or introduce wrapper-owned business logic.
2 of 5 tasks
Claudius-Maginificent
added
priority: high
Brings in the rust-dashcore pin c9aeac0d (DGW, the coinbase checks, masternode-sync liveness, the private sync module, the FFI removal), the verified quorum context provider (#5523, #5720) and its SDK opt-in (#5721). Semantic resolutions: - rs-sdk-trusted-context-provider: keeps this branch's `local` module next to v5.1-dev's `proof_fetches` and `proof_transport`. System contracts have one owner, `system_contracts`: `LocalContextResolver::data_contract` calls `system_data_contract`, and the table copied into `local.rs` is gone, so the resolver now also serves the decision-proposals and token-market contracts from protocol 15. - The token-market gate (`is_inactive_system_contract`) holds on every path: in `LocalContextResolver::data_contract` before the known contracts, and in `TrustedHttpContextProvider::get_data_contract` before the resolver and the fallback provider. New tests pin each gate; each was seen red with only its own gate removed. - `platform_activation_height` keeps the dash-context-provider copy; `system_contracts` re-exports it. - rs-sdk: keeps the fixed `context_provider: Arc<dyn ContextProvider>` and the removal of `Sdk::set_context_provider`, and v5.1-dev's `SdkBuilder::build` checks of the provider's network and proof requirement. The two setter tests now test the builder, through both `with_context_provider` and `with_context_provider_arc`; both go red without the build check. The balance checker, `read_contract` example and `quorum_key` test drop the setter calls. - platform-wallet `spv/runtime.rs`: takes v5.1-dev's dash-spv adaptations (`SyncProgress`/`SyncState` from the crate root, infallible `PeerNetworkManager::new`) and keeps this branch's byte-reversed quorum lookup key, `is_ready` (running, headers and masternodes `Synced`), the verified-only lookup, and the rejection of quorum types that name no LLMQ type, now matched as `LlmqtypeUnknown(_)`. - `VerifiedHttpContextProvider`'s docs no longer point at the removed setter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oviders `SdkBuilder::build` refuses a provider for another network, or one that requires proofs while they are off, by asking the provider it is given. `SpvContextProvider` and `AdaptiveContextProvider` answered with the trait defaults, so the SDK could not refuse a mismatched one. - Both report the network they were built for. - `AdaptiveContextProvider` reports its trusted provider's proof requirement: it is the only source known when the SDK is built, and the route Auto falls back to. - Neither forwards `core_height_verified`. The adaptive provider does not record which source supplied a quorum's key, and a verified height vouches only for that source; a test pins that the trusted provider is not told about a response an SPV key verified. Test would have caught this in CI: should_forward_provider_requirements_through_every_wrapper ✖ before the fix, ✔ after. The core_height_verified guard was seen red against an unconditional forward to the trusted provider. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rced Auto answers from the trusted provider whenever the SPV client is not ready, which an attacker who cuts the client off from its SPV peers can cause. Document that on AdaptiveContextProvider and ContextProviderMode::Auto, and point clients that must never use the trusted provider's keys at Spv mode. No behaviour change, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SDK.QuorumMode gains case docs: Auto uses SPV while it is synced and the trusted quorum service whenever it is not, which cutting off the SPV peers can force; .spv is the mode that never uses the trusted service. SwiftExampleApp's picker help said Auto stays on SPV once synced; it now says losing SPV falls back to trusted. Doc and help-text change only, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The example's closing comment still showed Sdk::set_context_provider, which this branch removed; the provider is now given to SdkBuilder::with_context_provider. Comment-only change, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er tip as synced
dash-spv keeps masternode sync `Synced` when later list updates fail. A
restored masternode engine whose every QRInfo attempt times out still
completes its pipeline and turns `Synced` at the stored height. Its own
docs say the list is current only once `current_height()` reaches
`block_header_tip_height()`. `SpvRuntime::is_ready` checked the state
alone, so such a list counted as ready.
A ready source's miss maps to `ContextProviderError::Generic`, which the
SDK charges to the responding node, so every honest node signed by a
quorum mined after the stale list was banned in turn. In Auto, the stale
source also kept lookups off the trusted provider.
Readiness now also needs the newest masternode list at most two blocks
behind the header tip (`MASTERNODE_LIST_TIP_LAG`). A tip diff in flight
leaves readiness alone. A list further behind makes the source not
ready: Spv mode reports `QuorumSourceUnavailable`, which bans nobody, and
Auto answers from the trusted provider until the list catches up.
Test would have caught this in CI:
should_not_be_ready_while_the_masternode_list_trails_the_header_tip
✖ before the fix ("a list behind the header tip must not be ready"), ✔
after.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The book's SdkBuilder and Sdk listings still showed the provider in a Box and an ArcSwapOption, which allowed swapping it after build. It is now an Arc fixed when build() runs, which also refuses a provider for another network or one that requires proofs while they are off. The app-connect comment in the shared system-contract loader no longer mentions a fallback provider: the loader returns None, and only the trusted provider has a fallback. Docs and a comment only, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-11-06T07:46:46.521Z |
Contributor
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
suggested changes
Nov 6, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified head 1ab26d6: 13 prior findings are fixed, and the SPV lease-placement objection is withdrawn. Two blocking manager-replacement defects remain—restoring before old persistence writers are quiescent and publishing an empty manager after restoration fails—plus one upstream readiness-tracking suggestion. This was a static review; current Rust workspace, wasm32, Swift and Kotlin CI checks pass, PR Hygiene remains pending, and the supplied base-drift analysis reports no merge conflicts.
🔴 2 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate change directly changes quorum public-key sourcing for cryptographic proof verification in packages/rs-context-provider/src/routing.rs and packages/rs-platform-wallet/src/spv_context_provider.rs, including trust-source routing and SPV eligibility checks. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift:297-307: Do not publish an empty replacement after wallet restoration fails
This catch converts an unavailable wallet snapshot into a successful empty replacement. The caller has already shut down the stale manager and unconditionally caches and publishes the new manager after this helper returns. Restoration genuinely can fail: the Swift persistence handler returns an error when fetching persisted wallets fails, and Rust's loader propagates that error and rolls back partial wallet registrations. The fresh manager can consequently remain empty while the SDK rebuild reports success.
This is not equivalent to a successful load containing no wallets. Existing wallets disappear from the active manager, and subsequent `activate` calls with the same SDK take the identity fast path without retrying hydration. The newly ordered handover therefore depends on a fallible operation that its comment incorrectly treats as unable to fail.
Carry restoration failure through the replacement lifecycle and expose an unavailable, retryable state rather than publishing an empty manager as successfully restored. Account for the fact that the old manager is already terminal: merely throwing into the current rollback path would leave the app retaining a shut-down manager. Add a failed-read/retry handover test.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift:262-265: Do not replace a manager after a non-clean shutdown
(existing thread: https://github.com/dashpay/platform/pull/3417#discussion_r4416654088)
Awaiting `stale.shutdown()` does not establish that its persistence writers have stopped. Rust's `PlatformWalletManager::shutdown` bounds coordinator drains and worker joins and explicitly reports timeouts with potentially live workers, but `platform_wallet_manager_destroy` removes the handle, logs that report and returns Success. Swift's shutdown metrics record those FFI results, not the worker-level report, so even inspecting the existing metrics would not establish quiescence.
The next statements load and publish another manager against the same network's persisted wallets. An already-admitted coordinator pass can finish after its drain budget and persist through its retained callback context; for example, `persistAddressBalances` directly assigns balance, nonce and `lastSeenHeight`. The replacement can therefore load before the old pass's final commit, or an old write can overwrite state subsequently written by the replacement. Callback ownership prevents dangling pointers, but does not serialize these writers.
Provide a wallet-owned replacement/quiescence contract that prevents hydration until old writers have terminated, or fences every superseded persistence writer, and expose its actionable outcome through FFI. Preserve the early configuration-failure behavior, and add coverage where an admitted worker outlives shutdown's budget; the current activation test only establishes that native destroy returned before the swap.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:65-75: Track the stale-sync readiness repair in dash-spv
The two-block tolerance is a legitimate Platform policy, but the need to supplement `Synced` reflects an upstream health-reporting gap. At both the pinned and current upstream revisions, `MasternodesManager::settle_diff` returns from a given-up tip update without withdrawing `Synced`, and `SyncProgress::is_synced` checks phase states without freshness. This guard protects this provider, while other dash-spv consumers still receive that misleading health result.
Track the freshness/degradation contract in dashpay/rust-dashcore's masternode progress API and link the follow-up from this PR before deferring it. dashpay/rust-dashcore#954 covers related initial QRInfo retry exhaustion, but its stated scope does not cover failed incremental tip updates; extend that issue or link a dedicated one. Platform can retain its allowed-lag policy while consuming authoritative upstream freshness information.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Consume upstream validation-before-fork-recovery fix — The pinned dash-spv header pipeline can emit ForkDetected for an unmatched header before checking its proof of work or required difficulty, allowing an invalid fork announcement to trigger recovery. This is an existing upstream synchronization-availability defect, separate from the repaired quorum-list authentication path.
- Follow-up: Track dashpay/rust-dashcore#1304 and consume its validation-before-routing fix in a separate dependency-update PR.
Comment on lines
+297
to
+307
| private func restoreWallets(into manager: PlatformWalletManager, network: Network) { | ||
| do { | ||
| _ = try manager.loadFromPersistor() | ||
| } catch { | ||
| SDKLogger.event( | ||
| "manager_restore_failed", | ||
| category: .lifecycle, | ||
| severity: .error, | ||
| fields: ["network": .publicText(network.displayName)], | ||
| error: error | ||
| ) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Do not publish an empty replacement after wallet restoration fails
This catch converts an unavailable wallet snapshot into a successful empty replacement. The caller has already shut down the stale manager and unconditionally caches and publishes the new manager after this helper returns. Restoration genuinely can fail: the Swift persistence handler returns an error when fetching persisted wallets fails, and Rust's loader propagates that error and rolls back partial wallet registrations. The fresh manager can consequently remain empty while the SDK rebuild reports success.
This is not equivalent to a successful load containing no wallets. Existing wallets disappear from the active manager, and subsequent activate calls with the same SDK take the identity fast path without retrying hydration. The newly ordered handover therefore depends on a fallible operation that its comment incorrectly treats as unable to fail.
Carry restoration failure through the replacement lifecycle and expose an unavailable, retryable state rather than publishing an empty manager as successfully restored. Account for the fact that the old manager is already terminal: merely throwing into the current rollback path would leave the app retaining a shut-down manager. Add a failed-read/retry handover test.
source: gpt-6.1-sol (phase2-reviewer: general)
Comment on lines
+65
to
+75
| fn is_synced_to_tip(progress: &SyncProgress) -> bool { | ||
| progress | ||
| .headers() | ||
| .is_ok_and(|headers| headers.state() == SyncState::Synced) | ||
| && progress.masternodes().is_ok_and(|masternodes| { | ||
| masternodes.state() == SyncState::Synced | ||
| && masternodes | ||
| .current_height() | ||
| .saturating_add(MASTERNODE_LIST_TIP_LAG) | ||
| >= masternodes.block_header_tip_height() | ||
| }) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Track the stale-sync readiness repair in dash-spv
The two-block tolerance is a legitimate Platform policy, but the need to supplement Synced reflects an upstream health-reporting gap. At both the pinned and current upstream revisions, MasternodesManager::settle_diff returns from a given-up tip update without withdrawing Synced, and SyncProgress::is_synced checks phase states without freshness. This guard protects this provider, while other dash-spv consumers still receive that misleading health result.
Track the freshness/degradation contract in dashpay/rust-dashcore's masternode progress API and link the follow-up from this PR before deferring it. dashpay/rust-dashcore#954 covers related initial QRInfo retry exhaustion, but its stated scope does not cover failed incremental tip updates; extend that issue or link a dedicated one. Platform can retain its allowed-lag policy while consuming authoritative upstream freshness information.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
This branch has not been deployed
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
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 from its own SPV client instead of relying only on the centralized quorum HTTP service. SDK consumers choose the trust model when they build the SDK: Trusted, strict SPV, or Adaptive (switchable Auto/SPV/Trusted, used by SwiftExampleApp).
Trust root. SPV quorum keys are as trustworthy as dash-spv's checks at Platform's rust-dashcore pin. That pin is
c9aeac0d, from v5.1-dev via #5729:Verified.ChainLock anchoring is not part of this trust root. dashpay/rust-dashcore#1309 was closed in favour of DIP 175 quorum proof chains (dashpay/rust-dashcore#1309 (comment)), which are a follow-up (see Follow-ups).
What was done?
Providers (
dash-context-provider::routing):SpvContextProvidertakes quorum keys only from an SPV client and never from the trusted service.AdaptiveContextProviderroutes each lookup by mode:core_height_verified, because it does not record which source supplied a quorum's key.ContractSource, which has no quorum keys.SdkBuilder::build. The adaptive provider also reports its trusted provider's proof requirement, so the builder's provider checks apply to them.SpvSourceSlotis where an SPV client publishes its quorum lookups. A client claims it and holds it through a lease, one lease at a time.QuorumSourceUnavailable, so the SDK does not ban the node whose proof it could not check.Local contract cache (
rs-sdk-trusted-context-provider):TrustedHttpContextProvideris nowLocalContextResolver. The trusted provider delegates to it.system_contractsloader, which includes the decision-proposals and token-market contracts from protocol 15.FFI (
rs-sdk-ffi):dash_sdk_create_trusted,dash_sdk_create_spvanddash_sdk_create_adaptivebuild the matching provider. Strict SPV never builds the trusted HTTP provider and ignoresquorum_url.dash_sdk_add_known_contractsuse the SDK's local contract cache, so they work for every kind. Trusted and Adaptive share the trusted provider's cache with proof verification.dash_sdk_set_context_provider_modeanddash_sdk_get_context_provider_sourcedrive the adaptive provider.Wallet (
rs-platform-wallet,rs-platform-wallet-ffi):platform_wallet_manager_create_with_sdk(with the persistence and event extensions) claims the SDK's slot before it builds the manager. A second manager on the same SDK is refused before it takes the host's callback contexts.The manager serves its SPV quorum keys from creation until shutdown or drop.
The source answers only while all of these hold:
Synced;dash-spv keeps masternode sync
Syncedwhen later list updates fail. Without the tip check, a stale list would count as ready and its misses would get honest nodes banned.Only quorums dash-spv marked
Verifiedare served. Quorum types that name no LLMQ type are refused.Swift:
SDK(network:contextProvider:)defaults to Trusted.SDK.quorumSourcereads the native provider.SDK.QuorumModedocuments each mode.PlatformWalletManager.configure(sdk:)creates the manager throughcreate_with_sdk.Merged with v5.1-dev at
46c26cb05f. This brings in the rust-dashcore pin above, the verified quorum provider (#5523, #5720) and its SDK opt-in (#5721).Sdk::set_context_providerchecks now run inSdkBuilder::build, since the provider is fixed at build. Its tests now test the builder.How Has This Been Tested?
cargo test --all-features --locked --lib --bins --tests -- --skip shield, CI's package list and env):1655562fbf.strategy_testsbinary ran, not only the two tests PR CI picks.platform-wallet,dash-context-provider,rs-sdk-trusted-context-provider(all features): 1,770 + 17 + 150 unit tests passed. The trusted provider's default-feature build: 138 passed.platform-wallet-storageshield-named SQLite tests: 7 passed.wasm-sdk: 217 native tests passed, and 45 wasm32 tests passed underwasm-bindgen-test-runner.cargo clippy --all-targets --all-features -- -D warnings, with no warnings, on:dash-context-provider,rs-sdk-trusted-context-provider,dash-sdk,platform-wallet,rs-sdk-ffi,platform-wallet-ffi,dash-platform-balance-checker;wasm-sdkfor wasm32.cargo check --workspace --all-targets.should_not_be_ready_while_the_masternode_list_trails_the_header_tip;should_forward_provider_requirements_through_every_wrapper;SDK.quorumSourcefor a strict SPV SDK (Swift).swiftc -parse). The Swift SDK job is the build and test gate.Known limits:
Verifiedis reported as unavailable. A node can name one without being banned.Follow-ups:
dash_sdk_add_known_contractsaccepts system-contract IDs.VerifiedHttpContextProviderbefore the trusted service, on networks whose relay serves/proofs. Today that is testnet only: mainnet's relay refuses/proofs, and Core v24 is not released.core_height_verifiedto the source that supplied a key comes with persisting the verified provider'scontinuation().VerifiedHttpContextProviderkeeps its own contract and token caches. They duplicateLocalContextResolverand lack its rule that a preload never replaces a newer cached contract. Delegate to the resolver.server, and belongs indpp.TrustedHttpContextProviderreports nonetwork();AdaptiveContextProvider::newdoes not compare its trusted provider's network with its own;Invalidreads as missing, so its lookups are charged to the node.Breaking Changes
None that affect consensus. SDK API changes:
Sdk::set_context_provider, which v5.1-dev made fallible in feat(sdk)!: let wasm-sdk and evo-sdk opt into the verified quorum context provider #5721. The context provider is fixed at SDK construction, and its network and proof requirement are checked bySdkBuilder::build.Sdk::context_provider()now returnsOption<Arc<dyn ContextProvider>>instead ofOption<impl ContextProvider>.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull request🤖 Generated with Claude Code
PR Hygiene ·
1ab26d6/self-reviewedrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/Cargo.toml,packages/rs-platform-wallet-ffi/src/manager.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/Cargo.toml,packages/rs-platform-wallet/src/lib.rs,packages/rs-platform-wallet/src/manager/mod.rsand 3 more) — HashEngineering or ZocoLini or llbartekll or romchornyirust-sdk-ffi(packages/rs-sdk-ffi/Cargo.toml,packages/rs-sdk-ffi/src/document/create.rs,packages/rs-sdk-ffi/src/document/delete.rsand 11 more) — lklimek or shumkovrust-sdk(packages/rs-sdk/examples/read_contract.rs,packages/rs-sdk/src/sdk.rs,packages/rs-sdk/src/sdk/protocol_gate_tests.rsand 1 more) — lklimek or shumkovswift-sdk(packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift,packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift,packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftand 11 more) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.