-
Notifications
You must be signed in to change notification settings - Fork 61
Add SSZ to PBS #372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add SSZ to PBS #372
Conversation
ltitanb
left a comment
There was a problem hiding this 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
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.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
ltitanb
left a comment
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(""), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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.
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
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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| { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed in 2499bd5.
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.