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

Include PVF execution and preparation timeouts into executor environment parameters #6793

Closed
s0me0ne-unkn0wn opened this issue Feb 27, 2023 · 6 comments · Fixed by #6823
Closed
Assignees
Labels
I3-bug Fails to follow expected behavior. T3-relay_chain This PR/Issue is related to the relay chain. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes.

Comments

@s0me0ne-unkn0wn
Copy link
Contributor

PVF timeouts are crucial for determining candidate validity, and changing a timeout throughout the network may retrospectively affect the validity of already included candidates. Thus, such changes must be effective per session, not per network. Execution environment parameters are a natural place to keep track of timeout changes for the dispute period.

@s0me0ne-unkn0wn s0me0ne-unkn0wn added the I3-bug Fails to follow expected behavior. label Feb 27, 2023
@s0me0ne-unkn0wn s0me0ne-unkn0wn self-assigned this Feb 27, 2023
@ordian ordian added T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes. T3-relay_chain This PR/Issue is related to the relay chain. labels Feb 27, 2023
@eskimor
Copy link
Member

eskimor commented Feb 27, 2023

We do have three timeouts which relate to each other:

  1. Timeout for collators.
  2. Timeout for backers.
  3. Timeout for approval checkers.

Timeout for backers has to be larger than the one for collators to make it very unlikely for them to reject a valid block.

Timeout for approval checkers needs to be way larger than the timeout for backing to avoid false disputes. We currently have 6x backing timeout here.

The latter is more critical than the previous one, so a smaller factor between timeout for collators to timeout for backers can be justified. Right now it is 4x.

@rphmeier
Copy link
Contributor

rphmeier commented Feb 28, 2023

Timeouts for collators live in Cumulus in practice now, and IMO that is the right place for them: either Cumulus or whatever other collation binary may be created. It's the responsibility of the collators to not create anything that backers will reject, while the backers will reject according to rules set on the relay chain. It makes the most sense for Cumulus to make use of the backing timeout and adjust based on the specifics of the parachain's block authoring mechanism.

@eskimor
Copy link
Member

eskimor commented Mar 3, 2023

@s0me0ne-unkn0wn let's prioritize this a bit. For fully utilizing async backing benefits we need that to be configurable, so we can bump it up in a secure way. Given that we don't have time disputes yet, we could cowboy it, but better if we had a proper configuration in place.

@eskimor
Copy link
Member

eskimor commented Mar 3, 2023

As @rphmeier we don't care about the collator timeout, but for #paritytech/cumulus/2252 we will need that dedicated collator API for fetching execution environment parameters.

@s0me0ne-unkn0wn
Copy link
Contributor Author

Yeah, I also think it should be implemented asap.

As for executor environments in collators, did you see this one? #6497 (comment)

@eskimor
Copy link
Member

eskimor commented Mar 3, 2023

Yeah, I also think it should be implemented asap.

As for executor environments in collators, did you see this one? #6497 (comment)

Thanks - No I did not. I realized this was mentioning PVFs instead of PoVs, which is what I was concerned about.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I3-bug Fails to follow expected behavior. T3-relay_chain This PR/Issue is related to the relay chain. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants