Merge pull request #117 from chainbound/fix/sidecar/constraints-deadline

Submit constraints only after commitments deadline + block template refactor

bolt-sidecar/bin/sidecar.rs

@@ -28,7 +28,8 @@ async fn main() -> eyre::Result<()> {
     let signer = Signer::new(config.private_key.clone().unwrap());
 
     let state_client = StateClient::new(config.execution_api_url.clone());
-    let mut execution_state = ExecutionState::new(state_client).await?;
+    let mut execution_state =
+        ExecutionState::new(state_client, config.limits.max_commitments_per_slot).await?;
 
     let mevboost_client = MevBoostClient::new(config.mevboost_url.clone());
     let beacon_client = BeaconClient::new(config.beacon_api_url.clone());
@@ -66,54 +67,40 @@ async fn main() -> eyre::Result<()> {
     loop {
         tokio::select! {
             Some(ApiEvent { request, response_tx }) = api_events_rx.recv() => {
-                tracing::info!("Received commitment request: {:?}", request);
+                let start = std::time::Instant::now();
 
                 let validator_index = match consensus_state.validate_request(&request) {
                     Ok(index) => index,
                     Err(e) => {
-                        tracing::error!("Failed to validate request: {:?}", e);
+                        tracing::error!(err = ?e, "Failed to validate request");
                         let _ = response_tx.send(Err(ApiError::Custom(e.to_string())));
                         continue;
                     }
                 };
 
-                if let Err(e) = execution_state
-                    .try_commit(&request)
-                    .await
-                {
-                    tracing::error!("Failed to commit request: {:?}", e);
+                if let Err(e) = execution_state.validate_commitment_request(&request).await {
+                    tracing::error!(err = ?e, "Failed to commit request");
                     let _ = response_tx.send(Err(ApiError::Custom(e.to_string())));
                     continue;
-                }
+                };
 
                 // TODO: match when we have more request types
                 let CommitmentRequest::Inclusion(request) = request;
-
                 tracing::info!(
+                    elapsed = ?start.elapsed(),
                     tx_hash = %request.tx.hash(),
                     "Validation against execution state passed"
                 );
 
-                // parse the request into constraints and sign them with the sidecar signer
-                let message = ConstraintsMessage::build(validator_index, request.slot, request);
+                // TODO: review all this `clone` usage
 
+                // parse the request into constraints and sign them with the sidecar signer
+                let slot = request.slot;
+                let message = ConstraintsMessage::build(validator_index, request);
                 let signature = signer.sign(&message.digest())?.to_string();
-                let signed_constraints = vec![SignedConstraints { message, signature }];
+                let signed_constraints = SignedConstraints { message, signature };
 
-                // TODO: fix retry logic
-                let max_retries = 5;
-                let mut i = 0;
-                'inner: while let Err(e) = mevboost_client
-                    .submit_constraints(&signed_constraints)
-                    .await
-                {
-                    tracing::error!(err = ?e, "Error submitting constraints, retrying...");
-                    tokio::time::sleep(Duration::from_millis(100)).await;
-                    i+=1;
-                    if i >= max_retries {
-                        break 'inner
-                    }
-                }
+                execution_state.add_constraint(slot, signed_constraints.clone());
 
                 let res = serde_json::to_value(signed_constraints).map_err(Into::into);
                 let _ = response_tx.send(res).ok();
@@ -133,12 +120,30 @@ async fn main() -> eyre::Result<()> {
             Some(slot) = consensus_state.commitment_deadline.wait() => {
                 tracing::info!(slot, "Commitment deadline reached, starting to build local block");
 
-                let Some(template) = execution_state.get_block_template(slot) else {
+                let Some(template) = execution_state.remove_block_template(slot) else {
                     tracing::warn!("No block template found for slot {slot} when requested");
                     continue;
                 };
 
-                if let Err(e) = local_builder.build_new_local_payload(template.transactions).await {
+                tracing::trace!(?template.signed_constraints_list, "Submitting constraints to MEV-Boost");
+
+                // TODO: fix retry logic, and move this to separate task
+                let max_retries = 5;
+                let mut i = 0;
+                'inner: while let Err(e) = mevboost_client
+                    .submit_constraints(&template.signed_constraints_list)
+                .await
+                {
+                    tracing::error!(err = ?e, "Error submitting constraints, retrying...");
+                    tokio::time::sleep(Duration::from_millis(100)).await;
+                    i+=1;
+                    if i >= max_retries {
+                        tracing::error!("Max retries reached while submitting to MEV-Boost");
+                        break 'inner
+                    }
+                }
+
+                if let Err(e) = local_builder.build_new_local_payload(&template).await {
                     tracing::error!(err = ?e, "CRITICAL: Error while building local payload at slot deadline for {slot}");
                 };
             },

bolt-sidecar/src/api/builder.rs

@@ -20,8 +20,8 @@ use std::{sync::Arc, time::Duration};
 use tokio::net::TcpListener;
 
 use super::spec::{
-    BuilderApi, BuilderApiError, ConstraintsApi, GET_HEADER_PATH, GET_PAYLOAD_PATH,
-    REGISTER_VALIDATORS_PATH, STATUS_PATH,
+    BuilderApiError, ConstraintsApi, GET_HEADER_PATH, GET_PAYLOAD_PATH, REGISTER_VALIDATORS_PATH,
+    STATUS_PATH,
 };
 use crate::{
     client::mevboost::MevBoostClient,
@@ -35,11 +35,9 @@ const GET_HEADER_WITH_PROOFS_TIMEOUT: Duration = Duration::from_millis(500);
 
 /// A proxy server for the builder API.
 /// Forwards all requests to the target after interception.
-pub struct BuilderProxyServer<T: BuilderApi, P> {
+pub struct BuilderProxyServer<T, P> {
     proxy_target: T,
-    // TODO: fill with local payload when we fetch a payload
-    // in failed get_header
-    // This will only be some in case of a failed get_header
+    /// INVARIANT: This will be `Some` IFF we have signed a local header for the latest slot.
     local_payload: Mutex<Option<GetPayloadResponse>>,
     /// The payload fetcher to get locally built payloads.
     payload_fetcher: P,
@@ -127,7 +125,12 @@ where
             Ok(res) => match res {
                 Err(builder_err) => builder_err,
                 Ok(header) => {
-                    tracing::debug!(elapsed = ?start.elapsed(), "Returning signed builder bid: {:?}", header);
+                    // Clear the local payload cache if we have a successful response
+                    // By definition of `server.local_payload`, this will be `Some` IFF we have signed a local header
+                    let mut local_payload = server.local_payload.lock();
+                    *local_payload = None;
+
+                    tracing::debug!(elapsed = ?start.elapsed(), "Returning signed builder bid");
                     return Ok(Json(header));
                 }
             },
@@ -137,33 +140,31 @@ 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 = match server.payload_fetcher.fetch_payload(slot).await {
-            Some(payload) => {
-                tracing::info!(elapsed = ?start.elapsed(), "Fetched local payload for slot {slot}");
-                payload
-            }
+        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 beacon client should
+                // 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 hash = payload.bid.message.header.block_hash.clone();
-        let number = payload.bid.message.header.block_number;
+        let hash = payload_and_bid.bid.message.header.block_hash.clone();
+        let number = payload_and_bid.bid.message.header.block_number;
+        tracing::info!(elapsed = ?start.elapsed(), %hash, "Fetched local payload for slot {slot}");
 
         {
-            // Set the payload for the following get_payload request
+            // Since we've signed a local header, set the payload for
+            // the following `get_payload` request.
             let mut local_payload = server.local_payload.lock();
-            *local_payload = Some(payload.payload);
+            *local_payload = Some(payload_and_bid.payload);
         }
 
         let versioned_bid = VersionedValue::<SignedBuilderBid> {
             version: Fork::Deneb,
-            data: payload.bid,
-            // TODO: a more elegant way to do this? Can we avoid this meta field?
+            data: payload_and_bid.bid,
             meta: Default::default(),
         };
 
@@ -192,7 +193,8 @@ where
                 e
             })?;
 
-        // If we have a locally built payload, return it and clear the cache.
+        // 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
@@ -204,8 +206,8 @@ where
             // 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(),
+                    expected = %requested_block.to_string(),
+                    have = %payload.block_hash().to_string(),
                     "Local block hash does not match requested block hash"
                 );
 
@@ -215,7 +217,7 @@ where
                 });
             };
 
-            tracing::info!("Local block found, returning: {payload:?}");
+            tracing::debug!("Local block found, returning: {payload:?}");
             return Ok(Json(payload));
         }
 

bolt-sidecar/src/api/spec.rs

@@ -66,6 +66,8 @@ pub enum BuilderApiError {
     InvalidFork(String),
     #[error("Invalid local payload block hash. expected: {expected}, got: {have}")]
     InvalidLocalPayloadBlockHash { expected: String, have: String },
+    #[error("Generic error: {0}")]
+    Generic(String),
 }
 
 impl IntoResponse for BuilderApiError {
@@ -110,6 +112,9 @@ impl IntoResponse for BuilderApiError {
             BuilderApiError::InvalidLocalPayloadBlockHash { .. } => {
                 (StatusCode::BAD_REQUEST, self.to_string()).into_response()
             }
+            BuilderApiError::Generic(err) => {
+                (StatusCode::INTERNAL_SERVER_ERROR, Json(err)).into_response()
+            }
         }
     }
 }

bolt-sidecar/src/builder/mod.rs

@@ -6,7 +6,6 @@ use ethereum_consensus::{
     ssz::prelude::{List, MerkleizationError},
 };
 use payload_builder::FallbackPayloadBuilder;
-use reth_primitives::TransactionSigned;
 use signature::sign_builder_message;
 
 use crate::{
@@ -99,8 +98,11 @@ impl LocalBuilder {
     ///
     pub async fn build_new_local_payload(
         &mut self,
-        transactions: Vec<TransactionSigned>,
+        template: &BlockTemplate,
     ) -> Result<(), BuilderError> {
+        let transactions = template.as_signed_transactions();
+        let blobs_bundle = template.as_blobs_bundle();
+
         // 1. build a fallback payload with the given transactions, on top of
         // the current head of the chain
         let sealed_block = self
@@ -119,8 +121,7 @@ impl LocalBuilder {
         let eth_payload = compat::to_consensus_execution_payload(&sealed_block);
         let payload_and_blobs = PayloadAndBlobs {
             execution_payload: eth_payload,
-            // TODO: add included blobs here
-            blobs_bundle: Default::default(),
+            blobs_bundle,
         };
 
         // 2. create a signed builder bid with the sealed block header we just created

bolt-sidecar/src/builder/signature.rs

@@ -9,7 +9,7 @@ use tree_hash_derive::TreeHash;
 
 use crate::ChainConfig;
 
-/// Sign a SSZ object a BLS secret key, using the Application Builder domain
+/// Sign a SSZ object with a BLS secret key, using the Application Builder domain
 /// for signing arbitrary builder-api messages in the out-of-protocol specifications.
 ///
 /// Fun Note: we use a `blst` secret key to sign a message, and produce an `alloy` signature,

bolt-sidecar/src/builder/state_root.rs

@@ -10,17 +10,24 @@ mod tests {
     use partial_mpt::StateTrie;
     use reqwest::Url;
 
-    use crate::{builder::CallTraceManager, client::rpc::RpcClient};
+    use crate::{
+        builder::CallTraceManager, client::rpc::RpcClient, test_util::try_get_execution_api_url,
+    };
 
+    #[ignore]
     #[tokio::test]
     async fn test_trace_call() -> eyre::Result<()> {
         dotenvy::dotenv().ok();
-        tracing_subscriber::fmt::init();
+        let _ = tracing_subscriber::fmt::try_init();
+
+        let Some(rpc_url) = try_get_execution_api_url().await else {
+            tracing::warn!("EL_RPC not reachable, skipping test");
+            return Ok(());
+        };
 
         tracing::info!("Starting test_trace_call");
 
-        let rpc_url = std::env::var("RPC_URL").expect("RPC_URL must be set");
-        let rpc_url = Url::parse(&rpc_url).unwrap();
+        let rpc_url = Url::parse(rpc_url).unwrap();
         let client = RpcClient::new(rpc_url.clone());
 
         let (call_trace_manager, call_trace_handler) = CallTraceManager::new(rpc_url);