Fix builder API headers (#7009)

pawanjay176 · web-flow · commit 522b3cbaab7a · 2025-02-24T03:39:13.000Z
Resolves #7000 Set the accept header on builder to the correct value when requesting ssz. This PR also adds a flag to disable ssz over the builder api altogether. In the case that builders/relays have an ssz bug, we can react quickly by asking clients to restart their nodes with the `--disable-ssz-builder` flag to force json. I'm not fully convinced if this is useful so open to removing it or opening another PR for it. Testing this currently.
diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs
@@ -779,6 +779,7 @@ where
                 SensitiveUrl::parse(format!("http://127.0.0.1:{port}").as_str()).unwrap(),
                 None,
                 None,
+                false,
             )
             .unwrap();
 
diff --git a/beacon_node/builder_client/src/lib.rs b/beacon_node/builder_client/src/lib.rs
@@ -29,6 +29,11 @@ pub const DEFAULT_GET_HEADER_TIMEOUT_MILLIS: u64 = 1000;
 /// Default user agent for HTTP requests.
 pub const DEFAULT_USER_AGENT: &str = lighthouse_version::VERSION;
 
+/// The value we set on the `ACCEPT` http header to indicate a preference for ssz response.
+pub const PREFERENCE_ACCEPT_VALUE: &str = "application/octet-stream;q=1.0,application/json;q=0.9";
+/// Only accept json responses.
+pub const JSON_ACCEPT_VALUE: &str = "application/json";
+
 #[derive(Clone)]
 pub struct Timeouts {
     get_header: Duration,
@@ -57,14 +62,20 @@ pub struct BuilderHttpClient {
     server: SensitiveUrl,
     timeouts: Timeouts,
     user_agent: String,
-    ssz_enabled: Arc<AtomicBool>,
+    /// Only use json for all requests/responses types.
+    disable_ssz: bool,
+    /// Indicates that the `get_header` response had content-type ssz
+    /// so we can set content-type header to ssz to make the `submit_blinded_blocks`
+    /// request.
+    ssz_available: Arc<AtomicBool>,
 }
 
 impl BuilderHttpClient {
     pub fn new(
         server: SensitiveUrl,
         user_agent: Option<String>,
         builder_header_timeout: Option<Duration>,
+        disable_ssz: bool,
     ) -> Result<Self, Error> {
         let user_agent = user_agent.unwrap_or(DEFAULT_USER_AGENT.to_string());
         let client = reqwest::Client::builder().user_agent(&user_agent).build()?;
@@ -73,7 +84,8 @@ impl BuilderHttpClient {
             server,
             timeouts: Timeouts::new(builder_header_timeout),
             user_agent,
-            ssz_enabled: Arc::new(false.into()),
+            disable_ssz,
+            ssz_available: Arc::new(false.into()),
         })
     }
 
@@ -124,15 +136,15 @@ impl BuilderHttpClient {
 
         let Ok(Some(fork_name)) = self.fork_name_from_header(&headers) else {
             // if no fork version specified, attempt to fallback to JSON
-            self.ssz_enabled.store(false, Ordering::SeqCst);
+            self.ssz_available.store(false, Ordering::SeqCst);
             return serde_json::from_slice(&response_bytes).map_err(Error::InvalidJson);
         };
 
         let content_type = self.content_type_from_header(&headers);
 
         match content_type {
             ContentType::Ssz => {
-                self.ssz_enabled.store(true, Ordering::SeqCst);
+                self.ssz_available.store(true, Ordering::SeqCst);
                 T::from_ssz_bytes_by_fork(&response_bytes, fork_name)
                     .map(|data| ForkVersionedResponse {
                         version: Some(fork_name),
@@ -142,15 +154,17 @@ impl BuilderHttpClient {
                     .map_err(Error::InvalidSsz)
             }
             ContentType::Json => {
-                self.ssz_enabled.store(false, Ordering::SeqCst);
+                self.ssz_available.store(false, Ordering::SeqCst);
                 serde_json::from_slice(&response_bytes).map_err(Error::InvalidJson)
             }
         }
     }
 
     /// Return `true` if the most recently received response from the builder had SSZ Content-Type.
-    pub fn is_ssz_enabled(&self) -> bool {
-        self.ssz_enabled.load(Ordering::SeqCst)
+    /// Return `false` otherwise.
+    /// Also returns `false` if we have explicitly disabled ssz.
+    pub fn is_ssz_available(&self) -> bool {
+        !self.disable_ssz && self.ssz_available.load(Ordering::SeqCst)
     }
 
     async fn get_with_timeout<T: DeserializeOwned, U: IntoUrl>(
@@ -213,19 +227,14 @@ impl BuilderHttpClient {
         &self,
         url: U,
         ssz_body: Vec<u8>,
-        mut headers: HeaderMap,
+        headers: HeaderMap,
         timeout: Option<Duration>,
     ) -> Result<Response, Error> {
         let mut builder = self.client.post(url);
         if let Some(timeout) = timeout {
             builder = builder.timeout(timeout);
         }
 
-        headers.insert(
-            CONTENT_TYPE_HEADER,
-            HeaderValue::from_static(SSZ_CONTENT_TYPE_HEADER),
-        );
-
         let response = builder
             .headers(headers)
             .body(ssz_body)
@@ -292,9 +301,21 @@ impl BuilderHttpClient {
             .push("blinded_blocks");
 
         let mut headers = HeaderMap::new();
-        if let Ok(value) = HeaderValue::from_str(&blinded_block.fork_name_unchecked().to_string()) {
-            headers.insert(CONSENSUS_VERSION_HEADER, value);
-        }
+        headers.insert(
+            CONSENSUS_VERSION_HEADER,
+            HeaderValue::from_str(&blinded_block.fork_name_unchecked().to_string())
+                .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+        );
+        headers.insert(
+            CONTENT_TYPE_HEADER,
+            HeaderValue::from_str(SSZ_CONTENT_TYPE_HEADER)
+                .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+        );
+        headers.insert(
+            ACCEPT,
+            HeaderValue::from_str(PREFERENCE_ACCEPT_VALUE)
+                .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+        );
 
         let result = self
             .post_ssz_with_raw_response(
@@ -326,9 +347,21 @@ impl BuilderHttpClient {
             .push("blinded_blocks");
 
         let mut headers = HeaderMap::new();
-        if let Ok(value) = HeaderValue::from_str(&blinded_block.fork_name_unchecked().to_string()) {
-            headers.insert(CONSENSUS_VERSION_HEADER, value);
-        }
+        headers.insert(
+            CONSENSUS_VERSION_HEADER,
+            HeaderValue::from_str(&blinded_block.fork_name_unchecked().to_string())
+                .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+        );
+        headers.insert(
+            CONTENT_TYPE_HEADER,
+            HeaderValue::from_str(JSON_CONTENT_TYPE_HEADER)
+                .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+        );
+        headers.insert(
+            ACCEPT,
+            HeaderValue::from_str(JSON_ACCEPT_VALUE)
+                .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+        );
 
         Ok(self
             .post_with_raw_response(
@@ -362,12 +395,20 @@ impl BuilderHttpClient {
             .push(pubkey.as_hex_string().as_str());
 
         let mut headers = HeaderMap::new();
-        if let Ok(ssz_content_type_header) = HeaderValue::from_str(&format!(
-            "{}; q=1.0,{}; q=0.9",
-            SSZ_CONTENT_TYPE_HEADER, JSON_CONTENT_TYPE_HEADER
-        )) {
-            headers.insert(ACCEPT, ssz_content_type_header);
-        };
+        if self.disable_ssz {
+            headers.insert(
+                ACCEPT,
+                HeaderValue::from_str(JSON_CONTENT_TYPE_HEADER)
+                    .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+            );
+        } else {
+            // Indicate preference for ssz response in the accept header
+            headers.insert(
+                ACCEPT,
+                HeaderValue::from_str(PREFERENCE_ACCEPT_VALUE)
+                    .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?,
+            );
+        }
 
         let resp = self
             .get_with_header(path, self.timeouts.get_header, headers)
@@ -395,3 +436,18 @@ impl BuilderHttpClient {
             .await
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_headers_no_panic() {
+        for fork in ForkName::list_all() {
+            assert!(HeaderValue::from_str(&fork.to_string()).is_ok());
+        }
+        assert!(HeaderValue::from_str(PREFERENCE_ACCEPT_VALUE).is_ok());
+        assert!(HeaderValue::from_str(JSON_ACCEPT_VALUE).is_ok());
+        assert!(HeaderValue::from_str(JSON_CONTENT_TYPE_HEADER).is_ok());
+    }
+}
diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs
@@ -441,6 +441,8 @@ pub struct Config {
     pub builder_header_timeout: Option<Duration>,
     /// User agent to send with requests to the builder API.
     pub builder_user_agent: Option<String>,
+    /// Disable ssz requests on builder. Only use json.
+    pub disable_builder_ssz_requests: bool,
     /// JWT secret for the above endpoint running the engine api.
     pub secret_file: Option<PathBuf>,
     /// The default fee recipient to use on the beacon node if none if provided from
@@ -470,6 +472,7 @@ impl<E: EthSpec> ExecutionLayer<E> {
             builder_url,
             builder_user_agent,
             builder_header_timeout,
+            disable_builder_ssz_requests,
             secret_file,
             suggested_fee_recipient,
             jwt_id,
@@ -539,7 +542,12 @@ impl<E: EthSpec> ExecutionLayer<E> {
         };
 
         if let Some(builder_url) = builder_url {
-            el.set_builder_url(builder_url, builder_user_agent, builder_header_timeout)?;
+            el.set_builder_url(
+                builder_url,
+                builder_user_agent,
+                builder_header_timeout,
+                disable_builder_ssz_requests,
+            )?;
         }
 
         Ok(el)
@@ -562,18 +570,21 @@ impl<E: EthSpec> ExecutionLayer<E> {
         builder_url: SensitiveUrl,
         builder_user_agent: Option<String>,
         builder_header_timeout: Option<Duration>,
+        disable_ssz: bool,
     ) -> Result<(), Error> {
         let builder_client = BuilderHttpClient::new(
             builder_url.clone(),
             builder_user_agent,
             builder_header_timeout,
+            disable_ssz,
         )
         .map_err(Error::Builder)?;
         info!(
             self.log(),
             "Using external block builder";
             "builder_url" => ?builder_url,
             "local_user_agent" => builder_client.get_user_agent(),
+            "ssz_disabled" => disable_ssz
         );
         self.inner.builder.swap(Some(Arc::new(builder_client)));
         Ok(())
@@ -1901,7 +1912,14 @@ impl<E: EthSpec> ExecutionLayer<E> {
         if let Some(builder) = self.builder() {
             let (payload_result, duration) =
                 timed_future(metrics::POST_BLINDED_PAYLOAD_BUILDER, async {
-                    if builder.is_ssz_enabled() {
+                    let ssz_enabled = builder.is_ssz_available();
+                    debug!(
+                        self.log(),
+                        "Calling submit_blinded_block on builder";
+                        "block_root" => ?block_root,
+                        "ssz" => ssz_enabled
+                    );
+                    if ssz_enabled {
                         builder
                             .post_builder_blinded_blocks_ssz(block)
                             .await
diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs
@@ -1460,6 +1460,15 @@ pub fn cli_app() -> Command {
                 .action(ArgAction::Set)
                 .display_order(0)
         )
+        .arg(
+            Arg::new("builder-disable-ssz")
+                .long("builder-disable-ssz")
+                .value_name("BOOLEAN")
+                .help("Disables sending requests using SSZ over the builder API.")
+                .requires("builder")
+                .action(ArgAction::SetTrue)
+                .display_order(0)
+        )
         .arg(
             Arg::new("reset-payload-statuses")
                 .long("reset-payload-statuses")
diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs
@@ -346,6 +346,8 @@ pub fn get_config<E: EthSpec>(
         el_config.builder_header_timeout =
             clap_utils::parse_optional(cli_args, "builder-header-timeout")?
                 .map(Duration::from_millis);
+
+        el_config.disable_builder_ssz_requests = cli_args.get_flag("builder-disable-ssz");
     }
 
     // Set config values from parse values.
diff --git a/book/src/help_bn.md b/book/src/help_bn.md
@@ -28,6 +28,8 @@ Options:
           network. Multiaddr is also supported.
       --builder <builder>
           The URL of a service compatible with the MEV-boost API.
+      --builder-disable-ssz
+          Disables sending requests using SSZ over the builder API.
       --builder-fallback-epochs-since-finalization <builder-fallback-epochs-since-finalization>
           If this node is proposing a block and the chain has not finalized
           within this number of epochs, it will NOT query any connected
diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs
@@ -720,6 +720,40 @@ fn builder_user_agent() {
     );
 }
 
+#[test]
+fn test_builder_disable_ssz_flag() {
+    run_payload_builder_flag_test_with_config(
+        "builder",
+        "http://meow.cats",
+        None,
+        None,
+        |config| {
+            assert!(
+                !config
+                    .execution_layer
+                    .as_ref()
+                    .unwrap()
+                    .disable_builder_ssz_requests,
+            );
+        },
+    );
+    run_payload_builder_flag_test_with_config(
+        "builder",
+        "http://meow.cats",
+        Some("builder-disable-ssz"),
+        None,
+        |config| {
+            assert!(
+                config
+                    .execution_layer
+                    .as_ref()
+                    .unwrap()
+                    .disable_builder_ssz_requests,
+            );
+        },
+    );
+}
+
 fn run_jwt_optional_flags_test(jwt_flag: &str, jwt_id_flag: &str, jwt_version_flag: &str) {
     use sensitive_url::SensitiveUrl;
 

Original file line number	Diff line number	Diff line change
`@@ -779,6 +779,7 @@ where`
`779`	`779`	`SensitiveUrl::parse(format!("http://127.0.0.1:{port}").as_str()).unwrap(),`
`780`	`780`	`None,`
`781`	`781`	`None,`
	`782`	`+ false,`
`782`	`783`	`)`
`783`	`784`	`.unwrap();`
`784`	`785`
Original file line number	Diff line number	Diff line change
`@@ -346,6 +346,8 @@ pub fn get_config<E: EthSpec>(`
`346`	`346`	`el_config.builder_header_timeout =`
`347`	`347`	`clap_utils::parse_optional(cli_args, "builder-header-timeout")?`
`348`	`348`	`.map(Duration::from_millis);`
	`349`	`+`
	`350`	`+ el_config.disable_builder_ssz_requests = cli_args.get_flag("builder-disable-ssz");`
`349`	`351`	`}`
`350`	`352`
`351`	`353`	`// Set config values from parse values.`