fix: pre-establish builder connections before requesting bids - #9947
fix: pre-establish builder connections before requesting bids#9947nflaig wants to merge 7 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
f47f8a9 to
e1d6082
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58cfb4c899
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| * 1667 = 16.67% of slot (2s on mainnet). Must stay under the few seconds an idle connection survives, | ||
| * while leaving room for a round trip to a distant builder to complete before the slot begins. | ||
| */ | ||
| const BUILDER_STATUS_CHECK_BEFORE_SLOT_BPS = 1667; |
There was a problem hiding this comment.
this could also just me a _MS value as it's not really related to slot duration, but either way seems fine to me
Post-Gloas
prepareNextSlotreplaced the builder status check with the circuit breaker update, so the builders a proposal is about to request bids from are never contacted beforehand. Every bid request then pays a full TCP/TLS handshake insideBUILDER_BID_DEADLINE_MS, which a distant builder can exhaust on the handshake alone.On glamsterdam-devnet-8 with 8 builders configured, 21 of 41 bid requests failed with
Timeout getExecutionPayloadBid request. Node drops idle keep-alive sockets after ~4s, measured against a builder at 262ms RTT:prepareNextSlotruns atPREPARE_NEXT_SLOT_BPS, a slot before the proposal, so a check there is cold again by the time bids are requested. Scheduling it close to the slot boundary instead, same builder:Running this on the same node took bid request errors from 51% (21/41) to 5.7% (8/140), and bids received from 49% to 83%.
checkStatus()toBuilderApiClient, pinging every known builder concurrently, failures are logged and never propagateprepareNextSlotpost-gloas, scheduledBUILDER_STATUS_CHECK_BEFORE_SLOT_BPSbefore the slot we proposelodestar_builder_api_status_checks_totalRaising the client keep-alive is not a substitute, a connection idle for minutes through NAT is frequently half-open and fails only once written to, inside the deadline where there is no recovery.
Only builders already in the
clientsmap are pinged, which the preference submissions populate ~96s ahead of a proposal. That map is never evicted, so a builder dropped from the config keeps being pinged until restart, see #9951.