fix(pbs): derive a proposal's fork from its slot, not the JSON decode

Author: JasonVranek

config.example.toml

@@ -4,7 +4,7 @@
 # Chain spec ID. Supported values:
 # A network ID. Supported values: Mainnet, Holesky, Sepolia, Hoodi. Lower case values e.g. "mainnet" are also accepted
 # A custom object, e.g., chain = { genesis_time_secs = 1695902400, path = "/path/to/spec.json" }, with a path to a chain spec file, either in .json format (e.g., as returned by the beacon endpoint /eth/v1/config/spec), or in .yml format (see examples in tests/data).
-# A custom object, e.g., chain = { genesis_time_secs = 1695902400, slot_time_secs = 12, genesis_fork_version = "0x01017000", chain_id = 17000 }.
+# A custom object, e.g., chain = { genesis_time_secs = 1695902400, slot_time_secs = 12, genesis_fork_version = "0x01017000", fulu_fork_slot = 5283840, chain_id = 17000 }.
 chain = "Holesky"
 
 # Configuration for the PBS module

crates/common/src/types.rs

@@ -181,10 +181,11 @@ impl Chain {
             Chain::Holesky => KnownChain::Holesky.fulu_fork_slot(),
             Chain::Sepolia => KnownChain::Sepolia.fulu_fork_slot(),
             Chain::Hoodi => KnownChain::Hoodi.fulu_fork_slot(),
-            Chain::Custom { slot_time_secs, .. } => *slot_time_secs,
+            Chain::Custom { fulu_fork_slot, .. } => *fulu_fork_slot,
         }
     }
 
+    /// TODO(gloas): this resolves to Electra or Fulu only
     pub fn fork_by_slot(&self, slot: u64) -> ForkName {
         if slot >= self.fulu_fork_slot() { ForkName::Fulu } else { ForkName::Electra }
     }
@@ -490,6 +491,49 @@ mod tests {
         })
     }
 
+    /// A custom chain must report its CONFIGURED fulu fork slot.
+    #[test]
+    fn custom_chain_reports_its_configured_fulu_fork_slot() {
+        let chain = Chain::Custom {
+            genesis_time_secs: 1,
+            slot_time_secs: 12,
+            genesis_fork_version: [1, 0, 0, 0],
+            fulu_fork_slot: 8192,
+            chain_id: U256::from(123),
+        };
+        assert_eq!(chain.fulu_fork_slot(), 8192, "must not return slot_time_secs");
+    }
+
+    /// `fork_by_slot` is the authoritative fork source for a proposal, so pin
+    /// its boundary exactly: the fork slot itself is already Fulu.
+    #[test]
+    fn fork_by_slot_switches_at_the_fork_boundary() {
+        let chain = Chain::Custom {
+            genesis_time_secs: 1,
+            slot_time_secs: 12,
+            genesis_fork_version: [1, 0, 0, 0],
+            fulu_fork_slot: 100,
+            chain_id: U256::from(123),
+        };
+        assert_eq!(chain.fork_by_slot(99), ForkName::Electra);
+        assert_eq!(chain.fork_by_slot(100), ForkName::Fulu);
+        assert_eq!(chain.fork_by_slot(101), ForkName::Fulu);
+    }
+
+    /// A chain that is Fulu from genesis (the devnet shape) reports Fulu at
+    /// every slot, including slot 0.
+    #[test]
+    fn fork_by_slot_handles_fulu_from_genesis() {
+        let chain = Chain::Custom {
+            genesis_time_secs: 1,
+            slot_time_secs: 12,
+            genesis_fork_version: [1, 0, 0, 0],
+            fulu_fork_slot: 0,
+            chain_id: U256::from(123),
+        };
+        assert_eq!(chain.fork_by_slot(0), ForkName::Fulu);
+    }
+
     #[test]
     fn test_spec_mainnet_data_json() {
         let a = env!("CARGO_MANIFEST_DIR");

crates/common/src/wire.rs

@@ -6,7 +6,7 @@ use axum::http::HeaderValue;
 use bytes::Bytes;
 use futures::StreamExt;
 use headers_accept::Accept;
-use lh_types::{BeaconBlock, ForkName};
+use lh_types::{BeaconBlock, ForkName, SignedBeaconBlock, map_fork_name};
 use mediatype::{MediaType, ReadParams, names};
 use reqwest::{
     Response,
@@ -403,8 +403,22 @@ pub fn deserialize_body(
     };
 
     match encoding {
-        EncodingType::Json => serde_json::from_slice::<SignedBlindedBeaconBlock>(&body)
-            .map_err(BodyDeserializeError::SerdeJsonError),
+        EncodingType::Json => match get_consensus_version_header(headers) {
+            // `SignedBlindedBeaconBlock` is untagged and Electra and Fulu are
+            // field-identical, so a plain `from_slice` stops at Electra and
+            // reports the wrong fork for every Fulu block.
+            Some(version) => Ok(map_fork_name!(
+                version,
+                SignedBeaconBlock,
+                serde_json::from_slice(&body).map_err(BodyDeserializeError::SerdeJsonError)?
+            )),
+            // builder-specs doesn't require the header for JSON bodies.
+            // A request without it still has to decode and an untagged decode would silently pick
+            // Electra. Assume Fulu to be conservative until ePBS warrants the refactor
+            None => Ok(SignedBeaconBlock::Fulu(
+                serde_json::from_slice(&body).map_err(BodyDeserializeError::SerdeJsonError)?,
+            )),
+        },
         EncodingType::Ssz => match get_consensus_version_header(headers) {
             Some(version) => SignedBlindedBeaconBlock::from_ssz_bytes_with(&body, |bytes| {
                 BeaconBlock::from_ssz_bytes_for_fork(bytes, version)
@@ -930,4 +944,62 @@ mod test {
         let err = deserialize_body(&headers, body).unwrap_err();
         assert!(matches!(err, BodyDeserializeError::MissingVersionHeader));
     }
+
+    /// A blinded block body, encoded as JSON, with the given consensus version.
+    ///
+    /// The Electra fixture is deliberately reused for the Fulu case: Electra
+    /// and Fulu have *identical* `BeaconBlockBody` fields, and their
+    /// `ExecutionPayloadHeader`s are identical too, so one byte string is a
+    /// valid encoding of BOTH forks. That ambiguity is the whole point - the
+    /// header is the only thing that can tell them apart.
+    fn blinded_block_json(version: Option<&'static str>) -> (HeaderMap, Bytes) {
+        let body = Bytes::from_static(include_bytes!(
+            "pbs/types/testdata/signed-blinded-beacon-block-electra.json"
+        ));
+        let mut headers = HeaderMap::new();
+        headers.insert(CONTENT_TYPE, HeaderValue::from_static(APPLICATION_JSON));
+        if let Some(version) = version {
+            headers.insert(
+                HeaderName::try_from(CONSENSUS_VERSION_HEADER).unwrap(),
+                HeaderValue::from_static(version),
+            );
+        }
+        (headers, body)
+    }
+
+    /// A JSON body labelled `fulu` must decode AS Fulu.
+    #[test]
+    fn test_deserialize_body_json_decodes_fulu_as_fulu() {
+        let (headers, body) = blinded_block_json(Some("fulu"));
+        let block = deserialize_body(&headers, body).expect("fulu body decodes");
+        assert_eq!(
+            block.fork_name_unchecked(),
+            ForkName::Fulu,
+            "decoded fork must follow Eth-Consensus-Version, not the untagged variant scan"
+        );
+    }
+
+    /// The same bytes labelled `electra` must still decode as Electra
+    /// The header is honoured in both directions
+    #[test]
+    fn test_deserialize_body_json_decodes_electra_as_electra() {
+        let (headers, body) = blinded_block_json(Some("electra"));
+        let block = deserialize_body(&headers, body).expect("electra body decodes");
+        assert_eq!(block.fork_name_unchecked(), ForkName::Electra);
+    }
+
+    /// The header is `required: false` for JSON requests in builder-specs
+    /// ("Required if request is SSZ encoded"), so a body without it must still
+    /// decode. It is assumed to be Fulu rather than left to the untagged scan,
+    /// which would silently pick Electra.
+    #[test]
+    fn test_deserialize_body_json_without_version_header_assumes_fulu() {
+        let (headers, body) = blinded_block_json(None);
+        let block = deserialize_body(&headers, body).expect("decodes without the header");
+        assert_eq!(
+            block.fork_name_unchecked(),
+            ForkName::Fulu,
+            "a headerless JSON body must not fall back to the untagged Electra match"
+        );
+    }
 }

crates/pbs/src/mev_boost/submit_block.rs

@@ -44,6 +44,17 @@ struct ProposalInfo {
 
     /// The version of the submit_block route being used
     api_version: BuilderApiVersion,
+
+    /// Fork of this proposal, derived from its SLOT via the chain's fork
+    /// schedule.
+    ///
+    /// Deliberately not `signed_blinded_block.fork_name_unchecked()`:
+    /// `SignedBlindedBeaconBlock` is an untagged enum, so a JSON body decodes
+    /// into the first variant that parses, and field-identical forks (Electra
+    /// and Fulu) cannot be told apart by shape. The slot can tell them apart,
+    /// and unlike `Eth-Consensus-Version` it does not depend on the proposer
+    /// sending a header that builder-specs makes optional for JSON.
+    fork: ForkName,
 }
 
 struct SubmitBlockResponseInfo {
@@ -92,8 +103,9 @@ pub async fn submit_block<S: BuilderApiState>(
     }
 
     // Send requests to all relays concurrently
+    let fork = state.config.chain.fork_by_slot(signed_blinded_block.slot().as_u64());
     let proposal_info =
-        Arc::new(ProposalInfo { signed_blinded_block, headers: send_headers, api_version });
+        Arc::new(ProposalInfo { signed_blinded_block, headers: send_headers, api_version, fork });
     let mut handles = Vec::with_capacity(state.all_relays().len());
     for relay in state.all_relays().iter() {
         handles.push(
@@ -201,12 +213,9 @@ async fn send_submit_block(
     // Extract the info needed for validation
     let got_block_hash = response.data.execution_payload.block_hash().0;
 
-    // Reject if response's fork mismatches BlindedBeaconBlock's fork
-    let expected_fork = match &proposal_info.signed_blinded_block.message() {
-        BlindedBeaconBlock::Electra(_) => ForkName::Electra,
-        BlindedBeaconBlock::Fulu(_) => ForkName::Fulu,
-        _ => return Err(PbsError::Validation(ValidationError::UnsupportedFork)),
-    };
+    // Reject if the response's fork mismatches the proposal's. Unsupported
+    // forks are rejected by the variant match below.
+    let expected_fork = proposal_info.fork;
     if response.version != expected_fork {
         return Err(PbsError::Validation(ValidationError::ForkMismatch {
             expected: expected_fork,
@@ -265,6 +274,7 @@ async fn send_submit_block_full(
         timeout_ms,
         &proposal_info.headers,
         &proposal_info.signed_blinded_block,
+        proposal_info.fork,
         retry,
         api_version,
     )
@@ -320,12 +330,14 @@ fn decode_by_encoding<T>(
 /// Sends the actual HTTP request to the relay's submit_block endpoint,
 /// returning the response (if applicable), the round-trip time, and the
 /// encoding type used for the body (if any). Used by send_submit_block.
+#[allow(clippy::too_many_arguments)]
 async fn send_submit_block_impl(
     relay: &RelayClient,
     url: Arc<Url>,
     timeout_ms: u64,
     headers: &HeaderMap,
     signed_blinded_block: &SignedBlindedBeaconBlock,
+    fork: ForkName,
     retry: u32,
     api_version: BuilderApiVersion,
 ) -> Result<SubmitBlockResponseInfo, PbsError> {
@@ -339,7 +351,7 @@ async fn send_submit_block_impl(
         .headers(headers.clone())
         .body(signed_blinded_block.as_ssz_bytes())
         .header(CONTENT_TYPE, EncodingType::Ssz.content_type_header())
-        .header(CONSENSUS_VERSION_HEADER, signed_blinded_block.fork_name_unchecked().to_string())
+        .header(CONSENSUS_VERSION_HEADER, fork.to_string())
         .send()
         .await
     {
@@ -380,6 +392,9 @@ async fn send_submit_block_impl(
             .headers(headers.clone())
             .body(json_body)
             .header(CONTENT_TYPE, EncodingType::Json.content_type_header())
+            // The SSZ attempt labels the fork; without it here the retry is the
+            // ambiguous one, since Electra and Fulu are identical in JSON.
+            .header(CONSENSUS_VERSION_HEADER, fork.to_string())
             .send()
             .await
         {