Add use_l2_client_for_state_root and allow_traffic_to_unhealthy_builder #381
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
cody-wang-cb
force-pushed
the
cody/add_base_fork_commits
branch
from
b928c57 to
b46ee59
Compare
|
|
||
| /// Allow all engine API calls to builder even when marked as unhealthy | ||
| /// This is default true assuming no builder CL set up | ||
| #[arg(long, env, default_value = "true")] |
There was a problem hiding this comment.
| #[arg(long, env, default_value = "true")] | |
| #[arg(long, env, default_value = "false")] |
default should meant to be false?
There was a problem hiding this comment.
the default is true currently, it's always going to send traffic to the builder regardless of the builder health
There was a problem hiding this comment.
maybe something like
ignore_unhealthy_builders?
| // async call to builder to sync the builder node | ||
| if !self.execution_mode().is_disabled() { | ||
| if !self.execution_mode().is_disabled() | ||
| && (self.allow_traffic_to_unhealthy_builder || builder_healthy) |
There was a problem hiding this comment.
this call should be forwarded to the builder regardless of whether the builder is healthy? its related to execution mode, not builder health
There was a problem hiding this comment.
it's blocking the case when the builder is unhealthy and it doesn't want to forward
builder_healthy=false and self.allow_traffic_to_unhealthy_builder=false
so the code is the inverse of that, but maybe it's not as clear
There was a problem hiding this comment.
suggest to rework this part a little bit, double negative, and overall quite confusing
Should also have good comment
| @@ -93,6 +93,15 @@ pub struct RollupBoostArgs { | |||
| #[arg(long, env)] | |||
| pub block_selection_policy: Option<BlockSelectionPolicy>, | |||
|
|
|||
| /// Should we use the l2 client for computing state root | |||
| #[arg(long, env, default_value = "false")] | |||
| pub use_l2_client_for_state_root: bool, | |||
| pub fn transactions(&self) -> Vec<Bytes> { | ||
| match self { | ||
| OpExecutionPayloadEnvelope::V3(payload) => payload | ||
| .execution_payload |
| // Get payload and validate with the local l2 client | ||
| tracing::Span::current().record("builder_has_payload", true); | ||
| info!(message = "builder has payload, calling get_payload on builder"); | ||
|
|
||
| let payload = self.builder_client.get_payload(payload_id, version).await?; | ||
| let _ = self | ||
|
|
||
| if self.use_l2_client_for_state_root == false { |
There was a problem hiding this comment.
| if self.use_l2_client_for_state_root == false { | |
| if !self.use_l2_client_for_state_root { |
There was a problem hiding this comment.
Why are we sending new payload only in one case?
I think we could just always send it, even if l2 was used for state root, or will it break anything?
|
|
||
| // If execution mode is disabled, return the l2 client response immediately | ||
| if self.execution_mode().is_disabled() { | ||
| return Ok(l2_fut.await?); | ||
| } | ||
|
|
||
| // If traffic to the unhealthy builder is not allowed and the builder is unhealthy, | ||
| if !self.allow_traffic_to_unhealthy_builder && !builder_healthy { |
There was a problem hiding this comment.
would be happy to see a refactoring, right now there is a lot of conditions, maybe we need to bundle them somehow
Add use_l2_client_for_state_root, such that if it's
false, it will use op-geth for state root calculationAdd allow_traffic_to_unhealthy_builder flag such that if it's
false, it won't send builder FCUs when it's unheathy, so EL sync won't be triggered