Merge branch 'rumenov/cjrpepr' into 'master'

fix: NET-1382 move the check_protocol_version inside consensus

The registry version check semantically important for the type and not for P2P. In this case it is better to move the check inside consensus. 

Closes NET-1382

See merge request dfinity-lab/public/ic!12859

rs/artifact_manager/src/manager.rs

@@ -82,7 +82,7 @@ impl ArtifactManager for ArtifactManagerImpl {
     /// to be processed by the corresponding artifact client based on the
     /// artifact type.
     ///
-    ///The method returns an `OnArtifactError::NotProcessed` if no clients
+    /// The method returns an `OnArtifactError::NotProcessed` if no clients
     /// were able to process it or an `OnArtifactError::ArtifactPoolError`
     /// if any other error has occurred.
     fn on_artifact(
@@ -276,7 +276,6 @@ where
                     received: advert.into(),
                     expected: expected.into(),
                 })?;
-                self.pool_reader.check_artifact_acceptance(&message)?;
                 // this sends to an unbounded channel, which is what we want here
                 self.sender
                     .send(UnvalidatedArtifact {

rs/artifact_manager/src/pool_readers.rs

@@ -3,9 +3,7 @@
 
 use ic_interfaces::{
     artifact_manager::ArtifactClient,
-    artifact_pool::{
-        ArtifactPoolError, PriorityFnAndFilterProducer, ReplicaVersionMismatch, ValidatedPoolReader,
-    },
+    artifact_pool::{PriorityFnAndFilterProducer, ValidatedPoolReader},
 };
 use ic_types::{
     artifact::*,
@@ -14,12 +12,10 @@ use ic_types::{
     chunkable::*,
     consensus::{
         certification::CertificationMessage, dkg::Message as DkgMessage, ConsensusMessage,
-        HasVersion,
     },
     malicious_flags::MaliciousFlags,
     messages::SignedIngress,
     single_chunked::*,
-    ReplicaVersion,
 };
 use std::sync::{Arc, RwLock};
 
@@ -42,36 +38,11 @@ impl<Pool, T> ConsensusClient<Pool, T> {
     }
 }
 
-/// The function checks if the version of the given artifact matches the default
-/// protocol version and returns an error if it does not.
-fn check_protocol_version<T: HasVersion>(artifact: &T) -> Result<(), ReplicaVersionMismatch> {
-    let version = artifact.version();
-    let expected_version = ReplicaVersion::default();
-    if version != &expected_version {
-        Err(ReplicaVersionMismatch {
-            expected: expected_version,
-            artifact: version.clone(),
-        })
-    } else {
-        Ok(())
-    }
-}
-
 impl<
         Pool: ValidatedPoolReader<ConsensusArtifact> + Send + Sync,
         T: PriorityFnAndFilterProducer<ConsensusArtifact, Pool> + 'static,
     > ArtifactClient<ConsensusArtifact> for ConsensusClient<Pool, T>
 {
-    /// The method checks if the protocol version in the *Consensus* message is
-    /// correct.
-    ///
-    /// If the version is correct, the message is returned in an
-    /// `ArtifactAcceptance` enum.
-    fn check_artifact_acceptance(&self, msg: &ConsensusMessage) -> Result<(), ArtifactPoolError> {
-        check_protocol_version(msg)?;
-        Ok(())
-    }
-
     /// The method returns `true` if and only if the *Consensus* pool contains
     /// the given *Consensus* message ID.
     fn has_artifact(&self, msg_id: &ConsensusMessageId) -> bool {
@@ -275,15 +246,6 @@ impl<
         T: PriorityFnAndFilterProducer<DkgArtifact, Pool> + 'static,
     > ArtifactClient<DkgArtifact> for DkgClient<Pool, T>
 {
-    /// The method checks if the protocol version is correct.
-    ///
-    /// If this is the case, the artifact is returned wrapped in an
-    /// `ArtifactAcceptance` enum.
-    fn check_artifact_acceptance(&self, msg: &DkgMessage) -> Result<(), ArtifactPoolError> {
-        check_protocol_version(msg)?;
-        Ok(())
-    }
-
     /// The method checks if the DKG pool contains a DKG message with the given
     /// ID.
     fn has_artifact(&self, msg_id: &DkgMessageId) -> bool {

rs/artifact_manager/tests/clients.rs

@@ -3,51 +3,13 @@
 mod setup;
 
 use assert_matches::assert_matches;
-use ic_interfaces::{artifact_manager::OnArtifactError, artifact_pool::ArtifactPoolError};
+use ic_interfaces::artifact_manager::OnArtifactError;
 use ic_test_utilities::{
     consensus::{fake::*, make_genesis},
     types::ids::node_test_id,
 };
-use ic_types::{
-    artifact::ArtifactKind, artifact_kind::ConsensusArtifact, consensus::*, ReplicaVersion,
-};
+use ic_types::{artifact::ArtifactKind, artifact_kind::ConsensusArtifact, consensus::*};
 use setup::run_test;
-use std::convert::TryFrom;
-
-#[test]
-fn test_artifact_version() {
-    run_test(|manager| {
-        // Positive case
-        let cup = make_genesis(ic_types::consensus::dkg::Summary::fake());
-        let block1 = BlockProposal::fake(cup.content.block.into_inner(), node_test_id(0));
-        let msg1 = block1.clone().into_message();
-        let result = manager.on_artifact(
-            msg1.clone().into(),
-            ConsensusArtifact::message_to_advert(&msg1).into(),
-            &node_test_id(0),
-        );
-        assert_matches!(result, Ok(()));
-
-        // Negative case
-        let next_version = ReplicaVersion::try_from(format!("{}.1234", block1.version())).unwrap();
-        let block2 = BlockProposal::fake(
-            block1.content.as_ref().fake_version(next_version),
-            node_test_id(1),
-        );
-        let msg2 = block2.into_message();
-        let result = manager.on_artifact(
-            msg2.clone().into(),
-            ConsensusArtifact::message_to_advert(&msg2).into(),
-            &node_test_id(0),
-        );
-        assert_matches!(
-            result,
-            Err(OnArtifactError::ArtifactPoolError(
-                ArtifactPoolError::ArtifactReplicaVersionError(_),
-            ),)
-        );
-    });
-}
 
 #[test]
 fn test_artifact_advert_match() {

rs/consensus/src/consensus.rs

@@ -65,6 +65,7 @@ use ic_types::{
     consensus::{BlockPayload, ConsensusMessageAttribute, ConsensusMessageHashable},
     malicious_flags::MaliciousFlags,
     replica_config::ReplicaConfig,
+    replica_version::ReplicaVersion,
     Height, Time,
 };
 pub use metrics::ValidatorMetrics;
@@ -95,6 +96,23 @@ enum ConsensusSubcomponent {
 /// registry for longer than this, the subnet should halt.
 pub const HALT_AFTER_REGISTRY_UNREACHABLE: Duration = Duration::from_secs(60 * 60);
 
+/// Describe expected version and artifact version when there is a mismatch.
+#[derive(Debug)]
+pub(crate) struct ReplicaVersionMismatch {}
+
+/// The function checks if the version of the given artifact matches the default
+/// protocol version and returns an error if it does not.
+pub(crate) fn check_protocol_version(
+    version: &ReplicaVersion,
+) -> Result<(), ReplicaVersionMismatch> {
+    let expected_version = ReplicaVersion::default();
+    if version != &expected_version {
+        Err(ReplicaVersionMismatch {})
+    } else {
+        Ok(())
+    }
+}
+
 /// [ConsensusImpl] holds all consensus subcomponents, and implements the
 /// Consensus trait by calling each subcomponent in round-robin manner.
 pub struct ConsensusImpl {