Retry failed PVF prepare jobs

[mrcnski]

PULL REQUEST

Overview

• Keep track of the time of failure and the number of failures.
• If we get a request to prepare a PVF, retry if:
  • Enough time has elapsed since a failure, and
  • The number of failures is not greater than NUM_PREPARE_RETRIES

TODO

• How to test this?

Issues closed

Closes #4288

[mrcnski]

Figured we wanted the same logic for heads-up requests, but wanted to double-check.

[mrcnski]

I figured out how to test this, but I will wait until the general structure of the change is approved.

[mrcnski]

I had a conversation with @ordian about this change. He was wondering if we should retry for every PrepareError or if we could skip on e.g. Prevalidation errors, as a retry probably wouldn't fix that.

I am inclined to keep it simple and not add extra hacks. I think @pepyakin would say something similar, that this is cold code and so the performance for this case doesn't matter too much.

[slumber]

I had a conversation with @ordian about this change. He was wondering if we should retry for every PrepareError or if we could skip on e.g. Prevalidation errors, as a retry probably wouldn't fix that.

I am inclined to keep it simple and not add extra hacks. I think @pepyakin would say something similar, that this is cold code and so the performance for this case doesn't matter too much.

I wouldn't expect validators to retry compiling bad code, and I don't see a reason to call it a hack, it is a simple match on an error.

However, if the code is indeed malicious, then we should detect it during prechecking, submit invalid vote and reject it, thus no retries.

If it was prechecked before, no Prevalidation errors should happen. Which is again an external guarantee.

All in all, not so much difference

[mrcnski]

I wouldn't expect validators to retry compiling bad code, and I don't see a reason to call it a hack, it is a simple match on an error.

@slumber Fair enough, to me it just felt like a lot of logic for an edge case. But I can add it, with a note explaining that we don't expect to hit that match arm.

However, if the code is indeed malicious, then we should detect it during prechecking, submit invalid vote and reject it, thus no retries.

Gotcha. Can you please point me to where this happens in the code?

[slumber]

Gotcha. Can you please point me to where this happens in the code?

When the validation is requested, validator always fetches the code from the Runtime, either by hash or relay parent.
For example,

polkadot/node/core/candidate-validation/src/lib.rs

Lines 361 to 367 in e3f6731

	let validation_code = runtime_api_request(
	sender,
	descriptor.relay_parent,
	RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, code_tx),
	code_rx,
	)
	.await;

In order for the code to get stored in the Runtime storage, it should undergo prechecking process (it's not yet enabled but soon will be), this happens in the paras pallet (on upgrade or initialisation):

polkadot/runtime/parachains/src/paras/mod.rs

Lines 1770 to 1775 in e3f6731

	fn kick_off_pvf_check(
	cause: PvfCheckCause<T::BlockNumber>,
	code_hash: ValidationCodeHash,
	code: ValidationCode,
	cfg: &configuration::HostConfiguration<T::BlockNumber>,
	) -> Weight {

[pepyakin]

Nit: reaches soft wrap limit and no good reason to not wrap the line.

[pepyakin]

Nit: tautology. What is this error exactly? The last one I presume?

[pepyakin]

Nit: I wonder if it's time to move it under the match above? Then, let state would become let (state, num_failures) and/or waiting_for_response.

[mrcnski]

I gave it a try, but I wasn't able to work around the compiler errors when I do that.

[pepyakin]

Yeah, @ordian has a good point. Whenever the preparation worker returns an error gracefully, I expect it will be reproducible.

that this is cold code and so the performance for this case doesn't matter too much.

Actually, the code that does the bookkeeping of the PVF host is cold. The code that does prevalidation, compilation, etc is quite heavy (up to 60s) and can consume quite some memory. Moreover, it can be malicious.

[eskimor]

Nice, great work @m-cat !

[eskimor]

NIT: For preparation we actually agreed that retrying is not required/desired here. The pre-checking logic will not actually retry anyway, so this code will never get executed - so it is fine, but given that not doing it actually simplifies things, instead of complicating things (due to this complete separate implementation) - I would suggest to just not do it.

[eskimor]

Would also remove some code duplication.

[mrcnski]

Makes sense, removed.

[eskimor]

@slumber has a good point, but as he also argued: We do not retry during prechecking and after prechecking, malicious code can be ruled out. So I am fine with retrying in any case, especially given that we will only do so every 15 minutes.

[slumber]

Don't forget to add tests, please

[eskimor]

Nice work @m-cat !