fix(drive): uncommitted state if db transaction fails #2305
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request primarily focus on enhancing the transaction handling logic across several functions in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
packages/rs-drive-abci/src/abci/handler/info.rs (3)
50-50: Address the TODO: Verify that the chain ID isevo1.There's a pending TODO comment to verify that the chain ID is
evo1. Implementing this verification is important to ensure that the exception for block32326only applies to the mainnet and does not affect other networks.Would you like assistance in implementing this chain ID verification?
51-63: Refactor conditional logic for clarity.The negated condition in the
ifstatement can be simplified to enhance readability. By restructuring the condition, the logic becomes more straightforward and maintainable.Apply this diff to simplify the condition:
#[allow(clippy::collapsible_if)] -if !(app.platform().config.network == Network::Dash && last_block_height == 32326) { +if app.platform().config.network != Network::Dash || last_block_height != 32326 { // App hash in memory must be equal to app hash on disk if drive_storage_root_hash != platform_state_app_hash { // We panic because we can't recover from this situation. // Better to restart the Drive, so we might self-heal the node - // reloading state form the disk + // reloading state from the disk panic!( "drive and platform state app hash mismatch: drive_storage_root_hash: {:?}, platform_state_app_hash: {:?}", drive_storage_root_hash, platform_state_app_hash ); } }This change directly checks when the network is not Dash or the last block height is not
32326, making the condition clearer.
57-58: Fix typographical error in the comment.In the comment, "reloading state form the disk" should be "reloading state from the disk".
Apply this diff to correct the typo:
// Better to restart the Drive, so we might self-heal the node -// reloading state form the disk +// reloading state from the disk panic!( "drive and platform state app hash mismatch: drive_storage_root_hash: {:?}, platform_state_app_hash: {:?}", drive_storage_root_hash, platform_state_app_hash );packages/rs-drive-abci/src/abci/handler/finalize_block.rs (2)
75-75: Reminder: Address the TODO commentThe TODO indicates the need to verify that the chain ID is
evo1. Implementing this check is important to ensure the conditional logic applies only to the intended network, preventing unintended behavior on other networks.Would you like assistance in implementing the chain ID verification?
76-76: Avoid hardcoding block height and network valuesTo improve maintainability and flexibility, consider defining constants or configuration parameters for the block height
32326and the networkNetwork::Dashinstead of hardcoding them directly in the conditional statement.packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs (1)
39-39: Typo in comment: 'commited' should be 'committed'Please correct the spelling in the comment: 'commited' should be 'committed'.
packages/rs-drive-abci/src/abci/handler/process_proposal.rs (2)
208-208: Simplify the conditional statement for better readabilityThe current condition uses a negation with
!(...), which can be harder to read. Consider inverting the logic to make it more straightforward and improve code clarity.Apply this diff:
- if !(app.platform().config.network == Network::Dash && request.height == 32327) { + if app.platform().config.network != Network::Dash || request.height != 32327 {
208-208: Use a named constant for the exceptional block heightHardcoding the block height
32327can affect maintainability and readability. Define it as a named constant to make the code clearer and to facilitate any future changes.Apply this diff:
+ const EXCEPTION_BLOCK_HEIGHT: i64 = 32327; - if !(app.platform().config.network == Network::Dash && request.height == 32327) { + if app.platform().config.network != Network::Dash || request.height != EXCEPTION_BLOCK_HEIGHT {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/rs-drive-abci/src/abci/handler/finalize_block.rs(2 hunks)packages/rs-drive-abci/src/abci/handler/info.rs(2 hunks)packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs(2 hunks)packages/rs-drive-abci/src/abci/handler/process_proposal.rs(2 hunks)packages/rs-drive-abci/src/config.rs(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-drive-abci/src/config.rs
🔇 Additional comments (2)
packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs (1)
64-75:
Inconsistency in block height handling
The code applies an exception for request.height == 32327, but according to the PR objectives, the chain halt occurred at block 32326, and the exception should be made for that block. Please verify the correct block height to ensure consistency between the code and the PR objectives.
Run the following script to verify references to block heights 32326 and 32327 in the codebase:
packages/rs-drive-abci/src/abci/handler/process_proposal.rs (1)
15-15: Import Network module for conditional checks
The addition of use dpp::dashcore::Network; is necessary for the network-specific conditional logic introduced later in the code.
…ommit' into fix/drive/patch-for-rocksdb-tx-commit # Conflicts: # packages/rs-drive-abci/src/abci/handler/finalize_block.rs # packages/rs-drive-abci/src/abci/handler/info.rs # packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs # packages/rs-drive-abci/src/abci/handler/process_proposal.rs
shumkov
dismissed
QuantumExplorer’s stale review
The base branch was changed.
Issue being fixed or feature implemented
We had a sequence of errors on the mainnet started since block 32326. We got RocksDB's "transaction is busy" error because of a bug (#2309). Due to another bug in Tenderdash (dashpay/tenderdash#966), validators just proceeded to the next block partially committing the state and updating the cache. Full nodes are stuck and proceeded after re-sync.
What was done?
For the mainnet chain, we enable these fixes at the block when we consider the state is consistent.
How Has This Been Tested?
Syncing testnet and mainnet
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
PlatformConfigstructure by removing theinitial_protocol_versionfield and related methods.Documentation