Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove parameterized-consensus-hook feature #4380

Merged

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

Closes #4366

@s0me0ne-unkn0wn s0me0ne-unkn0wn added R0-silent Changes should not be mentioned in any release notes T9-cumulus This PR/Issue is related to cumulus. labels May 5, 2024
@skunert
Copy link
Contributor

skunert commented May 6, 2024

I am trying to reason about the implications of these change.

After this PR is merged, all parachain runtimes using polkadot-sdk of next release need to supply a ConsensusHook. Since we have async backing enabled everywhere soon, this makes sense, but still a breaking change.

ConsensusHook Overview

  • We have ExpectParentIncluded. This is a workaround to make collators work that don't have parent_head included in their relay chain state proof. This was introduced in august 2023, I think we should deprecate this consensus hook in this PR. Longterm remove this logic here.
  • We have RequireParentIncluded and FixedCapacityUnincludedSegment. These don't perform any additional validation on the relay chain storage proof and set a fixed capacity to the unincluded segment. This one is still useful if you don't want the slot checking logic.
  • We have FixedVelocityConsensusHook which should be used for aura-based chains. It does slot validation logic and has a fixed unincluded segment length.

Which brings me to the deprecation of CheckInherents. Parachains were able to supply their own implementation of the trait. I am wondering if there are use cases where a parachain has custom inherents that they were able to validate using CheckInherents, but not using the ConsensusHook.

A quick github search shows that most chains are doing only slot checks that are now living in FixedVelocityConsensusHook. Usage of this seems in general very inconsistent, for example its not used in any of the system-parachains. @bkchr I remember we discussed briefly about the CheckInherents and its deprecation, do you remember the outcome?

@bkchr
Copy link
Member

bkchr commented May 6, 2024

@bkchr I remember we discussed briefly about the CheckInherents and its deprecation, do you remember the outcome?

The hook gets the StateProof passed. So, essentially the same as CheckInherents. Even if someone would have a custom implementation (which I doubt), they can still do the same checks as before. (Okay, a little bit harder if you need the transaction, but generally not impossible).

@skunert
Copy link
Contributor

skunert commented May 6, 2024

(Okay, a little bit harder if you need the transaction, but generally not impossible).

I mean that was the interesting part, I know that the state proof goes into the hook 👍 . But I agree that there are probably no custom implementations.

@s0me0ne-unkn0wn s0me0ne-unkn0wn marked this pull request as ready for review May 15, 2024 09:02
@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 15, 2024 09:03
@bkchr bkchr removed the R0-silent Changes should not be mentioned in any release notes label May 15, 2024
@s0me0ne-unkn0wn s0me0ne-unkn0wn added this pull request to the merge queue May 22, 2024
Merged via the queue into master with commit 04161b1 May 22, 2024
145 of 150 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove parameterized-consensus-hook feature
5 participants