Update guide candidate validation module#1264
Conversation
coriolinus
added
A3-in_progress
|
It looks like @coriolinus signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
coriolinus
added
A0-please_review
|
This is pretty brief, but Candidate Validation isn't all that complicated. There are some TODOs which need to be addressed, but the answers should hopefully be straightforward. |
| ## Functionality | ||
|
|
||
| Given a candidate, its validation code, and its PoV, determine whether the candidate is valid. There are a few different situations this code will be called in, and this will lead to variance in where the parameters originate. Determining the parameters is beyond the scope of this subsystem. | ||
| Given the hashes of a relay parent and a parachain candidate block, and either its PoV or the information with which to retrieve the PoV from the network, spawn a short-lived async job to determine whether the candidate is valid. |
There was a problem hiding this comment.
The old line said:
Determining the parameters is beyond the scope of this subsystem.
I find this vastly superior to a system where the validation subsystem is responsible for fetching the PoV. The caller will know best where to get the PoV.
There was a problem hiding this comment.
I didn't know what "determining the parameters" meant, in the old line.
If the caller is responsible for fetching the PoV, then what is this subsystem supposed to do when it receives a CandidateValidationMessage::Validate with PoVOrigin::Network? If it's the caller's responsibility, then we don't need PoVOrigin at all; we can just make the PoV one of Validate's fields.
There was a problem hiding this comment.
If it's the caller's responsibility, then we don't need PoVOrigin at all; we can just make the PoV one of Validate's fields
Yeah, this is what I'd advocate for.
There was a problem hiding this comment.
OK, I can make that change.
|
|
||
| Each job follows this process: | ||
|
|
||
| - on `PoVOrigin::Network`, send a `QueryPoV` request to the [Availability Store](/node/utility/availability-store.html). |
There was a problem hiding this comment.
Based on other review comments, I still think this shouldn't be a thing. But why would this help?
There was a problem hiding this comment.
Perhaps I'm confused, but my understanding is that:
- in order to validate a parachain block, you need two ingredients: the block itself, and its PoV
- this subsystem has ultimate responsibility for validating parachain blocks
In other words, it's a caller-friendliness thing: if the caller already has the PoV, it can embed that in PoVOrigin. If it just has the CandidateReceipt, then this module will handle the fetch. The assumption is that we may want to validate candidates from more than one different subsystem, given only the CandidateReceipt; by giving Candidate Validation the responsibility to get the PoV in that case, we avoid duplicating that code.
There was a problem hiding this comment.
Right. The availability store is not networked, though. It just stores data. So if you don't already have all the pieces you'd need, then it couldn't return the candidate.
The availability fetching subsystem hasn't been described yet
There was a problem hiding this comment.
That's my misunderstanding about the nature of the availability store, then. ac6fabe contains a brief sketch of what candidate fetch might look like.
| - Get the full candidate from the current relay chain state | ||
| - Check the candidate's proof | ||
| > TODO: that's extremely hand-wavey. What does that actually entail? | ||
| - Generate either `Statement::Valid` or `Statement::Invalid`. Note that this never generates `Statement::Seconded`; Candidate Backing is the only subsystem which upgrades valid to seconded. |
There was a problem hiding this comment.
I'd prefer to return true/false or Valid/Invalid on the channel as opposed to the Statement type which can included Seconded - doesn't make sense here.
|
(I merged and combined with #1270 ) |
3 participants
Closes #1233.