Merge branch 'leon/payload_builder_overhaul' into 'master'

Payload Builder Refactor (Part 1)

This MR refactors the way, the `BatchPayload` is being constructed.


The idea is quite simple: I am introducing a new trait called `BatchPayloadSectionBuilder`.
It is supposed to replace the traits `XNetPayloadBuilder`, `SelfValidatingPayloadBuilder` and `IngressSelector` (and also the not yet implemented `CanisterHttpPayloadBuilder`), as they all do the same things.

Then inside consensus, there is wrapper called `BatchPayloadSectionAdapter`, that is responsible for mapping the sections into the `BatchPayload`.
This boilerplate is unfortunately necessary, to eliminate the Generic in `BatchPayloadSectionBuilder`.
My original design would use another trait, however, this approach was prevented because of a [long standing compiler bug](rust-lang/rust#20400).


On the validation side, every validation call also returns its `byte_size`, so we can check, that the overall size is not exceeded either.

This MR also contains the implementations of `BatchPayloadSectionBuilder` on top the existing traits. This way, We don't need to touch test code for now. We might want to remove the old traits at a later date.

The payload builder itself just has a Vec<BatchPayloadSectionAdapter> that it calls in a rotating order on a default `BatchPayload`.

**NOTE**: This MR does NOT contain the functional changes to `payload_builder.rs`. Those will be made in a separate MR, to keep the scope of this MR small. 

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

Author: Sawchord

rs/bitcoin/consensus/src/payload_builder.rs

@@ -1,7 +1,8 @@
 use crate::metrics::BitcoinPayloadBuilderMetrics;
 use ic_btc_types_internal::{BitcoinAdapterResponse, BitcoinAdapterResponseWrapper};
-use ic_interfaces::self_validating_payload::{
-    SelfValidatingPayloadBuilder, SelfValidatingPayloadValidationError,
+use ic_interfaces::{
+    payload::BatchPayloadSectionBuilder,
+    self_validating_payload::{SelfValidatingPayloadBuilder, SelfValidatingPayloadValidationError},
 };
 use ic_interfaces_bitcoin_adapter_client::{BitcoinAdapterClient, Options};
 use ic_interfaces_state_manager::{StateManager, StateManagerError};
@@ -11,7 +12,8 @@ use ic_registry_subnet_features::BitcoinFeature;
 use ic_replicated_state::ReplicatedState;
 use ic_types::{
     batch::{SelfValidatingPayload, ValidationContext},
-    Height, NumBytes,
+    consensus::Payload,
+    CountBytes, Height, NumBytes, Time,
 };
 use std::sync::Arc;
 use thiserror::Error;
@@ -231,5 +233,31 @@ impl SelfValidatingPayloadBuilder for BitcoinPayloadBuilder {
     }
 }
 
+impl BatchPayloadSectionBuilder<SelfValidatingPayload> for BitcoinPayloadBuilder {
+    fn build_payload(
+        &self,
+        validation_context: &ValidationContext,
+        max_size: NumBytes,
+        _priority: usize,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> (SelfValidatingPayload, NumBytes) {
+        let past_payloads = self.filter_past_payloads(past_payloads);
+        let payload =
+            self.get_self_validating_payload(validation_context, &past_payloads, max_size);
+        let size = NumBytes::new(payload.count_bytes() as u64);
+        (payload, size)
+    }
+
+    fn validate_payload(
+        &self,
+        payload: &SelfValidatingPayload,
+        validation_context: &ValidationContext,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> Result<NumBytes, SelfValidatingPayloadValidationError> {
+        let past_payloads = self.filter_past_payloads(past_payloads);
+        self.validate_self_validating_payload(payload, validation_context, &past_payloads)
+    }
+}
+
 #[cfg(test)]
 mod tests;

rs/consensus/src/consensus.rs

@@ -11,6 +11,7 @@ mod malicious_consensus;
 pub(crate) mod membership;
 pub(crate) mod metrics;
 mod notary;
+mod payload;
 pub mod payload_builder;
 pub mod pool_reader;
 mod prelude;

rs/consensus/src/consensus/payload.rs

@@ -0,0 +1,74 @@
+// TODO: Remove after implementing functional change
+#![allow(dead_code)]
+
+//! Consensus internal traits that are used in the `payload_builder`.
+use ic_interfaces::{consensus::PayloadValidationError, payload::BatchPayloadSectionBuilder};
+use ic_types::{
+    batch::{BatchPayload, IngressPayload, SelfValidatingPayload, ValidationContext, XNetPayload},
+    consensus::Payload,
+    Height, NumBytes, Time,
+};
+
+// NOTE: Unfortunately, the BatchPayloadSectionAdapter has to be implemented as an enum, because of a compiler bug,
+// that effectively prevents mutually exclusive traits to be implemted.
+// See: https://github.com/rust-lang/rust/issues/20400
+
+/// Maps [`BatchPayloadSectionBuilder`] implementations onto the
+/// [`BatchPayload`]. By requiring this adapter, we can eliminate the generic inside [`BatchPayloadSectionBuilder`] and
+/// make sure that only the consensus crate has access to the whole [`BatchPayload`].
+enum BatchPayloadSectionAdapter {
+    Ingress(Box<dyn BatchPayloadSectionBuilder<IngressPayload>>),
+    XNet(Box<dyn BatchPayloadSectionBuilder<XNetPayload>>),
+    SelfValidating(Box<dyn BatchPayloadSectionBuilder<SelfValidatingPayload>>),
+}
+
+impl BatchPayloadSectionAdapter {
+    fn build_payload(
+        &self,
+        payload: &mut BatchPayload,
+        validation_context: &ValidationContext,
+        max_size: NumBytes,
+        priority: usize,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> NumBytes {
+        match self {
+            Self::Ingress(builder) => {
+                let (payload_section, size) =
+                    builder.build_payload(validation_context, max_size, priority, past_payloads);
+                payload.ingress = payload_section;
+                size
+            }
+            Self::XNet(builder) => {
+                let (payload_section, size) =
+                    builder.build_payload(validation_context, max_size, priority, past_payloads);
+                payload.xnet = payload_section;
+                size
+            }
+            Self::SelfValidating(builder) => {
+                let (payload_section, size) =
+                    builder.build_payload(validation_context, max_size, priority, past_payloads);
+                payload.self_validating = payload_section;
+                size
+            }
+        }
+    }
+
+    fn validate_payload(
+        &self,
+        payload: &BatchPayload,
+        validation_context: &ValidationContext,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> Result<NumBytes, PayloadValidationError> {
+        match self {
+            Self::Ingress(builder) => builder
+                .validate_payload(&payload.ingress, validation_context, past_payloads)
+                .map_err(PayloadValidationError::from),
+            Self::XNet(builder) => builder
+                .validate_payload(&payload.xnet, validation_context, past_payloads)
+                .map_err(PayloadValidationError::from),
+            Self::SelfValidating(builder) => builder
+                .validate_payload(&payload.self_validating, validation_context, past_payloads)
+                .map_err(PayloadValidationError::from),
+        }
+    }
+}

rs/consensus/src/consensus/payload_builder.rs

@@ -1,6 +1,6 @@
 //! Payload creation/validation subcomponent
 
-use crate::consensus::metrics::PayloadBuilderMetrics;
+use crate::consensus::{block_maker::SubnetRecords, metrics::PayloadBuilderMetrics};
 use ic_interfaces::{
     consensus::{PayloadPermanentError, PayloadTransientError, PayloadValidationError},
     ingress_manager::IngressSelector,
@@ -21,9 +21,7 @@ use ic_types::{
 };
 use std::sync::Arc;
 
-use super::block_maker::SubnetRecords;
-
-/// The PayloadBuilder is responsible for creating and validating payload that
+/// The [`PayloadBuilder`] is responsible for creating and validating payload that
 /// is included in consensus blocks.
 pub trait PayloadBuilder: Send + Sync {
     /// Produces a payload that is valid given `past_payloads` and `context`.

rs/ingress_manager/src/ingress_selector.rs

@@ -12,6 +12,7 @@ use ic_interfaces::{
         IngressTransientError,
     },
     ingress_pool::{IngressPoolSelect, SelectResult},
+    payload::BatchPayloadSectionBuilder,
     validation::{ValidationError, ValidationResult},
 };
 use ic_interfaces_state_manager::StateManagerError;
@@ -272,6 +273,32 @@ impl<'a> IngressSelector for IngressManager {
     }
 }
 
+impl BatchPayloadSectionBuilder<IngressPayload> for IngressManager {
+    fn build_payload(
+        &self,
+        validation_context: &ValidationContext,
+        max_size: NumBytes,
+        _priority: usize,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> (IngressPayload, NumBytes) {
+        let past_payloads = self.filter_past_payloads(past_payloads, validation_context);
+        let payload = self.get_ingress_payload(&past_payloads, validation_context, max_size);
+        let size = NumBytes::new(payload.count_bytes() as u64);
+        (payload, size)
+    }
+
+    fn validate_payload(
+        &self,
+        payload: &IngressPayload,
+        validation_context: &ValidationContext,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> Result<NumBytes, IngressPayloadValidationError> {
+        let past_payloads = self.filter_past_payloads(past_payloads, validation_context);
+        self.validate_ingress_payload(payload, &past_payloads, validation_context)?;
+        Ok(NumBytes::new(payload.count_bytes() as u64))
+    }
+}
+
 impl IngressManager {
     #[allow(clippy::too_many_arguments)]
     fn validate_ingress(

rs/interfaces/src/ingress_manager.rs

@@ -2,6 +2,7 @@
 use crate::{
     execution_environment::{CanisterOutOfCyclesError, IngressHistoryError},
     ingress_pool::{ChangeSet, IngressPool},
+    payload::BatchPayloadSectionType,
     validation::{ValidationError, ValidationResult},
 };
 use ic_types::{
@@ -80,6 +81,11 @@ pub enum IngressTransientError {
 pub type IngressPayloadValidationError =
     ValidationError<IngressPermanentError, IngressTransientError>;
 
+impl BatchPayloadSectionType for IngressPayload {
+    type PermanentValidationError = IngressPermanentError;
+    type TransientValidationError = IngressTransientError;
+}
+
 impl From<CryptoError> for IngressTransientError {
     fn from(err: CryptoError) -> IngressTransientError {
         IngressTransientError::CryptoError(err)

rs/interfaces/src/lib.rs

@@ -18,6 +18,7 @@ pub mod ingress_manager;
 pub mod ingress_pool;
 pub mod messages;
 pub mod messaging;
+pub mod payload;
 pub mod registry;
 pub mod replica_config;
 pub mod self_validating_payload;

rs/interfaces/src/messaging.rs

@@ -1,5 +1,5 @@
 //! Message Routing public interfaces.
-use crate::validation::ValidationError;
+use crate::{payload::BatchPayloadSectionType, validation::ValidationError};
 use ic_types::{
     batch::{Batch, ValidationContext, XNetPayload},
     consensus::Payload,
@@ -34,6 +34,10 @@ pub enum XNetTransientValidationError {
 pub type XNetPayloadValidationError =
     ValidationError<InvalidXNetPayload, XNetTransientValidationError>;
 
+impl BatchPayloadSectionType for XNetPayload {
+    type PermanentValidationError = InvalidXNetPayload;
+    type TransientValidationError = XNetTransientValidationError;
+}
 /// The public interface for the MessageRouting layer.
 pub trait MessageRouting: Send + Sync {
     /// Delivers a finalized `Batch` for deterministic processing.

rs/interfaces/src/payload.rs

@@ -0,0 +1,75 @@
+//! This module defines traits related to payloads and payload building.
+
+use crate::validation::ValidationError;
+use ic_base_types::NumBytes;
+use ic_types::{batch::ValidationContext, consensus::Payload, Height, Time};
+
+/// A marker trait, indicating, that some struct is a section of the [`BatchPayload`].
+pub trait BatchPayloadSectionType: Default {
+    /// The type of error that is returned, when the validation failed permanently.
+    type PermanentValidationError;
+
+    /// The type of error that is returned, when the validation does not pass transitively,
+    /// i.e. the payload may pass validation at a later state.
+    type TransientValidationError;
+}
+
+/// A [`BatchPayloadSectionBuilder`] is implemented by any artifact, that can be
+/// included into a [`BatchPayload`](ic_types::batch::BatchPayload).
+///
+/// # Invariants:
+/// It is **crucial** that any payload returned by
+/// [`build_payload`](BatchPayloadSectionBuilder::build_payload)
+/// succeeds when passed into
+/// [`validate_payload`](BatchPayloadSectionBuilder::validate_payload),
+/// given the same arguments for [`ValidationContext`] and `past_payloads`,
+/// and that the following monotony holds:
+///
+/// - Payload size returned by [`build_payload`](BatchPayloadSectionBuilder::build_payload)
+///     `<=` `max_size` passed into [`build_payload`](BatchPayloadSectionBuilder::build_payload)
+/// - Payload size returned by [`validate_payload`](BatchPayloadSectionBuilder::validate_payload)
+///     `<=` payload size returned by [`build_payload`](BatchPayloadSectionBuilder::build_payload)
+///
+/// It is advised to call the validation function after building the payload to be 100% sure.
+// [build_payload]: (BatchPayloadSectionBuilder::build_payload)
+// [validate_payload]: (BatchPayloadSectionBuilder::validate_payload)
+pub trait BatchPayloadSectionBuilder<T>
+where
+    T: BatchPayloadSectionType,
+{
+    /// Called to build the payload.
+    ///
+    /// # Arguments:
+    /// - `validation_context`: The [`ValidationContext`], under which the payload must be valid.
+    /// - `max_size`: The maximum size in [`NumBytes`], that the payload section has available in the current batch.
+    /// - `priority`: The order in which the individual [`BatchPayloadSectionBuilder`] have been called.
+    /// - `past_payloads`: All [`BatchPayload`]s from the certified height to the tip.
+    ///
+    /// # Returns:
+    /// The [`BatchPayloadSectionType`]. If there is no suitable payload, return [`Default`].
+    /// The size of the payload in [`NumBytes`].
+    fn build_payload(
+        &self,
+        validation_context: &ValidationContext,
+        max_size: NumBytes,
+        priority: usize,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> (T, NumBytes);
+
+    /// Called to validate the payload.
+    ///
+    /// # Argument:
+    /// - `payload`: The payload to verify.
+    /// - `validation_context`: The [`ValidationContext`], under which to validate the payload.
+    /// - `past_payloads`: All [`Payload`]s from the certified height to the tip.
+    ///
+    /// # Returns:
+    /// **On success:** The size of the section as [`NumBytes`].
+    /// **On error:**: Either a transient or permantent error.
+    fn validate_payload(
+        &self,
+        payload: &T,
+        validation_context: &ValidationContext,
+        past_payloads: &[(Height, Time, Payload)],
+    ) -> Result<NumBytes, ValidationError<T::PermanentValidationError, T::TransientValidationError>>;
+}

rs/interfaces/src/self_validating_payload.rs

@@ -1,4 +1,4 @@
-use crate::validation::ValidationError;
+use crate::{payload::BatchPayloadSectionType, validation::ValidationError};
 
 use ic_types::{
     batch::{SelfValidatingPayload, ValidationContext},
@@ -18,6 +18,11 @@ pub enum SelfValidatingTransientValidationError {}
 pub type SelfValidatingPayloadValidationError =
     ValidationError<InvalidSelfValidatingPayload, SelfValidatingTransientValidationError>;
 
+impl BatchPayloadSectionType for SelfValidatingPayload {
+    type PermanentValidationError = InvalidSelfValidatingPayload;
+    type TransientValidationError = SelfValidatingTransientValidationError;
+}
+
 pub trait SelfValidatingPayloadBuilder: Send + Sync {
     /// Produces a `SelfValidatingPayload` of maximum byte size `byte_limit`
     /// that is valid given a `ValidationContext` (certified height plus