fix(drive): consolidate historical contract proof verification retry logic#3165
fix(drive): consolidate historical contract proof verification retry logic#3165thepastaclaw wants to merge 7 commits intodashpay:v3.1-devfrom
Conversation
…logic When contract_known_keeps_history is None, any result other than Ok((hash, Some(contract))) now triggers exactly one retry with history enabled. Previously, retry logic was scattered across multiple match arms and several failure paths bypassed it entirely. Refactored verify_contract_v0 and verify_contract_return_serialization_v0 to extract _given_history helpers containing the pure verification logic, with retry decisions made solely in the outer functions. Added regression test: test_contract_keeps_history_verify_with_unknown_history_flag
The previous commit incorrectly restricted retry to Err(_) only, but Ok(None) from the non-historical path is a legitimate trigger for historical retry — it means the contract exists in the historical path, not that it's absent. Restoring the _ catch-all preserves the original fix from a7fec8b. Test assertions now require contracts to be returned (not accept None) for cases where the contract definitely exists, eliminating tautological acceptance paths.
📝 WalkthroughWalkthroughThis pull request refactors contract verification logic to be history-aware by introducing conditional verification flows based on a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
Actionable comments posted: 1
🤖 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-drive/tests/query_tests.rs`:
- Around line 4702-4703: Rename the test function
test_contract_keeps_history_verify_with_unknown_history_flag to follow the
repository convention by starting with "should_"; update the function signature
(fn test_contract_keeps_history_verify_with_unknown_history_flag) to a
descriptive name beginning with should_, e.g.
should_contract_keep_history_verify_with_unknown_history_flag, and update any
references or test runners expecting the old name so the test still compiles and
runs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rspackages/rs-drive/src/verify/contract/verify_contract_return_serialization/v0/mod.rspackages/rs-drive/tests/query_tests.rs
|
Just ran into this issue and finally ended up here |
Migrate all contract register/retrieve/update tutorials from legacy js-dash-sdk (CJS) to evo-sdk (ESM). Adds new variants (binary, indexed, timestamps, history-retrieve) and works around sdk.contracts.fetch() returning undefined for keepsHistory contracts (dashpay/platform#3165). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for confirming, @thephez! The root cause is that when This PR consolidates the retry logic so that when the history flag is unknown, it retries with The PR is currently in draft — will work on getting it reviewed and merged. |
Empirical testing of the retry logicI ran two diagnostic tests against this branch to understand the actual behavior of Test 1: Non-existent contract with
|
|
Great analysis, @lklimek — these tests are exactly what was needed. Your findings hold upI traced through the proof generation and verification paths, and I agree with your conclusions:
Your Test 2 confirms the key implication: the conditional proof contains enough tree structure information that
Both tests hold because the conditional proof is self-describing — it includes the necessary path information for either query. Where does this leave the PR?The code change itself is still cleaner than what's on The open question@thephez confirmed encountering the "Data contract not found" error in practice. That error originates from the SDK layer ( Possible explanations I haven't ruled out:
ProposalI'll update the PR description to accurately reflect that the retry is defensive rather than actively fixing a known triggering condition. The cleanup consolidation is still worthwhile IMO, but the "Data contract not found" report from thephez needs separate investigation to find the actual root cause. @thephez — could you share more context on when you hit this? Specifically: which SDK (Rust/JS/Swift), which platform version, and whether it was against testnet or a local setup? That would help narrow down where the actual failure is. |
Correction: bug confirmed, PR is correctMy earlier analysis was wrong. After further testing I can confirm: The bug is real and reproducible on
|
| in_multiple_contract_proof_form, | ||
| contract_id, | ||
| platform_version, | ||
| ) |
There was a problem hiding this comment.
SEC-001 (LOW) — Restore diagnostic context in retry log
The old code logged the original result before retrying:
tracing::debug!(?path_query, error=?e, "retrying contract verification with history enabled");The new code only logs contract_id, losing the original result and path query context that helps diagnose verification issues.
Suggestion: log the original result at debug level before retrying:
tracing::debug!(?contract_id, ?result, "retrying contract verification with history enabled");🤖 Claudius
| proof_contract_4.expect("contract exists — return_serialization must return it"); | ||
| assert_eq!(latest_contract, deserialized); | ||
| } | ||
|
|
There was a problem hiding this comment.
PROJ-001 (LOW) — Add test for non-existent contract with None history flag
The test covers the happy path (historical contract exists, None triggers successful retry) but does not cover the opposite: a contract ID that genuinely does not exist with contract_known_keeps_history=None.
Empirical testing confirms this case is safe (absence proof verifies on both paths, retry returns Ok((_, None)) correctly), but an explicit test would prevent regressions if the retry logic changes:
#[test]
fn test_verify_non_existent_contract_with_unknown_history_flag() {
let (drive, _contract) = setup_references_tests(10, 3334);
let platform_version = PlatformVersion::latest();
let fake_contract_id = [255u8; 32];
let contract_proof = drive
.prove_contract(fake_contract_id, None, platform_version)
.expect("expected to get proof for non-existent contract");
let (_, proof_returned_contract) = Drive::verify_contract(
contract_proof.as_slice(),
None,
false,
false,
fake_contract_id,
platform_version,
)
.expect("expected verification to succeed for non-existent contract");
assert!(
proof_returned_contract.is_none(),
"expected None for a non-existent contract, got Some",
);
}No such test exists in the codebase.
🤖 Claudius
| } | ||
|
|
||
| #[test] | ||
| fn test_contract_keeps_history_verify_with_unknown_history_flag() { |
There was a problem hiding this comment.
PROJ-002 (LOW) — Test does not verify retry was actually exercised
Test 1 asserts the end result is correct but doesn't prove the retry path was taken. If a GroveDb change made non-historical queries accidentally succeed on historical proofs, this test would still pass while the retry logic becomes dead code.
Consider capturing the tracing debug log ("retrying contract verification with history enabled") via tracing-test subscriber and asserting it was emitted exactly once.
🤖 Claudius
| let (proof_root_hash, proof_contract) = Drive::verify_contract( | ||
| contract_proof.as_slice(), | ||
| None, | ||
| false, |
There was a problem hiding this comment.
PROJ-003 (INFO) — No coverage for in_multiple_contract_proof_form=true
All four sub-cases use in_multiple_contract_proof_form=false. The verify_contracts (plural) function calls verify_contract with (None, true, true) — so this retry path is used in production but not directly tested by this PR or anywhere else in the test suite. Consider adding a test with true, or testing verify_contracts with a historical contract.
🤖 Claudius
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive/tests/query_tests.rs (1)
926-966:⚠️ Potential issue | 🟠 MajorAdd
#[cfg(feature = "server")]guard tosetup_references_tests_with_keeps_history.The helper at line 926 uses symbols that are only imported behind
#[cfg(feature = "server")]:setup_drive,DriveConfig,Drive,GroveDbOpBatch,setup_contract, andadd_init_contracts_structure_operations. Without a matching guard on the function itself, this will fail to compile when theserverfeature is disabled. The wrapper functionsetup_references_testsabove it is already guarded; the new helper should be too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/tests/query_tests.rs` around lines 926 - 966, Add the same feature-gate attribute used for the wrapper to the helper: put #[cfg(feature = "server")] immediately above the setup_references_tests_with_keeps_history function declaration so that symbols like setup_drive, DriveConfig, Drive, GroveDbOpBatch, setup_contract, and add_init_contracts_structure_operations are only referenced when the "server" feature is enabled.
🧹 Nitpick comments (2)
packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rs (1)
109-151: Extract the shared proof-shape validator.This block is now almost identical to
packages/rs-drive/src/verify/contract/verify_contract_return_serialization/v0/mod.rs, and the duplication already shows drift: the non-historical error path still says"historical contract". A common helper that validates(path, key, maybe_element)and returns the raw item bytes would keep both verifiers aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rs` around lines 109 - 151, Extract a shared helper (e.g., validate_proved_element or extract_proved_contract_bytes) that takes (path, key, maybe_element, contract_id, keeps_history, platform_version) and encapsulates the proof-shape validation and byte extraction currently duplicated in verify_contract::v0::mod.rs: check path against contract_keeping_history_root_path when keeps_history is true, otherwise check path == contract_root_path and key == vec![0], return a clear CorruptedProof error message appropriate to the historical vs non-historical case, convert maybe_element into item bytes and run DataContract::versioned_deserialize(..., false, platform_version) returning the deserialized contract or Error; replace the inline block in verify_contract::v0::mod.rs with a call to this helper and reuse the same helper from verify_contract_return_serialization::v0::mod.rs to eliminate duplication and fix the incorrect error text.packages/rs-drive/tests/query_tests.rs (1)
4702-4813: Prefer separateshould_...tests for each verification branch.This bundles four distinct paths into one test, so a failure won't tell you which branch regressed, and the new name falls outside the repo's convention for new tests.
Based on learnings, new tests in files under
testsshould start withshould …; existing tests can keep their current convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/tests/query_tests.rs` around lines 4702 - 4813, The current test test_contract_keeps_history_verify_with_unknown_history_flag bundles four verification branches (None, Some(true), Some(false), and verify_contract_return_serialization(None)) making failures ambiguous; split it into four focused tests named with the repo convention (e.g., should_verify_contract_with_unknown_history_retry, should_verify_contract_with_explicit_historical, should_verify_contract_with_explicit_non_historical, should_return_serialization_with_unknown_history) that each set up only the needed fixtures (use setup_references_tests_with_keeps_history and separate non-historical setup for the Some(false) case), call the appropriate functions (Drive::verify_contract or Drive::verify_contract_return_serialization) for that single branch, and assert only that branch’s expectations so failures clearly indicate which path regressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/rs-drive/tests/query_tests.rs`:
- Around line 926-966: Add the same feature-gate attribute used for the wrapper
to the helper: put #[cfg(feature = "server")] immediately above the
setup_references_tests_with_keeps_history function declaration so that symbols
like setup_drive, DriveConfig, Drive, GroveDbOpBatch, setup_contract, and
add_init_contracts_structure_operations are only referenced when the "server"
feature is enabled.
---
Nitpick comments:
In `@packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rs`:
- Around line 109-151: Extract a shared helper (e.g., validate_proved_element or
extract_proved_contract_bytes) that takes (path, key, maybe_element,
contract_id, keeps_history, platform_version) and encapsulates the proof-shape
validation and byte extraction currently duplicated in
verify_contract::v0::mod.rs: check path against
contract_keeping_history_root_path when keeps_history is true, otherwise check
path == contract_root_path and key == vec![0], return a clear CorruptedProof
error message appropriate to the historical vs non-historical case, convert
maybe_element into item bytes and run DataContract::versioned_deserialize(...,
false, platform_version) returning the deserialized contract or Error; replace
the inline block in verify_contract::v0::mod.rs with a call to this helper and
reuse the same helper from verify_contract_return_serialization::v0::mod.rs to
eliminate duplication and fix the incorrect error text.
In `@packages/rs-drive/tests/query_tests.rs`:
- Around line 4702-4813: The current test
test_contract_keeps_history_verify_with_unknown_history_flag bundles four
verification branches (None, Some(true), Some(false), and
verify_contract_return_serialization(None)) making failures ambiguous; split it
into four focused tests named with the repo convention (e.g.,
should_verify_contract_with_unknown_history_retry,
should_verify_contract_with_explicit_historical,
should_verify_contract_with_explicit_non_historical,
should_return_serialization_with_unknown_history) that each set up only the
needed fixtures (use setup_references_tests_with_keeps_history and separate
non-historical setup for the Some(false) case), call the appropriate functions
(Drive::verify_contract or Drive::verify_contract_return_serialization) for that
single branch, and assert only that branch’s expectations so failures clearly
indicate which path regressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c208a6b4-9346-43a1-ae9b-f05e50914679
📒 Files selected for processing (3)
packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rspackages/rs-drive/src/verify/contract/verify_contract_return_serialization/v0/mod.rspackages/rs-drive/tests/query_tests.rs
|
Thanks for the follow-up, @lklimek — and for running the PR's own test against Your breakdown of why I should have caught that distinction in my earlier response instead of agreeing that the retry was dead code — lesson learned. The PR description was correct all along. |
4 participants
Issue
When
getDataContractis called for a contract withkeepsHistory: true, it returns "Data contract not found" even thoughgetDataContractHistorysucceeds for the same contract.Root cause: In
verify_contract_v0, whencontract_known_keeps_historyisNone(as it always is fromFromProof<GetDataContractRequest>), the code defaults to the non-historical path. If that returnsOk(None)(contract not found on the non-historical path), it never retries with the historical path — it only retried onErr(_).Changes
Core fix (both
verify_contractandverify_contract_return_serialization)_given_historyhelpers that perform verification with a known history flagcontract_known_keeps_historyisNone, retry withkeeps_history = trueif the first attempt returnsErr(_)orOk((_, None))Tests
test_contract_keeps_history_verify_with_unknown_history_flagregression test covering:None(unknown) — must succeed via retry for historical contractsSome(true)— direct historical verificationSome(false)— non-historical contract verificationverify_contract_return_serializationwithNone— parallel path coverageValidation
cargo test -p drive✅cargo clippy -p drive -- -D warnings✅Summary by CodeRabbit
New Features
Bug Fixes
Tests