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 3 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.
|
🕓 Queued for automated review — 57th in line, estimated start in ~80 h (commit cf5e8e4)
|
|
Warning Review limit reachedNext included review available in 59 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.62% -2.07%
============================================
Files 2778 2792 +14
Lines 359190 370594 +11404
============================================
+ Hits 314960 317316 +2356
- Misses 44230 53278 +9048
🚀 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']
There was a problem hiding this comment.
Partially addressed in efd398de: the diagnostic fetch is no longer fatal. A Drive error in fetch_oldest_withdrawal_documents is logged at DEBUG and the method returns Ok(()), so block outcome no longer depends on the log level. Verified on head cf5e8e4a.
The question of whether this needs a v2 method version rather than an in-place edit to pool_withdrawals_into_transactions_queue_v1 is left for the maintainer to decide; this thread stays open for that.
🤖 Posted autonomously by Claude on behalf of pasta.
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.
|
Pushed efd398d on top of the original commit:
Ready for human review. 🤖 Posted autonomously by Claude on behalf of pasta. |
The one with a document already covers the guarded path; the empty case adds nothing.
|
Two updates from review feedback:
🤖 Posted autonomously by Claude on behalf of pasta. |
PastaPastaPasta
added
the
ready for final review
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 (#4573), where "block time" below means the time drive-abci spends executing a block, proposal and finalize together, not the 2.5-minute interval between blocks at the tip:
Every other measured phase adds up to roughly 7,000 µs per block and stays flat with height; this call is the entire slope, and it was still climbing at 200k. Removing it makes a block at 200k about 1.7× faster to execute. A synced node does not notice, since 5 ms is nothing against a 150-second block interval; a node replaying history spends 41% of its time on it.
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