Add additional broadcast validation tests for Fulu/PeerDAS (#7325)

Closes #6855


  Add PeerDAS broadcast validation tests and fix a small bug where `sampling_columns_indices` is none (indicating that we've already sampled the necessary columns) and `process_gossip_data_columns` gets called

.github/workflows/test-suite.yml

@@ -159,6 +159,28 @@ jobs:
     - name: Show cache stats
       if: env.SELF_HOSTED_RUNNERS == 'true'
       run: sccache --show-stats
+  http-api-tests:
+    name: http-api-tests
+    needs: [check-labels]
+    if: needs.check-labels.outputs.skip_ci != 'true'
+    # Use self-hosted runners only on the sigp repo.
+    runs-on: ${{ github.repository == 'sigp/lighthouse' && fromJson('["self-hosted", "linux", "CI", "large"]') || 'ubuntu-latest'  }}
+    env:
+        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+    steps:
+    - uses: actions/checkout@v4
+    - name: Get latest version of stable Rust
+      if: env.SELF_HOSTED_RUNNERS == 'false'
+      uses: moonrepo/setup-rust@v1
+      with:
+          channel: stable
+          cache-target: release
+          bins: cargo-nextest
+    - name: Run http_api tests for all recent forks
+      run: make test-http-api
+    - name: Show cache stats
+      if: env.SELF_HOSTED_RUNNERS == 'true'
+      run: sccache --show-stats
   op-pool-tests:
     name: op-pool-tests
     needs: [check-labels]
@@ -475,6 +497,7 @@ jobs:
       'op-pool-tests',
       'network-tests',
       'slasher-tests',
+      'http-api-tests',
       'debug-tests-ubuntu',
       'state-transition-vectors-ubuntu',
       'ef-tests-ubuntu',

Makefile

@@ -34,6 +34,9 @@ PROFILE ?= release
 # they run for different forks.
 FORKS=phase0 altair bellatrix capella deneb electra fulu
 
+# List of all recent hard forks. This list is used to set env variables for http_api tests
+RECENT_FORKS=electra fulu
+
 # Extra flags for Cargo
 CARGO_INSTALL_EXTRA_FLAGS?=
 
@@ -141,24 +144,26 @@ build-release-tarballs:
 test-release:
 	cargo test --workspace --release --features "$(TEST_FEATURES)" \
  		--exclude ef_tests --exclude beacon_chain --exclude slasher --exclude network
+		--exclude http_api
 
 # Runs the full workspace tests in **release**, without downloading any additional
 # test vectors, using nextest.
 nextest-release:
 	cargo nextest run --workspace --release --features "$(TEST_FEATURES)" \
-		--exclude ef_tests --exclude beacon_chain --exclude slasher --exclude network
+		--exclude ef_tests --exclude beacon_chain --exclude slasher --exclude network \
+		--exclude http_api
 
 # Runs the full workspace tests in **debug**, without downloading any additional test
 # vectors.
 test-debug:
 	cargo test --workspace --features "$(TEST_FEATURES)" \
-		--exclude ef_tests --exclude beacon_chain --exclude network
+		--exclude ef_tests --exclude beacon_chain --exclude network --exclude http_api
 
 # Runs the full workspace tests in **debug**, without downloading any additional test
 # vectors, using nextest.
 nextest-debug:
 	cargo nextest run --workspace --features "$(TEST_FEATURES)" \
-		--exclude ef_tests --exclude beacon_chain --exclude network
+		--exclude ef_tests --exclude beacon_chain --exclude network --exclude http_api
 
 # Runs cargo-fmt (linter).
 cargo-fmt:
@@ -188,6 +193,13 @@ test-beacon-chain: $(patsubst %,test-beacon-chain-%,$(FORKS))
 test-beacon-chain-%:
 	env FORK_NAME=$* cargo nextest run --release --features "fork_from_env,slasher/lmdb,$(TEST_FEATURES)" -p beacon_chain
 
+# Run the tests in the `beacon_chain` crate for all known forks.
+test-http-api: $(patsubst %,test-beacon-chain-%,$(RECENT_FORKS))
+
+test-http-api-%:
+	env FORK_NAME=$* cargo nextest run --release --features "fork_from_env,slasher/lmdb,$(TEST_FEATURES)" -p http_api
+
+
 # Run the tests in the `operation_pool` crate for all known forks.
 test-op-pool: $(patsubst %,test-op-pool-%,$(FORKS))
 

beacon_node/http_api/src/publish_blocks.rs

@@ -228,23 +228,25 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>>(
             .into_iter()
             .flatten()
             .filter(|data_column| sampling_columns_indices.contains(&data_column.index()))
-            .collect();
+            .collect::<Vec<_>>();
 
-        // Importing the columns could trigger block import and network publication in the case
-        // where the block was already seen on gossip.
-        if let Err(e) =
-            Box::pin(chain.process_gossip_data_columns(sampling_columns, publish_fn)).await
-        {
-            let msg = format!("Invalid data column: {e}");
-            return if let BroadcastValidation::Gossip = validation_level {
-                Err(warp_utils::reject::broadcast_without_import(msg))
-            } else {
-                error!(
-                    reason = &msg,
-                    "Invalid data column during block publication"
-                );
-                Err(warp_utils::reject::custom_bad_request(msg))
-            };
+        if !sampling_columns.is_empty() {
+            // Importing the columns could trigger block import and network publication in the case
+            // where the block was already seen on gossip.
+            if let Err(e) =
+                Box::pin(chain.process_gossip_data_columns(sampling_columns, publish_fn)).await
+            {
+                let msg = format!("Invalid data column: {e}");
+                return if let BroadcastValidation::Gossip = validation_level {
+                    Err(warp_utils::reject::broadcast_without_import(msg))
+                } else {
+                    error!(
+                        reason = &msg,
+                        "Invalid data column during block publication"
+                    );
+                    Err(warp_utils::reject::custom_bad_request(msg))
+                };
+            }
         }
     }
 

beacon_node/http_api/tests/broadcast_validation_tests.rs

@@ -1,3 +1,4 @@
+use beacon_chain::test_utils::test_spec;
 use beacon_chain::{
     test_utils::{AttestationStrategy, BlockStrategy},
     GossipVerifiedBlock, IntoGossipVerifiedBlock,
@@ -571,7 +572,8 @@ pub async fn equivocation_gossip() {
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let tester = InteractiveTester::<E>::new(None, validator_count).await;
+    let spec = test_spec::<E>();
+    let tester = InteractiveTester::<E>::new(Some(spec), validator_count).await;
 
     // Create some chain depth.
     tester.harness.advance_slot();
@@ -1359,18 +1361,22 @@ pub async fn blinded_equivocation_full_pass() {
         .block_is_known_to_fork_choice(&block.canonical_root()));
 }
 
-/// This test checks that an HTTP POST request with the block & blobs succeeds with a 200 response
-/// even if the block has already been seen on gossip without any blobs.
+/// This test checks that an HTTP POST request with the block & blobs/columns succeeds with a 200 response
+/// even if the block has already been seen on gossip without any blobs/columns.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-pub async fn block_seen_on_gossip_without_blobs() {
+pub async fn block_seen_on_gossip_without_blobs_or_columns() {
     let validation_level: Option<BroadcastValidation> = Some(BroadcastValidation::Gossip);
 
     // Validator count needs to be at least 32 or proposer boost gets set to 0 when computing
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let spec = ForkName::latest_stable().make_genesis_spec(E::default_spec());
-    let tester = InteractiveTester::<E>::new(Some(spec), validator_count).await;
+    let tester = InteractiveTester::<E>::new(None, validator_count).await;
+    let state = tester.harness.get_current_state();
+    let fork_name = state.fork_name(&tester.harness.spec).unwrap();
+    if !fork_name.deneb_enabled() {
+        return;
+    }
 
     // Create some chain depth.
     tester.harness.advance_slot();
@@ -1421,18 +1427,22 @@ pub async fn block_seen_on_gossip_without_blobs() {
         .block_is_known_to_fork_choice(&block.canonical_root()));
 }
 
-/// This test checks that an HTTP POST request with the block & blobs succeeds with a 200 response
-/// even if the block has already been seen on gossip without all blobs.
+/// This test checks that an HTTP POST request with the block & blobs/columns succeeds with a 200 response
+/// even if the block has already been seen on gossip without all blobs/columns.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-pub async fn block_seen_on_gossip_with_some_blobs() {
+pub async fn block_seen_on_gossip_with_some_blobs_or_columns() {
     let validation_level: Option<BroadcastValidation> = Some(BroadcastValidation::Gossip);
 
     // Validator count needs to be at least 32 or proposer boost gets set to 0 when computing
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let spec = ForkName::latest_stable().make_genesis_spec(E::default_spec());
-    let tester = InteractiveTester::<E>::new(Some(spec), validator_count).await;
+    let tester = InteractiveTester::<E>::new(None, validator_count).await;
+    let state = tester.harness.get_current_state();
+    let fork_name = state.fork_name(&tester.harness.spec).unwrap();
+    if !fork_name.deneb_enabled() {
+        return;
+    }
 
     // Create some chain depth.
     tester.harness.advance_slot();
@@ -1501,18 +1511,23 @@ pub async fn block_seen_on_gossip_with_some_blobs() {
         .block_is_known_to_fork_choice(&block.canonical_root()));
 }
 
-/// This test checks that an HTTP POST request with the block & blobs succeeds with a 200 response
-/// even if the blobs have already been seen on gossip.
+/// This test checks that an HTTP POST request with the block & blobs/columns succeeds with a 200 response
+/// even if the blobs/columns have already been seen on gossip.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-pub async fn blobs_seen_on_gossip_without_block() {
+pub async fn blobs_or_columns_seen_on_gossip_without_block() {
+    let spec = test_spec::<E>();
     let validation_level: Option<BroadcastValidation> = Some(BroadcastValidation::Gossip);
 
     // Validator count needs to be at least 32 or proposer boost gets set to 0 when computing
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let spec = ForkName::latest_stable().make_genesis_spec(E::default_spec());
-    let tester = InteractiveTester::<E>::new(Some(spec), validator_count).await;
+    let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
+    let state = tester.harness.get_current_state();
+    let fork_name = state.fork_name(&tester.harness.spec).unwrap();
+    if !fork_name.deneb_enabled() {
+        return;
+    }
 
     // Create some chain depth.
     tester.harness.advance_slot();
@@ -1570,15 +1585,19 @@ pub async fn blobs_seen_on_gossip_without_block() {
 /// This test checks that an HTTP POST request with the block succeeds with a 200 response
 /// if just the blobs have already been seen on gossip.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-pub async fn blobs_seen_on_gossip_without_block_and_no_http_blobs() {
+async fn blobs_or_columns_seen_on_gossip_without_block_and_no_http_blobs_or_columns() {
     let validation_level: Option<BroadcastValidation> = Some(BroadcastValidation::Gossip);
 
     // Validator count needs to be at least 32 or proposer boost gets set to 0 when computing
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let spec = ForkName::latest_stable().make_genesis_spec(E::default_spec());
-    let tester = InteractiveTester::<E>::new(Some(spec), validator_count).await;
+    let tester = InteractiveTester::<E>::new(None, validator_count).await;
+    let state = tester.harness.get_current_state();
+    let fork_name = state.fork_name(&tester.harness.spec).unwrap();
+    if !fork_name.deneb_enabled() {
+        return;
+    }
 
     // Create some chain depth.
     tester.harness.advance_slot();
@@ -1638,7 +1657,7 @@ pub async fn blobs_seen_on_gossip_without_block_and_no_http_blobs() {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-pub async fn slashable_blobs_seen_on_gossip_cause_failure() {
+async fn slashable_blobs_or_columns_seen_on_gossip_cause_failure() {
     let validation_level: Option<BroadcastValidation> =
         Some(BroadcastValidation::ConsensusAndEquivocation);
 
@@ -1646,8 +1665,12 @@ pub async fn slashable_blobs_seen_on_gossip_cause_failure() {
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let spec = ForkName::latest_stable().make_genesis_spec(E::default_spec());
-    let tester = InteractiveTester::<E>::new(Some(spec), validator_count).await;
+    let tester = InteractiveTester::<E>::new(None, validator_count).await;
+    let state = tester.harness.get_current_state();
+    let fork_name = state.fork_name(&tester.harness.spec).unwrap();
+    if !fork_name.deneb_enabled() {
+        return;
+    }
 
     // Create some chain depth.
     tester.harness.advance_slot();
@@ -1714,10 +1737,9 @@ pub async fn duplicate_block_status_code() {
     // `validator_count // 32`.
     let validator_count = 64;
     let num_initial: u64 = 31;
-    let spec = ForkName::latest_stable().make_genesis_spec(E::default_spec());
     let duplicate_block_status_code = StatusCode::IM_A_TEAPOT;
     let tester = InteractiveTester::<E>::new_with_initializer_and_mutator(
-        Some(spec),
+        None,
         validator_count,
         None,
         None,