fix(drive-abci): don't panic on unsupported version error#2679
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 Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA 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🧪 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/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
|
✅ 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: "69419d7caa3107b2a3fd05f9e34008484cbe9474b2cd406fe1f3353327c07428"
)Xcode manual integration:
|
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
I think the idea was that we should panic... @shumkov what do you think?
|
✅ Final review complete — no blockers (commit 43e65ce) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR correctly converts a node-killing panic into a returned validation error so process_proposal rejects rather than crashing. Implementation is sound; remaining items are a stale comment, a typo in the new error variant's docs, and a missing regression test for this consensus-critical path.
🟡 2 suggestion(s) | 💬 1 nitpick(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-drive-abci/src/execution/engine/run_block_proposal/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:84: Stale comment still says 'We should panic'
The comment above the `PlatformVersion::get(next_protocol_version)` block still reads `// We should panic if this node is not supported a new protocol version`, but this PR's whole purpose is to stop panicking and instead return a `ProtocolVersionNotSupported` validation error so Tenderdash can reject the proposal and try another proposer. Leaving the old comment in place is actively misleading for future readers and for anyone grepping for `panic`.
- [SUGGESTION] packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:85-103: Add regression test for unsupported protocol version rejection
The PR description states 'Not tested', but this branch is the entire behavioral guarantee being introduced and is consensus-critical: at an epoch change, an unsupported `next_epoch_protocol_version` must yield `Ok(ValidationResult::new_with_error(ExecutionError::ProtocolVersionNotSupported { .. }))` rather than panic or return `Err`. The Rust `Result` vs validation-error distinction matters here — `process_proposal` only takes the reject path through the validation-result variant; a future refactor that flips this to `Err` would still compile but reintroduce a DoS-style failure mode (or worse, a panic). A focused test that drives `run_block_proposal` past `is_epoch_change_but_not_genesis` with `next_epoch_protocol_version` set above `PlatformVersion::latest().protocol_version` and asserts both the validation error variant and (ideally) a reject response from `process_proposal` would lock the invariant in place.
| @@ -83,15 +83,23 @@ where | |||
|
|
|||
| // We should panic if this node is not supported a new protocol version | |||
There was a problem hiding this comment.
🟡 Suggestion: Stale comment still says 'We should panic'
The comment above the PlatformVersion::get(next_protocol_version) block still reads // We should panic if this node is not supported a new protocol version, but this PR's whole purpose is to stop panicking and instead return a ProtocolVersionNotSupported validation error so Tenderdash can reject the proposal and try another proposer. Leaving the old comment in place is actively misleading for future readers and for anyone grepping for panic.
| // We should panic if this node is not supported a new protocol version | |
| // If this node does not support the upgraded protocol version, do not panic: | |
| // return a validation error so process_proposal rejects the block and Tenderdash | |
| // can fall back to another proposer while the operator upgrades. | |
| let Ok(next_platform_version) = PlatformVersion::get(next_protocol_version) else { |
source: ['claude', 'codex']
| known_versions: Vec<FeatureVersion>, | ||
| }, | ||
|
|
||
| /// We don't have the required protocol version. Most liekly the platform must be upgraded. |
There was a problem hiding this comment.
💬 Nitpick: Typo 'liekly' in ProtocolVersionNotSupported doc comment
Doc comment for the new ProtocolVersionNotSupported variant contains liekly (should be likely). This renders on the public rustdoc surface of ExecutionError, so worth fixing while the variant is new.
| /// 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. |
source: ['claude', 'codex']
| let Ok(next_platform_version) = PlatformVersion::get(next_protocol_version) else { | ||
| panic!( | ||
| let max_supported = PlatformVersion::latest().protocol_version; | ||
| tracing::error!( | ||
| r#"Failed to upgrade the network protocol version {next_protocol_version}. | ||
|
|
||
| Please update your software to the latest version: https://docs.dash.org/platform-protocol-upgrade | ||
|
|
||
| Your software version: {}, latest supported protocol version: {}."#, | ||
| env!("CARGO_PKG_VERSION"), | ||
| PlatformVersion::latest().protocol_version | ||
| max_supported | ||
| ); | ||
|
|
||
| return Ok(ValidationResult::new_with_error(Error::Execution( | ||
| ExecutionError::ProtocolVersionNotSupported { | ||
| max_supported, | ||
| required: next_protocol_version, | ||
| }, | ||
| ))); | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Add regression test for unsupported protocol version rejection
The PR description states 'Not tested', but this branch is the entire behavioral guarantee being introduced and is consensus-critical: at an epoch change, an unsupported next_epoch_protocol_version must yield Ok(ValidationResult::new_with_error(ExecutionError::ProtocolVersionNotSupported { .. })) rather than panic or return Err. The Rust Result vs validation-error distinction matters here — process_proposal only takes the reject path through the validation-result variant; a future refactor that flips this to Err would still compile but reintroduce a DoS-style failure mode (or worse, a panic). A focused test that drives run_block_proposal past is_epoch_change_but_not_genesis with next_epoch_protocol_version set above PlatformVersion::latest().protocol_version and asserts both the validation error variant and (ideally) a reject response from process_proposal would lock the invariant in place.
source: ['claude', 'codex']
4 participants
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 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