Skip to content

Commit 1927bce

Browse files
committed
local audit fix: restrict SSZ→JSON retry to 406/415 per builder-spec
1 parent f4f1b2f commit 1927bce

4 files changed

Lines changed: 222 additions & 36 deletions

File tree

crates/common/src/utils.rs

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,8 @@ pub async fn read_chunked_body_with_max(
130130
}
131131

132132
// Break if content length is provided but it's too big
133-
if let Some(length) = content_length
134-
&& length as usize > max_size
133+
if let Some(length) = content_length &&
134+
length as usize > max_size
135135
{
136136
return Err(ResponseReadError::PayloadTooLarge {
137137
max: max_size,
@@ -818,47 +818,47 @@ fn get_ssz_value_offset_for_fork(fork: ForkName) -> Option<usize> {
818818
ForkName::Bellatrix => {
819819
// Message goes header -> value -> pubkey
820820
Some(
821-
get_message_offset::<BuilderBidBellatrix>()
822-
+ <ExecutionPayloadHeaderBellatrix as ssz::Decode>::ssz_fixed_len(),
821+
get_message_offset::<BuilderBidBellatrix>() +
822+
<ExecutionPayloadHeaderBellatrix as ssz::Decode>::ssz_fixed_len(),
823823
)
824824
}
825825

826826
ForkName::Capella => {
827827
// Message goes header -> value -> pubkey
828828
Some(
829-
get_message_offset::<BuilderBidCapella>()
830-
+ <ExecutionPayloadHeaderCapella as ssz::Decode>::ssz_fixed_len(),
829+
get_message_offset::<BuilderBidCapella>() +
830+
<ExecutionPayloadHeaderCapella as ssz::Decode>::ssz_fixed_len(),
831831
)
832832
}
833833

834834
ForkName::Deneb => {
835835
// Message goes header -> blob_kzg_commitments -> value -> pubkey
836836
Some(
837-
get_message_offset::<BuilderBidDeneb>()
838-
+ <ExecutionPayloadHeaderDeneb as ssz::Decode>::ssz_fixed_len()
839-
+ <KzgCommitments as ssz::Decode>::ssz_fixed_len(),
837+
get_message_offset::<BuilderBidDeneb>() +
838+
<ExecutionPayloadHeaderDeneb as ssz::Decode>::ssz_fixed_len() +
839+
<KzgCommitments as ssz::Decode>::ssz_fixed_len(),
840840
)
841841
}
842842

843843
ForkName::Electra => {
844844
// Message goes header -> blob_kzg_commitments -> execution_requests -> value ->
845845
// pubkey
846846
Some(
847-
get_message_offset::<BuilderBidElectra>()
848-
+ <ExecutionPayloadHeaderElectra as ssz::Decode>::ssz_fixed_len()
849-
+ <KzgCommitments as ssz::Decode>::ssz_fixed_len()
850-
+ <ExecutionRequests as ssz::Decode>::ssz_fixed_len(),
847+
get_message_offset::<BuilderBidElectra>() +
848+
<ExecutionPayloadHeaderElectra as ssz::Decode>::ssz_fixed_len() +
849+
<KzgCommitments as ssz::Decode>::ssz_fixed_len() +
850+
<ExecutionRequests as ssz::Decode>::ssz_fixed_len(),
851851
)
852852
}
853853

854854
ForkName::Fulu => {
855855
// Message goes header -> blob_kzg_commitments -> execution_requests -> value ->
856856
// pubkey
857857
Some(
858-
get_message_offset::<BuilderBidFulu>()
859-
+ <ExecutionPayloadHeaderFulu as ssz::Decode>::ssz_fixed_len()
860-
+ <KzgCommitments as ssz::Decode>::ssz_fixed_len()
861-
+ <ExecutionRequests as ssz::Decode>::ssz_fixed_len(),
858+
get_message_offset::<BuilderBidFulu>() +
859+
<ExecutionPayloadHeaderFulu as ssz::Decode>::ssz_fixed_len() +
860+
<KzgCommitments as ssz::Decode>::ssz_fixed_len() +
861+
<ExecutionRequests as ssz::Decode>::ssz_fixed_len(),
862862
)
863863
}
864864

crates/pbs/src/mev_boost/submit_block.rs

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,8 @@ async fn submit_block_with_timeout(
173173
// The caller (routes/submit_block.rs) serialises Full/Light responses
174174
// with the caller's negotiated encoding, independent of which endpoint
175175
// the relay actually served.
176-
if request_api_version == BuilderApiVersion::V1
177-
&& proposal_info.api_version != request_api_version
176+
if request_api_version == BuilderApiVersion::V1 &&
177+
proposal_info.api_version != request_api_version
178178
{
179179
warn!(
180180
relay_id = relay.id.as_ref(),
@@ -472,12 +472,17 @@ async fn send_submit_block_impl(
472472
}
473473
};
474474

475-
// If we got a client error, retry with JSON - the spec says that this should be
476-
// a 406 or 415, but we're a little more permissive here
477-
if res.status().is_client_error() {
475+
// Retry as JSON only on the two status codes the builder-spec defines as
476+
// "media type is the problem": 406 Not Acceptable and 415 Unsupported
477+
// Media Type (RFC 7231 §6.5.13). Any other 4xx (400 malformed, 401/403
478+
// auth, 409 conflict, 429 rate limit, etc.) is orthogonal to encoding
479+
// and MUST surface unchanged — retrying pollutes observability, doubles
480+
// load on the relay, and can mask real errors behind a JSON-path reply.
481+
if matches!(res.status(), StatusCode::NOT_ACCEPTABLE | StatusCode::UNSUPPORTED_MEDIA_TYPE,) {
478482
warn!(
479483
relay_id = relay.id.as_ref(),
480-
"relay does not support SSZ, resubmitting block with JSON content-type"
484+
status = %res.status(),
485+
"relay rejected SSZ content-type, resubmitting block with JSON content-type"
481486
);
482487
res = match relay
483488
.client
@@ -631,12 +636,12 @@ fn validate_unblinded_block(
631636
fork_name: ForkName,
632637
) -> Result<(), PbsError> {
633638
match fork_name {
634-
ForkName::Base
635-
| ForkName::Altair
636-
| ForkName::Bellatrix
637-
| ForkName::Capella
638-
| ForkName::Deneb
639-
| ForkName::Gloas => Err(PbsError::Validation(ValidationError::UnsupportedFork)),
639+
ForkName::Base |
640+
ForkName::Altair |
641+
ForkName::Bellatrix |
642+
ForkName::Capella |
643+
ForkName::Deneb |
644+
ForkName::Gloas => Err(PbsError::Validation(ValidationError::UnsupportedFork)),
640645
ForkName::Electra => validate_unblinded_block_electra(
641646
expected_block_hash,
642647
got_block_hash,
@@ -665,9 +670,9 @@ fn validate_unblinded_block_electra(
665670
}));
666671
}
667672

668-
if expected_commitments.len() != blobs_bundle.blobs.len()
669-
|| expected_commitments.len() != blobs_bundle.commitments.len()
670-
|| expected_commitments.len() != blobs_bundle.proofs.len()
673+
if expected_commitments.len() != blobs_bundle.blobs.len() ||
674+
expected_commitments.len() != blobs_bundle.commitments.len() ||
675+
expected_commitments.len() != blobs_bundle.proofs.len()
671676
{
672677
return Err(PbsError::Validation(ValidationError::KzgCommitments {
673678
expected_blobs: expected_commitments.len(),
@@ -704,9 +709,9 @@ fn validate_unblinded_block_fulu(
704709
}));
705710
}
706711

707-
if expected_commitments.len() != blobs_bundle.blobs.len()
708-
|| expected_commitments.len() != blobs_bundle.commitments.len()
709-
|| expected_commitments.len() * CELLS_PER_EXT_BLOB != blobs_bundle.proofs.len()
712+
if expected_commitments.len() != blobs_bundle.blobs.len() ||
713+
expected_commitments.len() != blobs_bundle.commitments.len() ||
714+
expected_commitments.len() * CELLS_PER_EXT_BLOB != blobs_bundle.proofs.len()
710715
{
711716
return Err(PbsError::Validation(ValidationError::KzgCommitments {
712717
expected_blobs: expected_commitments.len(),

tests/src/mock_relay.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ pub struct MockRelayState {
6363
large_body: bool,
6464
supports_submit_block_v2: bool,
6565
use_not_found_for_submit_block: bool,
66+
/// If set, `handle_submit_block_v1`/`v2` short-circuits with this status
67+
/// when the inbound request carries `Content-Type:
68+
/// application/octet-stream`. The counter is still incremented before
69+
/// the short-circuit so tests can observe the attempt. Used to drive C3
70+
/// (retry-as-JSON) tests.
71+
submit_block_ssz_status_override: Option<StatusCode>,
6672
received_get_header: Arc<AtomicU64>,
6773
received_get_status: Arc<AtomicU64>,
6874
received_register_validator: Arc<AtomicU64>,
@@ -93,6 +99,9 @@ impl MockRelayState {
9399
pub fn use_not_found_for_submit_block(&self) -> bool {
94100
self.use_not_found_for_submit_block
95101
}
102+
pub fn submit_block_ssz_status_override(&self) -> Option<StatusCode> {
103+
self.submit_block_ssz_status_override
104+
}
96105
pub fn set_response_override(&self, status: StatusCode) {
97106
*self.response_override.write().unwrap() = Some(status);
98107
}
@@ -106,6 +115,7 @@ impl MockRelayState {
106115
large_body: false,
107116
supports_submit_block_v2: true,
108117
use_not_found_for_submit_block: false,
118+
submit_block_ssz_status_override: None,
109119
received_get_header: Default::default(),
110120
received_get_status: Default::default(),
111121
received_register_validator: Default::default(),
@@ -136,6 +146,14 @@ impl MockRelayState {
136146
pub fn with_not_found_for_submit_block(self) -> Self {
137147
Self { use_not_found_for_submit_block: true, ..self }
138148
}
149+
150+
/// Make `handle_submit_block_v1`/`v2` respond with `status` whenever the
151+
/// request comes in as SSZ (`Content-Type: application/octet-stream`).
152+
/// JSON requests still go through the normal happy path, which lets a
153+
/// single test cover the SSZ→JSON retry behavior.
154+
pub fn with_submit_block_ssz_status(self, status: StatusCode) -> Self {
155+
Self { submit_block_ssz_status_override: Some(status), ..self }
156+
}
139157
}
140158

141159
pub fn mock_relay_app_router(state: Arc<MockRelayState>) -> Router {
@@ -301,6 +319,14 @@ async fn handle_submit_block_v1(
301319
return StatusCode::NOT_FOUND.into_response();
302320
}
303321
state.received_submit_block.fetch_add(1, Ordering::Relaxed);
322+
// Short-circuit SSZ requests with an overridden status so tests can
323+
// drive the PBS SSZ→JSON retry logic. JSON requests still take the
324+
// normal path so a single mock run can exercise both attempts.
325+
if let Some(status) = state.submit_block_ssz_status_override() &&
326+
get_content_type(&headers) == EncodingType::Ssz
327+
{
328+
return (status, "forced ssz override").into_response();
329+
}
304330
let accept_types = get_accept_types(&headers)
305331
.map_err(|e| (StatusCode::BAD_REQUEST, format!("error parsing accept header: {e}")));
306332
if let Err(e) = accept_types {
@@ -391,6 +417,13 @@ async fn handle_submit_block_v2(
391417
return StatusCode::NOT_FOUND.into_response();
392418
}
393419
state.received_submit_block.fetch_add(1, Ordering::Relaxed);
420+
// See comment in `handle_submit_block_v1`. Override SSZ with the
421+
// injected status so C3 tests can assert retry / no-retry behavior.
422+
if let Some(status) = state.submit_block_ssz_status_override() &&
423+
get_content_type(&headers) == EncodingType::Ssz
424+
{
425+
return (status, "forced ssz override").into_response();
426+
}
394427
let content_type = get_content_type(&headers);
395428
if !state.supported_content_types.contains(&content_type) {
396429
return (StatusCode::NOT_ACCEPTABLE, "No acceptable content type found".to_string())

tests/tests/pbs_post_blinded_blocks.rs

Lines changed: 149 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,6 @@ async fn submit_block_impl(
509509
remove_v2_support: bool,
510510
force_404s: bool,
511511
) -> Result<Response> {
512-
// Setup test environment
513512
setup_test_env();
514513
let signer = random_secret();
515514
let pubkey = signer.public_key();
@@ -574,3 +573,152 @@ async fn submit_block_impl(
574573
assert_eq!(res.status(), expected_code);
575574
Ok(res)
576575
}
576+
577+
// Retry-as-JSON trigger must be restricted
578+
// to 406 Not Acceptable and 415 Unsupported Media Type. Any other 4xx is
579+
// orthogonal to encoding and MUST surface unchanged.
580+
581+
/// Shared fixture: relay returns `ssz_status` when the PBS sends SSZ,
582+
/// everything else takes the happy path. Returns `(Response, attempt_count)`.
583+
/// `api_version` picks v1 or v2 endpoint; `relay_types` controls what the
584+
/// relay advertises as supported so the happy JSON path works when retried.
585+
async fn submit_block_ssz_override(
586+
api_version: BuilderApiVersion,
587+
ssz_status: StatusCode,
588+
) -> Result<(Response, u64)> {
589+
setup_test_env();
590+
let signer = random_secret();
591+
let pubkey = signer.public_key();
592+
let chain = Chain::Holesky;
593+
let pbs_listener = get_free_listener().await;
594+
let relay_listener = get_free_listener().await;
595+
let pbs_port = pbs_listener.local_addr().unwrap().port();
596+
let relay_port = relay_listener.local_addr().unwrap().port();
597+
598+
let mock_relay = generate_mock_relay(relay_port, pubkey)?;
599+
let mut mock_relay_state = MockRelayState::new(chain, signer);
600+
// Relay only advertises JSON so the retry (which goes out as JSON) lands
601+
// on a clean success path. The SSZ-status override below intercepts
602+
// before the supported-types check, so the first SSZ attempt still hits
603+
// our injected status regardless of what's advertised here.
604+
mock_relay_state.supported_content_types = Arc::new(HashSet::from([EncodingType::Json]));
605+
mock_relay_state = mock_relay_state.with_submit_block_ssz_status(ssz_status);
606+
let mock_state = Arc::new(mock_relay_state);
607+
tokio::spawn(start_mock_relay_service_with_listener(mock_state.clone(), relay_listener));
608+
609+
let pbs_config = get_pbs_config(pbs_port);
610+
let config = to_pbs_config(chain, pbs_config, vec![mock_relay]);
611+
let state = PbsState::new(config, PathBuf::new());
612+
drop(pbs_listener);
613+
tokio::spawn(PbsService::run::<(), DefaultBuilderApi>(state));
614+
615+
tokio::time::sleep(Duration::from_millis(100)).await;
616+
617+
let signed_blinded_block = load_test_signed_blinded_block();
618+
let mock_validator = MockValidator::new(pbs_port)?;
619+
// The BN sends SSZ; PBS forwards SSZ first, that's what our override hits.
620+
let accept_types = HashSet::from([EncodingType::Ssz, EncodingType::Json]);
621+
let res = match api_version {
622+
BuilderApiVersion::V1 => {
623+
mock_validator
624+
.do_submit_block_v1(
625+
Some(signed_blinded_block),
626+
accept_types,
627+
EncodingType::Ssz,
628+
ForkName::Electra,
629+
)
630+
.await?
631+
}
632+
BuilderApiVersion::V2 => {
633+
mock_validator
634+
.do_submit_block_v2(
635+
Some(signed_blinded_block),
636+
accept_types,
637+
EncodingType::Ssz,
638+
ForkName::Electra,
639+
)
640+
.await?
641+
}
642+
};
643+
Ok((res, mock_state.received_submit_block()))
644+
}
645+
646+
/// 406 is the spec-defined "retry with a different media type" signal, so we
647+
/// MUST retry as JSON and succeed.
648+
#[tokio::test]
649+
async fn test_submit_block_ssz_retries_as_json_on_406() -> Result<()> {
650+
let (res, attempts) =
651+
submit_block_ssz_override(BuilderApiVersion::V1, StatusCode::NOT_ACCEPTABLE).await?;
652+
assert_eq!(res.status(), StatusCode::OK, "retry-as-JSON must succeed on 406");
653+
assert_eq!(attempts, 2, "expected SSZ attempt + JSON retry");
654+
Ok(())
655+
}
656+
657+
/// 415 is the other spec-defined media-type rejection status; same retry.
658+
#[tokio::test]
659+
async fn test_submit_block_ssz_retries_as_json_on_415() -> Result<()> {
660+
let (res, attempts) =
661+
submit_block_ssz_override(BuilderApiVersion::V1, StatusCode::UNSUPPORTED_MEDIA_TYPE)
662+
.await?;
663+
assert_eq!(res.status(), StatusCode::OK, "retry-as-JSON must succeed on 415");
664+
assert_eq!(attempts, 2);
665+
Ok(())
666+
}
667+
668+
/// 400 Bad Request is a validation failure — encoding is not the problem.
669+
/// Retrying doubles relay load and hides the real error. MUST NOT retry.
670+
#[tokio::test]
671+
async fn test_submit_block_ssz_does_not_retry_on_400() -> Result<()> {
672+
let (_res, attempts) =
673+
submit_block_ssz_override(BuilderApiVersion::V1, StatusCode::BAD_REQUEST).await?;
674+
assert_eq!(attempts, 1, "400 is not a media-type error; must not retry");
675+
Ok(())
676+
}
677+
678+
/// 401 Unauthorized — auth problem, not encoding. No retry.
679+
#[tokio::test]
680+
async fn test_submit_block_ssz_does_not_retry_on_401() -> Result<()> {
681+
let (_res, attempts) =
682+
submit_block_ssz_override(BuilderApiVersion::V1, StatusCode::UNAUTHORIZED).await?;
683+
assert_eq!(attempts, 1);
684+
Ok(())
685+
}
686+
687+
/// 409 Conflict — state mismatch. No retry.
688+
#[tokio::test]
689+
async fn test_submit_block_ssz_does_not_retry_on_409() -> Result<()> {
690+
let (_res, attempts) =
691+
submit_block_ssz_override(BuilderApiVersion::V1, StatusCode::CONFLICT).await?;
692+
assert_eq!(attempts, 1);
693+
Ok(())
694+
}
695+
696+
/// 429 Too Many Requests — `PbsError::should_retry` already excludes this;
697+
/// retrying as JSON would add insult to injury. No retry.
698+
#[tokio::test]
699+
async fn test_submit_block_ssz_does_not_retry_on_429() -> Result<()> {
700+
let (_res, attempts) =
701+
submit_block_ssz_override(BuilderApiVersion::V1, StatusCode::TOO_MANY_REQUESTS).await?;
702+
assert_eq!(attempts, 1);
703+
Ok(())
704+
}
705+
706+
/// Same policy applies to the v2 endpoint.
707+
#[tokio::test]
708+
async fn test_submit_block_v2_ssz_retries_as_json_on_415() -> Result<()> {
709+
let (res, attempts) =
710+
submit_block_ssz_override(BuilderApiVersion::V2, StatusCode::UNSUPPORTED_MEDIA_TYPE)
711+
.await?;
712+
assert_eq!(res.status(), StatusCode::ACCEPTED, "v2 success is 202 Accepted");
713+
assert_eq!(attempts, 2);
714+
Ok(())
715+
}
716+
717+
/// v2 + 400: same no-retry rule as v1.
718+
#[tokio::test]
719+
async fn test_submit_block_v2_ssz_does_not_retry_on_400() -> Result<()> {
720+
let (_res, attempts) =
721+
submit_block_ssz_override(BuilderApiVersion::V2, StatusCode::BAD_REQUEST).await?;
722+
assert_eq!(attempts, 1);
723+
Ok(())
724+
}

0 commit comments

Comments
 (0)