fix(data): unify checkpoint-only first-segment seed rule across verifier modes - #856
proerror77 wants to merge 1 commit into
Conversation
…ier modes The market-tape verifier rejected a replay-safe checkpoint-only first segment in trade-summaries mode even though LOB mode already accepts a replay-safe checkpoint as the replay state seed. This made manifest-valid segments fail strict verification as non-replayable and blocked the 5-minute correctness Shadow from starting. Add the missing `!require_trade_summaries` exemption so a replay-safe checkpoint establishes replay state in trade mode too, matching LOB mode. The replay_safe/synced/bridged checks immediately after still reject unsafe checkpoints. Add a regression test covering the checkpoint-only first segment.
📝 WalkthroughWalkthroughThe Binance market-tape verifier now accepts replay-safe checkpoint-only first segments when aggregate-trade summaries are required. A regression test verifies snapshot-plus-checkpoint replay events without snapshot or diff rows. ChangesTrade-summary replay verification
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eee33ce03
ℹ️ 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".
| } else { | ||
| if identities.is_empty() | ||
| && !snapshot_seeds.contains(&symbol) | ||
| && !require_trade_summaries |
There was a problem hiding this comment.
When binance-lob-archiver performs strict verification without --require-lob-continuity (binance-lob-archiver.rs:1165-1169), this clause now suppresses the only requirement that the first segment contain a snapshot merely because the independent aggregate-trade summary contract is present. A checkpoint-only segment can therefore self-assert replay_safe/synced/bridged without any stream-coverage or LOB-continuity evidence and still produce a verified replay book, potentially admitting unproven depth state into the correctness Shadow. Keep this exemption tied to LOB continuity or require equivalent depth-specific evidence rather than using require_trade_summaries.
AGENTS.md reference: AGENTS.md:L41-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs (1)
2274-2294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for unsafe checkpoint-only segments.
Add cases that set
replay_safe,synced, andbridgedtofalse. Assert thatverify_binance_market_tape_with_required_trade_summariesrejects each case. This protects the safety checks preserved by the production change.🤖 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 2274 - 2294, Add negative test coverage alongside first_segment_replay_safe_checkpoint_seeds_plain_trade_verifier by creating checkpoint-only segments with replay_safe, synced, and bridged each set to false in separate cases. Assert that verify_binance_market_tape_with_required_trade_summaries rejects every case, preserving the existing positive replay-safe test.
🤖 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 2274-2294: Add negative test coverage alongside
first_segment_replay_safe_checkpoint_seeds_plain_trade_verifier by creating
checkpoint-only segments with replay_safe, synced, and bridged each set to false
in separate cases. Assert that
verify_binance_market_tape_with_required_trade_summaries rejects every case,
preserving the existing positive replay-safe test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ccbdebb2-edc1-417e-972b-89d9687d85df
📒 Files selected for processing (1)
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs
|
Withdrawn. The Codex P1 review is correct: the |
Change
The market-tape verifier rejected a replay-safe checkpoint-only first segment in trade-summaries mode, even though LOB mode already accepts a replay-safe checkpoint as the replay state seed. This made manifest-valid segments fail strict verification as non-replayable and blocked the 5-minute correctness Shadow from starting.
Add the missing
!require_trade_summariesexemption so a replay-safe checkpoint establishes replay state in trade mode too, matching LOB mode. Thereplay_safe/synced/bridgedchecks immediately after still reject unsafe checkpoints. Add a regression test covering the checkpoint-only first segment.Issue relationship
Refs #846
Validation
cargo test -p hft-data --locked first_segment_replay_safe_checkpoint_seeds_plain_trade_verifier— 1 passedcargo test -p hft-data --locked— 86 passed, 0 failedRuntime impact and rollback
None. Research/data-pipeline verifier only; no runtime, execution, or risk path touched. Rollback = revert this commit.
Summary by CodeRabbit