Skip to content

fix(dpp): broken chain lock proof deserialization#2950

Merged
shumkov merged 2 commits into
v3.0-devfrom
test/mainnet-st-6CDCC15AC4EC68DBB414EE0DA692DFE363A996A0F285423BEFC3A29F87948A0D
Jan 6, 2026
Merged

fix(dpp): broken chain lock proof deserialization#2950
shumkov merged 2 commits into
v3.0-devfrom
test/mainnet-st-6CDCC15AC4EC68DBB414EE0DA692DFE363A996A0F285423BEFC3A29F87948A0D

Conversation

@shumkov

@shumkov shumkov commented Jan 6, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

in #2850 we broke chain lock proof serialization

What was done?

  • Removed unnecessary buggy code.

How Has This Been Tested?

  • Test the deserialization using transaction from mainnet. Thanks @lklimek
  • Updated tests

Breaking Changes

None

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • Refactor
    • Updated asset lock proof serialization format to use a structured "txid:vout" representation in JSON output instead of base64-encoded format.
    • Modified OutPoint object representation to include separate txid and vout fields for improved consistency and clarity.

✏️ Tip: You can customize this high-level summary in your review settings.

@shumkov shumkov self-assigned this Jan 6, 2026
@shumkov shumkov added this to the v3.0.0 milestone Jan 6, 2026
@shumkov shumkov moved this to In review / testing in Platform team Jan 6, 2026
@coderabbitai

coderabbitai Bot commented Jan 6, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The PR removes custom OutPoint serialization logic from the Rust DPP library and updates test expectations across Rust and WASM bindings to reflect the default serialization format, which represents OutPoint as a structured object with txid and vout fields.

Changes

Cohort / File(s) Summary
Rust DPP Core Serialization
packages/rs-dpp/src/identity/state_transition/asset_lock_proof/chain/chain_asset_lock_proof.rs
Removed private outpoint_serde module providing custom (de)serialization logic; removed serde(with = "outpoint_serde") attribute from ChainAssetLockProof.out_point, causing OutPoint to use default serialization instead.
Rust DPP Tests
packages/rs-dpp/src/state_transition/serialization.rs
Added new test should_build_identity_create_transition_from_mainnet_asset_lock_transaction that deserializes a base64-encoded mainnet transaction, validates state transition hash, verifies chain asset lock proof references correct core transaction hash, and validates derived identity address.
WASM DPP Unit Tests
packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs, packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs
Updated test expectations to reflect OutPoint as a structured object with txid (Uint8Array) and vout (number) fields in object representation; updated JSON format expectations to "txid:vout" string pattern; revised fromObject inputs and assertion validations accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 No more custom tricks for OutPoint's dance,
Default serialization takes its chance—
Structured fields now clearly shine,
txid and vout align just fine,
Simpler paths where proofs divine! 🏆

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(dpp): broken chain lock proof deserialization' directly and clearly describes the main change—addressing a regression that broke chain lock proof deserialization by removing buggy custom serialization code.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f426b0 and 711240e.

📒 Files selected for processing (4)
  • packages/rs-dpp/src/identity/state_transition/asset_lock_proof/chain/chain_asset_lock_proof.rs
  • packages/rs-dpp/src/state_transition/serialization.rs
  • packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs
  • packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs
💤 Files with no reviewable changes (1)
  • packages/rs-dpp/src/identity/state_transition/asset_lock_proof/chain/chain_asset_lock_proof.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Rust code must pass cargo clippy --workspace linter checks
Rust code must be formatted using cargo 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/state_transition/serialization.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
📚 Learning: 2024-12-10T10:56:26.215Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2030
File: packages/rs-sdk/tests/vectors/test_asset_lock_proof/quorum_pubkey-100-4ce7fd81273c2b394c0f32367374fc5b09ba912e017aacb366d2171e9ca6f9d5.json:1-1
Timestamp: 2024-12-10T10:56:26.215Z
Learning: In the `packages/rs-sdk/tests/vectors/test_asset_lock_proof/` directory, files with the `.json` extension are mock data that may not follow standard JSON format; this is intentional and acceptable within the project's testing framework.

Applied to files:

  • packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs
  • packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.

Applied to files:

  • packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs
📚 Learning: 2025-09-03T19:33:21.688Z
Learnt from: thephez
Repo: dashpay/platform PR: 2754
File: packages/wasm-sdk/api-definitions.json:1285-1285
Timestamp: 2025-09-03T19:33:21.688Z
Learning: In packages/wasm-sdk/api-definitions.json, thephez prefers to keep the existing "ripemd160hash20bytes1234" placeholder for ECDSA_HASH160 data field in documentation examples rather than using a valid base64-encoded format, maintaining consistency with the previous documentation approach.

Applied to files:

  • packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs
📚 Learning: 2025-12-29T10:53:03.792Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2868
File: packages/wasm-sdk/tests/functional/addresses.spec.mjs:75-95
Timestamp: 2025-12-29T10:53:03.792Z
Learning: In this repository, tests use the dirty-chai plugin for Chai. Write assertions using the function-call style (e.g., expect(...).to.be.ok()) instead of the property-based style. Ensure dirty-chai is loaded in the test setup and documented so new tests conform to this pattern.

Applied to files:

  • packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs
  • packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs
📚 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-dpp/src/state_transition/serialization.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-dpp/src/state_transition/serialization.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-09T00:22:57.778Z
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-dpp/src/state_transition/serialization.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-dpp/src/state_transition/serialization.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.

Applied to files:

  • packages/rs-dpp/src/state_transition/serialization.rs
📚 Learning: 2024-10-30T11:07:18.462Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/sync.rs:0-0
Timestamp: 2024-10-30T11:07:18.462Z
Learning: The `test_case` crate is used in the project, and the `#[test_case::test_matrix(...)]` attribute is valid for parameterized testing in Rust.

Applied to files:

  • packages/rs-dpp/src/state_transition/serialization.rs
📚 Learning: 2024-11-25T01:17:02.001Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (`packages/rs-drive/tests/query_tests.rs`), when code is used only in tests, defining explicit enums for fields (like the `status` field in the `Withdrawal` struct) may not be necessary; using primitive types is acceptable.

Applied to files:

  • packages/rs-dpp/src/state_transition/serialization.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-dpp/src/state_transition/serialization.rs
📚 Learning: 2024-10-06T16:11:34.946Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs:19-30
Timestamp: 2024-10-06T16:11:34.946Z
Learning: In the Rust file `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs`, within the `create_owner_identity_v1` function, the `add_public_keys` method of the `Identity` struct cannot fail and does not require explicit error handling.

Applied to files:

  • packages/rs-dpp/src/state_transition/serialization.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2182
File: packages/strategy-tests/src/transitions.rs:182-247
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In test code, input validation in functions like `instant_asset_lock_proof_transaction_fixture_with_dynamic_amount` is not required.

Applied to files:

  • packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs
🧬 Code graph analysis (2)
packages/rs-dpp/src/state_transition/serialization.rs (3)
packages/rs-dpp/src/state_transition/mod.rs (1)
  • transaction_id (619-623)
packages/rs-dapi-client/src/mock.rs (1)
  • encode_hex_upper (198-200)
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
  • Chain (5807-5853)
packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs (1)
packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs (10)
  • outpoint (13-13)
  • outpoint (44-44)
  • outpoint (63-63)
  • outpoint (84-84)
  • outpoint (119-119)
  • outpoint (145-145)
  • outpoint (162-162)
  • outpoint (173-173)
  • chainlock (14-14)
  • wasm (4-4)
⏰ 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). (18)
  • GitHub Check: Rust packages (wasm-dpp2) / Unused dependencies
  • GitHub Check: Rust packages (drive) / Tests
  • GitHub Check: Rust packages (dash-sdk) / Tests
  • GitHub Check: Rust packages (wasm-dpp2) / Linting
  • GitHub Check: Rust packages (dpp) / Check each feature
  • GitHub Check: Rust packages (wasm-dpp2) / Tests
  • GitHub Check: Rust packages (dpp) / Tests
  • GitHub Check: Rust packages (drive-abci) / Tests
  • GitHub Check: Rust packages (dpp) / Unused dependencies
  • GitHub Check: Rust packages (dpp) / Linting
  • GitHub Check: Rust packages (wasm-sdk) / Tests
  • GitHub Check: Rust packages (wasm-dpp) / Tests
  • GitHub Check: Rust packages (rs-sdk-ffi) / Tests
  • 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: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
  • GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
  • GitHub Check: Rust crates security audit
🔇 Additional comments (5)
packages/rs-dpp/src/state_transition/serialization.rs (2)

64-110: Excellent integration test for mainnet transaction deserialization.

This test comprehensively validates the chain lock proof deserialization fix using a real mainnet transaction. It properly verifies:

  • State transition deserialization and hash matching
  • ChainAssetLockProof extraction
  • Out-point reference validation
  • Identity address derivation

The test directly addresses the regression mentioned in the PR objectives and provides strong evidence that the fix works correctly.


57-57: Remove the unused TransactionPayload import.

The import at line 57 is not referenced anywhere in the file and should be removed to keep imports clean.

⛔ Skipped due to learnings
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.
packages/wasm-dpp2/tests/unit/AssetLockProof.spec.mjs (1)

153-156: LGTM! Test expectations correctly updated for new outPoint shape.

The test now properly expects outPoint to be serialized as an object with txid and vout fields, consistent with the removal of custom OutPoint serialization and the move to default serde behavior.

packages/wasm-dpp2/tests/unit/ChainLockProof.spec.mjs (2)

20-35: LGTM! Correctly updated for new outPoint object format.

The test properly handles the new serialization format where outPoint is an object with txid (Uint8Array in non-human-readable serde) and vout fields. The validation checks ensure the round-trip deserialization works correctly.


43-54: LGTM! Comprehensive validation of outPoint serialization formats.

The test correctly validates:

  • toObject() returns outPoint as an object with txid and vout fields
  • toJSON() returns outPoint as a string in "txid:vout" format
  • Round-trip serialization preserves the outPoint data correctly

This ensures both human-readable and non-human-readable serialization formats work as expected.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@shumkov shumkov changed the title fix: broken chain lock proof deserialization fix(dpp): broken chain lock proof deserialization Jan 6, 2026
@shumkov
shumkov merged commit 02610f5 into v3.0-dev Jan 6, 2026
85 of 88 checks passed
@shumkov
shumkov deleted the test/mainnet-st-6CDCC15AC4EC68DBB414EE0DA692DFE363A996A0F285423BEFC3A29F87948A0D branch January 6, 2026 14:18
@github-project-automation github-project-automation Bot moved this from In review / testing to Done in Platform team Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants