network: remove handling of validation protocol versions 1 and 2 #7449
Conversation
sw10pa
added
the
T8-polkadot
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
sw10pa
changed the title
sw10pa
changed the title
There was a problem hiding this comment.
Really good job, nice! Left you some minor comments.
One thing, that we need to make sure is that we did not broke anything, so I would recommend once this PR gets approved and we are happy with it, to burn it on a kusama validators for a few days, to make sure everything works ok out in the wild.
| @@ -639,6 +558,14 @@ fn validator_index_for_msg( | |||
| (None, Some(split)) | |||
| }, | |||
| }, | |||
| _ => { | |||
| gum::warn!( | |||
There was a problem hiding this comment.
Now a bad validator can spam everyone with this warning, can we use warn_if_frequent or make it a trace ?
| ) | ||
| .await; | ||
| _ => { | ||
| gum::warn!( |
| @@ -544,111 +493,7 @@ impl AssignmentCriteria for MockAssignmentCriteria { | |||
| /// import an assignment | |||
| /// connect a new peer | |||
| /// the new peer sends us the same assignment | |||
| #[test] | |||
| fn try_import_the_same_assignment() { | |||
There was a problem hiding this comment.
This shouldn't be removed it should be ported to V3.
| @@ -1165,145 +1014,6 @@ fn peer_sending_us_duplicates_while_aggression_enabled_is_ok() { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn import_approval_happy_path_v1_v2_peers() { | |||
There was a problem hiding this comment.
Same here, I would migrate it to V3 to keep coverage the same.
| @@ -2518,112 +2236,7 @@ fn import_remotely_then_locally() { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn sends_assignments_even_when_state_is_approved() { | |||
There was a problem hiding this comment.
Shouldn't be removed it should be ported to V3.
| never!( | ||
| "Only versions 1 and 2 are supported; peer set connection checked above; qed" | ||
| ); | ||
| never!("Only version 3 is supported; peer set connection checked above; qed"); |
There was a problem hiding this comment.
Did we remove the ability to connect using the other protocols ? Because if we didn't then this never! might be triggered by malicious or bad validators, so we need to avoid this.
| ) | ||
| .await; | ||
| } | ||
|
|
||
| if target.targets_current() { |
There was a problem hiding this comment.
Probably this can be removed as well, wouldn't it ?
Issue
[#7410] Remove validation protocol versions 1 and 2
TODO
ValidationVersionV1andV2;ApprovalDistributionMessage;BitfieldDistributionMessage;StatementDistributionMessage;Versionedenum: remove old versions of the above messages and separate collation from validation (this is more complicated and better to do in the next cleaning iteration);