feat(research): verify real lifecycle latency evidence - #784
proerror77 merged 28 commits into
Conversation
…ntract-v2 # Conflicts: # agent-worktree.yml
|
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: defaults 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d748efc885
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| observations.push(( | ||
| event.observed_at, | ||
| metric_u64(&event.metrics, "intent_to_private_report_us")?, | ||
| metric_nonnegative(&event.metrics, "realized_slippage_bps")?, | ||
| )); |
There was a problem hiding this comment.
Deduplicate signed events before calculating percentiles
When an anchored JSONL contains the same valid signed event more than once—for example after retrying or concatenating logs—each copy is appended as a separate observation even though event_id identifies one runtime event. This inflates observations and reweights every reported percentile, allowing duplicated evidence to misrepresent the completeness and measured costs; reject conflicting duplicates or count each event ID once.
AGENTS.md reference: AGENTS.md:L65-L66
Useful? React with 👍 / 👎.
| || event.kind != AttributionKind::Fill | ||
| || event.outcome != AttributionOutcome::Healthy | ||
| || event.deployment_id != deployment_id | ||
| || event.venue.as_deref() != Some("binance") |
There was a problem hiding this comment.
Compare the Binance venue case-insensitively
A valid deployment can spell the venue as Binance: deployment validation only requires nonempty text, live activation matches configured venues case-insensitively, and attribution copies the envelope's original casing into the signed event. In that supported case this exact comparison silently discards every otherwise valid lifecycle observation and returns “no observations”; normalize the venue or use the same case-insensitive comparison as live intake.
Useful? React with 👍 / 👎.
| mode, | ||
| outcome: AttributionOutcome::Healthy, | ||
| kind: AttributionKind::Fill, | ||
| strategy_id: None, |
There was a problem hiding this comment.
Provide the required strategy ID in fill fixtures
Both added tests call write_fixture, but this fill event sets strategy_id to None. sign_runtime_attribution_event validates the event first, and the domain contract requires fill events to include strategy, order, account, venue, and symbol scope, so the unwrap() while signing always panics before either verifier assertion runs; populate a valid strategy ID so the focused tests exercise the new code.
Useful? React with 👍 / 👎.
| || event.deployment_id != deployment_id | ||
| || event.venue.as_deref() != Some("binance") |
There was a problem hiding this comment.
Bind lifecycle evidence to the snapshot market
When building a USDM snapshot, this filter cannot distinguish a BTCUSDT spot fill from a BTCUSDT futures fill because neither the verifier arguments nor the accepted event scope includes an instrument type. CexReplaySnapshotV2 supports both spot and usdm, while the current Binance live execution adapter explicitly rejects futures orders, so passing an existing spot deployment here can incorrectly supply spot latency and slippage as USDM evidence instead of failing for missing real derivatives observations.
AGENTS.md reference: AGENTS.md:L65-L66
Useful? React with 👍 / 👎.
…into codex/lob-pit-trade-features
…to codex/runtime-latency-evidence # Conflicts: # agent-worktree.yml
…eatures # Conflicts: # agent-worktree.yml # rust_hft/research-core/manifest/src/lib.rs
…to codex/runtime-latency-evidence # Conflicts: # agent-worktree.yml
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42dcef4ffd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || event.kind != AttributionKind::Fill | ||
| || event.outcome != AttributionOutcome::Healthy | ||
| || event.deployment_id != deployment_id | ||
| || event.account_id.as_deref() != Some(account_fingerprint) |
There was a problem hiding this comment.
Do not compare account IDs directly with fee fingerprints
When this verifier consumes real live output, this predicate filters every fill: order_attribution writes ActivationRequest.account_id into event.account_id (apps/live/src/runtime_attribution.rs:752), and configured IDs are human-readable values such as binance_main, whereas binance-fee-snapshot derives account_fingerprint as SHA-256 of the API key (tools/collector/src/bin/binance-fee-snapshot.rs:65). These are distinct identity domains, so valid evidence ends with “no observations”; pass the expected runtime account ID separately and bind it to the credential fingerprint at the credential/configuration boundary instead of equating the two.
Useful? React with 👍 / 👎.
| .symbol | ||
| .as_deref() | ||
| .is_some_and(|value| value.eq_ignore_ascii_case(symbol)) | ||
| || event.observed_at > available_before |
There was a problem hiding this comment.
Gate latency evidence on its availability time
When a private fill report is delayed or replayed across the snapshot cutoff, the event is created and signed after the cutoff but carries the earlier venue fill timestamp because runtime_attribution.rs:462 assigns observed_at from Fill.timestamp; the lifecycle timing event's local receipt time is not retained in the signed fill. Comparing available_before only with this venue timestamp therefore admits latency and slippage that were not knowable at snapshot time, introducing lookahead into PIT research; preserve and verify a local evidence-availability or publication timestamp and gate on that instead.
AGENTS.md reference: AGENTS.md:L65-L66
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81106840f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let arrival_price = matches!(envelope.intent.order_type, OrderType::Market) | ||
| .then_some(envelope.intent.price) | ||
| .flatten(); |
There was a problem hiding this comment.
Capture arrival prices for live IOC limit orders
Capture the executable arrival quote for marketable limit orders too, rather than only for OrderType::Market. Both currently activatable live strategies submit Binance orders as IOC limits (strategies/formula/src/lib.rs:163-169 and strategy-dl/src/onnx_lob_strategy.rs:618-625), so their OrderNew events carry arrival_price: None; every resulting fill then lacks arrival_slippage_bps, and the verifier fails at runtime_latency_evidence.rs:117 instead of producing any real lifecycle observations. The added test masks this production path by directly constructing an event with an arrival price.
AGENTS.md reference: AGENTS.md:L65-L66
Useful? React with 👍 / 👎.
| trusted_keys_path: &Path, | ||
| trusted_keys_sha256: &str, | ||
| deployment_id: &str, | ||
| market: &str, |
There was a problem hiding this comment.
Bind the logical account to the fee credential fingerprint
Require and verify a trusted mapping between account_id and the fee snapshot's credential fingerprint. At exact head 8110684, the fresh interface change now matches the signed event against the logical ID but removes the fingerprint entirely, and VerifiedRuntimeLatencyEvidence does not return either identity; a caller can therefore verify fills from Binance account A and attach them to the fee schedule from account B even though CexReplaySnapshotV2::validate treats the latency-cost fingerprint as the fee account identity (research-core/manifest/src/lib.rs:273-277). This is the unresolved half of the earlier account-domain comment, now evidenced by the fingerprint-free verifier API.
AGENTS.md reference: AGENTS.md:L67-L68
Useful? React with 👍 / 👎.
| let digest = Sha256::digest(line).to_vec(); | ||
| if let Some(previous) = event_digests.insert(event.event_id.clone(), digest.clone()) { | ||
| if previous != digest { | ||
| bail!("runtime feedback contains conflicting duplicate event IDs"); |
There was a problem hiding this comment.
Compare duplicate signed records by canonical identity
Deduplicate using the verified signed content rather than the raw JSONL bytes. At exact head 8110684, the new raw-line digest treats two valid serializations of the same SignedRuntimeAttributionEvent—for example compact versus pretty-printed JSON with the identical signed event, content_hash, key, and signature—as conflicting and aborts verification; the new test only concatenates byte-identical lines, so it does not cover this ordinary reserialization case. This is fresh evidence beyond the earlier duplicate-weighting comment: duplicates are no longer reweighted, but logically identical signed duplicates can now make an anchored log unusable.
Useful? React with 👍 / 👎.
| - rust_hft/tools/collector/src/runtime_latency_evidence.rs | ||
| - rust_hft/apps/live/src/runtime_attribution.rs | ||
| - rust_hft/execution-gateway/adapters/adapter-backpack/src/lib.rs | ||
| - rust_hft/market-core/engine/src/execution_worker.rs |
There was a problem hiding this comment.
Split runtime instrumentation from research verification
Split this contract into independently testable and rollbackable runtime-instrumentation and research-verification changes. The declared scope combines the collector verifier with live attribution, execution-adapter, and engine event changes, so rolling back a faulty research percentile or duplicate policy also rolls back runtime event production, while deploying the runtime schema cannot be reviewed or released independently of its research consumer; this directly mixes the repository's Research and Runtime authority boundaries in one change.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
…-evidence # Conflicts: # agent-worktree.yml
…ation' into codex/runtime-latency-evidence # Conflicts: # agent-worktree.yml # rust_hft/market-core/engine/src/execution_worker.rs # rust_hft/market-core/ports/src/events.rs
26890d6
into
codex/runtime-arrival-instrumentation
💡 Codex Reviewmonday/rust_hft/apps/live/src/runtime_attribution.rs Lines 473 to 474 in d38c7a4 When AGENTS.md reference: AGENTS.md:L35-L36 monday/rust_hft/tools/collector/src/runtime_latency_evidence.rs Lines 90 to 92 in d38c7a4 When the attribution receiver lags before the snapshot cutoff, AGENTS.md reference: AGENTS.md:L65-L66 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This reverts commit 26890d6.
* feat(runtime): emit signed arrival-cost evidence * feat(research): verify real lifecycle latency evidence (#784) * feat(research): define CEX replay snapshot V2 * fix(research): bind V2 snapshot evidence timelines * fix(research): keep V1 writer until consumers migrate * fix(research): harden V2 PIT evidence bounds * fix(research): bind complete PIT evidence sets * fix(research): close V2 evidence coverage gaps * fix(research): bind fee evidence to account * feat(research): materialize aggregate trade flow * feat(research): verify real lifecycle latency evidence * fix(research): bind V2 series identity * chore: refresh trade feature stack base * chore: refresh trade feature stack base * fix(research): bind latency evidence to fee account * refactor(research): pass prebuilt source lineage * chore: refresh latency evidence stack base * test(research): keep aggregate trade ids contiguous * chore: refresh latency evidence stack base * fix(research): bind real arrival latency evidence * fix(research): authenticate runtime instrument market --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com> * Revert "feat(research): verify real lifecycle latency evidence (#784)" This reverts commit 26890d6. * feat: identify Binance futures fill evidence * fix(runtime): preserve genuine arrival evidence * fix(runtime): satisfy strict arrival lint * fix(runtime): bind arrival evidence to engine quotes * fix(engine): preserve non-market lifecycle evidence * fix(runtime): tombstone conflicting order identities * test(runtime): cover conflicting arrival tombstones * fix(runtime): bind executable arrival quotes * fix(runtime): require current arrival quotes --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
* feat(research): define CEX replay snapshot V2 * fix(research): bind V2 snapshot evidence timelines * fix(research): keep V1 writer until consumers migrate * fix(research): harden V2 PIT evidence bounds * fix(research): bind complete PIT evidence sets * fix(research): close V2 evidence coverage gaps * fix(research): bind fee evidence to account * feat(research): materialize aggregate trade flow * feat(research): verify real lifecycle latency evidence * fix(research): bind V2 series identity * chore: refresh trade feature stack base * chore: refresh trade feature stack base * fix(research): bind latency evidence to fee account * refactor(research): pass prebuilt source lineage * chore: refresh latency evidence stack base * test(research): keep aggregate trade ids contiguous * chore: refresh latency evidence stack base * fix(research): bind real arrival latency evidence * fix(research): authenticate runtime instrument market * feat(runtime): emit signed arrival-cost evidence * feat(research): verify real lifecycle latency evidence (#784) * feat(research): define CEX replay snapshot V2 * fix(research): bind V2 snapshot evidence timelines * fix(research): keep V1 writer until consumers migrate * fix(research): harden V2 PIT evidence bounds * fix(research): bind complete PIT evidence sets * fix(research): close V2 evidence coverage gaps * fix(research): bind fee evidence to account * feat(research): materialize aggregate trade flow * feat(research): verify real lifecycle latency evidence * fix(research): bind V2 series identity * chore: refresh trade feature stack base * chore: refresh trade feature stack base * fix(research): bind latency evidence to fee account * refactor(research): pass prebuilt source lineage * chore: refresh latency evidence stack base * test(research): keep aggregate trade ids contiguous * chore: refresh latency evidence stack base * fix(research): bind real arrival latency evidence * fix(research): authenticate runtime instrument market --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com> * fix(research): canonicalize latency evidence selection * Revert "feat(research): verify real lifecycle latency evidence (#784)" This reverts commit 26890d6. * fix(research): retain verified runtime account identity * feat: identify Binance futures fill evidence * fix: enforce runtime evidence time bounds * fix: admit canonical Binance market venues * fix: stream bounded runtime evidence * fix(runtime): preserve genuine arrival evidence * fix(research): bound selected latency evidence * fix(runtime): satisfy strict arrival lint * fix(runtime): bind arrival evidence to engine quotes * fix(engine): preserve non-market lifecycle evidence * fix(runtime): tombstone conflicting order identities * test(runtime): cover conflicting arrival tombstones * fix(runtime): bind executable arrival quotes * fix(research): reject unavailable USDM latency evidence * fix(runtime): require current arrival quotes * fix(research): harden runtime latency verification * refactor(research): bind latency trust source --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
Change contract
Verify signed, pre-snapshot Binance LiveSmall fill lifecycle observations and derive deterministic latency and slippage percentiles for ResearchSnapshot V2.
Issue relationship
Refs #779
Out of scope
Materializer publication and fee, funding, and OI binding remain in the next stacked rollout unit. Paper and Shadow observations are deliberately rejected as non-real costs.
Dependencies and merge order
Depends on #783, which depends on #781. Merge in that order, then retarget each next PR to main.
Focused validation
Rollout and rollback
No runtime mutation and no order authority. Rollback is the single commit.
Scope exception
None.