From cc9540df95608d919833d1577bbb478dfb7f88a9 Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Wed, 30 Apr 2025 15:37:13 +0530 Subject: [PATCH 1/7] feat: simplify ExecutionMode --- src/debug_api.rs | 12 ------------ src/server.rs | 27 ++++----------------------- 2 files changed, 4 insertions(+), 35 deletions(-) diff --git a/src/debug_api.rs b/src/debug_api.rs index ed7c646f..aaed3887 100644 --- a/src/debug_api.rs +++ b/src/debug_api.rs @@ -152,17 +152,5 @@ mod tests { // Verify again with get_execution_mode let status = client.get_execution_mode().await.unwrap(); assert_eq!(status.execution_mode, ExecutionMode::Enabled); - - // Test setting fallback execution mode - let result = client - .set_execution_mode(ExecutionMode::Fallback) - .await - .unwrap(); - - assert_eq!(result.execution_mode, ExecutionMode::Fallback); - - // Verify again with get_execution_mode - let status = client.get_execution_mode().await.unwrap(); - assert_eq!(status.execution_mode, ExecutionMode::Fallback); } } diff --git a/src/server.rs b/src/server.rs index 82550072..846ca823 100644 --- a/src/server.rs +++ b/src/server.rs @@ -140,23 +140,16 @@ pub enum ExecutionMode { DryRun, // Not sending any requests Disabled, - // Defaulting to op-geth payloads - Fallback, } impl ExecutionMode { - fn is_get_payload_enabled(&self) -> bool { - // get payload is only enabled in 'enabled' mode - matches!(self, ExecutionMode::Enabled) + fn is_dry_run(&self) -> bool { + matches!(self, ExecutionMode::DryRun) } fn is_disabled(&self) -> bool { matches!(self, ExecutionMode::Disabled) } - - fn is_fallback_enabled(&self) -> bool { - matches!(self, ExecutionMode::Fallback) - } } pub struct RollupBoostServer { @@ -612,18 +605,6 @@ impl RollupBoostServer { ) -> RpcResult { let l2_client_future = self.l2_client.get_payload(payload_id, version); let builder_client_future = Box::pin(async move { - let execution_mode = self.execution_mode(); - if !execution_mode.is_get_payload_enabled() { - info!(message = "dry run mode is enabled, skipping get payload builder call"); - - // We are in dry run mode, so we do not want to call the builder. - return Err(ErrorObject::owned( - INVALID_REQUEST_CODE, - "Dry run mode is enabled", - None::, - )); - } - if let Some(cause) = self.payload_trace_context.trace_id(&payload_id) { tracing::Span::current().follows_from(cause); } @@ -631,7 +612,7 @@ impl RollupBoostServer { if !self.payload_trace_context.has_builder_payload(&payload_id) { // block builder won't build a block without attributes info!(message = "builder has no payload, skipping get_payload call to builder"); - return Ok(None); + return RpcResult::Ok(None); } let builder = self.builder_client.clone(); @@ -653,7 +634,7 @@ impl RollupBoostServer { (Ok(Some(builder)), Ok(l2_payload)) => { // builder successfully returned a payload self.probes.set_health(Health::Healthy); - if self.execution_mode().is_fallback_enabled() { + if self.execution_mode().is_dry_run() { // Default to op-geth's payload Ok((l2_payload, PayloadSource::L2)) } else { From f97b52465dcc887a794ebe9e742fa07a3e98a08c Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Thu, 1 May 2025 14:17:49 +0530 Subject: [PATCH 2/7] fix: dry run optimization --- src/server.rs | 78 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 58 insertions(+), 20 deletions(-) diff --git a/src/server.rs b/src/server.rs index 846ca823..3a9bd72c 100644 --- a/src/server.rs +++ b/src/server.rs @@ -629,30 +629,68 @@ impl RollupBoostServer { Ok(Some(payload)) }); - let (l2_payload, builder_payload) = tokio::join!(l2_client_future, builder_client_future); - let (payload, context) = match (builder_payload, l2_payload) { - (Ok(Some(builder)), Ok(l2_payload)) => { - // builder successfully returned a payload - self.probes.set_health(Health::Healthy); - if self.execution_mode().is_dry_run() { - // Default to op-geth's payload - Ok((l2_payload, PayloadSource::L2)) - } else { + let execution_mode = self.execution_mode(); + + // if mode is `dry_run` dont want to wait for the builder payload + let (payload, context) = if execution_mode.is_dry_run() { + let result = match l2_client_future.await { + Ok(payload) => { + self.probes.set_health(Health::Healthy); + Ok((payload, PayloadSource::L2)) + } + Err(e) => { + self.probes.set_health(Health::ServiceUnavailable); + Err(e) + } + }; + result + } else { + let (l2_payload, builder_payload) = + tokio::join!(l2_client_future, builder_client_future); + let result = match (builder_payload, l2_payload) { + (Ok(Some(builder)), Ok(_)) => { + // builder successfully returned a payload + self.probes.set_health(Health::Healthy); Ok((builder, PayloadSource::Builder)) } - } - (_, Ok(l2)) => { - // builder failed to return a payload - self.probes.set_health(Health::PartialContent); - Ok((l2, PayloadSource::L2)) - } - (_, Err(e)) => { - // builder and l2 failed to return a payload - self.probes.set_health(Health::ServiceUnavailable); - Err(e) - } + (_, Ok(l2)) => { + // builder failed to return a payload + self.probes.set_health(Health::PartialContent); + Ok((l2, PayloadSource::L2)) + } + (_, Err(e)) => { + // builder and l2 failed to return a payload + self.probes.set_health(Health::ServiceUnavailable); + Err(e) + } + }; + result }?; + // let (l2_payload, builder_payload) = tokio::join!(l2_client_future, builder_client_future); + // let (payload, context) = match (builder_payload, l2_payload) { + // (Ok(Some(builder)), Ok(l2_payload)) => { + // // builder successfully returned a payload + // self.probes.set_health(Health::Healthy); + // if self.execution_mode().is_dry_run() { + // // Default to op-geth's payload + // Ok((l2_payload, PayloadSource::L2)) + // } else { + // Ok((builder, PayloadSource::Builder)) + // } + // } + // (_, Ok(l2)) => { + // // builder failed to return a payload + // self.probes.set_health(Health::PartialContent); + // Ok((l2, PayloadSource::L2)) + // } + // (_, Err(e)) => { + // // builder and l2 failed to return a payload + // self.probes.set_health(Health::ServiceUnavailable); + // Err(e) + // } + // }?; + tracing::Span::current().record("payload_source", context.to_string()); // To maintain backwards compatibility with old metrics, we need to record blocks built // This is temporary until we migrate to the new metrics From a5ff2d7aca11489e7f7c8763f983ec1d312c1e98 Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Fri, 2 May 2025 13:43:56 +0530 Subject: [PATCH 3/7] fix: logic for disabled mode --- src/server.rs | 105 ++++++++++++++++++++++---------------------------- 1 file changed, 46 insertions(+), 59 deletions(-) diff --git a/src/server.rs b/src/server.rs index 3a9bd72c..c591697c 100644 --- a/src/server.rs +++ b/src/server.rs @@ -603,36 +603,11 @@ impl RollupBoostServer { payload_id: PayloadId, version: Version, ) -> RpcResult { - let l2_client_future = self.l2_client.get_payload(payload_id, version); - let builder_client_future = Box::pin(async move { - if let Some(cause) = self.payload_trace_context.trace_id(&payload_id) { - tracing::Span::current().follows_from(cause); - } - - if !self.payload_trace_context.has_builder_payload(&payload_id) { - // block builder won't build a block without attributes - info!(message = "builder has no payload, skipping get_payload call to builder"); - return RpcResult::Ok(None); - } - - let builder = self.builder_client.clone(); - let payload = builder.get_payload(payload_id, version).await?; - - // Send the payload to the local execution engine with engine_newPayload to validate the block from the builder. - // Otherwise, we do not want to risk the network to a halt since op-node will not be able to propose the block. - // If validation fails, return the local block since that one has already been validated. - let _ = self - .l2_client - .new_payload(NewPayload::from(payload.clone())) - .await?; - - Ok(Some(payload)) - }); - let execution_mode = self.execution_mode(); + let l2_client_future = self.l2_client.get_payload(payload_id, version); - // if mode is `dry_run` dont want to wait for the builder payload - let (payload, context) = if execution_mode.is_dry_run() { + // if mode is `disabled` dont await the builder payload + let (payload, context) = if execution_mode.is_disabled() { let result = match l2_client_future.await { Ok(payload) => { self.probes.set_health(Health::Healthy); @@ -645,21 +620,57 @@ impl RollupBoostServer { }; result } else { + // if mode is `enabled` or `dry_run` await the builder + L2 payload + let builder_client_future = Box::pin(async move { + if let Some(cause) = self.payload_trace_context.trace_id(&payload_id) { + tracing::Span::current().follows_from(cause); + } + + if !self.payload_trace_context.has_builder_payload(&payload_id) { + // block builder won't build a block without attributes + info!(message = "builder has no payload, skipping get_payload call to builder"); + return RpcResult::Ok(None); + } + + let builder = self.builder_client.clone(); + let payload = builder.get_payload(payload_id, version).await?; + + // Send the payload to the local execution engine with engine_newPayload to validate the block from the builder. + // Otherwise, we do not want to risk the network to a halt since op-node will not be able to propose the block. + // If validation fails, return the local block since that one has already been validated. + let _ = self + .l2_client + .new_payload(NewPayload::from(payload.clone())) + .await?; + + Ok(Some(payload)) + }); + let (l2_payload, builder_payload) = tokio::join!(l2_client_future, builder_client_future); - let result = match (builder_payload, l2_payload) { - (Ok(Some(builder)), Ok(_)) => { - // builder successfully returned a payload + let result = match (builder_payload, l2_payload, execution_mode.is_dry_run()) { + (Ok(Some(_)), Ok(l2), true) => { + // if builder and l2 is okay and `is_dry_run`, then always return L2 payload with signal `Healthy` + self.probes.set_health(Health::Healthy); + Ok((l2, PayloadSource::L2)) + } + (_, Ok(l2), true) => { + // if builder has failed/errored and l2 is okay and `is_dry_run`, then always return L2 payload, but signal `PartialContent` + self.probes.set_health(Health::PartialContent); + Ok((l2, PayloadSource::L2)) + } + (Ok(Some(builder)), Ok(_), false) => { + // if builder and L2 is okay and `is_dry_run` is false, then always return builder payload i.e. `Enabled` Mode with signal `Healthy` self.probes.set_health(Health::Healthy); Ok((builder, PayloadSource::Builder)) } - (_, Ok(l2)) => { - // builder failed to return a payload + (_, Ok(l2), false) => { + // builder failed to return/errored and `l2_payload` is ok and `is_dry_run` is false, then always return L2 payload with signal `PartialContent` self.probes.set_health(Health::PartialContent); Ok((l2, PayloadSource::L2)) } - (_, Err(e)) => { - // builder and l2 failed to return a payload + (_, Err(e), _) => { + // l2 failed to return a payload self.probes.set_health(Health::ServiceUnavailable); Err(e) } @@ -667,30 +678,6 @@ impl RollupBoostServer { result }?; - // let (l2_payload, builder_payload) = tokio::join!(l2_client_future, builder_client_future); - // let (payload, context) = match (builder_payload, l2_payload) { - // (Ok(Some(builder)), Ok(l2_payload)) => { - // // builder successfully returned a payload - // self.probes.set_health(Health::Healthy); - // if self.execution_mode().is_dry_run() { - // // Default to op-geth's payload - // Ok((l2_payload, PayloadSource::L2)) - // } else { - // Ok((builder, PayloadSource::Builder)) - // } - // } - // (_, Ok(l2)) => { - // // builder failed to return a payload - // self.probes.set_health(Health::PartialContent); - // Ok((l2, PayloadSource::L2)) - // } - // (_, Err(e)) => { - // // builder and l2 failed to return a payload - // self.probes.set_health(Health::ServiceUnavailable); - // Err(e) - // } - // }?; - tracing::Span::current().record("payload_source", context.to_string()); // To maintain backwards compatibility with old metrics, we need to record blocks built // This is temporary until we migrate to the new metrics From f901d50913d56ef765ce0dce5c4867f333117c4f Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Tue, 6 May 2025 11:20:05 +0530 Subject: [PATCH 4/7] chore: clean up get payload --- src/server.rs | 118 +++++++++++++++++++++++--------------------------- 1 file changed, 55 insertions(+), 63 deletions(-) diff --git a/src/server.rs b/src/server.rs index c591697c..6d66b954 100644 --- a/src/server.rs +++ b/src/server.rs @@ -603,80 +603,72 @@ impl RollupBoostServer { payload_id: PayloadId, version: Version, ) -> RpcResult { - let execution_mode = self.execution_mode(); - let l2_client_future = self.l2_client.get_payload(payload_id, version); + let l2_fut = self.l2_client.get_payload(payload_id, version); - // if mode is `disabled` dont await the builder payload - let (payload, context) = if execution_mode.is_disabled() { - let result = match l2_client_future.await { + // If execution mode is disabled, return the l2 payload without sending + // the request to the builder + if self.execution_mode().is_disabled() { + return match l2_fut.await { Ok(payload) => { self.probes.set_health(Health::Healthy); - Ok((payload, PayloadSource::L2)) - } - Err(e) => { - self.probes.set_health(Health::ServiceUnavailable); - Err(e) + tracing::Span::current().record("payload_source", "L2"); + counter!("rpc.blocks_created", "source" => "L2").increment(1); + + let execution_payload = ExecutionPayload::from(payload.clone()); + info!( + message = "returning block", + "hash" = %execution_payload.block_hash(), + "number" = %execution_payload.block_number(), + context = "L2", + %payload_id, + ); + + Ok(payload) } + + Err(e) => Err(e.into()), }; - result - } else { - // if mode is `enabled` or `dry_run` await the builder + L2 payload - let builder_client_future = Box::pin(async move { - if let Some(cause) = self.payload_trace_context.trace_id(&payload_id) { - tracing::Span::current().follows_from(cause); - } + } - if !self.payload_trace_context.has_builder_payload(&payload_id) { - // block builder won't build a block without attributes - info!(message = "builder has no payload, skipping get_payload call to builder"); - return RpcResult::Ok(None); - } + // Forward the get payload request to the builder + let builder_fut = async { + if let Some(cause) = self.payload_trace_context.trace_id(&payload_id) { + tracing::Span::current().follows_from(cause); + } + if !self.payload_trace_context.has_builder_payload(&payload_id) { + info!(message = "builder has no payload, skipping get_payload call to builder"); + return RpcResult::Ok(None); + } - let builder = self.builder_client.clone(); - let payload = builder.get_payload(payload_id, version).await?; + // Get payload and validate with the local l2 client + let payload = self.builder_client.get_payload(payload_id, version).await?; + let _ = self + .l2_client + .new_payload(NewPayload::from(payload.clone())) + .await?; - // Send the payload to the local execution engine with engine_newPayload to validate the block from the builder. - // Otherwise, we do not want to risk the network to a halt since op-node will not be able to propose the block. - // If validation fails, return the local block since that one has already been validated. - let _ = self - .l2_client - .new_payload(NewPayload::from(payload.clone())) - .await?; + Ok(Some(payload)) + }; - Ok(Some(payload)) - }); + let (l2_payload, builder_payload) = tokio::join!(l2_fut, builder_fut); - let (l2_payload, builder_payload) = - tokio::join!(l2_client_future, builder_client_future); - let result = match (builder_payload, l2_payload, execution_mode.is_dry_run()) { - (Ok(Some(_)), Ok(l2), true) => { - // if builder and l2 is okay and `is_dry_run`, then always return L2 payload with signal `Healthy` - self.probes.set_health(Health::Healthy); - Ok((l2, PayloadSource::L2)) - } - (_, Ok(l2), true) => { - // if builder has failed/errored and l2 is okay and `is_dry_run`, then always return L2 payload, but signal `PartialContent` - self.probes.set_health(Health::PartialContent); - Ok((l2, PayloadSource::L2)) - } - (Ok(Some(builder)), Ok(_), false) => { - // if builder and L2 is okay and `is_dry_run` is false, then always return builder payload i.e. `Enabled` Mode with signal `Healthy` - self.probes.set_health(Health::Healthy); - Ok((builder, PayloadSource::Builder)) - } - (_, Ok(l2), false) => { - // builder failed to return/errored and `l2_payload` is ok and `is_dry_run` is false, then always return L2 payload with signal `PartialContent` - self.probes.set_health(Health::PartialContent); - Ok((l2, PayloadSource::L2)) - } - (_, Err(e), _) => { - // l2 failed to return a payload - self.probes.set_health(Health::ServiceUnavailable); - Err(e) + // Evaluate the builder and l2 response and select the final payload + let (payload, context) = { + let l2_payload = + l2_payload.inspect_err(|_| self.probes.set_health(Health::ServiceUnavailable))?; + self.probes.set_health(Health::Healthy); + + if let Ok(Some(payload)) = builder_payload { + if self.execution_mode().is_dry_run() { + (l2_payload, PayloadSource::L2) + } else { + (payload, PayloadSource::Builder) } - }; - result - }?; + } else { + self.probes.set_health(Health::PartialContent); + (l2_payload, PayloadSource::L2) + } + }; tracing::Span::current().record("payload_source", context.to_string()); // To maintain backwards compatibility with old metrics, we need to record blocks built From 570b0e6a85a0d3371fc5c59459e8f2352d922888 Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Tue, 6 May 2025 11:29:08 +0530 Subject: [PATCH 5/7] fix: use lowercase l2 in context --- src/server.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/server.rs b/src/server.rs index 6d66b954..a0c84f55 100644 --- a/src/server.rs +++ b/src/server.rs @@ -611,15 +611,17 @@ impl RollupBoostServer { return match l2_fut.await { Ok(payload) => { self.probes.set_health(Health::Healthy); - tracing::Span::current().record("payload_source", "L2"); - counter!("rpc.blocks_created", "source" => "L2").increment(1); + tracing::Span::current() + .record("payload_source", PayloadSource::L2.to_string()); + counter!("rpc.blocks_created", "source" => PayloadSource::L2.to_string()) + .increment(1); let execution_payload = ExecutionPayload::from(payload.clone()); info!( message = "returning block", "hash" = %execution_payload.block_hash(), "number" = %execution_payload.block_number(), - context = "L2", + context = PayloadSource::L2.to_string(), %payload_id, ); From 70ca4bec27363151475059f4d6f802cbaed7eeb9 Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Wed, 7 May 2025 02:07:13 +0530 Subject: [PATCH 6/7] fix: return unhealthy incase of err --- src/server.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/server.rs b/src/server.rs index a0c84f55..963e7826 100644 --- a/src/server.rs +++ b/src/server.rs @@ -628,7 +628,10 @@ impl RollupBoostServer { Ok(payload) } - Err(e) => Err(e.into()), + Err(e) => { + self.probes.set_health(Health::ServiceUnavailable); + Err(e.into()) + } }; } From 5f889754825b67228545c0647a5981dea22a2143 Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Wed, 7 May 2025 11:58:04 +0530 Subject: [PATCH 7/7] fix: content formatting --- src/server.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/server.rs b/src/server.rs index 963e7826..40f99490 100644 --- a/src/server.rs +++ b/src/server.rs @@ -611,17 +611,16 @@ impl RollupBoostServer { return match l2_fut.await { Ok(payload) => { self.probes.set_health(Health::Healthy); - tracing::Span::current() - .record("payload_source", PayloadSource::L2.to_string()); - counter!("rpc.blocks_created", "source" => PayloadSource::L2.to_string()) - .increment(1); + let context = PayloadSource::L2; + tracing::Span::current().record("payload_source", context.to_string()); + counter!("rpc.blocks_created", "source" => context.to_string()).increment(1); let execution_payload = ExecutionPayload::from(payload.clone()); info!( message = "returning block", "hash" = %execution_payload.block_hash(), "number" = %execution_payload.block_number(), - context = PayloadSource::L2.to_string(), + %context, %payload_id, );