Update Cumulus for Parachains V1 #224
Conversation
collator/src/lib.rs
Outdated
|
|
||
| Some(Collation { | ||
| //TODO: What should we do here? | ||
| fees: 0, |
There was a problem hiding this comment.
The fees member can just be removed. We don't have anything that fees are charged for anymore, now that we use channels for messages.
There was a problem hiding this comment.
This comes from the Polkadot side.
There was a problem hiding this comment.
Yeah, we can remove it there. It's plumbed everywhere but essentially it starts it's life here.
so it's just a constant
| let inherent_data = self.inherent_data( | ||
| &validation_data, | ||
| // TODO get the downward messages | ||
| Vec::new(), |
There was a problem hiding this comment.
what was the blocker here so far? Is it not passed in to the collation pipeline at the moment?
There was a problem hiding this comment.
Oh, should it? I assumed the collator would call the runtime API for fetching those
There was a problem hiding this comment.
Before it was passed to the collation. I think it would be nice to follow this pattern, as we pass ValidationData as well.
There was a problem hiding this comment.
Yeah, I also think it would be convenient. But I also think it would potentially be less flexible, because after all it's the collator who knows which messages it should pull, whereas the collation-generation doesn't necessary know about the specifics of the network.
There was a problem hiding this comment.
Wouldn't it need to fetch all downward messages anyway?
whereas the collation-generation doesn't necessary know about the specifics of the network.
Can you explain what you mean?
There was a problem hiding this comment.
Potentially, it might be many, esp. if you include all the HRMP queues. But not only that, imagine a parachain that doesn't need HRMP, then it can just avoid requesting HRMP data at all. I think that it may be fair for XCMP as well. A conclusion I draw from this, is that the collator knows about the target chain (= "the network") better than the collation-generation can do (unless, we complicate the interface way more). That's why I say that the pull model is more flexible, because the collator has more information on when and how to query this data.
| inherent_data, | ||
| Default::default(), | ||
| //TODO: Fix this. | ||
| Duration::from_millis(500), |
There was a problem hiding this comment.
what would this fix look like?
There was a problem hiding this comment.
This comment is old. However, I think that I will be able to improve this with the switch to the slot interface. In the end I just need a reasonable max time for the block production. However, we will probably stick to 500ms for block production? This gives us 1.5 seconds for propagating it to the parachain validators and validation on their end.
| storage::unhashed::put(VALIDATION_FUNCTION_PARAMS, &vfp); | ||
| DidUpdateVFPs::put(true); | ||
| <T::OnValidationFunctionParams as OnValidationFunctionParams>::on_validation_function_params(vfp); | ||
| storage::unhashed::put(VALIDATION_DATA, &vfp); |
There was a problem hiding this comment.
Can you explain the new relationship between ValidationData and ValidationParams and for which situations each is relevant?
There was a problem hiding this comment.
ValidationData is passed using an inherent into the runtime, while ValidationParams are passed when validating the parachain.
We use the ValidationParams to validate the ValidationData while validating the PovBlock.
rphmeier
left a comment
rphmeier
left a comment
There was a problem hiding this comment.
The code changes look fine to me, but can you explain in a bit more detail the expected inputs to the validation function and how it interfaces with Polkadot?
Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
Hmm? We take the And with interfacing with Polkadot, do you mean the client side? |
No description provided.