starknet_transaction_prover: avoid redundant request-id re-parse in RequestSpanLayer - #14743
Conversation
…equestSpanLayer RequestSpanLayer re-parsed and re-validated the x-request-id header on every plaintext request via extract_or_generate_request_id, even though RequestLogLayer (upstream in the same chain) already computed and validated that exact id moments earlier. RequestLogLayer now stashes the validated id in a RequestId request extension; RequestSpanLayer reads it directly, falling back to re-deriving it only when used standalone (e.g. without RequestLogLayer upstream, as in some unit tests). Follow-up to #14222.
PR SummaryLow Risk Overview
Adds Reviewed by Cursor Bugbot for commit 981c4c8. Bugbot is set up for automated code reviews on this repo. Configure here. |
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gkaempfer).
|
Security scan complete — no issues detected. Generated by Claude Code |
What
Follow-up perf cleanup to #14222, which added
RequestSpanLayer(crates/starknet_transaction_prover/src/server/request_span.rs) below the OHTTP layer to bind a tracing span with a request-id over every HTTP request handled by the transaction-prover server.For plaintext (non-OHTTP) requests,
RequestSpanLayercalledextract_or_generate_request_id(&request), which re-parses and re-validates thex-request-idheader (UTF-8 conversion, length check, byte-by-byte ASCII-safety scan,Stringallocation) on every single request — even though the upstreamRequestLogLayer(request_log.rs), earlier in the same middleware chain (prover_http_middleware!inserver.rs), already computed and validated that exact id moments earlier.Fix
RequestLogLayer::callnow stashes the already-validated id in a newRequestId(String)request extension right after computing it.RequestSpanLayer::callreads that extension first, and only falls back toextract_or_generate_request_idwhen the extension is absent — e.g. whenRequestSpanLayeris exercised standalone in unit tests withoutRequestLogLayerupstream. The OHTTP-decapsulated branch (fresh envelope-unlinkable id) is untouched — it never reads this extension.This is safe because request extensions survive
MapRequestBodyLayer(http::Request::mappreserves them) and the OHTTP layer's non-OHTTP passthrough route (inner.oneshot(request)with the request unchanged), so the extension set by the outerRequestLogLayerreaches the innerRequestSpanLayerunmodified in the production chain.Tests
plaintext_prefers_request_id_extension_over_header, which sets theRequestIdextension and thex-request-idheader to two different values (a stateRequestLogLayernever produces in practice) and asserts, via a traced log line emitted inside the span, that the extension's value — not the header's — is what the span carries. This locks in the fast path so a future refactor can't silently regress back to header re-parsing.starknet_transaction_prover/tower_ohttptests pass (122 passed, 0 regressions; the only failure,test_compiled_class_v1_to_casm_round_trip, is a pre-existing, network-dependent Cairo-compiler-download failure unrelated to this change and reproduces identically on the unmodified base branch).cargo clippy -p starknet_transaction_prover -p tower_ohttp --all-targetsis clean.scripts/rust_fmt.shapplied, no outstanding diffs.An independent Opus review confirmed correctness (no cross-request leak — the extension lives in per-request
http::Extensions, not shared service state; OHTTP unlinkability is fully preserved) and flagged two style/coverage suggestions, both addressed in this PR (map_or_elseinstead ofmap().unwrap_or_else(), and the extension-vs-header divergence test above).Generated by Claude Code