fix(sdk): fail on invalid proof #2864
Merged
Conversation
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
Contributor
WalkthroughModified proof verification and error handling across the SDK. Removed debug logging output, expanded retry mechanisms to include proof-related errors, and added a performance optimization note for verification scheduling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
✅ gRPC Query Coverage Report |
![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: 0
🧹 Nitpick comments (2)
packages/rs-sdk/src/error.rs (1)
186-191: Retrying onError::Proof(_)aligns with PR goal; consider finer‑grained classification laterExtending
can_retryto includeError::Proof(_)matches the intent to fail over when a node returns an invalid or stale proof and keeps behavior consistent withStaleNode/TimeoutReached.One nuance:
Error::Proof(_)also covers locally-caused verifier errors such asContextProviderNotSet(seeparse_proof_with_metadata_and_proof), where retrying won’t help and only adds extra calls. It’s not a blocker, but over time it may be worth distinguishing “node‑dependent” proof failures from “local/misconfiguration” ones and only treating the former as retryable. Based on learningspackages/rs-sdk/src/sdk.rs (1)
337-339: TODO is accurate and well‑placed; consider tying it to a tracking issueThe new TODO correctly describes that metadata freshness checks are cheap compared to proof validation and should ideally run first. No functional change is introduced here; the comment just clarifies intended future work. If there’s a ticket tracking this optimization, referencing it in the TODO would make it easier to follow up. Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive-proof-verifier/src/proof/token_info.rs(0 hunks)packages/rs-sdk/src/error.rs(1 hunks)packages/rs-sdk/src/sdk.rs(1 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-drive-proof-verifier/src/proof/token_info.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-sdk/src/sdk.rspackages/rs-sdk/src/error.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2332
File: packages/rs-drive-proof-verifier/src/error.rs:21-23
Timestamp: 2024-11-15T14:39:23.704Z
Learning: In the `packages/rs-drive-proof-verifier`, avoid suggesting size limits for `proof_bytes` in errors unless there's a specific need, as it may not be necessary.
📚 Learning: 2024-10-18T15:43:32.447Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-sdk/src/sdk.rs:654-658
Timestamp: 2024-10-18T15:43:32.447Z
Learning: In `packages/rs-sdk/src/sdk.rs`, within the `verify_metadata_height` function, when using `compare_exchange` on `last_seen_height`, it's acceptable to use `Ordering::Relaxed` for the failure ordering, as any inconsistency would only trigger an extra loop iteration without affecting correctness.
Applied to files:
packages/rs-sdk/src/sdk.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-sdk/src/sdk.rspackages/rs-sdk/src/error.rs
📚 Learning: 2024-10-30T11:04:33.634Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/platform/fetch_unproved.rs:0-0
Timestamp: 2024-10-30T11:04:33.634Z
Learning: In `packages/rs-sdk/src/platform/fetch_unproved.rs`, the `execute()` method consumes the request object, so cloning the request is necessary before passing it to `execute()` and `maybe_from_unproved_with_metadata`.
Applied to files:
packages/rs-sdk/src/sdk.rs
📚 Learning: 2025-08-05T13:55:39.147Z
Learnt from: thephez
Repo: dashpay/platform PR: 2718
File: packages/wasm-sdk/index.html:0-0
Timestamp: 2025-08-05T13:55:39.147Z
Learning: The get_identity_keys_with_proof_info function in the Rust WASM bindings does not support the "search" key request type and lacks the searchPurposeMap parameter. When proof mode is enabled with keyRequestType === 'search', the implementation falls back to the non-proof version (get_identity_keys) to maintain functionality.
Applied to files:
packages/rs-sdk/src/sdk.rs
📚 Learning: 2024-11-15T14:39:23.704Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2332
File: packages/rs-drive-proof-verifier/src/error.rs:21-23
Timestamp: 2024-11-15T14:39:23.704Z
Learning: In the `packages/rs-drive-proof-verifier`, avoid suggesting size limits for `proof_bytes` in errors unless there's a specific need, as it may not be necessary.
Applied to files:
packages/rs-sdk/src/sdk.rs
📚 Learning: 2025-05-28T16:22:26.334Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2644
File: packages/rs-drive/src/cache/system_contracts.rs:18-19
Timestamp: 2025-05-28T16:22:26.334Z
Learning: In packages/rs-drive/src/cache/system_contracts.rs, the `active_since_protocol_version` field in `ActiveSystemDataContract` struct is intentionally added for future use, not current implementation. QuantumExplorer confirmed this is "meant for later" when questioned about the `#[allow(unused)]` attribute.
Applied to files:
packages/rs-sdk/src/sdk.rs
📚 Learning: 2024-10-04T14:16:05.798Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2207
File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664
Timestamp: 2024-10-04T14:16:05.798Z
Learning: In the implementation of `FromProof<platform::GetContestedResourceIdentityVotesRequest>` in `packages/rs-drive-proof-verifier/src/proof.rs`, when matching `maybe_votes`, using `.expect()` on `v.into_iter().next()` is acceptable because the prior match arm `Some(v) if v.is_empty()` ensures that the map is not empty, preventing a panic.
Applied to files:
packages/rs-sdk/src/sdk.rspackages/rs-sdk/src/error.rs
📚 Learning: 2024-10-18T13:52:36.233Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
Applied to files:
packages/rs-sdk/src/error.rs
📚 Learning: 2024-10-22T10:53:12.111Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/dapi_client.rs:137-139
Timestamp: 2024-10-22T10:53:12.111Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when passing data into asynchronous code, ensure that data structures are `Send + Sync`. Using `Arc<AtomicUsize>` is necessary for the retry counter.
Applied to files:
packages/rs-sdk/src/error.rs
📚 Learning: 2024-10-25T10:35:05.071Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2266
File: packages/rs-sdk/src/sync.rs:171-221
Timestamp: 2024-10-25T10:35:05.071Z
Learning: In `packages/rs-sdk/src/sync.rs`, within the `retry` function, the closure passed to `.when()` in `backon::Retryable::retry` is not executed concurrently, so using a mutable variable like `retries` inside it does not lead to data races.
Applied to files:
packages/rs-sdk/src/error.rs
🧬 Code graph analysis (2)
packages/rs-sdk/src/sdk.rs (1)
packages/wasm-sdk/src/queries/mod.rs (1)
metadata(237-239)
packages/rs-sdk/src/error.rs (1)
packages/js-dapi-client/lib/methods/platform/waitForStateTransitionResult/WaitForStateTransitionResultResponse.js (1)
Proof(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Swift SDK and Example build (warnings as errors)
|
✅ 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: "3817f56406cc5d57b3164248d5cb9c44aaf0bdb09af75532deb1539909bacdc6"
)Xcode manual integration:
|
Member
|
I don't think this is the correct approach, we should only retry when we get a proof error and the proof has metadata showing the block is old. |
Collaborator
Author
|
@QuantumExplorer malicious node can send you wrong formed proof but valid metadata. |
QuantumExplorer
approved these changes
Nov 21, 2025
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
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
SDK fails without retries when it gets invalid proof from a malicious or outdated node
What was done?
Future improvement: validate response height and time (freshness) before we validate proofs (heavy check).
How Has This Been Tested?
With existing tests.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Chores