add notes and warnings to ProvideInherent docs #9730

apopiak · 2021-09-08T10:10:22Z

This PR makes some changes to the inherents docs to avoid misunderstandings.

frame/support/src/inherent.rs

gui1117 · 2021-09-08T11:01:00Z

frame/support/src/inherent.rs

+	/// 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`].


security checks cannot be put in create_inherent AFAIK.
I don't really know the nature of the check that check_inherent provide, but it is important for security reason otherwise there is no point to it, no ?
Some check for the validity of the timestamp is done in check_inherent.

As far as I understand this: It is what other validator will use the check the validity of the inherents provided in comparison to some inherent data that the validator will provide.
For instance, a validator will check that the timestamp is not too much in the future compared to what he considers to be the present. (I.e. compared to the time he provided in his inherent data)

frame/support/src/inherent.rs

Co-authored-by: Bastian Köcher <[email protected]>

stale · 2021-10-23T13:52:04Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

apopiak · 2021-11-26T16:50:44Z

@bkchr @thiolliere What do you think of the rewording?

gui1117 · 2021-11-29T08:09:06Z

frame/support/src/inherent.rs

+	/// 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.


I don't very like this example, probably for the same reason as said above by Basti.

Like currently the block producer is passed by using digest, but I guess we could use an inherent.
Then the inherent could just write the author into a storage (which can be removed in on_finalize in order not to get into the block).
Later other pallets could check that the author is able to produce a block, and just panic in case he is not able to produce a block.

Thus the check whether the producer is eligible to create the block will not happen in the call, but later in the block production.

I think that this part:

/// 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.

is good, but maybe as an example I would write:

E.g. if this provide 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.
Later in the on_finalize hook, the pallet could also ensure that the timestamp is set.

gui1117

all good to me, except the example which I would prefer to be different https://github.com/paritytech/substrate/pull/9730/files#r758115926

gui1117 · 2021-11-29T08:14:09Z

frame/support/src/inherent.rs

 	/// If the inherent is not present, it will return `e`.
 	///
 	/// - `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.


I'm not sure how to write it but this is_inherent_required is currently only checked by other block producer same as check_inherent, maybe we can change it to make it part of the execute_block. But it is not the case currently.

stale · 2021-12-29T11:04:26Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

…/provide-inherent-docs

frame/support/src/inherent.rs

Co-authored-by: Bastian Köcher <[email protected]>

frame/support/src/inherent.rs

stale · 2022-03-25T12:38:56Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

* add notes and warnings to ProvideInherent docs * rephrase ProvideInherent doc comments * more comment refinement * remove multiple inherents note * remove repetition Co-authored-by: Bastian Köcher <[email protected]> * replace inherent example in docs * add note about who checks is_inherent_required * Apply suggestions from code review Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

add notes and warnings to ProvideInherent docs

197b216

apopiak added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 8, 2021

apopiak requested review from bkchr and gui1117 September 8, 2021 10:12

gui1117 reviewed Sep 8, 2021

View reviewed changes

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

gui1117 reviewed Sep 8, 2021

View reviewed changes

rephrase ProvideInherent doc comments

9ba7d68

apopiak added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Sep 8, 2021

bkchr reviewed Sep 8, 2021

View reviewed changes

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

frame/support/src/inherent.rs Show resolved Hide resolved

apopiak and others added 3 commits September 23, 2021 14:39

more comment refinement

895afe5

remove multiple inherents note

568a70f

remove repetition

e1f29cb

Co-authored-by: Bastian Köcher <[email protected]>

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 23, 2021

stale bot closed this Nov 6, 2021

apopiak reopened this Nov 26, 2021

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 26, 2021

gui1117 reviewed Nov 29, 2021

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 29, 2021

apopiak added 3 commits January 4, 2022 12:23

replace inherent example in docs

f848dfc

add note about who checks is_inherent_required

d7395be

Merge branch 'master' of github.com:paritytech/substrate into apopiak…

1234f51

…/provide-inherent-docs

apopiak commented Jan 4, 2022

View reviewed changes

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

apopiak removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jan 4, 2022

bkchr reviewed Jan 11, 2022

View reviewed changes

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

apopiak commented Jan 12, 2022

View reviewed changes

frame/support/src/inherent.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

57b0525

Co-authored-by: Bastian Köcher <[email protected]>

apopiak added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 12, 2022

apopiak marked this pull request as ready for review January 12, 2022 16:07

bkchr reviewed Jan 13, 2022

View reviewed changes

frame/support/src/inherent.rs Show resolved Hide resolved

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 13, 2022

paritytech deleted a comment from stale bot Feb 14, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 14, 2022

bkchr approved these changes Feb 21, 2022

View reviewed changes

apopiak requested a review from gui1117 February 23, 2022 11:25

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 25, 2022

bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 27, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 27, 2022

bkchr merged commit 65b44e9 into master Mar 27, 2022

bkchr deleted the apopiak/provide-inherent-docs branch March 27, 2022 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add notes and warnings to ProvideInherent docs #9730

add notes and warnings to ProvideInherent docs #9730

apopiak commented Sep 8, 2021 •

edited

Loading

gui1117 Sep 8, 2021 •

edited

Loading

stale bot commented Oct 23, 2021

apopiak commented Nov 26, 2021

gui1117 Nov 29, 2021 •

edited

Loading

gui1117 left a comment

gui1117 Nov 29, 2021

stale bot commented Dec 29, 2021

stale bot commented Mar 25, 2022

		/// 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`].

add notes and warnings to ProvideInherent docs #9730

add notes and warnings to ProvideInherent docs #9730

Conversation

apopiak commented Sep 8, 2021 • edited Loading

gui1117 Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

stale bot commented Oct 23, 2021

apopiak commented Nov 26, 2021

gui1117 Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

gui1117 left a comment

Choose a reason for hiding this comment

gui1117 Nov 29, 2021

Choose a reason for hiding this comment

stale bot commented Dec 29, 2021

stale bot commented Mar 25, 2022

apopiak commented Sep 8, 2021 •

edited

Loading

gui1117 Sep 8, 2021 •

edited

Loading

gui1117 Nov 29, 2021 •

edited

Loading