feat(pbs): add builder SSZ flow #252
Conversation
| BEACON_NODE_STATUS.with_label_values(&["200", SUBMIT_BLINDED_BLOCK_ENDPOINT_TAG]).inc(); | ||
| Ok((StatusCode::OK, Json(res).into_response())) | ||
|
|
||
| let response = match content_type_header { |
There was a problem hiding this comment.
Hey @eserilev, this response type should be based on the Accept header value, like we do in handle_get_header.
For reference, see this line in the spec: https://github.com/ethereum/builder-specs/blob/1bbe1e299aa174aa6b7ec7a84c59c678b4e1b063/apis/builder/blinded_blocks.yaml#L83
Use Accept header to choose this response type
There was a problem hiding this comment.
thanks for catching that, this should be fixed now
crates/common/src/utils.rs
Outdated
| pub enum ContentType { | ||
| Json, | ||
| Ssz | ||
| } |
There was a problem hiding this comment.
Just a nit, but I would rename this enum to MediaType which is more accurate.
There was a problem hiding this comment.
Well actually, I understand what you're doing now. You're splitting this into ContentType and Accept. I would probably combine these into a MediaType enum and try to not have an Any value.
crates/pbs/src/routes/get_header.rs
Outdated
| Accept::Ssz => { | ||
| let mut res = { | ||
| info!("sending response as JSON"); | ||
| (StatusCode::OK, max_bid.data.as_ssz_bytes()).into_response() | ||
| }; |
There was a problem hiding this comment.
I'm a little confused by this log message. It's wrong, right?
| if content_type.starts_with(&ContentType::Ssz.to_string()) { | ||
| let payload = T::from_ssz_bytes(&bytes) | ||
| .map_err(|_| StatusCode::BAD_REQUEST.into_response())?; | ||
| return Ok(Self(payload)); | ||
| } |
There was a problem hiding this comment.
I must be missing something. Where in this PR are we reading the Eth-Consensus-Version header value? I do not see a headers.get(CONSENSUS_VERSION_HEADER) call anywhere.
There was a problem hiding this comment.
Commit boost only works for Deneb at the moment so the consensus version header isn't relevant. They are working on supporting Electra in a separate PR. That PR should be adding logic to read the consensus version header
|
I've added some integration tests for the new ssz flow. I haven't had a chance to test this in kurtosis yet. I was having some difficulty running commit-boost in kurtosis (some missing config error). I'll try again this week and if I have issues I'll reach out on TG |
|
the kurtosis config issue is fixed in this PR: ethpandaops/ethereum-package#906 |
|
im working on resolving merge conflicts |
|
the kurtosis PR has been merged and will be released in 4.6.0 ethpandaops/ethereum-package#902 |
Spec: ethereum/builder-specs#104
Right now both mev-boost and commit-boost don't support the new SSZ builder flow. Theres already some effort being done to get mev-boost updated, so thought I'd go ahead and add SSZ to commit-boost. Lighthouse unstable already supports the new SSZ flow, i'll try testing locally and report back with the results of those tests.
It'd also be really really nice if we could get electra support in commit-boost. Not sure if any progress has been made on that front, if not I can try tackling that next. Ideally we'd love multiple builder implementations to test on electra testnets before the hardfork