fix: replace panic with error handling in get_data_from_inherent_data (PM-21799)#1234
Open
fix: replace panic with error handling in get_data_from_inherent_data (PM-21799)#1234
Conversation
Made-with: Cursor
09cd34b to
9857de0
Compare
…rom_inherent_data Replace the panicking .expect() in get_data_from_inherent_data with typed error propagation via Result<Option<...>, InherentError>, matching the established pattern in federated-authority-observation. Changes: - Add DecodeFailed variant to InherentError in primitives - Change get_data_from_inherent_data to return Result with .map_err() - Update create_inherent to log error and return None on decode failure - Update check_inherent to propagate DecodeFailed via ? - Update is_inherent_required to propagate DecodeFailed via ? - Add tests for malformed inherent data across all three callers This prevents all validators from panicking simultaneously on malformed inherent data, which would permanently halt the chain. Closes: PM-21799 Made-with: Cursor
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
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.
Summary
Replace
.expect()on inherent data decoding inget_data_from_inherent_datawith typedResult<Option<...>, InherentError>error handling, preventing simultaneous validator panics on malformed inherent data.🎫 PM-21799 📐 Engineering
Motivation
The
get_data_from_inherent_datafunction in thecnight-observationpallet decodes inherent data on every block through three consensus-critical entry points:create_inherent,check_inherent, andis_inherent_required. The function uses.expect()on the decode result, which means any malformed inherent data — from serialization bugs, version mismatches, or data corruption — causes all validators to panic simultaneously, permanently halting the chain with no automatic recovery.This fix adopts the error handling pattern already established in the sibling
federated-authority-observationpallet: returningResult<Option<...>, InherentError>with a newDecodeFailedvariant, allowing each caller to handle decode failures appropriately without crashing.Changes
Primitives (
primitives/cnight-observation/src/lib.rs)DecodeFailedvariant toInherentErrorenum with#[cfg_attr(feature = "std", error("Inherent data decode failed"))]is_fatal_error() = truefrom the blanketIsFatalErrorimplPallet (
pallets/cnight-observation/src/lib.rs)get_data_from_inherent_data— Return type changed fromOption<T>toResult<Option<T>, InherentError>;.expect()replaced with.map_err(|_| InherentError::DecodeFailed)create_inherent— Match onOk/Err; error path logslog::error!with targetcnight-observationand returnsNonecheck_inherent— Propagate decode error via?, then.ok_or(InherentError::Other)?for the missing-data caseis_inherent_required— Propagate decode error via?before checking.is_some()Tests (
pallets/cnight-observation/tests/tests.rs)create_malformed_inherent()helper (puts invalid bytes forINHERENT_IDENTIFIER)create_inherent_with_malformed_data_returns_none— verifies no panic, returnsNonecheck_inherent_with_malformed_data_returns_error— verifiesErr(InherentError::DecodeFailed)is_inherent_required_with_malformed_data_returns_error— verifiesErr(InherentError::DecodeFailed)Consensus safety
DecodeFailedis fatal (is_fatal_error() = true), so blocks with corrupted inherent data are rejected during import📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging