Skip to content

fix: replace panic with error handling in get_data_from_inherent_data (PM-21799)#1234

Open
m2ux wants to merge 2 commits intomainfrom
fix/PM-21799-inherent-data-panic
Open

fix: replace panic with error handling in get_data_from_inherent_data (PM-21799)#1234
m2ux wants to merge 2 commits intomainfrom
fix/PM-21799-inherent-data-panic

Conversation

@m2ux
Copy link
Copy Markdown
Contributor

@m2ux m2ux commented Apr 3, 2026

Summary

Replace .expect() on inherent data decoding in get_data_from_inherent_data with typed Result<Option<...>, InherentError> error handling, preventing simultaneous validator panics on malformed inherent data.

🎫 PM-21799 📐 Engineering


Motivation

The get_data_from_inherent_data function in the cnight-observation pallet decodes inherent data on every block through three consensus-critical entry points: create_inherent, check_inherent, and is_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-observation pallet: returning Result<Option<...>, InherentError> with a new DecodeFailed variant, allowing each caller to handle decode failures appropriately without crashing.


Changes

Primitives (primitives/cnight-observation/src/lib.rs)

  • Add DecodeFailed variant to InherentError enum with #[cfg_attr(feature = "std", error("Inherent data decode failed"))]
  • Variant inherits is_fatal_error() = true from the blanket IsFatalError impl

Pallet (pallets/cnight-observation/src/lib.rs)

  • get_data_from_inherent_data — Return type changed from Option<T> to Result<Option<T>, InherentError>; .expect() replaced with .map_err(|_| InherentError::DecodeFailed)
  • create_inherent — Match on Ok/Err; error path logs log::error! with target cnight-observation and returns None
  • check_inherent — Propagate decode error via ?, then .ok_or(InherentError::Other)? for the missing-data case
  • is_inherent_required — Propagate decode error via ? before checking .is_some()

Tests (pallets/cnight-observation/tests/tests.rs)

  • Add create_malformed_inherent() helper (puts invalid bytes for INHERENT_IDENTIFIER)
  • create_inherent_with_malformed_data_returns_none — verifies no panic, returns None
  • check_inherent_with_malformed_data_returns_error — verifies Err(InherentError::DecodeFailed)
  • is_inherent_required_with_malformed_data_returns_error — verifies Err(InherentError::DecodeFailed)

Consensus safety

  • Valid inherent data paths produce identical results before and after the change
  • DecodeFailed is fatal (is_fatal_error() = true), so blocks with corrupted inherent data are rejected during import

📌 Submission Checklist

  • Changes are backward-compatible (or flagged if breaking)
  • Pull request description explains why the change is needed
  • Self-reviewed the diff
  • I have included a change file, or skipped for this reason: bug fix with no product-visible change
  • If the changes introduce a new feature, I have bumped the node minor version
  • Update documentation (if relevant)
  • No new todos introduced

🔱 Fork Strategy

  • Node Runtime Update
  • Node Client Update
  • Other
  • N/A

🗹 TODO before merging

  • Ready for review
  • Commit signing (agent environment lacks GPG — may need manual re-sign)

@m2ux m2ux force-pushed the fix/PM-21799-inherent-data-panic branch from 09cd34b to 9857de0 Compare April 5, 2026 18:30
…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
@m2ux m2ux marked this pull request as ready for review April 6, 2026 13:15
@m2ux m2ux requested a review from a team as a code owner April 6, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant