Fix core sharing and make use of scheduling_lookahead

[tdimitrov]

Implements most of #1797

Core sharing (two parachains or more marachains scheduled on the same core with the same PartsOf57600 value) was not working correctly. The expected behaviour is to have Backed and Included event in each block for the paras sharing the core and the paras should take turns. E.g. for two cores we expect: Backed(a); Included(a)+Backed(b); Included(b)+Backed(a); etc. Instead of this each block contains just one event and there are a lot of gaps (blocks w/o events) during the session.

Core sharing should also work when collators are building collations ahead of time

TODOs:

• Add a zombienet test verifying that the behaviour mentioned above works.
• prdoc

[eskimor]

Ok. I don't get how this code fixes anything. The only place that is now fully taking advantage of the claim queue is prospective parachains, but that should not help much if we only ever provide collations for claim queue depth 0.

What do I miss?

[eskimor]

Ok. It likely works because max_ancestry_len is long enough, that your collation will still be valid on the next occurrence in the claim queue. This is why the change in prospective parachains has an effect.

[alindima]

that your collation will still be valid on the next occurrence in the claim queue. This is why the change in prospective parachains has an effect.

Correct. Still, we don't seem to need an allowed_ancestry_len larger than 2. I tested with this value and 4 parachains sharing a core with 1.8 second candidates and it works in zombienet.

@eskimor at this moment, we don't have any collator that builds collations for more than one relay chain block in advance (lookahead and slot-based don't do this). So adding any change here that does take into account the full claim queue will not have any effect.

Collators search for an availability core where the para id is scheduled at this block or at the very next block and it builds max_candidate_depth + 1 candidates.

That's why the fixes in this PR (besides the one in prospective-parachains) don't need the claim queue (and can do with the next_up_on_available value).
The fixes in collator-protocol (both validator and collator sides) work because we were previously not looking into next_up_on_available at all. We were using the paraid of the already occupied core.

The only place where the claimqueue makes a difference is indeed in prospective-parachains. There, we need to not drop candidates for a para which are not scheduled for the next core but may be scheduled for the one after that.

[alindima]

discussed offline with @eskimor about this. The PR was so far fixing core sharing but not implementing the required changes for collators to be able to build collations ahead of time.
So while it is working with the current collators, we want to have collators that are able to build collations before they're scheduled (particularly for useful for on demand). I'll make these changes

[eskimor]

We need to ensure fairness between paras. This can be a followup though.

[eskimor]

Not introduced here, but would be good to clear up. This function is called core_index_from_statement ... I would not expect it to do any validation. Do we have a better place to do this check?

[alindima]

I though about this and we could move this bit of validation to the caller, but I think that hurts readability. core_index_from_statement is already doing several validation steps

[eskimor]

Note: This limit is based on max_candidate_depth, but is not per para (despite the docs suggesting so). Maybe we want a global (for all paras) limit still, but it certainly is not enough. A global limit only protects the validator but not parachains from each other.

For the latter we need to ensure fairness in collation fetching:

1. paras should get fetches proportional to how often they occur in the claim queue. E.g for a claim queue [A,B,A], A can have twice as many fetches as B.
2. If for two paras the weighted fetch count (actual fetches/claim queue occurrences) is equal, then prefer the para that is higher up in the claim queue as it is more urgent.

[alindima]

Yes, I decided to leave the fairness bit as a follow-up.

I have some thoughts on this, I'll add them to #1797

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6477941

[eskimor]

Why do we need this pub? I mean as long as we are not keeping any invariants via the wrapper, I don't see a problem, but is it needed?

[alindima]

there aren't any invariants kept. and you suggested using the vecdeque directly: #4724 (comment)
instead of adding more methods that look similar to each other I decided there's no reason why we can't expose its inner type.

[alexggh]

Nit: Alternatively you could use: derive_more::Deref for read only access

[alexggh]

With this high timeouts and low block heights, aren't we at a high-risk of the test working accidentally.

[alindima]

each parachain should get one block every 4 relay chain blocks. so one in 24 seconds (which would amount to about 144 seconds). An extra minute is needed for the session change (we're registering the parachains manually, because zombienet would otherwise also assign cores). Just to be sure, I ran this test with the code on master and it fails

[alexggh]

Alright makes, sense then the timeouts aren't that high. Thank you!

[eskimor]

Not for this PR, but once we got rid of the legacy stuff, I think we should make e.g. this iterator accessible. Should enable us to be able to get rid of this parameter as well (and the fetching of the cores to begin with).

[eskimor]

Thank you! Great work!