perf(drive-abci): don't read every withdrawal document to build a debug log line - #4550
perf(drive-abci): don't read every withdrawal document to build a debug log line#4550PastaPastaPasta wants to merge 1 commit into
Conversation
…ug log line pool_withdrawals_into_transactions_queue_v1 fetches every withdrawal document the chain has ever produced, groups them by status and sorts each group, on any block with nothing queued — nearly every block — and then throws the result away unless debug logging is on. fetch_oldest_withdrawal_documents passes limit: None, and withdrawal documents are never removed, so the cost grows with chain history: measured at 4.8 ms per block by height 200,000 on mainnet and still climbing, against about 7 ms for everything else in a block put together. Do the work only when the line it feeds will be emitted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe withdrawal queue handler now avoids counting withdrawal documents when no documents are queued and DEBUG tracing is disabled. The debug status-counting branch now relies on the earlier tracing guard. ChangesWithdrawal queue processing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change removes unnecessary historical withdrawal-document reads when debug logging is disabled while preserving withdrawal queue behavior and application state. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
🕓 Ready for review — 12 ahead in queue (commit 05403a9) |
|
Superseded by #4569, which is the same change from a branch in 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
pool_withdrawals_into_transactions_queue_v1reads every withdrawal document the chain has ever produced, on nearly every block, and throws the result away.When nothing is queued — which is almost always — it calls
fetch_oldest_withdrawal_documents, which passeslimit: None. That deserializes every withdrawal document, groups them by status and sorts each group. The result feeds onetracing::debug!line and is otherwise discarded.Withdrawal documents are never removed, so the cost grows with chain history. Replaying mainnet with per-block phase timing:
Against roughly 7,000 µs for everything else in a block put together, and still climbing at 200k. Every other per-block phase is flat with depth; this one is the entire slope.
What was done?
Return before the diagnostic query when a
DEBUGline would not be emitted. The counting block below it was already guarded bytracing::enabled!; the expensive fetch that feeds it was not.How Has This Been Tested?
Replaying mainnet history from a local peer, genesis to 424,981, comparing against
v4.2-dev. Every committed app hash matched between the two runs across all 424,971 heights, and the final app hash matched an independent reference sync.cargo test -p drive-abci --lib withdrawal— 176 passed.Breaking Changes
None. The skipped work only ever fed a log line that is off by default.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit