chore: trim old order and transaction events on the leader#1408
Conversation
Time Submission Status
Submit or update total time with: Add time on top of previous submission with: See available commands to help comply with our Guidelines. |
📝 WalkthroughWalkthroughThe migration redeclares scheduler trim actions as public, leader-authorized actions while retaining capped, height-based deletion. A new test verifies non-leader calls fail and current-leader calls succeed for both actions. ChangesScheduler trim authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/migrations/054-scheduler-trim-authorization.sql`:
- Around line 34-37: Update the cutoff<=0 early-return branch in
trim_order_events to emit the scheduler’s established parseable no-op NOTICE
format, matching trim_transaction_events, before returning. Preserve the
existing no-op behavior while ensuring the scheduler can parse the broadcast
without retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b0fe48b-3f56-425d-bc9a-300613e1d1f7
📒 Files selected for processing (2)
internal/migrations/054-scheduler-trim-authorization.sqltests/streams/digest/scheduler_trim_leader_authorization_test.go
| if $cutoff <= 0 { | ||
| NOTICE('trim_order_events: cutoff<=0, nothing to trim'); | ||
| RETURN; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Emit a parseable no-op NOTICE to prevent scheduler retries.
The comment in trim_transaction_events explicitly warns that an unrecognized NOTICE string will cause a parse failure in the scheduler's broadcast path, triggering needless retries. Since the same tn_digest scheduler invokes both actions, trim_order_events must also emit the parseable no-op format when short-circuiting here.
🐛 Proposed fix
if $cutoff <= 0 {
- NOTICE('trim_order_events: cutoff<=0, nothing to trim');
+ NOTICE('trim_order_events: deleted=0 remaining=0 has_more=false');
RETURN;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if $cutoff <= 0 { | |
| NOTICE('trim_order_events: cutoff<=0, nothing to trim'); | |
| RETURN; | |
| } | |
| if $cutoff <= 0 { | |
| NOTICE('trim_order_events: deleted=0 remaining=0 has_more=false'); | |
| RETURN; | |
| } |
🤖 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 `@internal/migrations/054-scheduler-trim-authorization.sql` around lines 34 -
37, Update the cutoff<=0 early-return branch in trim_order_events to emit the
scheduler’s established parseable no-op NOTICE format, matching
trim_transaction_events, before returning. Preserve the existing no-op behavior
while ensuring the scheduler can parse the broadcast without retries.
c8fb970 to
8c4a072
Compare
|
@holdex pr submit-time 3h |
1 participant
resolves: https://github.com/trufnetwork/trufscan/issues/184
What
Leader-gates
trim_order_events(migration 044) andtrim_transaction_events(migration 052) so the leader-onlytn_digestscheduler is authorized to run them.Why
Both actions were declared
PUBLIC owner, but the scheduler broadcasts them signed with the node's leader key, not the namespace owner. So every scheduler call was rejected withaction is owner-only, and the trims never ran. On mainnet,trim_order_eventshas 159 failed attempts and 0 successes since ~block 1.7M, soob_order_events(and, once live,transaction_events) grow unbounded on the node.auto_digest, fired by the same scheduler with the same key, succeeds because it gates oncheck_leader_authorization()(@signer == @leader_sender). These two trims should use that same gate.Change
trim_order_eventsandtrim_transaction_eventsnow dropownerand callcheck_leader_authorization()at the top of the body. Edited in place in their defining migrations (044/052) — actions areCREATE OR REPLACEandmigrate.shre-applies every file, so this re-applies idempotently on the next migrate. The trim logic is otherwise unchanged: deletion stays deterministic (block-height cutoff, never wall-clock) and delete-capped, so every validator removes the same rows on replay. No.prod.sqltwin; the auth is namespace-agnostic. It is aCREATE OR REPLACE ACTIONchange only (noALTER/DDL), so it is safe to apply against a live, settling node.Test
tests/streams/digest/scheduler_trim_leader_authorization_test.go: the current block leader is accepted and a non-leader is rejected (Only the current block leader), for both actions. Existing coverage still passes —TestTrimTransactionEvents+ E2E (data retention),TestDigestActionsLeaderAuthorization, and the scheduler-path broadcast integration test.