impl ApprovedPeer UMP signal and add node feature for enabling it #7955
Conversation
alindima
added
the
T8-polkadot
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
1 similar comment
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| @@ -447,7 +457,7 @@ pub fn skip_ump_signals<'a>( | |||
| impl CandidateCommitments { | |||
| /// Returns the core selector and claim queue offset determined by `UMPSignal::SelectCore` | |||
| /// commitment, if present. | |||
| pub fn core_selector( | |||
| fn core_selector_before_approved_peer_allowed( | |||
There was a problem hiding this comment.
I don't really see why we want to change the functionality of this method and it's name. It can just return the core selector as it used to. If there are other signals, so be it. It should ignore them.
There was a problem hiding this comment.
there are two cases we need to handle, depending on whether the new node feature is enabled or not.
If it is disabled, preserve functionality, which is what this function does. (We can't just ignore new signals because they used to be errors). This function is private.
If it is enabled, allow approved peer ump signals. This is decided by the pub fn ump_signals.
I like your suggestion about having a CandidateUMPSignals, I'll do that
|
|
||
| /// Returns the ump signals of this candidate, if any, or an error if they violate the expected | ||
| /// format. | ||
| pub fn ump_signals( |
There was a problem hiding this comment.
This should rather be an internal method. The public methods should be core_selector() and approved_peer() that internally call ump_signals().
Or if we are afraid it's not efficient to do that, let's create a CandidateUMPSignals struct that this function returns. Then we can have the above methods live in there.
There was a problem hiding this comment.
good idea, added a CandidateUMPSignals
| @@ -471,126 +472,12 @@ impl ValidationBackend for MockValidateCandidateBackend { | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn session_index_checked_only_in_backing() { | |||
There was a problem hiding this comment.
removed this test because the same scenario was exercised in invalid_session_or_core_index
#7731
TODO:
FeatureIndex::ElasticScalingMVPis always enabled #6352. It'll add some simplifications