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

statement-distribution: fix parachains stalling on async_backing enablement #3063

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Jan 25, 2024

Topology is coming only at the beginning of each session, so we might lose it if prospective parachains was not enabled at the begining of the session, so cache it for later use.

Fixes: #3058

…lement

Topology is coming only at the beginning of each session, so we might
lose if prospective parachains was not enabled at the begining of the
session, so cache it for later use

Fixes: #3058

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh alexggh added T8-polkadot This PR/Issue is related to/affects the Polkadot network. R0-silent Changes should not be mentioned in any release notes labels Jan 25, 2024
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Nice catch! Worth adding a test?

@ordian ordian requested a review from rphmeier January 25, 2024 13:09
@alexggh
Copy link
Contributor Author

alexggh commented Jan 25, 2024

Nice catch! Worth adding a test?

It is always worth adding a test :D, but I will do it async I don't think it makes sense to wait with this PR until I figure it out how to do that.

Note! This is tested with my hacked setup described here: #3058, but obviously I can't commit that :D.

@sandreim
Copy link
Contributor

It is always worth adding a test :D, but I will do it async I don't think it makes sense to wait with this PR until I figure it out how to do that.

Yes, we can't enable async backing on Polkadot without this fix, but I think we have plenty of time to implement the test.

Note! This is tested with my hacked setup described here: #3058, but obviously I can't commit that :D.

Just an idea, I imagine a unit test where we drive the overseer messages to do something similar with runtime api requests as you did in your setup. Then we'd just want to see that messages are sent according to new topology to start with.

@rphmeier
Copy link
Contributor

#761

Further discussion here, along with a more elegant proposed fix which pull the topology rather than having it pushed.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh
Copy link
Contributor Author

alexggh commented Jan 26, 2024

#761

Further discussion here, along with a more elegant proposed fix which pull the topology rather than having it pushed.

Will have a look, but I guess that is further down the line, so I will go ahead and merge this fix since it is better to have it present rather than not.

Just an idea, I imagine a unit test where we drive the overseer messages to do something similar with runtime api requests as you did in your setup. Then we'd just want to see that messages are sent according to new topology to start with.

Turns out it was easy to change the current tests to cover both cases.

@alexggh alexggh added this pull request to the merge queue Jan 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 26, 2024
@alexggh alexggh added this pull request to the merge queue Jan 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 26, 2024
@rphmeier
Copy link
Contributor

Will have a look, but I guess that is further down the line, so I will go ahead and merge this fix since it is better to have it present rather than not

Agreed! The fix is definitely necessary now but a new API for pulling topologies would prevent this footgun when building network protocol upgrades in the future.

@alexggh alexggh added this pull request to the merge queue Jan 29, 2024
Merged via the queue into master with commit 987edd8 Jan 29, 2024
125 of 126 checks passed
@alexggh alexggh deleted the alexaggh/parachains_stalling branch January 29, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kusama parachains not producing blocks for a session
5 participants