Conversation
rphmeier
added
A0-please_review
rphmeier
force-pushed
the
rh-validation-data-refactor-impl
branch
from
573dcaf to
8b40845
Compare
| /// The validation data, persisted and transient. | ||
| validation_data: ValidationData, | ||
| /// The validation data, persisted. | ||
| validation_data: PersistedValidationData, |
There was a problem hiding this comment.
Can you elaborate on this change?
There was a problem hiding this comment.
The reason is that the ValidateCandidateExhaustive API supports not providing the TransientValidationData if not desired. Then there would be no way to return it with the validation outputs.
However, this change also doesn't impact our other primary user of the CandidateValidation subsystem: candidate backing. Since Candidate Validation checks the transient validation data when validating from the runtime state, there isn't anything else that candidate backing could meaningfully do with that data.
| head_data: _, | ||
| local_validation_data: _, | ||
| global_validation_data: _, | ||
| persisted_validation_data: _, |
There was a problem hiding this comment.
I was a little puzzled by the syntax here at first. Normally, for a partial destructuring, I'd expect something more like
let TestState {
chain_ids,
validator_public,
..
} = ...;such that the .. notation replaces all the unused fields. It's not a big deal here, but it's a useful syntax for the future.
There was a problem hiding this comment.
This is just me fixing fallout and preserving consistency with the code that was already there. But the syntax you mention is better for this, yeah.
| /// issued in this block after the first block | ||
| /// with `relay_chain_height` at least this value, if `Some`. if `None`, issue | ||
| /// no upgrade. | ||
| pub code_upgrade_allowed: Option<polkadot_core_primitives::BlockNumber>, |
There was a problem hiding this comment.
Did you intentionally remove code_upgrade_allowed? If yes, then it's not clear why; there's no obvious connection to the validation data refactor.
[edit] Maybe this is related to the code_upgrade_allowed field in TransientValidationData? If so, then this is fine.
There was a problem hiding this comment.
Yep, I did intentionally do it. Checking whether a code upgrade was submitted only when allowed is done by the relay-chain at inclusion, so it doesn't need to be kept as part of the persisted validation data.
| /// The validation data provide information about how to validate both the inputs and | ||
| /// outputs of a candidate. |
There was a problem hiding this comment.
👍 for thorough documentation here.
roadmap/implementers-guide/src/node/utility/candidate-validation.md
Outdated
Show resolved
Hide resolved
…on.md Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
|
bot merge |
|
Waiting for commit status. |
|
Checks failed; merge aborted. |
|
bot merge |
|
Waiting for commit status. |
|
Checks failed; merge aborted. |
* master: Companion for Substrate #6815 (Dynamic Whitelist) (#1612) Candidate backing respects scheduled collator (#1613) implementers-guide: in TOC move collators before backing, to match protocol pipeline (#1611) Initial guide text for approvals and especially approvals assignments (#1518) Implement validation data refactor (#1585) Implementer's Guide: Make HRMP use upward message kinds (#1591)
5 participants
Closes #1539
This also includes a couple auxiliary changes:
ValidationParams