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 ProspectiveParachainsMode usage in backing subsystem #6215

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

tdimitrov
Copy link
Contributor

@tdimitrov tdimitrov commented Oct 24, 2024

Since async backing parameters runtime api is released on all networks the code in backing subsystem can be simplified by removing the usages of ProspectiveParachainsMode and keeping only the branches of the code under ProspectiveParachainsMode::Enabled.

The PR does that and reworks the tests in mod.rs to use async backing. It's a preparation for #5079

@tdimitrov tdimitrov added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Oct 24, 2024

// Test that the candidate reaches quorum successfully.
#[test]
fn backing_works() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is completely removed since it overlaps with backing_works from mod.rs

polkadot/node/subsystem-util/src/backing_implicit_view.rs Outdated Show resolved Hide resolved
polkadot/statement-table/src/generic.rs Outdated Show resolved Hide resolved
polkadot/statement-table/src/generic.rs Show resolved Hide resolved
polkadot/node/core/backing/src/lib.rs Show resolved Hide resolved
polkadot/node/core/backing/src/lib.rs Outdated Show resolved Hide resolved
}
responses.push_back(futures::future::ok((true, head)).boxed());
}
for head in implicit_view.all_allowed_relay_parents() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we only need to do this for active leaves. We only care if we can back something on an active leaf. Prospective parachains will take the possible ancestry into account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I should use leaves() instead. Fixed.

Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

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

Are you going to remove ProspectiveParachainsMode by subsystem and then the enum inself?


// Check that subsystem job issues a request for the availability cores.
async fn assert_validate_from_exhaustive(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It might be more convenient to wrap assertions in macros instead of functions. If nothing has changed recently, failed tests with functions point to the line in the function itself rather than the specific line in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point but the macros add additional complexity. You can also set RUST_BACKTRACE=1, get a stack trace and see where exactly the assert function has failed.

@tdimitrov
Copy link
Contributor Author

Are you going to remove ProspectiveParachainsMode by subsystem and then the enum inself?

Actually I want to remove AsyncBackingParamters (#5079) but removing ProspectiveParachainsMode beforehand simplifies the code and the work so I extracted it as a separate PR.

I plan to do exactly what you suggested but after I am done with AsyncBackingParamters removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants