From 00ce1b1ed38883fd22cc98dcb83600b4ab8809a9 Mon Sep 17 00:00:00 2001 From: nicolas <48695862+merklefruit@users.noreply.github.com> Date: Mon, 22 Jul 2024 09:45:20 +0200 Subject: [PATCH] feat: add payload integrity tests --- bolt-sidecar/src/api/builder.rs | 189 ++++++++++++++++---- bolt-sidecar/src/api/spec.rs | 8 +- bolt-sidecar/src/builder/mod.rs | 3 - bolt-sidecar/src/builder/payload_builder.rs | 17 +- bolt-sidecar/src/primitives/mod.rs | 8 + 5 files changed, 169 insertions(+), 56 deletions(-) diff --git a/bolt-sidecar/src/api/builder.rs b/bolt-sidecar/src/api/builder.rs index 98cdf9520..2e6fdf215 100644 --- a/bolt-sidecar/src/api/builder.rs +++ b/bolt-sidecar/src/api/builder.rs @@ -17,6 +17,7 @@ use parking_lot::Mutex; use reqwest::Url; use serde::Deserialize; use std::{sync::Arc, time::Duration}; +use thiserror::Error; use tokio::net::TcpListener; use super::spec::{ @@ -140,15 +141,12 @@ where // On ANY error, we fall back to locally built block tracing::warn!(slot, elapsed = ?start.elapsed(), err = ?err, "Proxy error, fetching local payload instead"); - let payload_and_bid = match server.payload_fetcher.fetch_payload(slot).await { - Some(payload_and_bid) => payload_and_bid, - None => { - // TODO: handle failure? In this case, we don't have a fallback block - // which means we haven't made any commitments. This means the EL should - // fallback to local block building. - tracing::error!("No local payload produced for slot {slot}"); - return Err(BuilderApiError::FailedToFetchLocalPayload(slot)); - } + let Some(payload_and_bid) = server.payload_fetcher.fetch_payload(slot).await else { + // TODO: handle failure? In this case, we don't have a fallback block + // which means we haven't made any commitments. This means the EL should + // fallback to local block building. + tracing::debug!("No local payload with commitments produced for slot {slot}"); + return Err(BuilderApiError::FailedToFetchLocalPayload(slot)); }; let hash = payload_and_bid.bid.message.header.block_hash.clone(); @@ -195,30 +193,11 @@ where // If we have a locally built payload, it means we signed a local header. // Return it and clear the cache. - if let Some(payload) = server.local_payload.lock().take() { - let requested_block = &signed_blinded_block - .message - .body - .execution_payload_header - .block_hash; - - // WARNING: this is an important check. If the local block does not match what the - // beacon node has signed, we are at risk of equivocation and slashing. - if payload.block_hash() != requested_block { - tracing::error!( - expected = %requested_block.to_string(), - have = %payload.block_hash().to_string(), - "Local block hash does not match requested block hash" - ); - - return Err(BuilderApiError::InvalidLocalPayloadBlockHash { - expected: requested_block.to_string(), - have: payload.block_hash().to_string(), - }); - }; - - tracing::debug!("Local block found, returning: {payload:?}"); - return Ok(Json(payload)); + if let Some(local_payload) = server.local_payload.lock().take() { + check_locally_built_payload_integrity(&signed_blinded_block, &local_payload)?; + + tracing::debug!("Valid local block found, returning: {local_payload:?}"); + return Ok(Json(local_payload)); } // TODO: how do we deal with failures here? What if we submit the signed blinded block but don't get a response? @@ -285,3 +264,147 @@ where async fn index() -> Html<&'static str> { Html("Hello") } + +#[derive(Error, Debug, Clone)] +pub enum LocalPayloadIntegrityError { + #[error( + "Locally built payload does not match signed header. + {field_name} mismatch: expected {expected}, have {have}" + )] + FieldMismatch { + field_name: String, + expected: String, + have: String, + }, +} + +/// Helper macro to compare fields of the signed header and the local block. +macro_rules! assert_payload_fields_eq { + ($expected:expr, $have:expr, $field_name:ident) => { + if $expected != $have { + tracing::error!( + field_name = stringify!($field_name), + expected = %$expected, + have = %$have, + "Local block does not match signed header" + ); + return Err(LocalPayloadIntegrityError::FieldMismatch { + field_name: stringify!($field_name).to_string(), + expected: $expected.to_string(), + have: $have.to_string(), + }); + } + }; +} + +/// Perform some integrity checks on the locally built payload. +/// This is to ensure that the beacon node will accept the header that was signed +/// when we submit the full payload. +#[inline] +fn check_locally_built_payload_integrity( + signed_blinded_block: &SignedBlindedBeaconBlock, + local_payload: &GetPayloadResponse, +) -> Result<(), LocalPayloadIntegrityError> { + let header_signed_by_cl = &signed_blinded_block.message.body.execution_payload_header; + let local_execution_payload = local_payload.execution_payload(); + + assert_payload_fields_eq!( + &header_signed_by_cl.block_hash, + local_execution_payload.block_hash(), + BlockHash + ); + + assert_payload_fields_eq!( + header_signed_by_cl.block_number, + local_execution_payload.block_number(), + BlockNumber + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.state_root, + local_execution_payload.state_root(), + StateRoot + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.receipts_root, + local_execution_payload.receipts_root(), + ReceiptsRoot + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.prev_randao, + local_execution_payload.prev_randao(), + PrevRandao + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.gas_limit, + &local_execution_payload.gas_limit(), + GasLimit + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.gas_used, + &local_execution_payload.gas_used(), + GasUsed + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.timestamp, + &local_execution_payload.timestamp(), + Timestamp + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.extra_data, + local_execution_payload.extra_data(), + ExtraData + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.base_fee_per_gas, + local_execution_payload.base_fee_per_gas(), + BaseFeePerGas + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.parent_hash, + local_execution_payload.parent_hash(), + ParentHash + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.fee_recipient, + local_execution_payload.fee_recipient(), + FeeRecipient + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.logs_bloom, + local_execution_payload.logs_bloom(), + LogsBloom + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.blob_gas_used, + &local_execution_payload.blob_gas_used().unwrap_or_default(), + BlobGasUsed + ); + + assert_payload_fields_eq!( + &header_signed_by_cl.excess_blob_gas, + &local_execution_payload + .excess_blob_gas() + .unwrap_or_default(), + ExcessBlobGas + ); + + // TODO: Sanity check: recalculate transactions and withdrawals roots + // and assert them against the header + + // TODO: Sanity check: verify the validator signature + // signed_blinded_block.verify_signature()?; + + Ok(()) +} diff --git a/bolt-sidecar/src/api/spec.rs b/bolt-sidecar/src/api/spec.rs index 50d3b6201..8c36356c4 100644 --- a/bolt-sidecar/src/api/spec.rs +++ b/bolt-sidecar/src/api/spec.rs @@ -64,8 +64,8 @@ pub enum BuilderApiError { Timeout(#[from] tokio::time::error::Elapsed), #[error("Invalid fork: {0}")] InvalidFork(String), - #[error("Invalid local payload block hash. expected: {expected}, got: {have}")] - InvalidLocalPayloadBlockHash { expected: String, have: String }, + #[error("Locally-built payload does not match expected signed header")] + LocalPayloadIntegrity(#[from] super::builder::LocalPayloadIntegrityError), #[error("Generic error: {0}")] Generic(String), } @@ -109,8 +109,8 @@ impl IntoResponse for BuilderApiError { BuilderApiError::InvalidFork(err) => { (StatusCode::BAD_REQUEST, Json(err)).into_response() } - BuilderApiError::InvalidLocalPayloadBlockHash { .. } => { - (StatusCode::BAD_REQUEST, self.to_string()).into_response() + BuilderApiError::LocalPayloadIntegrity(err) => { + (StatusCode::BAD_REQUEST, err.to_string()).into_response() } BuilderApiError::Generic(err) => { (StatusCode::INTERNAL_SERVER_ERROR, Json(err)).into_response() diff --git a/bolt-sidecar/src/builder/mod.rs b/bolt-sidecar/src/builder/mod.rs index 3ea8ff42f..274d5aad6 100644 --- a/bolt-sidecar/src/builder/mod.rs +++ b/bolt-sidecar/src/builder/mod.rs @@ -95,7 +95,6 @@ impl LocalBuilder { /// Build a new payload with the given transactions. This method will /// cache the payload in the local builder instance, and make it available - /// pub async fn build_new_local_payload( &mut self, template: &BlockTemplate, @@ -160,8 +159,6 @@ impl LocalBuilder { /// transform a sealed header into a signed builder bid using /// the local builder's BLS key. - /// - /// TODO: add blobs bundle fn create_signed_builder_bid( &self, value: U256, diff --git a/bolt-sidecar/src/builder/payload_builder.rs b/bolt-sidecar/src/builder/payload_builder.rs index 2c56cfed3..a5f37d5c5 100644 --- a/bolt-sidecar/src/builder/payload_builder.rs +++ b/bolt-sidecar/src/builder/payload_builder.rs @@ -196,21 +196,6 @@ impl FallbackPayloadBuilder { withdrawals: Some(Withdrawals::new(withdrawals)), }; - // If there are some blob transactions, send them to the mempool - let blob_txs = transactions.iter().filter(|tx| tx.as_eip4844().is_some()); - for tx in blob_txs { - tracing::debug!(?tx.hash, "Sending blob tx to mempool"); - let mut bytes: Vec = Vec::new(); - tx.encode_enveloped(&mut bytes); - if let Err(e) = self - .execution_rpc_client - .send_raw_transaction(bytes.into()) - .await - { - tracing::error!(error = ?e, ?tx.hash, "Failed to send blob tx to mempool"); - } - } - let mut hints = Hints::default(); let max_iterations = 20; let mut i = 0; @@ -218,7 +203,7 @@ impl FallbackPayloadBuilder { let header = build_header_with_hints_and_context(&latest_block, &hints, &ctx); let sealed_header = header.seal_slow(); - let sealed_block = SealedBlock::new(sealed_header.clone(), body.clone()); + let sealed_block = SealedBlock::new(sealed_header, body.clone()); let block_hash = hints.block_hash.unwrap_or(sealed_block.hash()); diff --git a/bolt-sidecar/src/primitives/mod.rs b/bolt-sidecar/src/primitives/mod.rs index 920856011..3dcc76daa 100644 --- a/bolt-sidecar/src/primitives/mod.rs +++ b/bolt-sidecar/src/primitives/mod.rs @@ -195,6 +195,14 @@ impl GetPayloadResponse { GetPayloadResponse::Deneb(payload) => payload.execution_payload.block_hash(), } } + + pub fn execution_payload(&self) -> &ExecutionPayload { + match self { + GetPayloadResponse::Capella(payload) => payload, + GetPayloadResponse::Bellatrix(payload) => payload, + GetPayloadResponse::Deneb(payload) => &payload.execution_payload, + } + } } /// A struct representing the current chain head.