Repository navigation
Conversation
CheckTx sets ResponseCheckTx.Priority to the effective gas price in wei, clamped to int64. GetTxPriorityHint returns the same value from the RLP decode alone, without sender recovery, so a mempool can rank a tx before it pays for CheckTx. Refs: PLT-1371 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ority Blocked InsertTx calls are admitted by rank: senders of a shard this validator owns first, then higher CheckTx priority, then arrival order. The calls of one EVM sender are admitted in nonce order, so a later nonce never overtakes its predecessor and fails errBadNonce. - Ownership comes from the NextCommitEpoch committee that EvmProxy routes by. When the local validator is not in it, every tx counts as owned. - The queue keeps two indexed heaps over the eligible calls. Waking the best call and finding the worst one are both O(log n). Only the best call is signalled, as before. - On a full queue, a call that outranks the lowest-ranked eligible call evicts the last queued call of that call's sender. The evicted InsertTx returns errPendingFull. A new counter, tendermint_internal_autobahn_producer_evictions, counts evictions. - poolCanAdmit ranks a tx by the app's GetTxPriorityHint before CheckTx. The post-CheckTx loop uses the CheckTx priority. With equal priorities and one owner, admission stays FIFO. 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 At Reviewed by Cursor Bugbot for commit c5c1fa9. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4452 +/- ##
==========================================
+ Coverage 56.77% 56.78% +0.01%
==========================================
Files 2126 2127 +1
Lines 166876 167050 +174
==========================================
+ Hits 94737 94861 +124
- Misses 72134 72184 +50
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 replaces the FIFO admission queue for blocked InsertTx calls with a ranked queue: senders of a shard this validator owns go first, then higher priority, then earlier arrival. Calls from one sender stay in nonce order, the worst call is evicted when the queue is full, and a priority hint lets a call that would lose be rejected before CheckTx. Against the PR's merge tree (8e3a455), the heap, nonce-chain and eviction bookkeeping and the wakeup paths hold up, so nothing blocks; the notes below are about cost and a doc mismatch, and the only other reader (codex) returned nothing, so there were no outside findings to keep or drop.
Non-blocking
1 finding on the changed lines, as inline comments.
- Ownership is the first sort key and calls never age, so under steady saturation fallback (non-owned) inserts can wait forever and then be evicted, however much they pay. When
EnableEvmProxyis false these inserts have no other route to a lane, so for operators who turn the proxy off, roughly (n-1)/n of senders are penalised by address hash alone. The description already flags this. It is worth gating the ownership key on the proxy flag, or adding a bounded aging term, before this ships to configurations with the proxy off.
1 nit, not posted on the code
sei-tendermint/internal/autobahn/producer/mempool.go:233— The doc says an evicted or outranked call fails with errMempoolFull, butenqueueand the evicted check both return errPendingFull.
seidroid review · decision approve · session d024a7480b524ef1a01f7db4e8c8edea · turn resp_claude_82a82dbdc81452da18695bb1a082217a · item aeb68d22440a50d195c8a9f02ce1881b
Findings: 0 blocking | 2 non-blocking | 1 posted inline
…h the EVM proxy The pre-CheckTx priority hint now holds a CheckTx permit, since an app may recover the sender to compute it. Shard ownership ranks admission only when RankByShardOwnership is set, which the node sets from enable_evm_proxy; without the proxy a non-owned sender has no other lane. The InsertTx doc names errPendingFull for an evicted or outranked call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
Since the last review, the pre-CheckTx priority hint holds a CheckTx permit, ownership only affects ranking when RankByShardOwnership is set (the node takes it from enable_evm_proxy), and the errPendingFull doc is corrected; I checked each against the diff and all three earlier points are addressed. Nothing blocks: I kept codex's one finding as a suggestion, because I confirmed that a same-nonce or gapped call can evict a usable one, but it gets no further than the free queue-filling the old FIFO already allowed; codex was the only reader and raised nothing else.
Non-blocking
1 finding on the changed lines, as inline comments.
seidroid review · decision approve · session d024a7480b524ef1a01f7db4e8c8edea · turn resp_claude_f481e5420d248722696df4f9b8880cc4 · item e508a4947d49519ebbe06d758b3e443e
Findings: 0 blocking | 1 non-blocking | 1 posted inline
| Global.capacityWaitWakeups, | ||
| Global.inFlight, | ||
| Global.inserts, | ||
| Global.evictions, |
There was a problem hiding this comment.
For metrics, can we use the otel telemetry setup instead of this legacy metric generation? Let's not do this if it adds a bunch of scope but please create a linear ticket for me to do this in that case
There was a problem hiding this comment.
Tracked in PLT-1375 (https://linear.app/seilabs/issue/PLT-1375). Moving only this counter would split the producer package across two metric systems. All five Autobahn metrics packages use metricsgen, and the giga-testnet alerts and dashboards read their series names, so the move is a package-by-package change that keeps every name, label and bucket. internal/mempool/metrics.go is the OTel pattern to follow.
| } | ||
|
|
||
| // unqueuedSeq is the seq of a call that is not queued yet, so it loses every tie to a queued one. | ||
| const unqueuedSeq = math.MaxUint64 |
There was a problem hiding this comment.
Constants should be at the top of files please.
There was a problem hiding this comment.
Done in 3b3327b. Package-level consts now sit directly after the imports, with their doc comments, in every file this PR changes: admission.go, state.go and evmonlyapp/app.go.
A call that repeats a nonce already queued for its sender fails with errBadNonce before it joins the admission queue. A queued call is never replaced. Only a call that keeps its sender's queued nonces contiguous may evict at the bound: the sender has nothing queued, or the nonce directly follows its last queued call or directly precedes its first. A call that leaves a gap still queues while there is room. At the bound it fails with errPendingFull. The pre-CheckTx check cannot see the sender, so these calls are rejected after CheckTx. Package-level consts move to the top of each file this PR changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
The eligible calls sit in one google/btree ordered best-first by rank. Min is the call to wake and Max is the eviction candidate. It replaces the two indexed container/heap heaps and their per-ticket index fields. Every queued call has a unique seq, so no two compare equal. Inserting a call that is already in the tree, or deleting one that is missing, panics as a broken invariant. Admission order and behaviour are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Since the last review, a call that repeats a nonce already queued for its sender is rejected with errBadNonce, and eviction is limited to calls that Fit classifies as contiguous. I confirmed the duplicate half of my earlier finding is fixed, but codex's one new finding holds and I kept it: Fit only compares against the ends of the queue, never the sender's expected next nonce, so a call that is certain to fail can still evict. That matches the weight of my earlier note, so it stays a suggestion and the verdict stays approve; codex was the only reader and raised nothing else.
Non-blocking
1 finding on the changed lines, as inline comments.
seidroid review · decision approve · session d024a7480b524ef1a01f7db4e8c8edea · turn resp_claude_e38f05681753a1e8cd5c82664bfb0376 · item 5c4364a45b59528a99e416f87c95f039
Findings: 0 blocking | 1 non-blocking | 1 posted inline
|
@seidroid review |
There was a problem hiding this comment.
Since the last review, the two heaps of eligible calls were replaced by one google/btree ordered by rank. Every queued call gets a unique seq, so no two compare equal, and the module already depends directly on google/btree. The Fit contiguity check is unchanged, so my open thread on it still holds. Codex raised the same point at the moved line: I kept it and report it once, replacing that thread; codex added nothing else, and nothing blocks.
Non-blocking
1 finding on the changed lines, as inline comments.
seidroid review · decision approve · session d024a7480b524ef1a01f7db4e8c8edea · turn resp_claude_efa319459ee048579b040516b483f733 · item 1becef6fa6f95a539f018d2d416bb824
Findings: 0 blocking | 1 non-blocking | 1 posted inline
…evict A call evicts at the bound only when, after it joins, its sender's queued nonces form a gap-free run from the sender's expected next nonce. That nonce is the tracked nonce, or the pre-read app nonce while no block was pruned since the read. A call whose expected nonce is unknown may queue but never evicts. A call below the expected nonce fails with errBadNonce without queueing, as a repeated queued nonce already does. expectedNonce holds the nonce rule appendTx already used, so the admission loop reads it under the mempool lock without calling the app. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
Since the last review, Fit checks contiguity against the sender's expected next nonce, which comes from mempoolInner.expectedNonce, the same tracked-or-pre-read rule appendTx now shares. Only a gap-free run from that nonce may evict, a call below it is rejected with errBadNonce before it queues, and an unknown expected nonce never evicts. That addresses my last open finding, so nothing blocks; codex found nothing, and I found nothing new either.
seidroid review · decision approve · session d024a7480b524ef1a01f7db4e8c8edea · turn resp_claude_05cb548fd85d1eb189a7fef4db5e6660 · item 9757faaffcdd53dd836f023ac67c549e
Findings: 0 blocking | 0 non-blocking | 0 posted inline
The Autobahn producer mempool admits blocked
InsertTxcalls in arrival order and ignores fees, which theTODO(gprusak)onInsertTxcalled out for the case where a node takes more inserts per second than its lane can carry. That case is now routine under saturation (PLT-1366): a full lane gives its capacity to whoever arrived first, and non-owned transactions that the router keeps locally when their owner is not connected compete with the lane's own shard. This builds on #4451 and is tracked as PLT-1374.Waiting inserts are now ranked by shard ownership, then priority, then arrival. The EvmOnly app reports the effective gas price as
PriorityfromCheckTx, andGetTxPriorityHintcomputes the same value without sender recovery, sopoolCanAdmitcan still reject before CheckTx when a newcomer does not outrank the worst waiter. The hint holds a CheckTx permit, because an app may recover the sender to compute it. Ownership comes from the same commit epoch the router shards by (Committee.EvmShardfor the session lane's validator), and it ranks admission only whenConfig.RankByShardOwnershipis set, which the node sets fromenable_evm_proxy: without the proxy a non-owned sender has no other lane to reach. When the key is not in the committee every transaction counts as owned. The FIFOwaitersslice is replaced inadmission.goby onegoogle/btreeordered by rank, whoseMinis the call to wake andMaxthe eviction candidate, plus a nonce-ordered queue per EVM sender, so only a sender's next nonce competes and a higher fee never passes an earlier nonce. At theMaxPendingInsertsbound a better call evicts the worst one, which returns the retryableerrPendingFulland is counted by a newproducer_evictionsmetric. The evicted call is the last queued call of the worst waiter's sender rather than the waiter itself, because evicting the head would leave that sender's later nonces next in line to fail witherrBadNonce. A call evicts only when its sender's queued nonces, with it added, form a gap-free run from the sender's next nonce: the mempool's tracked nonce, or the app nonce read before CheckTx if no block was pruned since. Any other call queues while there is room and getserrPendingFullat the bound, and a call whose next nonce is unknown never evicts. A call that repeats a nonce already queued for its sender, or whose nonce is below the sender's next nonce, fails witherrBadNoncewithout queueing, so there is no replace-by-fee: a same-nonce resubmission, such as a fee bump, fails while the original is queued. With no priority hint and unknown ownership the behaviour equals #4451, and the existing producer tests pass unmodified.This changes admission policy that clients can observe but not consensus or state, so it is non-app-hash-breaking. One design point wants a reviewer's call. The pre-CheckTx check cannot know the sender without sender recovery, so it assumes owned and cannot see a duplicate, stale, gapped or far-future nonce (those are rejected after CheckTx and never evict); when the worst waiter is a fallback transaction, every new insert pays for CheckTx, which weakens #4451 under a flood of non-owned traffic (still bounded by
MaxConcurrentCheckTx). The clean fix is for the RPC layer to pass the sender it already recovers for routing, which changes theInsertTxinterface. Ownership is fixed when a call joins the queue, there is no aging, so with the proxy on a fallback call can wait or be evicted while owned load keeps arriving, andProxy.GetTxPriorityHintdoes not recover from panics the wayCheckTxSafedoes. New tests inadmission_test.gocover rank order, owned before fallback, eviction and pre-CheckTx rejection, per-sender nonce order, equal ranks that never replace one another in the tree, duplicate, stale, gapped and far-future nonces that never evict, a model that checks every gap-free run against the expected nonce, ownership ignored without the flag, the hint waiting for a CheckTx permit, close, a model-based queue test and a stress test with evictions, all under-race.