Send success code for duplicate blocks on HTTP (#4655)

## Issue Addressed

Closes #4473 (take 3)

## Proposed Changes

- Send a 202 status code by default for duplicate blocks, instead of 400. This conveys to the caller that the block was published, but makes no guarantees about its validity. Block relays can count this as a success or a failure as they wish.
- For users wanting finer-grained control over which status is returned for duplicates, a flag `--http-duplicate-block-status` can be used to adjust the behaviour. A 400 status can be supplied to restore the old (spec-compliant) behaviour, or a 200 status can be used to silence VCs that warn loudly for non-200 codes (e.g. Lighthouse prior to v4.4.0).
- Update the Lighthouse VC to gracefully handle success codes other than 200. The info message isn't the nicest thing to read, but it covers all bases and isn't a nasty `ERRO`/`CRIT` that will wake anyone up.

## Additional Info

I'm planning to raise a PR to `beacon-APIs` to specify that clients may return 202 for duplicate blocks. Really it would be nice to use some 2xx code that _isn't_ the same as the code for "published but invalid". I think unfortunately there aren't any suitable codes, and maybe the best fit is `409 CONFLICT`. Given that we need to fix this promptly for our release, I think using the 202 code temporarily with configuration strikes a nice compromise.

beacon_node/http_api/src/lib.rs

@@ -139,6 +139,8 @@ pub struct Config {
     pub data_dir: PathBuf,
     pub sse_capacity_multiplier: usize,
     pub enable_beacon_processor: bool,
+    #[serde(with = "eth2::types::serde_status_code")]
+    pub duplicate_block_status_code: StatusCode,
 }
 
 impl Default for Config {
@@ -154,6 +156,7 @@ impl Default for Config {
             data_dir: PathBuf::from(DEFAULT_ROOT_DIR),
             sse_capacity_multiplier: 1,
             enable_beacon_processor: true,
+            duplicate_block_status_code: StatusCode::ACCEPTED,
         }
     }
 }
@@ -510,6 +513,8 @@ pub fn serve<T: BeaconChainTypes>(
     let task_spawner_filter =
         warp::any().map(move || TaskSpawner::new(beacon_processor_send.clone()));
 
+    let duplicate_block_status_code = ctx.config.duplicate_block_status_code;
+
     /*
      *
      * Start of HTTP method definitions.
@@ -1284,11 +1289,11 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |block: Arc<SignedBeaconBlock<T::EthSpec>>,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
+            move |block: Arc<SignedBeaconBlock<T::EthSpec>>,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
                 task_spawner.spawn_async_with_rejection(Priority::P0, async move {
                     publish_blocks::publish_block(
                         None,
@@ -1297,9 +1302,9 @@ pub fn serve<T: BeaconChainTypes>(
                         &network_tx,
                         log,
                         BroadcastValidation::default(),
+                        duplicate_block_status_code,
                     )
                     .await
-                    .map(|()| warp::reply().into_response())
                 })
             },
         );
@@ -1314,11 +1319,11 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |block_bytes: Bytes,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
+            move |block_bytes: Bytes,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
                 task_spawner.spawn_async_with_rejection(Priority::P0, async move {
                     let block =
                         SignedBeaconBlock::<T::EthSpec>::from_ssz_bytes(&block_bytes, &chain.spec)
@@ -1334,9 +1339,9 @@ pub fn serve<T: BeaconChainTypes>(
                         &network_tx,
                         log,
                         BroadcastValidation::default(),
+                        duplicate_block_status_code,
                     )
                     .await
-                    .map(|()| warp::reply().into_response())
                 })
             },
         );
@@ -1352,12 +1357,12 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |validation_level: api_types::BroadcastValidationQuery,
-             block: Arc<SignedBeaconBlock<T::EthSpec>>,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
+            move |validation_level: api_types::BroadcastValidationQuery,
+                  block: Arc<SignedBeaconBlock<T::EthSpec>>,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
                 task_spawner.spawn_async_with_rejection(Priority::P0, async move {
                     publish_blocks::publish_block(
                         None,
@@ -1366,9 +1371,9 @@ pub fn serve<T: BeaconChainTypes>(
                         &network_tx,
                         log,
                         validation_level.broadcast_validation,
+                        duplicate_block_status_code,
                     )
                     .await
-                    .map(|()| warp::reply().into_response())
                 })
             },
         );
@@ -1384,12 +1389,12 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |validation_level: api_types::BroadcastValidationQuery,
-             block_bytes: Bytes,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
+            move |validation_level: api_types::BroadcastValidationQuery,
+                  block_bytes: Bytes,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
                 task_spawner.spawn_async_with_rejection(Priority::P0, async move {
                     let block =
                         SignedBeaconBlock::<T::EthSpec>::from_ssz_bytes(&block_bytes, &chain.spec)
@@ -1405,9 +1410,9 @@ pub fn serve<T: BeaconChainTypes>(
                         &network_tx,
                         log,
                         validation_level.broadcast_validation,
+                        duplicate_block_status_code,
                     )
                     .await
-                    .map(|()| warp::reply().into_response())
                 })
             },
         );
@@ -1427,11 +1432,11 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |block: SignedBeaconBlock<T::EthSpec, BlindedPayload<_>>,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
+            move |block: SignedBlindedBeaconBlock<T::EthSpec>,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
                 task_spawner.spawn_async_with_rejection(Priority::P0, async move {
                     publish_blocks::publish_blinded_block(
                         block,
@@ -1439,9 +1444,9 @@ pub fn serve<T: BeaconChainTypes>(
                         &network_tx,
                         log,
                         BroadcastValidation::default(),
+                        duplicate_block_status_code,
                     )
                     .await
-                    .map(|()| warp::reply().into_response())
                 })
             },
         );
@@ -1457,13 +1462,13 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |block_bytes: Bytes,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
+            move |block_bytes: Bytes,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
                 task_spawner.spawn_async_with_rejection(Priority::P0, async move {
-                    let block = SignedBeaconBlock::<T::EthSpec, BlindedPayload<_>>::from_ssz_bytes(
+                    let block = SignedBlindedBeaconBlock::<T::EthSpec>::from_ssz_bytes(
                         &block_bytes,
                         &chain.spec,
                     )
@@ -1476,9 +1481,9 @@ pub fn serve<T: BeaconChainTypes>(
                         &network_tx,
                         log,
                         BroadcastValidation::default(),
+                        duplicate_block_status_code,
                     )
                     .await
-                    .map(|()| warp::reply().into_response())
                 })
             },
         );
@@ -1494,87 +1499,63 @@ pub fn serve<T: BeaconChainTypes>(
         .and(network_tx_filter.clone())
         .and(log_filter.clone())
         .then(
-            |validation_level: api_types::BroadcastValidationQuery,
-             block: SignedBeaconBlock<T::EthSpec, BlindedPayload<_>>,
-             task_spawner: TaskSpawner<T::EthSpec>,
-             chain: Arc<BeaconChain<T>>,
-             network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-             log: Logger| {
-                task_spawner.spawn_async(Priority::P0, async move {
-                    match publish_blocks::publish_blinded_block(
+            move |validation_level: api_types::BroadcastValidationQuery,
+                  block: SignedBlindedBeaconBlock<T::EthSpec>,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
+                task_spawner.spawn_async_with_rejection(Priority::P0, async move {
+                    publish_blocks::publish_blinded_block(
                         block,
                         chain,
                         &network_tx,
                         log,
                         validation_level.broadcast_validation,
+                        duplicate_block_status_code,
                     )
                     .await
-                    {
-                        Ok(()) => warp::reply().into_response(),
-                        Err(e) => match warp_utils::reject::handle_rejection(e).await {
-                            Ok(reply) => reply.into_response(),
-                            Err(_) => warp::reply::with_status(
-                                StatusCode::INTERNAL_SERVER_ERROR,
-                                eth2::StatusCode::INTERNAL_SERVER_ERROR,
-                            )
-                            .into_response(),
-                        },
-                    }
                 })
             },
         );
 
-    let post_beacon_blinded_blocks_v2_ssz =
-        eth_v2
-            .and(warp::path("beacon"))
-            .and(warp::path("blinded_blocks"))
-            .and(warp::query::<api_types::BroadcastValidationQuery>())
-            .and(warp::path::end())
-            .and(warp::body::bytes())
-            .and(chain_filter.clone())
-            .and(network_tx_filter.clone())
-            .and(log_filter.clone())
-            .then(
-                |validation_level: api_types::BroadcastValidationQuery,
-                 block_bytes: Bytes,
-                 chain: Arc<BeaconChain<T>>,
-                 network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
-                 log: Logger| async move {
-                    let block =
-                        match SignedBeaconBlock::<T::EthSpec, BlindedPayload<_>>::from_ssz_bytes(
-                            &block_bytes,
-                            &chain.spec,
-                        ) {
-                            Ok(data) => data,
-                            Err(_) => {
-                                return warp::reply::with_status(
-                                    StatusCode::BAD_REQUEST,
-                                    eth2::StatusCode::BAD_REQUEST,
-                                )
-                                .into_response();
-                            }
-                        };
-                    match publish_blocks::publish_blinded_block(
+    let post_beacon_blinded_blocks_v2_ssz = eth_v2
+        .and(warp::path("beacon"))
+        .and(warp::path("blinded_blocks"))
+        .and(warp::query::<api_types::BroadcastValidationQuery>())
+        .and(warp::path::end())
+        .and(warp::body::bytes())
+        .and(task_spawner_filter.clone())
+        .and(chain_filter.clone())
+        .and(network_tx_filter.clone())
+        .and(log_filter.clone())
+        .then(
+            move |validation_level: api_types::BroadcastValidationQuery,
+                  block_bytes: Bytes,
+                  task_spawner: TaskSpawner<T::EthSpec>,
+                  chain: Arc<BeaconChain<T>>,
+                  network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
+                  log: Logger| {
+                task_spawner.spawn_async_with_rejection(Priority::P0, async move {
+                    let block = SignedBlindedBeaconBlock::<T::EthSpec>::from_ssz_bytes(
+                        &block_bytes,
+                        &chain.spec,
+                    )
+                    .map_err(|e| {
+                        warp_utils::reject::custom_bad_request(format!("invalid SSZ: {e:?}"))
+                    })?;
+                    publish_blocks::publish_blinded_block(
                         block,
                         chain,
                         &network_tx,
                         log,
                         validation_level.broadcast_validation,
+                        duplicate_block_status_code,
                     )
                     .await
-                    {
-                        Ok(()) => warp::reply().into_response(),
-                        Err(e) => match warp_utils::reject::handle_rejection(e).await {
-                            Ok(reply) => reply.into_response(),
-                            Err(_) => warp::reply::with_status(
-                                StatusCode::INTERNAL_SERVER_ERROR,
-                                eth2::StatusCode::INTERNAL_SERVER_ERROR,
-                            )
-                            .into_response(),
-                        },
-                    }
-                },
-            );
+                })
+            },
+        );
 
     let block_id_or_err = warp::path::param::<BlockId>().or_else(|_| async {
         Err(warp_utils::reject::custom_bad_request(

beacon_node/http_api/src/publish_blocks.rs

@@ -4,7 +4,7 @@ use beacon_chain::{
     BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, IntoGossipVerifiedBlock,
     NotifyExecutionLayer,
 };
-use eth2::types::BroadcastValidation;
+use eth2::types::{BroadcastValidation, ErrorMessage};
 use execution_layer::ProvenancedPayload;
 use lighthouse_network::PubsubMessage;
 use network::NetworkMessage;
@@ -19,7 +19,8 @@ use types::{
     AbstractExecPayload, BeaconBlockRef, BlindedPayload, EthSpec, ExecPayload, ExecutionBlockHash,
     FullPayload, Hash256, SignedBeaconBlock,
 };
-use warp::Rejection;
+use warp::http::StatusCode;
+use warp::{reply::Response, Rejection, Reply};
 
 pub enum ProvenancedBlock<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>> {
     /// The payload was built using a local EE.
@@ -47,7 +48,8 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>>(
     network_tx: &UnboundedSender<NetworkMessage<T::EthSpec>>,
     log: Logger,
     validation_level: BroadcastValidation,
-) -> Result<(), Rejection> {
+    duplicate_status_code: StatusCode,
+) -> Result<Response, Rejection> {
     let seen_timestamp = timestamp_now();
     let (block, is_locally_built_block) = match provenanced_block {
         ProvenancedBlock::Local(block, _) => (block, true),
@@ -75,10 +77,30 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>>(
     };
 
     /* if we can form a `GossipVerifiedBlock`, we've passed our basic gossip checks */
-    let gossip_verified_block = block.into_gossip_verified_block(&chain).map_err(|e| {
-        warn!(log, "Not publishing block, not gossip verified"; "slot" => beacon_block.slot(), "error" => ?e);
-        warp_utils::reject::custom_bad_request(e.to_string())
-    })?;
+    let gossip_verified_block = match block.into_gossip_verified_block(&chain) {
+        Ok(b) => b,
+        Err(BlockError::BlockIsAlreadyKnown) => {
+            // Allow the status code for duplicate blocks to be overridden based on config.
+            return Ok(warp::reply::with_status(
+                warp::reply::json(&ErrorMessage {
+                    code: duplicate_status_code.as_u16(),
+                    message: "duplicate block".to_string(),
+                    stacktraces: vec![],
+                }),
+                duplicate_status_code,
+            )
+            .into_response());
+        }
+        Err(e) => {
+            warn!(
+                log,
+                "Not publishing block - not gossip verified";
+                "slot" => beacon_block.slot(),
+                "error" => ?e
+            );
+            return Err(warp_utils::reject::custom_bad_request(e.to_string()));
+        }
+    };
 
     let block_root = block_root.unwrap_or(gossip_verified_block.block_root);
 
@@ -167,8 +189,7 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>>(
                     &log,
                 )
             }
-
-            Ok(())
+            Ok(warp::reply().into_response())
         }
         Err(BlockError::BeaconChainError(BeaconChainError::UnableToPublish)) => {
             Err(warp_utils::reject::custom_server_error(
@@ -178,10 +199,6 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>>(
         Err(BlockError::Slashable) => Err(warp_utils::reject::custom_bad_request(
             "proposal for this slot and proposer has already been seen".to_string(),
         )),
-        Err(BlockError::BlockIsAlreadyKnown) => {
-            info!(log, "Block from HTTP API already known"; "block" => ?block_root);
-            Ok(())
-        }
         Err(e) => {
             if let BroadcastValidation::Gossip = validation_level {
                 Err(warp_utils::reject::broadcast_without_import(format!("{e}")))
@@ -208,7 +225,8 @@ pub async fn publish_blinded_block<T: BeaconChainTypes>(
     network_tx: &UnboundedSender<NetworkMessage<T::EthSpec>>,
     log: Logger,
     validation_level: BroadcastValidation,
-) -> Result<(), Rejection> {
+    duplicate_status_code: StatusCode,
+) -> Result<Response, Rejection> {
     let block_root = block.canonical_root();
     let full_block: ProvenancedBlock<T, Arc<SignedBeaconBlock<T::EthSpec>>> =
         reconstruct_block(chain.clone(), block_root, block, log.clone()).await?;
@@ -219,6 +237,7 @@ pub async fn publish_blinded_block<T: BeaconChainTypes>(
         network_tx,
         log,
         validation_level,
+        duplicate_status_code,
     )
     .await
 }

beacon_node/http_api/src/task_spawner.rs

@@ -159,46 +159,6 @@ impl<E: EthSpec> TaskSpawner<E> {
                 .and_then(|x| x)
         }
     }
-
-    /// Executes an async task which always returns a `Response`.
-    pub async fn spawn_async(
-        self,
-        priority: Priority,
-        func: impl Future<Output = Response> + Send + Sync + 'static,
-    ) -> Response {
-        if let Some(beacon_processor_send) = &self.beacon_processor_send {
-            // Create a wrapper future that will execute `func` and send the
-            // result to a channel held by this thread.
-            let (tx, rx) = oneshot::channel();
-            let process_fn = async move {
-                // Await the future, collect the return value.
-                let func_result = func.await;
-                // Send the result down the channel. Ignore any failures; the
-                // send can only fail if the receiver is dropped.
-                let _ = tx.send(func_result);
-            };
-
-            // Send the function to the beacon processor for execution at some arbitrary time.
-            let result = send_to_beacon_processor(
-                beacon_processor_send,
-                priority,
-                BlockingOrAsync::Async(Box::pin(process_fn)),
-                rx,
-            )
-            .await;
-            convert_rejection(result).await
-        } else {
-            // There is no beacon processor so spawn a task directly on the
-            // tokio executor.
-            tokio::task::spawn(func).await.unwrap_or_else(|e| {
-                warp::reply::with_status(
-                    warp::reply::json(&format!("Tokio did not execute task: {e:?}")),
-                    eth2::StatusCode::INTERNAL_SERVER_ERROR,
-                )
-                .into_response()
-            })
-        }
-    }
 }
 
 /// Send a task to the beacon processor and await execution.

beacon_node/http_api/src/test_utils.rs

@@ -23,7 +23,7 @@ use network::{NetworkReceivers, NetworkSenders};
 use sensitive_url::SensitiveUrl;
 use slog::Logger;
 use std::future::Future;
-use std::net::{IpAddr, Ipv4Addr, SocketAddr};
+use std::net::SocketAddr;
 use std::sync::Arc;
 use std::time::Duration;
 use store::MemoryStore;
@@ -220,15 +220,9 @@ pub async fn create_api_server_on_port<T: BeaconChainTypes>(
     let ctx = Arc::new(Context {
         config: Config {
             enabled: true,
-            listen_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)),
             listen_port: port,
-            allow_origin: None,
-            tls_config: None,
-            allow_sync_stalled: false,
             data_dir: std::path::PathBuf::from(DEFAULT_ROOT_DIR),
-            spec_fork_name: None,
-            sse_capacity_multiplier: 1,
-            enable_beacon_processor: true,
+            ..Config::default()
         },
         chain: Some(chain),
         network_senders: Some(network_senders),