feat(dpp): add max_asset_lock_transaction_inputs limit to prevent stuck funds#3491
Conversation
…ck funds Asset lock transactions with too many inputs (from many small UTXOs) can exceed Platform's 20KB state transition size limit. Core accepts the transaction but Platform rejects it, leaving funds stuck. This adds a configurable input count limit (default 100) that validates early with a clear error message advising users to consolidate UTXOs first. Closes #3399 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a new consensus error Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow consistent patterns across multiple files (error definition, integration, configuration) with moderate logic density in the validator logic. Homogeneous repetition in version configuration files and standard WASM bindings reduce complexity, though the validator logic update and its test coverage warrant careful review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Needs to be tested with dash-evo-tool |
There was a problem hiding this comment.
Pull request overview
Adds a protocol-configured cap on Asset Lock transaction input count so oversized asset-lock state transitions are rejected before Core broadcast, avoiding “stuck funds” scenarios and providing a clearer, actionable consensus error.
Changes:
- Introduces
max_asset_lock_transaction_inputs(set to100) in platform version configs (v1–v3). - Enforces the new limit during asset lock transaction structure validation and adds unit tests around the boundary.
- Wires a new consensus error (code
10534) through Rust + WASM error surfaces.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/mod.rs | Adds max_asset_lock_transaction_inputs to identity asset-lock version config struct. |
| packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v1.rs | Sets max asset-lock inputs to 100 for v1. |
| packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v2.rs | Sets max asset-lock inputs to 100 for v2. |
| packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v3.rs | Sets max asset-lock inputs to 100 for v3. |
| packages/rs-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/mod.rs | Threads platform-version limit into v0 validator call. |
| packages/rs-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/v0/mod.rs | Implements input-count guard and adds unit tests for 1/100/101 inputs. |
| packages/rs-dpp/src/errors/consensus/basic/identity/mod.rs | Exposes the new identity consensus error type. |
| packages/rs-dpp/src/errors/consensus/basic/basic_error.rs | Adds the new BasicError enum variant for the consensus error. |
| packages/rs-dpp/src/errors/consensus/codes.rs | Assigns consensus error code 10534. |
| packages/rs-dpp/src/errors/consensus/basic/identity/identity_asset_lock_transaction_too_many_inputs_error.rs | Defines the new consensus error type and message. |
| packages/wasm-dpp/src/errors/consensus/basic/identity/mod.rs | Registers the new WASM error module + re-export. |
| packages/wasm-dpp/src/errors/consensus/basic/identity/identity_asset_lock_transaction_too_many_inputs_error.rs | Adds WASM bindings/accessors for the new error type. |
| packages/wasm-dpp/src/errors/consensus/consensus_error.rs | Maps the new BasicError variant into the WASM consensus error conversion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✅ Review complete (commit 3656c69) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3491 +/- ##
============================================
- Coverage 84.84% 84.83% -0.01%
============================================
Files 2476 2476
Lines 267915 267827 -88
============================================
- Hits 227303 227206 -97
- Misses 40612 40621 +9
🚀 New features to boost your workflow:
|
- Use `u16::try_from(input_count).unwrap_or(u16::MAX)` instead of `input_count as u16` when constructing `IdentityAssetLockTransactionTooManyInputsError`, so the reported count saturates at u16::MAX rather than silently wrapping for the (unrealistic) `input_count > 65535` case. - Strengthen `should_accept_transaction_at_max_inputs` by also asserting `result.is_valid()`, matching the pattern used in `should_accept_transaction_with_one_input`. The existing absence-of-specific-error check is kept. Addresses copilot-pull-request-reviewer nits on PR #3491. Third nit (remove `use crate::errors::ProtocolError;`) declined: the derive-generated code for `PlatformSerialize`/`PlatformDeserialize` on `IdentityAssetLockTransactionTooManyInputsError` references the `ProtocolError` type, so the import is not actually unused. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| output_index: u32, | ||
| max_inputs: u16, | ||
| ) -> ConsensusValidationResult<TxOut> { | ||
| let input_count = transaction.input.len(); |
There was a problem hiding this comment.
@QuantumExplorer please check if we don't need new version of validate_asset_lock_transaction_structure/v0/mod.rs
…set-lock inputs Raise `max_asset_lock_transaction_inputs` to `u16::MAX` on v1 and v2 state-transition version constants so previously valid asset-lock transactions remain valid across upgrades. Introduce the new 100-input limit only at v3, which is the latest/target protocol version for this change. The validator (`validate_asset_lock_transaction_structure_v0`) is version-agnostic and takes `max_inputs` as a runtime argument sourced from the platform version, so no new validator version is required. Addresses self-authored review comments on PR #3491 (lklimek). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lklimek
added
the
ready for final review
![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-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/v0/mod.rs`:
- Around line 67-79: The fixture currently embeds a WIF private-key literal in
make_asset_lock_transaction (private_key_hex / PrivateKey::from_str); replace it
with a programmatically generated key like the one-time key: create a
dashcore::secp256k1::SecretKey via thread_rng(), wrap it with
PrivateKey::new(Network::Testnet) to produce private_key, derive public_key from
that private_key (as is done for one_time_private_key/public_key) and use the
resulting public key/hash instead of parsing a string; remove the hard-coded
private_key_hex and PrivateKey::from_str usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c6d7d62-f1b4-4a7e-b9a0-1bc0cb16992d
📒 Files selected for processing (13)
packages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/errors/consensus/basic/identity/identity_asset_lock_transaction_too_many_inputs_error.rspackages/rs-dpp/src/errors/consensus/basic/identity/mod.rspackages/rs-dpp/src/errors/consensus/codes.rspackages/rs-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/mod.rspackages/rs-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v3.rspackages/wasm-dpp/src/errors/consensus/basic/identity/identity_asset_lock_transaction_too_many_inputs_error.rspackages/wasm-dpp/src/errors/consensus/basic/identity/mod.rspackages/wasm-dpp/src/errors/consensus/consensus_error.rs
Replace the hardcoded testnet WIF literal in `make_asset_lock_transaction` with a programmatically generated key via `SecretKey::new(&mut rng)` + `PrivateKey::new(..., Network::Testnet)`, mirroring the existing `one_time_private_key` pattern. The key was only ever used to derive a `pubkey_hash` for `ScriptBuf::new_p2pkh`, so a random key per run is semantically equivalent and keeps secret scanners quiet. Addresses coderabbitai[bot] review comment on PR #3491. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This change adds a new asset-lock input-count guard plus the matching consensus/WASM error plumbing, but the assigned SHA still has two blocking problems. First, the new check only runs during asset-lock proof / state-transition validation, while the main JS register/top-up flow broadcasts the Core asset-lock transaction before it ever builds the proof, so the advertised stuck-funds prevention does not actually happen. Second, the 100-input cap is written directly into the existing v1/v2/v3 platform-version snapshots instead of being gated behind a new validation version, which retroactively changes historical protocol behavior.
Reviewed commit: 3656c69
🔴 2 blocking | 🟡 1 suggestion(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-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/v0/mod.rs`:
- [BLOCKING] lines 15-22: The new input-limit check still runs only after the asset lock transaction has already been broadcast
This guard rejects oversized asset-lock transactions during `validate_asset_lock_transaction_structure_v0`, but that validator is only reached from asset-lock proof / state-transition validation. In the actual JS client flow, the asset-lock transaction is still broadcast first (`packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/register.ts:27-32` and `.../topUp.ts:25-30`), and only afterwards does `createAssetLockProof()` wait for metadata / InstantLock and invoke DPP validation (`packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/internal/createAssetLockProof.ts:24-95`). So on this SHA, a 101+ input asset-lock can still be sent to Core before this error ever fires, which means the stuck-funds scenario described in the PR is still present in the main registration/top-up path. The limit needs to be enforced before `broadcastTransaction`, in the asset-lock transaction creation path, not only during proof validation.
- [SUGGESTION] lines 15-18: `input_count as u16` can wrap and report the wrong actual input count
`transaction.input.len()` is a `usize`, but the new error payload stores `actual_inputs` as `u16` via an unchecked `as` cast. For pathological transactions with at least 65536 inputs, validation will still fail, but the reported `actual_inputs` value wraps modulo 2^16 and can contradict reality when surfaced through the Rust and WASM error accessors. Saturating or checked conversion keeps the diagnostic accurate without changing the validation outcome.
In `packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v1.rs`:
- [BLOCKING] lines 21-25: The 100-input cap is retrofitted into existing protocol snapshots instead of behind a new version gate
`validate_asset_lock_transaction_structure()` still dispatches only on version `0`, so the only thing changing consensus behavior here is the new `max_asset_lock_transaction_inputs` value. This SHA writes that value directly into the existing `STATE_TRANSITION_VERSIONS_V1`, and the same pattern is repeated in `.../v2.rs` and `.../v3.rs`, which means old and new binaries running the same protocol version can now disagree about whether a high-input asset-lock transaction is valid. That is exactly the kind of historical-rule mutation the versioned dispatch system is supposed to avoid. If this limit is meant to be a new consensus rule, it needs a new validation version (or a new protocol snapshot that references one) rather than editing existing v1/v2/v3 constants in place.
| max_inputs: u16, | ||
| ) -> ConsensusValidationResult<TxOut> { | ||
| let input_count = transaction.input.len(); | ||
| if input_count > max_inputs as usize { | ||
| return ConsensusValidationResult::new_with_error( | ||
| IdentityAssetLockTransactionTooManyInputsError::new(input_count as u16, max_inputs) | ||
| .into(), | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: The new input-limit check still runs only after the asset lock transaction has already been broadcast
This guard rejects oversized asset-lock transactions during validate_asset_lock_transaction_structure_v0, but that validator is only reached from asset-lock proof / state-transition validation. In the actual JS client flow, the asset-lock transaction is still broadcast first (packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/register.ts:27-32 and .../topUp.ts:25-30), and only afterwards does createAssetLockProof() wait for metadata / InstantLock and invoke DPP validation (packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/internal/createAssetLockProof.ts:24-95). So on this SHA, a 101+ input asset-lock can still be sent to Core before this error ever fires, which means the stuck-funds scenario described in the PR is still present in the main registration/top-up path. The limit needs to be enforced before broadcastTransaction, in the asset-lock transaction creation path, not only during proof validation.
source: ['codex']
🤖 Fix this 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-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/v0/mod.rs`:
- [BLOCKING] lines 15-22: The new input-limit check still runs only after the asset lock transaction has already been broadcast
This guard rejects oversized asset-lock transactions during `validate_asset_lock_transaction_structure_v0`, but that validator is only reached from asset-lock proof / state-transition validation. In the actual JS client flow, the asset-lock transaction is still broadcast first (`packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/register.ts:27-32` and `.../topUp.ts:25-30`), and only afterwards does `createAssetLockProof()` wait for metadata / InstantLock and invoke DPP validation (`packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/internal/createAssetLockProof.ts:24-95`). So on this SHA, a 101+ input asset-lock can still be sent to Core before this error ever fires, which means the stuck-funds scenario described in the PR is still present in the main registration/top-up path. The limit needs to be enforced before `broadcastTransaction`, in the asset-lock transaction creation path, not only during proof validation.
| required_asset_lock_duff_balance_for_processing_start_for_address_funding: 50000, | ||
| validate_asset_lock_transaction_structure: 0, | ||
| validate_instant_asset_lock_proof_structure: 0, | ||
| max_asset_lock_transaction_inputs: 100, |
There was a problem hiding this comment.
🔴 Blocking: The 100-input cap is retrofitted into existing protocol snapshots instead of behind a new version gate
validate_asset_lock_transaction_structure() still dispatches only on version 0, so the only thing changing consensus behavior here is the new max_asset_lock_transaction_inputs value. This SHA writes that value directly into the existing STATE_TRANSITION_VERSIONS_V1, and the same pattern is repeated in .../v2.rs and .../v3.rs, which means old and new binaries running the same protocol version can now disagree about whether a high-input asset-lock transaction is valid. That is exactly the kind of historical-rule mutation the versioned dispatch system is supposed to avoid. If this limit is meant to be a new consensus rule, it needs a new validation version (or a new protocol snapshot that references one) rather than editing existing v1/v2/v3 constants in place.
source: ['claude', 'codex']
🤖 Fix this 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-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v1.rs`:
- [BLOCKING] lines 21-25: The 100-input cap is retrofitted into existing protocol snapshots instead of behind a new version gate
`validate_asset_lock_transaction_structure()` still dispatches only on version `0`, so the only thing changing consensus behavior here is the new `max_asset_lock_transaction_inputs` value. This SHA writes that value directly into the existing `STATE_TRANSITION_VERSIONS_V1`, and the same pattern is repeated in `.../v2.rs` and `.../v3.rs`, which means old and new binaries running the same protocol version can now disagree about whether a high-input asset-lock transaction is valid. That is exactly the kind of historical-rule mutation the versioned dispatch system is supposed to avoid. If this limit is meant to be a new consensus rule, it needs a new validation version (or a new protocol snapshot that references one) rather than editing existing v1/v2/v3 constants in place.
| max_inputs: u16, | ||
| ) -> ConsensusValidationResult<TxOut> { | ||
| let input_count = transaction.input.len(); | ||
| if input_count > max_inputs as usize { |
There was a problem hiding this comment.
🟡 Suggestion: input_count as u16 can wrap and report the wrong actual input count
transaction.input.len() is a usize, but the new error payload stores actual_inputs as u16 via an unchecked as cast. For pathological transactions with at least 65536 inputs, validation will still fail, but the reported actual_inputs value wraps modulo 2^16 and can contradict reality when surfaced through the Rust and WASM error accessors. Saturating or checked conversion keeps the diagnostic accurate without changing the validation outcome.
💡 Suggested change
| max_inputs: u16, | |
| ) -> ConsensusValidationResult<TxOut> { | |
| let input_count = transaction.input.len(); | |
| if input_count > max_inputs as usize { | |
| let input_count = transaction.input.len(); | |
| if input_count > max_inputs as usize { | |
| let actual_inputs = u16::try_from(input_count).unwrap_or(u16::MAX); | |
| return ConsensusValidationResult::new_with_error( | |
| IdentityAssetLockTransactionTooManyInputsError::new(actual_inputs, max_inputs) | |
| .into(), | |
| ); | |
| } |
source: ['claude']
🤖 Fix this 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-dpp/src/identity/state_transition/asset_lock_proof/validate_asset_lock_transaction_structure/v0/mod.rs`:
- [SUGGESTION] lines 15-18: `input_count as u16` can wrap and report the wrong actual input count
`transaction.input.len()` is a `usize`, but the new error payload stores `actual_inputs` as `u16` via an unchecked `as` cast. For pathological transactions with at least 65536 inputs, validation will still fail, but the reported `actual_inputs` value wraps modulo 2^16 and can contradict reality when surfaced through the Rust and WASM error accessors. Saturating or checked conversion keeps the diagnostic accurate without changing the validation outcome.
Issue being fixed or feature implemented
Fixes #3399
Imagine you are a user funding a Platform address with tDASH accumulated from many small mining rewards. You send 400 tDASH, the Core transaction succeeds — but Platform rejects the state transition with a cryptic "Tx too large" error. Your funds are stuck: gone from Core wallet, never credited to Platform. This change catches the problem before the Core broadcast, with a clear error message telling you to consolidate UTXOs first.
What was done?
Added a
max_asset_lock_transaction_inputslimit (100) to prevent asset lock transactions from exceeding Platform's 20KBmax_state_transition_sizelimit.Size math: Each input adds ~184 bytes to the state transition (148 bytes for the transaction input + 36 bytes for the InstantLock outpoint). At 100 inputs the total is ~18,877 bytes — fits within the 20,480 byte limit with ~1,600 bytes (~8%) margin.
Changes across 3 packages:
rs-platform-version:max_asset_lock_transaction_inputs: u16toIdentityTransitionAssetLockVersions100in all version configs (v1, v2, v3)rs-dpp:IdentityAssetLockTransactionTooManyInputsError(code 10534) with actionable message: "Consolidate UTXOs before creating the asset lock"validate_asset_lock_transaction_structure_v0wasm-dpp:getMaxInputs(),getActualInputs(),getCode(), andmessageaccessorsHow Has This Been Tested?
cargo check -p dpp --features=validation— cleancargo check -p dash-sdk— cleancargo check -p drive-abci— cleancargo test -p dpp --features=validation -- asset_lock_transaction— 5/5 pass (3 new + 2 existing)Breaking Changes
None. The new limit (100 inputs) is well above typical transaction sizes. Only transactions that would have been rejected by Platform's existing 20KB state transition size limit are now caught earlier with a better error message.
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit