This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementer's Guide: Incorporate HRMP to TransientValidationData #1588
Implementer's Guide: Incorporate HRMP to TransientValidationData #1588
Changes from 14 commits
5a526e0
f4aaa88
aacfe76
2560365
395c5c4
83f1ed3
782dbde
af3ce69
76faea5
a95d276
257ebb2
acb5054
14f9207
22488e9
a960ac9
10d62a5
87e7190
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Do you think it's better to clarify that they are from the current state explictily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this and close requests need to be part of
PersistedValidationData
. We don't want the collator to be able to forge this data.Or maybe the hash of that data should be in the
PersistedValidationData
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the PVF needs a way to authenticate that the following events:
(I believe other cases can be tracked by the PVF itself. E.g. it should know that it shouldn't send two open requests to the same recipient, confirm the same inbound open request twice, etc)
A collator should not be able to conceal any of these events.
I see the following solutions:
PersistedValidationData
. That inflates the persistent data, but not that bad: the number of open and close requests are bounded (the former is by the sum of max ingress and egress channels, the latter by the number of already opened channels).confirmed: bool
for all inbound open requests.PersistedValidationData
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we assume that with #1642 we limit the total number of channels to a 2 digit number and then retire HRMP completely may be it makes sense to actually go with 1 for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #1663 to address this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a channel appear in this vector only during one parablock, or does it appear in this vector as long as the channel has not yet been closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are channel close requests and these two fields contain all the registered at the context block before the execution of PVF. You can think basically as a copy of the storage list found in the router, but only filtered to be relevant to the parachain at hand.
As the open/close requests found in the router module storage, they live up until they are handled. For the case of close requests - it's the first session boundary.