fix broadcast validation tests

sigp · Jan 4, 2024 · 4c3239d · 4c3239d
1 parent 208bf1e
commit 4c3239d
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 61 deletions.
diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs
@@ -599,6 +599,16 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
         });
     }
 
+    chain
+        .observed_slashable
+        .write()
+        .observe_slashable(
+            blob_sidecar.slot(),
+            blob_sidecar.block_proposer_index(),
+            block_root,
+        )
+        .map_err(|e| GossipBlobError::BeaconChainError(e.into()))?;
+
     // Now the signature is valid, store the proposal so we don't accept another blob sidecar
     // with the same `BlobIdentifier`.
     // It's important to double-check that the proposer still hasn't been observed so we don't
@@ -623,16 +633,6 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
         });
     }
 
-    chain
-        .observed_slashable
-        .write()
-        .observe_slashable(
-            blob_sidecar.slot(),
-            blob_sidecar.block_proposer_index(),
-            block_root,
-        )
-        .map_err(|e| GossipBlobError::BeaconChainError(e.into()))?;
-
     // Kzg verification for gossip blob sidecar
     let kzg = chain
         .kzg

diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs
@@ -945,6 +945,11 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             return Err(BlockError::ProposalSignatureInvalid);
         }
 
+        chain
+            .observed_slashable
+            .write()
+            .observe_slashable(block.slot(), block.message().proposer_index(), block_root)
+            .map_err(|e| BlockError::BeaconChainError(e.into()))?;
         // Now the signature is valid, store the proposal so we don't accept another from this
         // validator and slot.
         //
@@ -959,12 +964,6 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             return Err(BlockError::BlockIsAlreadyKnown);
         };
 
-        chain
-            .observed_slashable
-            .write()
-            .observe_slashable(block.slot(), block.message().proposer_index(), block_root)
-            .map_err(|e| BlockError::BeaconChainError(e.into()))?;
-
         if block.message().proposer_index() != expected_proposer as u64 {
             return Err(BlockError::IncorrectBlockProposer {
                 block: block.message().proposer_index(),

diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs
@@ -19,9 +19,9 @@ use std::time::Duration;
 use tokio::sync::mpsc::UnboundedSender;
 use tree_hash::TreeHash;
 use types::{
-    AbstractExecPayload, BeaconBlockRef, BlobSidecarList, EthSpec, ExecPayload, ExecutionBlockHash,
-    ForkName, FullPayload, FullPayloadMerge, Hash256, SignedBeaconBlock, SignedBlindedBeaconBlock,
-    VariableList,
+    AbstractExecPayload, BeaconBlockRef, BlobSidecar, BlobSidecarList, EthSpec, ExecPayload,
+    ExecutionBlockHash, ForkName, FullPayload, FullPayloadMerge, Hash256, SignedBeaconBlock,
+    SignedBlindedBeaconBlock, VariableList,
 };
 use warp::http::StatusCode;
 use warp::{reply::Response, Rejection, Reply};
@@ -117,6 +117,22 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlockConten
             | Err(BlockContentsError::BlobError(
                 beacon_chain::blob_verification::GossipBlobError::RepeatBlob { .. },
             )) => {
+                if let Err(e) = check_slashable(
+                    &chain_clone,
+                    &None,
+                    block_root.unwrap_or(block.canonical_root()),
+                    &block,
+                    &log_clone,
+                ) {
+                    warn!(
+                        log,
+                        "Not publishing block - not gossip verified";
+                        "slot" => slot,
+                        "error" => ?e
+                    );
+                    return Err(warp_utils::reject::custom_bad_request(e.to_string()));
+                }
+
                 // Allow the status code for duplicate blocks to be overridden based on config.
                 return Ok(warp::reply::with_status(
                     warp::reply::json(&ErrorMessage {
@@ -175,46 +191,20 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlockConten
             seen_timestamp,
         ),
         BroadcastValidation::ConsensusAndEquivocation => {
-            let slashable_cache = chain_clone.observed_slashable.read();
-            if let Some(blobs) = blobs_opt.as_ref() {
-                blobs.iter().try_for_each(|blob| {
-                    if slashable_cache
-                        .is_slashable(blob.slot(), blob.block_proposer_index(), blob.block_root())
-                        .map_err(|e| BlockError::BeaconChainError(e.into()))?
-                    {
-                        warn!(
-                            log_clone,
-                            "Not publishing equivocating blob";
-                            "slot" => block_clone.slot()
-                        );
-                        return Err(BlockError::Slashable);
-                    }
-                    Ok(())
-                })?;
-            };
-            if slashable_cache
-                .is_slashable(
-                    block_clone.slot(),
-                    block_clone.message().proposer_index(),
-                    block_root,
-                )
-                .map_err(|e| BlockError::BeaconChainError(e.into()))?
-            {
-                warn!(
-                    log_clone,
-                    "Not publishing equivocating block";
-                    "slot" => block_clone.slot()
-                );
-                Err(BlockError::Slashable)
-            } else {
-                publish_block(
-                    block_clone,
-                    blobs_opt,
-                    sender_clone,
-                    log_clone,
-                    seen_timestamp,
-                )
-            }
+            check_slashable(
+                &chain_clone,
+                &blobs_opt,
+                block_root,
+                &block_clone,
+                &log_clone,
+            )?;
+            publish_block(
+                block_clone,
+                blobs_opt,
+                sender_clone,
+                log_clone,
+                seen_timestamp,
+            )
         }
     };
 
@@ -471,3 +461,46 @@ fn late_block_logging<T: BeaconChainTypes, P: AbstractExecPayload<T::EthSpec>>(
         )
     }
 }
+
+/// Check if any of the blobs or the block are slashable. Returns `BlockError::Slashable` if so.
+fn check_slashable<T: BeaconChainTypes>(
+    chain_clone: &BeaconChain<T>,
+    blobs_opt: &Option<BlobSidecarList<T::EthSpec>>,
+    block_root: Hash256,
+    block_clone: &SignedBeaconBlock<T::EthSpec, FullPayload<T::EthSpec>>,
+    log_clone: &Logger,
+) -> Result<(), BlockError<T::EthSpec>> {
+    let slashable_cache = chain_clone.observed_slashable.read();
+    if let Some(blobs) = blobs_opt.as_ref() {
+        blobs.iter().try_for_each(|blob| {
+            if slashable_cache
+                .is_slashable(blob.slot(), blob.block_proposer_index(), blob.block_root())
+                .map_err(|e| BlockError::BeaconChainError(e.into()))?
+            {
+                warn!(
+                    log_clone,
+                    "Not publishing equivocating blob";
+                    "slot" => block_clone.slot()
+                );
+                return Err(BlockError::Slashable);
+            }
+            Ok(())
+        })?;
+    };
+    if slashable_cache
+        .is_slashable(
+            block_clone.slot(),
+            block_clone.message().proposer_index(),
+            block_root,
+        )
+        .map_err(|e| BlockError::BeaconChainError(e.into()))?
+    {
+        warn!(
+            log_clone,
+            "Not publishing equivocating block";
+            "slot" => block_clone.slot()
+        );
+        return Err(BlockError::Slashable);
+    }
+    Ok(())
+}
diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs
@@ -1226,9 +1226,13 @@ pub async fn blinded_equivocation_gossip() {
     );
 }
 
-/// This test checks that a block that is valid from both a gossip and consensus perspective but that equivocates **late** is rejected when using `broadcast_validation=consensus_and_equivocation`.
+/// This test checks that a block that is valid from both a gossip and
+/// consensus perspective but that equivocates **late** is rejected when using
+/// `broadcast_validation=consensus_and_equivocation`.
 ///
-/// This test is unique in that we can't actually test the HTTP API directly, but instead have to hook into the `publish_blocks` code manually. This is in order to handle the late equivocation case.
+/// This test is unique in that we can't actually test the HTTP API directly,
+/// but instead have to hook into the `publish_blocks` code manually. This is
+/// in order to handle the late equivocation case.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 pub async fn blinded_equivocation_consensus_late_equivocation() {
     /* this test targets gossip-level validation */