Refactor & harden the nonce-aware tx mempool (stacked on #971)

[vishalchangrani]

What

Refactors and hardens the nonce-aware transaction mempool introduced in #971, driven by a design review of services/requester/tx_mempool.go. This PR is stacked on #971 — its base is vishal/nonce-aware-mini-pool, so the diff is only the follow-up work on top of that pool. Draft for review.

No change to the pool's external behavior except one liveness fix (fast-path submit timeout); everything else is internal refactoring, observability, documentation, and tests.

Highlights

Structure / state machine

• Replaced ad-hoc in-flight tracking with an explicit per-EOA nonceTracker state machine (classify / markSubmitting / markSubmitted / rollbackSubmitting) and a nonceWrapper ("unset = −∞") type that removes scattered nil/zero checks.
• Decomposed the ~145-line collectDueBatches into focused helpers (collectQueue / collectPrefix / collectExpired / reportSize), surfacing the prefix-vs-TTL asymmetry.
• Deferred heldTx allocation past the cheap rejections; cached the block view by indexed height; assorted Go-idiom cleanups (helpers, prealloc, %w wrapping).

Correctness / safety

• Reject out-of-range nonces up front (ErrNonceTooLow / ErrNonceTooHigh, new --tx-max-nonce-gap).
• Liveness fix: bound the fast-path submit with a 10s ceiling so a hung Access-node call can't pin the pool-wide lock for up to the 120s request timeout.

Observability — no silent drops

• Every submission outcome is logged: WARN on drop (submit failure / stale prune / TTL submit-anyway) with full batch context, DEBUG on success. Invariant: any accepted tx id is either on-chain or in a WARN log — documented, including the one shutdown caveat (no graceful drain).

Documentation

• A top-of-file behavior spec enumerating all 12 cases + cross-cutting invariants, each with a concrete nonce example, usable as a test checklist.

Tests

• Dedicated nonceTracker unit tests; an injectable clock enabling timing tests through the real Add → tick → flush path (collection window, spacing gate, flush deadline, TTL, idle eviction); plus duplicate/in-flight/rejection coverage.

Validation

go build ./..., go vet, go test ./services/requester/ -race, and the Test_TxMemPool e2e all pass.

Notes for reviewers

• Commits are organized one-per-change and are meant to be read in order (the 7-step redesign, then a comment audit, then audit-driven fixes).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added a new setting to control how far ahead queued transactions can be from the current nonce.
  • Improved nonce handling so transactions are classified more consistently before submission.

• Bug Fixes

  • Rejected transactions that are clearly too low or too high for the account’s nonce.
  • Prevented duplicate queued transactions from being accepted.
  • Improved cleanup after failed submissions and stale queue entries.

• Tests

  • Expanded coverage for nonce limits, submission behavior, timing, and queue eviction.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1f988d4e-4853-45bf-9bbe-cfecaadd4f12

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch vishal/tx-mempool-redesign

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[vishalchangrani]

These are changes to incorporate @m-Peter comment in the earlier PR about reusing the block view to find the last nonce for an EOA. Caching the view for a given block height ensures we are not performing the expensive operation of reading the full block view each time when figuring out the latest on-chain nonce for an EOA.

[vishalchangrani]

this change is to address @zhangchiqing suggestion that we should reject nonce that are ridiculously higher than the current nonce. e.g. 1M nonce in the future.

[vishalchangrani]

Explicitly mentioning the expected behaviour as a spec for both humans and AI. @zhangchiqing mentioned that we were missing this and I agree. Hopefully, this makes it clear to whoever works on this code later about what the code does and the different use cases.

[vishalchangrani]

claude pointed out that in the fastpath where we submit the transaction right away instead of queing it up, the context has no deadline which means that a slow or unresponsive Access node could delay all subsequent transactions. This changes adds timed context for the fast path with this timeout value.

[vishalchangrani]

this also makes nonce comparision easy to read e.g. nonceWrapper.atLeast(5) instead of nonce != null && nonce >= 4

[vishalchangrani]

a note on why not all the fields of the eoa queue need to be initialized here

[vishalchangrani]

the nonce tracker helps us replace those .. && .. || conditions with switch and hopefully more readable statements

[vishalchangrani]

one thing claude flagged was that the shutdown was not gracious and inflight transactions will be lost. For now, there is just this comment about that issue.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

services/requester/tx_mempool_test.go (1)

250-256: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Tighten the fast-path timeout assertion.

The extra + time.Second slack can hide regressions above fastPathSubmitTimeout. Capture after := time.Now() after Add and assert the deadline is no later than after.Add(fastPathSubmitTimeout).

Proposed test tightening

 before := time.Now()
 require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 0, 1)))
+after := time.Now()

 require.True(t, hasDeadline, "fast-path submit context must carry a deadline")
 assert.Greater(t, deadline, before)
-assert.LessOrEqual(t, deadline.Sub(before), fastPathSubmitTimeout+time.Second,
+assert.LessOrEqual(t, deadline, after.Add(fastPathSubmitTimeout),
 	"deadline must be bounded by fastPathSubmitTimeout")

🤖 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/tx_mempool_test.go` around lines 250 - 256, Tighten the
fast-path deadline check in the mempool test by removing the extra slack that
can mask regressions. In tx_mempool_test.go, update the assertion around
pool.Add and the deadline capture so it records an after := time.Now()
immediately after the Add call, then verify the context deadline is no later
than after.Add(fastPathSubmitTimeout). Keep the existing hasDeadline check and
the lower-bound assertion, but use the captured after time together with
fastPathSubmitTimeout to make the bound precise.

🤖 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/tx_mempool.go`:
- Around line 892-901: The submission spacing timestamp is being set too early
in collectDueBatches, which can let later EOA batches bypass
TxSubmissionSpacing. Move the lastSubmittedAt update out of collection-time
logic and stamp it in the post-submit path, preferably in reconcileSubmission or
equivalent submit-completion handling using t.now(), so the spacing reflects the
actual Flow submission attempt. Apply the same fix to the TTL batch path and any
related detached submission flow so all batch types share the same timing
source.
- Around line 332-336: The max nonce-gap validation in tx_mempool.go can
overflow because the check in the nonce handling path compares against
n.localIndexedNonce+n.maxNonceGap directly. Update the logic in the nonce
validation block that returns nonceTooHigh so it uses the already-established
nonce >= n.localIndexedNonce relationship and compares the distance between
nonce and n.localIndexedNonce instead of adding the two values, keeping the
behavior in sync with the surrounding nonce-gap handling in the mempool gate.

---

Nitpick comments:
In `@services/requester/tx_mempool_test.go`:
- Around line 250-256: Tighten the fast-path deadline check in the mempool test
by removing the extra slack that can mask regressions. In tx_mempool_test.go,
update the assertion around pool.Add and the deadline capture so it records an
after := time.Now() immediately after the Add call, then verify the context
deadline is no later than after.Add(fastPathSubmitTimeout). Keep the existing
hasDeadline check and the lower-bound assertion, but use the captured after time
together with fastPathSubmitTimeout to make the bound precise.

🪄 Autofix (Beta)

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

Run ID: 6ca7e1c4-4951-4de1-9c9c-c1f7b176bf74

📥 Commits

Reviewing files that changed from the base of the PR and between 8f50460 and 46cf137.

📒 Files selected for processing (6)

• cmd/run/cmd.go
• config/config.go
• models/errors/errors.go
• services/requester/nonce_provider.go
• services/requester/tx_mempool.go
• services/requester/tx_mempool_test.go

[vishalchangrani]

@m-Peter , @zhangchiqing , @janezpodhostnik -

So, I have a new version of code now. However, I changed a lot of things so to make it easier to review, the changes are a PR on my earlier PR.

This new PR that build on the old PR #971.

What changed -

1. I have now some additional constructs to make the code more readable and easy to maintain -

• nonceTracker - all the nonce comparision is abstracted out to this struct. Add method simply calls nonce tracker to decide what to do with the incoming transaction.
• nonceWrapper - a simple wrapper for a nonce that help distinguish between uninitialized nonce and initialized but zero value nonce. Plus it has simple comparision methods for nonce.
• Split collectDueBatches into four separate functions.
• removed the isInFlight boolean flag and introduced a caching mechanism for the blockview for a given height.

2. Behaviour spec - a spec defintion at the top which calls out what the code does.

3. Claude suggested some additional changes

• Use an injectable now function to add more unit test for time based functions.

When you get a chance can you please review this PR ?

thanks a lot

[vishalchangrani]

@coderabbitai re the tx_mempool_test.go nitpick (tighten the fast-path timeout assertion): fixed in ec791ad5. Now captures after := time.Now() immediately after Add and asserts deadline <= after.Add(fastPathSubmitTimeout), dropping the +time.Second slack that could have masked a regression.

Summary of this review pass:

• Overflow in nonce-gap check (tx_mempool.go) — fixed, ec791ad5 (distance comparison).
• Fast-path timeout test slack — fixed, ec791ad5.
• Spacing stamped at collection time — concern valid; CodeRabbit's literal fix would regress (the collection-time stamp gates against collecting a follow-up batch while one is in flight). Documented as a bounded, accepted skew for the current few-EOA deployment (f0011415); see the thread reply for the precise fix if fan-out grows.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@vishalchangrani Thanks — that addresses the tx_mempool_test.go timeout nit, and the tighter after := time.Now() bound is exactly what I was looking for.

Also appreciate the write-up on the spacing stamp behavior. The rationale makes sense: moving the stamp to submission time would interfere with collecting a follow-up batch while one is already in flight, so documenting the current collection-time stamp as a bounded/intentional skew is a reasonable choice for the current deployment.

(•ᴗ•)🐇

---

✏️ Learnings added

Learnt from: vishalchangrani
URL: https://github.com/onflow/flow-evm-gateway/pull/974

Timestamp: 2026-06-26T16:11:10.786Z
Learning: In `services/requester/tx_mempool.go`, the spacing timestamp being recorded at collection time is an intentional tradeoff, not an oversight: moving it to submission time would gate collection of a follow-up batch while another batch is in flight. For the current few-EOA deployment this skew is accepted and documented, with a different fix reserved for future higher fan-out scenarios.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[zhangchiqing]

wonder if it's useful to include the highestSent nonce in the error message for debugging purpose, or would it become a risk that is leaking the internal state data?

[vishalchangrani]

lemme investigate. see what other chains do

[vishalchangrani]

Good question. Short answer: the account nonce is public (eth_getTransactionCount), so it isn't a state-leak — and geth/Erigon/Reth all include both the tx nonce and the next/expected nonce directly in the client-facing error. So this is worth doing, but it's broader than just the in-flight case (it applies to ErrNonceTooLow / ErrNonceTooHigh too, which are also bare sentinels today).

Rather than scope it into this PR, I've opened #975 to improve nonce-rejection error reporting across the board (include tx nonce + next/pending nonce in the error and logs, following the geth/Erigon/Reth precedent). Let's handle this comment there.

[vishalchangrani]

you actually pointed out to a broader issue. Other clients return the tx nonce and the on-chain nonce as part of the error. I am creating a new issue to improve error reporting and will handle that as a separate PR.

here is what other clients do:

Every major execution client includes the nonce values in the client-facing error. Verified from source:

• geth — nonce too low: next nonce 6, tx nonce 5 (core/txpool/validation.go); state layer adds address … tx: … state: …. The %w-wrapped string reaches the RPC client.
• Erigon — same format (nonce too low: address 0x… tx: 150 state: 161).
• Reth — transaction nonce is not consistent: next nonce 6, tx nonce 5.
• Nethermind — the lone outlier: bare "nonce too low", no numbers, in both the error and the log.

[zhangchiqing]

Suggested change

	if nonce < n.localIndexedNonce {
	if nonce <= n.localIndexedNonce {

I think nonce has to be strictly above local indexed nonce, right?

[vishalchangrani]

localIndexedNonce is the account's next expected nonce (what GetNonce returns), not the last-used one so nonce == localIndexedNonce is the valid next nonce and must be accepted (nonceNextExpected).

[vishalchangrani]

I can rename localIndexedNonce to nextIndexedNonce for clarity if you think localIndexedNonce is not clear.

[vishalchangrani]

added this additional line to explicitly call that out - https://github.com/onflow/flow-evm-gateway/pull/974/changes#diff-8b52571e515d91ade90131a27686cfb9df5fa5bb38aed986c082256511847043R28

[zhangchiqing]

Does maxNonceGap ever change? Can we check maxNonceGap in the constructor of n or even the mempool?

Suggested change

	if n.maxNonceGap > 0 && nonce-n.localIndexedNonce > n.maxNonceGap {
	if nonce-n.localIndexedNonce > n.maxNonceGap {

[vishalchangrani]

n.maxNonceGap never changes. lemme see if it can be moved out from here.

[vishalchangrani]

ok found a cleaner way to not have n.maxNonceGap > 0 in this condition all together.

[vishalchangrani]

this commit sets maxNonceGap to max value is user specified a 0 for it. That way we avoid checking for n.maxNonceGap > 0 all together.

[zhangchiqing]

We need to be specific that the only case that could reach here is nonce Queued or nonceNextExpected.

if ! (verdict == nonceQueued || verdict == nonceNextExpected) {
  return fmt.Errorf("unknown verdict: %v", verdict)
}

[vishalchangrani]

there is an if verdict == nonceNextExpected && condition for the if block this statement is part of which ensures this will never be executed if verdict was anything other than nonceNextExpected. However, I am refactoring the code to make easy to read by introducing a switch statement for each of the verdicts. For unhandled verdict, I decided to just panic 🤷

[vishalchangrani]

added this switch statement to keep all cases separate and easy to read - https://github.com/onflow/flow-evm-gateway/pull/974/changes#diff-eed503b56e9f1201642b8320513667c59e990d2853507fd4a96d1fa5538d215fR582-R623

[zhangchiqing]

Looks like we don't update lastSubmittedAt when submit failed.

IMO, I think we could update regardless submission succeed or not, so that we won't keep sending the tx over and over if it keeps failing, a failed submission would still have to wait for spacingElapsed for next attempt.

[vishalchangrani]

A failed fast-path submit is usually a Flow-side issue (e.g. Access node), not the tx's fault, and the error is returned to the client synchronously. Stamping lastSubmittedAt on failure would make the EOA's next (possibly valid) tx wait out the spacing for a failure it didn't cause. Retries are the client's call here (we don't retry internally), so I'd rather not rate-limit the EOA on a failed attempt.

(remember this is the fast path where transactions is sent out right away)

[zhangchiqing]

⚠️ the mutex was released and locked again, so we need to check the existing state variable before setting these state variables. Otherwise, it's overwriting updated state value.

[vishalchangrani]

added calls to re-read before attempting to set submitting

[vishalchangrani]

https://github.com/onflow/flow-evm-gateway/pull/974/changes#diff-eed503b56e9f1201642b8320513667c59e990d2853507fd4a96d1fa5538d215fR363-R409

[zhangchiqing]

since we have localIndexedNonce, can inFlight be derived from localIndexedNonce + 1 == txs[0].nonce?

For TTL-expiry batch, inFlight will be false, and for consecutive batch, it will be true.

[vishalchangrani]

but localIndexNonce can be well behind the current highestSent + 1 or might have advanced by Add separately.
I have renamed this inFlight field in flusWork to needsReconcile since thats what it actually is for - to tell reconcileSubmission() whether or not to reconcile or ignore.

[zhangchiqing]

Nice 👍 Looks good to me now. Just left a few suggestion to comments and variable renaming.

[zhangchiqing]

This has been confusing.

if GetNonce is returning next nonce. Then the following is wrong, because refreshIndexed should be refreshNextNonce. The word Indexed usually mean the nonce has been seen and indexed (used) on chain. If it's used as next expected, it would cause confusion:

	indexNonce, err := np.GetNonce(from)
	if err != nil {
		return 0, fmt.Errorf("reading indexed nonce for %s: %w", from.Hex(), err)
	}
	n.refreshIndexed(indexNonce)

• Maybe just rename it to GetNextNonce to be explicit.
• And also rename refreshIndexed to refreshNextNonce

[vishalchangrani]

renamed to "nextNonce" - 60b0bb6