Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

ValidationData refactoring #1539

Closed
rphmeier opened this issue Aug 5, 2020 · 4 comments · Fixed by #1585
Closed

ValidationData refactoring #1539

rphmeier opened this issue Aug 5, 2020 · 4 comments · Fixed by #1585
Assignees
Labels
I8-refactor Code needs refactoring.
Milestone

Comments

@rphmeier
Copy link
Contributor

rphmeier commented Aug 5, 2020

#1503 (review)

Rephrased more abstractly here:

We currently maintain an invariant that everything that is backed in the relay chain satisfies a number of checks on the commitments of the candidate - that they meet size limits, only send messages on valid channels, etc.

However, it is not clear to me how the validators are meant to check those things off-chain, because there is currently no runtime API that exposes these things. It's also not clear to me how collators are meant to construct blocks that satisfy these limits. I think we want to expose this data from the runtime API, but we don't need to keep it available for secondary checkers because of the invariant I mentioned at the beginning.

I think we should have two main changes to ValidationData with a couple of implied secondary changes.

Main changes:

  1. Combine LocalValidationData and GlobalValidationData into a single ValidationData struct.
  2. Split ValidationData into PersistedValidationData and NonPersistedValidationData.

The result would look something like this:

struct ValidationData {
    /// Validation data that needs to be persisted for secondary checkers.
    ///
    /// These validation data are generally derived from some relay-chain state to form inputs to 
    /// the validation function, and as such need to be persisted by the availability system
    ///  to avoid dependence on availability of the relay-chain state.
    persisted: PersistedValidationData,
    /// These validation data are derived from some relay-chain state to check outputs of the validation
    /// function. Since the commitments of the validation function are checked by the relay-chain,
    /// secondary checkers can rely on the invariant that the relay-chain only includes para-blocks
    /// for which these checks have already been done. As such, there is no need for this information
    /// to be persisted by the availability system. Nevertheless, we expose it so the backing validators
    /// can validate the outputs of a candidate before voting to submit it to the relay-chain and so
    /// collators can collate candidates that satisfy the criteria implied by this field.
    non_persisted: NonPersistedValidationData,
}

Secondary changes:

  1. Combine the global_validation_data and local_validation_data runtime APIs
  2. The validation_data_hash in candidate descriptors should reference only the persisted validation data.
  3. The AvailableData struct should store only the persisted validation data.

These changes encompass both the guide and the code-base.

@rphmeier rphmeier added the I8-refactor Code needs refactoring. label Aug 5, 2020
@rphmeier rphmeier added this to the White Flint milestone Aug 5, 2020
@rphmeier
Copy link
Contributor Author

rphmeier commented Aug 5, 2020

cc @pepyakin - what should go in NonPersistedValidationData for HRMP? Outgoing channel queue size limits? Upward message limits?

@coriolinus
Copy link
Contributor

Would that be struct ValidationData? Now that we no longer have GlobalValidationData, I don't see much benefit in adding the contrasting adjective.

@rphmeier
Copy link
Contributor Author

rphmeier commented Aug 5, 2020

@coriolinus yep, I edited the Rust example, if that's what you were referring to

@pepyakin
Copy link
Contributor

pepyakin commented Aug 5, 2020

I'd put the following items in NonPersistedValidationData. The description is in terms introduced by #1503. Also, I will refer to the para of the candidate as P.

  1. The length of DownwardMessagesQueues(P)
  2. HrmpWatermarks(P) to check that the candidate's watermark is greater than it was. (An alternative is to make a candidate store a delta of blocks. In other words, how many blocks the para advanced the watermark from the previous one. This would allow us to not include HrmpWatermarks(P) )
  3. HrmpChannelDigests(P) to ensure that the candidate's watermark doesn't fall on a block where there were no messages sent to P.
  4. Reduced information collected from all outbound HRMP channels. Namely, we need to know if some M if appended to a channel C won't exceed the total number of messages allowed in C and the total size of messages in C won't exceed the total allowed size.

post #1556 update:
we should include that the total number of sent messages doesn't exceed config.max_upward_message_num_per_candidate

Outgoing channel queue size limits and upward message limits are not part of #1503 as of now, but tracked in a separate issue #1534 . But yeah they should go there.

For tentative design of XCMP we would like to have roughly:

  1. The length of the CST row for P.
  2. Additionally, if we want to keep around read-only channels around for a while, we'd want to include the "condemned set", i.e. all read-only channels that cannot accept new messages.
  3. The current watermark for P.
  4. Perhaps analogue of HrmpChannelDigests but for XCMP, but this is not discussed and that depends on the checking regime, so I am not completely sure about that one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I8-refactor Code needs refactoring.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants