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

Implementer's Guide: Incorporate HRMP to TransientValidationData #1588

Merged
17 commits merged into from
Aug 31, 2020
Merged

Implementer's Guide: Incorporate HRMP to TransientValidationData #1588

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5a526e0
Add a note about time of collection of TransientValidationData
pepyakin Aug 14, 2020
f4aaa88
Add HRMP digest and dmq length to TransientValidationData
pepyakin Aug 14, 2020
aacfe76
Add a note that the vector in hrmp digest is never empty
pepyakin Aug 14, 2020
2560365
Add hrmp watermark to TransientValidationData
pepyakin Aug 14, 2020
395c5c4
Add HRMP egress limits
pepyakin Aug 14, 2020
83f1ed3
Incorporate the latest dispatchable upward messages changes.
pepyakin Aug 14, 2020
782dbde
Update candidate.md
pepyakin Aug 17, 2020
af3ce69
Update candidate.md docs
pepyakin Aug 18, 2020
76faea5
Fix wording
pepyakin Aug 18, 2020
a95d276
Delete assignmets.md
pepyakin Aug 18, 2020
257ebb2
Update roadmap/implementers-guide/src/types/candidate.md
pepyakin Aug 24, 2020
acb5054
Extract HrmpTransientValidationData and add additional data.
pepyakin Aug 24, 2020
14f9207
Some clarifications.
pepyakin Aug 24, 2020
22488e9
Introduce HrmpAbridgedOpenChannelRequest
pepyakin Aug 24, 2020
a960ac9
Update roadmap/implementers-guide/src/types/candidate.md
pepyakin Aug 31, 2020
10d62a5
Fix typo: egress->ingress
pepyakin Aug 31, 2020
87e7190
A note about sorting
pepyakin Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions roadmap/implementers-guide/src/runtime/router.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ HrmpEgressChannelsIndex: map ParaId => Vec<ParaId>;
HrmpChannelContents: map HrmpChannelId => Vec<InboundHrmpMessage>;
/// Maintains a mapping that can be used to answer the question:
/// What paras sent a message at the given block number for a given reciever.
/// Invariant: The vector is never empty.
HrmpChannelDigests: map ParaId => Vec<(BlockNumber, Vec<ParaId>)>;
```

Expand Down
33 changes: 32 additions & 1 deletion roadmap/implementers-guide/src/types/candidate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ Persisted validation data are generally derived from some relay-chain state to f

The validation data also serve the purpose of giving collators a means of ensuring that their produced candidate and the commitments submitted to the relay-chain alongside it will pass the checks done by the relay-chain when backing, and give validators the same understanding when determining whether to second or attest to a candidate.

Furthermore, the validation data acts as a way to authorize the additional data the collator needs to pass to the validation
function. For example, the validation function can check whether the incoming messages (e.g. downward messages) were actually
sent by using the data provided in the validation data using so called MQC heads.

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 the validation data used to inform validators and collators about the checks the relay-chain will perform 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 these transient validation data.

Design-wise we should maintain two properties about this data structure:
Expand All @@ -121,10 +125,12 @@ struct PersistedValidationData {
parent_head: HeadData,
/// The relay-chain block number this is in the context of. This informs the collator.
block_number: BlockNumber,
/// The relay-chain
/// The list of MQC heads for the inbound channels paired with the sender para ids. This
/// vector is sorted ascending by the para id and doesn't contain multiple entries with the same
/// sender.
///
/// The MQC heads will be used by the validation function to authorize the input messages passed
/// by the collator.
hrmp_mqc_heads: Vec<(ParaId, Hash)>,
}
```
Expand All @@ -133,6 +139,8 @@ struct PersistedValidationData {

These validation data are derived from some relay-chain state to check outputs of the validation function.

It's worth noting that all the data is collected **before** the candidate execution.

```rust
struct TransientValidationData {
/// The maximum code size permitted, in bytes, of a produced validation code upgrade.
Expand All @@ -159,6 +167,29 @@ struct TransientValidationData {
///
/// This informs a relay-chain backing check and the parachain logic.
code_upgrade_allowed: Option<BlockNumber>,
/// A vector that enumerates the list of blocks in which there was at least one message
/// received. The first number is always after the watermark.
hrmp_digest: Vec<BlockNumber>,
/// The watermark of the HRMP. That is, the block number up to which (inclusive) all HRMP messages
/// sent to the parachain are processed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs are a bit unclear. Is this the previous watermark of the para or the new watermark?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is the previous. I didn't want to repeat myself that all of those are collected before the execution of the PVF so I left a note above:

It's worth noting that all the data is collected before the candidate execution.

Do you think it's better to clarify that they are from the current state explictily?

hrmp_watermark: BlockNumber,
Copy link
Contributor

@rphmeier rphmeier Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkchr One thing to watch out for here, is that because this is in the TransientValidationData, it is not a direct parameter to the validation function and instead will be provided by inherent. That means that a malicious collator can provide something different (e.g. 100 instead of 90) and the relay-chain will still accept the block. So it is possible for this value to move backwards from the parachain's perspective and Cumulus needs to account for that. Similarly with other values in this struct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, after conversation in DM with Sergei, our conclusion was that this watermark should not be provided via inherent and instead Cumulus should be tracking the watermark internally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand this here, why isn't the relay chain checking that the watermark is >= old_watermark?

Copy link
Contributor Author

@pepyakin pepyakin Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relay-chain actually does check that the watermark is > old_watermark.

See the excerpt from the router.md

check_hrmp_watermark(P: ParaId, new_hrmp_watermark):
- new_hrmp_watermark should be strictly greater than the value of HrmpWatermarks for P (if any).
- new_hrmp_watermark must not be greater than the context's block number.
- in HrmpChannelDigests for P an entry with the block number equal to new_hrmp_watermark should exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the parachain can not process all downward messages of a given relay chain block and thus could not increase the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, in this particular place, we are discussing the HRMP watermark. For XCMP and by extension HRMP we have agreed on a simplifying assumption: we assume that a parachain can receive and process all the incoming messages within a block. I think that might be fine for HRMP, I don't think that we will reach the usage levels where they could be a problem and we are going to put aggressive limits on HRMP anyway (I see it happening within upcoming #1632).

For XCMP I think we might want to revisit this assumption. Under circumstances when the parachain is bombarded by the stream of messages over all the channels and processing of all these messages just don't fit into the PVF time or PoV space budget. Although that doesn't seem likely, I also feel that this simplifying assumption doesn't give us a lot - maybe it is not that hard to start with them?

Then, regarding downward messages. In the current version that we are discussing here, downward messages are implemented the same way you (@bkchr) introduced them: i.e. instead of a watermark we just used a simple processed_downward_messages. I liked this approach because, well, I didn't see a lot of reasons to use watermarks, but then today I finally broke down and switched to watermarks in #1629 (see the PR description to find out why).

But now I think maybe it would be a good idea to revert it back to processed_downward_messages. I'll think about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation :)

Although that doesn't seem likely, I also feel that this simplifying assumption doesn't give us a lot - maybe it is not that hard to start with them?

I'm still not sure that this is true, IMHO we can reach this limit relative easy. In the end the parachain wants to process some of its own transactions as well. Just alone think about a parathread that collects messages over quite some time, the parachain will probably not be able to process them in one block.

Copy link
Contributor Author

@pepyakin pepyakin Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in a big part it depends on the parametrization and there are two parameters in play: weight and pov size. I don't think that it is productive to speak about the weight here, because it is highly dependent on the exact code of PVF at hand and more primarily it's PVF which will be designed to not violate any constraints put onto the it by polkadot (although we need to come up with a less costraining system overall, about this later). Therefore, let's assume for now that messages received will always fit into the weight constraints.

Regarding the PoV size, this could be enforced simply.

Since:

  1. there can be only so many paras, N, executed at the same time (I think currently we aim for N = 100 "cores"), and
  2. every parachain can target the same single parachain, and
  3. every parachain can only send one XCMP/HRMP message per block per recipient

we arrive to the worst case of N messages can be received at the same time, then we can say that we want to devote a half of PoV at most for messages. Dividing the terms gives us the maximum size of the message which would make it impossible to overwhelm the parachain as long as it's PVF doesn't use more than half of its PVF (and once again, it should follow the constraints unless it accepts possibility of getting in the aforementioned unfortunate situation).

Also, to clarify, I say it is unlikely situation because I don't think getting 100 slots and spamming another chain is an easy task. I feel there might be just going to the chain and spamming it might be cheaper. (Note that is not to say that we shouldn't keep this vector is mind!)

Regarding the parathread, I don't think they are too special here. The alternative, AFAIU your concerns, is to allow to process messages not only a block worth but message-by-message. In such case, it would be approximately the same.

As an aside: there is also a thing that is related to one of our recent discussions in the parachains team, i.e. parathreads should be able to process all the messages so that these messages don't slide beyond the availability pruning window, which should be less than a day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

3\. every parachain can only send one XCMP/HRMP message per block

Okay, I never heard from this before. However, this clearly lowers the amount of messages :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I beg your pardon! It is actually

every parachain can only send one XCMP/HRMP message per block per recipient

/// A mapping that specifies if the parachain can send an HRMP message to the given recipient
/// channel. A candidate can send a message only to the recipients that are present in this
/// mapping.
/// Since it's a mapping there can't be two items with same `ParaId`.
hrmp_egress_limits: Vec<(ParaId, HrmpChannelLimits)>,
/// A copy of `config.max_upward_message_num_per_candidate` for checking that a candidate doesn't
/// send more messages that permitted.
config_max_upward_message_num_per_candidate: u32,
/// The number of messages pending of the downward message queue.
dmq_length: u32,
}

struct HrmpChannelLimits {
/// Indicates if the channel is already full and cannot accept any more messages.
overfilled: bool,
pepyakin marked this conversation as resolved.
Show resolved Hide resolved
/// A message sent to the channel can occupy only that many bytes.
available_size: u32,
}
```

Expand Down