Skip to content

Conversation

@JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Jan 20, 2021

What does it do?

Changes the inherent checking logic for the author inherent. It was previously "checking" the inherent as if the verifying node had made the inherent. It now checks the actual inherent that the actual author included.

What important points reviewers should know?

Docs for the ProvideInherent trait: https://crates.parity.io/sp_inherents/trait.ProvideInherent.html These docs are not great, and I've made an attempt to improve them in paritytech/substrate#7941

What alternative implementations were considered?

We considered not checking the inherent at all, but this would remove most of the usefulness of the stake pallet.

What value does it bring to the blockchain users?

It is possible to run full nodes.

Checklist

  • Does it require a purge of the network?
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ?

println!("{:?}", call);
}
// This if let should always be true. This is the only call that the inehrent could make.
if let Self::Call::set_author(claimed_author) = call {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why it is needed to set the author in the check.

Copy link
Contributor Author

@JoshOrndorff JoshOrndorff Jan 20, 2021

Choose a reason for hiding this comment

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

I'm pattern matching call, the parameter that was passed into the function, against the variants of the Self::Call enum. set_author is not actually a function and I'm not calling it. It is a variant of the Self::Call enum. This is hard to read because typically enum variants are written in ThisCaseStandard. But the Call enum is an exception because of how the FRAME macros work.

Copy link
Contributor

Choose a reason for hiding this comment

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

That might be worthy of a comment, although that's probably true of a lot of other cases...

@crystalin crystalin merged commit e57bc03 into master Jan 20, 2021
@crystalin crystalin deleted the joshy-parachain-full-nodes-verify-author-inherent branch January 20, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants