Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

advance parachain properly in XCM-emulator for tests #3023

Closed
wants to merge 2 commits into from

Conversation

rphmeier
Copy link
Contributor

This is cherry-picked from #2300 - we needed to treat the parachain as an actual chain for the unincluded segment logic introduced in #2948 .

This PR has test failures which need to be fixed by the system parachains folks

@@ -622,14 +626,16 @@ macro_rules! decl_test_parachains {
let block_number = <Self as Chain>::System::block_number();
let mut relay_block_number = <Self as NetworkComponent>::Network::relay_block_number();

let _ = <Self as Parachain>::ParachainSystem::set_validation_data(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling set_validation_data here is definitely illegal so had to be removed. Tests possibly depended on it, but it lead to an impossible sequence of calls.

Copy link
Contributor Author

@rphmeier rphmeier Aug 16, 2023

Choose a reason for hiding this comment

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

elaborating a bit more. init() is called within execute_block the first time it's run.

set_validation_data needs to be followed by a System::finalize call.
It might be more correct as

  1. initialize
  2. set_validation_data
  3. finalize
  4. initialize

but that is basically just running a full block, which shouldn't be necessary during the initialization step.

@joepetrowski joepetrowski added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 16, 2023
@rphmeier
Copy link
Contributor Author

related to #2703

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3403396

@rphmeier
Copy link
Contributor Author

rphmeier commented Aug 16, 2023

btw, one of the reasons these tests are so flaky is because the execute_with function for every parachain invokes Network::process_messages() which side-channel dumps messages into every other parachain as well as itself.

The side-effecting nature of the code here would likely make it near-impossible to write tests which didn't depend on highly specific implementation details and control flow of the test runner, as well as the ordering of calls to execute_with for different parachains, which is why they fail every time we so much as look at this code.

Strongly recommend a refactoring here. Executing a block on a parachain should cause messages to be processed only to itself. This may require tests to be rewritten to deliberately execute empty blocks on a receiving parachain in order to observe the events - this is OK and what happens in the real world.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants