Merge branch 'rumenov/rmfafjd' into 'master'

chore: remove the expiry check in the artifact manager

I don't think this check adds any value.

This checks prevent us to add elements to the unvalidated ingress pool iff P2P is very slow and it took around 5 minutes to propagate a messages from one node to another.

In case of malicious replica nodes, the check also doesn't help much because the malicous node can always set an expiry that will pass the check. 

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

Author: rumenov

rs/artifact_manager/src/lib.rs

@@ -113,7 +113,6 @@ use ic_interfaces::{
     ingress_pool::ChangeSet as IngressChangeSet,
     time_source::{SysTimeSource, TimeSource},
 };
-use ic_logger::ReplicaLogger;
 use ic_metrics::MetricsRegistry;
 use ic_types::{artifact::*, artifact_kind::*, malicious_flags::MaliciousFlags};
 use prometheus::{histogram_opts, labels, Histogram};
@@ -319,7 +318,6 @@ pub fn create_ingress_handlers<
     ingress_handler: Arc<
         dyn ChangeSetProducer<PoolIngress, ChangeSet = IngressChangeSet> + Send + Sync,
     >,
-    log: ReplicaLogger,
     metrics_registry: MetricsRegistry,
     malicious_flags: MaliciousFlags,
 ) -> (ArtifactClientHandle<IngressArtifact>, Box<dyn JoinGuard>) {
@@ -335,10 +333,8 @@ pub fn create_ingress_handlers<
         ArtifactClientHandle::<IngressArtifact> {
             sender,
             pool_reader: Box::new(pool_readers::IngressClient::new(
-                time_source.clone(),
                 ingress_pool,
                 priority_fn_and_filter_producer,
-                log,
                 malicious_flags,
             )),
             time_source,

rs/artifact_manager/src/pool_readers.rs

@@ -1,15 +1,12 @@
 //! The module contains implementations of the 'ArtifactClient' trait for all
 //! P2P clients that require consensus over their artifacts.
 
-use ic_constants::{MAX_INGRESS_TTL, PERMITTED_DRIFT_AT_ARTIFACT_MANAGER};
 use ic_interfaces::{
     artifact_manager::ArtifactClient,
     artifact_pool::{
         ArtifactPoolError, PriorityFnAndFilterProducer, ReplicaVersionMismatch, ValidatedPoolReader,
     },
-    time_source::TimeSource,
 };
-use ic_logger::{debug, ReplicaLogger};
 use ic_types::{
     artifact::*,
     artifact_kind::*,
@@ -123,35 +120,24 @@ impl<
 
 /// The ingress `ArtifactClient` to be managed by the `ArtifactManager`.
 pub struct IngressClient<Pool, T> {
-    /// The time source.
-    time_source: Arc<dyn TimeSource>,
     /// The ingress pool, protected by a read-write lock and automatic reference
     /// counting.
     pool: Arc<RwLock<Pool>>,
-
-    /// The logger.
-    log: ReplicaLogger,
-
     priority_fn_and_filter: T,
-
     #[allow(dead_code)]
     malicious_flags: MaliciousFlags,
 }
 
 impl<Pool, T> IngressClient<Pool, T> {
     /// The constructor creates an `IngressClient` instance.
     pub fn new(
-        time_source: Arc<dyn TimeSource>,
         pool: Arc<RwLock<Pool>>,
         priority_fn_and_filter: T,
-        log: ReplicaLogger,
         malicious_flags: MaliciousFlags,
     ) -> Self {
         Self {
-            time_source,
             pool,
             priority_fn_and_filter,
-            log,
             malicious_flags,
         }
     }
@@ -162,46 +148,6 @@ impl<
         T: PriorityFnAndFilterProducer<IngressArtifact, Pool> + 'static,
     > ArtifactClient<IngressArtifact> for IngressClient<Pool, T>
 {
-    /// The method checks whether the given signed ingress bytes constitutes a
-    /// valid singed ingress message.
-    ///
-    /// To this end, the method converts the signed bytes into a `SignedIngress`
-    /// message (if possible) and verifies that the message expiry time is
-    /// neither in the past nor too far in the future.
-    fn check_artifact_acceptance(&self, msg: &SignedIngress) -> Result<(), ArtifactPoolError> {
-        #[cfg(feature = "malicious_code")]
-        {
-            if self.malicious_flags.maliciously_disable_ingress_validation {
-                return Ok(());
-            }
-        }
-
-        let time_now = self.time_source.get_relative_time();
-        // We account for a bit of drift here and accept messages with a bit longer
-        // than `MAX_INGRESS_TTL` time-to-live into the ingress pool.
-        // The purpose is to be a bit more permissive than the HTTP handler when the
-        // ingress was first accepted because here the ingress may have come
-        // from the network.
-        let time_plus_ttl = time_now + MAX_INGRESS_TTL + PERMITTED_DRIFT_AT_ARTIFACT_MANAGER;
-        let msg_expiry_time = msg.expiry_time();
-        if msg_expiry_time < time_now {
-            Err(ArtifactPoolError::MessageExpired)
-        } else if msg_expiry_time > time_plus_ttl {
-            debug!(
-                self.log,
-                "check_artifact_acceptance";
-                ingress_message.message_id => format!("{}", msg.id()),
-                ingress_message.reason => "message_expiry_too_far_in_future",
-                ingress_message.expiry_time => Some(msg_expiry_time.as_nanos_since_unix_epoch()),
-                ingress_message.batch_time => Some(time_now.as_nanos_since_unix_epoch()),
-                ingress_message.batch_time_plus_ttl => Some(time_plus_ttl.as_nanos_since_unix_epoch())
-            );
-            Err(ArtifactPoolError::MessageExpiryTooLong)
-        } else {
-            Ok(())
-        }
-    }
-
     /// The method checks if the ingress pool contains an ingress message with
     /// the given ID.
     fn has_artifact(&self, msg_id: &IngressMessageId) -> bool {

rs/constants/src/lib.rs

@@ -20,20 +20,6 @@ pub const PERMITTED_DRIFT: Duration = Duration::from_secs(60);
 /// of rejecting them right away.
 pub const PERMITTED_DRIFT_AT_VALIDATOR: Duration = Duration::from_secs(30);
 
-/// Duration added to `MAX_INGRESS_TTL` when checking the max allowed expiry
-/// at the artifact manager when it receives ingress from http_handler or p2p.
-/// The purpose is to account for time drift between subnet nodes.
-///
-/// Together with `PERMITTED_DRIFT_AT_VALIDATOR` we give some leeway to
-/// accommodate possible time drift both between the user client and a subnet
-/// node, and between subnet nodes.
-///
-/// Note that when a blockmaker creates a payload, it will only choose from
-/// its ingress pool based on MAX_INGRESS_TTL. So time drift considerations
-/// may lead to more messages being admitted to the ingress pool, but
-/// shouldn't impact other parts of the system.
-pub const PERMITTED_DRIFT_AT_ARTIFACT_MANAGER: Duration = Duration::from_secs(60);
-
 /// The maximum number of messages that can be present in the ingress history
 /// at any one time.
 ///

rs/replica/setup_ic_network/src/lib.rs

@@ -399,7 +399,6 @@ fn setup_artifact_manager(
             Arc::clone(&artifact_pools.ingress_pool),
             ingress_prioritizer,
             ingress_manager,
-            replica_logger.clone(),
             metrics_registry.clone(),
             malicious_flags.clone(),
         );