Add CandidateDescriptorV3 support to experimental validator#11306
Add CandidateDescriptorV3 support to experimental validator#11306AlexandruCihodaru merged 19 commits intomasterfrom
Conversation
… validator side * Enable V3 candidate descriptor handling in the experimental collator * Add tests for V3 acceptance, scheduling parent leaf enforcement, backwards compatibility, and unknown version rejection Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
|
/cmd prdoc --audience node_dev --bump minor |
…e_dev --bump minor'
| } | ||
|
|
||
| #[tokio::test] | ||
| // Test that a V3 descriptor is treated as V1 via backward compatibility |
There was a problem hiding this comment.
it can't be treated as v1, but indeed would be good to test a v2 descriptor is fine with v3 enabled
…on initialization Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
…idator_candidate_v3
…idator_candidate_v3
…d slot Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
…idator_candidate_v3
|
/cmd fmt |
Extract LeafSchedulingInfo, slot extraction, and scheduling parent validation into shared functions in lib.rs. Fix V3 tests that incorrectly asserted scheduling_parent instead of relay_parent for PVD requests. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
alindima
left a comment
There was a problem hiding this comment.
LGTM, some suggestions for the tests remain
| } | ||
|
|
||
| #[tokio::test] | ||
| // Test that a V3 descriptor is treated as V1 via backward compatibility |
There was a problem hiding this comment.
it can't be treated as v1, but indeed would be good to test a v2 descriptor is fine with v3 enabled
eskimor
left a comment
There was a problem hiding this comment.
Good work!
Some nits, but overall looks good!
Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
…idator_candidate_v3
Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
See discussion here: #11306 (comment) also fixes a bug, where for held off advertisements we would assume v3 descriptors are never used.
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)
…idator_candidate_v3
|
/cmd fmt |
Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
eskimor
left a comment
There was a problem hiding this comment.
Looks good on a quick pass, a few nits from last time don't seem addressed yet though.
…idator_candidate_v3
Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io>
| }, | ||
| }; | ||
|
|
||
| let core = match self.get_our_core(sender, leaf, session_index).await { |
There was a problem hiding this comment.
Pre-existing but this looks wrong. We should determine our core based on the scheduling parent - not the active leaf. Otherwise calling this in the loop is also completely pointless: We enforce the session index to be the same for all ancestors, so this call will return the same core always.
Related: Requesting the session index for each ancestor may be sensible, if we use it to enforce the same session constraint (but that should already be enforced elsewhere), otherwise it is completely pointless to call here either.
There was a problem hiding this comment.
Given that this is pre-existing, not opposing a follow-up to keep diffs small.
There was a problem hiding this comment.
Pre-existing but this looks wrong. We should determine our core based on the scheduling parent - not the active leaf. Otherwise calling this in the loop is also completely pointless:
Indeed, this should be the ancestor.
Given that this is pre-existing, not opposing a follow-up to keep diffs small.
I agree
There was a problem hiding this comment.
polkadot/node/network/collator-protocol/src/validator_side_experimental/collation_manager/mod.rs whould this do the trick?
There was a problem hiding this comment.
polkadot/node/network/collator-protocol/src/validator_side_experimental/collation_manager/mod.rs whould this do the trick?
I think so, although the test is setting the same core for all relay parents, so it would pass even without the fix. Moreover, maybe we could add a regression test that exercises the core index check instead of exposing this implementation detail as a test-only getter function.
Considering that this PR is already open for quite a while and ready for merge, let's merge it and open a subsequent PR later (this bug has already been here for a while)
eskimor
left a comment
There was a problem hiding this comment.
I found two issues, which should be addressed in a followup:
- Pre-existing, mixing up leaf with ancestor fetches.
- We validate the pvd on advertisements - but we use the scheduling parent. We should either just skip that check or use the relay parent if available (v3 protocol as introduced by Alin).
| futures::join!( | ||
| state.handle_fetched_collation(&mut sender, (adv, res)), | ||
| test_state.assert_pvd_request(adv, None) | ||
| test_state.assert_pvd_request(adv, None, adv.scheduling_parent) |
There was a problem hiding this comment.
This will fail with v3. We should either skip this check entirely or use the relay parent Alin introduced in the advertisement.
Where? |
You mean as part of the CanSecond request? I'll add the relay parent there on the prospective parachains PR |
|
Sorry looked deeper, we only check that for v2. So it is fine. @AlexandruCihodaru |
Adds CandidateDescriptorV3 support to the experimental validator-side collator protocol. Fixes: #11084 --------- Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit a1eb95e)
|
Successfully created backport PR for |
Backport #11306 into `stable2603` from AlexandruCihodaru. 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} --> Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io> Co-authored-by: Alexandru Cihodaru <40807189+AlexandruCihodaru@users.noreply.github.com> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds CandidateDescriptorV3 support to the experimental validator-side collator protocol. Fixes: #11084 --------- Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds CandidateDescriptorV3 support to the experimental validator-side collator protocol.
Fixes: #11084