Skip to content

[author-inherent] Use ensure!() to avoid storage transactions are left open by the runtime #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
arturgontijo wants to merge 1 commit into from
Closed

[author-inherent] Use ensure!() to avoid storage transactions are left open by the runtime #91

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pallets/author-inherent/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ pub mod pallet {
let new_slot = T::SlotBeacon::slot();

// Now check that the author is valid in this slot
assert!(
ensure!(
Copy link
Contributor

@RomarQ RomarQ Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to panic here. The fix for the pending block inherents needs to be on the client side, when we provide the pending_consenus_data_provider.

Copy link
Contributor

@manuelmauro manuelmauro Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch Rodri, shall we comment something like this here then:

Why assert! is critical here

  1. This is a mandatory inherent (DispatchClass::Mandatory at line 138). Mandatory inherents must be included in every block AND must succeed. If validation fails, the block itself is invalid.
  2. Block-level validity, not transaction-level - The comment explicitly says "Block invalid". If an ineligible author produced a block, we don't want that block to exist at all. Using ensure! would merely fail
  the inherent call while potentially allowing the block to be finalized.
  3. Security invariant - Author eligibility is a consensus-critical check. If this fails, something is fundamentally wrong (either a bug or an attack). The block must be rejected entirely, not just have a failed
   transaction in it.
  4. No recovery path - There's no sensible way to "handle" an invalid author gracefully. The block simply shouldn't exist.

and possibly add proper testing for this path?

T::CanAuthor::can_author(&Self::get(), &new_slot),
"Block invalid, supplied author is not eligible."
Error::<T>::CannotBeAuthor
);

InherentIncluded::<T>::put(true);
Expand Down