Isolate stale USD-M raw trades - #851
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds stale raw-trade parsing and auditing. The collector archives delayed trades, resumes sequence validation, and marks affected symbols incomplete. Segment manifests persist these symbols, and artifact verification validates their declarations and stale-row metadata. ChangesStale raw-trade auditing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BinanceStream
participant event_from_frame_for_shard
participant Segment
participant ManifestVerifier
BinanceStream->>event_from_frame_for_shard: provide delayed raw-trade frame
event_from_frame_for_shard->>Segment: emit StaleRawTrade audit event
Segment->>Segment: record incomplete symbol and archive row
Segment->>ManifestVerifier: provide finalized manifest and stale row
ManifestVerifier->>ManifestVerifier: validate row and symbol declaration
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
26e2246 to
813e317
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs (1)
2547-2579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tamper-case coverage for the new
stale_raw_tradearm.The two new tests cover the manifest declaration contract in both directions. The row-level integrity checks added at lines 661-733 have no direct coverage.
The uncovered checks are the non-forgeability guarantees of the audit row: row symbol versus frame symbol, row stream versus frame stream, row
E/Tversus frame clocks, the three delta values, and theproducer_idshard bound. A row that fakes a smallerrecv_minus_event_mswould currently be caught by line 708, but no test pins that behavior.The existing
stale_book_ticker_producer_id_must_be_declaredtest at line 2615 shows the pattern. Each case is a one-field mutation ofstale_raw_trade_row.🧪 Example: pin the delta-tampering rejection
#[test] fn stale_raw_trade_clock_audit_must_match_its_frame() { let root = tempdir(); let mut rows = valid_v2_rows(); let mut stale = stale_raw_trade_row(START_NS + 330_000_000); stale["recv_minus_event_ms"] = json!(30_000); rows.insert(5, stale); let rows = with_stream_coverage_v2(rows, &["BTCUSDT"], &V2_STREAM_TYPES); let (triplet, anchor) = write_triplet_v2(root.path(), &rows, &["BTCUSDT"], &V2_STREAM_TYPES); let error = verify_binance_market_tape(vec![ seal_binance_market_tape_triplet(&triplet, &anchor).unwrap(), ]) .unwrap_err(); assert!(error .to_string() .contains("stale raw trade clock audit does not match its frame")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs` around lines 2547 - 2579, Add direct tamper-case tests for the row-level checks of stale_raw_trade validation, following stale_book_ticker_producer_id_must_be_declared. Mutate one field at a time in stale_raw_trade_row to cover symbol, stream, E/T clocks, all three delta values, and producer_id shard-bound mismatches, then assert verification rejects each mutation with the corresponding audit-mismatch error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs`:
- Around line 2547-2579: Add direct tamper-case tests for the row-level checks
of stale_raw_trade validation, following
stale_book_ticker_producer_id_must_be_declared. Mutate one field at a time in
stale_raw_trade_row to cover symbol, stream, E/T clocks, all three delta values,
and producer_id shard-bound mismatches, then assert verification rejects each
mutation with the corresponding audit-mismatch error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5c188a5-f3a3-4cd1-b758-17a6d6405963
📒 Files selected for processing (4)
rust_hft/data-pipelines/core/src/binance_market_tape.rsrust_hft/data-pipelines/core/src/binance_market_tape_artifact.rsrust_hft/tools/collector/src/bin/binance-lob-archiver.rsrust_hft/tools/collector/src/lob_archiver.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26e224651b
ℹ️ 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".
813e317 to
151692a
Compare
Summary
received_at-Eexceeds 30s into an archived audit event without restarting the websocket shardValidation
cargo test -p hft-data --lib --locked(85 passed)cargo test -p hft-collector --bin binance-lob-archiver --locked(98 passed; local macOS shim temporarily mapsRUSAGE_THREADtoRUSAGE_SELF, restored byte-exact)cargo clippy --target x86_64-unknown-linux-gnu -p hft-data --lib --locked -- -D warningscargo clippy --target x86_64-unknown-linux-gnu -p hft-collector --bin binance-lob-archiver --locked -- -D warningsgit diff --checkBase:
aa69b52e70d9bc63a9be758d5fd42d9e078775a4Head:
26e224651b9d5051c2d145cb6e68d1a4350b9cbaSummary by CodeRabbit
New Features
Bug Fixes