fix(drive-abci): don't panic on unsupported version error #2679
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
WalkthroughA new error variant, Changes
Sequence Diagram(s)sequenceDiagram
participant Platform
participant Logger
participant Caller
Caller->>Platform: run_block_proposal(next_protocol_version)
Platform->>Platform: Check if next_protocol_version is supported
alt Supported
Platform-->>Caller: Proceed with proposal execution
else Not Supported
Platform->>Logger: Log error with version details
Platform-->>Caller: Return ValidationResult with ProtocolVersionNotSupported error
end
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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/error/execution.rs (1)
55-63: Fix typo in comment and approve the new error variant.The new
ProtocolVersionNotSupportederror variant is well-designed with clear field names and a helpful error message that guides users to upgrade. However, there's a typo in the comment.- /// We don't have the required protocol version. Most liekly the platform must be upgraded. + /// We don't have the required protocol version. Most likely the platform must be upgraded.packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1)
84-84: Update comment to reflect the new non-panic behavior.The comment states "We should panic if this node is not supported a new protocol version" but the code now returns an error instead of panicking. Update the comment to match the current implementation.
- // We should panic if this node is not supported a new protocol version + // We should return an error if this node does not support the new protocol version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/error/execution.rs(1 hunks)packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (3)
Learnt from: QuantumExplorer
PR: dashpay/platform#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.
Learnt from: QuantumExplorer
PR: dashpay/platform#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.
Learnt from: QuantumExplorer
PR: dashpay/platform#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.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper 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/engine/run_block_proposal/mod.rs (1)
86-102: Excellent graceful error handling implementation.This change successfully addresses the core issue by:
- Preventing network-wide panics: Instead of panicking when an unsupported protocol version is encountered, the code now returns a controlled error
- Providing comprehensive logging: The error message includes software version and upgrade instructions
- Enabling proper upstream handling: Returns a
ValidationResultthat allows callers to handle the error appropriately- Security improvement: Prevents misbehaving proposers from causing network-wide crashes
The implementation correctly constructs the
ProtocolVersionNotSupportederror with the required parameters and maintains consistency with the function's return type.
lklimek
changed the title
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
When Drive does not support new version during upgrade, it panics.
It means that some misbehaving proposer can propose a block that will panic the whole network.
Correct behavior is to reject such block proposal, and let Tenderdash handle the issue (eg. get proposal from another node).
What was done?
Changed
panicto error, and added trace::error to display previous message to the user.How Has This Been Tested?
Not tested
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit