From 522b3cbaab7ae5d4d3554768a90d83ce3fec48ae Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Sun, 23 Feb 2025 19:39:13 -0800 Subject: [PATCH] Fix builder API headers (#7009) Resolves https://github.com/sigp/lighthouse/issues/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. --- beacon_node/beacon_chain/src/test_utils.rs | 1 + beacon_node/builder_client/src/lib.rs | 106 ++++++++++++++++----- beacon_node/execution_layer/src/lib.rs | 22 ++++- beacon_node/src/cli.rs | 9 ++ beacon_node/src/config.rs | 2 + book/src/help_bn.md | 2 + lighthouse/tests/beacon_node.rs | 34 +++++++ 7 files changed, 149 insertions(+), 27 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 8c9e3929f6b..24c85b3e070 100644 --- 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 index 5f64ac7e43c..6d82542cefc 100644 --- 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,7 +62,12 @@ pub struct BuilderHttpClient { server: SensitiveUrl, timeouts: Timeouts, user_agent: String, - ssz_enabled: Arc, + /// 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, } impl BuilderHttpClient { @@ -65,6 +75,7 @@ impl BuilderHttpClient { server: SensitiveUrl, user_agent: Option, builder_header_timeout: Option, + disable_ssz: bool, ) -> Result { 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,7 +136,7 @@ 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); }; @@ -132,7 +144,7 @@ impl BuilderHttpClient { 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( @@ -213,7 +227,7 @@ impl BuilderHttpClient { &self, url: U, ssz_body: Vec, - mut headers: HeaderMap, + headers: HeaderMap, timeout: Option, ) -> Result { let mut builder = self.client.post(url); @@ -221,11 +235,6 @@ impl BuilderHttpClient { 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 index 6e5e4fca01e..4fd7188c206 100644 --- 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, /// User agent to send with requests to the builder API. pub builder_user_agent: Option, + /// 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, /// The default fee recipient to use on the beacon node if none if provided from @@ -470,6 +472,7 @@ impl ExecutionLayer { builder_url, builder_user_agent, builder_header_timeout, + disable_builder_ssz_requests, secret_file, suggested_fee_recipient, jwt_id, @@ -539,7 +542,12 @@ impl ExecutionLayer { }; 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,11 +570,13 @@ impl ExecutionLayer { builder_url: SensitiveUrl, builder_user_agent: Option, builder_header_timeout: Option, + 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!( @@ -574,6 +584,7 @@ impl ExecutionLayer { "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 ExecutionLayer { 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 index 4c2daecdd34..2c8b271bd25 100644 --- 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 index 24d569bea22..84320762d6c 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -346,6 +346,8 @@ pub fn get_config( 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 index cbcb1ec5a3d..79c8d8ead85 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -28,6 +28,8 @@ Options: network. Multiaddr is also supported. --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 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 index 03314930b9b..da10c2c4bd9 100644 --- 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;