Rework BatchTxPool functionality to a legitimate Ethereum tx mempool - #986
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBatch transaction pooling now tracks transactions per EOA and nonce. It validates local nonces, rejects duplicates, submits eligible transactions immediately, batches sequential transactions, retries failed submissions, and adds regression coverage. ChangesNonce-aware transaction pooling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant BatchTxPool
participant NonceProvider
participant Gateway
Client->>BatchTxPool: Add transaction
BatchTxPool->>NonceProvider: Read local EOA nonce
NonceProvider-->>BatchTxPool: Return current nonce
BatchTxPool->>BatchTxPool: Queue by nonce or select immediate submission
BatchTxPool->>Gateway: Submit sequential transaction batch
Gateway-->>BatchTxPool: Return Flow transaction ID or error
BatchTxPool->>BatchTxPool: Requeue failed batches or record successful submission
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
7abd408 to
911d22f
Compare
911d22f to
8aa8dff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/requester/batch_tx_pool.go (1)
281-332: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftRelease
txQueuesMuxbefore FastPath submission.
Add()holdstxQueuesMuxwhileGetNextNonce()reads local state and then whilesubmitSingleTransaction()sends a Flow transaction. All otherAdd()calls—any EOA—and the flush loop serialize behind the same mutex. Detach the fast-path batch like the flush path and re-acquiretxQueuesMuxafterSendTransactionreturns to recordlastSubmittedAtandlastSubmittedNonce.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/requester/batch_tx_pool.go` around lines 281 - 332, Update Add() to release txQueuesMux before the fast-path submitSingleTransaction call, avoiding holding the global queue lock during nonce lookup and network submission. Re-acquire the mutex after SendTransaction returns before updating eoaQueue.lastSubmittedAt and lastSubmittedNonce, while preserving timeout, error propagation, and submission metrics behavior.
🧹 Nitpick comments (6)
services/requester/batch_tx_pool.go (4)
535-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
eoaQueueEntryineoaEnqueueTxs.Both functions duplicate the get-or-create logic for a queue.
♻️ Proposed refactor
func (t *BatchTxPool) eoaEnqueueTxs(address gethCommon.Address, txs []pooledEvmTx) *txQueue { - queue, ok := t.txQueues[address] - if !ok { - queue = &txQueue{ - txs: make(map[uint64]pooledEvmTx), - } - t.txQueues[address] = queue - } + queue := t.eoaQueueEntry(address) for _, tx := range txs { queue.txs[tx.nonce] = tx } return queue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/requester/batch_tx_pool.go` around lines 535 - 551, Update eoaEnqueueTxs to reuse the existing eoaQueueEntry helper for the get-or-create queue logic instead of duplicating lookup and initialization. Preserve the subsequent transaction insertion into the returned queue and its current return behavior.
425-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assignment has no effect.
batchis the loop copy of the map value. Assigningbatch.eoaQueuedoes not updatetxBatchByAddress, andbatchis not read again in this iteration.eoaEnqueueTxsalready performs the re-enqueue. Drop the assignment.♻️ Proposed cleanup
- batch.eoaQueue = t.eoaEnqueueTxs(address, batch.txs) + t.eoaEnqueueTxs(address, batch.txs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/requester/batch_tx_pool.go` at line 425, Remove the assignment to batch.eoaQueue in the loop around txBatchByAddress; call eoaEnqueueTxs(address, batch.txs) directly, since the loop-local batch copy is not persisted or reused.
194-203: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBreak the loop at the first nonce gap.
The slice is sorted ascending. After
tx.nonce != stateNonce, no later element can match, because nonces only increase. The loop still scans the remaining entries.♻️ Proposed simplification
for _, tx := range txs { if len(txSequence) >= maxTxBatch { break } - if tx.nonce == stateNonce { - txSequence = append(txSequence, tx) - stateNonce += 1 - } + if tx.nonce != stateNonce { + break + } + txSequence = append(txSequence, tx) + stateNonce += 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/requester/batch_tx_pool.go` around lines 194 - 203, Update the loop building txSequence to break immediately when tx.nonce differs from stateNonce, since the sorted input cannot match afterward; retain the maxTxBatch limit and sequential nonce append/increment behavior.
138-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the comment with the actual bound.
The check
tx.nonce > maxEOAPoolSize+stateNoncebounds the nonce window, not the transaction count. The comment states "keep up tomaxEOAPoolSizetxs per EOA". Both descriptions coincide only when the queue is dense. Update the comment to describe a nonce window, or count entries to enforce a true size cap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/requester/batch_tx_pool.go` around lines 138 - 177, Update the pool-limit comment in txQueue.pruneTxs to describe the enforced nonce window from stateNonce through maxEOAPoolSize+stateNonce, rather than claiming a transaction-count limit. Keep the existing nonce comparison and pruning behavior unchanged.bootstrap/bootstrap.go (1)
290-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the shared nonce provider construction.
The
TxMemPoolModebranch on Lines 273-278 builds the sameLocalNonceProviderwith the same arguments. Create it once before the branch and pass it to whichever pool needs it. This keeps the two branches consistent if the provider gains a dependency later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bootstrap/bootstrap.go` around lines 290 - 304, Move the LocalNonceProvider construction out of the TxMemPoolMode conditional so it is created once before the branch. Reuse that shared nonceProvider when constructing either transaction pool, including the NewBatchTxPool call, while preserving the existing constructor arguments and branch behavior.tests/tx_batching_test.go (1)
273-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the actual production constant.
The comment refers to
TxMaxBatchSize. The pool defines the cap asmaxTxBatchinservices/requester/batch_tx_pool.go. Update the comment so a reader can find the source of the value thatmaxBatchSizeduplicates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tx_batching_test.go` around lines 273 - 277, Update the comment above Test_BatchTxPool_BatchSizeCap to reference the production constant maxTxBatch instead of TxMaxBatchSize, matching the cap defined by the batch transaction pool while preserving the test’s existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/requester/batch_tx_pool.go`:
- Line 431: Update services/requester/batch_tx_pool.go at lines 431-431 and
476-486: in the flush loop, move TxPoolSubmission from the per-attempt path into
the successful-send branch, or give failed attempts a distinct reason; in the
send-failure handling, replace TransactionsDropped with a submission-failure
metric while the batch is re-enqueued, and call TransactionsDropped only when
the pool permanently discards the transaction.
- Around line 352-360: Update the block-view failure handling in the batch
transaction pool tick around nonceProvider.GetBlockView: replace logger.Fatal
with a non-terminating error log, then return from the current operation so this
tick is skipped and the next tick can retry.
- Around line 307-338: After a successful fast-path submission in the
eoaQueue.validNonce branch, delete the entry for tx.Nonce() from eoaQueue.txs
before updating submission metadata and returning. Keep the deletion limited to
the successful submission path so failed submissions retain their queued state.
- Around line 405-430: Update the batch detachment logic before unlocking
txQueuesMux to record the detached batch’s lastSubmittedAt and
lastSubmittedNonce immediately. Preserve the previous values, and restore them
in the submission-error path alongside re-enqueuing batch.txs; keep the
successful completion path from overwriting these reservation values.
In `@tests/tx_batching_test.go`:
- Around line 965-968: In the bootstrap synchronization sequence, move the
time.Sleep call below the <-bootstrapDone receive so the Gateway settle period
starts only after bootstrap reports readiness. Keep the existing two-second
delay and its explanatory comment with the sleep.
- Around line 248-259: Update the stale timing comment and replace the hardcoded
2500ms sleep in the transaction batching test with a multiple of the configured
cfg.TxBatchInterval, ensuring the delay spans at least one complete flush
interval and avoids asserting exactly on a single tick boundary.
---
Outside diff comments:
In `@services/requester/batch_tx_pool.go`:
- Around line 281-332: Update Add() to release txQueuesMux before the fast-path
submitSingleTransaction call, avoiding holding the global queue lock during
nonce lookup and network submission. Re-acquire the mutex after SendTransaction
returns before updating eoaQueue.lastSubmittedAt and lastSubmittedNonce, while
preserving timeout, error propagation, and submission metrics behavior.
---
Nitpick comments:
In `@bootstrap/bootstrap.go`:
- Around line 290-304: Move the LocalNonceProvider construction out of the
TxMemPoolMode conditional so it is created once before the branch. Reuse that
shared nonceProvider when constructing either transaction pool, including the
NewBatchTxPool call, while preserving the existing constructor arguments and
branch behavior.
In `@services/requester/batch_tx_pool.go`:
- Around line 535-551: Update eoaEnqueueTxs to reuse the existing eoaQueueEntry
helper for the get-or-create queue logic instead of duplicating lookup and
initialization. Preserve the subsequent transaction insertion into the returned
queue and its current return behavior.
- Line 425: Remove the assignment to batch.eoaQueue in the loop around
txBatchByAddress; call eoaEnqueueTxs(address, batch.txs) directly, since the
loop-local batch copy is not persisted or reused.
- Around line 194-203: Update the loop building txSequence to break immediately
when tx.nonce differs from stateNonce, since the sorted input cannot match
afterward; retain the maxTxBatch limit and sequential nonce append/increment
behavior.
- Around line 138-177: Update the pool-limit comment in txQueue.pruneTxs to
describe the enforced nonce window from stateNonce through
maxEOAPoolSize+stateNonce, rather than claiming a transaction-count limit. Keep
the existing nonce comparison and pruning behavior unchanged.
In `@tests/tx_batching_test.go`:
- Around line 273-277: Update the comment above Test_BatchTxPool_BatchSizeCap to
reference the production constant maxTxBatch instead of TxMaxBatchSize, matching
the cap defined by the batch transaction pool while preserving the test’s
existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78833572-8bf3-40ef-9bc7-6b4f918f48ca
📒 Files selected for processing (4)
bootstrap/bootstrap.gogo.modservices/requester/batch_tx_pool.gotests/tx_batching_test.go
Kay-Zee
left a comment
Kay-Zee
left a comment
There was a problem hiding this comment.
Approving — the rework does what it sets out to do. I verified the three core behaviors against the code (and CI is green): an immediately-ready tx fast-paths synchronously out of Add (spacing always elapses for a fresh queue, validNonce checks the local state index, and unknown accounts read as nonce 0, so first-ever transactions qualify); a second tx from the same EOA inside TxBatchInterval never fast-paths, and the flush loop gates on the state index rather than the timer — a queued tx only enters a batch once the index confirms its predecessor executed, which is strictly stronger than time spacing; and selectSequentialNonces builds the run starting exactly at the state nonce and stops at the first gap, so pooled {5,6,8,9,10} at state nonce 5 submits exactly {5,6} and holds the rest until 7 lands. Test_BatchTxPool_GapHoldAndFill is that scenario end-to-end, and the failed-batch re-queue with reservation rollback means transient submission errors can't manufacture nonce gaps either.
Two inline items — one behavior gap worth a fix (in-flight duplicates), one comment that overstates what this pool does. Neither blocks the merge: the gap can't corrupt state, since the execution-layer nonce check makes double-execution impossible no matter how the pool behaves.
One nit: EOAActivityCacheTTL is dead config now — only the flag and the struct field remain.
| // check; needs no index read). | ||
| eoaQueue := t.eoaQueueEntry(from) | ||
| if existing, ok := eoaQueue.txs[tx.Nonce()]; ok && existing.txHash == tx.Hash() { | ||
| return errs.ErrDuplicateTransaction |
There was a problem hiding this comment.
This dedup only covers the queue, and submitted txs are deleted from it — so a wallet retrying an identical tx after it fast-pathed gets it re-accepted, and if the index hasn't advanced it can be resubmitted: queued at the next tick (selectSequentialNonces picks it up while the state nonce is unchanged), or fast-pathed outright once spacing elapses, since validNonce(5, 5) still holds while the index lags. Concretely: 5 fast-paths, a resent 5 arrives, then 6 — at the next tick 5(resent) and 6 can go out together in one batch.
No double-execution is possible — run.cdc treats nonce-too-low as invalid, so the duplicate either gets skipped in the batch or fails the Cadence wrapper — but it costs a Cadence submission, possibly a failed wrapper tx, noisy metrics, and a success response where Ethereum clients expect already known / nonce too low.
Add already has both numbers in hand, so this can be closed at admission: reject txNonce < stateNonce as nonce-too-low, and stateNonce <= txNonce <= lastSubmittedNonce (when lastSubmittedAt is set) as ErrInFlightNonce — mirroring what TxMemPool does in tx_mempool.go. That also converts the flush-time pruneTxs silent drops (which happen after Add already returned success) into admission-time rejections the caller can actually see.
Worth noting this is inherently per-gateway: a retry hitting a different gateway replica can't be caught this way, and that's fine — the execution-layer nonce check is the real backstop there, same as multi-node Ethereum. Sticky per-EOA routing would make the per-gateway guard effective for realistic retry patterns.
There was a problem hiding this comment.
Nice catch, I missed that part.
Note that nonce-too-low is a check that applies to any pool implementation (SingleTxPool / BatchTxPool / TxMempool), so it already happens in the layers above, to avoid any lock-acquisition from the configured pool strategy, see here: https://github.com/onflow/flow-evm-gateway/blob/main/services/requester/requester.go#L602-L611 .
That being said, I added the check for rejecting in-flight nonces in 427224b .
| // gap, not yet submitted) is rejected with ErrDuplicateTransaction. | ||
| // | ||
| // A fast-path-submitted tx instead becomes IN-FLIGHT, so resending it would | ||
| // surface as ErrInFlightNonce (see Test_TxMemPool_InFlightNonceRejection). |
There was a problem hiding this comment.
This comment overstates what BatchTxPool does: ErrInFlightNonce is only enforced in TxMemPool (tx_mempool.go) — this pool has no in-flight tracking, so a resent fast-pathed tx is currently re-accepted, not rejected (see my comment on the duplicate check in batch_tx_pool.go). As written, the test docs describe behavior the code doesn't have; either fix the comment, or fold in the admission-time guard and the claim becomes true.
There was a problem hiding this comment.
Good point 👍 I added the appropriate logic and a dedicated test in 427224b .
|
The |
…tests - flush success log tagged as flushReasonPrefix (metric already correct) - flush success/failure paths merge lastSubmittedNonce/At with max() so a concurrent Add() fast-path is never regressed by a stale ack or rollback - eoaEnqueueTxs preserves an existing same-nonce entry: last-write-wins keeps a client's fresher payload over a re-queued failed batch - rename maxEOAPoolSize -> maxNonceLookahead (that's what it enforces and add a real per-EOA size cap at admission time via maxEOAQueueSize + ErrTxPoolFull - staleEntry no longer marks a freshly-created queue (zero lastSubmittedAt) as stale - selectSequentialNonces walks the nonce-keyed map directly (O(k)) instead of sorting the full queue every tick - add batch_tx_pool_test.go: txQueue primitives + rollback preserve-fresh
Solidify BatchTxPool: log tag, rollback race, size cap, unit tests
| if !existsAtNonce && eoaQueue.size() >= maxEOAQueueSize { | ||
| return errs.ErrTxPoolFull | ||
| } |
There was a problem hiding this comment.
This check runs before the fast-path check below. With a full queue (50 txs held behind a missing head nonce), the gap-filling tx that would fast-path and unblock the whole queue is rejected with ErrTxPoolFull, and the queue can never drain on its own. The EOA is wedged until restart. Move the cap check to just before the enqueue, so fast-path-eligible txs bypass it.
There was a problem hiding this comment.
That's a legit scenario that could cause some serious issues, for high-volume dApps/bots. Fixed in bf4c366 .
| eoaQueue := t.eoaEnqueueTxs(address, batch.txs) | ||
| if eoaQueue.lastSubmittedNonce == batch.txs[len(batch.txs)-1].nonce { | ||
| eoaQueue.lastSubmittedNonce = batch.lastSubmittedNonce | ||
| eoaQueue.lastSubmittedAt = batch.lastSubmittedAt | ||
| } |
There was a problem hiding this comment.
Failed batches are re-enqueued with no attempt cap or TTL, so a deterministically failing submission is rebuilt and resent every TxBatchInterval forever, while higher nonces pile up behind it. Cap retries per tx and drop with a WARN + TransactionsDropped after N attempts.
There was a problem hiding this comment.
Although failing submission should largely be of transient networking issues, because we just use the client from flow-go-sdk to submit transactions to ANs, I have added a cap for retries in aafa055 , to be on the safe side.
| } | ||
|
|
||
| t.eoaActivity.Add(from, time.Now()) | ||
| eoaQueue.txs[tx.Nonce()] = userTx |
There was a problem hiding this comment.
A tx held behind a nonce gap that never fills stays in the pool silently until restart: within maxNonceLookahead it is never pruned, and a non-empty queue never goes stale. TxMemPool covers this with TxPoolTTL/collectExpired. Add an enqueuedAt + TTL with a submit-anyway or WARN-drop path so a stuck tx eventually gets an observable outcome.
There was a problem hiding this comment.
Fair point, we should be quite meticulous about not letting the queue grow without control. Added a enqueuedAt + TTL logic in 01bfc16 .
| // Reject an exact duplicate of a transaction already in the queue | ||
| // (cheapest check; needs no index read). | ||
| eoaQueue := t.eoaQueueEntry(from) | ||
| existing, existsAtNonce := eoaQueue.txs[tx.Nonce()] | ||
| if existsAtNonce && existing.txHash == tx.Hash() { | ||
| return errs.ErrDuplicateTransaction |
There was a problem hiding this comment.
nit: GetNextNonce runs above (line ~293) for every tx, so "needs no index read" is inaccurate, and pure duplicate retries still pay a view lookup. Move the duplicate and in-flight checks (queue state only) above the GetNextNonce call, or fix the comment.
| // in the local state index: | ||
| // 1. If it matches the transaction nonce, we submit it right away, and record | ||
| // this activity in the EOA's dedicated queue, for use in future submissions. | ||
| // 2. If the transaction nonce is higher, we check for any recent submissions | ||
| // to see if we can form a valid sequence. This could happen from in-flight | ||
| // transaction submission, that have not yet been index by the local state | ||
| // index. In this case we optimistically submit right away. and record this |
There was a problem hiding this comment.
nit: steps 1-2 say matching nonces are submitted "right away", but the fast path also requires spacingElapsed — a matching-nonce tx arriving within TxBatchInterval is enqueued, not submitted. Also two typos: "been index by" -> "been indexed by", and a stray period in "right away. and record".
There was a problem hiding this comment.
Fair point, comments updated in 298b60e .
| // batchSubmission is a batch selected for submission, detached from the queue so | ||
| // the network call happens outside queueMux. | ||
| type batchSubmission struct { | ||
| from gethCommon.Address |
There was a problem hiding this comment.
nit: batchSubmission.from is assigned but never read — the flush loop uses the map key everywhere.
| func (t *BatchTxPool) eoaEnqueueTxs(address gethCommon.Address, txs []pooledEvmTx) *txQueue { | ||
| queue, ok := t.txQueues[address] | ||
| if !ok { | ||
| queue = &txQueue{ | ||
| txs: make(map[uint64]pooledEvmTx), | ||
| } | ||
| t.txQueues[address] = queue |
There was a problem hiding this comment.
nit: this duplicates eoaQueueEntry's get-or-create block. Call eoaQueueEntry and keep only the re-enqueue loop.
There was a problem hiding this comment.
Oh, nice, I missed that.. Updated in 686ae8f .
| batchTail := batch.txs[len(batch.txs)-1].nonce | ||
| if batchTail > batch.eoaQueue.lastSubmittedNonce { | ||
| batch.eoaQueue.lastSubmittedNonce = batchTail | ||
| } |
There was a problem hiding this comment.
nit: no test covers the ErrTxPoolFull admission path or the flush failure/success merge branches (rollback guard, non-regression merge). A unit test with a fake failing submitter plus a concurrent-Add interleave would cover both.
There was a problem hiding this comment.
Added 3 dedicated unit tests in 6f1b87e .
a7d5981 to
01bfc16
Compare
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Fixes look great! Thanks. Found 2 new things. the rest looks good.
| } | ||
| queue.txs[tx.nonce] = tx | ||
| } | ||
| queue.retries += 1 |
There was a problem hiding this comment.
retries is never reset on success. A queue is only re-zeroed when it is evicted (empty and idle for 2x interval), so a continuously active EOA keeps its counter forever. After 5 cumulative failures, even transient blips spread far apart, every later failure drops the batch on the first attempt, which manufactures the nonce gap the retry mechanism exists to prevent.
| eoaQueue.lastSubmittedAt = time.Now() | ||
| eoaQueue.lastSubmittedNonce = txSequence[len(txSequence)-1].nonce |
There was a problem hiding this comment.
This reservation can regress lastSubmittedNonce below a concurrent fast-path advance. Scenario: batch {5,6,7} is in flight (marker=7), the submit outlives the interval so Add fast-paths nonce 8 (marker=8), the batch fails and is re-enqueued (rollback skipped since 8 != 7), then the next tick re-detaches {5,6,7} and overwrites the marker back to 7. A subsequent Add for nonce 8 now passes the in-flight guard (8 <= 7 is false) and validNonce (7+1), so nonce 8 is submitted twice. Make this non-regressing, like the success-path merge: only advance when the batch tail exceeds the current value.
There was a problem hiding this comment.
@janezpodhostnik I'm having difficulty understanding the actual issue here, and the suggested fix.
If the submission of batch {5,6,7} outlives the interval, Add will fast-path nonce 8, which is going to fail with nonce too high: address 0xab, tx: 8, state: 5, because the expected nonce is 5, and the batch submission has not yet happened.
Assuming the batch fails, transactions {5,6,7} are re-enqueued, the next tick re-detaches {5,6,7} and overwrites lastSubmittedNonce back to 7, from 8. To me this is legitimate behavior, because lastSubmittedNonce = 8 doesn't necessarily mean the on-chain nonce advanced, only the marker in EVM Gateway did. In fact, since we haven't yet submitted the batch of {5,6,7}, we can be certain that the on-chain nonce is for sure not 8.
That being said, does your suggestion still hold?
| // 2. If enough spacing has elapsed and the transaction nonce is higher, we check | ||
| // for any recent submissions to see if we can form a valid sequence. This could | ||
| // happen from in-flight transaction submission, that have not yet been indexed | ||
| // by the local state index. In this case we optimistically submit right away and |
There was a problem hiding this comment.
The reason we have case one above is to optimize for the case of ordinary user sending transaction without brust, so that they don't have to wait for 2.5sec just to send a single tx.
But the problem we ran into last time is from case 2, where we received currentNonce + 1, we started waiting, and when the waiting is over, we submitted the batch right away hoping that this batch can be accepted as soon as the in-flight tx, which carries the currentNonce and sent previously, is accepted. However, we still ran into problem, I think it's because even though the single currentNonce tx is sent earlier, it ended up reordered later than the batch tx in the same block, which caused the batch tx to fail.
We'd better add a test case simulating that problem we ran into and verify.
Basically, we had hard control on how long is actually enough for the waiting before sending the batch tx, so that it can be very likely ordered after the single currentNonce tx. Maybe we should make this parameter configurable via flag and experiment with it. For instance, if we configure it with 2.5 sec initially and reduce it down slowly.
With this logic, I think a batch tx without any waiting can still be achieved by sending txs with a nonce gap first, and then send a no-op tx to fill the nonce gap to trigger the whole batch to be sent. Would DFNS send like this?
There was a problem hiding this comment.
The reason we have case one above is to optimize for the case of ordinary user sending transaction without brust, so that they don't have to wait for 2.5sec just to send a single tx.
Exactly, and the reasoning behind it is this: with soft-finality it takes about 6-8 seconds until the local state index is updated after a particular transaction submission. For regular users this is enough time, we basically have the updated nonce in the local state index, by the time the user submits a subsequent transaction.
But the problem we ran into last time is from case 2, where we received currentNonce + 1, we started waiting, and when the waiting is over, we submitted the batch right away hoping that this batch can be accepted as soon as the in-flight tx, which carries the currentNonce and sent previously, is accepted. However, we still ran into problem, I think it's because even though the single currentNonce tx is sent earlier, it ended up reordered later than the batch tx in the same block, which caused the batch tx to fail.
The implementation that produced this issue, has a number of knobs & timers:
TxCollectionWindow(defaults to 300 ms): Per-EOA sliding collection window for the transaction mempool. Resets on each arrival from the same EOA.TxSubmissionSpacing(defaults to 1.2 seconds): Minimum gap between consecutive Cadence submissions for the same EOA in the transaction mempool; also serves as the flush deadline for a continuously-fed collection window. Recommended ~1.5x the block production rate.
which makes it a bit harder to reason about the root cause of the issue.
The implementation proposed in this PR, has a single timer:
TxBatchInterval(defaults to 2.5 seconds): Time interval upon which to submit the transaction batches to the Flow network.
to make sure that we don't cause any disruptions for burst dApps, we are more defensive in the waiting/queuing time. We start from the state nonce, and submit batches with sequential nonces. But we can't wait the 6-8 seconds it takes to index the state from the submitted batch, we sequentially update lastSubmittedNonce, so that we can further reduce to waiting time for burst dApps as well.
For that, the we'll run some live tests on testnet (evm-001) because with Emulator it's hard to simulate the collection topology and traffic that exists for testnet/mainnet.
| ) | ||
| return err | ||
| } | ||
| t.logSubmission(from, []pooledEvmTx{userTx}, flushReasonFastPath, flowTxID) |
There was a problem hiding this comment.
there are two cases that eoaQueue.validNonce would return true, can we distinguish them in the flushReasonFastPath logs?
- the tx's nonce is the localIndexedNonce + 1 (i.e. log as `fast-path: next unindexed nonce)
- the tx's nonce is the lastSubmittedNonce + 1 (i.e. log as `fast-path: next unsubmitted nonce)
So that if we ran into wrong nonce issue, it could confirm what case exactly is hitting.
maybe we can extract the logic in the if condition into a function to be reused by both cases?
There was a problem hiding this comment.
That would definitely help with debugging. Updated in dca6f94 .
| // has elapsed and the tx nonce is the next expected, we submit right | ||
| // away and update the `lastSubmittedAt` & `lastSubmittedNonce` fields, | ||
| // for classifying future submissions, that might arrive shortly. | ||
| if eoaQueue.spacingElapsed(time.Now(), t.config.TxBatchInterval) && eoaQueue.validNonce(tx.Nonce(), nonce) { |
There was a problem hiding this comment.
There is an edge case where lastSubmittedAt < tx.Nonce() < nextIndexedNonce, which we should reject, but would be added to the queue?
Why would lastSubmittedAt < nextIndexedNonce? maybe because there is another gateway sending tx for this EOA as well.
So if lastSubmitted is 10, nextIndexedNonce is 20, and tx.Nonce() is 18, it ended up being added to the queue, but actually could be rejected.
There was a problem hiding this comment.
Note that there are prior validation checks at outer layers (the Requester for example), before even a transaction gets in the pool (regardless of which pool implementation is in use).
Two such checks are:
- Making sure that the sender has enough balance to cover gas fees + transfer value
- The transaction nonce is not lower than the
nextIndexedNonce
Check 2. is done here: https://github.com/onflow/flow-evm-gateway/blob/main/services/requester/requester.go#L602-L611 . So in your example scenario above, the transaction would have been rejected even before reaching the pool, and an appropriate error message would be returned to the author.
| lastSubmittedAt time.Time | ||
| lastSubmittedNonce uint64 |
There was a problem hiding this comment.
please add comments to this variable, I think they are used for reverting the queue's last submitted state in order to retry submission, right?
There was a problem hiding this comment.
Yep, you are right. Added comments in 1e2b22e .
| if eoaQueue.lastSubmittedNonce == batch.txs[len(batch.txs)-1].nonce { | ||
| eoaQueue.lastSubmittedNonce = batch.lastSubmittedNonce | ||
| eoaQueue.lastSubmittedAt = batch.lastSubmittedAt | ||
| } |
There was a problem hiding this comment.
maybe this can be moved into eoaEnqueueTxs, since this is the only case we enqueue a batch of txs, so the lastSubmittedNonce should also be checked and updated. In this case, it doesn't need to return the eoaQueue any more.
There was a problem hiding this comment.
That makes more sense, and it will allow for better unit tests. Updated in 8ebe992 .
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
| @@ -514,59 +975,6 @@ func Test_MultipleTransactionSubmissionsWithinNonRecentInterval(t *testing.T) { | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
could we have a test case to simulate this case?
#983 (comment)
Basically, we need to verify that if an EOA have a series of txs with nonce [1-10] send to the GW in whichever order and spacing, the GW will never send to the AN two txs, which has consecutive nonce, within a batch interval. We could verify by making the mock tx receiver panic when receiving nonce 2 too quickly after reciving nonce 1, which is the case that caused both nonce 1 and 2 are included in the same block but in the wrong order.
There was a problem hiding this comment.
Good idea 👍 While adding such an E2E test in 250bfaf, I realized we had a small gap in processPooledTransactions(), that could manifest the case you mentioned above.
The case was this: We could be holding a batch [1-5], for some amount of time. When 0, it would be fast-pathed, but depending on the status of the timer in processPooledTransactions(), we could end up submitting the batch [1-5] in quite a short interval. So I added:
// skip processing for this EOA, if there was any recent activity
// from concurrent `Add()`
if !eoaQueue.spacingElapsed(time.Now(), t.config.TxBatchInterval) {
continue
}The batch would be picked up on the next tick, which I made more frequent now, to avoid extending the waiting time.
Correction on prior testnet validation comment (deleted)The earlier "Testnet validation" comment I posted was misleading and has been deleted. Here is the corrected picture. What was wrong:
What's needed for a real testnet validation of this PR:
Wedge-recovery validation (issue #983 comment 5112498568) cannot be done on testnet at all:
Follow-up work when this ships:
🤖 Generated with Claude Code |
Testnet validation — actual
|
| Scenario | Result |
|---|---|
fastpath — single tx, expected next nonce |
1/1 mined status 1 in 4.6s ✅ |
burst -n 50 — 50 sequential nonces, concurrent send |
41/50 mined; tail nonces 409..417 dropped by pool TTL |
gap — send N+5 first, then fill N..N+4 |
N+5 held while gap unfilled; all 6 mined in 14s once gap filled ✅ |
duplicate — resend identical bytes of a queued tx |
Second submission rejected: invalid: transaction already in pool ✅ |
The burst 50 tail-drop
Mechanism:
TxBatchInterval = 2.5sgates consecutive submits per EOA (spacingElapsedinbatch_tx_pool.go:141).maxTxBatch = 5caps each batch → 50 txs need 10 sequential batches.maxQueueTTL = 30sprunes any queued tx that's been held past the TTL (pruneTxs, line 213-222).- 10 batches × 2.5s spacing + state indexing lag between batches ≈ 30+s to drain the queue, so tail nonces hit TTL before their batch can form.
- Verified after the fact: on-chain nonce reached 408 (41 txs mined). Nonces 409..417 were dropped; the subsequent
gapscenario re-signed 409..414 which then mined, leaving 415..417 permanently gapped on chain.
Context:
- The old
--tx-mempool-mode=trueconfig used a 1.2sTxSubmissionSpacingdefault → same 50-tx burst previously got 50/50 in 15s. - The 2.5s
--tx-batch-intervalvalue is the mainnet convention (matchestests/tx_batching_test.go:1106), andstalenessFactorwas tuned to it in8529e5cc, so this is by design for the new pool — not a code regression in ReworkBatchTxPoolfunctionality to a legitimate Ethereum tx mempool #986. - Real-world DFNS traffic is sequential-over-time, not a 50-nonce burst, so this may not manifest on mainnet at current traffic patterns. But it does mean burst tolerance is lower with this pool at 2.5s spacing than with the old mempool at 1.2s spacing.
Follow-up questions this raises (not blockers for #986 itself):
- Should mainnet activation use
--tx-batch-interval=1.2sinstead of 2.5s to preserve the previous burst tolerance? Trade-off: shorterstaleEntrywindow (4.8s vs 10s wedge recovery). - Would raising
maxQueueTTL(currently a hard-coded 30s constant) help without hurting the wedge-recovery semantics?
Coverage note
- Golden-path smoke (fastpath / gap / duplicate) validates that Rework
BatchTxPoolfunctionality to a legitimate Ethereum tx mempool #986's Add / classify / flush loop behaves correctly on the reworked pool. - Wedge-recovery cannot be validated on testnet — see test(requester): add BatchTxPool wedge → recovery e2e test #988 for the in-repo test using
emu.DisableAutoMine().
🤖 Generated with Claude Code
Reproduces the wedge state from #983 (Cadence wrapper reverts before advancing state, leaving the pool's in-flight marker ahead of the on-chain frontier) and asserts BatchTxPool clears it within TxBatchInterval*stalenessFactor. Auto-mine is disabled so the pool's staleEntry timing is exposed - otherwise validateTransactionWithState preempts once state catches up, masking the pool's recovery. This is the counterpart to Test_BatchTxPool_InFlightNonceRejection, which only verifies the wedge state is entered, not that it recovers. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zhangchiqing
left a comment
zhangchiqing
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the update!
Mainnet rollout status — first 24h clean ✅
Prometheus snapshot (
No wedge events, no TTL prunes, no batch-retry exhaustion logs. Keeping the watch on for another 24h before calling it fully baked, but so far the rework is behaving exactly as intended: fast-path dominant, queue drained, drops flat. |
5 participants
Closes: #983
Problem
Flow did not have a traditional EVM mempool. On standard EVM chains, when a wallet sends transactions out-of-nonce-sequence (e.g., nonces 5, 7, 6 in parallel), the mempool holds future-nonce transactions until the gap is filled. Flow EVM had no such pooling mechanism — a transaction whose nonce does not match the current account nonce is simply dropped.
The original
BatchTxPoolimplementation partially addressed this by batching transactions that arrived from an EOA with "recent activity" (i.e., a prior transaction withinTxBatchInterval,2.5 secondsformainnet). However, it still submitted the FIRST transaction from any burst immediately — before the rest of the burst had a chance to arrive. If that first transaction happened to carry a future nonce (due to parallel dispatch), it failed, and the gap it left caused all subsequent nonces in the batch to fail as well.Fix
For all incoming transactions, we are now inspecting the EOA's current nonce in the local state index:
TxBatchInterval) is the sole submission trigger. This guarantees that parallel transactions from the same wallet accumulate in the pool before being sorted by nonce and submitted atomically. When we process the per-EOA pool, we read again the current nonce from the local state index, any stale transactions are pruned, we attempt to select a list of transactions with sequential nonces (up to 5 transactions max), and we submit those in a batch. On successful submission, we record this activity in the EOA's dedicated queue, for use in future submissions. When there are submission errors, e.g. due to network, we add the transactions back to the pool as a retry mechanism. This is an important part to avoid gaps, which would require users to resubmit.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes