fix(drive): use historical path query for contracts with keeps_history=true #2976
Conversation
…y=true When creating/updating a data contract with keeps_history=true, the proof verification would fail with "proof did not contain contract with id ...". Root cause: In prove_state_transition_v0, the code always used non-historical path queries regardless of whether the contract had keeps_history=true. Fix: - Check keeps_history() config on data contract and use appropriate path query - Update verify_state_transitions.rs to pass keeps_history to Drive::verify_contract - Add config() accessor to DataContractInSerializationFormat Includes regression tests that verify: - Contract creation with keeps_history=true works with proof verification - Both historical and non-historical contracts work correctly together Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughThis PR exposes a Changes
Sequence DiagramsequenceDiagram
participant Validator as StateTransitionValidator
participant Action as DataContractCreate/Update
participant Serialized as SerializedDataContract
participant ProofGen as ProofGenerator
participant PathQuery as PathQuerySelector
Validator->>Action: Receive state transition
Action->>Serialized: access config()
Serialized-->>Action: DataContractConfig (keeps_history)
Action->>ProofGen: request proof (include keeps_history)
ProofGen->>PathQuery: select query based on keeps_history
alt keeps_history = true
PathQuery-->>ProofGen: historical_path_query (unlimited)
else keeps_history = false
PathQuery-->>ProofGen: non_historical_path_query
end
ProofGen-->>Validator: return proof
Validator->>Action: verification result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs (1)
38-39: Consider renaming tests to follow "should ..." convention.Per coding guidelines, test names should start with "should ...". Consider:
run_chain_create_contract_with_keeps_history_true→should_create_contract_with_keeps_history_truerun_chain_create_contracts_with_and_without_history→should_create_contracts_with_and_without_historyAlso applies to: 161-162
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rspackages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
🧠 Learnings (14)
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2025-10-09T15:59:28.329Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs:181-187
Timestamp: 2025-10-09T15:59:28.329Z
Learning: In `packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs`, the maintainer requires logging full state transition bytes (`tx_bytes = hex::encode(st_bytes)`) at debug level when a state transition passes CheckTx but is removed from the block by the proposer, to facilitate debugging of this rare edge case.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.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-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
📚 Learning: 2024-09-30T12:11:35.148Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2186
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs:48-54
Timestamp: 2024-09-30T12:11:35.148Z
Learning: In the identity credit withdrawal transition code, the field `platform_version.drive_abci.validation_and_processing.state_transitions.identity_credit_withdrawal_state_transition.transform_into_action` is not an `Option` type, so handling `None` cases is unnecessary.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2024-10-06T16:18:07.994Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use state transitions for updates in documents and data contracts
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts (ensuring it's between 3 and 100 characters) is already handled at the data contract validation level in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, making additional checks in the update operations redundant.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
📚 Learning: 2024-10-04T09:08:47.901Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2206
File: packages/rs-drive-abci/tests/strategy_tests/main.rs:1162-1162
Timestamp: 2024-10-04T09:08:47.901Z
Learning: In the Rust test file `packages/rs-drive-abci/tests/strategy_tests/main.rs`, specific protocol versions like `PROTOCOL_VERSION_1` are intentionally used in tests instead of `PROTOCOL_VERSION_LATEST`.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
🧬 Code graph analysis (4)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (5)
packages/rs-dpp/src/data_contract/config/mod.rs (1)
data_contract(121-143)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v0/mod.rs (1)
data_contract(14-67)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs (1)
data_contract(14-68)packages/rs-dpp/src/data_contract/v0/serialization/mod.rs (3)
data_contract(49-77)data_contract(79-119)data_contract(121-162)packages/rs-dpp/src/data_contract/v1/serialization/mod.rs (3)
data_contract(48-76)data_contract(78-127)data_contract(129-191)
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs (2)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (1)
config(146-151)packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/mod.rs (1)
keeps_history(181-200)
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs (3)
packages/rs-drive-abci/tests/strategy_tests/execution.rs (1)
run_chain_for_strategy(51-784)packages/rs-dpp/src/tests/json_document.rs (1)
json_document_to_created_contract(81-94)packages/rs-drive-abci/src/config.rs (1)
default_minimal_verifications(889-898)
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs (1)
packages/rs-drive/src/drive/contract/queries.rs (1)
fetch_historical_contracts_query(167-180)
⏰ 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). (21)
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp2) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (9)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (1)
145-151: LGTM! Clean accessor following established patterns.The new
config()method correctly provides access toDataContractConfigfor both V0 and V1 variants, enabling thekeeps_history()check needed for path query selection in proof generation.packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs (2)
51-57: LGTM! Helper function correctly mirrors the non-historical variant.The new
contract_ids_to_historical_path_queryhelper follows the same pattern as the existing non-historical version, correctly usingDrive::fetch_historical_contracts_queryand settinglimit = Nonefor consistency.
67-79: LGTM! Fix correctly addresses the root cause.The conditional logic properly selects the path query based on the contract's
keeps_history()configuration for bothDataContractCreateandDataContractUpdatetransitions. This directly fixes the proof verification failure described in the PR.packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rs (1)
6-6: LGTM!Module declaration correctly adds the new regression test suite.
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs (2)
401-413: LGTM! Verification correctly aligned with proof generation.The
keeps_historyflag is properly extracted and passed toDrive::verify_contract, ensuring the verification uses the same path query type (historical vs non-historical) as the proof generation.
434-446: LGTM! Consistent handling for DataContractUpdate.The update case correctly mirrors the create case, and the fixed expect message ("expected to verify contract" instead of "expected to verify full identity") is a good housekeeping fix.
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs (3)
1-12: LGTM! Excellent documentation of the regression test purpose.The module docstring clearly explains the bug, its root cause, and what these tests verify. This provides valuable context for future maintainers.
82-84: Good: Critical flag enabling proof verification is well-documented.The comment clearly explains that
verify_state_transition_results: trueis what catches the bug by enabling proof verification. This is essential for the regression test to be effective.
107-121: LGTM! Test assertions are thorough.Both tests properly:
- Run the chain with proof verification enabled
- Check all state transition result codes are 0
- Fetch contracts and verify their
keeps_historyconfigurationThis comprehensive verification ensures the fix works end-to-end.
Also applies to: 237-249
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs (3)
40-41: Consider renaming to follow "should..." convention.Per coding guidelines, test names in
**/tests/**/*.rsshould start with "should...". However, therun_chain_*pattern appears to be an established convention in strategy tests, so this is a minor alignment consideration.📝 Example naming
- fn run_chain_create_contract_with_keeps_history_true() { + fn should_create_contract_with_keeps_history_true() {
113-131: Consider usingeprintln!or a logging macro for debug output.The
println!statements provide useful debug visibility, but they will appear in normal test output (even on success). If these are meant to be permanent, consider keeping them; if they were for debugging during development, they could be removed or gated.
163-165: Same naming convention consideration.Per coding guidelines, consider renaming to follow the "should..." pattern if aligning with guidelines is desired.
📝 Example naming
- fn run_chain_create_contracts_with_and_without_history() { + fn should_create_contracts_with_and_without_history() {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
🧠 Learnings (8)
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
📚 Learning: 2024-10-04T09:08:47.901Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2206
File: packages/rs-drive-abci/tests/strategy_tests/main.rs:1162-1162
Timestamp: 2024-10-04T09:08:47.901Z
Learning: In the Rust test file `packages/rs-drive-abci/tests/strategy_tests/main.rs`, specific protocol versions like `PROTOCOL_VERSION_1` are intentionally used in tests instead of `PROTOCOL_VERSION_LATEST`.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-drive/src/prove/prove_state_transition/v0/mod.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-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use state transitions for updates in documents and data contracts
Applied to files:
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs (2)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (2)
config(146-151)id(109-114)packages/rs-drive/src/drive/contract/queries.rs (1)
fetch_historical_contracts_query(167-180)
⏰ 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). (21)
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- 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, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (8)
packages/rs-drive-abci/tests/strategy_tests/test_cases/data_contract_history_tests.rs (5)
1-12: Excellent documentation of the regression context.The module-level documentation clearly explains the bug, symptoms, and root cause, making it easy for future maintainers to understand why these tests exist.
13-29: Imports are well-organized and appropriate.All imported items are used in the test implementations. The imports correctly pull in the necessary traits for accessing and mutating data contract configuration.
59-88: Well-structured strategy configuration with clear critical flag documentation.The inline comment on lines 84-86 clearly explains why
verify_state_transition_results: trueis critical for catching the regression. This is exactly the kind of documentation that helps future maintainers understand test intent.
192-222: Good test coverage of both code paths in a single test.Testing both historical (
keeps_history=true) and non-historical (keeps_history=false) contracts together is an effective approach to verify both paths work correctly and don't interfere with each other.
255-301: Solid verification logic for both contracts.The assertions correctly verify:
- Both contracts exist in the outcome
- Historical contract has
keeps_history=true- Non-historical contract has
keeps_history=falseThis provides good regression coverage for the proof verification fix.
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs (3)
10-10: LGTM - Imports are correctly added for the new functionality.The
DataContractConfigGettersV0trait provides access tokeeps_history(), and the transition accessor traits provide access todata_contract()from the state transitions.Also applies to: 24-25
51-57: LGTM - Historical path query helper follows the established pattern.The helper correctly mirrors
contract_ids_to_non_historical_path_query(lines 43-49), usingDrive::fetch_historical_contracts_queryinstead. Settingpath_query.query.limit = Noneis consistent with the non-historical variant and allows unbounded proof queries.
67-79: Verify side is NOT passing keeps_history to verify_contract.The prove side correctly uses
keeps_history()to select the appropriate path query. However, the verification side at lines 75 and 98 passesNonefor thecontract_known_keeps_historyparameter instead of extracting and passing the actual value from the available data contract. The data contract is available at verification time (data_contract_create.data_contract()anddata_contract_update.data_contract()), so it could be passed asSome(data_contract.config().keeps_history()), but currently it's not.Likely an incorrect or invalid review comment.
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
@pauldelucia can you verify this one?
|
Still getting the same error using local network. It seems proof generation and the test harness for proof verification were fixed, but not the production proof verification :) I fixed it and it works now. I will push here once Pasta approves. |
Issue being fixed or feature implemented
When creating/updating a data contract with keeps_history=true, the proof verification would fail with "proof did not contain contract with id ...".
Root cause: In prove_state_transition_v0, the code always used non-historical path queries regardless of whether the contract had keeps_history=true.
What was done?
Fix:
How Has This Been Tested?
Includes regression tests that verify:
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.