fix(drive-abci): verify apphash in finalize_block #2878
Conversation
WalkthroughThe pull request adds app_hash validation to the block state information matching process. A new app_hash parameter is introduced to block finalization validation, with conversion handling and comparison against stored app_hash values throughout the execution pipeline. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![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 (2)
packages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rs (1)
95-105: App hash validation logic looks correct; simplify Option checks for clarityThe extended
matches_expected_block_infoimplementation does what you want: it convertsapp_hashwith an appropriateBadRequestDataSizeerror, includes it in tracing, and requires bothself.block_hashandself.app_hashto be present and equal to the received values.Two minor clarity nits:
- Redundant
is_some()+unwrap_or_default()patternYou currently have:
Ok(self.height == height && self.round == round && self.core_chain_locked_height == core_block_height && self.proposer_pro_tx_hash == proposer_pro_tx_hash && self.block_hash.is_some() && self.block_hash.unwrap() == received_hash && self.app_hash.is_some() && self.app_hash.unwrap_or_default() == received_app_hash)You can avoid the
is_some()+unwrap*combo and make the intent “Some and equal” explicit withmap_or:- Ok(self.height == height - && self.round == round - && self.core_chain_locked_height == core_block_height - && self.proposer_pro_tx_hash == proposer_pro_tx_hash - && self.block_hash.is_some() - && self.block_hash.unwrap() == received_hash - && self.app_hash.is_some() - && self.app_hash.unwrap_or_default() == received_app_hash) + Ok(self.height == height + && self.round == round + && self.core_chain_locked_height == core_block_height + && self.proposer_pro_tx_hash == proposer_pro_tx_hash + && self + .block_hash + .map_or(false, |hash| hash == received_hash) + && self + .app_hash + .map_or(false, |hash| hash == received_app_hash))This keeps the semantics (must be
Someand equal) while avoidingunwrapand making the Option handling a bit more idiomatic.
- Doc comment drift (optional)
The doc comment still says “Does this match a height and round?”, but the method now validates height, round, core height, proposer, block hash, and app hash. Consider updating the comment to reflect the full set of invariants when convenient.
Also applies to: 147-187
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs (1)
100-122: Align expected app_hash logging with block_hash and avoid ambiguous default valueIncluding
block_header.app_hashinmatches_expected_block_infoand in the error message is the right move.For the expected side of the error message you currently do:
block_state_info.block_hash().map(hex::encode).unwrap_or("None".to_string()), hex::encode(block_state_info.app_hash().unwrap_or_default()),If
block_state_info.app_hash()isNone, this will log an all‑zero app_hash, which is ambiguous and inconsistent with how you formatblock_hash.You can make this clearer and symmetric with
block_hash:- block_state_info.block_hash().map(hex::encode).unwrap_or("None".to_string()), - hex::encode(block_state_info.app_hash().unwrap_or_default()), + block_state_info + .block_hash() + .map(hex::encode) + .unwrap_or("None".to_string()), + block_state_info + .app_hash() + .map(hex::encode) + .unwrap_or("None".to_string()),Optionally, you might also tweak the nearby comment (“check the basics, height, round and hash”) to mention app_hash now that it’s part of the basic invariants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/types/block_state_info/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/src/execution/types/block_state_info/mod.rspackages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs
🧠 Learnings (9)
📓 Common 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.
📚 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/src/execution/types/block_state_info/mod.rspackages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/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/types/block_state_info/mod.rspackages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/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/types/block_state_info/mod.rspackages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rs
📚 Learning: 2024-10-06T16:17:34.571Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:105-105
Timestamp: 2024-10-06T16:17:34.571Z
Learning: In `run_block_proposal` in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, when retrieving `last_block_time_ms`, it's acceptable to use `platform_state` instead of `block_platform_state`, even after updating the protocol version.
Applied to files:
packages/rs-drive-abci/src/execution/types/block_state_info/mod.rspackages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/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/types/block_state_info/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/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/src/execution/types/block_state_info/v0/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/types/block_state_info/v0/mod.rs
📚 Learning: 2024-10-18T15:43:32.447Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-sdk/src/sdk.rs:654-658
Timestamp: 2024-10-18T15:43:32.447Z
Learning: In `packages/rs-sdk/src/sdk.rs`, within the `verify_metadata_height` function, when using `compare_exchange` on `last_seen_height`, it's acceptable to use `Ordering::Relaxed` for the failure ordering, as any inconsistency would only trigger an extra loop iteration without affecting correctness.
Applied to files:
packages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rs
🧬 Code graph analysis (3)
packages/rs-drive-abci/src/execution/types/block_state_info/mod.rs (1)
packages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rs (2)
app_hash(214-214)app_hash(273-275)
packages/rs-drive-abci/src/execution/types/block_state_info/v0/mod.rs (1)
packages/rs-drive-abci/src/execution/types/block_state_info/mod.rs (4)
app_hash(63-67)height(21-25)round(27-31)proposer_pro_tx_hash(45-49)
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs (3)
packages/rs-drive-abci/src/execution/types/block_state_info/mod.rs (5)
height(21-25)round(27-31)app_hash(63-67)core_chain_locked_height(51-55)block_hash(57-61)packages/rs-dpp/src/block/extended_block_info/mod.rs (2)
round(93-97)app_hash(63-67)packages/rs-dpp/src/block/extended_block_info/v0/mod.rs (4)
round(51-51)round(105-107)app_hash(38-38)app_hash(85-87)
⏰ 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). (7)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- 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
🔇 Additional comments (1)
packages/rs-drive-abci/src/execution/types/block_state_info/mod.rs (1)
164-181: App hash is correctly threaded through the BlockStateInfo wrapperThe new
app_hash: Iparameter is cleanly added tomatches_expected_block_infoand correctly forwarded to the v0 implementation. As long as bothhashandapp_hashuse the same concrete type at call sites (which appears to be the case in this PR), this change is sound and keeps the versioned enum API aligned with v0.
|
✅ 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: "a15a391b4986ffdb7cb1abc7ea345174193704819d68d6b4a9d6569cb0175a22"
)Xcode manual integration:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs (1)
110-123: Consider error message clarity for missing app_hash.The error message enhancement is helpful for debugging. However, on line 120, using
unwrap_or_default()will display all zeros ("000...") when the stored app_hash isNone, which could be confused with an actual all-zeros app_hash. Consider using a more explicit representation like"None"for missing values to improve clarity.Apply this diff to make the error message more explicit:
- hex::encode(block_state_info.app_hash().unwrap_or_default()), + block_state_info.app_hash().map(hex::encode).unwrap_or("None".to_string()),This matches the pattern already used for
block_hashon line 119.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/src/execution/engine/finalize_block_proposal/v0/mod.rs
🧠 Learnings (6)
📓 Common 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.
📚 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/src/execution/engine/finalize_block_proposal/v0/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/engine/finalize_block_proposal/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/engine/finalize_block_proposal/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/engine/run_block_proposal/mod.rs:105-105
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In `run_block_proposal` in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, when retrieving `last_block_time_ms`, it's acceptable to use `platform_state` instead of `block_platform_state`, even after updating the protocol version.
Applied to files:
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/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/src/execution/engine/finalize_block_proposal/v0/mod.rs
⏰ 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). (7)
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- 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 JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (2)
packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs (2)
101-108: LGTM! App hash validation correctly integrated.The addition of
block_header.app_hashas a parameter tomatches_expected_block_infoaligns with the PR objective of preventing corrupted data storage. The error handling via the?operator appropriately propagates any conversion errors from the underlying type conversion.
217-226: LGTM! Validated app hash correctly propagated.The
ExtendedBlockInfoV0construction correctly uses the validatedblock_header.app_hash, ensuring that only verified app hashes are committed to the extended block info.
Issue being fixed or feature implemented
Drive-ABCI does not verify apphash in FinalizeBlock, what can lead in some edge cases to corrupted data storage that is detected at next height. This means a node may get stuck, and need full platform reset.
What was done?
Added verification.
How Has This Been Tested?
GHA passing.
Mainnet fullnode resync (in progress).
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.