Repository navigation
perf(autobahn): reject an insert into a full producer mempool before CheckTx - #4451
Conversation
…CheckTx State.doInsertTx now takes the mempool lock once before CheckTx and returns ErrNotProducing, errMempoolFull, or errPendingFull when the admission loop would return that error at that moment. The admission loop after CheckTx is unchanged and stays authoritative. An insert that cannot be admitted or queued no longer pays for ecrecover and the sender cache write in CheckTx. Refs: PLT-1371, PLT-1366 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview
Behavior change: a tx that is invalid and sent while the pool is full now gets a retryable full/pending-full error instead of an immediate CheckTx rejection; tests pin that ordering. New Reviewed by Cursor Bugbot for commit 66ba3ee. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4451 +/- ##
=======================================
Coverage 56.75% 56.75%
=======================================
Files 2126 2126
Lines 166863 166871 +8
=======================================
+ Hits 94704 94714 +10
+ Misses 72154 72152 -2
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This change adds rejectBeforeCheckTx, which turns away an insert into a closed session, a full mempool (TryInsertTx) or a full pending queue (InsertTx) before the CheckTx permit wait and CheckTx itself. I checked each early rejection against the admission loop with no ticket, given maxPendingInserts() >= 1, and each returns the same error the loop would, so nothing blocks; codex's reading found nothing, which matches this one except for the error-priority note below. I reviewed against the PR's merge tree, but could not build or run the tests because the sandbox has no Go toolchain.
Non-blocking
1 finding on the changed lines, as inline comments.
seidroid review · decision approve · session db87b35dc278425eb175590acb20293b · turn resp_claude_50ba2e4ec87c2d4b55d1214833f9c6d8 · item e7f6622c9f225ac3a07201f0af3f9a5a
Findings: 0 blocking | 1 non-blocking | 1 posted inline
…ll mempool An invalid tx sent to a full mempool now gets the retryable errMempoolFull, because the capacity check runs before CheckTx. The test pins that order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
Since my last review only the tests changed: TestTryInsertTx_FullRejectsInvalidTxAsFull now checks that an undecodable tx sent to a full mempool gets errMempoolFull without CheckTx running. The PR description now says the errors are unchanged only for a valid tx and explains the new error order, so my one suggestion (already marked resolved) is addressed, mempool.go is unchanged and still correct, and nothing blocks. codex again found nothing, which matches this reading, and I reviewed against the PR's merge tree but still could not run the tests because there is no Go toolchain here.
seidroid review · decision approve · session db87b35dc278425eb175590acb20293b · turn resp_claude_64c5b63f473fbe43667c434185d4198c · item 17927a3f751a5438b9c010219536e73f
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Name the pre-CheckTx check after the admission decision it mirrors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
Since my last review the only change renames rejectBeforeCheckTx to poolCanAdmit and rewrites its doc comment. No old-name references are left in the tree, the logic and tests are unchanged, and my earlier error-order suggestion stays addressed, so nothing blocks. codex again found nothing, which matches this reading; I reviewed against the PR's merge tree but could not run the tests because there is no Go toolchain here.
seidroid review · decision approve · session db87b35dc278425eb175590acb20293b · turn resp_claude_3b2bd5f0df14e8a3993e98ffbf683bae · item 04d8fd6ee17a5ca1a749447b005be5fb
Findings: 0 blocking | 0 non-blocking | 0 posted inline
shemnon
left a comment
There was a problem hiding this comment.
The unit tests only check the failure modes, are the existing positive tests sufficient with this new gate?
Otherwise, LGTM
|
@shemnon awesome, thanks for taking a look. Triple checked and the existing positive checks will cover this well |
An insert into a full producer mempool ran the app CheckTx before it learned that the mempool had no room. CheckTx does ecrecover and writes the sender cache, so each such insert paid the full CheckTx cost and then failed. On a trapped giga-testnet-0 validator, about 4.4x more CheckTx attempts ran than inserts succeeded (
ok797/s,pending_full3,476/s). That wasted CPU keeps a slow validator behind under saturation load. Linear: PLT-1371 (from PLT-1366).State.doInsertTxnow callspoolCanAdmitafter it gets the session mempool and beforecheckTx. That function takes the mempool lock once. It returns the error that the admission loop would return at that moment for an insert without a ticket. That isErrNotProducingfor a closed session,errMempoolFullforTryInsertTxwhenIsFull(), anderrPendingFullforInsertTxwhenlen(waiters) >= maxPendingInserts(). The errors are the same values, soinsertTxcounts them under the sameproducer_inserts{result}labels. For a valid tx, RPC callers see the same errors as before. The admission loop after CheckTx is unchanged and stays authoritative. The check also runs before the CheckTx permit wait, so a rejected insert does not hold acheckTxSemslot or atryInsertSemslot for the CheckTx duration.Two points differ from the ticket text. The pending check omits
IsFull(): with no ticket andlen(waiters) >= max >= 1,isHeadis false, so the loop rejects witherrPendingFullwhether or not the mempool is full. The closed check comes first, because the loop also checksclosedfirst; without it, a closed and full session would reportfullwhere the loop reportsnot_producing.Error order. The order changes for a tx that is both invalid and sent to a full mempool. Before, it got its non-retryable CheckTx rejection, or
errTooLargefrom the gas checks. Now it gets the retryableerrMempoolFullorerrPendingFull. The client then retries a tx that can never land, and it learns that only when there is room. Under saturation this is the intended trade: CheckTx is the cost this change removes.TestTryInsertTx_FullRejectsInvalidTxAsFullpins the new order.Race analysis. Each early rejection is exactly the loop's answer at the moment of the check. After the check, other inserts can fill the mempool, and a prune can free it by advancing
m.first. Thus an insert that the check rejects could have succeeded only if capacity freed during its own permit wait and CheckTx. One permit wait plus one CheckTx bounds that window. In that case the caller gets the documented, retryable mempool-full error, the same answer as with an instantaneous CheckTx. Under saturation, I expect other in-flight inserts to take the freed capacity, so throughput does not drop.A closed session never reopens, so the closed check has no race. The happy path takes one more short mempool lock (three instead of two). The
admitandcheck_txlatency histograms get fewer samples, because early rejections skip both phases.Tests: three new tests in
checktx_limit_test.gouse acountingApp. They show thatTryInsertTxon a full mempool,InsertTxwithMaxPendingInsertsqueued, and both calls on a closed session return the expected result and do not call CheckTx. All three fail onmain. The existing producer tests pass with no change.🤖 Generated with Claude Code