Fix inherents race condition

[nazar-pc]

Fixes #871, see #871 (comment) for the reason why this was happening.

Note that previously notification about importing block was technically happening AFTER block was imported (but before next block in import queue is imported, which wasn't the only way to get blocks imported), now it is happening BEFORE block is actually imported into Substrate.

So happy to finally understand what is going on there 😌

Code contributor checklist:

• I have read, understood and followed contributing guide

[NingLin-P]

Make sense overall! Plz also updates https://github.com/subspace/subspace/blob/1c9a7825267ecab281b97acbc9919f8b47d01645/test/subspace-test-service/src/lib.rs#L843 to reflect the change in domain tests.

[nazar-pc]

@NingLin-P looks like that broke some execution tests, could you look into it? I think there were some implicit assumptions about which blocks are already imported there.

[NingLin-P]

@NingLin-P looks like that broke some execution tests, could you look into it? I think there were some implicit assumptions about which blocks are already imported there.

Yeah, the domain tests broke due to some event ordering assumptions are broke. Before the changes, we made solid assumptions about the ordering of the following events and ensured the previous event must finish before next event (even though they are running is different workers concurrently) :

1. Produce slot, the operator construct/submit bundle.
2. Produce consensus block that includes the above produced bundle
3. Process the consensus block and derive a domain block
4. Produce the next slot...

With the change, event 3 can't guaranteed to be finished before the next slot, thus the operator may skip to produce bundle and further skip to produce domain block because there is no bundle in the consensus block.

For now, I don't have a better idea than to revert the change in the domain test 🙁

[nazar-pc]

2. Produce consensus block that includes the above produced bundle

This is not guaranteed to be the very next block.

3. Process the consensus block and derive a domain block

One of the next blocks will have that bundle, but there is no guarantee which one.

I think test was relying on fragine logic it should not really do that, it is not how things work in actual chain anyway.

[NingLin-P]

It is not fragile, the ordering is guaranteed in main, we have spent quite a lot of effort to make the assumption reliable and the test deterministic even under the slow github-host CI machine.

it is not how things work in actual chain anyway.

It is how the actual chain work in most case, where each step have seconds of interval to finish. But there are certainly a lot of scenarios not covered by the test because each domain test is focused on and dedicated to a specific case, while testing how the operator handles consensus chain fork we do not care about what happens if there is an empty consensus block in the new fork because empty consensus block is already tested in another test.

[nazar-pc]

There are several possible solutions here:

• make tests not rely on when bundle gets included (which means potentially waiting for extra blocks)
• tighter control over block production in test environment such that you do not produce blocks until necessary
• introduce another notification just for testing purposes at the old place in the workflow

I would prefer the first option for sure. While yes, it works in many cases on real chain the way you describe it, it doesn't always work like that.

[NingLin-P]

tighter control over block production in test environment such that you do not produce blocks until necessary

This is the exact current approach, the first option will make the test more verbose and complicated because in some tests we have assert about consensus/domain block number.

It is impossible to cover all the scenarios and the combination of them in the test (the number is exponential), so we just chose a relatively efficient way.

[nazar-pc]

This is the exact current approach, the first option will make the test more verbose and complicated because in some tests we have assert about consensus/domain block number.

It is impossible to cover all the scenarios and the combination of them in the test (the number is exponential), so we just chose a relatively efficient way.

Well, I'll be relying on you do to something about it I think.

[NingLin-P]

I have tested locally that the domain test is fixed with #2087, plz resolve the conflict.

[nazar-pc]

@NingLin-P one of the domains tests hanged apparently on Windows and macOS

[NingLin-P]

@NingLin-P one of the domains tests hanged apparently on Windows and macOS

I can't reproduce it on my macOS, I have re-run the CI job to see how it goes.

[nazar-pc]

Are you using nextest as well?

[NingLin-P]

Are you using nextest as well?

Yeah, created NingLin-P#8 to debug and see if it can be reproduced in github-host CI runner.

[nazar-pc]

@NingLin-P same test hanged on main here: https://github.com/subspace/subspace/actions/runs/6534587384/job/17742123483

[nazar-pc]

@NingLin-P I have pulled latest main into here, yet it still fails in CI. I think this is your PR now to finish, it would be difficult for me to do something about it.

[NingLin-P]

Sorry for the annoying force push, it is used to remove some commits that I brought to this PR accidentally, the last commit is the only change I pushed which should fix the CI ideally, PTAL.

[nazar-pc]

According to force-push you just updated some logs (removing a bunch of them from default levels actually). Not sure how it is supposed to fix anything.

UPD: Ah, there were other commit before that, okay...

[nazar-pc]

Some nits, let's see if CI passes