-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
CanAuthotWith trait removed. Also all dependencies, parameters, type paramers were removed. This is related to removal of native runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar, but looks good. And welcome 🎩!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bot merge |
* Remove CanAuthorWith trait CanAuthotWith trait removed. Also all dependencies, parameters, type paramers were removed. This is related to removal of native runtime. * Remove commented code * Fix code formatting * trigger CI job * trigger CI job * trigger CI job * trigger CI job * trigger CI job * trigger CI job * trigger CI job
@@ -1036,16 +1027,6 @@ where | |||
create_inherent_data_providers: CIDP::InherentDataProviders, | |||
execution_context: ExecutionContext, | |||
) -> Result<(), Error<Block>> { | |||
if let Err(e) = self.can_author_with.can_author_with(&block_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoudn't there still be runtime API compatibility check before calling check_inherents_with_context
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runtime api should be compatible and this should always have returned Ok(())
for Polkadot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does aura verifier have this check then?
substrate/client/consensus/aura/src/import_queue.rs
Lines 229 to 235 in 3449fb0
if self | |
.client | |
.runtime_api() | |
.has_api_with::<dyn BlockBuilderApi<B>, _>( | |
&BlockId::Hash(parent_hash), | |
|v| v >= 2, | |
) |
The runtime api should be compatible and this should always have returned Ok(()) for Polkadot.
Except it did not when the state was not available. E.g. when importing historical blocks after the warp sync. These blocks are not executed at all and there's no parent state to call the runtime on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does aura verifier have this check then?
I mean it is compatible or better, we at least check and skip if it is not compatible. I don't see any problem there? Especially nothing that would have been solved by can_author_with
.
Except it did not when the state was not available. E.g. when importing historical blocks after the warp sync. These blocks are not executed at all and there's no parent state to call the runtime on.
This sounds like the bug, however I wonder how this worked before? Because can_author_with
should not have protected you against trying to call into a non available state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can_author_with
returns an error when there's no state. And the code above simply checked for any error. I'll make a PR to skip this check if the block is imported with no execution.
* Remove CanAuthorWith trait CanAuthotWith trait removed. Also all dependencies, parameters, type paramers were removed. This is related to removal of native runtime. * Remove commented code * Fix code formatting * trigger CI job * trigger CI job * trigger CI job * trigger CI job * trigger CI job * trigger CI job * trigger CI job
CanAuthotWith trait removed. Also all dependencies, parameters, type parameters were removed. This is related to removal of native runtime. (#10966)
polkadot companion: paritytech/polkadot#5986
cumulus companion: paritytech/cumulus#1609