Rework dispute-coordinator to use RuntimeInfo for obtaining session information instead of RollingSessionWindow

[tdimitrov]

Part of #6812

The PR contains two notable changes:

1. Remove RollingSessionInfo usage from dispute-coordinator
2. Simplify disputes spam slots pruning logic - now they are cleared after a 6 session window passes. The window is no longer extended up to the session whereh the last finalized block was produced.

To use RuntimeInfo instead of RollingSessionWindow two problems has to be solved:

1. Each RuntimeInfo call needs a sender and a parent_hash for querying the runtime. The first one seems to be always available. The situation with the second one is more complicated.
2. RollingSessionWindow provides two methods - earliest_sesson and latest_session. These are the first and the last session cached. RuntimeInfo hasn't got such concept so it needs to be emulated (or dispute-coordinator to be reworked) somehow.

Problem 1 is mainly caused by the way dispute-coordinator is initialized. It gets the first active leaf as an Option and two Vecs with scraped onchain votes and participations. On further investigation I realised that these parameters are used only on startup and then they are empty. The Vecs are drain-ed and the Option is takeen. So if the subsytem crashes and gets restarted - it starts up with empty data.

For this reason I introduce struct InitialData containing all three of them:

pub struct InitialData {
	pub participations: Vec<(ParticipationPriority, ParticipationRequest)>,
	pub votes: Vec<ScrapedOnChainVotes>,
	pub leaf: ActivatedLeaf,
}

It is passed to the dispute-coordinator via Option. So if there is some initialization data (which requires runtime calls) we have got the leaf used to fetch it and we can use it. If there is no initialization data - we are good.

Problem 2 is more straightforward. dispute-coordinator will keep track of the sessions by itself. I made one simplification here which I'm not sure is correct (see notable change 2).

A few words about caching, which I have misunderstood initially.
RuntimeInfo has its own cache for SessionInfo (and doesn't solely rely on the ruintime-api subsystem cache for runtime calls as I wrongly assumed). This has got a nice side effect. For example in this call:

polkadot/node/subsystem-util/src/runtime/mod.rs

Line 153 in 27ddd27

self.get_session_info_by_index(sender, relay_parent, session_index).await

If the session with index session_index is in cache the relay_parent parameter doesn't matter as no runtime call is made.
This is an improvement over the runtime-api cache where the results for different relay parents are treated as separate keys.

TODOs:

• Fix tests
• Burn-in on versi

[eskimor]

Looks good overall. We can maximize robustness (left comments) by using different relay_parents in pre-filling and actual use where possible. This way if at least one of them works, we are good, which means maximum robustness against errors. (Pruning, migration problems, ..)

[BradleyOlson64]

Looks good to me. Getting rid of the rolling session window demystifies session logic a lot! Much appreciated.

[eskimor]

This is an improvement over the runtime-api cache where the results for different relay parents are treated as separate keys.

This is not true. runtime-api cache caches session info also only by session index. So you have a valid point, that making the LRU size 6 is not strictly necessary. I think it is a good idea regardless, as we have better control and more importantly a guarantee*) that the last 6 sessions are indeed cached and not already pruned.

*) In the absence of errors.

[tdimitrov]

This is not true. runtime-api cache caches session info also only by session index.

I haven't noticed that SessionInfo is handled more specially than the rest of the calls in runtime-api subsystem. Lesson learned: Runtime calls are treated differently and making assumptions is a mistake.

think it is a good idea regardless, as we have better control and more importantly a guarantee*) that the last 6 sessions are indeed cached and not already pruned.

I agree, I've modified it already.

[ordian]

This is an improvement over the runtime-api cache where the results for different relay parents are treated as separate keys.

If you look at runtime-api cache for session info, it actually ignores relay parent:

polkadot/node/core/runtime-api/src/lib.rs

Line 137 in ecad912

SessionInfo(_relay_parent, session_index, info) =>

[tdimitrov]

This is an improvement over the runtime-api cache where the results for different relay parents are treated as separate keys.

If you look at runtime-api cache for session info, it actually ignores relay parent:

polkadot/node/core/runtime-api/src/lib.rs

Line 137 in ecad912

SessionInfo(_relay_parent, session_index, info) =>

That's true. I think @eskimor mentioned that somewhere but I had forgotten to edit the description.

[tdimitrov]

Suggested change

	match session_idx {
	Ok(session_idx)
	if self.last_consecutive_cached_session.is_none() ||
	session_idx >
	self.last_consecutive_cached_session.expect(
	"The first clause explicitly handles `None` case. qed.",
	) =>
	let should_cache_session = |session_idx: &SessionIndex| {
	self.last_consecutive_cached_session.is_none() ||
	session_idx >
	&self
	.last_consecutive_cached_session
	.expect("The first clause explicitly handles `None` case. qed.")
	};
	match session_idx {
	Ok(session_idx) if should_cache_session(&session_idx) => {

Is it more readable to extract the check as a closure?

[sandreim]

IMO, no :)

[sandreim]

IMO, no :)

[sandreim]

What does last_consecutive_cached_session stand for exactly ? For example if we have this this situation: [ fail ok, ok, ok, ok] ?

[tdimitrov]

We want the last DISPUTE_WINDOW sessions cached. IF we have to cache sessions 1..5 and for some reason 3 fails, on next ActiveLeaves update we want to retry fetching it and start caching from 3. So in this case last_consecutive_cached_session will be set to 2.
Regarding your question: [ fail ok, ok, ok, ok]. If the failed session is with index X, then last_consecutive_cached_session will be set to x-1.

[tdimitrov]

I'll do one final burn in next week and merge.

[eskimor]

@tdimitrov can we merge? I will likely have to touch some dispute-coordinator code soon, would be good to have this merged first.

[tdimitrov]

I wanted to do one more burnin but the new changes are small - I'd say it's safe to merge.
I'll fix the pipeline and we can merge.

[eskimor]

bot merge