Add SSZ to PBS #372
Add SSZ to PBS #372
Conversation
crates/pbs/src/routes/get_header.rs
Outdated
|
|
||
| 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 { |
There was a problem hiding this comment.
here we just asssume the relay just support both? probably fine but ok double checking
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
would rather default to json here instead just in case?
| /// 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> { |
There was a problem hiding this comment.
should we add a few unit tests for this function?
| req_headers | ||
| .get(CONSENSUS_VERSION_HEADER) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .unwrap_or(""), |
There was a problem hiding this comment.
if missing should we default to the current fork instead?
| impl FromStr for EncodingType { | ||
| type Err = String; | ||
| fn from_str(value: &str) -> Result<Self, Self::Err> { | ||
| match value { |
There was a problem hiding this comment.
should we match on lowercase string?
| send_headers.clone(), | ||
| state.pbs_config().timeout_register_validator_ms, | ||
| state.pbs_config().register_validator_retry_limit, | ||
| handles.push( |
There was a problem hiding this comment.
do you why these unrelated changes show up here? they should already be in main
| 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()); | ||
| } |
There was a problem hiding this comment.
this should be a PbsError where we implement into_response already, ideally we keep all the error mappings there
| 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(); |
There was a problem hiding this comment.
we should encode as SSZ after we checked that we can return it, otherwise we might do the SSZ encoding for nothing
| return Ok((StatusCode::OK, axum::Json(max_bid)).into_response()); | ||
| }; | ||
| let Ok(content_type_header) = | ||
| HeaderValue::from_str(&format!("{}", EncodingType::Ssz)) |
There was a problem hiding this comment.
this could be a static, so we avoid the string allocation
| 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| { |
There was a problem hiding this comment.
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
| )); | ||
| }; | ||
| let Ok(content_type_header) = | ||
| HeaderValue::from_str(&EncodingType::Ssz.to_string()) |
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.