Repository navigation
feat(swift-sdk): use SPV-synced quorums for Platform proof verification - #3417
QuantumExplorer wants to merge 39 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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
|
|
⛔ Final review complete — 2 blocking finding(s) (commit 615bcbf) · triage: critical |
|
✅ 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:
|
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>
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>
|
All review feedback addressed. Additionally, bumped rust-dashcore to |
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>
… SPV lease for the manager's life
Strict SPV:
- dash_sdk_create_spv no longer builds the trusted HTTP provider. It
failed for reasons unrelated to SPV: devnet without a quorum URL, or
an unresolvable quorum host on native targets. It reads contracts from
a standalone LocalContextResolver and ignores quorum_url.
- The document FFIs and dash_sdk_add_known_contracts read and fill the
SDK's local contract cache (SDKWrapper.local_contracts) instead of the
trusted provider. A strict SPV SDK has no trusted provider, so it
could not run document operations or preload contracts. Trusted and
Adaptive share the trusted provider's own cache, so preloaded contracts
stay visible to proof verification.
Provider routing:
- rs-sdk-ffi builds the providers from dash-context-provider::routing
and keeps only construction and marshalling.
- PlatformWalletManager serves its SPV quorum keys into the SDK's slot
from creation (platform_wallet_manager_create_with_sdk claims the slot
first, so a refusal never takes the host's callback contexts) until
shutdown or drop, which release it before SPV stops. SPV start and
stop no longer touch the lease, so a manager whose SPV stopped
without releasing it (cleared storage, a failed run, start racing
stop) is no longer refused by its own stale lease on the next start.
PlatformWalletManagerHandle is gone; SPV start, stop and destroy are
upstream's code again.
- The source counts as ready only while sync is running and synced: a
failed sync keeps its last "synced" progress. A lookup that fails
because the client stopped, and the runtime-bridge refusals, are
QuorumSourceUnavailable, so the SDK does not ban a node for this
side's state.
Tests: should_build_a_strict_spv_sdk_without_a_quorum_service fails on
the previous construction ("Devnet name must be provided for devnet
network") and passes now. Cache sharing, end-to-end preloading on a
strict SPV SDK, the manager's lease lifetime and the error classes are
covered. The lease wedge and the failed-sync readiness gap need a
started dash-spv client to reproduce and have no test here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The SPV routing and lease changes are generally coherent, and the previously reported runtime, strict-SPV construction, diagnostic, and same-network rebuild issues are fixed. Two blocking issues remain: the SPV quorum trust root still relies on unauthenticated masternode state from the pinned dash-spv revision, and network selection can activate a wallet manager with the previous SDK because SDK rebuilding is launched asynchronously. Two additional hardening issues concern unchecked quorum-type narrowing and cache downgrades during contract preloading. Rust workspace tests and JS builds were still running at the captured CI snapshot, with no failed code checks reported.
🔴 2 blocking | 🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Coordinate network SDK rebuild before activating the wallet manager
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:19-24
Changing currentNetwork still launches switchNetwork(to:) in an unawaited Task. The SwiftUI .onChange handler then immediately calls activateManager(for:), which uses the current platformState.sdk before the asynchronous rebuild has assigned the new SDK. The rebuild later replaces platformState.sdk but does not activate the manager again. On a network change, the manager can therefore retain a clone of the previous network's SDK, including its adaptive provider and SPV source slot, while the app-level SDK points at the new network. Make network switching a single MainActor-coordinated operation that stops the old manager/SPV state, rebuilds the SDK, activates the manager with that exact SDK, and only then rebinds wallet-scoped services; do not start a competing rebuild from currentNetwork.didSet.
source: gpt-6.1-sol (phase2-reviewer: general)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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 public-key sourcing and Platform proof-verification trust boundaries in functions such asdash_sdk_create_spvandSpvContextProvider, directly affecting cryptographic key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane 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/AppState.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift:19-24: Coordinate network SDK rebuild before activating the wallet manager
Changing `currentNetwork` still launches `switchNetwork(to:)` in an unawaited `Task`. The SwiftUI `.onChange` handler then immediately calls `activateManager(for:)`, which uses the current `platformState.sdk` before the asynchronous rebuild has assigned the new SDK. The rebuild later replaces `platformState.sdk` but does not activate the manager again. On a network change, the manager can therefore retain a clone of the previous network's SDK, including its adaptive provider and SPV source slot, while the app-level SDK points at the new network. Make network switching a single MainActor-coordinated operation that stops the old manager/SPV state, rebuilds the SDK, activates the manager with that exact SDK, and only then rebinds wallet-scoped services; do not start a competing rebuild from `currentNetwork.didSet`.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:448-454: Reject out-of-range quorum types before narrowing to u8
`get_quorum_public_key_with_status` is public and converts its caller-supplied `u32` quorum type with `as u8`, so values above 255 wrap and can select a different `LLMQType`. `SpvQuorumSource` currently rejects non-Platform types before calling this method, but the runtime itself should fail closed rather than relying on every caller to enforce that invariant. A future direct caller could otherwise look up an unintended quorum family.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:413-417: 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 proves the quorum signature against the masternode entries currently held by dash-spv, but it does not prove that those entries and the quorum commitment came from the authenticated Core chain. The pinned rust-dashcore revision `626c91d1efcc471d2ea06290cadd08ef222aee35` applies MnListDiff and QRInfo state after matching requested block hashes and resolving stored heights, without verifying the coinbase Merkle inclusion or comparing the reconstructed masternode-list and quorum roots with the CbTx commitments. The upstream `e4acaa3f` implementation adds those checks for every relevant diff and replay path. Because this PR exposes a dash-spv `Verified` quorum key as the Platform proof trust root, an attacker controlling relevant SPV responses together with a cooperating DAPI endpoint could authenticate forged Platform proofs using attacker-controlled quorum state. Update the dependency pin to include the authenticated coinbase/commitment checks and keep this trust path unavailable until that authenticated state is available.
In `packages/rs-sdk-trusted-context-provider/src/local.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/local.rs:109-118: Prevent contract preloading from downgrading the verifier cache
`LocalContextResolver` already provides `add_known_contract_if_newer`, which preserves the existing body for equal versions and rejects older versions. However, bulk preloading through `add_known_contracts` unconditionally replaces entries. Since the FFI preload path feeds contracts from persisted Swift storage into the same cache used by proof verification, a stale contract can overwrite a newer proof-registered contract and cause subsequent verification or document operations to use obsolete schema data. Route bulk insertion through the version-aware helper, without holding the cache mutex across helper calls.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
…he SPV lookup `SpvRuntime::get_quorum_public_key_with_status` narrowed its u32 `quorum_type` with `as u8`, so 262 wrapped to 6 and returned the held type 6 quorum's key, and values naming no LLMQ type (0, 7-99, 108-255) all searched the quorums dash-spv files under `LlmqtypeUnknown`. It now converts with `u8::try_from`, rejects `LlmqtypeUnknown`, and fails with `PlatformWalletError::InvalidParameter` before taking the client lock. `quorum_lookup_context_error` maps that to `Generic`, so it fails closed. `SpvQuorumSource` already rejects non-Platform types; the runtime no longer relies on every caller doing so. Addresses thepastaclaw's review of 57bd3bf (runtime.rs:448-454). Test would have caught this in CI: should_reject_a_quorum_type_above_one_byte ✖ before the fix: type 262 returned Ok with the type 6 key and status Verified; type 0 returned SpvError "type 0_Unknown ... not found" ✔ after the fix (cargo test -p platform-wallet --all-features --lib). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`LocalContextResolver::add_known_contracts` replaced cached entries unconditionally. Swift and Kotlin preload stored contracts through `dash_sdk_add_known_contracts` into the same cache proof verification reads, so a stale stored copy could replace a newer contract registered after a proven fetch, and later proofs and document operations would use the obsolete schema. Each contract now goes through `add_known_contract_if_newer`, which takes the cache lock per contract: an older or equal version is ignored and the cached body kept, and within one batch the highest version wins. `TrustedHttpContextProvider::add_known_contracts` and `with_known_contracts` delegate here; public signatures are unchanged. No caller of the bulk path relied on overwriting (the FFI preload, the JNI bridge, a test helper; nothing calls `with_known_contracts`). `add_known_contract`, `add_known_data_contract` and `register_data_contract` still overwrite: registration is authoritative on purpose, e.g. the moderation flow registers the exact contract a document was serialized with. Addresses thepastaclaw's review of 57bd3bf (local.rs:109-118). Test would have caught this in CI: should_not_downgrade_a_cached_contract_when_preloading_contracts ✖ before the fix: "an older preload downgraded the cache" (left: 1, right: 2) ✔ after the fix (cargo test -p rs-sdk-trusted-context-provider --all-features; the test module needs a system-contract feature). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s SDK on a network switch Picking a network published `currentNetwork`, whose didSet started an unawaited SDK rebuild, while the App scene's `.onChange(of: currentNetwork)` activated the wallet manager with whatever `platformState.sdk` held at that moment, usually the previous network's. The manager kept that SDK's clone, adaptive provider and SPV source slot, and nothing activated it again after the rebuild. A network switch now runs as one main-actor sequence, `rebuildSdkAndMoveWalletManager`, which the Docker toggle and devnet edits already used through OptionsView's `rebuildSdk`: stop SPV, re-check the network, apply (service resets, publish the network, Docker auto-disable), build the SDK, activate the manager with the SDK the build returned, apply the quorum mode, then bump `walletScopedServicesRebindTick` so the services rebind against the new manager. Each step runs only if the previous one succeeded. - `currentNetwork` is `private(set)`; `publishCurrentNetwork(_:)` only publishes, and its didSet only persists. A `$appState.currentNetwork` binding can no longer bypass the rebuild. - `switchNetwork(to:)` returns the SDK it published. - The App no longer observes `currentNetwork`; `activateManager(for:)` is removed (its `manager_activated` event too; the store still logs `manager_materialized`). - The picker still stops SPV before publishing and keeps its `invalidatePendingSpvStarts()` / `isSwitchingNetwork` guards; `isSwitchingNetwork` now stays set until the manager has moved. SdkRebuildSequenceTests pins the order, that activation receives the rebuilt SDK, and the early exits (failed stop, network changed during the stop, failed build, failed activation), for a network switch and a same-network rebuild. Not shown red: Swift does not build on the dev host (no Xcode), and the old code had no single sequence a unit test could drive; the race spanned a didSet Task and a SwiftUI onChange. The sequence and tests type-check with Swift 6.1.2 under -warnings-as-errors against stand-in modules; CI's Swift job is the only real build. Addresses thepastaclaw's blocking finding on 57bd3bf (AppState.swift:19-24). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`AppState.showError(message:)` set `showError` and `errorMessage`, but no view presented them, so every caller's message was dropped: a failed SPV stop before a network switch or SDK rebuild, a failed SDK build, a wallet manager that could not move to the rebuilt SDK, and identity refresh failures. ContentView now shows them in an alert. No test: SwiftUI alert presentation has no unit-test harness in SwiftExampleAppTests, and Swift does not build on the dev host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in v5.1-dev up to 4545b88, including the rust-dashcore pin aca02026, the grovedb pin 2128ea18, the async SPV start (#5512) and DPNS contract v3 (#5337). Conflicts: - rs-platform-wallet/src/spv/runtime.rs: imports only. v5.1-dev no longer names the storage manager; this branch's readiness check still needs `SyncState`. Semantic fix: `local::tests::should_keep_a_newer_contract_when_an_older_one_is_offered` built its "older" contract from DPNS without a version. With DPNS at version 3 the "older" copy became the newer one; it now sets version 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 3 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The SPV routing and provider-lifecycle changes address seven prior findings, but the new trust root still relies on unauthenticated masternode/quorum state and headers from the pinned rust-dashcore dependency. SDK rebuild failures can also leave the selected network, SDK, and active wallet manager inconsistent; the status API additionally misreports Adaptive routing. Build-related CI checks were still queued at the supplied snapshot.
🔴 3 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Update the existing SDK status API for adaptive providers
packages/rs-sdk-ffi/src/system/status.rs:33-43
dash_sdk_get_status still derives its mode solely from wrapper.trusted_provider.is_some(). Adaptive SDKs retain the trusted provider even when their live routing mode is SPV, so they report "trusted" and the trusted HTTP cache's quorum count while the proof path is using SPV. The SwiftExampleApp exposes this API through diagnostics and other status consumers, which contradicts the new live quorum-source indicator and can mislead users about which trust model is active. Represent Adaptive explicitly or read the native active-source getter, and distinguish the trusted-cache count from the active SPV source.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
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: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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 public-key sourcing and proof-verification trust boundaries in the SPV wallet, SDK, and FFI layers, directly affecting cryptographic key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity 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
The readiness gate treats headers as synchronized when dash-spv reports `SyncState::Synced`, and the subsequent lookup accepts a quorum key when its status is `Verified`. In the pinned dependency, `BlockHeaderValidator::validate` checks only header continuity and whether each hash satisfies the target encoded in that same header; it does not compare `bits` with the network-required Dark Gravity Wave difficulty. The upstream rust-dashcore commit `03e740ed50e4a8ec7a46645d4a2124d292d1cb10` adds the required network-aware validation and explicitly describes the attack in which peers serve a minimally difficult chain after the last checkpoint, but that commit is not in the pinned revision. Without this check, a malicious peer can extend a real checkpoint with a cheap artificial header tail and then provide internally consistent coinbase/list data for attacker-controlled quorums. Gate this integration on the network-difficulty fix as well, or keep SPV proof verification unavailable until authenticated headers are available.
- [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` result accepted here only verifies quorum signatures against masternode entries that dash-spv derived from its in-memory lists. The pinned rust-dashcore revision `aca020263990a86a15cc1d97ae2cbf965d488db5` still applies `MnListDiff` through `MasternodeList::from_diff` and `apply_diff` without authenticating coinbase inclusion against the stored header or comparing the reconstructed masternode and quorum roots with the coinbase commitments. QRInfo and replayed stored messages use the same unauthenticated paths. Because this PR exposes those keys as the Platform proof trust root, a malicious SPV peer plus a collaborating DAPI endpoint can supply fabricated Platform-quorum membership and obtain a `Verified` key without controlling the genuine quorum. Keep the merge blocked until the dependency pin includes the authenticated diff and replay handling from rust-dashcore#1077, or fail closed until it does.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SdkRebuildSequence.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SdkRebuildSequence.swift:47-51: Restore a coherent network state when SDK rebuilding fails
The sequence applies the target network or endpoint settings before the fallible SDK rebuild and manager activation. `AppState.switchNetwork` also changes `dataManager.currentNetwork` before constructing the SDK, then clears `sdk` on failure; the old `WalletManagerStore.activeManager` remains bound to the previous SDK and network because activation was never reached. For example, switching to devnet when masternode discovery or SDK construction fails leaves the app reporting devnet while the active manager still belongs to testnet. The initialized UI can then pass the newly selected network together with that old manager to `CoreSpvLauncher`, allowing SPV operations to use mismatched network configuration and persistence. Make publication transactional with SDK and manager activation, roll back the selected network and data scope on failure, or gate wallet/SPV operations until a manager matching the selected SDK and network is active.
In `packages/rs-sdk-ffi/src/system/status.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/system/status.rs:33-43: Update the existing SDK status API for adaptive providers
`dash_sdk_get_status` still derives its mode solely from `wrapper.trusted_provider.is_some()`. Adaptive SDKs retain the trusted provider even when their live routing mode is SPV, so they report `"trusted"` and the trusted HTTP cache's quorum count while the proof path is using SPV. The SwiftExampleApp exposes this API through diagnostics and other status consumers, which contradicts the new live quorum-source indicator and can mislead users about which trust model is active. Represent Adaptive explicitly or read the native active-source getter, and distinguish the trusted-cache count from the active SPV source.
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 preloaded contract buffers valid throughout the native call — The Swift contract-preload path collects pointers from
Data.withUnsafeBytesand passes them after those closures have ended, allowing the native FFI call to observe invalidated borrowed memory. This implementation predates the provider/cache changes and is not introduced by this PR, so it should be tracked separately rather than blocking this review.- Follow-up: Keep each
withUnsafeBytesscope open through the synchronous FFI call, or copy the contract bytes into explicitly owned buffers whose lifetime covers the call.
- Follow-up: Keep each
| self.sync_progress().await.is_some_and(|progress| { | ||
| progress | ||
| .headers() | ||
| .is_ok_and(|headers| headers.state() == SyncState::Synced) | ||
| && progress | ||
| .masternodes() | ||
| .is_ok_and(|masternodes| masternodes.state() == SyncState::Synced) |
There was a problem hiding this comment.
🔴 Blocking: Require network-difficulty validation before trusting SPV headers
The readiness gate treats headers as synchronized when dash-spv reports SyncState::Synced, and the subsequent lookup accepts a quorum key when its status is Verified. In the pinned dependency, BlockHeaderValidator::validate checks only header continuity and whether each hash satisfies the target encoded in that same header; it does not compare bits with the network-required Dark Gravity Wave difficulty. The upstream rust-dashcore commit 03e740ed50e4a8ec7a46645d4a2124d292d1cb10 adds the required network-aware validation and explicitly describes the attack in which peers serve a minimally difficult chain after the last checkpoint, but that commit is not in the pinned revision. Without this check, a malicious peer can extend a real checkpoint with a cheap artificial header tail and then provide internally consistent coinbase/list data for attacker-controlled quorums. Gate this integration on the network-difficulty fix as well, or keep SPV proof verification unavailable until authenticated headers are available.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
No code change for this one. #3417 is gated on the rust-dashcore trust-root stack:
- coinbase and commitment checks: fix(dash-spv)!: leave the masternode engine untouched by a refused or interrupted QRInfo rust-dashcore#1211, feat(dash-spv)!: let a network request avoid given peers rust-dashcore#1212, fix(dash-spv)!: keep masternode sync live after refused responses rust-dashcore#1205, feat(dash-spv)!: check masternode list diffs against the block coinbase rust-dashcore#1206;
- ChainLock anchoring;
- network-difficulty (Dark Gravity Wave) validation of headers: feat(dash-spv)!: validate header difficulty against Dark Gravity Wave rust-dashcore#1161.
Platform's rust-dashcore pin moves only once those land, and SPV quorum keys don't become a merged trust root before then. Leaving this thread open, like the SPV-authentication thread.
There was a problem hiding this comment.
That merge gate addresses my concern: the fix can live upstream, provided Platform’s pin includes the network-difficulty validation from dashpay/rust-dashcore#1161 before this integration merges. The checkout still pins aca020263990a86a15cc1d97ae2cbf965d488db5, so I agree with leaving this thread open pending that dependency update and verification of the validation path.
The finding stands on this commit until that change is pushed; the push gets a full review.
…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>
|
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
left a comment
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.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
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).
Blocked on dashpay/rust-dashcore#1077. dash-spv does not yet check masternode-list diffs against the block coinbase (the merkle proof against the stored header, and the CbTx masternode-list and quorum roots). Until it does, SPV quorum keys are only as trustworthy as the P2P peers that served the lists. This PR merges only after Platform's rust-dashcore pin includes #1077.
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. Auto uses SPV once it is published and synced, and trusted until then. A missing key is fetched only on the trusted route.ContractSource, which has no quorum keys.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): the known-contracts, token-configuration and system-contract part ofTrustedHttpContextProvideris nowLocalContextResolver. The trusted provider delegates to it unchanged.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.Verified.Swift:
SDK(network:contextProvider:)defaults to Trusted.SDK.quorumSourcereads the native provider.PlatformWalletManager.configure(sdk:)creates the manager throughcreate_with_sdk.The branch is merged with
v5.1-dev. v5.1-dev already returns onlyVerifiedSPV quorum keys (#5425); this branch adapts to that and to the SDK's asynchronous quorum-key fetch.How Has This Been Tested?
cargo test --all-features --lib --bins --tests):dash-context-provider,rs-sdk-trusted-context-provider,dash-sdk,platform-wallet,rs-sdk-ffi,platform-wallet-ffi: 2920 passed, 0 failed.cargo clippy --all-targets --all-features -D warningson those crates, plus no-feature clippy on the two context-provider crates.cargo check --workspace --all-targets, andwasm-sdkfor wasm32.SDK.quorumSourcefor a strict SPV SDK (Swift).Known limits:
Verifiedis reported as unavailable. A node can name one without being banned.Follow-ups:
dash_sdk_add_known_contractsaccepts system-contract IDs.Breaking Changes
None that affect consensus. SDK API changes:
Sdk::set_context_provider; the context provider is fixed at SDK construction.Sdk::context_provider()now returnsOption<Arc<dyn ContextProvider>>.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull request🤖 Generated with Claude Code
PR Hygiene ·
615bcbf/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/quorum_key.rs) — 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 7 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.