Uh oh!
There was an error while loading. Please reload this page.
fix(sdk): backport proved current-epoch fetch fix to v4.1 - #4480
fix(sdk): backport proved current-epoch fetch fix to v4.1#4480PastaPastaPasta wants to merge 1 commit into
Conversation
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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
⛔ Blockers found — Opus deferred (commit 0267427) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## v4.1-dev #4480 +/- ##
=============================================
- Coverage 87.54% 66.44% -21.11%
=============================================
Files 2671 27 -2644 Lines 338859 2798 -336061 =============================================
- Hits 296666 1859 -294807 + Misses 42193 939 -41254
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The explicit-start proof flow correctly avoids accepting unsigned epoch metadata as the current epoch, but two blocking paths remain: independently routed confirmation requests can produce false EpochNotFound errors during normal node convergence, and the FFI status wrapper converts that fail-closed result into successful zero-valued status. The patch also introduces a source-compatibility break in a public exhaustive Rust enum and leaks the error allocation in a new FFI test.
Source: Codex reviewer lanes (general, security-auditor, rust-quality, and ffi-engineer; exact backend model IDs were not present in the supplied evidence); Claude final verifier (exact backend model ID was not present in the supplied evidence). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus 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— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 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/platform/types/epoch.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/types/epoch.rs:124-127: Retry when the confirmation lands on an honest node behind the probe
The probe and confirmation are separate fetch operations, and each request independently chooses a random live address through `AddressList::get_live_address`. At an epoch boundary, the probe can observe epoch `n` from one node while the confirmation reaches an honest node still at `n - 1`. The second node returns a valid proof with no started epoch at or above candidate `n`, and its one-block-lower metadata passes the SDK's default height tolerance of 1. This branch then creates `EpochNotFound` outside the fetch retry mechanism, so the restored API fails intermittently during ordinary network convergence. The same failure can occur after a proof advances `candidate` and the next refinement reaches an older node. Retry empty confirmations with a bounded policy that preserves the highest proven candidate, or keep the multi-step operation on one node; do not lower the candidate from unsigned metadata.
In `packages/rs-sdk-ffi/src/system/queries/platform_status.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/system/queries/platform_status.rs:88-98: Do not turn an untrusted inflated epoch hint into successful zero status
`EpochNotFound` from the new resolver does not authenticate that Platform has no epochs. The metadata epoch is omitted from the Tenderdash `StateId` constructed in `rs-drive-proof-verifier/src/verify.rs`, so a server can inflate that field without invalidating the quorum signature. The confirmation then validly proves that the attacker-selected future range is empty and returns `EpochNotFound`. This arm converts that fail-closed error into a successful response claiming zero block and core heights; the same false success can result from the honest cross-node race in the SDK resolver. Since an initialized Platform necessarily has a genesis epoch, propagate this error rather than representing it as valid status data.
- [NITPICK] packages/rs-sdk-ffi/src/system/queries/platform_status.rs:225-228: Free the FFI error returned by the new test
On this path, `dash_sdk_get_platform_status` allocates both a boxed `DashSDKError` and its `CString` message. Ownership is returned through `DashSDKResult.error`, but the new test destroys only the SDK handle, leaking both allocations and modeling the foreign caller's ownership contract incorrectly. Return the error through `dash_sdk_error_free` before destroying the handle.
In `packages/rs-sdk/src/error.rs`:
- [SUGGESTION] packages/rs-sdk/src/error.rs:390-402: Adding a public error variant breaks 4.1 source compatibility
`StaleNodeError` was already a public, exhaustive enum in the released 4.1.1 API. Downstream Rust code can legally exhaustively match its `Height` and `Time` variants; adding `Epoch` makes that code fail to compile with a non-exhaustive-pattern error. This conflicts with the patch backport's claim that it has no breaking changes. Represent the condition using an existing compatible error shape or explicitly treat the release as source-breaking. Adding `#[non_exhaustive]` in this patch would not restore compatibility because that annotation also breaks existing exhaustive matches.
| match started.keys().next_back().copied() { | ||
| // Nothing at or above the candidate has started: the query landed in | ||
| // Drive's pre-created empty epoch window. | ||
| None => return Err(Error::EpochNotFound), |
There was a problem hiding this comment.
🔴 Blocking: Retry when the confirmation lands on an honest node behind the probe
The probe and confirmation are separate fetch operations, and each request independently chooses a random live address through AddressList::get_live_address. At an epoch boundary, the probe can observe epoch n from one node while the confirmation reaches an honest node still at n - 1. The second node returns a valid proof with no started epoch at or above candidate n, and its one-block-lower metadata passes the SDK's default height tolerance of 1. This branch then creates EpochNotFound outside the fetch retry mechanism, so the restored API fails intermittently during ordinary network convergence. The same failure can occur after a proof advances candidate and the next refinement reaches an older node. Retry empty confirmations with a bounded policy that preserves the highest proven candidate, or keep the multi-step operation on one node; do not lower the candidate from unsigned metadata.
source: ['codex']
| Err(dash_sdk::Error::EpochNotFound) => { | ||
| // No epoch to read a version off, so fall back to the highest | ||
| // version the SDK has verified so far. | ||
| let json = format!( | ||
| r#"{{"version":{},"network":"{}","blockHeight":0,"coreHeight":0}}"#, | ||
| sdk.protocol_version_number(), | ||
| network_str | ||
| ); | ||
| Ok(json) | ||
| } | ||
| Err(e) => Err(format!("Failed to fetch platform status: {}", e)), |
There was a problem hiding this comment.
🔴 Blocking: Do not turn an untrusted inflated epoch hint into successful zero status
EpochNotFound from the new resolver does not authenticate that Platform has no epochs. The metadata epoch is omitted from the Tenderdash StateId constructed in rs-drive-proof-verifier/src/verify.rs, so a server can inflate that field without invalidating the quorum signature. The confirmation then validly proves that the attacker-selected future range is empty and returns EpochNotFound. This arm converts that fail-closed error into a successful response claiming zero block and core heights; the same false success can result from the honest cross-node race in the SDK resolver. Since an initialized Platform necessarily has a genesis epoch, propagate this error rather than representing it as valid status data.
| Err(dash_sdk::Error::EpochNotFound) => { | |
| // No epoch to read a version off, so fall back to the highest | |
| // version the SDK has verified so far. | |
| let json = format!( | |
| r#"{{"version":{},"network":"{}","blockHeight":0,"coreHeight":0}}"#, | |
| sdk.protocol_version_number(), | |
| network_str | |
| ); | |
| Ok(json) | |
| } | |
| Err(e) => Err(format!("Failed to fetch platform status: {}", e)), | |
| Err(e) => Err(format!("Failed to fetch platform status: {}", e)), |
source: ['codex']
| /// Server kept reporting a current epoch that its own proofs contradict | ||
| /// | ||
| /// The epoch index in response metadata is not covered by the quorum | ||
| /// signature, so `ExtendedEpochInfo::fetch_current` only uses it to shape a | ||
| /// proved query and then checks it against the proof. This error means the | ||
| /// check kept failing: every proof showed a newer epoch already started. | ||
| #[error("received epoch is outdated: hinted {hinted_epoch}, proven started epoch {proven_epoch}; try another server")] | ||
| Epoch { | ||
| /// Epoch index the server reported as current in unsigned response metadata | ||
| hinted_epoch: EpochIndex, | ||
| /// Newer epoch index that the server's own proof showed as already started | ||
| proven_epoch: EpochIndex, | ||
| }, |
There was a problem hiding this comment.
🟡 Suggestion: Adding a public error variant breaks 4.1 source compatibility
StaleNodeError was already a public, exhaustive enum in the released 4.1.1 API. Downstream Rust code can legally exhaustively match its Height and Time variants; adding Epoch makes that code fail to compile with a non-exhaustive-pattern error. This conflicts with the patch backport's claim that it has no breaking changes. Represent the condition using an existing compatible error shape or explicitly treat the release as source-breaking. Adding #[non_exhaustive] in this patch would not restore compatibility because that annotation also breaks existing exhaustive matches.
source: ['codex']
| let result = dash_sdk_get_platform_status(handle); | ||
| assert!(!result.error.is_null()); | ||
| assert_eq!(result.data_type, DashSDKResultDataType::NoData); | ||
| destroy_mock_sdk_handle(handle); |
There was a problem hiding this comment.
💬 Nitpick: Free the FFI error returned by the new test
On this path, dash_sdk_get_platform_status allocates both a boxed DashSDKError and its CString message. Ownership is returned through DashSDKResult.error, but the new test destroys only the SDK handle, leaking both allocations and modeling the foreign caller's ownership contract incorrectly. Return the error through dash_sdk_error_free before destroying the handle.
| let result = dash_sdk_get_platform_status(handle); | |
| assert!(!result.error.is_null()); | |
| assert_eq!(result.data_type,DashSDKResultDataType::NoData); | |
| destroy_mock_sdk_handle(handle); | |
| let result = dash_sdk_get_platform_status(handle); | |
| assert!(!result.error.is_null()); | |
| assert_eq!(result.data_type,DashSDKResultDataType::NoData); | |
| crate::dash_sdk_error_free(result.error); | |
| destroy_mock_sdk_handle(handle); |
source: ['codex']
Issue being fixed or feature implemented
epoch.current()/ExtendedEpochInfo::fetch_currentalways fails in the released v4.1.0 and v4.1.1 with:The proof-context binding hardening (#4166) shipped in v4.1.0 and rejects proved descending epoch queries without an explicit start epoch, but the SDK's own current-epoch fetch still issued exactly that query shape. The fix (#4231) was merged to v4.2-dev on Aug 2 and never reached the 4.1 line, so v4.1.1 (published Aug 18, current npm latest for @dashevo/evo-sdk) ships with the regression. Reproducible against testnet with the published npm artifact.
What was done?
Clean cherry-pick of 65b5bae (#4231) onto v4.1-dev: restores the proved current-epoch fetch as a two-step, guard-compliant query (proved anchor first, then an explicit-start descending epochs query), including the updated offline test vectors.
How Has This Been Tested?
cargo check -p dash-sdk -p drive --lockedon this branch: cleancargo test -p dash-sdk epoch(offline vectors): 5 passed, 0 failedBreaking Changes
None — restores intended behavior of an existing API.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code