[author-inherent] Move kick_off_authorship_validation() to post_inherents()#93
[author-inherent] Move kick_off_authorship_validation() to post_inherents()#93arturgontijo wants to merge 10 commits intomainfrom
kick_off_authorship_validation() to post_inherents()#93Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces inherent-driven author validation with a PostInherents hook that extracts authors from pre-runtime digests and validates via Changes
Sequence DiagramsequenceDiagram
participant System as Frame System
participant PostInherents as PostInherents Hook
participant Pallet as AuthorInherent Pallet
participant CanAuthor as CanAuthor Validator
participant Storage as Runtime Storage
System->>PostInherents: post_inherents()
PostInherents->>Pallet: read pre-runtime digest
Pallet->>Pallet: extract NimbusId, author, slot
Pallet->>CanAuthor: can_author(author, slot)?
alt author valid
CanAuthor-->>Pallet: true
Pallet->>Storage: write `Author<T>`
Storage-->>Pallet: ack
else missing/invalid/ineligible
CanAuthor-->>Pallet: false / missing
Pallet-->>PostInherents: panic / abort
end
PostInherents-->>System: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pallets/author-inherent/src/tests.rs (1)
52-61: Add one test for the “author not eligible” panic path.Current coverage validates success + missing digest, but not the eligibility assert in
pallets/author-inherent/src/lib.rsLine 168.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/author-inherent/src/tests.rs` around lines 52 - 61, Add a test in the tests module that triggers the "author not eligible" assert path in the pallet: use new_test_ext() and System::initialize with a Digest containing a pre-runtime author digest (so the missing-digest case is not hit), ensure the test environment does not mark that author as eligible (so the eligibility assert in pallets/author-inherent/src/lib.rs is hit), call AuthorInherent::post_inherents() and annotate the test with #[should_panic(expected = "Author not eligible")] so the panic message matches the pallet's assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallets/author-inherent/src/lib.rs`:
- Around line 89-106: Implement an on_initialize hook in the existing Hooks impl
for Pallet<T> that clears the Author storage at the start of each block (e.g.,
call Author::<T>::kill() or set it to None) so the Get<T::AuthorId> trait
implementation and any pallet reading Pallet::<T>::author() do not observe stale
author data before post_inherents() runs; add this logic to the
Hooks<BlockNumberFor<T>> for Pallet<T> alongside the existing integrity_test and
ensure it only clears Author (leave post_inherents() assignment unchanged).
---
Nitpick comments:
In `@pallets/author-inherent/src/tests.rs`:
- Around line 52-61: Add a test in the tests module that triggers the "author
not eligible" assert path in the pallet: use new_test_ext() and
System::initialize with a Digest containing a pre-runtime author digest (so the
missing-digest case is not hit), ensure the test environment does not mark that
author as eligible (so the eligibility assert in
pallets/author-inherent/src/lib.rs is hit), call
AuthorInherent::post_inherents() and annotate the test with
#[should_panic(expected = "Author not eligible")] so the panic message matches
the pallet's assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
pallets/author-inherent/Cargo.tomlpallets/author-inherent/src/benchmarks.rspallets/author-inherent/src/lib.rspallets/author-inherent/src/mock.rspallets/author-inherent/src/tests.rspallets/author-inherent/src/weights.rstemplate/runtime/src/lib.rs
💤 Files with no reviewable changes (2)
- pallets/author-inherent/src/weights.rs
- pallets/author-inherent/src/benchmarks.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pallets/author-inherent/src/tests.rs (1)
52-115: Add a test for the “author not eligible” panic path.Current tests validate “missing digest author” and lifecycle behavior, but do not exercise the assertion branch where the digest author exists and
T::CanAuthor::can_author(...)returns false (lib.rsLines 176-179). Adding this would close the main negative-path gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/author-inherent/src/tests.rs` around lines 52 - 115, Add a new unit test (e.g., test_post_inherents_panics_when_author_not_eligible) that initializes System with a pre-runtime digest containing ALICE's NimbusId and configures the test runtime so T::CanAuthor::can_author(...) returns false for that author, then call AuthorInherent::post_inherents() wrapped with #[should_panic(expected = "Block invalid, author not eligible")] to assert the panic; reference AuthorInherent::post_inherents and the T::CanAuthor::can_author check to locate the assertion branch to exercise.pallets/author-inherent/src/lib.rs (1)
99-106:integrity_test()is coupled to mutable storage state.At Line 104, asserting
Author::<T>::get().is_none()makes integrity checks depend on current storage assumptions rather than invariant/type-level constraints. This can become brittle if runtime initialization/migrations evolve.♻️ Proposed adjustment
fn integrity_test() { // Test that SlotBeacon can be called and returns a valid slot let slot = T::SlotBeacon::slot(); - // Author storage should be accessible (this is a compile-time check) - assert!( - Author::<T>::get().is_none(), - "Author storage should be none" - ); // Test that CanAuthor trait can be called let _ = Pallet::<T>::can_author(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/author-inherent/src/lib.rs` around lines 99 - 106, The integrity_test currently depends on runtime storage by asserting Author::<T>::get().is_none(); remove that state-dependent assertion and replace it with a compile-time type-level check so the test stays invariant: keep the SlotBeacon::slot() call, then add a typed binding like let _ : Option<T::AccountId> = Author::<T>::get(); (or simply remove the Author assertion entirely) so you verify the Author storage accessor compiles without depending on current storage contents; update the integrity_test function (reference: integrity_test, T::SlotBeacon::slot, Author::<T>::get) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallets/author-inherent/src/lib.rs`:
- Around line 173-174: The comment claiming "First check that the slot number is
valid (greater than the previous highest)" is inaccurate: the code only reads
the current slot into new_slot via T::SlotBeacon::slot() and does not perform a
monotonicity check. Update the comment to state that this line simply retrieves
the current slot from the SlotBeacon (reference new_slot and
T::SlotBeacon::slot()), or if you intended to enforce monotonicity, implement a
comparison against the stored highest slot (e.g., a Pallet storage item like
HighestSlot) and handle rejections accordingly; but do not leave the existing
monotonicity wording without the corresponding check.
---
Nitpick comments:
In `@pallets/author-inherent/src/lib.rs`:
- Around line 99-106: The integrity_test currently depends on runtime storage by
asserting Author::<T>::get().is_none(); remove that state-dependent assertion
and replace it with a compile-time type-level check so the test stays invariant:
keep the SlotBeacon::slot() call, then add a typed binding like let _ :
Option<T::AccountId> = Author::<T>::get(); (or simply remove the Author
assertion entirely) so you verify the Author storage accessor compiles without
depending on current storage contents; update the integrity_test function
(reference: integrity_test, T::SlotBeacon::slot, Author::<T>::get) accordingly.
In `@pallets/author-inherent/src/tests.rs`:
- Around line 52-115: Add a new unit test (e.g.,
test_post_inherents_panics_when_author_not_eligible) that initializes System
with a pre-runtime digest containing ALICE's NimbusId and configures the test
runtime so T::CanAuthor::can_author(...) returns false for that author, then
call AuthorInherent::post_inherents() wrapped with #[should_panic(expected =
"Block invalid, author not eligible")] to assert the panic; reference
AuthorInherent::post_inherents and the T::CanAuthor::can_author check to locate
the assertion branch to exercise.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pallets/author-inherent/src/lib.rs (1)
90-95:⚠️ Potential issue | 🟠 Major
post_inherents()work is still unmetered.
on_initialize()only charges theAuthor::<T>::kill(), but the new path also scans digests, decodesNimbusId, doesAccountLookup/CanAuthor, and writesAuthor::<T>again. Becausepost_inherents()cannot return weight, every block is now undercharged unless that worst-case cost is moved back onto a weighted path again, e.g. via a dedicatedWeightInfoentry charged fromon_initialize().In Substrate FRAME, does `frame_support::traits::PostInherents::post_inherents()` participate in block weight accounting, or should its worst-case work be charged from `on_initialize()` or another weighted path?Also applies to: 150-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/author-inherent/src/lib.rs` around lines 90 - 95, The post_inherents() path does non-trivial work (scanning digests, decoding NimbusId, performing AccountLookup/CanAuthor, and writing Author::<T>) but its cost is not charged; update on_initialize() to include a worst-case weight for that work by adding a WeightInfo entry (e.g. WeightInfo::post_inherents or similar) and charge it via T::DbWeight::get().writes(...) + WeightInfo::post_inherents() (or equivalent) in the on_initialize() implementation so the expensive operations performed in post_inherents() are accounted for; reference on_initialize, post_inherents, Author::<T>::kill, and the WeightInfo trait when implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pallets/author-inherent/src/lib.rs`:
- Around line 90-95: The post_inherents() path does non-trivial work (scanning
digests, decoding NimbusId, performing AccountLookup/CanAuthor, and writing
Author::<T>) but its cost is not charged; update on_initialize() to include a
worst-case weight for that work by adding a WeightInfo entry (e.g.
WeightInfo::post_inherents or similar) and charge it via
T::DbWeight::get().writes(...) + WeightInfo::post_inherents() (or equivalent) in
the on_initialize() implementation so the expensive operations performed in
post_inherents() are accounted for; reference on_initialize, post_inherents,
Author::<T>::kill, and the WeightInfo trait when implementing this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ad6bb59-a8d5-4249-baae-bad77bffae4b
📒 Files selected for processing (3)
pallets/author-inherent/src/lib.rspallets/author-inherent/src/mock.rspallets/author-inherent/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- pallets/author-inherent/src/mock.rs
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
-
High: author validation no longer participates in the
check_inherents()path used during block import. The verifier still callsruntime_api().check_inherents(...)at client/consensus/nimbus-consensus/src/import_queue.rs:140, and the runtime implementation still only runsdata.check_extrinsics(&block)at template/runtime/src/lib.rs:757. Since author validation now lives only inPostInherentsat pallets/author-inherent/src/lib.rs:153 and the pallet no longer exposes an inherent extrinsic in template/runtime/src/lib.rs:702, invalid authors stop surfacing as inherent-check failures during import. Full execution may still reject the block, but the import-time validation behavior has changed. -
Medium: pallets/author-inherent/src/lib.rs:90 now hardcodes the post-inherent charge to
DbWeight::reads_writes(3, 2), but the actual validation path is materially heavier in the template runtime.find_author()still does an account lookup through template/runtime/src/pallet_account_set.rs:80, andT::CanAuthor::can_authorresolves toAuthorFilter, which readsEligibleCount, loads the full stored author set, and samples randomness while filtering at pallets/author-slot-filter/src/lib.rs:107 and pallets/author-slot-filter/src/lib.rs:150. Removing the benchmarked weight path means this mandatory per-block work is now under-accounted, which can skew block weight/proof accounting.
3 participants
This PR moves the
kick_off_authorship_validation()logic to thepost_inherents().It also adds
integrity_test()hook with some checks.Note: Do we need more integrity checks there? I decided to keep
tests.rssimple as this new approach is.As we see here,
integrity_testsis also being tested too.Downstream runtimes using
pallet-author-inherentmust update their integration:AuthorInherenttoframe_system::Config::PostInherents.Call/Inherentfrom the pallet’sconstruct_runtime!entry:pallet_author_inherent::{Pallet, Call, Storage, Inherent}pallet_author_inherent::{Pallet, Storage}type WeightInfo = ...fromimpl pallet_author_inherent::Config.