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

Replace RollingSessionWindow with existing RuntimeInfo cache #6812

Closed
eskimor opened this issue Mar 2, 2023 · 5 comments
Closed

Replace RollingSessionWindow with existing RuntimeInfo cache #6812

eskimor opened this issue Mar 2, 2023 · 5 comments
Assignees

Comments

@eskimor
Copy link
Member

eskimor commented Mar 2, 2023

RollingSessionWindow keeps causing issues.

Instead of just requesting (and caching) sessions based on a given relay parent (usually of some candidate), we just grow a session window based on active leaves updates. This mechanism proved to be less reliable. In particular for example on startup we will be requesting the older sessions of the window based on a head that is in a much younger session. While in theory this should work, it does not always because of runtime upgrades and migrations/timing (bugs).

Contrary, with the classic approach of requesting sessions based on a relay parent in that very session, we don't even need the runtime to support queries of historic sessions. The only downside is that this only works for non finalized blocks, but we also only care for non-finalized blocks. In addition the query will very likely be cached by the runtime-api subsystem so it usually will even work for already finalized blocks.

Another reason the rolling session window is more fragile is that the window needs to be continuous, it does not support holes - hence if a single session fetch fails, none will be retrievable -> it will not try fetching more recent sessions (which are usually more relevant) if fetching of an older one fails for some reason.

In order to make sure the cache is populated in case we need it in disputes, we could increase the lru cache size of RuntimeInfo to 6 and just query the current SessionInfo from the cache on each active leaf update. This way we would keep the behavior of pro-actively caching sessions, to boost reliability even more (at the cost of some constant overhead).

@tdimitrov tdimitrov self-assigned this Mar 9, 2023
@tdimitrov
Copy link
Contributor

Browsing the RollingSessionWindow code I saw this:

// TODO: https://github.com/paritytech/polkadot/issues/6144

#6144 and this issue are related.

@sandreim
Copy link
Contributor

sandreim commented Mar 9, 2023

We can close that one as well if we remove it :P

@tdimitrov
Copy link
Contributor

Small update on why this is taking so much time.

#6968 removes RollingSessionWindow from dispute-coordinator. I'll do one final burn-in and it can be merged.

I tried to apply the same approach to approval-voting but it's not looking good there. session_window is kept in State which is intended to be immutable. Putting RuntimeInfo there will break its immutability because each session look up might perform caching as a side effect and requires &mut self.

On top of it a lot of functions should be made async and should accept additional SubsystemSender parameter. This as big problem as the immutability but I want to avoid changing the subsystem too much (if possible).

My plan for now is to add yet another session cache to State which keeps its immutability. It will be updated only during leaf updates and if we fail to cache a session there it will be considered unavailable in the rest of the code. This looks fine on first sight but also I might be missing something.

@eskimor
Copy link
Member Author

eskimor commented Apr 18, 2023

No, I really don't like that. The issues with immutability should be fixable. A simple solution could be for example to just no longer put it in the state, but keep it separate. With a little refactor better solutions should be possible as well, but I understand if you don't want to touch to much for this task.

@tdimitrov
Copy link
Contributor

With a little refactor better solutions should be possible as well, but I understand if you don't want to touch to much for this task.

No, if it's fine to rework the code a little bit to fit in the RuntimeInfo - I'll do it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants