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

polkadot-parachain: Unify asset-hub authoring codepath with general AURA #4932

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Jul 2, 2024

Recently thought about the special handling we have for asset-hub chains. They started with relay chain consensus and transitioned to AURA at some point. However, nobody should be authoring with relay chain consensus on these chains anymore, the transition is long done.

I propose to remove this special handling, allowing us to unify one more execution path.

@skunert skunert added the T0-node This PR/Issue is related to the topic “node”. label Jul 2, 2024
@skunert skunert requested review from serban300 and a team July 2, 2024 18:34
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

If we do this, we should do it properly. Basically all the start_whatever_node can just be deleted and all of them can just use start_basic_lookahead_node.

@@ -635,109 +630,42 @@ where
para_id,
build_parachain_rpc_extensions::<RuntimeApi>,
Copy link
Member

Choose a reason for hiding this comment

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

This entire function is not required.

@serban300
Copy link
Contributor

serban300 commented Jul 3, 2024

If we do this, we should do it properly. Basically all the start_whatever_node can just be deleted and all of them can just use start_basic_lookahead_node.

If there are no concerns related to the conceptual change in this PR, I think it would be ok to merge it as it is.

I have a lot of cleanup related to start_whatever_node prepared in #4916 (I deleted all of those functions) and after this PR is merged, I can adapt #4916 accordingly

Copy link
Contributor

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the consensus logic. I would prefer if someone with more knowledge of this logic would take a look also. But apart from that, looks good to me.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

If @serban300 is getting rid of this in his next pr, we can continue.

@skunert skunert enabled auto-merge July 3, 2024 08:18
@skunert skunert added this pull request to the merge queue Jul 3, 2024
Merged via the queue into master with commit b31880c Jul 3, 2024
154 of 158 checks passed
@skunert skunert deleted the unify-aura-authoring branch July 3, 2024 09:51
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this pull request Jul 7, 2024
… AURA (paritytech#4932)

Recently thought about the special handling we have for asset-hub
chains. They started with relay chain consensus and transitioned to AURA
at some point. However, nobody should be authoring with relay chain
consensus on these chains anymore, the transition is long done.

I propose to remove this special handling, allowing us to unify one more
execution path.
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
… AURA (paritytech#4932)

Recently thought about the special handling we have for asset-hub
chains. They started with relay chain consensus and transitioned to AURA
at some point. However, nobody should be authoring with relay chain
consensus on these chains anymore, the transition is long done.

I propose to remove this special handling, allowing us to unify one more
execution path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

4 participants