collation-generation: use v2 receipts #5908
Conversation
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…reim/node_v2_descriptors Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…me_dev --bump major'
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…to alindima/collation-generation-v2-receipts
…generation-v2-receipts
| #[case(0)] | ||
| #[case(1)] | ||
| #[case(2)] | ||
| fn distribute_collation_for_free_cores_with_async_backing_enabled_and_elastic_scaling( |
There was a problem hiding this comment.
Do we still have a test that covers this ?
There was a problem hiding this comment.
Yes. Since we no longer care about occupied cores, this test and distribute_collation_for_occupied_cores_with_async_backing_enabled_and_elastic_scaling have been merged into distribute_collation_with_elastic_scaling
| #[rstest] | ||
| #[case(RuntimeApiRequest::CLAIM_QUEUE_RUNTIME_REQUIREMENT - 1)] | ||
| #[case(RuntimeApiRequest::CLAIM_QUEUE_RUNTIME_REQUIREMENT)] | ||
| fn requests_validation_data_for_scheduled_matches(#[case] runtime_version: u32) { |
There was a problem hiding this comment.
What is the reasoning to remove this test ?
There was a problem hiding this comment.
This is a test that verifies the handling of the active leaves, assuming we can have multiple activations at once. This is no longer true (probably used to be)
There was a problem hiding this comment.
The name of the test was misleading in this case.
| descriptor: CandidateDescriptorV2::new( | ||
| para_id, | ||
| relay_parent, | ||
| core_index, |
There was a problem hiding this comment.
We can simplify the collator code and also avoid checking the core index here if we use commitments.committed_core_index(). We need to however use the PreparedCollation core index if there is no core index commitment.
There was a problem hiding this comment.
how would this simplify the collator code?
There was a problem hiding this comment.
I mean we don't have to look into the commitments because we just do it here when assembling the committed candidate receipt.
| }, | ||
| Err(e @ RuntimeApiError::Execution { .. }) => Err(e.into()), | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn erasure_root( | ||
| n_validators: usize, | ||
| persisted_validation: PersistedValidationData, | ||
| pov: PoV, |
There was a problem hiding this comment.
Given the above comment, this should be receipt.core_index().unwrap_or(core_index)
There was a problem hiding this comment.
I don't understand how this helps. We are creating the receipt just above with this core index
There was a problem hiding this comment.
The main point here is that we should pick the core index that the candidate commits to. There is a bit of confusion since there are 2 core indices, one from the PreparedCollation and potentially another one in the commitments. If we are using v2 receipts, the collator code retrieves the core index from the commitments and puts it in the SubmitCollationParams and then we check it once again later here. This is likely prone to errors if the core_index is different from the one passed in the commitments. Maybe we should have a separate SubmitCollationParamsV2 that doesn't have a core_index field at all (which should be used when the candidate commits to a core_index), so there is no confusion.
There was a problem hiding this comment.
ok. what you're saying seems doable. However, I'm not sure it simplifies things.
We'd need to handle two message types here in collation-generation and also decide which message type to use in the collator based on the node feature and whether the core index commitment is present. I'm not sure it simplifies things.
Would be an improvement idea for when we completely switch to v2 receipts and make the commitment mandatory
There was a problem hiding this comment.
The commitments being optional indeed introduces more complexity, but for v2 receipts, the collator doesn't actually need to know the core index, just the amount of cores assigned. We don't even need a runtime API to get the next core selector, we just build a block and we look at the commitments, if there is a core committed, we just push the collation to collation generation. We can create a ticket to handle this at a later time.
There was a problem hiding this comment.
I think I remember why I coded it this way. What you're suggesting has the caveat that we're breaking the generic nature of the core selection policy of the parachain runtime, since we assume that the runtime will choose a core that is assigned. If it won't, it would have built a block and only after that see that the selected core is not assigned.
Part of #5047
Plus some cleanups