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

Closes sigp#4473 (take 3)

- 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.

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/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

@@ -21,7 +21,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;
@@ -217,15 +217,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),

beacon_node/http_api/tests/broadcast_validation_tests.rs

@@ -364,13 +364,14 @@ pub async fn consensus_partial_pass_only_consensus() {
     /* submit `block_b` which should induce equivocation */
     let channel = tokio::sync::mpsc::unbounded_channel();
 
-    let publication_result: Result<(), Rejection> = publish_block(
+    let publication_result = publish_block(
         None,
         ProvenancedBlock::local(gossip_block_b.unwrap()),
         tester.harness.chain.clone(),
         &channel.0,
         test_logger,
         validation_level.unwrap(),
+        StatusCode::ACCEPTED,
     )
     .await;
 
@@ -641,13 +642,14 @@ pub async fn equivocation_consensus_late_equivocation() {
 
     let channel = tokio::sync::mpsc::unbounded_channel();
 
-    let publication_result: Result<(), Rejection> = publish_block(
+    let publication_result = publish_block(
         None,
         ProvenancedBlock::local(gossip_block_b.unwrap()),
         tester.harness.chain,
         &channel.0,
         test_logger,
         validation_level.unwrap(),
+        StatusCode::ACCEPTED,
     )
     .await;
 
@@ -1294,12 +1296,13 @@ pub async fn blinded_equivocation_consensus_late_equivocation() {
 
     let channel = tokio::sync::mpsc::unbounded_channel();
 
-    let publication_result: Result<(), Rejection> = publish_blinded_block(
+    let publication_result = publish_blinded_block(
         block_b,
         tester.harness.chain,
         &channel.0,
         test_logger,
         validation_level.unwrap(),
+        StatusCode::ACCEPTED,
     )
     .await;
 

beacon_node/http_api/tests/tests.rs

@@ -8,7 +8,7 @@ use eth2::{
     mixin::{RequestAccept, ResponseForkName, ResponseOptional},
     reqwest::RequestBuilder,
     types::{BlockId as CoreBlockId, ForkChoiceNode, StateId as CoreStateId, *},
-    BeaconNodeHttpClient, Error, Timeouts,
+    BeaconNodeHttpClient, Error, StatusCode, Timeouts,
 };
 use execution_layer::test_utils::TestingBuilder;
 use execution_layer::test_utils::DEFAULT_BUILDER_THRESHOLD_WEI;
@@ -1318,6 +1318,63 @@ impl ApiTester {
         self
     }
 
+    pub async fn test_post_beacon_blocks_duplicate(self) -> Self {
+        let block = self
+            .harness
+            .make_block(
+                self.harness.get_current_state(),
+                self.harness.get_current_slot(),
+            )
+            .await
+            .0;
+
+        assert!(self.client.post_beacon_blocks(&block).await.is_ok());
+
+        let blinded_block = block.clone_as_blinded();
+
+        // Test all the POST methods in sequence, they should all behave the same.
+        let responses = vec![
+            self.client.post_beacon_blocks(&block).await.unwrap_err(),
+            self.client
+                .post_beacon_blocks_v2(&block, None)
+                .await
+                .unwrap_err(),
+            self.client
+                .post_beacon_blocks_ssz(&block)
+                .await
+                .unwrap_err(),
+            self.client
+                .post_beacon_blocks_v2_ssz(&block, None)
+                .await
+                .unwrap_err(),
+            self.client
+                .post_beacon_blinded_blocks(&blinded_block)
+                .await
+                .unwrap_err(),
+            self.client
+                .post_beacon_blinded_blocks_v2(&blinded_block, None)
+                .await
+                .unwrap_err(),
+            self.client
+                .post_beacon_blinded_blocks_ssz(&blinded_block)
+                .await
+                .unwrap_err(),
+            self.client
+                .post_beacon_blinded_blocks_v2_ssz(&blinded_block, None)
+                .await
+                .unwrap_err(),
+        ];
+        for (i, response) in responses.into_iter().enumerate() {
+            assert_eq!(
+                response.status().unwrap(),
+                StatusCode::ACCEPTED,
+                "response {i}"
+            );
+        }
+
+        self
+    }
+
     pub async fn test_beacon_blocks(self) -> Self {
         for block_id in self.interesting_block_ids() {
             let expected = block_id
@@ -4651,6 +4708,14 @@ async fn post_beacon_blocks_invalid() {
         .await;
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn post_beacon_blocks_duplicate() {
+    ApiTester::new()
+        .await
+        .test_post_beacon_blocks_duplicate()
+        .await;
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn beacon_pools_post_attestations_valid() {
     ApiTester::new()

beacon_node/src/cli.rs

@@ -398,6 +398,15 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .help("Multiplier to apply to the length of HTTP server-sent-event (SSE) channels. \
                        Increasing this value can prevent messages from being dropped.")
         )
+        .arg(
+            Arg::with_name("http-duplicate-block-status")
+                .long("http-duplicate-block-status")
+                .takes_value(true)
+                .default_value("202")
+                .value_name("STATUS_CODE")
+                .help("Status code to send when a block that is already known is POSTed to the \
+                       HTTP API.")
+        )
         .arg(
             Arg::with_name("http-enable-beacon-processor")
                 .long("http-enable-beacon-processor")

beacon_node/src/config.rs

@@ -156,6 +156,9 @@ pub fn get_config<E: EthSpec>(
     client_config.http_api.enable_beacon_processor =
         parse_required(cli_args, "http-enable-beacon-processor")?;
 
+    client_config.http_api.duplicate_block_status_code =
+        parse_required(cli_args, "http-duplicate-block-status")?;
+
     if let Some(cache_size) = clap_utils::parse_optional(cli_args, "shuffling-cache-size")? {
         client_config.chain.shuffling_cache_size = cache_size;
     }

common/eth2/src/types.rs

@@ -1328,6 +1328,26 @@ pub struct BroadcastValidationQuery {
     pub broadcast_validation: BroadcastValidation,
 }
 
+pub mod serde_status_code {
+    use crate::StatusCode;
+    use serde::{de::Error, Deserialize, Serialize};
+
+    pub fn serialize<S>(status_code: &StatusCode, ser: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        status_code.as_u16().serialize(ser)
+    }
+
+    pub fn deserialize<'de, D>(de: D) -> Result<StatusCode, D::Error>
+    where
+        D: serde::de::Deserializer<'de>,
+    {
+        let status_code = u16::deserialize(de)?;
+        StatusCode::try_from(status_code).map_err(D::Error::custom)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

lighthouse/tests/beacon_node.rs

@@ -2426,3 +2426,22 @@ fn http_sse_capacity_multiplier_override() {
         .run_with_zero_port()
         .with_config(|config| assert_eq!(config.http_api.sse_capacity_multiplier, 10));
 }
+
+#[test]
+fn http_duplicate_block_status_default() {
+    CommandLineTest::new()
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert_eq!(config.http_api.duplicate_block_status_code.as_u16(), 202)
+        });
+}
+
+#[test]
+fn http_duplicate_block_status_override() {
+    CommandLineTest::new()
+        .flag("http-duplicate-block-status", Some("301"))
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert_eq!(config.http_api.duplicate_block_status_code.as_u16(), 301)
+        });
+}

validator_client/src/block_service.rs

@@ -10,7 +10,8 @@ use crate::{
     validator_store::{Error as ValidatorStoreError, ValidatorStore},
 };
 use environment::RuntimeContext;
-use eth2::BeaconNodeHttpClient;
+use eth2::{BeaconNodeHttpClient, StatusCode};
+use slog::Logger;
 use slog::{crit, debug, error, info, trace, warn};
 use slot_clock::SlotClock;
 use std::fmt::Debug;
@@ -572,12 +573,7 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
                             beacon_node
                                 .post_beacon_blocks(&signed_block)
                                 .await
-                                .map_err(|e| {
-                                    BlockError::Irrecoverable(format!(
-                                        "Error from beacon node when publishing block: {:?}",
-                                        e
-                                    ))
-                                })?
+                                .or_else(|e| handle_block_post_error(e, slot, log))?
                         }
                         BlockType::Blinded => {
                             let _post_timer = metrics::start_timer_vec(
@@ -587,12 +583,7 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
                             beacon_node
                                 .post_beacon_blinded_blocks(&signed_block)
                                 .await
-                                .map_err(|e| {
-                                    BlockError::Irrecoverable(format!(
-                                        "Error from beacon node when publishing block: {:?}",
-                                        e
-                                    ))
-                                })?
+                                .or_else(|e| handle_block_post_error(e, slot, log))?
                         }
                     }
                     Ok::<_, BlockError>(())
@@ -613,3 +604,29 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
         Ok(())
     }
 }
+
+fn handle_block_post_error(err: eth2::Error, slot: Slot, log: &Logger) -> Result<(), BlockError> {
+    // Handle non-200 success codes.
+    if let Some(status) = err.status() {
+        if status == StatusCode::ACCEPTED {
+            info!(
+                log,
+                "Block is already known to BN or might be invalid";
+                "slot" => slot,
+                "status_code" => status.as_u16(),
+            );
+            return Ok(());
+        } else if status.is_success() {
+            debug!(
+                log,
+                "Block published with non-standard success code";
+                "slot" => slot,
+                "status_code" => status.as_u16(),
+            );
+            return Ok(());
+        }
+    }
+    Err(BlockError::Irrecoverable(format!(
+        "Error from beacon node when publishing block: {err:?}",
+    )))
+}