perf(drive-abci): don't read every withdrawal document to build a debug log line - #4569
perf(drive-abci): don't read every withdrawal document to build a debug log line#4569PastaPastaPasta wants to merge 2 commits 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.
|
⛔ Final review complete — 1 blocking finding(s) (commit 05403a9) |
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team 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 now checks debug logging before fetching historical withdrawal documents. The status-counting branch relies on this earlier check. ChangesWithdrawal queue processing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change removes an unnecessary history-wide read from the normal no-withdrawal path while preserving withdrawal processing and externally visible behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4569 +/- ##
============================================
- Coverage 87.68% 85.09% -2.60%
============================================
Files 2778 2786 +8
Lines 359190 370311 +11121
============================================
+ Hits 314960 315101 +141
- Misses 44230 55210 +10980
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The intended default-path performance improvement is real, but the implementation modifies an active versioned block-execution method and makes execution success depend on node-local tracing configuration. The newly conditional DEBUG diagnostics also lack focused coverage.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-5.6-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-5.6-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer,gpt-5.6-sol— rust-quality (completed); agentphase2-reviewer
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs:58-64: Do not change active versioned block-execution behavior in place
`pool_withdrawals_into_transactions_queue_v1` is selected from protocol v5 onward, including v14 through `DRIVE_ABCI_METHOD_VERSIONS_V10`, while the v0 implementation can also delegate to it. This therefore changes behavior for already-defined protocol versions. More importantly, `fetch_oldest_withdrawal_documents` calls the fallible `query_documents` path and previously propagated its error unconditionally; after this guard, the same block and state can return `Ok(())` on a node without DEBUG tracing while a DEBUG-enabled node or older binary still executes the query and can fail. The successful app-hash replay confirms the normal path but does not cover this changed failure behavior. Keep v0/v1 frozen, introduce a v2 implementation selected at a protocol activation boundary, and make its purely diagnostic query non-fatal so tracing configuration cannot affect block-execution success.
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs:58-70: Exercise the DEBUG-enabled diagnostic path
The existing no-queued-documents test installs no DEBUG subscriber, so it now returns at line 59; `test_pooling` has queued documents and never enters this branch. Consequently, no test executes the guarded historical fetch or the status-grouping branch, matching Codecov's seven uncovered changed lines. Add a focused test using a scoped DEBUG subscriber and at least one non-queued withdrawal document, then assert that the call succeeds and leaves the document unchanged. This covers both the enabled guard and the grouped-status diagnostic path.
| if !tracing::enabled!(tracing::Level::DEBUG) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let all_documents = self | ||
| .drive | ||
| .fetch_oldest_withdrawal_documents(transaction, platform_version)?; |
There was a problem hiding this comment.
🔴 Blocking: Do not change active versioned block-execution behavior in place
pool_withdrawals_into_transactions_queue_v1 is selected from protocol v5 onward, including v14 through DRIVE_ABCI_METHOD_VERSIONS_V10, while the v0 implementation can also delegate to it. This therefore changes behavior for already-defined protocol versions. More importantly, fetch_oldest_withdrawal_documents calls the fallible query_documents path and previously propagated its error unconditionally; after this guard, the same block and state can return Ok(()) on a node without DEBUG tracing while a DEBUG-enabled node or older binary still executes the query and can fail. The successful app-hash replay confirms the normal path but does not cover this changed failure behavior. Keep v0/v1 frozen, introduce a v2 implementation selected at a protocol activation boundary, and make its purely diagnostic query non-fatal so tracing configuration cannot affect block-execution success.
source: ['claude']
| @@ -57,7 +67,7 @@ where | |||
| height = block_info.height, | |||
| "No withdrawal documents found at all" | |||
| ); | |||
| } else if tracing::enabled!(tracing::Level::DEBUG) { | |||
| } else { | |||
There was a problem hiding this comment.
🟡 Suggestion: Exercise the DEBUG-enabled diagnostic path
The existing no-queued-documents test installs no DEBUG subscriber, so it now returns at line 59; test_pooling has queued documents and never enters this branch. Consequently, no test executes the guarded historical fetch or the status-grouping branch, matching Codecov's seven uncovered changed lines. Add a focused test using a scoped DEBUG subscriber and at least one non-queued withdrawal document, then assert that the call succeeds and leaves the document unchanged. This covers both the enabled guard and the grouped-status diagnostic path.
source: ['codex']
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Review
Verdict: merge after two small changes (make the diagnostic fetch non-fatal; cover the DEBUG path with a test so codecov/patch passes).
1. Correctness
The guard is right. fetch_oldest_withdrawal_documents passes limit: None, deserializes every withdrawal document ever written, and the result only feeds a tracing::debug! line. Returning before it when DEBUG is off changes no state; the 425k-block app-hash match confirms that.
One thing the guard does change: the fetch used ?, so a Drive error inside it failed the block. Now a node with DEBUG off returns Ok(()) while a node with DEBUG on still fails the block. The error itself would mean a broken database, so this is theoretical, but tracing configuration should never decide whether a block succeeds. Since the query is purely diagnostic, log the error and continue instead of propagating it. That also resolves thepastaclaw's blocking comment without a new method version: the change is diagnostic-only, touches neither state nor app hash, and does not need protocol activation.
2. Clarity
Title and description are clear. The cost-by-height table makes the case. The code comment explains why, which is what a reader needs.
3. Codebase standards
Follows the tracing::enabled! pattern already used a few lines below. Conventional-commit title with the right scope. No test covers the new early return or the DEBUG branch, which is why codecov/patch fails.
4. Importance and alternatives
4.8 ms of a ~7 ms block at height 200k, still growing. There is no simpler fix; deleting the diagnostic entirely would also work but loses a useful log line.
5. Existing bot findings
- thepastaclaw "do not change versioned behavior in place": disagree on needing a v2 (nothing consensus-relevant changes); agree on making the diagnostic non-fatal.
- thepastaclaw "exercise the DEBUG path": agree; a test with a scoped DEBUG subscriber and one non-queued document covers both branches.
I will push both changes to this branch.
🤖 Posted autonomously by Claude on behalf of pasta.
The summary only runs when DEBUG logging is on, so a Drive error inside it would fail the block on a node with verbose logging and pass on one without. Log the error and carry on instead. Two tests with a scoped DEBUG subscriber cover the summary with and without history.
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