feat(sdk): prefer DAPI peers supporting the client's latest protocol version - #4221
feat(sdk): prefer DAPI peers supporting the client's latest protocol version#4221QuantumExplorer wants to merge 6 commits into
Conversation
|
✅ Final review complete — no blockers (commit 3709c6c) |
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change records supported protocol versions for peers, biases live-address selection toward the configured version, and adds bounded concurrent peer probing before SDK protocol-version refresh logic. Probe failures do not change the stored protocol-version ratchet. ChangesProtocol-aware peer selection
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Sdk
participant EvoNodeStatus
participant AddressList
participant ProtocolRefresh
Sdk->>AddressList: Sample live peers
Sdk->>EvoNodeStatus: Probe sampled peers with unproven status requests
EvoNodeStatus-->>Sdk: Return reported protocol versions
Sdk->>AddressList: Record peer versions and set preferred target
Sdk->>ProtocolRefresh: Continue refresh logic
ProtocolRefresh-->>Sdk: Return verified or pinned protocol version
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/sdk.rs (1)
406-425: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider running the peer probe concurrently with the proven ratchet query.
prefer_peers_with_latest_protocol_version()(worst case ~PEER_VERSION_PROBE_TIMEOUT= 4s) is awaited sequentially before the provenExtendedEpochInfo::fetch_currentquery. Since this function is documented to run on app start / network switch, the two steps could overlap instead of stacking their latencies — both only borrow&selfand are independent.⚡ Proposed refactor to overlap the probe and the proven query
pub async fn refresh_protocol_version(&self) -> Result<u32, Error> { - // Best-effort: bias peer selection towards nodes that already support - // this client's newest protocol version. Never fails, never ratchets. - self.prefer_peers_with_latest_protocol_version().await; - - if !self.prove() { - return Ok(self.protocol_version_number()); - } - if !self.version_pinned { - if let Err(error) = ExtendedEpochInfo::fetch_current(self).await { - tracing::warn!( - target: "dash_sdk::protocol_version", - %error, - "proven protocol-version refresh failed; keeping current version \ - (never falling back to an unverified one)" - ); - } - } - Ok(self.protocol_version_number()) + // Best-effort: bias peer selection towards nodes that already support + // this client's newest protocol version. Never fails, never ratchets. + let should_ratchet = self.prove() && !self.version_pinned; + + if should_ratchet { + let (_, ratchet_result) = futures::join!( + self.prefer_peers_with_latest_protocol_version(), + ExtendedEpochInfo::fetch_current(self) + ); + if let Err(error) = ratchet_result { + tracing::warn!( + target: "dash_sdk::protocol_version", + %error, + "proven protocol-version refresh failed; keeping current version \ + (never falling back to an unverified one)" + ); + } + } else { + self.prefer_peers_with_latest_protocol_version().await; + } + Ok(self.protocol_version_number()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/src/sdk.rs` around lines 406 - 425, Update refresh_protocol_version to start prefer_peers_with_latest_protocol_version and the proven ExtendedEpochInfo::fetch_current operation concurrently when applicable, awaiting both while preserving the existing best-effort probe behavior, version_pinned guard, warning, and returned protocol version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 406-425: Update refresh_protocol_version to start
prefer_peers_with_latest_protocol_version and the proven
ExtendedEpochInfo::fetch_current operation concurrently when applicable,
awaiting both while preserving the existing best-effort probe behavior,
version_pinned guard, warning, and returned protocol version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 129416b7-e33d-4750-b878-ab9fe70d6310
📒 Files selected for processing (3)
packages/rs-dapi-client/src/address_list.rspackages/rs-sdk-ffi/src/protocol_version/queries/refresh.rspackages/rs-sdk/src/sdk.rs
|
Re CodeRabbit's nitpick about overlapping the peer probe with the proven ratchet query: declining — the sequencing is intentional. The probe sets the peer preference before the proven 🤖 Addressed by Claude Code |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Reviewed at 5c5cb98c. I found two mobile/runtime blockers and one binding-documentation regression; details are inline.
Validation completed:
cargo test -p rs-dapi-client— 115 unit tests plus integration/doc tests passedcargo test -p dash-sdk --lib— 174 tests passedgit diff --check— clean
I did not find FFI ABI, pointer-lifetime, CString ownership, or Swift memory-management regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- Around line 519-520: Update the documentation comment immediately preceding
the blocking SDK call to state that it always waits for the probe, and waits for
the proven query only when the SDK is not pinned; retain the guidance to invoke
it from a background queue or task rather than the main thread.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 421a1a2c-2cde-420e-aca4-c52764bd1146
📒 Files selected for processing (8)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/QueriesNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.ktpackages/rs-dapi-client/Cargo.tomlpackages/rs-dapi-client/src/address_list.rspackages/rs-sdk-ffi/src/protocol_version/queries/refresh.rspackages/rs-sdk/src/sdk.rspackages/rs-unified-sdk-jni/src/queries.rspackages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-sdk-ffi/src/protocol_version/queries/refresh.rs
- packages/rs-dapi-client/src/address_list.rs
- packages/rs-sdk/src/sdk.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The prior fan-out and binding-documentation issues are fixed, and the Android call path was already marshalled through Dispatchers.IO. Three blocking issues remain: targeted probes can mutate an unrelated peer's ban state, deadline-dropped probes preserve stale rankings, and an unproved self-reported capability can become an exclusive and persistent routing gate.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
🤖 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-sdk/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk/src/sdk.rs:501-506: Probe responses can unban an unrelated peer
The targeted `EvoNode` request still runs through the ordinary `DapiClient` executor. That executor selects a live address B and later records B in `ExecutionResponse.address`, while `EvoNode::execute_transport` ignores the supplied client and creates a separate connection to the requested probe address A. A successful response from A therefore invokes `update_address_ban_status` for B in both the DAPI-client and SDK retry layers. `ban_failed_address: false` suppresses failure-driven bans but does not suppress success-driven unbanning, so the probe can clear B's expired ban ladder or lift a ban placed concurrently while A was being queried. Execute targeted probes through a path that reports A as the response address, or disable health-state mutation for these maintenance requests.
- [BLOCKING] packages/rs-sdk/src/sdk.rs:496-535: Clear stale versions for probes dropped by the total deadline
Only futures yielded before `take_until` reach `set_supported_protocol_version`. If a peer already has `Some(target)` from an earlier refresh and its new probe is dropped at the six-second deadline, the old value remains and the peer is still treated as preferred even though the current pass learned nothing. This is especially reachable because the second concurrency batch can start after the four-second per-node timeouts and then has less than two seconds before the total deadline. Clear sampled addresses before launching the pass or explicitly clear every unfinished sampled address after the stream ends.
In `packages/rs-dapi-client/src/address_list.rs`:
- [BLOCKING] packages/rs-dapi-client/src/address_list.rs:382-397: Do not let an unproved capability report become an exclusive routing gate
The version stored for each peer comes directly from its unproved, self-asserted `getStatus` response, but this branch turns that assertion into an exclusive eligibility filter whenever at least one matching peer exists. A malicious sampled peer can claim `version >= preferred` and become the only selected endpoint when no other sampled peer reports the target. The ranking has no expiry and survives unban, while non-retryable gRPC responses such as `InvalidArgument` and `PermissionDenied` do not ban the peer; the attacker can therefore remain selected and persistently block proven refreshes, queries, and state-transition submissions while observing the client's DAPI traffic. Use this untrusted signal as a non-exclusive weight, preserving a nonzero selection probability for other live peers, or require independently authenticated capability information before filtering exclusively.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4221 +/- ##
=========================================
Coverage 87.53% 87.53%
=========================================
Files 2678 2678
Lines 341047 341047
=========================================
Hits 298519 298519
Misses 42528 42528
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At exact head 6dbe886, combined Codex checkpoint and successful Sonnet general, security, and FFI evidence found no remaining in-scope issues. Source inspection confirms that all three prior blockers are fixed: probes bypass health-mutating DAPI execution, sampled rankings are cleared before probing, and protocol preference is weighted rather than exclusive.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— security-auditor (completed)
…version On startup (and network switch), the SDK now best-effort probes the known DAPI peers with an unproven getStatus and records each node's reported version.protocol.drive.latest — the highest protocol version that node's software supports. Subsequent peer selection prefers, when possible, peers whose reported version is at least the client's own latest supported protocol version (PlatformVersion::latest()). - rs-dapi-client: AddressList tracks a per-address supported protocol version and an optional preferred version target (shared across clones); get_live_address picks among preferred peers first and falls back to any live peer — no peer is ever excluded by version alone, and version knowledge survives unban. - rs-sdk: new Sdk::prefer_peers_with_latest_protocol_version() fans out concurrent getStatus probes (4 s per-node timeout, no retries, no banning), records results, and sets the preference. It is wired into Sdk::refresh_protocol_version(), the documented app-start/network-switch hook, so the FFI/Kotlin/Swift bindings get the behavior without changes. The probe is unproven and feeds only peer ordering; the protocol-version ratchet stays proof-driven. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review feedback on the startup peer probe: - Probe a random sample of at most 16 live peers (mainnet seeds ship ~340 Evo nodes) instead of the whole address list, with at most 8 connections in flight and a 6 s overall wall-clock budget on top of the 4 s per-node timeout. Results that arrive within the budget are kept; unprobed or unanswered peers simply stay unranked and remain usable through the best-effort fallback. New AddressList::sample_live_addresses backs the sampling (rand `alloc` feature enabled for choose_multiple). - Update the binding-facing contracts: Swift SDK.refreshProtocolVersion no longer promises a pinned refresh is a no-op (the probe still runs) and is documented as blocking — call off the main thread; Kotlin/JNI docs now state the probe, the network I/O in pinned mode, and that the blocking native call is marshalled onto Dispatchers.IO by queryGate.op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address the three blocking findings from review: - Probes no longer run through the DapiClient executor, which pairs each response with an address it selected itself and would attribute a targeted probe's success to an unrelated peer, mutating that peer's ban state (success-driven unban / ladder reset). The probe now executes its transport directly against the target address and parses the response via FromUnproved, leaving all peer health state untouched. - Each probe pass clears the recorded supported version of every sampled peer up front, so probes dropped by the overall deadline leave their peer unranked instead of preserving a stale ranking from an earlier pass (previously only completed-but-failed probes were cleared). - Peer preference is now a weight, not an exclusive gate: 90% of selections consult the preferred tier first, the rest select uniformly from all live peers, so a peer's unproved self-reported version can bias routing but never capture it. Selection tests updated from exact-match loops to statistical bounds accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6dbe886 to
0d2a3e1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/sdk.rs (1)
594-604: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider stopping the probe when the SDK is cancelled.
The probe pass only ends on the 6 second budget or on completion. If a caller triggers
Sdk::shutdownduringrefresh_protocol_version, the call still waits for the budget. Add the cancel token to the stop condition to shorten shutdown.♻️ Proposed change to also stop on cancellation
- .take_until(rs_dapi_client::transport::sleep( - PEER_VERSION_PROBE_TOTAL_BUDGET, - ))); + .take_until(futures::future::select( + Box::pin(rs_dapi_client::transport::sleep( + PEER_VERSION_PROBE_TOTAL_BUDGET, + )), + Box::pin(self.cancelled()), + )));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/src/sdk.rs` around lines 594 - 604, Update the probe stream construction in refresh_protocol_version to include the SDK cancellation token in the take_until stop condition alongside PEER_VERSION_PROBE_TOTAL_BUDGET. Ensure Sdk::shutdown causes the probe loop consuming probes to terminate promptly while preserving the existing time-budget behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 594-604: Update the probe stream construction in
refresh_protocol_version to include the SDK cancellation token in the take_until
stop condition alongside PEER_VERSION_PROBE_TOTAL_BUDGET. Ensure Sdk::shutdown
causes the probe loop consuming probes to terminate promptly while preserving
the existing time-budget behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 52c60b38-d252-43b9-a8f3-8d0799c528c0
📒 Files selected for processing (8)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/QueriesNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.ktpackages/rs-dapi-client/Cargo.tomlpackages/rs-dapi-client/src/address_list.rspackages/rs-sdk-ffi/src/protocol_version/queries/refresh.rspackages/rs-sdk/src/sdk.rspackages/rs-unified-sdk-jni/src/queries.rspackages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/QueriesNative.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt
- packages/rs-unified-sdk-jni/src/queries.rs
- packages/rs-sdk-ffi/src/protocol_version/queries/refresh.rs
- packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
- packages/rs-dapi-client/Cargo.toml
- packages/rs-dapi-client/src/address_list.rs
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Applied CodeRabbit's cancellation nitpick in 3709c6c: the probe's 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The only substantive change since the last clean review (6dbe886) is commit 3709c6c, a 14-line diff to packages/rs-sdk/src/sdk.rs that adds a futures::future::select cancellation arm to the peer protocol-version probe's take_until, so Sdk::shutdown() now stops the probe fan-out immediately instead of waiting out the full PEER_VERSION_PROBE_TOTAL_BUDGET. I re-verified this directly: cargo check and cargo clippy --lib -- -D warnings both pass cleanly for dash-sdk, the cancel_token/cancelled()/shutdown() wiring is correct and consistent with its documented contract, and the change touches no consensus-critical, proof, or FFI-exposed surface. No agent raised any in-scope blocking or suggestion findings, and none were fabricated here — the PR remains clean.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— security-auditor (completed)
2 participants
Issue being fixed or feature implemented
When a client (rs-sdk / platform wallet / mobile bindings) connects to DAPI, peer selection is uniformly random over all live addresses. Nodes running older software may not support the client's newest protocol version, which can degrade behavior for version-sensitive flows. On startup the client should make a best effort to talk to peers whose software already supports (i.e. desires/votes for) the client's latest protocol version.
What was done?
AddressListnow tracks, per address, the highest protocol version the node reports supporting, plus an optional preferred-version target shared across clones.get_live_addresspicks randomly among live peers whose reported version is at least the preferred one, and falls back to any live peer when none match — no peer is ever excluded by version alone, and version knowledge survives unban (health and version are orthogonal).Sdk::prefer_peers_with_latest_protocol_version()concurrently issues an unprovengetStatusto every live peer (4 s per-node timeout, no retries, no banning), records each node'sversion.protocol.drive.latest, and sets the preference target toPlatformVersion::latest().protocol_version(the client's newest supported version). Probe failures leave the peer in rotation, unranked.Sdk::refresh_protocol_version()— the documented app-start / network-switch hook — so rs-sdk-ffi (Swift), the JNI bindings (Kotlin), and any caller of refresh get the behavior with no binding changes. It runs even for pinned or proofs-disabled SDKs (peer preference is orthogonal to version pinning); the proven version-ratchet path is unchanged and the unproven probe never feeds it.How Has This Been Tested?
rs-dapi-clientcovering: preference targeting (>=semantics, newer peers count as preferred), best-effort fallback when no peer matches, ban filtering taking precedence over preference, version knowledge surviving unban, and preference propagation acrossAddressListclones.cargo test -p rs-dapi-clientandcargo test -p dash-sdk --libpass;cargo clippy --all-targetsclean on both crates;rs-sdk-ffiandwasm-sdk(wasm32-unknown-unknown) still compile.Breaking Changes
None — all API additions are backward compatible, and with no preference set peer selection behaves exactly as before.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit