Skip to content

Conversation

@jclapis
Copy link
Collaborator

@jclapis jclapis commented Sep 9, 2025

This is a modernization of #252 since that's been dormant for a while, but was re-raised in #364. Just about everything was ported over cleanly.

NOTE: This now has #397 built in, so that should be merged first.

@jclapis jclapis requested a review from ltitanb September 9, 2025 20:38
@jclapis jclapis self-assigned this Sep 9, 2025
@jclapis jclapis added the pbs Pbs module / Builder API label Sep 9, 2025
Copy link
Collaborator

@ltitanb ltitanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should ideally also test this with kurtosis


BEACON_NODE_STATUS.with_label_values(&["200", GET_HEADER_ENDPOINT_TAG]).inc();
Ok((StatusCode::OK, axum::Json(max_bid)).into_response())
let response = match accept_header {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we just asssume the relay just support both? probably fine but ok double checking

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, if we want to add support for relays that only allow JSON for example, we'll have to probably figure that out on startup and flag them accordingly so we don't ping them to negotiate encoding with every request (assuming they never change it down the line). Do we have stats on how many support SSZ and how many don't?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about catching an error and resubmitting with a different encoding? i assume that's what the BN does already instead of mapping whether a given sidecar supports what

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a significant rework; so much so that it now lives in its own branch built on top of this one: https://github.com/Commit-Boost/commit-boost-client/tree/ssz-update-v2

Copy link
Collaborator

@ltitanb ltitanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general i think we should try to keep a "charitable" approach, if the request is malformed / some header is missing we should try to pick a reasonable default based on current information (eg current fork, JSON default) and log a warning , instead of returning an error

accept.media_types().for_each(|mt| match mt.essence().to_string().as_str() {
APPLICATION_OCTET_STREAM => ssz_type = true,
APPLICATION_JSON | WILDCARD => json_type = true,
_ => unsupported_type = true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would rather default to json here instead just in case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed somewhat in v2, so now application/octet-stream corresponds to SSZ and application/json, */*, and no header at all correspond to JSON. I don't think it's a good idea to reinterpret anything else as JSON (like application/text or something) because the client went out of its way to tell us that it doesn't accept JSON. Giving it JSON back anyway is going to be messy.

/// defaulting to JSON if missing. Returns an error if malformed or unsupported
/// types are requested. Supports requests with multiple ACCEPT headers or
/// headers with multiple media types.
pub fn get_accept_type(req_headers: &HeaderMap) -> eyre::Result<EncodingType> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a few unit tests for this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. v2 has lots of them - take a look.

req_headers
.get(CONSENSUS_VERSION_HEADER)
.and_then(|value| value.to_str().ok())
.unwrap_or(""),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if missing should we default to the current fork instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to stick to the spec on this one, which says if the request / response use SSZ, then the header must be present; otherwise it doesn't have to be. Either way, this particular function probably isn't the proper place to backfill a missing header; whatever uses it can decide to do that if this reports a missing header, it really needs to.

impl FromStr for EncodingType {
type Err = String;
fn from_str(value: &str) -> Result<Self, Self::Err> {
match value {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we match on lowercase string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Hyper will convert them all to lowercase, but I added an explicit conversion just in case in b22eed8.

send_headers.clone(),
state.pbs_config().timeout_register_validator_ms,
state.pbs_config().register_validator_retry_limit,
handles.push(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you why these unrelated changes show up here? they should already be in main

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, maybe something related to a merge - there have been a lot of them since this branch started.

Comment on lines +38 to +44
let accept_type = get_accept_type(&req_headers).map_err(|e| {
error!(%e, "error parsing accept header");
PbsClientError::DecodeError(format!("error parsing accept header: {e}"))
});
if let Err(e) = accept_type {
return Ok((StatusCode::BAD_REQUEST, e).into_response());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a PbsError where we implement into_response already, ideally we keep all the error mappings there

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, check 8ebfbd0 and see if that's what you're looking for.

Ok((StatusCode::OK, axum::Json(max_bid)).into_response())
let response = match accept_type {
EncodingType::Ssz => {
let mut res = max_bid.data.as_ssz_bytes().into_response();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should encode as SSZ after we checked that we can return it, otherwise we might do the SSZ encoding for nothing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's what it's doing, no? If ACCEPT has application/octet-stream in it then we proceed, since the request told us it supports SSZ. v2 actually support multiple ACCEPT headers or one header with multiple comma-separated types in it, but same idea.

return Ok((StatusCode::OK, axum::Json(max_bid)).into_response());
};
let Ok(content_type_header) =
HeaderValue::from_str(&format!("{}", EncodingType::Ssz))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a static, so we avoid the string allocation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, fixed in 2499bd5.

api_version: BuilderApiVersion,
) -> Result<impl IntoResponse, PbsClientError> {
let signed_blinded_block = Arc::new(
deserialize_body(&req_headers, raw_request.body_bytes).await.map_err(|e| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deserialize_body could return either a PbsError or an error that has a #[from] in PbsError, so we avoid the manual error mapping, we can still log it with eg .inspect_err if needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't know you could do this - more Rust idiosyncrasies. Take a look at 41d879e, how's that?

));
};
let Ok(content_type_header) =
HeaderValue::from_str(&EncodingType::Ssz.to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a static

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixed in 2499bd5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pbs Pbs module / Builder API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants