Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Better handling of error in inherents logic. #14521

Merged
merged 7 commits into from
Jul 10, 2023
Merged

Better handling of error in inherents logic. #14521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
603a9c1
impl
gui1117 Jul 6, 2023
9426361
trigger CI
gui1117 Jul 8, 2023
327b853
Revert "trigger CI"
gui1117 Jul 8, 2023
e7054cf
Fix
gui1117 Jul 8, 2023
ae10012
fix
gui1117 Jul 8, 2023
3ba4122
fix
gui1117 Jul 8, 2023
c746c9c
fix
gui1117 Jul 8, 2023
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
40 changes: 34 additions & 6 deletions frame/support/procedural/src/construct_runtime/expand/inherent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,37 @@ pub fn expand_outer_inherent(
use #scrate::inherent::{ProvideInherent, IsFatalError};
use #scrate::traits::{IsSubType, ExtrinsicCall};
use #scrate::sp_runtime::traits::Block as _;
use #scrate::_private::sp_inherents::Error;
use #scrate::log;

let mut result = #scrate::inherent::CheckInherentsResult::new();

// This handle assume we abort on the first fatal error.
fn handle_put_error_result(res: Result<(), Error>) {
const LOG_TARGET: &str = "runtime::inherent";
match res {
Ok(()) => (),
Err(Error::InherentDataExists(id)) =>
log::debug!(
target: LOG_TARGET,
"Some error already reported for inherent {:?}, new non fatal \
error is ignored",
id
),
Err(Error::FatalErrorReported) =>
log::error!(
target: LOG_TARGET,
"Fatal error already reported, unexpected considering there is \
only one fatal error",
),
Err(_) =>
Copy link
Contributor Author

@gui1117 gui1117 Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

error variant are different when sp-inherents is compiled with std or not.
but some test crate use construct_runtime without having a std feature. So there is no way to know what is the actual error type.

EDIT: Anyway I will do a follow up issue with proposals for improving inherent code

log::error!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use defensive macro here instead, as we still know these should (almost) never happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but defensive doesn't support renaming of frame_support contrary to construct_runtime.

(by renaming mean if somebody rename in its Cargo.toml like this:

s = { version = "*", package = "frame-support" }

)

target: LOG_TARGET,
"Unexpected error from `put_error` operation",
),
}
}

for xt in block.extrinsics() {
// Inherents are before any other extrinsics.
// And signed extrinsics are not inherents.
Expand All @@ -110,9 +138,9 @@ pub fn expand_outer_inherent(
if #pallet_names::is_inherent(call) {
is_inherent = true;
if let Err(e) = #pallet_names::check_inherent(call, self) {
result.put_error(
handle_put_error_result(result.put_error(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that we move the if fata_error { return } part also to a helper function as this is the part that is still important and ensures we bail after seeing one error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly sure how to factorise it out. something like this?

@@ -138,10 +138,12 @@ pub fn expand_outer_inherent(
                                                                if #pallet_names::is_inherent(call) {
                                                                        is_inherent = true;
                                                                        if let Err(e) = #pallet_names::check_inherent(call, self) {
-                                                                               handle_put_error_result(result.put_error(
-                                                                                       #pallet_names::INHERENT_IDENTIFIER, &e
+                                                                               let is_fatal = put_error_and_is_error_fatal(
+                                                                                       result,
+                                                                                       #pallet_names::INHERENT_IDENTIFIER,
+                                                                                       &e
                                                                                ));
-                                                                               if e.is_fatal_error() {
+                                                                               if is_fatal {
                                                                                        return result;
                                                                                }
                                                                        }

#pallet_names::INHERENT_IDENTIFIER, &e
).expect("There is only one fatal error; qed");
));
if e.is_fatal_error() {
return result;
}
Expand Down Expand Up @@ -153,19 +181,19 @@ pub fn expand_outer_inherent(
});

if !found {
result.put_error(
handle_put_error_result(result.put_error(
#pallet_names::INHERENT_IDENTIFIER, &e
).expect("There is only one fatal error; qed");
));
if e.is_fatal_error() {
return result;
}
}
},
Ok(None) => (),
Err(e) => {
result.put_error(
handle_put_error_result(result.put_error(
#pallet_names::INHERENT_IDENTIFIER, &e
).expect("There is only one fatal error; qed");
));
if e.is_fatal_error() {
return result;
}
Expand Down
6 changes: 6 additions & 0 deletions frame/support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1521,6 +1521,12 @@ pub mod tests {
}
}

/// Private module re-exporting items used by frame support macros.
#[doc(hidden)]
pub mod _private {
pub use sp_inherents;
}

/// Prelude to be used for pallet testing, for ease of use.
#[cfg(feature = "std")]
pub mod testing_prelude {
Expand Down