fix(drive-abci): validate state on check tx for masternode voting #2861
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThe changes introduce a trait method to signal whether full state validation should occur during check_tx operations. The check_tx verification function now conditionally validates state transitions when the flag is enabled, performing state validation and returning errors if invalid. MasternodeVoteTransition implements this method to return true. Changes
Sequence DiagramsequenceDiagram
participant CheckTx as check_tx_verification
participant Transition as StateTransition
participant Validator as state_transition.validate_state()
CheckTx->>Transition: validates_full_state_on_check_tx()?
alt Full state validation enabled
Transition-->>CheckTx: true
CheckTx->>Validator: validate_state(...)
alt Validation succeeds
Validator-->>CheckTx: validated_action
CheckTx-->>CheckTx: use validated_action
else Validation fails
Validator-->>CheckTx: Error
CheckTx-->>CheckTx: return error
end
else Full state validation disabled
Transition-->>CheckTx: false
CheckTx-->>CheckTx: proceed without validation
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs (1)
188-209: Full-state-on-CheckTx logic looks correct but needlessly drops the earlieractionThe new block correctly:
- Uses the previously computed
action(fromrequires_advanced_structure_validation_with_state_on_check_tx) when callingvalidate_state, and- Restricts full state validation to transitions that explicitly opt in via
validates_full_state_on_check_tx()(currently masternode vote).However, for transitions that don’t opt in (e.g., Batch):
- You still execute the earlier
transform_into_action+validate_advanced_structure_from_stateblock.- Then this new block sets
actiontoNone, shadowing and discarding the priorSome(action).- The code later recomputes
transform_into_actionto rebuild theactionfor the execution event.So behavior is correct, but
Batchgetstransform_into_actionexecuted twice in FirstTimeCheck, which is unnecessary overhead for larger batches.A small refactor would avoid double work and keep the flow clearer, for example by:
- Making the first
actionbinding mutable, and- Updating it in-place when
validates_full_state_on_check_tx()is true, instead of shadowing it with a freshlet.Conceptually:
- let action = if state_transition.requires_advanced_structure_validation_with_state_on_check_tx() { + let mut action = if state_transition.requires_advanced_structure_validation_with_state_on_check_tx() { // transform_into_action + validate_advanced_structure_from_state Some(action) } else { None }; - let action = if state_transition.validates_full_state_on_check_tx() { + if state_transition.validates_full_state_on_check_tx() { let result = state_transition.validate_state( - action, + action, platform, ValidationMode::CheckTx, platform.state.last_block_info(), &mut state_transition_execution_context, None, )?; if !result.is_valid() { return Ok(ConsensusValidationResult::<Option<ExecutionEvent>>::new_with_errors( result.errors, )); } - result.data - } else { - None - }; + action = result.data; + }Then the
let action = if let Some(action) = action { .. } else { .. }below can operate directly on this singleactionvariable.This keeps masternode voting behavior unchanged while avoiding duplicate transformation work for other transitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:83-85
Timestamp: 2024-10-24T05:00:56.657Z
Learning: An upcoming filter will be introduced to the `unconfirmed_txs` endpoint in `broadcastStateTransitionHandlerFactory.js`, as noted in the TODO comment. Therefore, optimizing transaction existence checks may not be necessary at this time.
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 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/src/execution/validation/state_transition/processor/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.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-abci/src/execution/validation/state_transition/processor/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.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/src/execution/validation/state_transition/processor/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/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-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.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/src/execution/validation/state_transition/check_tx_verification/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/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs
📚 Learning: 2024-10-09T00:22:57.778Z
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-09T00:22:57.778Z
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-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs
🧬 Code graph analysis (3)
packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs (1)
validates_full_state_on_check_tx(83-85)
packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs (1)
packages/rs-dpp/src/validation/validation_result.rs (1)
new_with_errors(111-113)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs (1)
packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs (1)
validates_full_state_on_check_tx(517-519)
⏰ 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). (8)
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / 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 (3)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs (1)
83-85: Enable full-state CheckTx for masternode votes — LGTMOverride is correct and uniquely scoped to MasternodeVoteTransition. Verified:
- Flag is properly consulted at
check_tx_verification/v0/mod.rs:188-204andvalidate_stateis called conditionally when true- No other
StateTransitionStateValidationV0implementations return true; all others inherit the defaultfalse- Wiring is sound and isolated
packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs (1)
486-520: Newvalidates_full_state_on_check_txflag is well-scoped and safeAdding this predicate with a default
falsekeeps existing transitions unchanged and lets specific transitions (like masternode vote) opt in to full state validation during CheckTx. The API shape and default are reasonable, and the doc comment accurately reflects the current intended usage.No changes needed here.
packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs (1)
18-23: Import and documentation changes are consistent with new behaviorBringing
StateTransitionStateValidationV0into scope and documenting the CheckTx helper’s versioning semantics align with the new full-state-on-check-tx path. No issues here.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "3e0e32834091e11cdfb0251aa2ac04cc544aed49972b2a30fc8755e90ca377e5"
)Xcode manual integration:
|
QuantumExplorer
changed the title
shumkov
approved these changes
Nov 18, 2025
Contributor
|
Closes #2769 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes