add relay parent to advertisement v3#11393
Conversation
eskimor
left a comment
There was a problem hiding this comment.
Please let's leave the versioning. It is genuinely useful. E.g. if we had that, we would have had no strong need for a new protocol version to begin with.
| let candidate_descriptor_version = | ||
| collation.receipt.descriptor.version(per_scheduling_parent.v3_enabled); | ||
| // For V3 descriptors, send `Some(scheduling_parent)` so the validator knows this | ||
| // is a V3 candidate and can apply the scheduling parent validation rules. |
There was a problem hiding this comment.
Shouldn't be necessary since we also have the version? No strong opinion here, but just setting both to the relay parent for v1/v2 is less special casing, which is nice.
There was a problem hiding this comment.
Actually I have a bit stronger opinion. There exists a possible future where every parachain adopted v3 and we deprecate the old versions ... then we have an Option that will always be Some, which is annoying - just complicating code.
There was a problem hiding this comment.
There exists a possible future where every parachain adopted v3 and we deprecate the old versions ... then we have an Option that will always be Some, which is annoying - just complicating code.
That future is far in the future :) My expectation is we not gonna see anyone except us rolling it out this year. So scheduling parent will be None for most of parachain candidates. Also if it will become always Some in next 2 years is not something that has negative objective impact.
| parent_head_data_hash: Hash, | ||
| /// The version of the candidate descriptor. | ||
| candidate_descriptor_version: CandidateDescriptorVersion, | ||
| /// Optional scheduling parent, if the candidate is a v3 descriptor. |
There was a problem hiding this comment.
No please don't. While I can get on board in collation generation to just pass in an optional SchedulingParent and derive the descriptor version to use from that, I would really leave the version here and more importantly not make the scheduling parent optional.
That's just a much cleaner API:
- Knowing the version in the advertisement is useful and sensible: E.g. if we had this, we would not have had a strong need to introduce a new version of the protocol at all for v3.
- scheduling parent can always be provided, it just happens to be the same as the relay parent in v1 and v2: Having less code paths is always better!
(2) is really crucial: Once v1 is removed and the feature is always enabled, we don't need any special casing for v3 at all then: Just read scheduling session & scheduling parent - it will always give you the right information, no matter the descriptor version - they would no longer need to be Options either. This is by design, let's not break this.
There was a problem hiding this comment.
I think this is fine. I like it explicit vs implicit. The actual data that collator protocol needs for filtering is not the version, but the RP and SP.
Then, when we fetch the collation we have the receipt which has all the information.
There was a problem hiding this comment.
We don't really know how v4 is gonna look and what is gonna introduce. If it's something new we need in advertisement whatever we do here is not gonna help. It would only work if there is some semantics change on the very same information already provided now.
There was a problem hiding this comment.
But this is exactly what a new candidate descriptor version does, no?
There was a problem hiding this comment.
You all hated that I introduced a new collator protocol version - there would have bee no need, if we just had the descriptor version. And as said in chat, we already know an upcoming change where the version is handy too: Deprecating v1.
There was a problem hiding this comment.
scope creeping a last minute protocol upgrade that will make us have more seamless protocol changes in the future is not a good idea now. It'd need proper thought and debate. I'd rather not rush it and maybe it's something we could design for all of our protocols going further.
There was a problem hiding this comment.
collation protocol is the easiest one to upgrade really, as it's only being used for one subsystem, unlike the way we multiplex all message types on the validation protocol (which makes it a pain to upgrade)
There was a problem hiding this comment.
scope creeping a last minute protocol upgrade that will make us have more seamless protocol changes in the future is not a good idea now. It'd need proper thought and debate. I'd rather not rush it and maybe it's something we could design for all of our protocols going further.
I like the idea of doing it for all protocols we use on the node. It is really painful to add a single change to validation protocol, a lot of code gets added every time.
Definitely needs design first.
There was a problem hiding this comment.
An alternative is #810 which we never got to do.
…parent-to-v3-advertisement
| /// The version of the candidate descriptor. | ||
| candidate_descriptor_version: CandidateDescriptorVersion, | ||
| /// The relay parent of the candidate. | ||
| relay_parent: Hash, |
There was a problem hiding this comment.
I'd move this after scheduling parent, they are both parents :)
See discussion here: #11306 (comment) also fixes a bug, where for held off advertisements we would assume v3 descriptors are never used. (cherry picked from commit f02ad9e)
|
Successfully created backport PR for |
Backport #11393 into `stable2603` from alindima. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Alin Dima <alin@parity.io> Co-authored-by: eskimor <robert@gonimo.com> Co-authored-by: Egor_P <egor@parity.io>
See discussion here: #11306 (comment)
also fixes a bug, where for held off advertisements we would assume v3 descriptors are never used.