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

Start PVF prechecking on finalized blocks not on active heads #4446

Closed
Tracked by #3211
eskimor opened this issue Dec 2, 2021 · 6 comments
Closed
Tracked by #3211

Start PVF prechecking on finalized blocks not on active heads #4446

eskimor opened this issue Dec 2, 2021 · 6 comments
Assignees
Labels
I2-security The node fails to follow expected, security-sensitive, behaviour.

Comments

@eskimor
Copy link
Member

eskimor commented Dec 2, 2021

This issue made me think: We currently use a current relay chain head to fetch the PVF on disputes for example. Unfortunately this could fail if the code upgrade was signaled in a block that has been reverted. It is very unlikely that this can happen as we are delaying code upgrades for at least a session, but it is not impossible.

By only starting the pre-checking on finalized blocks instead of on active heads, we can do away with that possibility entirely.

It also solves handling of multiple forks, where one contains the upgrade and another one does not in a very elegant way and thus reduces code complexity.

@rphmeier any reasons, why this might be a bad idea?

@eskimor eskimor added the I2-security The node fails to follow expected, security-sensitive, behaviour. label Dec 2, 2021
@burdges
Copy link
Contributor

burdges commented Dec 2, 2021

We do not currently throw an similar to unexpected epoch change in this situation?

@eskimor
Copy link
Member Author

eskimor commented Dec 2, 2021

The worst that could happen is that the runtime upgrade times out, if I am not mistaken.

@pepyakin
Copy link
Contributor

It also solves handling of multiple forks, where one contains the upgrade and another one does not in a very elegant way and thus reduces code complexity.

I spoke too soon here, it was only on glance. So actually it's the other way around.

@eskimor
Copy link
Member Author

eskimor commented Dec 28, 2021

Actually the issue is different than I was describing it. We need runtime enactment to be delayed significantly or wait for the code including block to be finalized because we keep track of the most recent head for fetching runtime code when needed.

On chain reversion, we won't update that head (because the new heads have a lower block number) - so we would still have access to any runtime upgrade containing block that just has been reverted, but we would not have access to any runtime upgrade that happened on the new chain currently being built. That is, until it's block number surpassed the number of the most recent head we had previously, only then will we use the new chain for fetching the code. If the runtime upgrade would have been enacted earlier, approval checking and disputes would break.

@rphmeier
Copy link
Contributor

rphmeier commented Jan 5, 2022

@eskimor We already have defenses against that:

Announced upgrades are applied only after a delay. I think in practice this is around 1 hour. So any reversion less than 1 hour will still have the code available as long as pending code is accessible by hash, which I believe it is. Framed another way: if there's a block that's disputed it must have some ancestor B that is at least 1 hour old where the code was initially posted on-chain. Whatever our 'best block' in the disputes subsystem is, it's exceedingly likely to have B as an ancestor as well. Furthermore, old code is kept on-chain for a long time as well which would help prevent this issue.

@eskimor
Copy link
Member Author

eskimor commented Jan 5, 2022

Thanks @rphmeier ! @pepyakin and I already discussed and we agreed that probabilistic finality is good enough. Let's close this - it kind of asks for food (attention) @pepyakin ;-)

@eskimor eskimor closed this as completed Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I2-security The node fails to follow expected, security-sensitive, behaviour.
Projects
None yet
Development

No branches or pull requests

4 participants