From 197b21676789fcd4a69a7912ab089c2a9bc5f384 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Wed, 8 Sep 2021 12:10:01 +0200 Subject: [PATCH 1/8] add notes and warnings to ProvideInherent docs --- frame/support/src/inherent.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index 2125f3e7f50a7..2ed2e0d119b4c 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -36,6 +36,11 @@ pub trait ProvideInherent { const INHERENT_IDENTIFIER: self::InherentIdentifier; /// Create an inherent out of the given `InherentData`. + /// + /// NOTE: All checks necessary to ensure that the inherent is correct and that can be done in + /// the runtime should happen here. + /// E.g. if this provides a block producer, check here that the producer is eligible to create + /// the block. fn create_inherent(data: &InherentData) -> Option; /// Determines whether this inherent is required in this block. @@ -44,7 +49,7 @@ pub trait ProvideInherent { /// implementation returns this. /// /// - `Ok(Some(e))` indicates that this inherent is required in this block. `construct_runtime!` - /// will call this function from in its implementation of `fn check_extrinsics`. + /// will call this function in its implementation of `fn check_extrinsics`. /// If the inherent is not present, it will return `e`. /// /// - `Err(_)` indicates that this function failed and further operations should be aborted. @@ -64,21 +69,29 @@ pub trait ProvideInherent { /// included in the block by its author. Whereas the second parameter represents the inherent /// data that the verifying node calculates. /// - /// NOTE: A block can contains multiple inherent. + /// This is intended to allow for checks that cannot be done within the runtime such as, e.g., + /// the timestamp. + /// + /// NOTE: A block can contain multiple inherents. + /// + /// # Warning + /// + /// This check is not guaranteed to be run as part of consensus and cannot be relied upon for + /// security. Run security relevant checks in [`Self::create_inherent`]. fn check_inherent(_: &Self::Call, _: &InherentData) -> Result<(), Self::Error> { Ok(()) } /// Return whether the call is an inherent call. /// - /// NOTE: Signed extrinsics are not inherent, but signed extrinsic with the given call variant - /// can be dispatched. + /// NOTE: Signed extrinsics are not inherents, but a signed extrinsic with the given call + /// variant can be dispatched. /// /// # Warning /// - /// In FRAME, inherent are enforced to be before other extrinsics, for this reason, + /// In FRAME, inherents are enforced to be executed before other extrinsics. For this reason, /// pallets with unsigned transactions **must ensure** that no unsigned transaction call /// is an inherent call, when implementing `ValidateUnsigned::validate_unsigned`. - /// Otherwise block producer can produce invalid blocks by including them after non inherent. + /// Otherwise block producers can produce invalid blocks by including them after non inherent. fn is_inherent(call: &Self::Call) -> bool; } From 9ba7d68b5b5ef270fb332a5076ea9e6f5d4c9a0f Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Wed, 8 Sep 2021 14:47:46 +0200 Subject: [PATCH 2/8] rephrase ProvideInherent doc comments --- frame/support/src/inherent.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index 2ed2e0d119b4c..9811d4b94e360 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -38,9 +38,9 @@ pub trait ProvideInherent { /// Create an inherent out of the given `InherentData`. /// /// NOTE: All checks necessary to ensure that the inherent is correct and that can be done in - /// the runtime should happen here. - /// E.g. if this provides a block producer, check here that the producer is eligible to create - /// the block. + /// the runtime should happen in the returned `Call`. + /// E.g. if this provides a block producer, check in the returned extrinsic that the producer is + /// eligible to create the block. fn create_inherent(data: &InherentData) -> Option; /// Determines whether this inherent is required in this block. @@ -76,8 +76,9 @@ pub trait ProvideInherent { /// /// # Warning /// - /// This check is not guaranteed to be run as part of consensus and cannot be relied upon for - /// security. Run security relevant checks in [`Self::create_inherent`]. + /// This check is not guaranteed to be run by all full nodes and cannot be relied upon for + /// ensuring correct block import. Run security relevant checks in the extrinsic returned by + /// [`Self::create_inherent`] as much as possible. fn check_inherent(_: &Self::Call, _: &InherentData) -> Result<(), Self::Error> { Ok(()) } From 895afe58ec18d5ac5f83e812d3e84ffd1f5fbb74 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Sep 2021 14:39:11 +0200 Subject: [PATCH 3/8] more comment refinement --- frame/support/src/inherent.rs | 9 ++++++--- primitives/inherents/src/lib.rs | 12 ++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index 9811d4b94e360..324ba9ee60248 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -24,9 +24,12 @@ pub use sp_inherents::{ CheckInherentsResult, InherentData, InherentIdentifier, IsFatalError, MakeFatalError, }; -/// A pallet that provides or verifies an inherent extrinsic. +/// A pallet that provides or verifies an inherent extrinsic will implement this trait. /// -/// The pallet may provide the inherent, verify an inherent, or both provide and verify. +/// The pallet may provide an inherent, verify an inherent, or both provide and verify. +/// +/// Briefly, inherent extrinsics ("inherents") are extrinsics that are added to a block by the block +/// producer. See [`sp_inherents`] for more documentation on inherents. pub trait ProvideInherent { /// The call type of the pallet. type Call; @@ -93,6 +96,6 @@ pub trait ProvideInherent { /// In FRAME, inherents are enforced to be executed before other extrinsics. For this reason, /// pallets with unsigned transactions **must ensure** that no unsigned transaction call /// is an inherent call, when implementing `ValidateUnsigned::validate_unsigned`. - /// Otherwise block producers can produce invalid blocks by including them after non inherent. + /// Otherwise block producers can produce invalid blocks by including them after non-inherents. fn is_inherent(call: &Self::Call) -> bool; } diff --git a/primitives/inherents/src/lib.rs b/primitives/inherents/src/lib.rs index 90f4e455a42d3..cb3b49ee26a97 100644 --- a/primitives/inherents/src/lib.rs +++ b/primitives/inherents/src/lib.rs @@ -15,14 +15,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Substrate inherent extrinsics +//! Substrate Inherent Extrinsics //! //! Inherent extrinsics are extrinsics that are inherently added to each block. However, it is up to -//! runtime implementation to require an inherent for each block or to make it optional. Inherents -//! are mainly used to pass data from the block producer to the runtime. So, inherents require some -//! part that is running on the client side and some part that is running on the runtime side. Any -//! data that is required by an inherent is passed as [`InherentData`] from the client to the -//! runtime when the inherents are constructed. +//! the runtime implementation to require an inherent for each block or to make it optional. +//! Inherents are mainly used to pass data from the block producer to the runtime. So, inherents +//! require some part that is running on the client side and some part that is running on the +//! runtime side. Any data that is required by an inherent is passed as [`InherentData`] from the +//! client to the runtime when the inherents are constructed. //! //! The process of constructing and applying inherents is the following: //! From 568a70f7e6659c0f00408928d91cd0d6a6c4537c Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Sep 2021 14:40:49 +0200 Subject: [PATCH 4/8] remove multiple inherents note --- frame/support/src/inherent.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index 324ba9ee60248..ef284437cb391 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -57,7 +57,7 @@ pub trait ProvideInherent { /// /// - `Err(_)` indicates that this function failed and further operations should be aborted. /// - /// NOTE: If inherent is required then the runtime asserts that the block contains at least + /// NOTE: If the inherent is required then the runtime asserts that the block contains at least /// one inherent for which: /// * type is [`Self::Call`], /// * [`Self::is_inherent`] returns true. @@ -75,8 +75,6 @@ pub trait ProvideInherent { /// This is intended to allow for checks that cannot be done within the runtime such as, e.g., /// the timestamp. /// - /// NOTE: A block can contain multiple inherents. - /// /// # Warning /// /// This check is not guaranteed to be run by all full nodes and cannot be relied upon for From e1f29cbff813dc1ac6148ff5c7cd8f62436e04ac Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Sep 2021 14:42:22 +0200 Subject: [PATCH 5/8] remove repetition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/support/src/inherent.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index ef284437cb391..ee2e651c4be0a 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -78,8 +78,7 @@ pub trait ProvideInherent { /// # Warning /// /// This check is not guaranteed to be run by all full nodes and cannot be relied upon for - /// ensuring correct block import. Run security relevant checks in the extrinsic returned by - /// [`Self::create_inherent`] as much as possible. + /// ensuring that the block is correct. fn check_inherent(_: &Self::Call, _: &InherentData) -> Result<(), Self::Error> { Ok(()) } From f848dfcf3862dbc659fe0a57095c4c6200b9e133 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 4 Jan 2022 12:23:43 +0100 Subject: [PATCH 6/8] replace inherent example in docs --- frame/support/src/inherent.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index ee2e651c4be0a..c59edfc471f73 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -42,8 +42,9 @@ pub trait ProvideInherent { /// /// NOTE: All checks necessary to ensure that the inherent is correct and that can be done in /// the runtime should happen in the returned `Call`. - /// E.g. if this provides a block producer, check in the returned extrinsic that the producer is - /// eligible to create the block. + /// E.g. if this provides the timestamp, the call will check that the given timestamp is + /// increasing the old timestamp by more than a minimum and it will also check that the + /// timestamp hasn't already been set. fn create_inherent(data: &InherentData) -> Option; /// Determines whether this inherent is required in this block. From d7395be5c682d1a6fefb6e9714cf092a0b5284e7 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 4 Jan 2022 12:26:34 +0100 Subject: [PATCH 7/8] add note about who checks is_inherent_required --- frame/support/src/inherent.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index c59edfc471f73..a2f192a0fe667 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -62,6 +62,8 @@ pub trait ProvideInherent { /// one inherent for which: /// * type is [`Self::Call`], /// * [`Self::is_inherent`] returns true. + /// + /// NOTE: This is currently only checked by block validators, not all full nodes. fn is_inherent_required(_: &InherentData) -> Result, Self::Error> { Ok(None) } From 57b0525f2fbd43200d1cc3653431752d1c7d4b45 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Wed, 12 Jan 2022 17:06:31 +0100 Subject: [PATCH 8/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/support/src/inherent.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/support/src/inherent.rs b/frame/support/src/inherent.rs index 4ab2a27351ca1..0aa6b9d3f75a9 100644 --- a/frame/support/src/inherent.rs +++ b/frame/support/src/inherent.rs @@ -44,7 +44,7 @@ pub trait ProvideInherent { /// the runtime should happen in the returned `Call`. /// E.g. if this provides the timestamp, the call will check that the given timestamp is /// increasing the old timestamp by more than a minimum and it will also check that the - /// timestamp hasn't already been set. + /// timestamp hasn't already been set in the current block. fn create_inherent(data: &InherentData) -> Option; /// Determines whether this inherent is required in this block. @@ -63,7 +63,7 @@ pub trait ProvideInherent { /// * type is [`Self::Call`], /// * [`Self::is_inherent`] returns true. /// - /// NOTE: This is currently only checked by block validators, not all full nodes. + /// NOTE: This is currently only checked by block producers, not all full nodes. fn is_inherent_required(_: &InherentData) -> Result, Self::Error> { Ok(None) } @@ -96,6 +96,6 @@ pub trait ProvideInherent { /// In FRAME, inherents are enforced to be executed before other extrinsics. For this reason, /// pallets with unsigned transactions **must ensure** that no unsigned transaction call /// is an inherent call, when implementing `ValidateUnsigned::validate_unsigned`. - /// Otherwise block producers can produce invalid blocks by including them after non-inherents. + /// Otherwise block producers can produce invalid blocks by including them after non inherents. fn is_inherent(call: &Self::Call) -> bool; }