feat(builder): add readiness gating and builder identity resolution - #9781
feat(builder): add readiness gating and builder identity resolution#9781markolazic01 wants to merge 35 commits into
Conversation
|
Marking this ready. Leaving metrics and readiness tests as a follow-up. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 250ae7bff1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
nflaig
left a comment
There was a problem hiding this comment.
did a quick pass, generally looks pretty good
|
|
||
| if (builderEntry.status !== "active") { | ||
| throw Error(`Builder not active: ${builderEntry.status}`); | ||
| } |
There was a problem hiding this comment.
what if builder is deposited later, should this wait instead of throwing?
There was a problem hiding this comment.
we can add the wait, probably on the fetchBuilder()'s length check:
https://github.com/markolazic01/lodestar/blob/51c3493b2b7cfd788d665b99611de9856d6307b1/packages/builder/src/identity.ts#L61
because that would indicate that the builder is not yet known, while status other than "active" indicates that builder exited.
How does this sound?
There was a problem hiding this comment.
yep that sounds good, the reason I brought this up is because it's quite common for operators today to import bls keys in the validator client even if those are completely unknown to the chain, but it's good so you can prepare your setup, then later, once those keys are deposited and become known to the chain, the validator clients picks them up automatically with no interaction from the operator. Similar ux would be great to have for the builder
|
|
||
| if (!builderRes.ok) { | ||
| throw Error(`Failed to get builder state from beacon node: ${builderRes.status}`); | ||
| } |
There was a problem hiding this comment.
might be useful to also print out the error response from the beacon node
There was a problem hiding this comment.
applied, noting this requires await res.errorBody() first, as error() reads the error body synchronously.
There was a problem hiding this comment.
noting this requires await res.errorBody()
I don't like this pattern, see #9781 (comment)
| if (!builderRes.ok) { | ||
| await builderRes.errorBody(); |
There was a problem hiding this comment.
I don't really like this manual checking for ok and then handling the error like this, the api client is designed in a way that most of the time it's better to use .value() or .assertOk()
There was a problem hiding this comment.
this is fixed now, there is another check in here:
https://github.com/markolazic01/lodestar/blob/7386db08081ce635662d6898c925036cd2015b71/packages/builder/src/readiness.ts#L17-L24
I left this one in because I haven't found a cleaner way to log the error message otherwise.
There was a problem hiding this comment.
that's fine but keep in mind that network errors will still throw (eg. host not reachable or any tcp error)
regarding http errors (which ok is related to) there shouldn't be any scenario where getSyncingStatus returns 500 or similar
so need to make sure to handle network error gracefully which usually requires a try/catch or .catch
|
|
||
| export async function logNodeVersion(api: ApiClient, logger: Logger): Promise<void> { | ||
| try { | ||
| const versionRes = await api.node.getNodeVersionV2(); |
There was a problem hiding this comment.
some clients may not have this api yet, but that's fine for now, we might need to bully them a bit to implement it, but since it's gloas related, it's fine to assume it's implemented by clients
|
the title is a bit too long, would be good to make it more succinct, details are in the commit body afterwards based on the PR description |
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
|
Thanks for reviewing, applied the suggestions and updated the description. |
| return apiResponse; | ||
| } | ||
|
|
||
| export async function mockApiErrorResponse<E extends Endpoint>(status: HttpStatusCode): Promise<ApiResponse<E>> { |
There was a problem hiding this comment.
maybe could dedupe these in packages/test-utils if it's 1:1 what the validator uses, would be rather a follow-up PR though
There was a problem hiding this comment.
The only difference between implementation here and in the builder would be the functions listed here:
but I think it's alright to have all the functions that consumers need in one place.
I also removed some unused parts of it here, but these two can be put in sync based on what is used.
There was a problem hiding this comment.
Should we tell lodekeeper to make an issue for it? So we don't forget about this.
There was a problem hiding this comment.
@lodekeeper can you create an issue on this?
It's about adapting apiStub helper and moving it into a common place (packages/test-utils) after this PR is merged.
There was a problem hiding this comment.
Done — opened #9819 to track this.
Captured the reconciliation points from the current files: mockApiResponse is already identical, the getApiClientStub function lists differ (builder is a minimal subset + getStateBuilders), and — one extra beyond the function list — the builder's mockApiErrorResponse is now async + await res.errorBody() vs the validator's sync version, so the dedup will need to reconcile that too.
|
|
||
| if (syncingStatus.isOptimistic) { | ||
| logger.warn("Beacon node head is optimistic, execution payloads are not yet verified"); | ||
| } |
There was a problem hiding this comment.
so for the operator it should be clear that in case the EL is syncing we won't be able to submit any bids
| throw Error(`Builder not known to the beacon node: ${id}`); | ||
| } | ||
|
|
||
| return builders[0]; |
There was a problem hiding this comment.
we should probably sanity check the response from the beacon node to make sure it resolved "our" builder
| if (builderStatus.status !== "active") { | ||
| throw Error(`Builder not active: ${builderStatus.status}`); | ||
| } | ||
| const index = await resolveBuilderIdentity(api, logger, builderSigner.getPubkeyHex()); |
There was a problem hiding this comment.
so this still assumes the builder is active before starting up the builder client? see #9781 (comment)
I do think this is fine for now, we don't need to address it in this PR, it's more a nice to have to improve ux for operators
Motivation
Further development of ePBS builder entity.
Description
identity.tsand expands basic functionality intoresolveBuilderIdentityandgetBuilderStatus.readiness.ts, containingwaitForNodeReadyfunction which polls until the BN is synced and its EL is online +logNodeVersion.BuilderStatusTrackerservice for tracking builder status and balance.--executionFeeRecipientcli optionLacks: