fix: address PR concerns

Author: KolbyML

bin/e2hs-writer/src/reader.rs

@@ -70,17 +70,14 @@ impl EpochReader {
 
         let starting_block = epoch_index * EPOCH_SIZE;
         let epoch_accumulator = match starting_block < MERGE_BLOCK_NUMBER {
-            true => {
-                let header_validator = HeaderValidator::new();
-                Some(Arc::new(
-                    lookup_epoch_acc(
-                        epoch_index,
-                        &header_validator.pre_merge_acc,
-                        &epoch_acc_path,
-                    )
-                    .await?,
-                ))
-            }
+            true => Some(Arc::new(
+                lookup_epoch_acc(
+                    epoch_index,
+                    &HeaderValidator::default().pre_merge_acc,
+                    &epoch_acc_path,
+                )
+                .await?,
+            )),
             false => None,
         };
 

bin/portal-bridge/src/api/execution.rs

@@ -19,10 +19,7 @@ use ethportal_api::{
 use serde_json::{json, Value};
 use tokio::time::sleep;
 use tracing::{debug, error, info, warn};
-use trin_validation::{
-    accumulator::PreMergeAccumulator, constants::MERGE_BLOCK_NUMBER,
-    header_validator::HeaderValidator,
-};
+use trin_validation::{accumulator::PreMergeAccumulator, constants::MERGE_BLOCK_NUMBER};
 use url::Url;
 
 use crate::{
@@ -40,7 +37,6 @@ const BATCH_LIMIT: usize = 10;
 pub struct ExecutionApi {
     pub primary: Url,
     pub fallback: Url,
-    pub header_validator: HeaderValidator,
     pub request_timeout: u64,
 }
 
@@ -60,11 +56,9 @@ impl ExecutionApi {
         if let Err(err) = check_provider(&client).await {
             error!("Primary el provider is offline: {err:?}");
         }
-        let header_validator = HeaderValidator::default();
         Ok(Self {
             primary,
             fallback,
-            header_validator,
             request_timeout,
         })
     }

bin/portal-bridge/src/bridge/e2hs.rs

@@ -41,7 +41,7 @@ use tokio::{
 };
 use tracing::{debug, error, info, warn};
 use trin_metrics::bridge::BridgeMetricsReporter;
-use trin_validation::header_validator::{HeaderValidator, HistoricalSummariesProvider};
+use trin_validation::header_validator::HeaderValidator;
 
 use super::offer_report::{GlobalOfferReport, OfferReport};
 use crate::{
@@ -80,7 +80,6 @@ impl E2HSBridge {
         let block_semaphore = Arc::new(Semaphore::new(offer_limit));
         let mode = format!("{}-{}:{}", block_range.start, block_range.end, random_fill);
         let metrics = BridgeMetricsReporter::new("e2hs".to_string(), &mode);
-        let header_validator = HeaderValidator::new();
         let http_client = Client::builder()
             .default_headers(HeaderMap::from_iter([(
                 CONTENT_TYPE,
@@ -99,7 +98,7 @@ impl E2HSBridge {
         Ok(Self {
             gossiper,
             block_semaphore,
-            header_validator,
+            header_validator: HeaderValidator::default(),
             block_range,
             random_fill,
             e2hs_files,
@@ -219,7 +218,7 @@ impl E2HSBridge {
             BlockHeaderProof::HistoricalSummaries(_)
         ) {
             self.header_validator
-                .validate_header_with_proof(header_with_proof, HistoricalSummariesProvider::None)
+                .validate_header_with_proof(header_with_proof)
                 .await?;
         }
         let body = &block_tuple.body.body;

crates/subnetworks/history/src/validation.rs

@@ -26,7 +26,9 @@ pub struct ChainHistoryValidator {
 
 impl ChainHistoryValidator {
     pub fn new(header_oracle: Arc<RwLock<HeaderOracle>>) -> Self {
-        let header_validator = HeaderValidator::default();
+        let header_validator = HeaderValidator::new(HistoricalSummariesProvider::HeaderOracle(
+            header_oracle.clone(),
+        ));
         info!(
             hash_tree_root = %hex_encode(header_validator.pre_merge_acc.tree_hash_root().0),
             "Loaded pre-merge accumulator."
@@ -57,10 +59,7 @@ impl Validator<HistoryContentKey> for ChainHistoryValidator {
                     hex_encode(header_hash)
                 );
                 self.header_validator
-                    .validate_header_with_proof(
-                        &header_with_proof,
-                        HistoricalSummariesProvider::HeaderOracle(self.header_oracle.clone()),
-                    )
+                    .validate_header_with_proof(&header_with_proof)
                     .await?;
 
                 Ok(ValidationResult::new(true))
@@ -77,10 +76,7 @@ impl Validator<HistoryContentKey> for ChainHistoryValidator {
                     key.block_number
                 );
                 self.header_validator
-                    .validate_header_with_proof(
-                        &header_with_proof,
-                        HistoricalSummariesProvider::HeaderOracle(self.header_oracle.clone()),
-                    )
+                    .validate_header_with_proof(&header_with_proof)
                     .await?;
 
                 Ok(ValidationResult::new(true))

crates/validation/src/header_validator.rs

@@ -20,14 +20,15 @@ use crate::{
     oracle::HeaderOracle,
 };
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Default, Clone)]
 pub enum HistoricalSummariesProvider {
     /// The historical summaries are provided by the header oracle.
     HeaderOracle(Arc<RwLock<HeaderOracle>>),
     /// The historical summaries are provided by passed historical summaries.
     HistoricalSummaries(HistoricalSummaries),
     /// We don't have historical summaries, for example the E2HS bridge doesn't use a provider and
     /// hence can't get HistoricalSummaries. HistoricalSummary proof validation will fail.
+    #[default]
     None,
 }
 
@@ -63,29 +64,33 @@ impl HistoricalSummariesProvider {
 
 /// HeaderValidator is responsible for validating pre-merge and post-merge headers with their
 /// respective proofs.
-#[derive(Debug, Clone, PartialEq, Eq, Default)]
+///
+/// Default can only be used for validating pre-capella headers.
+#[derive(Debug, Clone, Default)]
 pub struct HeaderValidator {
     /// Pre-merge accumulator used to validate pre-merge headers.
     pub pre_merge_acc: PreMergeAccumulator,
     /// Historical roots accumulator used to validate post-merge/pre-Capella headers.
     pub historical_roots_acc: HistoricalRootsAccumulator,
+    /// Historical summaries provider used to validate post-Capella headers.
+    pub historical_summaries_provider: HistoricalSummariesProvider,
 }
 
 impl HeaderValidator {
-    pub fn new() -> Self {
+    pub fn new(historical_summaries_provider: HistoricalSummariesProvider) -> Self {
         let pre_merge_acc = PreMergeAccumulator::default();
         let historical_roots_acc = HistoricalRootsAccumulator::default();
 
         Self {
             pre_merge_acc,
             historical_roots_acc,
+            historical_summaries_provider,
         }
     }
 
     pub async fn validate_header_with_proof(
         &self,
         header_with_proof: &HeaderWithProof,
-        historical_summaries_provider: HistoricalSummariesProvider,
     ) -> anyhow::Result<()> {
         match &header_with_proof.proof {
             BlockHeaderProof::HistoricalHashes(proof) => {
@@ -123,7 +128,6 @@ impl HeaderValidator {
                     header_with_proof.header.number,
                     header_with_proof.header.hash_slow(),
                     proof,
-                    historical_summaries_provider,
                 )
                 .await
             }
@@ -182,7 +186,6 @@ impl HeaderValidator {
         block_number: u64,
         header_hash: B256,
         proof: &BlockProofHistoricalSummaries,
-        historical_summaries_provider: HistoricalSummariesProvider,
     ) -> anyhow::Result<()> {
         if block_number < SHANGHAI_BLOCK_NUMBER {
             return Err(anyhow!(
@@ -202,7 +205,8 @@ impl HeaderValidator {
         let historical_summary_index =
             (proof.slot - CAPELLA_FORK_EPOCH * SLOTS_PER_EPOCH) / EPOCH_SIZE;
 
-        let block_summary_root = historical_summaries_provider
+        let block_summary_root = self
+            .historical_summaries_provider
             .get_historical_summary(
                 proof.slot / SLOTS_PER_EPOCH,
                 historical_summary_index as usize,
@@ -320,7 +324,6 @@ mod test {
         .unwrap();
         let json: Value = serde_json::from_str(&file).unwrap();
         let hwps = json.as_object().unwrap();
-        let header_validator = get_mainnet_header_validator();
         let obj = hwps.get(&block_number.to_string()).unwrap();
         // Validate content_key decodes
         let raw_ck = obj.get("content_key").unwrap().as_str().unwrap();
@@ -340,17 +343,12 @@ mod test {
             _ => panic!("test reached invalid state"),
         };
         assert_eq!(trin_proof, fluffy_proof);
-        let hwp = HeaderWithProof {
+        let header_with_proof = HeaderWithProof {
             header,
             proof: BlockHeaderProof::HistoricalHashes(trin_proof),
         };
-        header_validator
-            .validate_header_with_proof(
-                &hwp,
-                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(
-                    HeaderOracle::default(),
-                ))),
-            )
+        HeaderValidator::default()
+            .validate_header_with_proof(&header_with_proof)
             .await
             .unwrap();
     }
@@ -374,46 +372,37 @@ mod test {
             header,
             proof: BlockHeaderProof::HistoricalHashes(proof),
         };
-        HeaderValidator::new()
-            .validate_header_with_proof(
-                &header_with_proof,
-                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(
-                    HeaderOracle::default(),
-                ))),
-            )
+        HeaderValidator::default()
+            .validate_header_with_proof(&header_with_proof)
             .await
             .unwrap();
-        let encoded_hwp = hex_encode(header_with_proof.as_ssz_bytes());
+        let encoded_header_with_proof = hex_encode(header_with_proof.as_ssz_bytes());
 
-        let hwp_test_vector = serde_yaml::from_str::<serde_yaml::Value>(
+        let header_with_proof_test_vector = serde_yaml::from_str::<serde_yaml::Value>(
             &read_portal_spec_tests_file(format!(
                 "tests/mainnet/history/headers_with_proof/{block_number}.yaml",
             ))
             .unwrap(),
         )
         .unwrap();
-        let expected_hwp = hwp_test_vector["content_value"].as_str().unwrap();
-        assert_eq!(encoded_hwp, expected_hwp);
+        let expected_header_with_proof = header_with_proof_test_vector["content_value"]
+            .as_str()
+            .unwrap();
+        assert_eq!(encoded_header_with_proof, expected_header_with_proof);
     }
 
     #[tokio::test]
     async fn invalidate_invalid_proofs() {
-        let header_validator = get_mainnet_header_validator();
         let header = get_header(1_000_001);
         let epoch_accumulator = read_epoch_accumulator_122();
         let mut proof = PreMergeAccumulator::construct_proof(&header, &epoch_accumulator).unwrap();
         proof.swap(0, 1);
-        let hwp = HeaderWithProof {
+        let header_with_proof = HeaderWithProof {
             header,
             proof: BlockHeaderProof::HistoricalHashes(proof),
         };
-        assert!(header_validator
-            .validate_header_with_proof(
-                &hwp,
-                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(
-                    HeaderOracle::default(),
-                ))),
-            )
+        assert!(HeaderValidator::default()
+            .validate_header_with_proof(&header_with_proof)
             .await
             .unwrap_err()
             .to_string()
@@ -423,27 +412,19 @@ mod test {
     #[tokio::test]
     #[should_panic(expected = "Invalid proof type found for post-merge header.")]
     async fn header_validator_invalidates_post_merge_header_with_accumulator_proof() {
-        let header_validator = get_mainnet_header_validator();
         let future_height = MERGE_BLOCK_NUMBER + 1;
         let future_header = generate_random_header(&future_height);
         let future_hwp = HeaderWithProof {
             header: future_header,
             proof: BlockHeaderProof::HistoricalHashes(Default::default()),
         };
-        header_validator
-            .validate_header_with_proof(
-                &future_hwp,
-                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(
-                    HeaderOracle::default(),
-                ))),
-            )
+        HeaderValidator::default()
+            .validate_header_with_proof(&future_hwp)
             .await
             .unwrap();
     }
     #[tokio::test]
     async fn header_validator_validate_post_merge_pre_capella_header() {
-        let header_validator = get_mainnet_header_validator();
-
         // Read the historical roots block proof from a test file
         let file = read_portal_spec_tests_file(PathBuf::from(SPEC_TESTS_DIR).join(
             "headers_with_proof/block_proofs_bellatrix/beacon_block_proof-15539558-cdf9ed89b0c43cda17398dc4da9cfc505e5ccd19f7c39e3b43474180f1051e01.yaml",
@@ -460,6 +441,7 @@ mod test {
         let historical_roots_block_proof: BlockProofHistoricalRoots =
             serde_yaml::from_value(value).unwrap();
 
+        let header_validator = HeaderValidator::default();
         header_validator
             .verify_historical_roots(block_number, header_hash, &historical_roots_block_proof)
             .unwrap();
@@ -486,8 +468,6 @@ mod test {
     #[case(17062257)]
     #[tokio::test]
     async fn header_validator_validate_post_capella_header(#[case] block_number: u64) {
-        let header_validator = get_mainnet_header_validator();
-
         // Read the historical roots block proof from a test file
         let file = read_portal_spec_tests_file(PathBuf::from(SPEC_TESTS_DIR).join(format!(
             "headers_with_proof/block_proofs_capella/beacon_block_proof-{block_number}.yaml"
@@ -512,12 +492,15 @@ mod test {
         let historical_summaries = HistoricalSummaries::from_ssz_bytes(&historical_summaries_bytes)
             .expect("cannot decode HistoricalSummaries bytes");
 
+        let header_validator = HeaderValidator::new(
+            HistoricalSummariesProvider::HistoricalSummaries(historical_summaries),
+        );
+
         header_validator
             .verify_historical_summaries(
                 block_number,
                 header_hash,
                 &historical_summaries_block_proof,
-                HistoricalSummariesProvider::HistoricalSummaries(historical_summaries.clone()),
             )
             .await
             .unwrap();
@@ -528,24 +511,11 @@ mod test {
                 SHANGHAI_BLOCK_NUMBER - 1,
                 header_hash,
                 &historical_summaries_block_proof,
-                HistoricalSummariesProvider::HistoricalSummaries(historical_summaries),
             )
             .await;
         assert!(validator_result.is_err());
     }
 
-    //
-    // Testing utils
-    //
-    fn get_mainnet_header_validator() -> HeaderValidator {
-        let header_validator = HeaderValidator::default();
-        assert_eq!(
-            header_validator.pre_merge_acc.tree_hash_root(),
-            B256::from_str(DEFAULT_PRE_MERGE_ACC_HASH).unwrap()
-        );
-        header_validator
-    }
-
     pub(crate) fn get_header(number: u64) -> Header {
         let file = fs::read_to_string("./src/assets/header_rlps.json").unwrap();
         let json: Value = serde_json::from_str(&file).unwrap();
@@ -581,9 +551,8 @@ mod test {
 
     #[tokio::test]
     async fn header_oracle_bootstraps_with_default_pre_merge_acc() {
-        let header_validator = HeaderValidator::default();
         assert_eq!(
-            header_validator.pre_merge_acc.tree_hash_root(),
+            HeaderValidator::default().pre_merge_acc.tree_hash_root(),
             B256::from_str(DEFAULT_PRE_MERGE_ACC_HASH).unwrap(),
         );
     }