perf: prepare oracle quote contexts concurrently - #2826
Conversation
How to use the Graphite Merge QueueAdd the label Raindex-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7a8d133 to
67a98bf
Compare
29be9b8 to
f663494
Compare
| if preparation.oracle_url.is_some() { | ||
| debug!( | ||
| raindex = %raindex, | ||
| input_index, | ||
| output_index, | ||
| "fetched quote oracle context" | ||
| ); |
There was a problem hiding this comment.
minor: the old code timed each oracle POST and put duration_ms on both the success debug and the failure warn. Those are gone, and neither the new debug nor the warn carries a duration or an endpoint, so the only timing left is the aggregate on the completed-fetches span, which is dominated by the slowest request in the batch. Given the change is motivated by production evidence of 49.6 requests and 9.7s per cache miss, losing per-request attribution removes exactly the signal that diagnosed the problem. I'd have fetch_signed_contexts return elapsed time per request, or emit a debug with host and duration inside the per-request future, and put duration_ms plus the endpoint host back on the per-pair lines.
| oracle_batch_request_count = 0, | ||
| "starting bounded quote oracle context fetches" | ||
| ); | ||
| let oracle_results: Vec<Result<SignedContextV1, String>> = if oracle_requests.is_empty() { |
There was a problem hiding this comment.
minor: before this change each get_order_quotes call awaited oracle POSTs serially, so it held at most one global permit at a time. Now one call runs its whole pair set through buffer_unordered(8) against the same eight-permit process-wide semaphore, so a call whose order meta points at a dead endpoint fills every permit with requests that each sit for the full 10s timeout, and refills them wave after wave at roughly 50 requests per page. Every other concurrent quote request in the process blocks on acquire(), which has no deadline and isn't covered by the reqwest timeout because the timeout only starts once the permit is held. Order meta is caller-supplied, so on the multi-tenant REST API this is one order degrading everyone. Capping in-flight requests per URL as well as globally, and putting a deadline around the permit acquisition so a stalled endpoint surfaces as a per-pair failure, would contain it.
| let mut oracle_results = oracle_results.into_iter(); | ||
|
|
||
| for preparation in pair_preparations { | ||
| let oracle_context = if preparation.oracle_url.is_some() { | ||
| oracle_results | ||
| .next() | ||
| .unwrap_or_else(|| Err("Missing oracle response slot".to_string())) | ||
| .map(|context| vec![context]) | ||
| } else { | ||
| Ok(vec![]) | ||
| }; |
There was a problem hiding this comment.
minor: the mapping from oracle responses back to pairs is positional by convention. The build loop pushes under if let Some(url) = &oracle_url and a separate later loop drains oracle_results.next() under preparation.oracle_url.is_some(), with nothing tying them together except both re-deriving the same predicate. It's correct today, but the unwrap_or_else(|| Err("Missing oracle response slot")) fallback only fires for trailing pairs after the iterator empties, so if a later change ever skips or dedupes a request in the build loop, every subsequent pair silently gets the previous pair's signed context and only the last one errors. That context goes on-chain as QuoteV2.signedContext, so a shift would attach an attestation to an order it was never issued for. Storing an oracle_request_index: Option<usize> on the preparation and indexing by it, plus a hard error on a length mismatch before the loop, makes the binding explicit and fails loudly.
| if !oracle_endpoint_counted { | ||
| oracle_urls.insert(url.clone()); | ||
| oracle_endpoint_counted = true; |
There was a problem hiding this comment.
minor: HashSet::insert is already idempotent, so this flag only saves a String clone, and oracle_url is loop-invariant across both inner loops anyway. Moving the insert up next to the extraction drops the flag entirely and keeps the inner loop about per-pair work. Worth noting it would then also count an endpoint for an order whose pairs are all self-trades and skipped, so if that distinction matters, deriving the count at log time from the prepared pairs avoids both the flag and the clones.
| oracle_request_count = oracle_fetch_count, | ||
| oracle_endpoint_count = oracle_urls.len(), | ||
| oracle_concurrency_limit = ORACLE_REQUEST_CONCURRENCY_LIMIT, | ||
| oracle_batch_request_count = 0, |
There was a problem hiding this comment.
minor: this is a literal, and per the deliberate constraints the batch encoder is never invoked from this path, so the field can't be anything but 0 while the code is in this shape. The description lists batching as one of the tracing goals, so someone building a dashboard on it would be measuring nothing and might conclude batching is broken. I'd drop the field until batch-endpoint support exists, or compute it from pairs whose metadata declares batch compatibility so at least the intent is visible in the expression.
| Err(error) => { | ||
| let error = error.to_string(); | ||
| (0..oracle_fetch_count) | ||
| .map(|_| Err(error.clone())) | ||
| .collect() |
There was a problem hiding this comment.
nit: the range index is discarded here, so this is vec!'s repeat form written as an iterator chain, and it clones the error once per element including the last. vec![Err(error); oracle_fetch_count] says the same thing and clones n-1 times.
| let response = routes | ||
| .iter() | ||
| .find_map(|(expected_body, response)| { | ||
| (expected_body == &body).then_some(response) | ||
| }) | ||
| .and_then(Option::as_ref); |
There was a problem hiding this comment.
nit: then_some yields Option<&Option<OracleResponse>> and the trailing and_then(Option::as_ref) exists only to flatten it, which collapses two different meanings of none: no route matched the body, and a route matched but is configured to fail. The test asserts on a 503, so that distinction is load-bearing. If encode_oracle_body ever changes encoding, an unmatched body falls through to the same 503 path and the test keeps passing for the wrong reason. Using find and borrowing the payload drops the nested Option, and naming the two outcomes (respond vs unavailable) with a panic on no match would make an encoding drift fail loudly.
f663494 to
eb53ceb
Compare
67a98bf to
832fcea
Compare
2d51004 to
4ae9e1a
Compare
Merge activity
|
## Why Quote-target construction currently awaits each oracle POST inside the nested order/input/output loops. Production evidence from the st0x REST API showed about 49.6 oracle requests and about 9.7 seconds of target-build latency per cache miss, while the subsequent batched chain quote usually took only 100–600 ms. ## What changed - prepare every valid pair and its established single-pair oracle request body in positional order - execute oracle POSTs through the shared process-wide client with a conservative global concurrency limit of 8 - restore results to their original request slots before composing quote targets - preserve oracle-before-injector signed-context order - isolate an oracle error to only its corresponding pair response - keep the chain quote as one `BatchQuoteTarget::do_quote` operation - add tracing for oracle request, endpoint, success/failure, concurrency-limit, batching, and duration data - add a deterministic end-to-end test that asserts exact oracle request bodies, partial failure isolation, exact ordered RPC multicall contents, signed-context composition, distinct RPC-result slot mapping, and a single chain RPC request ## Deliberate constraints - The existing batch encoder/API is not used automatically because order metadata exposes only an oracle URL and does not declare that the configured endpoint accepts the batch ABI body. - Requests remain individual POSTs and are bounded globally at 8; there is no unbounded fan-out. - No retry/backoff was added because the endpoint contract does not declare POST idempotency. Existing 429/503 status handling remains a per-pair failure. - Public response contracts, native/WASM support, timeout behavior, and chain RPC batching semantics are unchanged. ## Verification - `cargo test -p raindex_quote --lib` - `cargo clippy -p raindex_quote --all-targets --all-features -- -D warnings -D clippy::all` - `cargo fmt --all -- --check` - `nix develop .#wasm-shell -c bash -c 'CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner cargo test --target wasm32-unknown-unknown --lib -p raindex_quote'` - `nix develop .#wasm-shell -c rainix-rs-static` - `git diff --check` ## Stack Depends on #2825, which adds the reusable shared HTTP client and bounded ordered oracle-request primitive.
eb53ceb to
19bf215
Compare
4ae9e1a to
16049a3
Compare
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
## Dependent PRs - Raindex bounded oracle HTTP client (merge first): rainlanguage/raindex#2825 - Raindex batched oracle context preparation (merge second): rainlanguage/raindex#2826 ## Motivation The REST application's order-quote passes can require many oracle contexts at once. Issuing one HTTP request per context adds avoidable connection and request overhead even when the contexts share the same oracle endpoint. ## Solution - Bump `lib/rain.orderbook` from `0fa60a6` to `4ae9e1a02`. - Send one ABI batch request per exact oracle URL and scatter the returned contexts back to their quote pairs. - Preserve bounded concurrency across distinct oracle endpoints with a limit of 8. - Remove the previous per-context concurrent HTTP request path while leaving chain quote batching unchanged. ## Verification - `nix develop -c cargo check` - Upstream quote crate test suite — 63 passed - Upstream strict Clippy checks - Repository pre-commit hooks - `git diff --check` - Live local REST API validation against Base: - `/v2/swap/quote` returned HTTP 200 and `fullyFilled: true` for wtCOIN, wtNVDA, and wtMSTR. - Batch telemetry reduced 55 oracle contexts to 2 HTTP requests; 54 contexts succeeded and one legacy oracle endpoint returned HTTP 404. - A request-scoped quote recorded one oracle context in one batch request and completed successfully.
3 participants
Why
Quote-target construction currently awaits each oracle POST inside the nested order/input/output loops. Production evidence from the st0x REST API showed about 49.6 oracle requests and about 9.7 seconds of target-build latency per cache miss, while the subsequent batched chain quote usually took only 100–600 ms.
What changed
BatchQuoteTarget::do_quoteoperationDeliberate constraints
Verification
cargo test -p raindex_quote --libcargo clippy -p raindex_quote --all-targets --all-features -- -D warnings -D clippy::allcargo fmt --all -- --checknix develop .#wasm-shell -c bash -c 'CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner cargo test --target wasm32-unknown-unknown --lib -p raindex_quote'nix develop .#wasm-shell -c rainix-rs-staticgit diff --checkStack
Depends on #2825, which adds the reusable shared HTTP client and bounded ordered oracle-request primitive.