Include a reference to the validation data in the candidate descriptor

[rphmeier]

While implementing the candidate validation subsystem in #1432, one of the deeper issues that's gone mostly unnoticed finally bubbled up to the surface.

The wrinkle started with this question: "How does the CandidateBackingSubsystem know which OccupiedCoreAssumption to make?". From the existing CandidateDescriptor struct, it wasn't clear how you were supposed to tell, once given a candidate, whether it assumed that the occupied core at the head of the chain had timed out or was free. Ignoring the case when the para was not occupying any core, as there is no choice on parameters.

There was also another issue floating around my head for a bit: if we want to eventually report bad candidates to the chain which were never actually included, we'll need the candidate descriptor to cryptographically reference all parameters to the validation function.

So this change satisfies both of those issues. I'm planning to answer the first question with a little bit of a side-step by doing it in CandidateValidation instead. What we'll do is be aware of the corresponding validation_data_hash for each of the two choices (TimedOut and Included) and compare against the validation_data_hash of the descriptor we've been asked to validate. If it's neither of those, then we'll return a ValidationFailed variant.

We might also return a ValidationResult::Invalid, although it is unclear how the dispute would actually resolve without heavily depending on state of the relay_parent remaining available - if the validation_data_hash references valid parameters to the validation function, just not those that are expected at the relay_parent, then as soon as the state data goes out of scope for the relay_parent on most validators, how can you tell if the candidate is indeed good or bad?

[rphmeier]

What we'll do is be aware of the corresponding validation_data_hash for each of the two choices (TimedOut and Included) and compare against the validation_data_hash of the descriptor we've been asked to validate. If it's neither of those, then we'll return a ValidationFailed variant.

I expanded on this in 4530c51

[coriolinus]

These numeric literals feel ripe for extraction into constants. Alternately, would SCALE do the right thing with (relay_parent.as_ref(), validation_data_hash.as_ref(), pov_hash.as_ref()).encode()?

[rphmeier]

It would, and maybe this is premature optimization, but I wanted to do this on the stack as this is called within the runtime and allocations might be expensive.

[rphmeier]

Why specifically would constants help here?

[rphmeier]

(let's address this as follow-on, since the status quo is "no constants")

[coriolinus]

Working on the stack is a plausible reason to copy from slices instead of using higher-level methods.

Constants help because they make typos into compiler errors. Contrast these approaches:

payload[0..32].copy_from_slice(relay_parent.as_ref());
u32::from(*para_id).using_encoded(|s| payload[32..32 + s.len()].copy_from_slice(s));
payload[36..68].copy_from_slice(pov_hash.as_ref());
payload[36..68].copy_from_slice(validation_data_hash.as_ref());
payload[66..100].copy_from_slice(pov_hash.as_ref()); // subtle bug which will panic

payload[0..PARA_ID].copy_from_slice(relay_parent.as_ref());
u32::from(*para_id).using_encoded(|s| payload[PARA_ID..PARA_ID + s.len()].copy_from_slice(s));
payload[POV_HASH..VALIDATION_DATA_HASH].copy_from_slice(pov_hash.as_ref());
payload[VALIDATION_DATA_HASH..POV_HASH].copy_from_slice(validation_data_hash.as_ref());
payload[POV_HASH..FINAL_SIZE].copy_from_slice(pov_hash.as_ref());

A reasonable test suite would catch the panic in this case, but that's not true in every situation. It just feels like a good practice to name applicable numeric constants and magic numbers instead of typing them by hand all the time.

[rphmeier]

You could also just as easily mess up the definition of POV_HASH. It doesn't guarantee anything except that when an offset is used twice, it's not mis-typed the second time.

I am in favor of extracting literals to constants when they are not hyper-localized. If there is a constant that an entire module needs to make use of, then it doesn't make sense to use a literal. But this is hyper-localized code within a single function that is less than 10 lines of implementation.

[drahnr]

LGTM, @coriolinus already covered all major points, everything else is nitpicks from my side.