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

Ensure that inherents are first and unsigned #1654

Merged
merged 2 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions pallets/parachain-system/src/validate_block/implementation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ where
let inherent_data = block
.extrinsics()
.iter()
// Inherents are at the front of the block and are unsigned.
//
// If `is_signed` is returning `None`, we keep it safe and assume that it is "signed".
// We are searching for unsigned transactions anyway.
.take_while(|e| !e.is_signed().unwrap_or(true))
Comment on lines +123 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

@bkchr just to clarify if I get the comments correctly.

This take_while(..) will include the extrinsic if is_signed() returns None, while the docs say to keep it safe and assumes the extrinsic is signed, which I understand as rejecting the extrinsic at that point instead of including it. Hence, I am confused.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a ! at the front. So, we only take extrinsics until we find the first signed one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, missed that!

.filter_map(|e| e.call().is_sub_type())
.find_map(|c| match c {
crate::Call::set_validation_data { data: validation_data } =>
Expand Down
37 changes: 37 additions & 0 deletions pallets/parachain-system/src/validate_block/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,40 @@ fn check_inherent_fails_on_validate_block_as_expected() {
);
}
}

#[test]
fn check_inherents_are_unsigned_and_before_all_other_extrinsics() {
sp_tracing::try_init_simple();

if env::var("RUN_TEST").is_ok() {
let (client, parent_head) = create_test_client();

let TestBlockData { block, validation_data } =
build_block_with_witness(&client, Vec::new(), parent_head.clone(), Default::default());

let (header, mut extrinsics, proof) = block.deconstruct();

extrinsics.insert(0, transfer(&client, Alice, Bob, 69));

call_validate_block(
parent_head,
ParachainBlockData::new(header, extrinsics, proof),
validation_data.relay_parent_storage_root,
)
.unwrap_err();
} else {
let output = Command::new(env::current_exe().unwrap())
.args(&[
"check_inherents_are_unsigned_and_before_all_other_extrinsics",
"--",
"--nocapture",
])
.env("RUN_TEST", "1")
.output()
.expect("Runs the test");
assert!(output.status.success());

assert!(dbg!(String::from_utf8(output.stderr).unwrap())
bkchr marked this conversation as resolved.
Show resolved Hide resolved
.contains("Could not find `set_validation_data` inherent"));
}
}