Refactor check_validation_outputs

[Lldenaurois]

A particular check that ensures the backed_candidate's persisted validation hash matches the expected hash (fetched via the util function fn make_persisted_validation_data) was not included the fn verify_backed_candidate function and was present higher in the call-stack instead.

However, this logic needs to be called in the fn create_inherent_inner function but was incorrectly ommitted from this inner function. In order to reconcile this discrepancy we must move the persisted validation hash verification into the fn verify_backed_candidate function, which ensures this logic is called in both fn process_candidates and fn create_inherent_inner as desired.

[rphmeier]

This is true in my branch but not here

[drahnr]

@rphmeier is there a particular reason we are discarding all other candidates too just rather than discarding the one with an invalid parachain? We could as well just continue there. Another Q, we do the same filtering as the block producer, so I don't see a particular reason for the special handling and introducing Result<Result<_>>.

[rphmeier]

@drahnr

is there a particular reason we are discarding all other candidates too just rather than discarding the one with an invalid parachain?

We have 2 places with use of check_validation_outputs. The first is in inclusion and is logic that runs on-chain. The second is in filtering and runs off-chain.

In inclusion, it has always been the case to not silently ignore invalid backed candidates, and this PR simply continues that behavior. The logic of process_backed_candidates, as far as I can see, is exactly the same as before.

From my reading of this PR, the usage in filtering is as a predicate in sanitize_backed_candidates and so should only skip candidates that fail the predicate.

Another Q, we do the same filtering as the block producer, so I don't see a particular reason for the special handling and introducing Result<Result<_>>

The Result<Result<_>> is not particularly relevant in filtering but is used on-chain process_candidates to preserve the behavior that a failure in constructing the PersistedValidationData in process_candidates leads to the silent ignoring of all candidates. This is an internal error code path which should really never be hit (as the comment above it explains) and the changes here preserve previous behavior.

I hope this is helpful / sane.

[drahnr]

I don't think this is correct, we'd want to filter out all of them, not only the outer error. Missing a para chain header is also a good enough reason to drop the candidate iiuc

[rphmeier]

I'm having a hard time connecting your comment to the code here. Can you explain in more detail?

[drahnr]

We are now only considering the outer error of the nested result, I'd argue that an error of the inner result type is also a good enough reason to discard the backed candidate iiuc. This can be deferred to a follow up PR, since as you correctly stated, behavior does not change compared to current master

[rphmeier]

I'd argue that an error of the inner result type is also a good enough reason to discard the backed candidate iiuc

Ok, fair. We should probably discard all backed candidates when encountering this error. It means something is seriously wrong and governance will probably need to step in. The on-chain logic is to ignore all the candidates anyway for the same reasons.

[eskimor]

I assume we can change that in the next release/new PR.

[drahnr]

I agree that fn process_candidates has the same behavior as before, yet it's not really in-line with what we want to do. Further discussions to be had about this, the change LGTM, besides one minor thing I just added.