Runtime API for checking validation outputs#1842
Conversation
Add `CheckValidationOutputs` runtime api and also change the candidate-validation stuff
i.e. move it from node specific primitives to global v1 primitives. This will be needed when we share it later in the runtime inclusion module
factor out the common code to share it during the block inclusion and for the forthcoming CheckValidationOutputs runtime api. Also note that the acceptance criteria was updated to incorporate checks that exist now in candidate-validation
apart from that refactor, update docs and tidy a bit
pepyakin
changed the title
pepyakin
added
B0-silent
This is to fix a test that performs an upgrade.
| AssumptionCheckOutcome::DoesNotMatch => {}, | ||
| AssumptionCheckOutcome::BadRequest => return Ok(Err(ValidationFailed("Bad request".into()))), | ||
| OccupiedCoreAssumption::TimedOut, | ||
| // TODO: Why don't we check `Free`? The guide assumes there are only two possible assumptions. |
There was a problem hiding this comment.
Included and TimedOut on a free core are both no-ops, so any candidate which assumes the core is free would already pass one of those. Adding the third check is a no-op.
Free is kind of an odd one out because there are very few situations where you actually want to check to see that a core is fully free. Off the top of my head, I can't think of any.
| fn persisted_validation_data(para_id: Id, assumption: OccupiedCoreAssumption) | ||
| -> Option<PersistedValidationData<N>>; | ||
|
|
||
| // TODO: Adding a Runtime API should be backwards compatible... right? |
There was a problem hiding this comment.
Yeah, it is fine. We don't need to worry about backcompat until we've deployed on Kusama, probably. This TODO needs resolving before merge
| ); | ||
| } | ||
|
|
||
| // TODO: messaging acceptance criteria rules will go here. |
rphmeier
left a comment
rphmeier
left a comment
There was a problem hiding this comment.
A couple stray TODOs but looks good!
|
Those aren't stray actually : ) I placed two of them to source the comments on review. So they fulfilled their purpose and now can go. Then last one is actually a marker (so I will have less fun time rebasing / resolving conflicts) and is supposed to be resolved by within #1679. Now I realized that I can put a PR number there to be inline with our TODO guidelines. I can remove it as well, it's not critical Another thing I realized is that I probably need to bump the runtime spec versions. |
|
Ok, I know I need to review #1679 :) happy to leave those TODOs be until then |
3 participants
Closes #1594
This PR features structured commits. Please see each commit individually.
The difference between this PR and is that I had to promote the struct
ValidationOutputsto top-level primitives.This is because CandidateCommitments host not only the validation outputs, but also
erasure_root. Getting erasure root happens in the backing subsystem, (i.e. later) and I don't think it's a good idea to move it earlier. Moreover, the runtime API in question should check the acceptance criteria anderasure_rootis not required for that.ValidationOutputsis the best thing we have so far that. Unfortunately, it lives in node primitives. So I just promoted it to top-level primitives.As a potential refactoring we could express
CandidateCommitmentsin terms ofValidationOutputs.I used fully-qualified names instead of imports deliberately to minimize conflicts. Before merging I prefer to switch over to imports.
Split off from #1679