Backers retry on failed candidate validation

[Overkillus]

Loosely connected to #1434 where the potential issue was first recognised.

Backers seem to retry failed validation (once per error type). In general it would be better to have backing stricter than approval voting / disputes. This is currently the case for timeouts but another aspect should be respecting transient errors and not doing any retries in in the backing stage. This reduces the chance of nondeterministic blocks getting backed and honest backers getting slashed.

Call path in question:
polkadot-sdk/polkadot/node/core/backing/src/lib.rs always delegates backing validation to fn request_candidate_validation which in turn send a message CandidateValidationMessage::ValidateFromExhaustive. This particular message triggers a fn validate_candidate_exhaustive which leads directly to fn validate_candidate_with_retry. Backing uses the validation implementation with a retry instead of the fn validate_candidate.

fn validate_candidate_with_retry allows the validation to be retried in cases of:

• completely invalid candidate
• ambiguous worker death
• internal error

Each case can be retried once. So it is possible that multiple retires will be attempted.

This can be somewhat easily amended by passing extra details in the message and changing the behaviour in candidate validation in case of backing validation. The new behaviour will simply refer to the non retry version of the func.

[Overkillus]

@eskimor if you could take a look if we want to go with it

[burdges]

Is there any reason backers should retry under any circumstances? I'd think they could just report the error and move on, either the operator or the parachain can then fix the error.

[burdges]

Invalid candidates maybe interesting if we want the backer to post a dispute, but this could be handled by making those backers act like normal approval checkers on candidates which they failed the first time. I doubt this matters either way honestly since it's a negligible number of nodes.

[s0me0ne-unkn0wn]

"Can be easily amended" does not work well with "passing extra details in the message" 😅 Adding a new field to the validation message is a rather big pain in the ass. See #1190 for an example, ~90% of code in that PR is about adding a field and fixing all the tests (that entire approach requires some refactoring for sure, but definitely not in the scope of this issue).

But indeed I believe you don't need a new field. The ValidateCandidateFromExhaustive has the PvfExecTimeoutKind field, it's either Backing or Approval, so you can find out what job is supposed to be done. Yes, it looks like a misuse of the field, but I believe it's better to, say, rename PvfExecTimeoutKind into something more general than to introduce a new one.

[Overkillus]

"Can be easily amended" does not work well with "passing extra details in the message" 😅

Didn't realise it would be such a hassle.

But indeed I believe you don't need a new field. The ValidateCandidateFromExhaustive has the PvfExecTimeoutKind field, it's either Backing or Approval

I've seen the timeout field but didn't want to go this route as it didn't seem particularly clean. Maybe renaming that is inded a good middle-ground 👍

[eskimor]

Renaming and making it more general purpose should be the way to go. Separation of concerns should be maintained. So instead of telling candidate validation that we want this validation for backing/approval/dispute/... we should tell it:

Do the validation with this timeout, this priority and don't do any retries.