Skip to content
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.

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

[FRAME] Enforce inherent order #2154

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
976 changes: 15 additions & 961 deletions substrate/frame/executive/src/lib.rs

Large diffs are not rendered by default.

1,264 changes: 1,264 additions & 0 deletions substrate/frame/executive/src/tests.rs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ pub fn expand_outer_inherent(
) -> TokenStream {
let mut pallet_names = Vec::new();
let mut pallet_attrs = Vec::new();
let mut pallet_indices = Vec::new();
let mut query_inherent_part_macros = Vec::new();

for pallet_decl in pallet_decls {
if pallet_decl.exists_part("Inherent") {
let name = &pallet_decl.name;
let path = &pallet_decl.path;
let index = &pallet_decl.index;
let attr = pallet_decl.cfg_pattern.iter().fold(TokenStream::new(), |acc, pattern| {
let attr = TokenStream::from_str(&format!("#[cfg({})]", pattern.original()))
.expect("was successfully parsed before; qed");
Expand All @@ -47,6 +49,7 @@ pub fn expand_outer_inherent(

pallet_names.push(name);
pallet_attrs.push(attr);
pallet_indices.push(index);
query_inherent_part_macros.push(quote! {
#path::__substrate_inherent_check::is_inherent_part_defined!(#name);
});
Expand Down Expand Up @@ -204,13 +207,59 @@ pub fn expand_outer_inherent(
}
}

impl #scrate::traits::EnsureInherentsAreOrdered<#block> for #runtime {
fn ensure_inherents_are_ordered(block: &#block, num_inherents: usize) -> Result<(), #scrate::__private::sp_inherents::InherentOrderError> {
use #scrate::inherent::ProvideInherent;
use #scrate::traits::{IsSubType, ExtrinsicCall};
use #scrate::sp_runtime::traits::Block as _;
use #scrate::__private::sp_inherents::{InherentOrder, InherentOrderError};

// The last order that we encountered. Must increase monotonic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The last order that we encountered. Must increase monotonic.
// The last order that we encountered. Must increase monotonically.

let mut last: Option<InherentOrder> = None;

for (i, xt) in block.extrinsics().iter().take(num_inherents).enumerate() {
let call = <#unchecked_extrinsic as ExtrinsicCall>::call(xt);

let mut current: Option<InherentOrder> = None;

#(
if let Some(call) = IsSubType::<_>::is_sub_type(call) {
if #pallet_names::is_inherent(&call) {
let order = #pallet_names::inherent_order().unwrap_or(InherentOrder::Index(#pallet_indices as u32));
Copy link
Contributor

Choose a reason for hiding this comment

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

Inherent order shouldn't depend on pallet indexes, because pallet indexes can't be modified for a chain that's already been in production for a long time (this breaks metadata and SCALE encoding). I suggest instead defaulting to the order of declaration in the construct_runtime macro, and allowing the runtime to define a different explicit order.


if current.is_some() {
return Err(InherentOrderError::InherentWithMultiplePallets);
}
current = Some(order);
// Note: we cannot break here since its a macro expand.
}
}
)*

let Some(current) = current else {
return Err(InherentOrderError::InherentWithoutPallet);
};

if let Some(last) = last {
if last >= current {
return Err(InherentOrderError::OutOfOrder(last, current));
}
}

last = Some(current);
Comment on lines +239 to +249
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some comments here

}

Ok(())
}
}

impl #scrate::traits::EnsureInherentsAreFirst<#block> for #runtime {
fn ensure_inherents_are_first(block: &#block) -> Result<(), u32> {
fn ensure_inherents_are_first(block: &#block) -> Result<u32, u32> {
use #scrate::inherent::ProvideInherent;
use #scrate::traits::{IsSubType, ExtrinsicCall};
use #scrate::sp_runtime::traits::Block as _;

let mut first_signed_observed = false;
let mut num_inherents = 0usize;

for (i, xt) in block.extrinsics().iter().enumerate() {
let is_signed = #scrate::sp_runtime::traits::Extrinsic::is_signed(xt)
Expand All @@ -235,16 +284,16 @@ pub fn expand_outer_inherent(
is_inherent
};

if !is_inherent {
first_signed_observed = true;
}
if is_inherent {
if num_inherents != i {
return Err(i as u32);
}

if first_signed_observed && is_inherent {
return Err(i as u32)
num_inherents += 1; // Safe since we are in an `enumerate` loop.
}
}

Ok(())
Ok(num_inherents as u32)
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion substrate/frame/support/src/inherent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
// limitations under the License.

pub use sp_inherents::{
CheckInherentsResult, InherentData, InherentIdentifier, IsFatalError, MakeFatalError,
CheckInherentsResult, InherentData, InherentIdentifier, InherentOrder, IsFatalError,
MakeFatalError,
};

/// A pallet that provides or verifies an inherent extrinsic will implement this trait.
Expand Down Expand Up @@ -81,6 +82,13 @@ pub trait ProvideInherent {
Ok(())
}

/// The order of the inherent relative to all other inherents.
///
/// A return value of `None` indicates that the pallet index should be used.
fn inherent_order() -> Option<InherentOrder> {
None
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that defining the order at pallet level is not good design. It's up to the runtime to define the order of inherents, because for the same pallet set, different runtimes will have different requirements.

}
Comment on lines +88 to +90
Copy link
Contributor

@liamaharon liamaharon Nov 7, 2023

Choose a reason for hiding this comment

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

Maybe 'priority' instead of 'order' is better description for the api.

So, inherents are 'ordered' by their 'priority'.

Suggested change
fn inherent_order() -> Option<InherentOrder> {
None
}
fn inherent_priority() -> Option<InherentPriority> {
None
}


/// Return whether the call is an inherent call.
///
/// NOTE: Signed extrinsics are not inherents, but a signed extrinsic with the given call
Expand Down
9 changes: 5 additions & 4 deletions substrate/frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ pub use misc::{
defensive_prelude::{self, *},
AccountTouch, Backing, ConstBool, ConstI128, ConstI16, ConstI32, ConstI64, ConstI8, ConstU128,
ConstU16, ConstU32, ConstU64, ConstU8, DefensiveMax, DefensiveMin, DefensiveSaturating,
DefensiveTruncateFrom, EnsureInherentsAreFirst, EqualPrivilegeOnly, EstimateCallFee,
ExecuteBlock, ExtrinsicCall, Get, GetBacking, GetDefault, HandleLifetime, IsSubType, IsType,
Len, OffchainWorker, OnKilledAccount, OnNewAccount, PrivilegeCmp, SameOrOther, Time,
TryCollect, TryDrop, TypedGet, UnixTime, VariantCount, WrapperKeepOpaque, WrapperOpaque,
DefensiveTruncateFrom, EnsureInherentsAreFirst, EnsureInherentsAreOrdered, EqualPrivilegeOnly,
EstimateCallFee, ExecuteBlock, ExtrinsicCall, Get, GetBacking, GetDefault, HandleLifetime,
IsSubType, IsType, Len, OffchainWorker, OnKilledAccount, OnNewAccount, PrivilegeCmp,
SameOrOther, Time, TryCollect, TryDrop, TypedGet, UnixTime, VariantCount, WrapperKeepOpaque,
WrapperOpaque,
};
#[allow(deprecated)]
pub use misc::{PreimageProvider, PreimageRecipient};
Expand Down
20 changes: 18 additions & 2 deletions substrate/frame/support/src/traits/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use impl_trait_for_tuples::impl_for_tuples;
use scale_info::{build::Fields, meta_type, Path, Type, TypeInfo, TypeParameter};
use sp_arithmetic::traits::{CheckedAdd, CheckedMul, CheckedSub, One, Saturating};
use sp_core::bounded::bounded_vec::TruncateFrom;
use sp_inherents::InherentOrderError;
#[doc(hidden)]
pub use sp_runtime::traits::{
ConstBool, ConstI128, ConstI16, ConstI32, ConstI64, ConstI8, ConstU128, ConstU16, ConstU32,
Expand Down Expand Up @@ -892,8 +893,23 @@ pub trait GetBacking {
pub trait EnsureInherentsAreFirst<Block> {
/// Ensure the position of inherent is correct, i.e. they are before non-inherents.
///
/// On error return the index of the inherent with invalid position (counting from 0).
fn ensure_inherents_are_first(block: &Block) -> Result<(), u32>;
/// On error return the index of the inherent with invalid position (counting from 0). On
/// success it returns the index of the last inherent. `0` therefore means that there are no
/// inherents.
fn ensure_inherents_are_first(block: &Block) -> Result<u32, u32>;
}

/// Ensures the inherent order of a block.
pub trait EnsureInherentsAreOrdered<Block> {
/// Checks that the order of the inhernets matches the order reported by
/// [`ProvideInherent::inherent_order`].
///
/// The second arg are the number of inherents and can be acquired by calling
/// [`EnsureInherentsAreFirst::ensure_inherents_are_first`].
fn ensure_inherents_are_ordered(
block: &Block,
num_inherents: usize,
) -> Result<(), InherentOrderError>;
}

/// An extrinsic on which we can get access to call.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,24 @@ error[E0599]: no function or associated item named `is_inherent_required` found
= note: the following trait defines an item `is_inherent_required`, perhaps you need to implement it:
candidate #1: `ProvideInherent`
= note: this error originates in the macro `construct_runtime` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no function or associated item named `inherent_order` found for struct `pallet::Pallet` in the current scope
--> tests/construct_runtime_ui/undefined_inherent_part.rs:65:1
|
28 | pub struct Pallet<T>(_);
| -------------------- function or associated item `inherent_order` not found for this struct
...
65 | construct_runtime! {
| _^
66 | | pub struct Runtime
67 | | {
68 | | System: frame_system expanded::{}::{Pallet, Call, Storage, Config<T>, Event<T>},
69 | | Pallet: pallet expanded::{}::{Pallet, Inherent},
70 | | }
71 | | }
| |_^ function or associated item not found in `Pallet<Runtime>`
|
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following trait defines an item `inherent_order`, perhaps you need to implement it:
candidate #1: `ProvideInherent`
= note: this error originates in the macro `construct_runtime` (in Nightly builds, run with -Z macro-backtrace for more info)
51 changes: 50 additions & 1 deletion substrate/primitives/inherents/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ use sp_std::{

#[cfg(feature = "std")]
mod client_side;
#[cfg(test)]
mod tests;

#[cfg(feature = "std")]
pub use client_side::*;
Expand Down Expand Up @@ -367,6 +369,53 @@ impl PartialEq for CheckInherentsResult {
}
}

/// The order of an inherent.
#[derive(Copy, Clone, Debug, Eq, PartialEq, Encode, Decode, scale_info::TypeInfo)]
pub enum InherentOrder {
/// The first inherent.
First,
Comment on lines +375 to +376
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't index 0 always first? why do we need a special case for that?

/// An inherent at a specific index.
Index(u32),
/// The last inherent.
Last,
}

/// The reason why the order of inherents was invalid.
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum InherentOrderError {
/// An inherent does not belong to any pallet.
InherentWithoutPallet,
/// An inherent belongs to multiple pallets.
InherentWithMultiplePallets,
/// An inherent was encountered out of order.
///
/// The first and second parameters are supposed to be ordered ascending, but were not.
OutOfOrder(InherentOrder, InherentOrder),
}

impl Ord for InherentOrder {
fn cmp(&self, other: &Self) -> core::cmp::Ordering {
use core::cmp::Ordering::*;
use InherentOrder::*;

match (self, other) {
(First, First) => Equal,
(First, _) => Less,
(Index(a), Index(b)) => a.cmp(b),
(Index(_), First) => Greater,
(Index(_), Last) => Less,
(Last, Last) => Equal,
(Last, _) => Greater,
}
}
}

impl PartialOrd for InherentOrder {
fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
Some(self.cmp(other))
}
}

/// Did we encounter a fatal error while checking an inherent?
///
/// A fatal error is everything that fails while checking an inherent error, e.g. the inherent
Expand Down Expand Up @@ -397,7 +446,7 @@ impl<E: codec::Encode> IsFatalError for MakeFatalError<E> {
}

#[cfg(test)]
mod tests {
mod unit_tests {
use super::*;
use codec::{Decode, Encode};

Expand Down
51 changes: 51 additions & 0 deletions substrate/primitives/inherents/src/tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use super::*;

#[test]
fn inherent_order_ord_works() {
// `First` is the first:
for i in 0..10 {
assert!(InherentOrder::First < InherentOrder::Index(i));
}
assert!(InherentOrder::First < InherentOrder::Last);
assert!(InherentOrder::First == InherentOrder::First);

// `Last` is the last:
for i in 0..10 {
assert!(InherentOrder::Last > InherentOrder::Index(i));
}
assert!(InherentOrder::Last == InherentOrder::Last);
assert!(InherentOrder::Last > InherentOrder::First);

// `Index` is ordered correctly:
for i in 0..10 {
for j in 0..10 {
let a = InherentOrder::Index(i);
let b = InherentOrder::Index(j);

if i < j {
assert!(a < b);
} else if i > j {
assert!(a > b);
} else {
assert!(a == b);
}
}
}
}
Loading