From de58a9e20e6a500524f9492170af28a65e7bb476 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 16:37:43 -0400 Subject: [PATCH 01/31] feat(requester): add GetBlockView to NonceProvider PR #971 review (m-Peter): expose the EVM block view so a caller reading many EOAs' nonces at once (a mempool flush tick) can build the expensive view once and reuse it, instead of rebuilding it per address. - Add NonceView interface (GetNonce at a fixed state) and GetBlockView() to NonceProvider; LocalNonceProvider.GetNonce now builds the view via GetBlockView (DRY). NonceView is an interface so tests can fake it. - Test fake returns itself as the NonceView. The per-tick shared-view usage in collectDueBatches lands with the collectDueBatches rewrite in the next commit. Co-Authored-By: Claude Opus 4.8 --- services/requester/nonce_provider.go | 30 +++++++++++++++++++++++++-- services/requester/tx_mempool_test.go | 7 +++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/services/requester/nonce_provider.go b/services/requester/nonce_provider.go index 435a55e5..2125bcee 100644 --- a/services/requester/nonce_provider.go +++ b/services/requester/nonce_provider.go @@ -10,6 +10,15 @@ import ( "github.com/onflow/flow-evm-gateway/storage/pebble" ) +// NonceView reads EOA nonces at a single, fixed EVM state (one built block +// view). The mempool reads many EOAs' nonces from one view per flush tick +// rather than rebuilding the (expensive) view per address. It is an interface +// so tests can fake it without constructing a real query.View. +type NonceView interface { + // GetNonce returns the nonce of the given EOA at this view's state. + GetNonce(address gethCommon.Address) (uint64, error) +} + // NonceProvider returns the current nonce of the given EOA address. // The transaction mempool uses it to determine the expected next nonce. type NonceProvider interface { @@ -21,6 +30,13 @@ type NonceProvider interface { // as a hard failure (reject the transaction / abort the operation) // rather than a routine, recoverable condition to swallow. GetNonce(address gethCommon.Address) (uint64, error) + + // GetBlockView builds a NonceView over the latest indexed EVM state. A + // caller that reads many EOAs' nonces at once (e.g. a single mempool flush + // tick) can build the view once and reuse it, instead of paying the view + // build cost per address as GetNonce does. A non-nil error is an + // EXCEPTION, same contract as GetNonce. + GetBlockView() (NonceView, error) } // LocalNonceProvider reads the EOA nonce from the latest height of the @@ -45,10 +61,11 @@ func NewLocalNonceProvider( } } -func (p *LocalNonceProvider) GetNonce(address gethCommon.Address) (uint64, error) { +// GetBlockView builds a NonceView over the latest indexed EVM height. +func (p *LocalNonceProvider) GetBlockView() (NonceView, error) { height, err := p.blocks.LatestEVMHeight() if err != nil { - return 0, err + return nil, err } viewProvider := query.NewViewProvider( @@ -60,6 +77,15 @@ func (p *LocalNonceProvider) GetNonce(address gethCommon.Address) (uint64, error ) view, err := viewProvider.GetBlockView(height) + if err != nil { + return nil, err + } + + return view, nil +} + +func (p *LocalNonceProvider) GetNonce(address gethCommon.Address) (uint64, error) { + view, err := p.GetBlockView() if err != nil { return 0, err } diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index c4bd83a1..a4d8bc3b 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -111,6 +111,13 @@ func (f *fakeNonceProvider) GetNonce(_ gethCommon.Address) (uint64, error) { return f.nonce, f.err } +// GetBlockView returns the fake itself as the NonceView: a single fake reads +// every EOA's nonce as f.nonce/f.err, mirroring the production single-view +// read path. +func (f *fakeNonceProvider) GetBlockView() (NonceView, error) { + return f, nil +} + func newTestPool( np NonceProvider, submit func(context.Context, []heldTx) error, From 9cce00c95ce8c0275c10f69a8d2d2b8e2ca9fc8c Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 16:48:56 -0400 Subject: [PATCH 02/31] refactor(requester): replace hasInFlight with an explicit nonceTracker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #971 call w/ zhangchiqing: remove the hard-to-reason-about hasInFlight bool and track submission state as explicit nonce facts, so Add/collectDueBatches call well-named methods instead of poking raw fields and writing compound conditions. Introduce nonceTracker (nested in eoaQueue), holding three facts: - localIndexedNonce: cached on-chain frontier (refreshed from a fresh read) - submitting: highest nonce sent but not yet ack'd - lastConsecutivelySubmitted: highest nonce successfully, consecutively ack'd The latter two are optionalNonce (value + set flag): nonce 0 is a valid nonce, so a bare uint64 cannot represent "absent". optionalNonce owns the comparison logic (atLeast/is/max, treating unset as -∞) so callers carry no set checks. nonceTracker exposes classify(nonce) -> {nextExpected, inFlight, queue}, expectedNonce(), and the transitions markSubmitting/markSubmitted/ rollbackSubmitting/refreshIndexed. Add's reject+fast-path chain collapses onto classify; the fragile `lastSubmittedNonce == batchMax` rollback guard is gone. Explicit success update (per the call): on a successful detached submission reconcileSubmission advances submitted under the lock, rather than waiting for the index to confirm — so `submitting` strictly means "network call outstanding" (documented on markSubmitted). On failure it clears submitting, preventing the permanent-wedge. collectDueBatches now builds the block view once per tick (via the new GetBlockView) and reads every due EOA's nonce from it. Added dedicated nonceTracker unit tests (classify table, expectedNonce, transitions). Unit + e2e suites pass. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 340 ++++++++++++++++++-------- services/requester/tx_mempool_test.go | 137 +++++++---- 2 files changed, 333 insertions(+), 144 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 811c67e9..8a69ff21 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -19,6 +19,11 @@ import ( "github.com/onflow/flow-evm-gateway/services/requester/keystore" ) +// TODO: Document what the TX Mempool does with clear examples of the difference use cases it handles. e.g. transactions coming in out of sequence with a gap in between, transactions expiring in the queue as local index nonce moves beyond the nonce of the the tx. +// TODO: Overall, refactor the code such that its easier to understand and more importantly easier to maintain +// TODO: Log when transactions are being discarded from the queue to help debug issues if lets say a client compains about transactions being lost or erroring out. +// TODO: Add max gap in nonce similar to whats here:https://github.com/onflow/flow-evm-gateway/pull/925/changes#diff-f17ac7d49cb8f2272b61813e130583e6fadbfde51b795bfdb255b936ee6e0c46R221. The range should be min(local index, last nonce submitted) for nonce too high error and max(local index, last nonce submitted) for nonce too low error + // heldTx is a transaction held in the mempool, waiting for its // collection window to elapse or its nonce gap to be filled. type heldTx struct { @@ -75,6 +80,148 @@ const txMemPoolTickInterval = 50 * time.Millisecond // recent activity is kept before being removed, to bound memory usage. const idleQueueRetention = time.Minute +// optionalNonce represents a nonce that may not be set: when set is true, v is a +// valid nonce; when set is false, the nonce has not been initialized yet. It +// lets us compare nonces uniformly even when one may be absent — including the +// ambiguous case where the value is 0 (a valid nonce) but set is false. An unset +// optionalNonce behaves as -∞ in the comparisons below (atLeast/is/max): it is +// below every real nonce and never "at or above" one, so callers carry no set +// checks. +type optionalNonce struct { + v uint64 + set bool +} + +func knownNonce(v uint64) optionalNonce { return optionalNonce{v: v, set: true} } + +// atLeast reports whether this nonce is set and >= n. An unset nonce (-∞) is +// never >= a real nonce, so it returns false. +func (o optionalNonce) atLeast(n uint64) bool { return o.set && o.v >= n } + +// is reports whether this nonce is set and exactly equals n. +func (o optionalNonce) is(n uint64) bool { return o.set && o.v == n } + +// max returns the greater of two optional nonces, treating unset as -∞. +func (o optionalNonce) max(other optionalNonce) optionalNonce { + if !o.set { + return other + } + if other.set && other.v > o.v { + return other + } + return o +} + +// nonceVerdict is what classify decides should happen to an incoming nonce. +type nonceVerdict int + +const ( + // nonceNextExpected: nonce == expectedNonce; eligible for immediate submit. + nonceNextExpected nonceVerdict = iota + // nonceInFlight: nonce is at or below one we have already sent (still in + // flight or already ack'd). Re-accepting it would burn Flow fees on a + // guaranteed nonce-mismatch, so it is rejected. + nonceInFlight + // nonceQueue: a future nonce beyond the expected one (a gap ahead); hold it. + nonceQueue +) + +// nonceTracker is the per-EOA submission-state machine. It records the nonce +// facts the mempool reasons about and answers "what should happen to an +// incoming nonce?" (classify) and "what is the next nonce to submit?" +// (expectedNonce), so callers never compare raw fields or write compound +// conditions. All methods assume the pool's queueMux is held: the tracker has +// no lock of its own, and the single submit goroutine plus Add-under-lock model +// guarantees serialized access (see TxMemPool docstring). +type nonceTracker struct { + // localIndexedNonce is the EOA's next expected nonce per the local state + // index (the on-chain frontier). A CACHE refreshed from a fresh read via + // refreshIndexed — a fact about the chain, not about our sends. + localIndexedNonce uint64 + // submitting is the highest nonce SENT to Flow but not yet ack'd: the window + // between collecting a batch and its submit result. Unset when no submission + // is outstanding. It means strictly "a network call is in flight" and is + // cleared the moment that call returns — by markSubmitted on success or + // rollbackSubmitting on failure. + submitting optionalNonce + // lastConsecutivelySubmitted is the highest nonce CONSECUTIVELY, SUCCESSFULLY + // submitted (ack'd). Unset before the EOA's first success. Only consecutive + // submissions advance it; TTL-expiry (gapped) batches do NOT, so it never + // includes a nonce past a gap. + lastConsecutivelySubmitted optionalNonce +} + +// inFlight reports whether a submission is outstanding (sent, not yet ack'd). +func (n *nonceTracker) inFlight() bool { return n.submitting.set } + +// highestSent returns the highest nonce we have already sent — whether still in +// flight or already ack'd (unset if neither). Re-accepting a nonce at or below +// it would burn Flow fees on a guaranteed nonce-mismatch. +func (n *nonceTracker) highestSent() optionalNonce { + return n.lastConsecutivelySubmitted.max(n.submitting) +} + +// expectedNonce is the next nonce eligible for submission: one past the highest +// nonce already sent, or the indexed frontier, whichever is higher. +func (n *nonceTracker) expectedNonce() uint64 { + if hi := n.highestSent(); hi.atLeast(n.localIndexedNonce) { + return hi.v + 1 + } + return n.localIndexedNonce +} + +// classify decides what to do with an incoming nonce against the currently +// cached frontier. Refresh via refreshIndexed first where freshness matters. +func (n *nonceTracker) classify(nonce uint64) nonceVerdict { + // At or below the highest nonce we've already sent: already in flight or + // submitted, so reject. + if n.highestSent().atLeast(nonce) { + return nonceInFlight + } + if nonce == n.expectedNonce() { + return nonceNextExpected + } + return nonceQueue +} + +// markSubmitting records that nonces up to highNonce have been sent but not yet +// ack'd. Set when a batch is detached for async submission in collectDueBatches +// (the synchronous fast path in Add skips it — it holds the lock across the +// whole submit, so there is no concurrency window to guard). +func (n *nonceTracker) markSubmitting(highNonce uint64) { + n.submitting = knownNonce(highNonce) +} + +// markSubmitted acks a successful submission: it advances the consecutively- +// submitted nonce and clears the in-flight marker. Called the moment a +// submission succeeds, EXPLICITLY and under the lock, rather than waiting for +// the index to confirm — so `submitting` strictly means "a network call is +// outstanding" and is cleared as soon as the call returns (here on success, or +// via rollbackSubmitting on failure). This costs one quick lock per successful +// submission, which we accept for a state machine that is trivial to reason +// about. +func (n *nonceTracker) markSubmitted(highNonce uint64) { + n.lastConsecutivelySubmitted = knownNonce(highNonce) + n.submitting = optionalNonce{} +} + +// rollbackSubmitting clears the in-flight marker after a FAILED submission, but +// only when it still refers to highNonce (a newer submission may have replaced +// it). Because a failure never advances lastConsecutivelySubmitted, there is +// nothing else to undo: the next flush recomputes expectedNonce from the +// unchanged frontier. This replaces the old, fragile +// "lastSubmittedNonce == batchMax" guard. +func (n *nonceTracker) rollbackSubmitting(highNonce uint64) { + if n.submitting.is(highNonce) { + n.submitting = optionalNonce{} + } +} + +// refreshIndexed updates the cached on-chain frontier from a fresh index read. +func (n *nonceTracker) refreshIndexed(indexedNonce uint64) { + n.localIndexedNonce = indexedNonce +} + // eoaQueue tracks the held transactions and submission state for one EOA. type eoaQueue struct { // txs holds pending transactions keyed by nonce. Keying by nonce gives @@ -90,19 +237,15 @@ type eoaQueue struct { // deliberately no separate "hard cap" knob: TxSubmissionSpacing serves // both purposes (see PR #965 discussion). flushDeadline time.Time - // lastSubmittedAt is when the last Cadence tx for this EOA was submitted. + // lastSubmittedAt is when the last Cadence tx for this EOA was submitted + // (used for submission spacing). lastSubmittedAt time.Time - // lastSubmittedNonce is the highest nonce included in the last submission. - // Only meaningful while hasInFlight is true. (Kept next to lastSubmittedAt: - // "submitted" and "sent" mean the same action here.) - lastSubmittedNonce uint64 // lastActivity is when this EOA was last touched — a transaction received // (Add) or a batch flushed (collectDueBatches). It bounds memory: a queue // with no held txs and no activity past idleQueueRetention is removed. lastActivity time.Time - // hasInFlight reports whether a submission exists that the local index - // has not yet confirmed (index nonce <= lastSubmittedNonce). - hasInFlight bool + // nonces is the submission-state machine for this EOA. + nonces nonceTracker } // isEmpty reports whether the queue holds no transactions. Callers must hold @@ -220,11 +363,18 @@ func (t *TxMemPool) Add( q, ok := t.queues[from] if !ok { + // A fresh queue's other fields are intentionally left at their zero + // values: the nonceTracker's optionalNonce fields read as "unset" (nonce 0 + // is not mistaken for a real submission), and the timing fields + // (collectionWindowEndsAt/flushDeadline/lastSubmittedAt) are only ever + // read after being set on the first enqueue or submission below. q = &eoaQueue{txs: make(map[uint64]heldTx)} t.queues[from] = q } now := time.Now() + // The EOA was "touched" even if this turns out to be a duplicate, so record + // activity here to keep the idle-queue retention clock accurate. q.lastActivity = now userTx := heldTx{ @@ -235,34 +385,26 @@ func (t *TxMemPool) Add( } // Reject obvious cases before reading the index nonce — each read builds a - // full block view, so when we already know we will reject the transaction - // we must not pay that cost. + // full block view, so when we already know we will reject we must not pay + // that cost. // Reject an exact duplicate of a transaction already in the queue. if existing, ok := q.txs[tx.Nonce()]; ok && existing.txHash == tx.Hash() { return errs.ErrDuplicateTransaction } - // Reject a nonce that has been submitted and is still in flight: it - // would burn Flow fees on a guaranteed nonce-mismatch failure. A nonce at - // or below lastSubmittedNonce while hasInFlight is inherently in flight, so - // rejecting it here is correct. - // - // Note (zhangchiqing): ErrInFlightNonce covers a nonce at/below the last - // in-flight nonce. A nonce strictly below the indexed (already-used) nonce - // is NOT separately distinguished here — doing so would require an extra - // index read on every Add. Such a transaction is instead pruned by - // pruneStaleTxs on the background loop, or fails observably on-chain. - if q.hasInFlight && tx.Nonce() <= q.lastSubmittedNonce { + // Reject a nonce we have already sent (in flight or ack'd): resubmitting it + // would burn Flow fees on a guaranteed nonce-mismatch. classify answers this + // from the cached frontier, with no index read. + if q.nonces.classify(tx.Nonce()) == nonceInFlight { return errs.ErrInFlightNonce } - // Read the index nonce — an expensive operation that builds a full block - // view — at most once per Add, and only when it can change the decision: - // to clear a stale in-flight marker, and/or to evaluate the fast path for - // an empty, spacing-satisfied queue. - if q.hasInFlight || (q.isEmpty() && t.spacingElapsed(q, now)) { - indexNonce, nonceErr := q.queryAndRefreshInFlight(t.nonceProvider, from) + // Read the index nonce — an expensive full-block-view build — only when it + // can change the decision: while a submission is outstanding, or to evaluate + // the fast path for an empty, spacing-satisfied queue. + if q.nonces.inFlight() || (q.isEmpty() && t.spacingElapsed(q, now)) { + indexNonce, nonceErr := t.nonceProvider.GetNonce(from) if nonceErr != nil { // A nonce lookup failure is an exception, not an expected // condition: this is a local state-index read that should not @@ -271,27 +413,25 @@ func (t *TxMemPool) Add( // it through the queue path. return nonceErr } + q.nonces.refreshIndexed(indexNonce) // We deliberately do NOT prune stale txs (nonce < indexNonce) here, // even though the index may have just advanced: pruning is deferred to // collectDueBatches, which walks every queued tx anyway, so repeating // it per-Add would be redundant work. - // Fast path: the queue is empty, nothing is in flight (the marker may - // have just been cleared above), spacing is satisfied and this tx is - // exactly the next expected nonce. Submit right away — zero added - // latency for the common case. - if q.isEmpty() && !q.hasInFlight && - t.spacingElapsed(q, now) && tx.Nonce() == indexNonce { + // Fast path: empty queue, nothing in flight, spacing satisfied, and this + // tx is exactly the next expected nonce — submit immediately, zero added + // latency for the common case. The lock is held across the whole submit, + // so there is no concurrency window: no need to mark "submitting" first; + // on success we record the ack, on failure we leave the EOA untouched. + if q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) && + q.nonces.classify(tx.Nonce()) == nonceNextExpected { if submitErr := t.submitBatch(ctx, []heldTx{userTx}); submitErr != nil { - // Submission failed: leave queue state untouched so the EOA - // is neither marked in flight nor rate-limited behind a tx - // that never landed. return submitErr } + q.nonces.markSubmitted(tx.Nonce()) q.lastSubmittedAt = time.Now() - q.lastSubmittedNonce = tx.Nonce() - q.hasInFlight = true return nil } // On an unexpected nonce, fall through to the queue path. @@ -312,30 +452,6 @@ func (t *TxMemPool) Add( return nil } -// refreshInFlight clears the in-flight marker once the local index has -// advanced past the last submitted nonce. Callers must hold the pool's -// queueMux. -func (q *eoaQueue) refreshInFlight(indexNonce uint64) { - if q.hasInFlight && indexNonce > q.lastSubmittedNonce { - q.hasInFlight = false - } -} - -// queryAndRefreshInFlight reads the EOA's current nonce from the local index -// and clears the in-flight marker if the index has advanced past the last -// submitted nonce, returning the index nonce. Callers must hold queueMux. -func (q *eoaQueue) queryAndRefreshInFlight( - np NonceProvider, - from gethCommon.Address, -) (uint64, error) { - indexNonce, err := np.GetNonce(from) - if err != nil { - return 0, err - } - q.refreshInFlight(indexNonce) - return indexNonce, nil -} - // spacingElapsed reports whether enough time has passed since the last // Cadence submission for this EOA. Callers must hold queueMux. func (t *TxMemPool) spacingElapsed(q *eoaQueue, now time.Time) bool { @@ -348,11 +464,11 @@ func (t *TxMemPool) spacingElapsed(q *eoaQueue, now time.Time) bool { type flushWork struct { from gethCommon.Address txs []heldTx - // inFlight is true for consecutive-prefix batches, which optimistically - // advance lastSubmittedNonce/hasInFlight on the queue and must therefore - // roll those back if the submission fails (see rollbackFailedSubmission). - // TTL-expiry batches are not marked in flight and must never clear the - // marker. + // inFlight is true for consecutive-prefix batches, which mark the queue's + // nonceTracker "submitting" and must therefore reconcile it once the submit + // returns (markSubmitted on success, rollbackSubmitting on failure — see + // reconcileSubmission). TTL-expiry batches are not marked in flight and must + // never touch the tracker. inFlight bool } @@ -377,36 +493,41 @@ func (t *TxMemPool) processQueues(ctx context.Context) { } } -// submitWork submits one detached batch. On failure it rolls back the state -// the queue committed optimistically when the batch was collected; on success -// there is nothing to do — that optimistic state already reflects the -// submission. +// submitWork submits one detached batch and reconciles the queue's nonce state +// once the network call returns. func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { err := t.submitBatch(ctx, w.txs) - if err != nil { - t.rollbackFailedSubmission(w) - } + t.reconcileSubmission(w, err) return err } -// rollbackFailedSubmission re-opens an EOA after a failed flush. -// collectDueBatches optimistically marks a consecutive-prefix batch in flight -// and advances lastSubmittedNonce BEFORE the network call. If that call fails, -// the batch's transactions are dropped (already counted and logged by -// submitBatch) and never reach the chain — so without this rollback every -// resubmission of those nonces would be rejected with ErrInFlightNonce, and the -// index would never advance past them to clear the marker: the EOA would be -// permanently wedged. +// reconcileSubmission updates the EOA's nonceTracker after a detached +// consecutive-prefix submission returns. collectDueBatches marked the batch +// "submitting" (under the lock) before the network call; this records the +// outcome: +// +// - On SUCCESS we explicitly advance the consecutively-submitted nonce now +// (markSubmitted), under the lock, rather than waiting for the index to +// confirm. This is the deliberate "update on success" decision: it keeps +// `submitting` meaning strictly "a network call is outstanding". // -// The marker is cleared only when it still belongs to the failed batch — a -// newer submission may have replaced it while the failed one was on the wire. -// Only in-flight (prefix) batches are rolled back; TTL-expiry batches never set -// the marker. +// - On FAILURE the batch's transactions are dropped (already counted and +// logged by submitBatch) and never reach the chain, so we clear the +// "submitting" marker (rollbackSubmitting). Without this, every resubmission +// of those nonces would be rejected as in-flight forever and the index would +// never advance to clear the marker — the EOA would be permanently wedged. +// rollbackSubmitting clears the marker only if it still refers to this +// batch (a newer submission may have replaced it). lastSubmittedAt is +// deliberately NOT restored: a brief, self-correcting spacing delay after a +// rare failure is harmless. // -// lastSubmittedAt is deliberately NOT restored: a brief, self-correcting -// spacing delay after a rare submission failure is harmless and not worth the -// extra bookkeeping. -func (t *TxMemPool) rollbackFailedSubmission(w flushWork) { +// TTL-expiry batches (w.inFlight == false) never mark the tracker, so there is +// nothing to reconcile for them. +func (t *TxMemPool) reconcileSubmission(w flushWork, submitErr error) { + if !w.inFlight { + return + } + t.queueMux.Lock() defer t.queueMux.Unlock() @@ -414,9 +535,13 @@ func (t *TxMemPool) rollbackFailedSubmission(w flushWork) { if !ok { return } - if w.inFlight && q.hasInFlight && q.lastSubmittedNonce == w.txs[len(w.txs)-1].nonce { - q.hasInFlight = false + + highNonce := w.txs[len(w.txs)-1].nonce + if submitErr != nil { + q.nonces.rollbackSubmitting(highNonce) + return } + q.nonces.markSubmitted(highNonce) } // collectDueBatches selects, under the queue lock, every batch that is due @@ -429,6 +554,21 @@ func (t *TxMemPool) collectDueBatches() []flushWork { now := time.Now() work := make([]flushWork, 0) + // Build the EVM block view at most once per tick and read every due EOA's + // nonce from it: building the view is expensive, so we avoid rebuilding it + // per address. Built lazily on first need so an all-idle tick does no work. + var blockView NonceView + indexNonceOf := func(addr gethCommon.Address) (uint64, error) { + if blockView == nil { + v, err := t.nonceProvider.GetBlockView() + if err != nil { + return 0, err + } + blockView = v + } + return blockView.GetNonce(addr) + } + for from, q := range t.queues { if q.isEmpty() { // Bound memory: drop queues with no held txs and no activity past @@ -453,7 +593,7 @@ func (t *TxMemPool) collectDueBatches() []flushWork { continue } - indexNonce, err := t.nonceProvider.GetNonce(from) + indexNonce, err := indexNonceOf(from) if err != nil { // Exception: a local state-index nonce read should not fail // under normal operation. This is a background loop with no @@ -465,18 +605,13 @@ func (t *TxMemPool) collectDueBatches() []flushWork { continue } - q.refreshInFlight(indexNonce) + q.nonces.refreshIndexed(indexNonce) // Prune transactions that can never execute: their nonce is already // used on-chain (e.g. filled via another gateway). They would only // burn fees at TTL expiry. t.pruneStaleTxs(q, from, indexNonce) - expected := indexNonce - if q.hasInFlight && q.lastSubmittedNonce+1 > expected { - expected = q.lastSubmittedNonce + 1 - } - // At most one batch is collected per EOA per tick. The consecutive // prefix below takes precedence and `continue`s; only when there is no // eligible prefix (a gap at the head) do we consider the TTL-expiry @@ -484,15 +619,16 @@ func (t *TxMemPool) collectDueBatches() []flushWork { // drained on a later tick, gated by submission spacing — it is never // merged with this batch, since a head gap would make the whole Flow // transaction fail. - prefix := selectConsecutivePrefix(q.txs, expected, t.config.TxMaxBatchSize) + prefix := selectConsecutivePrefix(q.txs, q.nonces.expectedNonce(), t.config.TxMaxBatchSize) if len(prefix) > 0 { for _, htx := range prefix { delete(q.txs, htx.nonce) } - q.lastSubmittedNonce = prefix[len(prefix)-1].nonce + // Optimistically mark the batch submitting; reconcileSubmission + // advances submitted on success or clears it on failure. + q.nonces.markSubmitting(prefix[len(prefix)-1].nonce) q.lastSubmittedAt = now q.lastActivity = now - q.hasInFlight = true if !q.isEmpty() { // Re-arm for the remaining (post-gap or over-cap) txs. q.collectionWindowEndsAt = now.Add(t.config.TxCollectionWindow) diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index a4d8bc3b..f8c39bc6 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -198,8 +198,9 @@ func Test_TxMemPool_FastPathSubmitsImmediately(t *testing.T) { q := pool.queues[from] require.NotNil(t, q) assert.Empty(t, q.txs) - assert.True(t, q.hasInFlight) - assert.Equal(t, uint64(0), q.lastSubmittedNonce) + // Explicit success update: submitting cleared, submitted advanced to 0. + assert.False(t, q.nonces.inFlight()) + assert.Equal(t, knownNonce(0), q.nonces.lastConsecutivelySubmitted) } func Test_TxMemPool_UnexpectedNonceEnqueues(t *testing.T) { @@ -226,7 +227,7 @@ func Test_TxMemPool_UnexpectedNonceEnqueues(t *testing.T) { held, ok := q.txs[5] require.True(t, ok) assert.Equal(t, tx.Hash(), held.txHash) - assert.False(t, q.hasInFlight) + assert.False(t, q.nonces.inFlight()) } func Test_TxMemPool_NonceReadErrorRejectsTx(t *testing.T) { @@ -255,7 +256,7 @@ func Test_TxMemPool_NonceReadErrorRejectsTx(t *testing.T) { q := pool.queues[from] require.NotNil(t, q) assert.Empty(t, q.txs) - assert.False(t, q.hasInFlight) + assert.False(t, q.nonces.inFlight()) } func Test_TxMemPool_InFlightDuplicateRejected(t *testing.T) { @@ -308,20 +309,20 @@ func Test_TxMemPool_FailedFlushDoesNotWedgeEOA(t *testing.T) { // State was committed optimistically under the lock. q := pool.queues[from] - require.True(t, q.hasInFlight) - assert.Equal(t, uint64(1), q.lastSubmittedNonce) + require.True(t, q.nonces.inFlight()) + assert.Equal(t, knownNonce(1), q.nonces.submitting) - // The submission fails; submitWork must reconcile the in-flight marker. + // The submission fails; submitWork must clear the in-flight marker. err = pool.submitWork(context.Background(), work[0]) require.ErrorIs(t, err, submitErr) - assert.False(t, q.hasInFlight) + assert.False(t, q.nonces.inFlight()) // A resubmission of the failed nonce must NOT be rejected as in flight. err = pool.Add(context.Background(), signedTestTx(t, key, 0, 2)) assert.NotErrorIs(t, err, errs.ErrInFlightNonce) } -func Test_RollbackFailedSubmission_OnlyClearsMatchingBatch(t *testing.T) { +func Test_ReconcileSubmission_OnlyReconcilesMatchingInFlightBatch(t *testing.T) { pool := newTestPool( &fakeNonceProvider{nonce: 0}, func(_ context.Context, _ []heldTx) error { return nil }, @@ -329,39 +330,42 @@ func Test_RollbackFailedSubmission_OnlyClearsMatchingBatch(t *testing.T) { ) from := gethCommon.HexToAddress("0xabc") pool.queues[from] = &eoaQueue{ - txs: map[uint64]heldTx{}, - hasInFlight: true, - lastSubmittedNonce: 7, + txs: map[uint64]heldTx{}, + nonces: nonceTracker{submitting: knownNonce(7)}, } + submitErr := errors.New("network down") - // A different (newer) batch owns the marker: not cleared. - pool.rollbackFailedSubmission( + // A different (newer) in-flight nonce owns the marker: not cleared. + pool.reconcileSubmission( flushWork{from: from, txs: []heldTx{makeHeldTx(5, time.Time{})}, inFlight: true}, + submitErr, ) - assert.True(t, pool.queues[from].hasInFlight) + assert.True(t, pool.queues[from].nonces.inFlight()) - // A TTL-expiry batch (inFlight false) never clears the marker, even if its - // last nonce coincides with lastSubmittedNonce. - pool.rollbackFailedSubmission( + // A TTL-expiry batch (inFlight false) never touches the tracker. + pool.reconcileSubmission( flushWork{from: from, txs: []heldTx{makeHeldTx(7, time.Time{})}, inFlight: false}, + submitErr, ) - assert.True(t, pool.queues[from].hasInFlight) + assert.True(t, pool.queues[from].nonces.inFlight()) // The failed in-flight batch still owns the marker: cleared. - pool.rollbackFailedSubmission( + pool.reconcileSubmission( flushWork{from: from, txs: []heldTx{makeHeldTx(7, time.Time{})}, inFlight: true}, + submitErr, ) - assert.False(t, pool.queues[from].hasInFlight) + assert.False(t, pool.queues[from].nonces.inFlight()) // Unknown EOA: no panic. - pool.rollbackFailedSubmission( + pool.reconcileSubmission( flushWork{from: gethCommon.HexToAddress("0xdef"), txs: []heldTx{makeHeldTx(7, time.Time{})}, inFlight: true}, + submitErr, ) } -// A successful submission leaves the optimistically-committed in-flight state -// intact — there is no success-path reconciliation to undo it. -func Test_TxMemPool_SuccessfulFlushKeepsInFlight(t *testing.T) { +// A successful submission advances the consecutively-submitted nonce and clears +// the in-flight marker (the explicit success update). +func Test_TxMemPool_SuccessfulFlushMarksSubmitted(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) from := crypto.PubkeyToAddress(key.PublicKey) @@ -388,8 +392,8 @@ func Test_TxMemPool_SuccessfulFlushKeepsInFlight(t *testing.T) { require.NoError(t, pool.submitWork(context.Background(), work[0])) q := pool.queues[from] - assert.True(t, q.hasInFlight) - assert.Equal(t, uint64(1), q.lastSubmittedNonce) + assert.False(t, q.nonces.inFlight()) + assert.Equal(t, knownNonce(1), q.nonces.lastConsecutivelySubmitted) } // Fix 1: a failed fast-path submission must not rate-limit the EOA via @@ -411,7 +415,8 @@ func Test_TxMemPool_FailedFastPathLeavesNoState(t *testing.T) { q := pool.queues[from] require.NotNil(t, q) - assert.False(t, q.hasInFlight) + assert.False(t, q.nonces.inFlight()) + assert.False(t, q.nonces.lastConsecutivelySubmitted.set, "failed submission must not advance submitted") assert.True(t, q.lastSubmittedAt.IsZero(), "failed submission must not stamp lastSubmittedAt") } @@ -481,11 +486,11 @@ func Test_TxMemPool_EmptyQueueAgesOut(t *testing.T) { ) from := gethCommon.HexToAddress("0xabc") - // Empty queue, never submitted (lastSubmittedAt zero), but in flight and + // Empty queue, never submitted, but with a lingering in-flight marker and // last active beyond the retention window: must be removed. pool.queues[from] = &eoaQueue{ txs: map[uint64]heldTx{}, - hasInFlight: true, + nonces: nonceTracker{submitting: knownNonce(5)}, lastActivity: time.Now().Add(-2 * idleQueueRetention), } pool.collectDueBatches() @@ -581,18 +586,66 @@ func Test_TxMemPool_CollectDueBatchesReportsSize(t *testing.T) { assert.Equal(t, 3, collector.txPoolQueued) } -func Test_RefreshInFlight(t *testing.T) { - q := &eoaQueue{hasInFlight: true, lastSubmittedNonce: 3} - - // Index has not advanced past the sent nonce: stays in flight. - q.refreshInFlight(3) - assert.True(t, q.hasInFlight) +func Test_NonceTracker_Classify(t *testing.T) { + tests := []struct { + name string + tracker nonceTracker + nonce uint64 + want nonceVerdict + }{ + {"fresh: indexed nonce is next-expected", nonceTracker{localIndexedNonce: 5}, 5, nonceNextExpected}, + {"fresh: gap ahead queues", nonceTracker{localIndexedNonce: 5}, 7, nonceQueue}, + {"fresh: below index queues (pruned later)", nonceTracker{localIndexedNonce: 5}, 3, nonceQueue}, + {"nonce 0 is next-expected on a zero tracker", nonceTracker{}, 0, nonceNextExpected}, + {"submitted: at submitted is in-flight", nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}, 6, nonceInFlight}, + {"submitted: below submitted is in-flight", nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}, 4, nonceInFlight}, + {"submitted: next after submitted is expected", nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}, 7, nonceNextExpected}, + {"submitting: at submitting is in-flight", nonceTracker{localIndexedNonce: 5, submitting: knownNonce(8)}, 8, nonceInFlight}, + {"submitting: next after submitting is expected", nonceTracker{localIndexedNonce: 5, submitting: knownNonce(8)}, 9, nonceNextExpected}, + {"index ahead of submitted: expected follows index", nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: knownNonce(6)}, 10, nonceNextExpected}, + {"index ahead of submitted: between submitted and index queues", nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: knownNonce(6)}, 8, nonceQueue}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, tc.tracker.classify(tc.nonce)) + }) + } +} - // Index advanced past the sent nonce: cleared. - q.refreshInFlight(4) - assert.False(t, q.hasInFlight) +func Test_NonceTracker_ExpectedNonce(t *testing.T) { + assert.Equal(t, uint64(5), (&nonceTracker{localIndexedNonce: 5}).expectedNonce()) + assert.Equal(t, uint64(7), + (&nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}).expectedNonce()) + assert.Equal(t, uint64(9), + (&nonceTracker{localIndexedNonce: 5, submitting: knownNonce(8)}).expectedNonce()) + // The indexed frontier wins when it is ahead of our own sends. + assert.Equal(t, uint64(10), + (&nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: knownNonce(6)}).expectedNonce()) +} - // No-op when nothing is in flight. - q.refreshInFlight(100) - assert.False(t, q.hasInFlight) +func Test_NonceTracker_Transitions(t *testing.T) { + n := &nonceTracker{localIndexedNonce: 5} + + // markSubmitting sets the in-flight marker. + n.markSubmitting(7) + assert.True(t, n.inFlight()) + assert.Equal(t, knownNonce(7), n.submitting) + + // markSubmitted advances submitted and clears submitting. + n.markSubmitted(7) + assert.False(t, n.inFlight()) + assert.Equal(t, knownNonce(7), n.lastConsecutivelySubmitted) + + // rollbackSubmitting only clears a matching in-flight nonce. + n.markSubmitting(9) + n.rollbackSubmitting(8) // non-matching: no-op + assert.True(t, n.inFlight()) + n.rollbackSubmitting(9) // matching: cleared + assert.False(t, n.inFlight()) + // A rollback never disturbs the consecutively-submitted nonce. + assert.Equal(t, knownNonce(7), n.lastConsecutivelySubmitted) + + // refreshIndexed updates the cached frontier. + n.refreshIndexed(12) + assert.Equal(t, uint64(12), n.localIndexedNonce) } From 24a6b3fdcd8ecbf7badfc7555474b4e4d0c7114c Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 17:39:24 -0400 Subject: [PATCH 03/31] feat(requester): reject out-of-range nonces in the mempool Add path PR #971 call w/ zhangchiqing: reject far-future and already-used nonces up front in Add, so a client gets immediate feedback instead of the mempool holding an unexecutable tx until TTL (which then burns Flow fees on a guaranteed nonce-mismatch). - New config TxMaxNonceGap (flag --tx-max-nonce-gap, default 500; 0 disables). Stored on nonceTracker as maxNonceGap, seeded from config at queue creation. - classify now returns nonceTooLow (nonce < indexed frontier) and nonceTooHigh (nonce > indexed frontier + maxNonceGap), gated on maxNonceGap > 0. The in-flight check takes precedence. A behind/stale index can only make this over-strict, which is acceptable (the gateway is catching up). - New errors ErrNonceTooLow / ErrNonceTooHigh. - Add restructured around a single classify switch: reject duplicates (no read), then read the on-chain frontier unless the nonce is already one we've sent (in-flight retries stay read-free), then act on the verdict. Bound is anchored on the local index (Leo: "maxGap + local index"), which is anti-runaway (the index advances only on execution, not on our own sends) and robust when the chain has moved past our sends. Default gap is off in tests, so existing behavior is preserved; new unit tests cover the range verdicts and the Add rejections. Unit + e2e suites pass. Co-Authored-By: Claude Opus 4.8 --- cmd/run/cmd.go | 1 + config/config.go | 8 ++ models/errors/errors.go | 7 ++ services/requester/tx_mempool.go | 138 ++++++++++++++++---------- services/requester/tx_mempool_test.go | 78 ++++++++++++--- 5 files changed, 166 insertions(+), 66 deletions(-) diff --git a/cmd/run/cmd.go b/cmd/run/cmd.go index ca425125..92417152 100644 --- a/cmd/run/cmd.go +++ b/cmd/run/cmd.go @@ -323,6 +323,7 @@ func init() { Cmd.Flags().DurationVar(&cfg.TxSubmissionSpacing, "tx-submission-spacing", 1200*time.Millisecond, "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.") Cmd.Flags().DurationVar(&cfg.TxPoolTTL, "tx-pool-ttl", 30*time.Second, "How long the transaction mempool holds an out-of-order transaction waiting for its nonce gap to fill, before submitting it anyway.") Cmd.Flags().IntVar(&cfg.TxMaxBatchSize, "tx-max-batch-size", 5, "Maximum number of EVM transactions per EVM.batchRun Cadence transaction in the transaction mempool.") + Cmd.Flags().Uint64Var(&cfg.TxMaxNonceGap, "tx-max-nonce-gap", 500, "How far ahead of an EOA's on-chain nonce the transaction mempool accepts a nonce; nonces beyond indexedNonce+gap are rejected as nonce-too-high. 0 means no upper bound. A nonce below the indexed nonce is always rejected as nonce-too-low regardless of this setting.") Cmd.Flags().DurationVar(&cfg.RpcRequestTimeout, "rpc-request-timeout", time.Second*120, "Sets the maximum duration at which JSON-RPC requests should generate a response, before they timeout. The default is 120 seconds.") err := Cmd.Flags().MarkDeprecated("init-cadence-height", "This flag is no longer necessary and will be removed in future version. The initial Cadence height is known for testnet/mainnet and this was only required for fresh deployments of EVM Gateway. Once the DB has been initialized, the latest index Cadence height will be used upon start-up.") diff --git a/config/config.go b/config/config.go index b37597ba..4206b415 100644 --- a/config/config.go +++ b/config/config.go @@ -147,6 +147,14 @@ type Config struct { // single EVM.batchRun Cadence transaction by the transaction mempool, // bounded by the Cadence transaction computation limit. TxMaxBatchSize int + // TxMaxNonceGap is how far ahead of an EOA's on-chain nonce the transaction + // mempool will accept a nonce. A transaction whose nonce exceeds + // indexedNonce + TxMaxNonceGap is rejected up front with ErrNonceTooHigh, + // giving the client immediate feedback instead of holding an unexecutable tx + // until TTL. 0 means no upper bound (any future nonce is accepted). This + // bounds only the upper end: a nonce below the indexed nonce is always + // rejected with ErrNonceTooLow regardless of this setting. + TxMaxNonceGap uint64 // RpcRequestTimeout is the maximum duration at which JSON-RPC requests should generate // a response, before they timeout. RpcRequestTimeout time.Duration diff --git a/models/errors/errors.go b/models/errors/errors.go index 4438ea22..871e8034 100644 --- a/models/errors/errors.go +++ b/models/errors/errors.go @@ -36,6 +36,13 @@ var ( // already been submitted to the network and is awaiting execution. Letting // it through would burn Flow fees on a guaranteed nonce-mismatch failure. ErrInFlightNonce = fmt.Errorf("%w: %s", ErrInvalid, "transaction with the same nonce already submitted") + // ErrNonceTooLow is returned when a transaction's nonce is below the EOA's + // current on-chain nonce: it has already been used and can never execute. + ErrNonceTooLow = fmt.Errorf("%w: %s", ErrInvalid, "nonce too low") + // ErrNonceTooHigh is returned when a transaction's nonce is more than the + // configured maximum gap ahead of the EOA's on-chain nonce. Such a tx cannot + // execute until the gap fills, so it is rejected up front for fast feedback. + ErrNonceTooHigh = fmt.Errorf("%w: %s", ErrInvalid, "nonce too high") // Storage errors diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 8a69ff21..020d87b6 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -22,7 +22,6 @@ import ( // TODO: Document what the TX Mempool does with clear examples of the difference use cases it handles. e.g. transactions coming in out of sequence with a gap in between, transactions expiring in the queue as local index nonce moves beyond the nonce of the the tx. // TODO: Overall, refactor the code such that its easier to understand and more importantly easier to maintain // TODO: Log when transactions are being discarded from the queue to help debug issues if lets say a client compains about transactions being lost or erroring out. -// TODO: Add max gap in nonce similar to whats here:https://github.com/onflow/flow-evm-gateway/pull/925/changes#diff-f17ac7d49cb8f2272b61813e130583e6fadbfde51b795bfdb255b936ee6e0c46R221. The range should be min(local index, last nonce submitted) for nonce too high error and max(local index, last nonce submitted) for nonce too low error // heldTx is a transaction held in the mempool, waiting for its // collection window to elapse or its nonce gap to be filled. @@ -122,7 +121,14 @@ const ( // flight or already ack'd). Re-accepting it would burn Flow fees on a // guaranteed nonce-mismatch, so it is rejected. nonceInFlight - // nonceQueue: a future nonce beyond the expected one (a gap ahead); hold it. + // nonceTooLow: nonce is below the on-chain frontier — already used, can + // never execute. Rejected up front. + nonceTooLow + // nonceTooHigh: nonce is more than maxNonceGap beyond the on-chain frontier; + // it cannot execute until the gap fills, so it is rejected up front. + nonceTooHigh + // nonceQueue: a future nonce beyond the expected one (a gap ahead) but within + // the accepted window; hold it. nonceQueue ) @@ -149,6 +155,11 @@ type nonceTracker struct { // submissions advance it; TTL-expiry (gapped) batches do NOT, so it never // includes a nonce past a gap. lastConsecutivelySubmitted optionalNonce + // maxNonceGap is how far above localIndexedNonce a nonce may be before it is + // rejected as too-high. 0 means no upper bound. It does NOT affect the + // too-low check (a nonce below localIndexedNonce is always rejected). Set + // once from config when the queue is created. + maxNonceGap uint64 } // inFlight reports whether a submission is outstanding (sent, not yet ack'd). @@ -170,18 +181,53 @@ func (n *nonceTracker) expectedNonce() uint64 { return n.localIndexedNonce } -// classify decides what to do with an incoming nonce against the currently -// cached frontier. Refresh via refreshIndexed first where freshness matters. -func (n *nonceTracker) classify(nonce uint64) nonceVerdict { - // At or below the highest nonce we've already sent: already in flight or - // submitted, so reject. +// classify returns the verdict for an incoming nonce, refreshing the cached +// on-chain frontier as part of the decision. +// +// A nonce at or below our highest already-sent nonce is an in-flight/duplicate +// retry, rejected from local state alone — no index read. We check this first +// precisely because reading the frontier builds a block view (the expensive +// step) that the retry case must avoid. +// +// Any other nonce is beyond what we've sent, so the remaining verdicts +// (too-low/too-high/next-expected) need the on-chain frontier: we read and +// refresh it. A read error is an exception (a local state-index read should not +// fail) and is returned so the caller can reject the transaction. Pruning of any +// now-stale queued txs is deferred to collectDueBatches. +// +// Callers must hold queueMux. +func (n *nonceTracker) classify( + nonce uint64, + np NonceProvider, + from gethCommon.Address, +) (nonceVerdict, error) { + // At or below the highest nonce we've already sent: in flight or submitted. if n.highestSent().atLeast(nonce) { - return nonceInFlight + return nonceInFlight, nil + } + + // Beyond what we've sent: refresh the frontier for the remaining verdicts. + indexNonce, err := np.GetNonce(from) + if err != nil { + return 0, err + } + n.refreshIndexed(indexNonce) + + // Below the on-chain frontier: already used, can never execute. Always + // rejected — unrelated to maxNonceGap (it bounds only the upper end). + if nonce < n.localIndexedNonce { + return nonceTooLow, nil + } + // More than maxNonceGap beyond the frontier (only when a gap is configured): + // cannot execute until the gap fills. A behind (stale) index can only make + // this over-strict, which is acceptable — the gateway is catching up. + if n.maxNonceGap > 0 && nonce > n.localIndexedNonce+n.maxNonceGap { + return nonceTooHigh, nil } if nonce == n.expectedNonce() { - return nonceNextExpected + return nonceNextExpected, nil } - return nonceQueue + return nonceQueue, nil } // markSubmitting records that nonces up to highNonce have been sent but not yet @@ -367,8 +413,12 @@ func (t *TxMemPool) Add( // values: the nonceTracker's optionalNonce fields read as "unset" (nonce 0 // is not mistaken for a real submission), and the timing fields // (collectionWindowEndsAt/flushDeadline/lastSubmittedAt) are only ever - // read after being set on the first enqueue or submission below. - q = &eoaQueue{txs: make(map[uint64]heldTx)} + // read after being set on the first enqueue or submission below. Only + // maxNonceGap needs seeding from config. + q = &eoaQueue{ + txs: make(map[uint64]heldTx), + nonces: nonceTracker{maxNonceGap: t.config.TxMaxNonceGap}, + } t.queues[from] = q } @@ -384,49 +434,36 @@ func (t *TxMemPool) Add( enqueuedAt: now, } - // Reject obvious cases before reading the index nonce — each read builds a - // full block view, so when we already know we will reject we must not pay - // that cost. - - // Reject an exact duplicate of a transaction already in the queue. + // Reject an exact duplicate of a transaction already in the queue (cheapest + // check; needs no index read). if existing, ok := q.txs[tx.Nonce()]; ok && existing.txHash == tx.Hash() { return errs.ErrDuplicateTransaction } - // Reject a nonce we have already sent (in flight or ack'd): resubmitting it - // would burn Flow fees on a guaranteed nonce-mismatch. classify answers this - // from the cached frontier, with no index read. - if q.nonces.classify(tx.Nonce()) == nonceInFlight { - return errs.ErrInFlightNonce + // Classify the nonce, reading the on-chain frontier only when needed (see + // classify). A read error is an exception — reject rather than routing + // through the queue path. + verdict, err := q.nonces.classify(tx.Nonce(), t.nonceProvider, from) + if err != nil { + return err } - // Read the index nonce — an expensive full-block-view build — only when it - // can change the decision: while a submission is outstanding, or to evaluate - // the fast path for an empty, spacing-satisfied queue. - if q.nonces.inFlight() || (q.isEmpty() && t.spacingElapsed(q, now)) { - indexNonce, nonceErr := t.nonceProvider.GetNonce(from) - if nonceErr != nil { - // A nonce lookup failure is an exception, not an expected - // condition: this is a local state-index read that should not - // fail under normal operation. The gateway is in an unknown - // state, so reject the transaction rather than silently routing - // it through the queue path. - return nonceErr - } - q.nonces.refreshIndexed(indexNonce) - - // We deliberately do NOT prune stale txs (nonce < indexNonce) here, - // even though the index may have just advanced: pruning is deferred to - // collectDueBatches, which walks every queued tx anyway, so repeating - // it per-Add would be redundant work. - - // Fast path: empty queue, nothing in flight, spacing satisfied, and this - // tx is exactly the next expected nonce — submit immediately, zero added - // latency for the common case. The lock is held across the whole submit, - // so there is no concurrency window: no need to mark "submitting" first; - // on success we record the ack, on failure we leave the EOA untouched. - if q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) && - q.nonces.classify(tx.Nonce()) == nonceNextExpected { + switch verdict { + case nonceInFlight: + return errs.ErrInFlightNonce + case nonceTooLow: + return errs.ErrNonceTooLow + case nonceTooHigh: + return errs.ErrNonceTooHigh + case nonceNextExpected: + // Fast path: an empty queue with nothing in flight and spacing satisfied + // can submit the expected nonce immediately — zero added latency. The + // lock is held across the whole submit, so there is no concurrency + // window: no need to mark "submitting" first; on success we record the + // ack, on failure we leave the EOA untouched. If we cannot fast-path yet + // (queue non-empty, in flight, or spacing not elapsed), fall through to + // enqueue and let the background loop flush it. + if q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { if submitErr := t.submitBatch(ctx, []heldTx{userTx}); submitErr != nil { return submitErr } @@ -434,7 +471,8 @@ func (t *TxMemPool) Add( q.lastSubmittedAt = time.Now() return nil } - // On an unexpected nonce, fall through to the queue path. + case nonceQueue: + // A gap ahead within the accepted window — fall through to enqueue. } // Enqueue. A same-nonce, different-payload resubmission replaces the diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index f8c39bc6..9e31f856 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -587,31 +587,77 @@ func Test_TxMemPool_CollectDueBatchesReportsSize(t *testing.T) { } func Test_NonceTracker_Classify(t *testing.T) { + // classify reads the on-chain frontier from the provider, so the frontier + // is supplied via `frontier` (not set directly on the tracker). In-flight + // cases return before the read, so their frontier value is irrelevant. tests := []struct { - name string - tracker nonceTracker - nonce uint64 - want nonceVerdict + name string + tracker nonceTracker + frontier uint64 + nonce uint64 + want nonceVerdict }{ - {"fresh: indexed nonce is next-expected", nonceTracker{localIndexedNonce: 5}, 5, nonceNextExpected}, - {"fresh: gap ahead queues", nonceTracker{localIndexedNonce: 5}, 7, nonceQueue}, - {"fresh: below index queues (pruned later)", nonceTracker{localIndexedNonce: 5}, 3, nonceQueue}, - {"nonce 0 is next-expected on a zero tracker", nonceTracker{}, 0, nonceNextExpected}, - {"submitted: at submitted is in-flight", nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}, 6, nonceInFlight}, - {"submitted: below submitted is in-flight", nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}, 4, nonceInFlight}, - {"submitted: next after submitted is expected", nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}, 7, nonceNextExpected}, - {"submitting: at submitting is in-flight", nonceTracker{localIndexedNonce: 5, submitting: knownNonce(8)}, 8, nonceInFlight}, - {"submitting: next after submitting is expected", nonceTracker{localIndexedNonce: 5, submitting: knownNonce(8)}, 9, nonceNextExpected}, - {"index ahead of submitted: expected follows index", nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: knownNonce(6)}, 10, nonceNextExpected}, - {"index ahead of submitted: between submitted and index queues", nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: knownNonce(6)}, 8, nonceQueue}, + {"indexed nonce is next-expected", nonceTracker{}, 5, 5, nonceNextExpected}, + {"gap ahead queues", nonceTracker{}, 5, 7, nonceQueue}, + {"below index is too low (no gap configured)", nonceTracker{}, 5, 3, nonceTooLow}, + {"nonce 0 is next-expected on a zero tracker", nonceTracker{}, 0, 0, nonceNextExpected}, + {"at submitted is in-flight", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 5, 6, nonceInFlight}, + {"below submitted is in-flight", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 5, 4, nonceInFlight}, + {"next after submitted is expected", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 5, 7, nonceNextExpected}, + {"at submitting is in-flight", nonceTracker{submitting: knownNonce(8)}, 5, 8, nonceInFlight}, + {"next after submitting is expected", nonceTracker{submitting: knownNonce(8)}, 5, 9, nonceNextExpected}, + {"index ahead of submitted: expected follows index", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 10, 10, nonceNextExpected}, + {"index ahead of submitted: below index is too low", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 10, 8, nonceTooLow}, + // Range checks (maxNonceGap > 0). + {"gap: index nonce is next-expected", nonceTracker{maxNonceGap: 50}, 5, 5, nonceNextExpected}, + {"gap: below index is too low", nonceTracker{maxNonceGap: 50}, 5, 4, nonceTooLow}, + {"gap: at the upper bound is accepted (queued)", nonceTracker{maxNonceGap: 50}, 5, 55, nonceQueue}, + {"gap: beyond the upper bound is too high", nonceTracker{maxNonceGap: 50}, 5, 56, nonceTooHigh}, + {"gap: in-flight takes precedence over too-low", nonceTracker{maxNonceGap: 50, lastConsecutivelySubmitted: knownNonce(6)}, 5, 3, nonceInFlight}, + {"no gap: far-ahead nonce queues (no upper bound)", nonceTracker{}, 5, 100_000, nonceQueue}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, tc.tracker.classify(tc.nonce)) + tracker := tc.tracker + got, err := tracker.classify( + tc.nonce, + &fakeNonceProvider{nonce: tc.frontier}, + gethCommon.HexToAddress("0xabc"), + ) + require.NoError(t, err) + assert.Equal(t, tc.want, got) }) } } +// With a configured max gap, Add rejects out-of-range nonces up front. +func Test_TxMemPool_RejectsNonceOutOfRange(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + + cfg := testPoolConfig() + cfg.TxMaxNonceGap = 50 + + submitCalls := 0 + pool := newTestPool( + &fakeNonceProvider{nonce: 100}, // on-chain frontier at nonce 100 + func(_ context.Context, _ []heldTx) error { submitCalls++; return nil }, + cfg, + ) + ctx := context.Background() + + // Below the frontier: already used. + require.ErrorIs(t, pool.Add(ctx, signedTestTx(t, key, 99, 1)), errs.ErrNonceTooLow) + + // More than maxNonceGap (50) ahead of the frontier. + require.ErrorIs(t, pool.Add(ctx, signedTestTx(t, key, 200, 1)), errs.ErrNonceTooHigh) + + // Within the accepted window (ahead of expected, but <= frontier+gap): held. + require.NoError(t, pool.Add(ctx, signedTestTx(t, key, 120, 1))) + + assert.Zero(t, submitCalls, "out-of-order/rejected txs are never fast-path submitted") +} + func Test_NonceTracker_ExpectedNonce(t *testing.T) { assert.Equal(t, uint64(5), (&nonceTracker{localIndexedNonce: 5}).expectedNonce()) assert.Equal(t, uint64(7), From b535dc1f4c15c3557ca37b558a7c6e8cefc4d8a5 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 20:59:40 -0400 Subject: [PATCH 04/31] perf(requester): cache the block view by indexed height MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add (via classify) read the EOA nonce with np.GetNonce per transaction, which built a fresh EVM block view every time — so a burst of N txs paid N view builds even though they were all at the same indexed height. Only collectDueBatches reused a view (once per tick, via a local closure). Cache the built view on LocalNonceProvider keyed by the indexed height: reuse it while the height is unchanged, rebuild when a new block is indexed. Reuse is correct because an EOA's on-chain nonce cannot change without a new block. This makes Add bursts within one block build the view once, and lets collectDueBatches drop its per-tick closure and just call GetNonce (the cache now spans the whole pass, and across ticks at the same height). The cached view is shared across reads; callers serialize via the pool's queueMux (the only caller). Guarded by a mutex on the provider for safety. Co-Authored-By: Claude Opus 4.8 --- services/requester/nonce_provider.go | 36 ++++++++++++++++++++++------ services/requester/tx_mempool.go | 23 ++++-------------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/services/requester/nonce_provider.go b/services/requester/nonce_provider.go index 2125bcee..1a8b4096 100644 --- a/services/requester/nonce_provider.go +++ b/services/requester/nonce_provider.go @@ -1,6 +1,8 @@ package requester import ( + "sync" + gethCommon "github.com/ethereum/go-ethereum/common" "github.com/onflow/flow-go/fvm/evm" "github.com/onflow/flow-go/fvm/evm/offchain/query" @@ -31,20 +33,25 @@ type NonceProvider interface { // rather than a routine, recoverable condition to swallow. GetNonce(address gethCommon.Address) (uint64, error) - // GetBlockView builds a NonceView over the latest indexed EVM state. A - // caller that reads many EOAs' nonces at once (e.g. a single mempool flush - // tick) can build the view once and reuse it, instead of paying the view - // build cost per address as GetNonce does. A non-nil error is an - // EXCEPTION, same contract as GetNonce. + // GetBlockView returns a NonceView over the latest indexed EVM state. A + // non-nil error is an EXCEPTION, same contract as GetNonce. GetBlockView() (NonceView, error) } // LocalNonceProvider reads the EOA nonce from the latest height of the -// local state index. +// local state index. It caches the built block view and reuses it while the +// indexed height is unchanged (see GetBlockView). type LocalNonceProvider struct { chainID flowGo.ChainID registerStore *pebble.RegisterStorage blocks storage.BlockIndexer + + // mu guards the cached view below. The cached view is shared across reads; + // callers that read it concurrently must serialize (the mempool does, via + // its queueMux). + mu sync.Mutex + cachedView NonceView + cachedHeight uint64 } var _ NonceProvider = &LocalNonceProvider{} @@ -61,13 +68,25 @@ func NewLocalNonceProvider( } } -// GetBlockView builds a NonceView over the latest indexed EVM height. +// GetBlockView returns a NonceView over the latest indexed EVM height. The view +// is cached and reused while the indexed height is unchanged, so a burst of +// reads within one block — many Add calls, or a collectDueBatches pass — builds +// the (expensive) view only once. It is rebuilt when a new block is indexed. +// Reuse is safe because an EOA's on-chain nonce cannot change without a new +// block being indexed. func (p *LocalNonceProvider) GetBlockView() (NonceView, error) { height, err := p.blocks.LatestEVMHeight() if err != nil { return nil, err } + p.mu.Lock() + defer p.mu.Unlock() + + if p.cachedView != nil && p.cachedHeight == height { + return p.cachedView, nil + } + viewProvider := query.NewViewProvider( p.chainID, evm.StorageAccountAddress(p.chainID), @@ -81,6 +100,9 @@ func (p *LocalNonceProvider) GetBlockView() (NonceView, error) { return nil, err } + p.cachedView = view + p.cachedHeight = height + return view, nil } diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 020d87b6..e7f22c57 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -213,8 +213,7 @@ func (n *nonceTracker) classify( } n.refreshIndexed(indexNonce) - // Below the on-chain frontier: already used, can never execute. Always - // rejected — unrelated to maxNonceGap (it bounds only the upper end). + // Below the on-chain frontier: already used, can never execute. if nonce < n.localIndexedNonce { return nonceTooLow, nil } @@ -592,21 +591,9 @@ func (t *TxMemPool) collectDueBatches() []flushWork { now := time.Now() work := make([]flushWork, 0) - // Build the EVM block view at most once per tick and read every due EOA's - // nonce from it: building the view is expensive, so we avoid rebuilding it - // per address. Built lazily on first need so an all-idle tick does no work. - var blockView NonceView - indexNonceOf := func(addr gethCommon.Address) (uint64, error) { - if blockView == nil { - v, err := t.nonceProvider.GetBlockView() - if err != nil { - return 0, err - } - blockView = v - } - return blockView.GetNonce(addr) - } - + // Each due EOA's nonce is read via GetNonce; the provider caches the block + // view by indexed height, so all reads in this pass (and across ticks at the + // same height) reuse one built view rather than rebuilding per address. for from, q := range t.queues { if q.isEmpty() { // Bound memory: drop queues with no held txs and no activity past @@ -631,7 +618,7 @@ func (t *TxMemPool) collectDueBatches() []flushWork { continue } - indexNonce, err := indexNonceOf(from) + indexNonce, err := t.nonceProvider.GetNonce(from) if err != nil { // Exception: a local state-index nonce read should not fail // under normal operation. This is a background loop with no From 54fd7d33df3308462fc8ba1a88f74ce1c4939137 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 22:59:52 -0400 Subject: [PATCH 05/31] refactor(requester): rename optionalNonce -> nonceWrapper Per review preference: optionalNonce -> nonceWrapper and its constructor knownNonce -> toNonceWrapper. Pure rename, no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 30 ++++++++++----------- services/requester/tx_mempool_test.go | 38 +++++++++++++-------------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index e7f22c57..3f732569 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -79,29 +79,29 @@ const txMemPoolTickInterval = 50 * time.Millisecond // recent activity is kept before being removed, to bound memory usage. const idleQueueRetention = time.Minute -// optionalNonce represents a nonce that may not be set: when set is true, v is a +// nonceWrapper represents a nonce that may not be set: when set is true, v is a // valid nonce; when set is false, the nonce has not been initialized yet. It // lets us compare nonces uniformly even when one may be absent — including the // ambiguous case where the value is 0 (a valid nonce) but set is false. An unset -// optionalNonce behaves as -∞ in the comparisons below (atLeast/is/max): it is +// nonceWrapper behaves as -∞ in the comparisons below (atLeast/is/max): it is // below every real nonce and never "at or above" one, so callers carry no set // checks. -type optionalNonce struct { +type nonceWrapper struct { v uint64 set bool } -func knownNonce(v uint64) optionalNonce { return optionalNonce{v: v, set: true} } +func toNonceWrapper(v uint64) nonceWrapper { return nonceWrapper{v: v, set: true} } // atLeast reports whether this nonce is set and >= n. An unset nonce (-∞) is // never >= a real nonce, so it returns false. -func (o optionalNonce) atLeast(n uint64) bool { return o.set && o.v >= n } +func (o nonceWrapper) atLeast(n uint64) bool { return o.set && o.v >= n } // is reports whether this nonce is set and exactly equals n. -func (o optionalNonce) is(n uint64) bool { return o.set && o.v == n } +func (o nonceWrapper) is(n uint64) bool { return o.set && o.v == n } // max returns the greater of two optional nonces, treating unset as -∞. -func (o optionalNonce) max(other optionalNonce) optionalNonce { +func (o nonceWrapper) max(other nonceWrapper) nonceWrapper { if !o.set { return other } @@ -149,12 +149,12 @@ type nonceTracker struct { // is outstanding. It means strictly "a network call is in flight" and is // cleared the moment that call returns — by markSubmitted on success or // rollbackSubmitting on failure. - submitting optionalNonce + submitting nonceWrapper // lastConsecutivelySubmitted is the highest nonce CONSECUTIVELY, SUCCESSFULLY // submitted (ack'd). Unset before the EOA's first success. Only consecutive // submissions advance it; TTL-expiry (gapped) batches do NOT, so it never // includes a nonce past a gap. - lastConsecutivelySubmitted optionalNonce + lastConsecutivelySubmitted nonceWrapper // maxNonceGap is how far above localIndexedNonce a nonce may be before it is // rejected as too-high. 0 means no upper bound. It does NOT affect the // too-low check (a nonce below localIndexedNonce is always rejected). Set @@ -168,7 +168,7 @@ func (n *nonceTracker) inFlight() bool { return n.submitting.set } // highestSent returns the highest nonce we have already sent — whether still in // flight or already ack'd (unset if neither). Re-accepting a nonce at or below // it would burn Flow fees on a guaranteed nonce-mismatch. -func (n *nonceTracker) highestSent() optionalNonce { +func (n *nonceTracker) highestSent() nonceWrapper { return n.lastConsecutivelySubmitted.max(n.submitting) } @@ -234,7 +234,7 @@ func (n *nonceTracker) classify( // (the synchronous fast path in Add skips it — it holds the lock across the // whole submit, so there is no concurrency window to guard). func (n *nonceTracker) markSubmitting(highNonce uint64) { - n.submitting = knownNonce(highNonce) + n.submitting = toNonceWrapper(highNonce) } // markSubmitted acks a successful submission: it advances the consecutively- @@ -246,8 +246,8 @@ func (n *nonceTracker) markSubmitting(highNonce uint64) { // submission, which we accept for a state machine that is trivial to reason // about. func (n *nonceTracker) markSubmitted(highNonce uint64) { - n.lastConsecutivelySubmitted = knownNonce(highNonce) - n.submitting = optionalNonce{} + n.lastConsecutivelySubmitted = toNonceWrapper(highNonce) + n.submitting = nonceWrapper{} } // rollbackSubmitting clears the in-flight marker after a FAILED submission, but @@ -258,7 +258,7 @@ func (n *nonceTracker) markSubmitted(highNonce uint64) { // "lastSubmittedNonce == batchMax" guard. func (n *nonceTracker) rollbackSubmitting(highNonce uint64) { if n.submitting.is(highNonce) { - n.submitting = optionalNonce{} + n.submitting = nonceWrapper{} } } @@ -409,7 +409,7 @@ func (t *TxMemPool) Add( q, ok := t.queues[from] if !ok { // A fresh queue's other fields are intentionally left at their zero - // values: the nonceTracker's optionalNonce fields read as "unset" (nonce 0 + // values: the nonceTracker's nonceWrapper fields read as "unset" (nonce 0 // is not mistaken for a real submission), and the timing fields // (collectionWindowEndsAt/flushDeadline/lastSubmittedAt) are only ever // read after being set on the first enqueue or submission below. Only diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 9e31f856..7cfb5bab 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -200,7 +200,7 @@ func Test_TxMemPool_FastPathSubmitsImmediately(t *testing.T) { assert.Empty(t, q.txs) // Explicit success update: submitting cleared, submitted advanced to 0. assert.False(t, q.nonces.inFlight()) - assert.Equal(t, knownNonce(0), q.nonces.lastConsecutivelySubmitted) + assert.Equal(t, toNonceWrapper(0), q.nonces.lastConsecutivelySubmitted) } func Test_TxMemPool_UnexpectedNonceEnqueues(t *testing.T) { @@ -310,7 +310,7 @@ func Test_TxMemPool_FailedFlushDoesNotWedgeEOA(t *testing.T) { // State was committed optimistically under the lock. q := pool.queues[from] require.True(t, q.nonces.inFlight()) - assert.Equal(t, knownNonce(1), q.nonces.submitting) + assert.Equal(t, toNonceWrapper(1), q.nonces.submitting) // The submission fails; submitWork must clear the in-flight marker. err = pool.submitWork(context.Background(), work[0]) @@ -331,7 +331,7 @@ func Test_ReconcileSubmission_OnlyReconcilesMatchingInFlightBatch(t *testing.T) from := gethCommon.HexToAddress("0xabc") pool.queues[from] = &eoaQueue{ txs: map[uint64]heldTx{}, - nonces: nonceTracker{submitting: knownNonce(7)}, + nonces: nonceTracker{submitting: toNonceWrapper(7)}, } submitErr := errors.New("network down") @@ -393,7 +393,7 @@ func Test_TxMemPool_SuccessfulFlushMarksSubmitted(t *testing.T) { q := pool.queues[from] assert.False(t, q.nonces.inFlight()) - assert.Equal(t, knownNonce(1), q.nonces.lastConsecutivelySubmitted) + assert.Equal(t, toNonceWrapper(1), q.nonces.lastConsecutivelySubmitted) } // Fix 1: a failed fast-path submission must not rate-limit the EOA via @@ -490,7 +490,7 @@ func Test_TxMemPool_EmptyQueueAgesOut(t *testing.T) { // last active beyond the retention window: must be removed. pool.queues[from] = &eoaQueue{ txs: map[uint64]heldTx{}, - nonces: nonceTracker{submitting: knownNonce(5)}, + nonces: nonceTracker{submitting: toNonceWrapper(5)}, lastActivity: time.Now().Add(-2 * idleQueueRetention), } pool.collectDueBatches() @@ -601,19 +601,19 @@ func Test_NonceTracker_Classify(t *testing.T) { {"gap ahead queues", nonceTracker{}, 5, 7, nonceQueue}, {"below index is too low (no gap configured)", nonceTracker{}, 5, 3, nonceTooLow}, {"nonce 0 is next-expected on a zero tracker", nonceTracker{}, 0, 0, nonceNextExpected}, - {"at submitted is in-flight", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 5, 6, nonceInFlight}, - {"below submitted is in-flight", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 5, 4, nonceInFlight}, - {"next after submitted is expected", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 5, 7, nonceNextExpected}, - {"at submitting is in-flight", nonceTracker{submitting: knownNonce(8)}, 5, 8, nonceInFlight}, - {"next after submitting is expected", nonceTracker{submitting: knownNonce(8)}, 5, 9, nonceNextExpected}, - {"index ahead of submitted: expected follows index", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 10, 10, nonceNextExpected}, - {"index ahead of submitted: below index is too low", nonceTracker{lastConsecutivelySubmitted: knownNonce(6)}, 10, 8, nonceTooLow}, + {"at submitted is in-flight", nonceTracker{lastConsecutivelySubmitted: toNonceWrapper(6)}, 5, 6, nonceInFlight}, + {"below submitted is in-flight", nonceTracker{lastConsecutivelySubmitted: toNonceWrapper(6)}, 5, 4, nonceInFlight}, + {"next after submitted is expected", nonceTracker{lastConsecutivelySubmitted: toNonceWrapper(6)}, 5, 7, nonceNextExpected}, + {"at submitting is in-flight", nonceTracker{submitting: toNonceWrapper(8)}, 5, 8, nonceInFlight}, + {"next after submitting is expected", nonceTracker{submitting: toNonceWrapper(8)}, 5, 9, nonceNextExpected}, + {"index ahead of submitted: expected follows index", nonceTracker{lastConsecutivelySubmitted: toNonceWrapper(6)}, 10, 10, nonceNextExpected}, + {"index ahead of submitted: below index is too low", nonceTracker{lastConsecutivelySubmitted: toNonceWrapper(6)}, 10, 8, nonceTooLow}, // Range checks (maxNonceGap > 0). {"gap: index nonce is next-expected", nonceTracker{maxNonceGap: 50}, 5, 5, nonceNextExpected}, {"gap: below index is too low", nonceTracker{maxNonceGap: 50}, 5, 4, nonceTooLow}, {"gap: at the upper bound is accepted (queued)", nonceTracker{maxNonceGap: 50}, 5, 55, nonceQueue}, {"gap: beyond the upper bound is too high", nonceTracker{maxNonceGap: 50}, 5, 56, nonceTooHigh}, - {"gap: in-flight takes precedence over too-low", nonceTracker{maxNonceGap: 50, lastConsecutivelySubmitted: knownNonce(6)}, 5, 3, nonceInFlight}, + {"gap: in-flight takes precedence over too-low", nonceTracker{maxNonceGap: 50, lastConsecutivelySubmitted: toNonceWrapper(6)}, 5, 3, nonceInFlight}, {"no gap: far-ahead nonce queues (no upper bound)", nonceTracker{}, 5, 100_000, nonceQueue}, } for _, tc := range tests { @@ -661,12 +661,12 @@ func Test_TxMemPool_RejectsNonceOutOfRange(t *testing.T) { func Test_NonceTracker_ExpectedNonce(t *testing.T) { assert.Equal(t, uint64(5), (&nonceTracker{localIndexedNonce: 5}).expectedNonce()) assert.Equal(t, uint64(7), - (&nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: knownNonce(6)}).expectedNonce()) + (&nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: toNonceWrapper(6)}).expectedNonce()) assert.Equal(t, uint64(9), - (&nonceTracker{localIndexedNonce: 5, submitting: knownNonce(8)}).expectedNonce()) + (&nonceTracker{localIndexedNonce: 5, submitting: toNonceWrapper(8)}).expectedNonce()) // The indexed frontier wins when it is ahead of our own sends. assert.Equal(t, uint64(10), - (&nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: knownNonce(6)}).expectedNonce()) + (&nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: toNonceWrapper(6)}).expectedNonce()) } func Test_NonceTracker_Transitions(t *testing.T) { @@ -675,12 +675,12 @@ func Test_NonceTracker_Transitions(t *testing.T) { // markSubmitting sets the in-flight marker. n.markSubmitting(7) assert.True(t, n.inFlight()) - assert.Equal(t, knownNonce(7), n.submitting) + assert.Equal(t, toNonceWrapper(7), n.submitting) // markSubmitted advances submitted and clears submitting. n.markSubmitted(7) assert.False(t, n.inFlight()) - assert.Equal(t, knownNonce(7), n.lastConsecutivelySubmitted) + assert.Equal(t, toNonceWrapper(7), n.lastConsecutivelySubmitted) // rollbackSubmitting only clears a matching in-flight nonce. n.markSubmitting(9) @@ -689,7 +689,7 @@ func Test_NonceTracker_Transitions(t *testing.T) { n.rollbackSubmitting(9) // matching: cleared assert.False(t, n.inFlight()) // A rollback never disturbs the consecutively-submitted nonce. - assert.Equal(t, knownNonce(7), n.lastConsecutivelySubmitted) + assert.Equal(t, toNonceWrapper(7), n.lastConsecutivelySubmitted) // refreshIndexed updates the cached frontier. n.refreshIndexed(12) From 9c6f6c53fbd9475fb46e82d037ceba97148305e9 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 23:31:56 -0400 Subject: [PATCH 06/31] refactor(requester): document coherent zero-value queue init (step 4) Verify and document that a freshly created eoaQueue is safe to leave with its non-nonce fields at their zero values. Traced every read: - collectionWindowEndsAt / flushDeadline are read only once the queue is non-empty (collectDueBatches skips empty queues) and are set on the first enqueue before that, so their zero value is never observed; - lastSubmittedAt may be read while still zero on the fast path, but spacingElapsed treats zero as "never submitted" => spacing satisfied; - lastActivity is set unconditionally right after creation. The prior comment claimed the timing fields were "only ever read after being set," which is inaccurate for lastSubmittedAt. Tighten the comment to state each field's actual safety argument. Comment-only; no behavior change (the nonceWrapper work in step 2 already removed the ambiguous zero-nonce case Leo flagged). Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 3f732569..4d1f8efd 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -409,11 +409,18 @@ func (t *TxMemPool) Add( q, ok := t.queues[from] if !ok { // A fresh queue's other fields are intentionally left at their zero - // values: the nonceTracker's nonceWrapper fields read as "unset" (nonce 0 - // is not mistaken for a real submission), and the timing fields - // (collectionWindowEndsAt/flushDeadline/lastSubmittedAt) are only ever - // read after being set on the first enqueue or submission below. Only - // maxNonceGap needs seeding from config. + // values, each safe to read as-is: + // - the nonceTracker's nonceWrapper fields read as "unset", so nonce 0 + // is never mistaken for a real submission (see nonceWrapper); + // - collectionWindowEndsAt and flushDeadline are read only once the + // queue is non-empty (collectDueBatches skips empty queues), and the + // first enqueue below sets them before that — their zero value is + // never observed; + // - lastSubmittedAt MAY be read while still zero (the fast path checks + // spacing immediately after creation), but spacingElapsed treats zero + // as "never submitted" → spacing satisfied, which is exactly right; + // - lastActivity is set unconditionally on the next line. + // Only maxNonceGap needs seeding from config. q = &eoaQueue{ txs: make(map[uint64]heldTx), nonces: nonceTracker{maxNonceGap: t.config.TxMaxNonceGap}, From 4019174de1c6de28f897c2675b0b117f03838359 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 23:45:10 -0400 Subject: [PATCH 07/31] feat(requester): log every submission outcome, no silent drops (step 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Leo and Ardit flagged that on a Flow submit failure the batch's EVM transactions were dropped with only a low-context Error log buried in submitTxBatch. Establish Leo's invariant: for any tx id you can either find it on-chain (sent) OR find a WARN log (dropped) — never nothing. Decision (Vishal): keep drop-and-rely-on-client-resubmit; do NOT re-enqueue failed txs for retry. This change is therefore observability only, no behavior change to the submission state machine. - Add logSubmission, called from both submission paths (the Add fast path and submitWork). On failure it WARNs with everything needed to debug a "lost transaction" report: eoa, tx hashes, nonce range, indexed frontier, batch size, flush reason, error. On success it emits a lighter DEBUG line (eoa + nonce range + reason). - Enrich flushWork with reason (fast-path / consecutive-prefix / ttl-expiry) and the indexed frontier, set in collectDueBatches. - Make submitTxBatch a pure submission primitive: keep the TransactionsDropped metric (reserved for Cadence build/send errors), drop its now-redundant logging — logSubmission at the call site has the richer context. Remove the unused logTxsDropped helper. - Add local-indexed-nonce (and expected-nonce) to the existing stale-prune and TTL-expiry WARN logs for consistent debuggability. - Remove the file-top "log discarded txs" TODO, now addressed. Tests (TDD): a failed flush emits an observable WARN with the dropped tx hashes and debug fields; a successful flush emits a DEBUG trace line. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 112 +++++++++++++++++++++----- services/requester/tx_mempool_test.go | 83 +++++++++++++++++++ 2 files changed, 176 insertions(+), 19 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 4d1f8efd..0e019585 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -21,7 +21,6 @@ import ( // TODO: Document what the TX Mempool does with clear examples of the difference use cases it handles. e.g. transactions coming in out of sequence with a gap in between, transactions expiring in the queue as local index nonce moves beyond the nonce of the the tx. // TODO: Overall, refactor the code such that its easier to understand and more importantly easier to maintain -// TODO: Log when transactions are being discarded from the queue to help debug issues if lets say a client compains about transactions being lost or erroring out. // heldTx is a transaction held in the mempool, waiting for its // collection window to elapse or its nonce gap to be filled. @@ -470,7 +469,10 @@ func (t *TxMemPool) Add( // (queue non-empty, in flight, or spacing not elapsed), fall through to // enqueue and let the background loop flush it. if q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { - if submitErr := t.submitBatch(ctx, []heldTx{userTx}); submitErr != nil { + batch := []heldTx{userTx} + submitErr := t.submitBatch(ctx, batch) + t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) + if submitErr != nil { return submitErr } q.nonces.markSubmitted(tx.Nonce()) @@ -514,8 +516,23 @@ type flushWork struct { // reconcileSubmission). TTL-expiry batches are not marked in flight and must // never touch the tracker. inFlight bool + // reason is why this batch was flushed, recorded purely for the submission + // log (see logSubmission): flushReasonPrefix for a consecutive-prefix flush, + // flushReasonTTL for a TTL-expiry submit-anyway. + reason string + // localIndexedNonce is the on-chain frontier observed when the batch was + // collected. It is logged on drop so a "lost transaction" report can be + // debugged against where the chain actually was. + localIndexedNonce uint64 } +// flushReason values label why a batch was submitted, for the submission log. +const ( + flushReasonFastPath = "fast-path" + flushReasonPrefix = "consecutive-prefix" + flushReasonTTL = "ttl-expiry" +) + func (t *TxMemPool) processQueues(ctx context.Context) { ticker := time.NewTicker(txMemPoolTickInterval) defer ticker.Stop() @@ -537,14 +554,69 @@ func (t *TxMemPool) processQueues(ctx context.Context) { } } -// submitWork submits one detached batch and reconciles the queue's nonce state -// once the network call returns. +// submitWork submits one detached batch, records its fate (logSubmission), and +// reconciles the queue's nonce state once the network call returns. func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { err := t.submitBatch(ctx, w.txs) + t.logSubmission(w.from, w.txs, w.reason, w.localIndexedNonce, err) t.reconcileSubmission(w, err) return err } +// logSubmission records the fate of a submitted batch so a transaction is never +// silently lost. This is the observability half of Leo's invariant: for any tx +// id, you can either find it on-chain (sent) OR find a WARN log here (dropped) — +// never nothing. +// +// - On a Flow submit FAILURE the batch's EVM transactions are dropped (we do +// not retry — clients resubmit), so we WARN with everything needed to debug +// a "lost transaction" report: eoa, tx hashes, nonce range, the indexed +// frontier, batch size, the flush reason, and the error. +// - On SUCCESS we emit a lighter DEBUG line (eoa + nonce range) so a sent +// batch is traceable without the noise of a warning. +// +// txs is assumed nonce-ascending (selectConsecutivePrefix / selectExpired and +// the single-tx fast path all satisfy this), so txs[0] is the low nonce. +func (t *TxMemPool) logSubmission( + from gethCommon.Address, + txs []heldTx, + reason string, + localIndexedNonce uint64, + submitErr error, +) { + if len(txs) == 0 { + return + } + lowNonce := txs[0].nonce + highNonce := txs[len(txs)-1].nonce + + if submitErr != nil { + txHashes := make([]string, len(txs)) + for i, htx := range txs { + txHashes[i] = htx.txHash.Hex() + } + t.logger.Warn(). + Err(submitErr). + Str("eoa", from.Hex()). + Strs("tx-hashes", txHashes). + Uint64("low-nonce", lowNonce). + Uint64("high-nonce", highNonce). + Uint64("local-indexed-nonce", localIndexedNonce). + Int("batch-size", len(txs)). + Str("reason", reason). + Msg("Flow submission failed, EVM transactions dropped") + return + } + + t.logger.Debug(). + Str("eoa", from.Hex()). + Uint64("low-nonce", lowNonce). + Uint64("high-nonce", highNonce). + Int("batch-size", len(txs)). + Str("reason", reason). + Msg("submitted EVM transactions to Flow") +} + // reconcileSubmission updates the EOA's nonceTracker after a detached // consecutive-prefix submission returns. collectDueBatches marked the batch // "submitting" (under the lock) before the network call; this records the @@ -667,9 +739,11 @@ func (t *TxMemPool) collectDueBatches() []flushWork { q.flushDeadline = now.Add(t.config.TxSubmissionSpacing) } work = append(work, flushWork{ - from: from, - txs: prefix, - inFlight: true, + from: from, + txs: prefix, + inFlight: true, + reason: flushReasonPrefix, + localIndexedNonce: indexNonce, }) continue } @@ -712,10 +786,14 @@ func (t *TxMemPool) collectDueBatches() []flushWork { txHashes[i] = htx.txHash.Hex() } t.logger.Warn().Strs("tx-hashes", txHashes).Str("eoa", from.Hex()). + Uint64("local-indexed-nonce", indexNonce). + Uint64("expected-nonce", q.nonces.expectedNonce()). Msg("nonce gap never filled within TTL, submitting held transactions anyway") work = append(work, flushWork{ - from: from, - txs: expired, + from: from, + txs: expired, + reason: flushReasonTTL, + localIndexedNonce: indexNonce, }) } } @@ -750,6 +828,7 @@ func (t *TxMemPool) pruneStaleTxs( } if len(stale) > 0 { t.logger.Warn().Strs("tx-hashes", stale).Str("eoa", from.Hex()). + Uint64("local-indexed-nonce", indexNonce). Msg("dropping stale transactions with nonce below indexed state") } } @@ -769,6 +848,11 @@ func (t *TxMemPool) submitTxBatch(ctx context.Context, txs []heldTx) error { } script := replaceAddresses(runTxScript, t.config.FlowNetworkID) + // On a build/send failure the batch's EVM transactions are dropped; count + // them here (the metric is reserved for Cadence build/submission errors) and + // return the error. The observable WARN drop log — with eoa, nonce range and + // flush reason — is emitted by logSubmission at the call site, which has that + // context; this keeps submitTxBatch a pure submission primitive. flowTx, err := t.buildTransaction( ctx, t.getReferenceBlock(), @@ -778,23 +862,13 @@ func (t *TxMemPool) submitTxBatch(ctx context.Context, txs []heldTx) error { ) if err != nil { t.collector.TransactionsDropped(len(txs)) - t.logTxsDropped(txs, err, "failed to build Flow transaction, EVM transactions dropped") return err } if err := t.client.SendTransaction(ctx, *flowTx); err != nil { t.collector.TransactionsDropped(len(txs)) - t.logTxsDropped(txs, err, "failed to send Flow transaction, EVM transactions dropped") return err } return nil } - -func (t *TxMemPool) logTxsDropped(txs []heldTx, err error, msg string) { - txHashes := make([]string, len(txs)) - for i, htx := range txs { - txHashes[i] = htx.txHash.Hex() - } - t.logger.Error().Err(err).Strs("tx-hashes", txHashes).Msg(msg) -} diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 7cfb5bab..92d6aa65 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -1,6 +1,7 @@ package requester import ( + "bytes" "context" "crypto/ecdsa" "errors" @@ -396,6 +397,88 @@ func Test_TxMemPool_SuccessfulFlushMarksSubmitted(t *testing.T) { assert.Equal(t, toNonceWrapper(1), q.nonces.lastConsecutivelySubmitted) } +// No silent drops (Leo's invariant): a failed flush submission must produce an +// observable WARN log carrying the dropped tx hashes and enough context to +// debug a "lost transaction" report (eoa, nonce range, indexed nonce, batch +// size, reason, error). +func Test_TxMemPool_FailedSubmitLogsDropWarning(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + var logBuf bytes.Buffer + submitErr := errors.New("network down") + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, _ []heldTx) error { return submitErr }, + testPoolConfig(), + ) + pool.logger = zerolog.New(&logBuf) + + tx0 := signedTestTx(t, key, 0, 1) + tx1 := signedTestTx(t, key, 1, 1) + past := time.Now().Add(-time.Second) + pool.queues[from] = &eoaQueue{ + txs: map[uint64]heldTx{ + 0: {txHash: tx0.Hash(), nonce: 0, enqueuedAt: past}, + 1: {txHash: tx1.Hash(), nonce: 1, enqueuedAt: past}, + }, + collectionWindowEndsAt: past, + flushDeadline: past, + } + + work := pool.collectDueBatches() + require.Len(t, work, 1) + require.Error(t, pool.submitWork(context.Background(), work[0])) + + out := logBuf.String() + assert.Contains(t, out, `"level":"warn"`, "drop must be observable at WARN level") + assert.Contains(t, out, tx0.Hash().Hex(), "dropped tx hash must be logged") + assert.Contains(t, out, tx1.Hash().Hex(), "dropped tx hash must be logged") + assert.Contains(t, out, from.Hex(), "eoa must be logged") + assert.Contains(t, out, `"local-indexed-nonce":0`, "indexed frontier must be logged") + assert.Contains(t, out, `"batch-size":2`, "batch size must be logged") + assert.Contains(t, out, "network down", "submit error must be logged") +} + +// A successful submission is traceable via a DEBUG log carrying the eoa and +// nonce range, so a sent batch can be found in logs without a warning. +func Test_TxMemPool_SuccessfulSubmitLogsDebug(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + var logBuf bytes.Buffer + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, _ []heldTx) error { return nil }, + testPoolConfig(), + ) + pool.logger = zerolog.New(&logBuf) + + tx0 := signedTestTx(t, key, 0, 1) + tx1 := signedTestTx(t, key, 1, 1) + past := time.Now().Add(-time.Second) + pool.queues[from] = &eoaQueue{ + txs: map[uint64]heldTx{ + 0: {txHash: tx0.Hash(), nonce: 0, enqueuedAt: past}, + 1: {txHash: tx1.Hash(), nonce: 1, enqueuedAt: past}, + }, + collectionWindowEndsAt: past, + flushDeadline: past, + } + + work := pool.collectDueBatches() + require.Len(t, work, 1) + require.NoError(t, pool.submitWork(context.Background(), work[0])) + + out := logBuf.String() + assert.Contains(t, out, `"level":"debug"`, "successful send must be traceable at DEBUG level") + assert.Contains(t, out, from.Hex(), "eoa must be logged") + assert.Contains(t, out, `"low-nonce":0`) + assert.Contains(t, out, `"high-nonce":1`) +} + // Fix 1: a failed fast-path submission must not rate-limit the EOA via // lastSubmittedAt, and must leave nothing in flight. func Test_TxMemPool_FailedFastPathLeavesNoState(t *testing.T) { From 328ccf41bf2d7232f365b9a1336059e6be3d967f Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 23:48:06 -0400 Subject: [PATCH 08/31] refactor(requester): defer heldTx allocation past cheap rejections (step 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Leo (round-3 #1): build the heldTx only AFTER the cheap rejections (duplicate + classify), so a transaction we are about to reject never costs a heldTx allocation. Previously userTx was built before the duplicate check, allocating even for duplicates and in-flight retries (the common DFNS-resubmit case). Split the verdict switch: the reject verdicts (in-flight / too-low / too-high) now return up front, then userTx is built once, then the fast path is a flat guard on nonceNextExpected. No behavior change — existing unit + e2e tests pass unchanged. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 62 +++++++++++++++++--------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 0e019585..d7e70c50 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -432,13 +432,6 @@ func (t *TxMemPool) Add( // activity here to keep the idle-queue retention clock accurate. q.lastActivity = now - userTx := heldTx{ - txPayload: hexEncodedTx, - txHash: tx.Hash(), - nonce: tx.Nonce(), - enqueuedAt: now, - } - // Reject an exact duplicate of a transaction already in the queue (cheapest // check; needs no index read). if existing, ok := q.txs[tx.Nonce()]; ok && existing.txHash == tx.Hash() { @@ -453,6 +446,8 @@ func (t *TxMemPool) Add( return err } + // Reject out-of-range / in-flight nonces up front, before allocating any + // held state: a transaction we are about to reject must not cost a heldTx. switch verdict { case nonceInFlight: return errs.ErrInFlightNonce @@ -460,31 +455,40 @@ func (t *TxMemPool) Add( return errs.ErrNonceTooLow case nonceTooHigh: return errs.ErrNonceTooHigh - case nonceNextExpected: - // Fast path: an empty queue with nothing in flight and spacing satisfied - // can submit the expected nonce immediately — zero added latency. The - // lock is held across the whole submit, so there is no concurrency - // window: no need to mark "submitting" first; on success we record the - // ack, on failure we leave the EOA untouched. If we cannot fast-path yet - // (queue non-empty, in flight, or spacing not elapsed), fall through to - // enqueue and let the background loop flush it. - if q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { - batch := []heldTx{userTx} - submitErr := t.submitBatch(ctx, batch) - t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) - if submitErr != nil { - return submitErr - } - q.nonces.markSubmitted(tx.Nonce()) - q.lastSubmittedAt = time.Now() - return nil + } + + // Past every cheap rejection: the transaction will be kept (submitted now or + // held), so build the heldTx now rather than before the checks above. + userTx := heldTx{ + txPayload: hexEncodedTx, + txHash: tx.Hash(), + nonce: tx.Nonce(), + enqueuedAt: now, + } + + // Fast path: a next-expected nonce with an empty queue, nothing in flight and + // spacing satisfied is submitted immediately — zero added latency. The lock + // is held across the whole submit, so there is no concurrency window: no need + // to mark "submitting" first; on success we record the ack, on failure we + // leave the EOA untouched. If we cannot fast-path yet (queue non-empty, in + // flight, or spacing not elapsed), fall through to enqueue and let the + // background loop flush it. + if verdict == nonceNextExpected && + q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { + batch := []heldTx{userTx} + submitErr := t.submitBatch(ctx, batch) + t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) + if submitErr != nil { + return submitErr } - case nonceQueue: - // A gap ahead within the accepted window — fall through to enqueue. + q.nonces.markSubmitted(tx.Nonce()) + q.lastSubmittedAt = time.Now() + return nil } - // Enqueue. A same-nonce, different-payload resubmission replaces the - // queued transaction (last write wins), matching mempool semantics. + // Enqueue (a gap ahead, or a next-expected nonce that could not fast-path + // yet). A same-nonce, different-payload resubmission replaces the queued + // transaction (last write wins), matching mempool semantics. wasEmpty := q.isEmpty() q.txs[tx.Nonce()] = userTx q.collectionWindowEndsAt = now.Add(t.config.TxCollectionWindow) From d1ab394963786ca8c1824010eb0e66aa9f19d3b5 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Thu, 25 Jun 2026 23:50:53 -0400 Subject: [PATCH 09/31] docs(requester): top-of-file behavior spec + dedupe struct doc (step 7) Replace the two file-top TODOs with a thorough behavior spec enumerating every case the mempool handles, numbered to read as a test checklist: fast path, burst batching, consecutive-prefix flush, submission spacing, queue-on-gap, stale pruning, TTL submit-anyway, idle-queue retention, and the four synchronous rejections (duplicate, in-flight, too-low, too-high), plus the cross-cutting invariants (no silent drops, failure handling, concurrency). Each applicable case carries a concrete nonce example (e.g. queued 1,2,3,5,6,7 -> flush sends 1,2,3, holds 5,6,7). Trim the TxMemPool struct doc to its unique content (the dual role of TxSubmissionSpacing and the locking tradeoff) and point at the spec instead of duplicating the prose. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 131 +++++++++++++++++++++++-------- 1 file changed, 97 insertions(+), 34 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index d7e70c50..740f5e21 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -19,8 +19,89 @@ import ( "github.com/onflow/flow-evm-gateway/services/requester/keystore" ) -// TODO: Document what the TX Mempool does with clear examples of the difference use cases it handles. e.g. transactions coming in out of sequence with a gap in between, transactions expiring in the queue as local index nonce moves beyond the nonce of the the tx. -// TODO: Overall, refactor the code such that its easier to understand and more importantly easier to maintain +// This file implements TxMemPool, a nonce-aware transaction mempool that +// decides when and how to submit EVM transactions to Flow. It treats the EOA's +// nonce from the local state index as the on-chain frontier and reasons about +// each incoming nonce relative to that frontier and to what it has already sent. +// +// BEHAVIOR SPEC — every case the pool handles. This is meant to read as a +// checklist for tests; each numbered case maps to code and ideally to a test. +// +// (The examples below assume the EOA's next expected nonce is N unless stated.) +// +// Submission paths +// 1. Fast path: a transaction whose nonce is the next expected nonce, arriving +// to an empty queue with nothing in flight and submission spacing +// satisfied, is submitted synchronously inside Add — zero added latency. +// Example: expected nonce 5, an empty queue, tx with nonce 5 arrives → it +// is sent immediately, nothing is queued. +// 2. Burst batching: when the fast path does not apply, transactions queue +// per-EOA. A sliding collection window (TxCollectionWindow, reset on each +// arrival) decides when a burst is complete; a flush deadline anchored at +// the FIRST enqueue (TxSubmissionSpacing) caps how long a continuously +// resetting window can defer the flush. +// Example: nonces 5,6,7 arrive within a few ms of each other; the window +// keeps resetting, so they are collected together and flushed as one batch +// once arrivals pause for TxCollectionWindow (or the deadline is hit). +// 3. Consecutive-prefix flush: on flush, the longest run of consecutive nonces +// starting at the expected nonce is sent as ONE Cadence transaction, capped +// at TxMaxBatchSize. A nonce gap splits the queue: only the prefix before +// the first gap is sent; the post-gap remainder waits for a later tick. +// Example: txs with nonces 1,2,3,5,6,7 arrive (gap at 4); the queue holds +// 1,2,3,5,6,7; the flush sends a batch of 1,2,3 while 5,6,7 wait in the +// queue for nonce 4 to arrive (or to age out via case 7). +// 4. Submission spacing: consecutive Cadence submissions for one EOA are kept +// at least TxSubmissionSpacing apart, so two Flow transactions land in +// different blocks and cannot be reordered by Collection Nodes. +// Example: a batch is sent at t=0; the next batch for the same EOA is held +// until t=TxSubmissionSpacing even if it is already due. +// +// Holding and eventual disposal of out-of-order transactions +// 5. Queue (gap ahead): a future nonce within the accepted window is held +// until the gap fills or it ages out. +// Example: expected nonce 5, tx with nonce 7 arrives (5 and 6 missing) → +// 7 is held, not sent. +// 6. Stale pruning: a held tx whose nonce has fallen below the indexed +// frontier (e.g. filled via another gateway) can never execute and is +// dropped with a WARN log before it would burn fees. +// Example: nonce 3 sits in the queue; the frontier advances to 5 (3 and 4 +// were filled elsewhere) → 3 is dropped with a WARN, never submitted. +// 7. TTL submit-anyway: a held tx whose head gap never fills within TxPoolTTL +// is submitted ANYWAY (capped at TxMaxBatchSize) rather than dropped, so +// its failure is observable on-chain instead of a silent disappearance. +// Example: nonce 7 is held while 5,6 never arrive; after TxPoolTTL, 7 is +// submitted and fails on-chain with a nonce mismatch (visible on flowscan). +// 8. Idle-queue retention: a queue with no held txs and no activity for +// idleQueueRetention is removed to bound memory. +// +// Rejections (returned synchronously from Add; the tx is never queued) +// 9. Duplicate: same nonce AND same tx hash as one already queued → +// ErrDuplicateTransaction (cheapest check; no index read). +// Example: tx with nonce 5 is queued; the identical tx (same hash) is +// submitted again → rejected. (A same-nonce tx with a DIFFERENT hash is +// not a duplicate — it replaces the queued one, last write wins.) +// 10. In-flight: nonce at or below the highest nonce already sent (still in +// flight or already ack'd) → ErrInFlightNonce; re-accepting would burn +// Flow fees on a guaranteed nonce-mismatch. +// Example: nonces 5,6 were just sent and are in flight; a new tx with +// nonce 6 arrives → rejected. +// 11. Too-low: nonce below the indexed frontier (already used) → +// ErrNonceTooLow. Always enforced. +// Example: frontier is 5, tx with nonce 4 arrives → rejected. +// 12. Too-high: nonce more than TxMaxNonceGap beyond the frontier → +// ErrNonceTooHigh. Only enforced when TxMaxNonceGap > 0. +// Example: frontier 5, TxMaxNonceGap 500, tx with nonce 600 arrives → +// rejected (it cannot execute until ~595 intervening nonces are filled). +// +// Cross-cutting invariants +// - No silent drops: for any accepted tx id you can either find it on-chain +// (submitted) or find a WARN log saying it was dropped (submit failure or +// stale prune) — never nothing. See logSubmission. +// - Failure handling: a failed submission drops the batch (clients resubmit) +// and never wedges the EOA (the in-flight marker is rolled back). The pool +// does NOT retry internally. +// - Concurrency: one background goroutine (processQueues) flushes due queues +// and Add runs under the same pool-wide queueMux; see the note on TxMemPool. // heldTx is a transaction held in the mempool, waiting for its // collection window to elapse or its nonce gap to be filled. @@ -304,40 +385,22 @@ func (q *eoaQueue) size() int { return len(q.txs) } -// TxMemPool is a `TxPool` implementation that uses the EOA nonce from -// the local state index to decide when and how to submit transactions to the -// Flow network. -// -// Fast path: a transaction carrying the expected next nonce, with an empty -// queue, nothing in flight, and submission spacing satisfied, is submitted -// IMMEDIATELY — zero added latency for the common case. -// -// Otherwise transactions queue per-EOA. A sliding collection window -// (`TxCollectionWindow`, reset on each arrival) decides when a burst is -// complete. `TxSubmissionSpacing` is BOTH (a) the minimum gap between -// consecutive Cadence submissions for the same EOA (so two Flow transactions -// land in different blocks and cannot be reordered by Collection Nodes) and -// (b) the flush deadline anchored at first enqueue (caps a -// continuously-resetting window). There is deliberately NO separate hard-cap -// knob. -// -// On flush, the longest consecutive nonce prefix starting at the expected -// nonce (from the local index, advanced past any in-flight submission) is -// submitted, capped at `TxMaxBatchSize`. -// -// Out-of-order transactions are held until the gap fills, the local index -// advances past them (then they are stale and pruned), or `TxPoolTTL` -// expires — on expiry they are submitted anyway so the failure is observable -// on-chain rather than a silent drop. +// TxMemPool is a `TxPool` implementation that uses the EOA nonce from the local +// state index to decide when and how to submit transactions to the Flow +// network. The full behavior — fast path, burst batching, gap handling, TTL +// submit-anyway, and the rejection rules — is enumerated in the behavior spec +// at the top of this file. // -// A nonce already submitted and still in flight is rejected with -// `ErrInFlightNonce`, since a duplicate would burn Flow fees on a guaranteed -// nonce-mismatch failure. +// `TxSubmissionSpacing` deliberately serves two roles at once: (a) the minimum +// gap between consecutive Cadence submissions for one EOA (so two Flow +// transactions land in different blocks and cannot be reordered by Collection +// Nodes) and (b) the flush deadline anchored at first enqueue. There is +// intentionally NO separate hard-cap knob. // -// Note on locking: fast-path submissions hold the pool-wide queue lock for -// the duration of one Flow submission, trading cross-EOA throughput for the -// simplicity of atomic state updates; a per-EOA lock is the known upgrade -// path if contention shows up. +// Note on locking: fast-path submissions hold the pool-wide queue lock for the +// duration of one Flow submission, trading cross-EOA throughput for the +// simplicity of atomic state updates; a per-EOA lock is the known upgrade path +// if contention shows up. type TxMemPool struct { *SingleTxPool nonceProvider NonceProvider From d1db5d80a2028776897826f17219af7bbc8244fe Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:07:47 -0400 Subject: [PATCH 10/31] docs(requester): audit and tighten tx_mempool comments Comment-only pass over tx_mempool.go and its test for accuracy, concision and first-reader clarity: - Fix two stale/inaccurate comments: the TTL-expiry block referenced the removed hasInFlight/lastSubmittedNonce identifiers (now the nonceTracker submitting marker), and the far-ahead-nonce tradeoff still described rejection as an unresolved open question when TxMaxNonceGap/classify already enforce a too-high bound at Add time. - Remove personal-name and insider references (Leo, zhangchiqing, PR #965 discussion, Fix 1/2/3/4 test prefixes), restating the rationale plainly. - Trim verbosity across the nonceTracker/classify/markSubmitted/ reconcileSubmission docs while preserving every invariant and rationale. - Call out load-bearing assumptions explicitly (queueMux discipline + single-goroutine access, nonce-ascending inputs, unset nonceWrapper as -inf). No behavior change; build/vet/test/gofmt all clean. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 151 ++++++++++++-------------- services/requester/tx_mempool_test.go | 22 ++-- 2 files changed, 83 insertions(+), 90 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 740f5e21..613b2019 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -24,8 +24,8 @@ import ( // nonce from the local state index as the on-chain frontier and reasons about // each incoming nonce relative to that frontier and to what it has already sent. // -// BEHAVIOR SPEC — every case the pool handles. This is meant to read as a -// checklist for tests; each numbered case maps to code and ideally to a test. +// BEHAVIOR SPEC — every case the pool handles, written as a checklist that maps +// to code and to tests. // // (The examples below assume the EOA's next expected nonce is N unless stated.) // @@ -159,12 +159,10 @@ const txMemPoolTickInterval = 50 * time.Millisecond // recent activity is kept before being removed, to bound memory usage. const idleQueueRetention = time.Minute -// nonceWrapper represents a nonce that may not be set: when set is true, v is a -// valid nonce; when set is false, the nonce has not been initialized yet. It -// lets us compare nonces uniformly even when one may be absent — including the -// ambiguous case where the value is 0 (a valid nonce) but set is false. An unset -// nonceWrapper behaves as -∞ in the comparisons below (atLeast/is/max): it is -// below every real nonce and never "at or above" one, so callers carry no set +// nonceWrapper is a nonce that may be unset (set == false). It disambiguates the +// otherwise ambiguous value 0, which is both a valid nonce and the zero value. +// An unset nonceWrapper behaves as -∞ in the comparisons below (atLeast/is/max): +// below every real nonce and never "at or above" one, so callers need no set // checks. type nonceWrapper struct { v uint64 @@ -215,30 +213,31 @@ const ( // nonceTracker is the per-EOA submission-state machine. It records the nonce // facts the mempool reasons about and answers "what should happen to an // incoming nonce?" (classify) and "what is the next nonce to submit?" -// (expectedNonce), so callers never compare raw fields or write compound -// conditions. All methods assume the pool's queueMux is held: the tracker has -// no lock of its own, and the single submit goroutine plus Add-under-lock model -// guarantees serialized access (see TxMemPool docstring). +// (expectedNonce), so callers never compare raw fields directly. +// +// The tracker has no lock of its own: all methods assume the pool's queueMux is +// held. Serialized access is guaranteed by the single submit goroutine plus +// Add running under that same lock (see the TxMemPool docstring). type nonceTracker struct { // localIndexedNonce is the EOA's next expected nonce per the local state - // index (the on-chain frontier). A CACHE refreshed from a fresh read via - // refreshIndexed — a fact about the chain, not about our sends. + // index (the on-chain frontier). A cache of a fact about the chain, refreshed + // from a fresh read via refreshIndexed — not a record of our own sends. localIndexedNonce uint64 - // submitting is the highest nonce SENT to Flow but not yet ack'd: the window - // between collecting a batch and its submit result. Unset when no submission - // is outstanding. It means strictly "a network call is in flight" and is - // cleared the moment that call returns — by markSubmitted on success or - // rollbackSubmitting on failure. + // submitting is the highest nonce sent to Flow but not yet ack'd. It marks the + // window between detaching a batch and its submit result returning, and + // strictly means "a network call is in flight": it is cleared the moment that + // call returns, by markSubmitted on success or rollbackSubmitting on failure. + // Unset when no submission is outstanding. submitting nonceWrapper - // lastConsecutivelySubmitted is the highest nonce CONSECUTIVELY, SUCCESSFULLY - // submitted (ack'd). Unset before the EOA's first success. Only consecutive - // submissions advance it; TTL-expiry (gapped) batches do NOT, so it never - // includes a nonce past a gap. + // lastConsecutivelySubmitted is the highest nonce consecutively and + // successfully submitted (ack'd). Only consecutive-prefix batches advance it; + // TTL-expiry (gapped) batches do NOT, so it never includes a nonce past a gap. + // Unset before the EOA's first success. lastConsecutivelySubmitted nonceWrapper // maxNonceGap is how far above localIndexedNonce a nonce may be before it is - // rejected as too-high. 0 means no upper bound. It does NOT affect the - // too-low check (a nonce below localIndexedNonce is always rejected). Set - // once from config when the queue is created. + // rejected as too-high; 0 means no upper bound. It does NOT affect the too-low + // check (a nonce below localIndexedNonce is always rejected). Set once from + // config when the queue is created. maxNonceGap uint64 } @@ -265,13 +264,13 @@ func (n *nonceTracker) expectedNonce() uint64 { // on-chain frontier as part of the decision. // // A nonce at or below our highest already-sent nonce is an in-flight/duplicate -// retry, rejected from local state alone — no index read. We check this first -// precisely because reading the frontier builds a block view (the expensive -// step) that the retry case must avoid. +// retry, decided from local state alone with no index read. This case is checked +// FIRST because reading the frontier builds a block view (the expensive step), +// which the retry path must avoid. // // Any other nonce is beyond what we've sent, so the remaining verdicts -// (too-low/too-high/next-expected) need the on-chain frontier: we read and -// refresh it. A read error is an exception (a local state-index read should not +// (too-low/too-high/next-expected) need the frontier, which is read and +// refreshed. A read error is an exception (a local state-index read should not // fail) and is returned so the caller can reject the transaction. Pruning of any // now-stale queued txs is deferred to collectDueBatches. // @@ -310,32 +309,30 @@ func (n *nonceTracker) classify( } // markSubmitting records that nonces up to highNonce have been sent but not yet -// ack'd. Set when a batch is detached for async submission in collectDueBatches -// (the synchronous fast path in Add skips it — it holds the lock across the -// whole submit, so there is no concurrency window to guard). +// ack'd. Called when a batch is detached for async submission in +// collectDueBatches. The synchronous fast path in Add skips it: Add holds the +// lock across the whole submit, so there is no concurrency window to guard. func (n *nonceTracker) markSubmitting(highNonce uint64) { n.submitting = toNonceWrapper(highNonce) } // markSubmitted acks a successful submission: it advances the consecutively- -// submitted nonce and clears the in-flight marker. Called the moment a -// submission succeeds, EXPLICITLY and under the lock, rather than waiting for -// the index to confirm — so `submitting` strictly means "a network call is -// outstanding" and is cleared as soon as the call returns (here on success, or -// via rollbackSubmitting on failure). This costs one quick lock per successful -// submission, which we accept for a state machine that is trivial to reason +// submitted nonce and clears the in-flight marker. It runs the moment the +// submission succeeds, under the lock, rather than waiting for the index to +// confirm — so `submitting` strictly means "a network call is outstanding" and +// clears as soon as the call returns. The cost is one quick lock per successful +// submission, accepted in exchange for a state machine that is easy to reason // about. func (n *nonceTracker) markSubmitted(highNonce uint64) { n.lastConsecutivelySubmitted = toNonceWrapper(highNonce) n.submitting = nonceWrapper{} } -// rollbackSubmitting clears the in-flight marker after a FAILED submission, but +// rollbackSubmitting clears the in-flight marker after a failed submission, but // only when it still refers to highNonce (a newer submission may have replaced // it). Because a failure never advances lastConsecutivelySubmitted, there is // nothing else to undo: the next flush recomputes expectedNonce from the -// unchanged frontier. This replaces the old, fragile -// "lastSubmittedNonce == batchMax" guard. +// unchanged frontier. func (n *nonceTracker) rollbackSubmitting(highNonce uint64) { if n.submitting.is(highNonce) { n.submitting = nonceWrapper{} @@ -357,10 +354,10 @@ type eoaQueue struct { // not flushed until the current time has passed this instant: it marks the // end of the sliding collection window, NOT a deadline to act before. collectionWindowEndsAt time.Time - // flushDeadline is firstEnqueue + TxSubmissionSpacing. It caps how long - // a continuously-resetting collection window can defer a flush. There is - // deliberately no separate "hard cap" knob: TxSubmissionSpacing serves - // both purposes (see PR #965 discussion). + // flushDeadline is firstEnqueue + TxSubmissionSpacing. It caps how long a + // continuously-resetting collection window can defer a flush. There is + // deliberately no separate "hard cap" knob: TxSubmissionSpacing serves both + // purposes. flushDeadline time.Time // lastSubmittedAt is when the last Cadence tx for this EOA was submitted // (used for submission spacing). @@ -530,12 +527,11 @@ func (t *TxMemPool) Add( } // Fast path: a next-expected nonce with an empty queue, nothing in flight and - // spacing satisfied is submitted immediately — zero added latency. The lock - // is held across the whole submit, so there is no concurrency window: no need - // to mark "submitting" first; on success we record the ack, on failure we - // leave the EOA untouched. If we cannot fast-path yet (queue non-empty, in - // flight, or spacing not elapsed), fall through to enqueue and let the - // background loop flush it. + // spacing satisfied is submitted immediately — zero added latency. The lock is + // held across the whole submit, so there is no concurrency window: no need to + // mark "submitting" first; on success we record the ack, on failure we leave + // the EOA untouched. Otherwise (queue non-empty, in flight, or spacing not + // elapsed) fall through to enqueue for the background loop. if verdict == nonceNextExpected && q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { batch := []heldTx{userTx} @@ -631,9 +627,9 @@ func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { } // logSubmission records the fate of a submitted batch so a transaction is never -// silently lost. This is the observability half of Leo's invariant: for any tx -// id, you can either find it on-chain (sent) OR find a WARN log here (dropped) — -// never nothing. +// silently lost. This is the observability half of the no-silent-drops +// invariant: for any tx id you can either find it on-chain (sent) OR find a WARN +// log here (dropped) — never nothing. // // - On a Flow submit FAILURE the batch's EVM transactions are dropped (we do // not retry — clients resubmit), so we WARN with everything needed to debug @@ -689,20 +685,19 @@ func (t *TxMemPool) logSubmission( // "submitting" (under the lock) before the network call; this records the // outcome: // -// - On SUCCESS we explicitly advance the consecutively-submitted nonce now +// - On SUCCESS it advances the consecutively-submitted nonce now // (markSubmitted), under the lock, rather than waiting for the index to -// confirm. This is the deliberate "update on success" decision: it keeps -// `submitting` meaning strictly "a network call is outstanding". +// confirm — keeping `submitting` meaning strictly "a network call is +// outstanding". // // - On FAILURE the batch's transactions are dropped (already counted and -// logged by submitBatch) and never reach the chain, so we clear the -// "submitting" marker (rollbackSubmitting). Without this, every resubmission -// of those nonces would be rejected as in-flight forever and the index would -// never advance to clear the marker — the EOA would be permanently wedged. -// rollbackSubmitting clears the marker only if it still refers to this -// batch (a newer submission may have replaced it). lastSubmittedAt is -// deliberately NOT restored: a brief, self-correcting spacing delay after a -// rare failure is harmless. +// logged) and never reach the chain, so the "submitting" marker is cleared +// (rollbackSubmitting). Without this, resubmissions of those nonces would be +// rejected as in-flight forever — the index never advances to clear the +// marker, so the EOA would be permanently wedged. rollbackSubmitting clears +// the marker only if it still refers to this batch (a newer submission may +// have replaced it). lastSubmittedAt is deliberately NOT restored: a brief, +// self-correcting spacing delay after a rare failure is harmless. // // TTL-expiry batches (w.inFlight == false) never mark the tracker, so there is // nothing to reconcile for them. @@ -821,16 +816,14 @@ func (t *TxMemPool) collectDueBatches() []flushWork { // Rationale (no silent drops): submitting an unexecutable transaction // produces a real, observable on-chain failure (operators can see the // failed Flow transaction and its nonce-mismatch), whereas silently - // dropping it leaves no trace. The no-silent-drop requirement is the - // whole reason this pool exists (see PR #965 / DFNS), so an observable - // failure is strictly preferable to an invisible drop. + // dropping it leaves no trace. Avoiding silent drops is the whole reason + // this pool exists, so an observable failure is strictly preferable to an + // invisible drop. // - // Known tradeoff (zhangchiqing): a transaction whose nonce is far - // ahead of the index will still be submitted at TTL and burn fees on a - // guaranteed failure. Rejecting far-ahead nonces in Add is deliberately - // NOT done — it remains an open design question (how far ahead is "too - // far", and whether to add a knob for it), and is intentionally left - // out of this change rather than guessed at. + // Tradeoff: a tx whose nonce is far ahead of the index is still + // submitted at TTL and burns fees on a guaranteed failure. How far + // ahead a nonce may be is bounded instead at Add time by TxMaxNonceGap + // (see classify); when that is unset (0), far-ahead nonces reach here. // // Cap the batch at TxMaxBatchSize so a long-lived gap cannot produce an // unbounded Flow transaction; the remainder drains on later ticks, @@ -843,9 +836,9 @@ func (t *TxMemPool) collectDueBatches() []flushWork { for _, htx := range expired { delete(q.txs, htx.nonce) } - // Deliberately do NOT set hasInFlight/lastSubmittedNonce here: these - // nonces are out of order, and marking them in flight would - // corrupt the expected-nonce computation for future flushes. + // Deliberately do NOT mark the nonceTracker submitting here: these + // nonces are out of order (past a gap), and marking them in flight + // would corrupt the expected-nonce computation for future flushes. q.lastSubmittedAt = now q.lastActivity = now txHashes := make([]string, len(expired)) diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 92d6aa65..5d5e0a67 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -397,10 +397,10 @@ func Test_TxMemPool_SuccessfulFlushMarksSubmitted(t *testing.T) { assert.Equal(t, toNonceWrapper(1), q.nonces.lastConsecutivelySubmitted) } -// No silent drops (Leo's invariant): a failed flush submission must produce an -// observable WARN log carrying the dropped tx hashes and enough context to -// debug a "lost transaction" report (eoa, nonce range, indexed nonce, batch -// size, reason, error). +// No silent drops: a failed flush submission must produce an observable WARN log +// carrying the dropped tx hashes and enough context to debug a "lost +// transaction" report (eoa, nonce range, indexed nonce, batch size, reason, +// error). func Test_TxMemPool_FailedSubmitLogsDropWarning(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) @@ -479,8 +479,8 @@ func Test_TxMemPool_SuccessfulSubmitLogsDebug(t *testing.T) { assert.Contains(t, out, `"high-nonce":1`) } -// Fix 1: a failed fast-path submission must not rate-limit the EOA via -// lastSubmittedAt, and must leave nothing in flight. +// A failed fast-path submission must not rate-limit the EOA via lastSubmittedAt, +// and must leave nothing in flight. func Test_TxMemPool_FailedFastPathLeavesNoState(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) @@ -503,8 +503,8 @@ func Test_TxMemPool_FailedFastPathLeavesNoState(t *testing.T) { assert.True(t, q.lastSubmittedAt.IsZero(), "failed submission must not stamp lastSubmittedAt") } -// Fix 2: resubmitting a single held tx with the same nonce must not re-arm the -// flush deadline anchored at first enqueue. +// Resubmitting a single held tx with the same nonce must not re-arm the flush +// deadline anchored at first enqueue. func Test_TxMemPool_SameNonceReplacementKeepsFlushDeadline(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) @@ -525,7 +525,7 @@ func Test_TxMemPool_SameNonceReplacementKeepsFlushDeadline(t *testing.T) { assert.Equal(t, firstDeadline, pool.queues[from].flushDeadline) } -// Fix 3: TTL-expiry flushes are capped at TxMaxBatchSize. +// TTL-expiry flushes are capped at TxMaxBatchSize. func Test_TxMemPool_TTLFlushCappedAtMaxBatch(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) @@ -559,8 +559,8 @@ func Test_TxMemPool_TTLFlushCappedAtMaxBatch(t *testing.T) { assert.Len(t, pool.queues[from].txs, 4) } -// Fix 4: a queue emptied without ever submitting (e.g. all txs pruned) must -// still age out via lastActivity rather than leaking forever. +// A queue emptied without ever submitting (e.g. all txs pruned) must still age +// out via lastActivity rather than leaking forever. func Test_TxMemPool_EmptyQueueAgesOut(t *testing.T) { pool := newTestPool( &fakeNonceProvider{nonce: 0}, From 8a9053c3d0348b690336ccb346ef065d556e64fe Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:32:21 -0400 Subject: [PATCH 11/31] test(requester): inject a clock to test timing behavior through the real path Audit finding H1: the pool called time.Now() directly, so the core time-based behavior (collection-window reset, flush deadline, submission spacing, TTL expiry, idle-queue retention) could only be tested by hand-building deadline timestamps, never through the real Add->tick->flush path. Add an injectable now func() time.Time field (defaulting to time.Now, mirroring the existing submitBatch field-injection pattern) and route the three direct time.Now() calls through it. Add a controllable fakeClock and five clock-driven tests exercising: window reset on each arrival, the spacing gate deferring then releasing a flush, the flush deadline forcing a flush while the window is still open, TTL submit-anyway, and idle-queue eviction. No behavior change; race-clean. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 12 +- services/requester/tx_mempool_test.go | 219 ++++++++++++++++++++++++++ 2 files changed, 228 insertions(+), 3 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 613b2019..641c9de6 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -406,6 +406,11 @@ type TxMemPool struct { // submitBatch performs the actual Flow submission. It defaults to // submitTxBatch and exists as a field so tests can inject a fake. submitBatch func(ctx context.Context, txs []heldTx) error + // now returns the current time. It defaults to time.Now and exists as a + // field so tests can drive the collection window, flush deadline, submission + // spacing, TTL expiry and idle-queue retention with a controllable clock + // rather than wall-clock sleeps. + now func() time.Time } var _ TxPool = &TxMemPool{} @@ -431,6 +436,7 @@ func NewTxMemPool( SingleTxPool: singleTxPool, nonceProvider: nonceProvider, queues: make(map[gethCommon.Address]*eoaQueue), + now: time.Now, } pool.submitBatch = pool.submitTxBatch @@ -487,7 +493,7 @@ func (t *TxMemPool) Add( t.queues[from] = q } - now := time.Now() + now := t.now() // The EOA was "touched" even if this turns out to be a duplicate, so record // activity here to keep the idle-queue retention clock accurate. q.lastActivity = now @@ -541,7 +547,7 @@ func (t *TxMemPool) Add( return submitErr } q.nonces.markSubmitted(tx.Nonce()) - q.lastSubmittedAt = time.Now() + q.lastSubmittedAt = t.now() return nil } @@ -729,7 +735,7 @@ func (t *TxMemPool) collectDueBatches() []flushWork { t.queueMux.Lock() defer t.queueMux.Unlock() - now := time.Now() + now := t.now() work := make([]flushWork, 0) // Each due EOA's nonce is read via GetNonce; the provider caches the block diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 5d5e0a67..9c2ca41c 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -6,6 +6,7 @@ import ( "crypto/ecdsa" "errors" "math/big" + "sync" "testing" "time" @@ -133,11 +134,34 @@ func newTestPool( }, nonceProvider: np, queues: make(map[gethCommon.Address]*eoaQueue), + now: time.Now, } pool.submitBatch = submit return pool } +// fakeClock is a controllable clock for driving the pool's time-based behavior +// in tests without wall-clock sleeps. It is safe for the single-goroutine unit +// tests here (Add and collectDueBatches are called synchronously). +type fakeClock struct { + mu sync.Mutex + t time.Time +} + +func newFakeClock(t time.Time) *fakeClock { return &fakeClock{t: t} } + +func (c *fakeClock) now() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + return c.t +} + +func (c *fakeClock) advance(d time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + c.t = c.t.Add(d) +} + func testPoolConfig() config.Config { return config.Config{ TxCollectionWindow: 100 * time.Millisecond, @@ -778,3 +802,198 @@ func Test_NonceTracker_Transitions(t *testing.T) { n.refreshIndexed(12) assert.Equal(t, uint64(12), n.localIndexedNonce) } + +// --- Clock-driven timing tests ------------------------------------------- +// These drive the collection window, flush deadline, submission spacing, TTL +// expiry and idle-queue retention through the real Add -> collectDueBatches +// path using an injected clock, rather than hand-building deadline timestamps. + +var timingClockBase = time.Unix(1_700_000_000, 0) + +// The sliding collection window is re-armed on every arrival. +func Test_TxMemPool_CollectionWindowResetsOnEachArrival(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + clk := newFakeClock(timingClockBase) + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, _ []heldTx) error { return nil }, + testPoolConfig(), + ) + pool.now = clk.now + + // A gapped nonce (frontier 0, nonce 5) queues rather than fast-pathing. + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 5, 1))) + q := pool.queues[from] + require.NotNil(t, q) + assert.Equal(t, timingClockBase.Add(testPoolConfig().TxCollectionWindow), q.collectionWindowEndsAt) + firstDeadline := q.flushDeadline + + // A later arrival re-arms the window relative to its own arrival time, but + // must NOT re-arm the first-enqueue flush deadline. + clk.advance(40 * time.Millisecond) + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 6, 1))) + assert.Equal(t, clk.now().Add(testPoolConfig().TxCollectionWindow), q.collectionWindowEndsAt) + assert.Equal(t, firstDeadline, q.flushDeadline, "flush deadline stays anchored at first enqueue") +} + +// Submission spacing gates the background flush: a due batch is held until +// TxSubmissionSpacing has elapsed since the previous submission, then flushed. +func Test_TxMemPool_SpacingGateDefersThenFlushes(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + clk := newFakeClock(timingClockBase) + var submitted [][]heldTx + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, txs []heldTx) error { + submitted = append(submitted, txs) + return nil + }, + testPoolConfig(), + ) + pool.now = clk.now + cfg := testPoolConfig() + + // nonce 0 fast-paths immediately and stamps lastSubmittedAt = base. + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 0, 1))) + require.Len(t, submitted, 1) + // nonces 1,2 arrive while spacing has not elapsed, so they queue. + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 1, 1))) + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 2, 1))) + + // Past the collection window but within the spacing gap: NOT flushed yet. + clk.advance(cfg.TxCollectionWindow + 50*time.Millisecond) + pool.collectDueBatches() + assert.Len(t, submitted, 1, "spacing gate must defer the flush") + assert.Len(t, pool.queues[from].txs, 2) + + // Once spacing has elapsed, the consecutive prefix {1,2} flushes. + clk.advance(cfg.TxSubmissionSpacing) + for _, w := range pool.collectDueBatches() { + require.NoError(t, pool.submitWork(context.Background(), w)) + } + require.Len(t, submitted, 2) + assert.Equal(t, []uint64{1, 2}, noncesOf(submitted[1])) + assert.Empty(t, pool.queues[from].txs) +} + +// The first-enqueue flush deadline forces a flush even while the (continuously +// re-armed) collection window is still in the future. +func Test_TxMemPool_FlushDeadlineForcesFlushWhileWindowOpen(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + clk := newFakeClock(timingClockBase) + var submitted [][]heldTx + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, txs []heldTx) error { + submitted = append(submitted, txs) + return nil + }, + testPoolConfig(), + ) + pool.now = clk.now + cfg := testPoolConfig() + + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 0, 1))) // fast-path + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 1, 1))) // queues; deadline = base + spacing + + // Just before the deadline, re-arm the window with a late arrival so the + // window remains in the future at flush time. + clk.advance(cfg.TxSubmissionSpacing - 50*time.Millisecond) + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 2, 1))) + q := pool.queues[from] + require.True(t, clk.now().Before(q.collectionWindowEndsAt), "window must still be open") + + // Cross the deadline (still before the window end): the deadline forces it. + clk.advance(60 * time.Millisecond) + require.True(t, clk.now().Before(q.collectionWindowEndsAt), "window still open at flush time") + require.False(t, clk.now().Before(q.flushDeadline), "deadline has passed") + for _, w := range pool.collectDueBatches() { + require.NoError(t, pool.submitWork(context.Background(), w)) + } + require.Len(t, submitted, 2) + assert.Equal(t, []uint64{1, 2}, noncesOf(submitted[1])) +} + +// A nonce stuck behind a permanent head gap is submitted anyway once TxPoolTTL +// elapses (driven by the clock via enqueuedAt), not silently dropped. +func Test_TxMemPool_TTLExpiryViaClock(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + clk := newFakeClock(timingClockBase) + var submitted []flushWork + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, _ []heldTx) error { return nil }, + testPoolConfig(), + ) + pool.now = clk.now + cfg := testPoolConfig() + + // Frontier 0, nonce 10 — a permanent head gap, so the only exit is TTL. + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 10, 1))) + + // Before TTL: held, not submitted. + clk.advance(cfg.TxCollectionWindow + 50*time.Millisecond) + assert.Empty(t, pool.collectDueBatches()) + assert.Len(t, pool.queues[from].txs, 1) + + // Past TTL: submitted anyway, as a non-in-flight TTL batch. + clk.advance(cfg.TxPoolTTL) + work := pool.collectDueBatches() + require.Len(t, work, 1) + submitted = work + assert.False(t, submitted[0].inFlight) + assert.Equal(t, flushReasonTTL, submitted[0].reason) + assert.Equal(t, []uint64{10}, noncesOf(submitted[0].txs)) + assert.Empty(t, pool.queues[from].txs) +} + +// An empty queue with no activity past idleQueueRetention is evicted. +func Test_TxMemPool_IdleQueueEvictedViaClock(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + clk := newFakeClock(timingClockBase) + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, _ []heldTx) error { return nil }, + testPoolConfig(), + ) + pool.now = clk.now + + // nonce 0 fast-paths and leaves an empty queue with lastActivity = base. + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 0, 1))) + require.NotNil(t, pool.queues[from]) + + // Within retention: kept. + clk.advance(idleQueueRetention / 2) + pool.collectDueBatches() + assert.NotNil(t, pool.queues[from]) + + // Past retention: evicted. + clk.advance(idleQueueRetention) + pool.collectDueBatches() + _, ok := pool.queues[from] + assert.False(t, ok, "idle empty queue must be evicted past retention") +} + +// noncesOf extracts the nonces of a batch in order, for concise assertions. +func noncesOf(txs []heldTx) []uint64 { + ns := make([]uint64, len(txs)) + for i, htx := range txs { + ns[i] = htx.nonce + } + return ns +} From c70572263ba83f089213496e4caf67282238b6a7 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:33:32 -0400 Subject: [PATCH 12/31] docs/test(requester): correct burst example, test the real duplicate path Two accuracy fixes from the audit: - The Case 2 (burst batching) example in the behavior spec was misleading: it showed nonces 5,6,7 being collected together, but if 5 is the next expected nonce on an empty queue it fast-paths (Case 1) rather than queueing. Reword the example so the lead nonce is sent and the following nonces queue behind the submission-spacing gap, and note the fast-path caveat. - Add Test_TxMemPool_DuplicateQueuedTxRejected. The existing InFlightDuplicateRejected test is named "duplicate" but actually exercises the in-flight rejection (the first tx is submitted and removed from the queue before the second Add), leaving the genuine duplicate-of-a-held-tx path (Case 9) untested. The new test holds a tx in the queue, asserts the identical tx is rejected with ErrDuplicateTransaction, and that a same-nonce different-hash tx replaces it (last write wins). No behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 10 +++++--- services/requester/tx_mempool_test.go | 33 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 641c9de6..f02f81e1 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -40,9 +40,13 @@ import ( // arrival) decides when a burst is complete; a flush deadline anchored at // the FIRST enqueue (TxSubmissionSpacing) caps how long a continuously // resetting window can defer the flush. -// Example: nonces 5,6,7 arrive within a few ms of each other; the window -// keeps resetting, so they are collected together and flushed as one batch -// once arrivals pause for TxCollectionWindow (or the deadline is hit). +// Example: nonce 5 fast-paths and is sent; nonces 6,7,8 then arrive a few +// ms apart while the submission-spacing gap since that send is still open, +// so they cannot fast-path and queue instead. The window keeps resetting as +// they arrive, and they flush together as one batch once arrivals pause for +// TxCollectionWindow (or the deadline is hit) AND spacing has elapsed. (A +// burst whose lead nonce is itself the next expected one on an empty, idle +// queue would fast-path that first tx — case 1 — not queue it.) // 3. Consecutive-prefix flush: on flush, the longest run of consecutive nonces // starting at the expected nonce is sent as ONE Cadence transaction, capped // at TxMaxBatchSize. A nonce gap splits the queue: only the prefix before diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 9c2ca41c..6da35e8e 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -303,6 +303,39 @@ func Test_TxMemPool_InFlightDuplicateRejected(t *testing.T) { assert.ErrorIs(t, err, errs.ErrInFlightNonce) } +// Case 9 (duplicate): re-adding the IDENTICAL transaction (same nonce AND same +// hash) of one already HELD in the queue is rejected with +// ErrDuplicateTransaction, while a same-nonce tx with a DIFFERENT hash replaces +// the queued one (last write wins). This exercises the duplicate check itself, +// distinct from the in-flight rejection above (which removes the tx from the +// queue before the second Add). +func Test_TxMemPool_DuplicateQueuedTxRejected(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + from := crypto.PubkeyToAddress(key.PublicKey) + + pool := newTestPool( + // Frontier 0, so a nonce-5 tx is out of order and is HELD (not submitted). + &fakeNonceProvider{nonce: 0}, + func(_ context.Context, _ []heldTx) error { return nil }, + testPoolConfig(), + ) + + tx := signedTestTx(t, key, 5, 1) + require.NoError(t, pool.Add(context.Background(), tx)) + + // Re-adding the identical tx (same hash) is a duplicate. + err = pool.Add(context.Background(), tx) + assert.ErrorIs(t, err, errs.ErrDuplicateTransaction) + + // A same-nonce, different-payload (different hash) tx is NOT a duplicate; it + // replaces the held one. + replacement := signedTestTx(t, key, 5, 2) + require.NotEqual(t, tx.Hash(), replacement.Hash()) + require.NoError(t, pool.Add(context.Background(), replacement)) + assert.Equal(t, replacement.Hash(), pool.queues[from].txs[5].txHash) +} + func Test_TxMemPool_FailedFlushDoesNotWedgeEOA(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) From ad71012f5fb88a1ee1e716337480916df726898a Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:35:43 -0400 Subject: [PATCH 13/31] fix(requester): bound the fast-path submit to protect the pool lock Audit liveness finding: the fast path holds the pool-wide queueMux across the entire Flow submission, and forwarded the caller (JSON-RPC request) context straight into it. With the default rpc-request-timeout of 120s, a hung Access-node call could pin the global lock for up to two minutes, blocking every other EOA Add and the background flush loop. Wrap the fast-path submit context with a 10s fastPathSubmitTimeout ceiling. This is a liveness safety net, not a latency SLA: normal submits (~sub- second, well under the ~1.2s spacing) return and release the lock at once; only a genuinely stalled call is cut off, and its tx is dropped-and-logged for the client to resubmit. The background submit path does not hold the lock and is left unchanged. Adds a test asserting the fast-path submit context carries a bounded deadline. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 18 +++++++++++++++-- services/requester/tx_mempool_test.go | 28 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index f02f81e1..f6beba3d 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -156,9 +156,19 @@ func selectExpired( // txMemPoolTickInterval is the resolution at which due queues are // scanned and flushed. Deadlines are therefore honored with up to this -// much slack, which is acceptable relative to the 300ms collection window. +// much slack, which is acceptable relative to the collection window. const txMemPoolTickInterval = 50 * time.Millisecond +// fastPathSubmitTimeout bounds how long a single fast-path submission may take. +// The fast path holds the pool-wide queueMux across the whole Flow submission +// (see the note on TxMemPool), so without a bound a hung Access-node call would +// block every other EOA's Add and the background flush loop for as long as the +// caller's context allows — up to RpcRequestTimeout (120s by default). This is a +// liveness safety ceiling, NOT a latency SLA: normal submits complete well +// within it and release the lock immediately; only a genuinely stalled call is +// cut off (and its tx is dropped-and-logged for the client to resubmit). +const fastPathSubmitTimeout = 10 * time.Second + // idleQueueRetention is how long a queue with no held transactions and no // recent activity is kept before being removed, to bound memory usage. const idleQueueRetention = time.Minute @@ -545,7 +555,11 @@ func (t *TxMemPool) Add( if verdict == nonceNextExpected && q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { batch := []heldTx{userTx} - submitErr := t.submitBatch(ctx, batch) + // Bound the submit so a hung call cannot pin queueMux indefinitely + // (see fastPathSubmitTimeout). + submitCtx, cancel := context.WithTimeout(ctx, fastPathSubmitTimeout) + submitErr := t.submitBatch(submitCtx, batch) + cancel() t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) if submitErr != nil { return submitErr diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 6da35e8e..fb063c3a 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -228,6 +228,34 @@ func Test_TxMemPool_FastPathSubmitsImmediately(t *testing.T) { assert.Equal(t, toNonceWrapper(0), q.nonces.lastConsecutivelySubmitted) } +// The fast-path submit must run under a bounded-deadline context so a hung +// Flow call cannot pin the pool-wide queueMux indefinitely (audit liveness +// finding). Before the bound, Add forwarded its raw context (here Background, +// with no deadline) straight into the submit. +func Test_TxMemPool_FastPathSubmitHasBoundedTimeout(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + + var deadline time.Time + var hasDeadline bool + pool := newTestPool( + &fakeNonceProvider{nonce: 0}, + func(ctx context.Context, _ []heldTx) error { + deadline, hasDeadline = ctx.Deadline() + return nil + }, + testPoolConfig(), + ) + + before := time.Now() + require.NoError(t, pool.Add(context.Background(), signedTestTx(t, key, 0, 1))) + + 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, + "deadline must be bounded by fastPathSubmitTimeout") +} + func Test_TxMemPool_UnexpectedNonceEnqueues(t *testing.T) { key, err := crypto.GenerateKey() require.NoError(t, err) From 7303ab9a05d110b4c04cfe15e2128d674613cea3 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:38:38 -0400 Subject: [PATCH 14/31] refactor(requester): mechanical cleanups (helpers, prealloc, wrapping, naming) Low-risk readability cleanups from the audit, behavior-preserving: - Extract deleteByNonce and txHashHexes helpers, replacing four near- identical loops across collectDueBatches, pruneStaleTxs and logSubmission. - Preallocate slices with known caps: selectConsecutivePrefix (maxBatch), selectExpired (len txs), collectDueBatches work (len queues). - Wrap pass-through errors with %w (classify index read, submitTxBatch build/send) for triage context; leave the errs.Err* sentinels bare so errors.Is still matches. - Rename the nonceWrapper receiver o -> w (leftover from the old optionalNonce name) and the Add local userTx -> held. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 75 +++++++++++++++++--------------- 1 file changed, 40 insertions(+), 35 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index f6beba3d..13ad5139 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -3,6 +3,7 @@ package requester import ( "context" "encoding/hex" + "fmt" "sort" "sync" "time" @@ -124,7 +125,7 @@ func selectConsecutivePrefix( expectedNonce uint64, maxBatch int, ) []heldTx { - prefix := make([]heldTx, 0) + prefix := make([]heldTx, 0, maxBatch) for nonce := expectedNonce; len(prefix) < maxBatch; nonce++ { tx, ok := txs[nonce] if !ok { @@ -135,6 +136,22 @@ func selectConsecutivePrefix( return prefix } +// deleteByNonce removes every transaction in batch from txs, keyed by nonce. +func deleteByNonce(txs map[uint64]heldTx, batch []heldTx) { + for _, htx := range batch { + delete(txs, htx.nonce) + } +} + +// txHashHexes returns the hex tx hashes of a batch, for structured log fields. +func txHashHexes(txs []heldTx) []string { + hashes := make([]string, len(txs)) + for i, htx := range txs { + hashes[i] = htx.txHash.Hex() + } + return hashes +} + // selectExpired returns the held transactions older than ttl, sorted by // nonce ascending. func selectExpired( @@ -142,7 +159,7 @@ func selectExpired( now time.Time, ttl time.Duration, ) []heldTx { - expired := make([]heldTx, 0) + expired := make([]heldTx, 0, len(txs)) for _, tx := range txs { if now.Sub(tx.enqueuedAt) > ttl { expired = append(expired, tx) @@ -187,20 +204,20 @@ func toNonceWrapper(v uint64) nonceWrapper { return nonceWrapper{v: v, set: true // atLeast reports whether this nonce is set and >= n. An unset nonce (-∞) is // never >= a real nonce, so it returns false. -func (o nonceWrapper) atLeast(n uint64) bool { return o.set && o.v >= n } +func (w nonceWrapper) atLeast(n uint64) bool { return w.set && w.v >= n } // is reports whether this nonce is set and exactly equals n. -func (o nonceWrapper) is(n uint64) bool { return o.set && o.v == n } +func (w nonceWrapper) is(n uint64) bool { return w.set && w.v == n } // max returns the greater of two optional nonces, treating unset as -∞. -func (o nonceWrapper) max(other nonceWrapper) nonceWrapper { - if !o.set { +func (w nonceWrapper) max(other nonceWrapper) nonceWrapper { + if !w.set { return other } - if other.set && other.v > o.v { + if other.set && other.v > w.v { return other } - return o + return w } // nonceVerdict is what classify decides should happen to an incoming nonce. @@ -302,7 +319,7 @@ func (n *nonceTracker) classify( // Beyond what we've sent: refresh the frontier for the remaining verdicts. indexNonce, err := np.GetNonce(from) if err != nil { - return 0, err + return 0, fmt.Errorf("reading indexed nonce for %s: %w", from.Hex(), err) } n.refreshIndexed(indexNonce) @@ -539,7 +556,7 @@ func (t *TxMemPool) Add( // Past every cheap rejection: the transaction will be kept (submitted now or // held), so build the heldTx now rather than before the checks above. - userTx := heldTx{ + held := heldTx{ txPayload: hexEncodedTx, txHash: tx.Hash(), nonce: tx.Nonce(), @@ -554,7 +571,7 @@ func (t *TxMemPool) Add( // elapsed) fall through to enqueue for the background loop. if verdict == nonceNextExpected && q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { - batch := []heldTx{userTx} + batch := []heldTx{held} // Bound the submit so a hung call cannot pin queueMux indefinitely // (see fastPathSubmitTimeout). submitCtx, cancel := context.WithTimeout(ctx, fastPathSubmitTimeout) @@ -573,7 +590,7 @@ func (t *TxMemPool) Add( // yet). A same-nonce, different-payload resubmission replaces the queued // transaction (last write wins), matching mempool semantics. wasEmpty := q.isEmpty() - q.txs[tx.Nonce()] = userTx + q.txs[tx.Nonce()] = held q.collectionWindowEndsAt = now.Add(t.config.TxCollectionWindow) // Anchor the flush deadline at the FIRST enqueue only. Re-arming it on a // same-nonce replacement would let a client defer the flush indefinitely @@ -678,14 +695,10 @@ func (t *TxMemPool) logSubmission( highNonce := txs[len(txs)-1].nonce if submitErr != nil { - txHashes := make([]string, len(txs)) - for i, htx := range txs { - txHashes[i] = htx.txHash.Hex() - } t.logger.Warn(). Err(submitErr). Str("eoa", from.Hex()). - Strs("tx-hashes", txHashes). + Strs("tx-hashes", txHashHexes(txs)). Uint64("low-nonce", lowNonce). Uint64("high-nonce", highNonce). Uint64("local-indexed-nonce", localIndexedNonce). @@ -754,7 +767,7 @@ func (t *TxMemPool) collectDueBatches() []flushWork { defer t.queueMux.Unlock() now := t.now() - work := make([]flushWork, 0) + work := make([]flushWork, 0, len(t.queues)) // Each due EOA's nonce is read via GetNonce; the provider caches the block // view by indexed height, so all reads in this pass (and across ticks at the @@ -811,9 +824,7 @@ func (t *TxMemPool) collectDueBatches() []flushWork { // transaction fail. prefix := selectConsecutivePrefix(q.txs, q.nonces.expectedNonce(), t.config.TxMaxBatchSize) if len(prefix) > 0 { - for _, htx := range prefix { - delete(q.txs, htx.nonce) - } + deleteByNonce(q.txs, prefix) // Optimistically mark the batch submitting; reconcileSubmission // advances submitted on success or clears it on failure. q.nonces.markSubmitting(prefix[len(prefix)-1].nonce) @@ -857,19 +868,13 @@ func (t *TxMemPool) collectDueBatches() []flushWork { expired = expired[:t.config.TxMaxBatchSize] } if len(expired) > 0 { - for _, htx := range expired { - delete(q.txs, htx.nonce) - } + deleteByNonce(q.txs, expired) // Deliberately do NOT mark the nonceTracker submitting here: these // nonces are out of order (past a gap), and marking them in flight // would corrupt the expected-nonce computation for future flushes. q.lastSubmittedAt = now q.lastActivity = now - txHashes := make([]string, len(expired)) - for i, htx := range expired { - txHashes[i] = htx.txHash.Hex() - } - t.logger.Warn().Strs("tx-hashes", txHashes).Str("eoa", from.Hex()). + t.logger.Warn().Strs("tx-hashes", txHashHexes(expired)).Str("eoa", from.Hex()). Uint64("local-indexed-nonce", indexNonce). Uint64("expected-nonce", q.nonces.expectedNonce()). Msg("nonce gap never filled within TTL, submitting held transactions anyway") @@ -903,15 +908,15 @@ func (t *TxMemPool) pruneStaleTxs( from gethCommon.Address, indexNonce uint64, ) { - stale := make([]string, 0) + var stale []heldTx for nonce, htx := range q.txs { if nonce < indexNonce { - stale = append(stale, htx.txHash.Hex()) - delete(q.txs, nonce) + stale = append(stale, htx) } } if len(stale) > 0 { - t.logger.Warn().Strs("tx-hashes", stale).Str("eoa", from.Hex()). + deleteByNonce(q.txs, stale) + t.logger.Warn().Strs("tx-hashes", txHashHexes(stale)).Str("eoa", from.Hex()). Uint64("local-indexed-nonce", indexNonce). Msg("dropping stale transactions with nonce below indexed state") } @@ -946,12 +951,12 @@ func (t *TxMemPool) submitTxBatch(ctx context.Context, txs []heldTx) error { ) if err != nil { t.collector.TransactionsDropped(len(txs)) - return err + return fmt.Errorf("building Flow transaction: %w", err) } if err := t.client.SendTransaction(ctx, *flowTx); err != nil { t.collector.TransactionsDropped(len(txs)) - return err + return fmt.Errorf("sending Flow transaction: %w", err) } return nil From b04c8edc376b6bd2a497e4e3c10e6ca4a3429ed9 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:40:44 -0400 Subject: [PATCH 15/31] refactor(requester): decompose collectDueBatches into focused helpers Audit finding M1: collectDueBatches was ~145 lines spanning five responsibilities (idle eviction, due/spacing gating, frontier refresh + stale prune, consecutive-prefix collection, TTL-expiry collection, metrics). Split it into: - collectDueBatches: lock + iterate + reportSize - collectQueue: the per-EOA gating + at-most-one-batch decision - collectPrefix: the consecutive-prefix path - collectExpired: the TTL submit-anyway path - reportSize: the memory-footprint metric The prefix-vs-TTL asymmetry (prefix marks submitting + re-arms windows; TTL does neither) is now visible as two sibling functions instead of being implied by a continue 80 lines down. Pure behavior-preserving refactor; unit + timing tests pass unchanged, race-clean. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 270 +++++++++++++++++-------------- 1 file changed, 152 insertions(+), 118 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 13ad5139..98409303 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -759,145 +759,179 @@ func (t *TxMemPool) reconcileSubmission(w flushWork, submitErr error) { q.nonces.markSubmitted(highNonce) } -// collectDueBatches selects, under the queue lock, every batch that is due -// for submission, updates the queue state optimistically, and returns the -// detached work items. +// collectDueBatches selects, under the queue lock, every batch that is due for +// submission, updates the queue state optimistically, and returns the detached +// work items. The per-EOA decision lives in collectQueue. +// +// Each due EOA's nonce is read via GetNonce; the provider caches the block view +// by indexed height, so all reads in this pass (and across ticks at the same +// height) reuse one built view rather than rebuilding per address. func (t *TxMemPool) collectDueBatches() []flushWork { t.queueMux.Lock() defer t.queueMux.Unlock() now := t.now() work := make([]flushWork, 0, len(t.queues)) - - // Each due EOA's nonce is read via GetNonce; the provider caches the block - // view by indexed height, so all reads in this pass (and across ticks at the - // same height) reuse one built view rather than rebuilding per address. for from, q := range t.queues { - if q.isEmpty() { - // Bound memory: drop queues with no held txs and no activity past - // the retention period. Any in-flight submission has long since - // resolved on-chain after this window, so discarding a lingering - // in-flight marker here is safe — a later transaction for the EOA - // creates a fresh queue and re-reads the index nonce. - if now.Sub(q.lastActivity) > idleQueueRetention { - delete(t.queues, from) - } - continue + if w, ok := t.collectQueue(from, q, now); ok { + work = append(work, w) } + } + t.reportSize() + return work +} - // Not due yet: both the sliding window and the flush deadline are - // still in the future. - if now.Before(q.collectionWindowEndsAt) && now.Before(q.flushDeadline) { - continue +// collectQueue evaluates one EOA's queue and returns its due batch, if any. It +// applies the gating checks (idle eviction, due-time, submission spacing, a +// fresh frontier read + stale prune) and then collects at most one batch: +// the consecutive prefix takes precedence, and only when a gap at the head +// blocks it does the TTL-expiry path run. The post-gap / over-cap remainder is +// left in the queue for a later tick — never merged with this batch, since a +// head gap would fail the whole Flow transaction. Callers must hold queueMux; +// it may evict an idle empty queue as a side effect. +func (t *TxMemPool) collectQueue( + from gethCommon.Address, + q *eoaQueue, + now time.Time, +) (flushWork, bool) { + if q.isEmpty() { + // Bound memory: drop queues with no held txs and no activity past the + // retention period. Any in-flight submission has long since resolved + // on-chain after this window, so discarding a lingering in-flight marker + // here is safe — a later transaction for the EOA creates a fresh queue + // and re-reads the index nonce. + if now.Sub(q.lastActivity) > idleQueueRetention { + delete(t.queues, from) } + return flushWork{}, false + } - // Safety gap since the previous submission not yet elapsed. - if !t.spacingElapsed(q, now) { - continue - } + // Not due yet: both the sliding window and the flush deadline are still in + // the future. + if now.Before(q.collectionWindowEndsAt) && now.Before(q.flushDeadline) { + return flushWork{}, false + } - indexNonce, err := t.nonceProvider.GetNonce(from) - if err != nil { - // Exception: a local state-index nonce read should not fail - // under normal operation. This is a background loop with no - // caller to reject the tx to, so skip this EOA for the current - // tick (its batch is deferred until the read succeeds) without - // aborting the whole flush for other EOAs. - t.logger.Error().Err(err).Str("eoa", from.Hex()). - Msg("unexpected failure reading nonce from local index, skipping EOA this tick") - continue - } + // Safety gap since the previous submission not yet elapsed. + if !t.spacingElapsed(q, now) { + return flushWork{}, false + } - q.nonces.refreshIndexed(indexNonce) - - // Prune transactions that can never execute: their nonce is already - // used on-chain (e.g. filled via another gateway). They would only - // burn fees at TTL expiry. - t.pruneStaleTxs(q, from, indexNonce) - - // At most one batch is collected per EOA per tick. The consecutive - // prefix below takes precedence and `continue`s; only when there is no - // eligible prefix (a gap at the head) do we consider the TTL-expiry - // path. The post-gap / over-cap remainder is left in the queue and - // drained on a later tick, gated by submission spacing — it is never - // merged with this batch, since a head gap would make the whole Flow - // transaction fail. - prefix := selectConsecutivePrefix(q.txs, q.nonces.expectedNonce(), t.config.TxMaxBatchSize) - if len(prefix) > 0 { - deleteByNonce(q.txs, prefix) - // Optimistically mark the batch submitting; reconcileSubmission - // advances submitted on success or clears it on failure. - q.nonces.markSubmitting(prefix[len(prefix)-1].nonce) - q.lastSubmittedAt = now - q.lastActivity = now - if !q.isEmpty() { - // Re-arm for the remaining (post-gap or over-cap) txs. - q.collectionWindowEndsAt = now.Add(t.config.TxCollectionWindow) - q.flushDeadline = now.Add(t.config.TxSubmissionSpacing) - } - work = append(work, flushWork{ - from: from, - txs: prefix, - inFlight: true, - reason: flushReasonPrefix, - localIndexedNonce: indexNonce, - }) - continue - } + indexNonce, err := t.nonceProvider.GetNonce(from) + if err != nil { + // Exception: a local state-index nonce read should not fail under normal + // operation. This is a background loop with no caller to reject the tx + // to, so skip this EOA for the current tick (its batch is deferred until + // the read succeeds) without aborting the whole flush for other EOAs. + t.logger.Error().Err(err).Str("eoa", from.Hex()). + Msg("unexpected failure reading nonce from local index, skipping EOA this tick") + return flushWork{}, false + } - // No eligible prefix (gap at the head). Submit transactions held - // past their TTL anyway instead of dropping them. - // - // Rationale (no silent drops): submitting an unexecutable transaction - // produces a real, observable on-chain failure (operators can see the - // failed Flow transaction and its nonce-mismatch), whereas silently - // dropping it leaves no trace. Avoiding silent drops is the whole reason - // this pool exists, so an observable failure is strictly preferable to an - // invisible drop. - // - // Tradeoff: a tx whose nonce is far ahead of the index is still - // submitted at TTL and burns fees on a guaranteed failure. How far - // ahead a nonce may be is bounded instead at Add time by TxMaxNonceGap - // (see classify); when that is unset (0), far-ahead nonces reach here. - // - // Cap the batch at TxMaxBatchSize so a long-lived gap cannot produce an - // unbounded Flow transaction; the remainder drains on later ticks, - // gated by submission spacing. - expired := selectExpired(q.txs, now, t.config.TxPoolTTL) - if len(expired) > t.config.TxMaxBatchSize { - expired = expired[:t.config.TxMaxBatchSize] - } - if len(expired) > 0 { - deleteByNonce(q.txs, expired) - // Deliberately do NOT mark the nonceTracker submitting here: these - // nonces are out of order (past a gap), and marking them in flight - // would corrupt the expected-nonce computation for future flushes. - q.lastSubmittedAt = now - q.lastActivity = now - t.logger.Warn().Strs("tx-hashes", txHashHexes(expired)).Str("eoa", from.Hex()). - Uint64("local-indexed-nonce", indexNonce). - Uint64("expected-nonce", q.nonces.expectedNonce()). - Msg("nonce gap never filled within TTL, submitting held transactions anyway") - work = append(work, flushWork{ - from: from, - txs: expired, - reason: flushReasonTTL, - localIndexedNonce: indexNonce, - }) - } + q.nonces.refreshIndexed(indexNonce) + + // Prune transactions that can never execute: their nonce is already used + // on-chain (e.g. filled via another gateway). They would only burn fees at + // TTL expiry. + t.pruneStaleTxs(q, from, indexNonce) + + if w, ok := t.collectPrefix(from, q, now, indexNonce); ok { + return w, true + } + return t.collectExpired(from, q, now, indexNonce) +} + +// collectPrefix detaches the longest consecutive nonce run starting at the +// expected nonce (capped at TxMaxBatchSize), marks it in flight, and re-arms the +// queue for any remainder. Returns false when a gap at the head leaves no +// eligible prefix. Callers must hold queueMux. +func (t *TxMemPool) collectPrefix( + from gethCommon.Address, + q *eoaQueue, + now time.Time, + indexNonce uint64, +) (flushWork, bool) { + prefix := selectConsecutivePrefix(q.txs, q.nonces.expectedNonce(), t.config.TxMaxBatchSize) + if len(prefix) == 0 { + return flushWork{}, false + } + + deleteByNonce(q.txs, prefix) + // Optimistically mark the batch submitting; reconcileSubmission advances + // submitted on success or clears it on failure. + q.nonces.markSubmitting(prefix[len(prefix)-1].nonce) + q.lastSubmittedAt = now + q.lastActivity = now + if !q.isEmpty() { + // Re-arm for the remaining (post-gap or over-cap) txs. + q.collectionWindowEndsAt = now.Add(t.config.TxCollectionWindow) + q.flushDeadline = now.Add(t.config.TxSubmissionSpacing) + } + return flushWork{ + from: from, + txs: prefix, + inFlight: true, + reason: flushReasonPrefix, + localIndexedNonce: indexNonce, + }, true +} + +// collectExpired detaches transactions held past their TTL when a head gap +// blocks the prefix path, submitting them anyway (capped at TxMaxBatchSize) +// rather than dropping them. Returns false when nothing has expired. Callers +// must hold queueMux. +// +// Rationale (no silent drops): submitting an unexecutable transaction produces a +// real, observable on-chain failure (operators can see the failed Flow +// transaction and its nonce-mismatch), whereas silently dropping it leaves no +// trace. Avoiding silent drops is the whole reason this pool exists, so an +// observable failure is strictly preferable to an invisible drop. The batch is +// deliberately NOT marked in flight: these nonces are out of order (past a gap), +// and marking them would corrupt the expected-nonce computation for future +// flushes. +// +// Tradeoff: a tx whose nonce is far ahead of the index is still submitted at TTL +// and burns fees on a guaranteed failure. How far ahead a nonce may be is +// bounded instead at Add time by TxMaxNonceGap (see classify); when that is +// unset (0), far-ahead nonces reach here. +func (t *TxMemPool) collectExpired( + from gethCommon.Address, + q *eoaQueue, + now time.Time, + indexNonce uint64, +) (flushWork, bool) { + expired := selectExpired(q.txs, now, t.config.TxPoolTTL) + if len(expired) > t.config.TxMaxBatchSize { + expired = expired[:t.config.TxMaxBatchSize] } + if len(expired) == 0 { + return flushWork{}, false + } + + deleteByNonce(q.txs, expired) + q.lastSubmittedAt = now + q.lastActivity = now + t.logger.Warn().Strs("tx-hashes", txHashHexes(expired)).Str("eoa", from.Hex()). + Uint64("local-indexed-nonce", indexNonce). + Uint64("expected-nonce", q.nonces.expectedNonce()). + Msg("nonce gap never filled within TTL, submitting held transactions anyway") + return flushWork{ + from: from, + txs: expired, + reason: flushReasonTTL, + localIndexedNonce: indexNonce, + }, true +} - // Report the pool's memory footprint while still holding queueMux: the - // number of per-EOA queues and the total number of held transactions. - // Counting must happen under the lock since t.queues is mutated - // concurrently (and idle queues are pruned above). +// reportSize emits the pool's memory footprint: the number of per-EOA queues and +// the total number of held transactions. Callers must hold queueMux, since it +// reads t.queues (mutated concurrently, and pruned during collection). +func (t *TxMemPool) reportSize() { queuedTxs := 0 for _, q := range t.queues { queuedTxs += q.size() } t.collector.TxPoolSize(len(t.queues), queuedTxs) - - return work } // pruneStaleTxs removes queued transactions whose nonce is below the current From 82306fe02afde9874f9abc2db37b8608ff5a849d Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:42:45 -0400 Subject: [PATCH 16/31] refactor(requester): unify the per-batch drop/submit log fields Audit finding M3: the three WARN-level per-batch logs (submission failure in logSubmission, stale prune, TTL submit-anyway) had drifted to different, overlapping field sets, so the no-silent-drops fields were inconsistent and the invariant did not really live in one place. Add a batchFields helper that attaches the common fields (eoa, tx-hashes, low/high nonce, batch-size, local-indexed-nonce) to a caller-supplied zerolog event, and route all three WARN sites through it. Each site then only adds its specifics (reason+err / expected-nonce / none). The nonce range is computed as the true min/max so it is correct even for the stale-prune batch, which is collected in unordered map iteration. The lighter success DEBUG line is intentionally left as-is. No behavior change beyond the prune/TTL logs gaining the shared fields. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 59 ++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 98409303..d861b53a 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -667,15 +667,46 @@ func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { return err } +// batchFields attaches to e the structured fields common to every per-batch +// lifecycle log — submission failure, stale prune, and TTL submit-anyway — so +// the no-silent-drops fields stay consistent and greppable across all three +// sites (eoa, tx hashes, nonce range, batch size, the indexed frontier). The +// caller creates e at the desired level, adds any site-specific fields (reason, +// error, expected-nonce, ...), and calls Msg. The nonce range is computed as the +// true min/max so it is correct even when txs is not sorted (e.g. a stale-prune +// batch collected in map order). +func batchFields( + e *zerolog.Event, + from gethCommon.Address, + txs []heldTx, + indexNonce uint64, +) *zerolog.Event { + var lowNonce, highNonce uint64 + for i, htx := range txs { + if i == 0 || htx.nonce < lowNonce { + lowNonce = htx.nonce + } + if i == 0 || htx.nonce > highNonce { + highNonce = htx.nonce + } + } + return e. + Str("eoa", from.Hex()). + Strs("tx-hashes", txHashHexes(txs)). + Uint64("low-nonce", lowNonce). + Uint64("high-nonce", highNonce). + Int("batch-size", len(txs)). + Uint64("local-indexed-nonce", indexNonce) +} + // logSubmission records the fate of a submitted batch so a transaction is never // silently lost. This is the observability half of the no-silent-drops // invariant: for any tx id you can either find it on-chain (sent) OR find a WARN // log here (dropped) — never nothing. // // - On a Flow submit FAILURE the batch's EVM transactions are dropped (we do -// not retry — clients resubmit), so we WARN with everything needed to debug -// a "lost transaction" report: eoa, tx hashes, nonce range, the indexed -// frontier, batch size, the flush reason, and the error. +// not retry — clients resubmit), so we WARN with the full batch context +// (batchFields) plus the flush reason and the error. // - On SUCCESS we emit a lighter DEBUG line (eoa + nonce range) so a sent // batch is traceable without the noise of a warning. // @@ -691,27 +722,19 @@ func (t *TxMemPool) logSubmission( if len(txs) == 0 { return } - lowNonce := txs[0].nonce - highNonce := txs[len(txs)-1].nonce if submitErr != nil { - t.logger.Warn(). - Err(submitErr). - Str("eoa", from.Hex()). - Strs("tx-hashes", txHashHexes(txs)). - Uint64("low-nonce", lowNonce). - Uint64("high-nonce", highNonce). - Uint64("local-indexed-nonce", localIndexedNonce). - Int("batch-size", len(txs)). + batchFields(t.logger.Warn(), from, txs, localIndexedNonce). Str("reason", reason). + Err(submitErr). Msg("Flow submission failed, EVM transactions dropped") return } t.logger.Debug(). Str("eoa", from.Hex()). - Uint64("low-nonce", lowNonce). - Uint64("high-nonce", highNonce). + Uint64("low-nonce", txs[0].nonce). + Uint64("high-nonce", txs[len(txs)-1].nonce). Int("batch-size", len(txs)). Str("reason", reason). Msg("submitted EVM transactions to Flow") @@ -911,8 +934,7 @@ func (t *TxMemPool) collectExpired( deleteByNonce(q.txs, expired) q.lastSubmittedAt = now q.lastActivity = now - t.logger.Warn().Strs("tx-hashes", txHashHexes(expired)).Str("eoa", from.Hex()). - Uint64("local-indexed-nonce", indexNonce). + batchFields(t.logger.Warn(), from, expired, indexNonce). Uint64("expected-nonce", q.nonces.expectedNonce()). Msg("nonce gap never filled within TTL, submitting held transactions anyway") return flushWork{ @@ -950,8 +972,7 @@ func (t *TxMemPool) pruneStaleTxs( } if len(stale) > 0 { deleteByNonce(q.txs, stale) - t.logger.Warn().Strs("tx-hashes", txHashHexes(stale)).Str("eoa", from.Hex()). - Uint64("local-indexed-nonce", indexNonce). + batchFields(t.logger.Warn(), from, stale, indexNonce). Msg("dropping stale transactions with nonce below indexed state") } } From 8cbdfcedf74cf875b045b2da3aa8e91d5a13f968 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:43:26 -0400 Subject: [PATCH 17/31] docs(requester): document the no-graceful-drain-on-shutdown caveat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit concurrency finding (Low): on ctx cancellation processQueues returns immediately, so transactions still held in a queue are discarded without a WARN — the one gap in the no-silent-drops invariant, which holds in steady state but not across shutdown/restart. Document this at the ctx.Done() return and note the exception in the top-of-file invariant, so the limitation is discoverable and a future drain has an obvious home. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index d861b53a..c1fb220b 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -101,7 +101,9 @@ import ( // Cross-cutting invariants // - No silent drops: for any accepted tx id you can either find it on-chain // (submitted) or find a WARN log saying it was dropped (submit failure or -// stale prune) — never nothing. See logSubmission. +// stale prune) — never nothing. See logSubmission. The one exception is +// shutdown: held-but-not-yet-submitted txs are discarded without a WARN when +// the pool's context is cancelled (no graceful drain — see processQueues). // - Failure handling: a failed submission drops the batch (clients resubmit) // and never wedges the EOA (the in-flight marker is rolled back). The pool // does NOT retry internally. @@ -644,6 +646,14 @@ func (t *TxMemPool) processQueues(ctx context.Context) { for { select { case <-ctx.Done(): + // Shutdown is NOT a graceful drain: any transactions still held in a + // queue (waiting on their collection window, gap fill, or TTL) are + // dropped without a WARN, which is the one gap in the no-silent-drops + // invariant — it holds during steady-state operation, not across a + // shutdown/restart. This is acceptable because clients resubmit, and + // a held tx has not been sent to Flow (nothing on-chain to reconcile + // against). If this ever needs to change, drain due+held batches here + // before returning rather than relying on resubmission. return case <-ticker.C: for _, w := range t.collectDueBatches() { From 46cf13723cc820df5bfbdd1d2367800d8c5c9584 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 00:59:46 -0400 Subject: [PATCH 18/31] refactor(requester): rename batchFields -> batchLogFields Clarify that the helper is a logging concern, not part of the core mempool logic. Rename only; no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index c1fb220b..48b83a84 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -677,7 +677,7 @@ func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { return err } -// batchFields attaches to e the structured fields common to every per-batch +// batchLogFields attaches to e the structured fields common to every per-batch // lifecycle log — submission failure, stale prune, and TTL submit-anyway — so // the no-silent-drops fields stay consistent and greppable across all three // sites (eoa, tx hashes, nonce range, batch size, the indexed frontier). The @@ -685,7 +685,7 @@ func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { // error, expected-nonce, ...), and calls Msg. The nonce range is computed as the // true min/max so it is correct even when txs is not sorted (e.g. a stale-prune // batch collected in map order). -func batchFields( +func batchLogFields( e *zerolog.Event, from gethCommon.Address, txs []heldTx, @@ -716,7 +716,7 @@ func batchFields( // // - On a Flow submit FAILURE the batch's EVM transactions are dropped (we do // not retry — clients resubmit), so we WARN with the full batch context -// (batchFields) plus the flush reason and the error. +// (batchLogFields) plus the flush reason and the error. // - On SUCCESS we emit a lighter DEBUG line (eoa + nonce range) so a sent // batch is traceable without the noise of a warning. // @@ -734,7 +734,7 @@ func (t *TxMemPool) logSubmission( } if submitErr != nil { - batchFields(t.logger.Warn(), from, txs, localIndexedNonce). + batchLogFields(t.logger.Warn(), from, txs, localIndexedNonce). Str("reason", reason). Err(submitErr). Msg("Flow submission failed, EVM transactions dropped") @@ -944,7 +944,7 @@ func (t *TxMemPool) collectExpired( deleteByNonce(q.txs, expired) q.lastSubmittedAt = now q.lastActivity = now - batchFields(t.logger.Warn(), from, expired, indexNonce). + batchLogFields(t.logger.Warn(), from, expired, indexNonce). Uint64("expected-nonce", q.nonces.expectedNonce()). Msg("nonce gap never filled within TTL, submitting held transactions anyway") return flushWork{ @@ -982,7 +982,7 @@ func (t *TxMemPool) pruneStaleTxs( } if len(stale) > 0 { deleteByNonce(q.txs, stale) - batchFields(t.logger.Warn(), from, stale, indexNonce). + batchLogFields(t.logger.Warn(), from, stale, indexNonce). Msg("dropping stale transactions with nonce below indexed state") } } From ec791ad5b4e280ffcb93bf7921e25f3472fac6b3 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 11:39:51 -0400 Subject: [PATCH 19/31] fix(requester): overflow-safe nonce-gap check; tighten timeout test Address CodeRabbit review on PR #974: - classify: compare the nonce distance (nonce - localIndexedNonce) against maxNonceGap instead of localIndexedNonce + maxNonceGap, which could overflow near math.MaxUint64. The preceding too-low check guarantees nonce >= localIndexedNonce, so the subtraction never underflows. Boundary semantics unchanged (verified by Test_NonceTracker_Classify). - Tighten the fast-path timeout test: capture after := time.Now() and assert the deadline is <= after.Add(fastPathSubmitTimeout), removing the +1s slack that could mask a regression. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 5 ++++- services/requester/tx_mempool_test.go | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 48b83a84..7ecafe4e 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -332,7 +332,10 @@ func (n *nonceTracker) classify( // More than maxNonceGap beyond the frontier (only when a gap is configured): // cannot execute until the gap fills. A behind (stale) index can only make // this over-strict, which is acceptable — the gateway is catching up. - if n.maxNonceGap > 0 && nonce > n.localIndexedNonce+n.maxNonceGap { + // Compare the distance rather than localIndexedNonce+maxNonceGap, which could + // overflow near math.MaxUint64; the too-low check above guarantees + // nonce >= localIndexedNonce, so the subtraction never underflows. + if n.maxNonceGap > 0 && nonce-n.localIndexedNonce > n.maxNonceGap { return nonceTooHigh, nil } if nonce == n.expectedNonce() { diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index fb063c3a..83df6ac2 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -249,10 +249,11 @@ func Test_TxMemPool_FastPathSubmitHasBoundedTimeout(t *testing.T) { 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") } From f001141502164392c64f97935f32033438e39eaa Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 12:09:19 -0400 Subject: [PATCH 20/31] docs(requester): document the submission-spacing collection-time skew CodeRabbit PR #974 flagged that lastSubmittedAt is stamped at collection time, not actual submit completion, so a later EOA in a tick can have slightly short effective spacing. Document the bounded skew on spacingElapsed and why the collection-time stamp is load-bearing (it gates a follow-up batch from being collected while one is in flight). Accepted for the current few-EOA deployment; the precise fix (re-stamp on submit completion in reconcileSubmission) is noted for if higher EOA fan-out makes the skew material. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 7ecafe4e..b5e70298 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -607,8 +607,22 @@ func (t *TxMemPool) Add( return nil } -// spacingElapsed reports whether enough time has passed since the last -// Cadence submission for this EOA. Callers must hold queueMux. +// spacingElapsed reports whether enough time has passed since the last Cadence +// submission for this EOA. Callers must hold queueMux. +// +// Timing caveat: for background batches, lastSubmittedAt is stamped at COLLECTION +// time (collectPrefix/collectExpired), not when the detached submit actually +// returns. Since one tick's batches are submitted sequentially afterward, a +// later EOA's stamp can predate its real submit by the cumulative submit latency +// ahead of it, so its NEXT batch's effective spacing can be short by that skew. +// The skew is bounded by (concurrently-due EOAs ahead) x (per-submit latency), +// which is small for the current few-EOA deployment and accepted. The +// collection-time stamp is also load-bearing the other way: it is what gates a +// follow-up batch from being collected while the previous one is still in flight +// (the spacing check runs before prefix selection, and the in-flight marker does +// not by itself stop the next consecutive prefix from being selected). A precise +// fix would keep this gate and additionally re-stamp on submit completion in +// reconcileSubmission. func (t *TxMemPool) spacingElapsed(q *eoaQueue, now time.Time) bool { return q.lastSubmittedAt.IsZero() || now.Sub(q.lastSubmittedAt) >= t.config.TxSubmissionSpacing From 8b3b742fb2d1dc2b0b5d40aad660ca76000b1bc0 Mon Sep 17 00:00:00 2001 From: Vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Fri, 26 Jun 2026 19:55:04 -0400 Subject: [PATCH 21/31] Apply suggestion from @zhangchiqing Co-authored-by: Leo Zhang --- services/requester/tx_mempool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index b5e70298..3964e963 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -256,7 +256,7 @@ type nonceTracker struct { // index (the on-chain frontier). A cache of a fact about the chain, refreshed // from a fresh read via refreshIndexed — not a record of our own sends. localIndexedNonce uint64 - // submitting is the highest nonce sent to Flow but not yet ack'd. It marks the + // submitting is the highest consecutive nonce sent to Flow but not yet ack'd. It marks the // window between detaching a batch and its submit result returning, and // strictly means "a network call is in flight": it is cleared the moment that // call returns, by markSubmitted on success or rollbackSubmitting on failure. From fb426a5873bc795d13e9dcbcfc54b520eb024b8c Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sat, 27 Jun 2026 00:07:06 -0400 Subject: [PATCH 22/31] docs(requester): clarify GetNonce returns the next-to-use account nonce The doc said "current nonce", which read ambiguously (last-used vs next-to- use) in PR #974 review. Reword to "account nonce ... the next nonce the EOA should use (matches eth_getTransactionCount)" so the semantics behind the too-low check (nonce < frontier) are unmistakable. Comment-only. Co-Authored-By: Claude Opus 4.8 --- services/requester/nonce_provider.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/requester/nonce_provider.go b/services/requester/nonce_provider.go index 1a8b4096..13a8c782 100644 --- a/services/requester/nonce_provider.go +++ b/services/requester/nonce_provider.go @@ -24,7 +24,8 @@ type NonceView interface { // NonceProvider returns the current nonce of the given EOA address. // The transaction mempool uses it to determine the expected next nonce. type NonceProvider interface { - // GetNonce returns the current nonce of the given EOA address. + // GetNonce returns the account nonce of the given EOA — its transaction count, + // i.e. the next nonce the EOA should use (matches eth_getTransactionCount). // // A non-nil error represents an EXCEPTION, not an expected condition: // the underlying read is a local state-index lookup that should not From 968afb10ec1b0508346861ddd368de8e086e56fa Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sat, 27 Jun 2026 00:53:26 -0400 Subject: [PATCH 23/31] refactor(requester): normalize maxNonceGap so classify drops the gap>0 guard Per @zhangchiqing (#974): maxNonceGap is set once, so re-checking "maxNonceGap > 0" on every classify call is noise. Normalize the config value at the mempool boundary (queue creation) via normalizeNonceGap: the config "0 = no upper bound" becomes math.MaxUint64, and classify uses a single distance comparison "nonce - localIndexedNonce > maxNonceGap" (still overflow-safe; distance is at most MaxUint64 so the unbounded case never trips). Normalization lives at the consumer, not in parseConfigFromFlags, so the config field keeps its honest "0 = unbounded" contract regardless of how a Config is built. The nonceTracker now stores the normalized value (MaxUint64 = unbounded), documented on the field. classify table test normalizes the same way; adds Test_NormalizeNonceGap. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 38 +++++++++++++++++++-------- services/requester/tx_mempool_test.go | 11 ++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 3964e963..d67b11f1 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -4,6 +4,7 @@ import ( "context" "encoding/hex" "fmt" + "math" "sort" "sync" "time" @@ -243,6 +244,17 @@ const ( nonceQueue ) +// normalizeNonceGap maps the configured max nonce gap to the value stored in a +// nonceTracker. The config value 0 ("no upper bound") becomes math.MaxUint64, so +// classify can use a single distance comparison without a per-call "gap > 0" +// guard. Any non-zero value is used as-is. +func normalizeNonceGap(configGap uint64) uint64 { + if configGap == 0 { + return math.MaxUint64 + } + return configGap +} + // nonceTracker is the per-EOA submission-state machine. It records the nonce // facts the mempool reasons about and answers "what should happen to an // incoming nonce?" (classify) and "what is the next nonce to submit?" @@ -268,9 +280,11 @@ type nonceTracker struct { // Unset before the EOA's first success. lastConsecutivelySubmitted nonceWrapper // maxNonceGap is how far above localIndexedNonce a nonce may be before it is - // rejected as too-high; 0 means no upper bound. It does NOT affect the too-low - // check (a nonce below localIndexedNonce is always rejected). Set once from - // config when the queue is created. + // rejected as too-high. math.MaxUint64 means no upper bound: the config value + // TxMaxNonceGap, where 0 = "unbounded", is normalized to it at construction + // (see normalizeNonceGap) so classify needs no per-call "is a gap configured?" + // guard. It does NOT affect the too-low check (a nonce below localIndexedNonce + // is always rejected). Set once when the queue is created. maxNonceGap uint64 } @@ -329,13 +343,15 @@ func (n *nonceTracker) classify( if nonce < n.localIndexedNonce { return nonceTooLow, nil } - // More than maxNonceGap beyond the frontier (only when a gap is configured): - // cannot execute until the gap fills. A behind (stale) index can only make - // this over-strict, which is acceptable — the gateway is catching up. - // Compare the distance rather than localIndexedNonce+maxNonceGap, which could - // overflow near math.MaxUint64; the too-low check above guarantees - // nonce >= localIndexedNonce, so the subtraction never underflows. - if n.maxNonceGap > 0 && nonce-n.localIndexedNonce > n.maxNonceGap { + // More than maxNonceGap beyond the frontier: cannot execute until the gap + // fills. A behind (stale) index can only make this over-strict, which is + // acceptable — the gateway is catching up. maxNonceGap is normalized + // (math.MaxUint64 = unbounded), so no "is a gap configured?" guard is needed. + // We compare the distance rather than localIndexedNonce+maxNonceGap (which + // could overflow): the too-low check above guarantees nonce >= localIndexedNonce + // so the subtraction never underflows, and the distance is at most MaxUint64 + // so the unbounded case never trips. + if nonce-n.localIndexedNonce > n.maxNonceGap { return nonceTooHigh, nil } if nonce == n.expectedNonce() { @@ -524,7 +540,7 @@ func (t *TxMemPool) Add( // Only maxNonceGap needs seeding from config. q = &eoaQueue{ txs: make(map[uint64]heldTx), - nonces: nonceTracker{maxNonceGap: t.config.TxMaxNonceGap}, + nonces: nonceTracker{maxNonceGap: normalizeNonceGap(t.config.TxMaxNonceGap)}, } t.queues[from] = q } diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 83df6ac2..106d082f 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -5,6 +5,7 @@ import ( "context" "crypto/ecdsa" "errors" + "math" "math/big" "sync" "testing" @@ -788,6 +789,9 @@ func Test_NonceTracker_Classify(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { tracker := tc.tracker + // classify expects a normalized gap (0 in the table means "unbounded", + // stored as math.MaxUint64), matching how queues are constructed. + tracker.maxNonceGap = normalizeNonceGap(tracker.maxNonceGap) got, err := tracker.classify( tc.nonce, &fakeNonceProvider{nonce: tc.frontier}, @@ -799,6 +803,13 @@ func Test_NonceTracker_Classify(t *testing.T) { } } +// normalizeNonceGap maps the config "0 = no upper bound" to MaxUint64 (so +// classify needs no per-call gap>0 guard) and passes other values through. +func Test_NormalizeNonceGap(t *testing.T) { + assert.Equal(t, uint64(math.MaxUint64), normalizeNonceGap(0), "0 = unbounded maps to MaxUint64") + assert.Equal(t, uint64(500), normalizeNonceGap(500)) +} + // With a configured max gap, Add rejects out-of-range nonces up front. func Test_TxMemPool_RejectsNonceOutOfRange(t *testing.T) { key, err := crypto.GenerateKey() From ac59fc420782733f296e8ef902cd0711a7741d53 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sat, 27 Jun 2026 01:00:28 -0400 Subject: [PATCH 24/31] refactor(requester): make Add nonce-verdict handling exhaustive Per @zhangchiqing (#974): be explicit that only nonceNextExpected and nonceQueue may reach the enqueue path. Fold the keep verdicts into the existing reject switch as an explicit case, and panic in default so a newly added, unhandled verdict is a loud programming error rather than silently falling through to enqueue. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index d67b11f1..1e2eac11 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -564,8 +564,11 @@ func (t *TxMemPool) Add( return err } - // Reject out-of-range / in-flight nonces up front, before allocating any - // held state: a transaction we are about to reject must not cost a heldTx. + // Handle every verdict exhaustively. The rejections return up front, before + // allocating any held state (a transaction we are about to reject must not + // cost a heldTx); the two keep verdicts fall through to the fast-path/enqueue + // logic below. A new verdict that isn't handled here is a programming error, + // so panic rather than silently routing it through enqueue. switch verdict { case nonceInFlight: return errs.ErrInFlightNonce @@ -573,6 +576,10 @@ func (t *TxMemPool) Add( return errs.ErrNonceTooLow case nonceTooHigh: return errs.ErrNonceTooHigh + case nonceNextExpected, nonceQueue: + // Keep: fall through to the fast-path / enqueue logic below. + default: + panic(fmt.Sprintf("unhandled nonce verdict: %d", verdict)) } // Past every cheap rejection: the transaction will be kept (submitted now or From 7123a4ec16b59d20161b1f276253cc3669de5289 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sat, 27 Jun 2026 01:20:57 -0400 Subject: [PATCH 25/31] refactor(requester): handle keep verdicts inside the Add switch Follow-up to the exhaustive-switch change: move the fast-path and enqueue logic into the nonceNextExpected / nonceQueue cases instead of re-checking the verdict after the switch. Each case returns, default still panics, so the switch is the function tail (no trailing return). Extract the shared enqueue logic into a small enqueue() helper; build the heldTx in place in each keep case (no helper for a plain struct literal). Reads top-to-bottom as one decision per verdict. No behavior change; unit + e2e green. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 99 +++++++++++++++++--------------- 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 1e2eac11..bb55d6f6 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -564,11 +564,10 @@ func (t *TxMemPool) Add( return err } - // Handle every verdict exhaustively. The rejections return up front, before - // allocating any held state (a transaction we are about to reject must not - // cost a heldTx); the two keep verdicts fall through to the fast-path/enqueue - // logic below. A new verdict that isn't handled here is a programming error, - // so panic rather than silently routing it through enqueue. + // Handle every verdict. The rejections return up front, before allocating any + // held state (a transaction we are about to reject must not cost a heldTx). + // The two keep verdicts build the heldTx and either fast-path or enqueue it. + // An unhandled verdict is a programming error, so panic. switch verdict { case nonceInFlight: return errs.ErrInFlightNonce @@ -576,58 +575,64 @@ func (t *TxMemPool) Add( return errs.ErrNonceTooLow case nonceTooHigh: return errs.ErrNonceTooHigh - case nonceNextExpected, nonceQueue: - // Keep: fall through to the fast-path / enqueue logic below. - default: - panic(fmt.Sprintf("unhandled nonce verdict: %d", verdict)) - } - - // Past every cheap rejection: the transaction will be kept (submitted now or - // held), so build the heldTx now rather than before the checks above. - held := heldTx{ - txPayload: hexEncodedTx, - txHash: tx.Hash(), - nonce: tx.Nonce(), - enqueuedAt: now, - } - - // Fast path: a next-expected nonce with an empty queue, nothing in flight and - // spacing satisfied is submitted immediately — zero added latency. The lock is - // held across the whole submit, so there is no concurrency window: no need to - // mark "submitting" first; on success we record the ack, on failure we leave - // the EOA untouched. Otherwise (queue non-empty, in flight, or spacing not - // elapsed) fall through to enqueue for the background loop. - if verdict == nonceNextExpected && - q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { - batch := []heldTx{held} - // Bound the submit so a hung call cannot pin queueMux indefinitely - // (see fastPathSubmitTimeout). - submitCtx, cancel := context.WithTimeout(ctx, fastPathSubmitTimeout) - submitErr := t.submitBatch(submitCtx, batch) - cancel() - t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) - if submitErr != nil { - return submitErr + case nonceQueue: + // A gap ahead within the accepted window — hold it for the background loop. + held := heldTx{ + txPayload: hexEncodedTx, + txHash: tx.Hash(), + nonce: tx.Nonce(), + enqueuedAt: now, } - q.nonces.markSubmitted(tx.Nonce()) - q.lastSubmittedAt = t.now() + t.enqueue(q, held, now) return nil + case nonceNextExpected: + held := heldTx{ + txPayload: hexEncodedTx, + txHash: tx.Hash(), + nonce: tx.Nonce(), + enqueuedAt: now, + } + // Fast path: an empty queue with nothing in flight and spacing satisfied + // submits immediately — zero added latency. The lock is held across the + // whole submit, so there is no concurrency window: no need to mark + // "submitting" first; on success we record the ack, on failure we leave + // the EOA untouched. Otherwise fall back to enqueue for the background loop. + if q.isEmpty() && !q.nonces.inFlight() && t.spacingElapsed(q, now) { + batch := []heldTx{held} + // Bound the submit so a hung call cannot pin queueMux indefinitely + // (see fastPathSubmitTimeout). + submitCtx, cancel := context.WithTimeout(ctx, fastPathSubmitTimeout) + submitErr := t.submitBatch(submitCtx, batch) + cancel() + t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) + if submitErr != nil { + return submitErr + } + q.nonces.markSubmitted(tx.Nonce()) + q.lastSubmittedAt = t.now() + return nil + } + t.enqueue(q, held, now) + return nil + default: + panic(fmt.Sprintf("unhandled nonce verdict: %d", verdict)) } +} - // Enqueue (a gap ahead, or a next-expected nonce that could not fast-path - // yet). A same-nonce, different-payload resubmission replaces the queued - // transaction (last write wins), matching mempool semantics. +// enqueue holds a transaction for the background flush loop, (re)arming the +// collection window. A same-nonce, different-payload resubmission replaces the +// queued transaction (last write wins), matching mempool semantics. Callers must +// hold queueMux. +func (t *TxMemPool) enqueue(q *eoaQueue, held heldTx, now time.Time) { wasEmpty := q.isEmpty() - q.txs[tx.Nonce()] = held + q.txs[held.nonce] = held q.collectionWindowEndsAt = now.Add(t.config.TxCollectionWindow) // Anchor the flush deadline at the FIRST enqueue only. Re-arming it on a - // same-nonce replacement would let a client defer the flush indefinitely - // by resubmitting one held transaction before each deadline. + // same-nonce replacement would let a client defer the flush indefinitely by + // resubmitting one held transaction before each deadline. if wasEmpty { q.flushDeadline = now.Add(t.config.TxSubmissionSpacing) } - - return nil } // spacingElapsed reports whether enough time has passed since the last Cadence From 59383db5ba51a231efb4057da0b609a9187ca9e9 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sat, 27 Jun 2026 01:52:32 -0400 Subject: [PATCH 26/31] fix(requester): guard markSubmitted against a stale success ack Per @zhangchiqing (#974): reconcileSubmission re-acquires queueMux after the detached submit, and the failure path (rollbackSubmitting) already guards against a stale ack clobbering newer state, but the success path (markSubmitted) overwrote unconditionally. Safe today (submitWork is sequential in the single processQueues goroutine, and concurrent Add never writes submitting while a batch is in flight), but it becomes a real double-submit hazard the moment submissions are parallelized. Make markSubmitted symmetric with rollbackSubmitting: advance lastConsecutivelySubmitted via max (never regress) and clear submitting only if it still refers to this batch. Backward-compatible in sequential operation (highNonce is always the current submitting). Adds a nonceTracker test for the stale/out-of-order ack case. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 11 +++++++++-- services/requester/tx_mempool_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index bb55d6f6..59e61ceb 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -375,9 +375,16 @@ func (n *nonceTracker) markSubmitting(highNonce uint64) { // clears as soon as the call returns. The cost is one quick lock per successful // submission, accepted in exchange for a state machine that is easy to reason // about. +// +// Both updates are guarded so a stale ack cannot corrupt newer state (symmetric +// with rollbackSubmitting): lastConsecutivelySubmitted only advances (never +// regresses), and submitting is cleared only if it still refers to this batch — +// a newer submission may have replaced it once submissions run concurrently. func (n *nonceTracker) markSubmitted(highNonce uint64) { - n.lastConsecutivelySubmitted = toNonceWrapper(highNonce) - n.submitting = nonceWrapper{} + n.lastConsecutivelySubmitted = n.lastConsecutivelySubmitted.max(toNonceWrapper(highNonce)) + if n.submitting.is(highNonce) { + n.submitting = nonceWrapper{} + } } // rollbackSubmitting clears the in-flight marker after a failed submission, but diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 106d082f..19d91ba2 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -876,6 +876,31 @@ func Test_NonceTracker_Transitions(t *testing.T) { assert.Equal(t, uint64(12), n.localIndexedNonce) } +// A stale success ack (e.g. once submissions run concurrently and a newer batch +// has already been marked in flight) must not clear the newer in-flight marker +// or regress the consecutively-submitted nonce. +func Test_NonceTracker_StaleMarkSubmittedDoesNotClobber(t *testing.T) { + n := &nonceTracker{} + + n.markSubmitting(6) // batch A {.,6} in flight + n.markSubmitting(8) // batch B {7,8} replaces it as the newer in-flight batch + + // Batch A's (stale) success ack arrives: it must not clear B's marker. + n.markSubmitted(6) + assert.True(t, n.inFlight(), "newer in-flight marker must survive a stale ack") + assert.Equal(t, toNonceWrapper(8), n.submitting) + assert.Equal(t, toNonceWrapper(6), n.lastConsecutivelySubmitted) + + // Batch B's ack then clears the marker and advances submitted to 8. + n.markSubmitted(8) + assert.False(t, n.inFlight()) + assert.Equal(t, toNonceWrapper(8), n.lastConsecutivelySubmitted) + + // An out-of-order ack for an older nonce must not regress submitted. + n.markSubmitted(7) + assert.Equal(t, toNonceWrapper(8), n.lastConsecutivelySubmitted, "submitted must never regress") +} + // --- Clock-driven timing tests ------------------------------------------- // These drive the collection window, flush deadline, submission spacing, TTL // expiry and idle-queue retention through the real Add -> collectDueBatches From 17417b02d6197bf0bec9c018ad2940a471aa076b Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sat, 27 Jun 2026 11:44:45 -0400 Subject: [PATCH 27/31] refactor(requester): extract clearSubmittingIf shared by mark/rollback The guarded in-flight clear (clear submitting only if it still refers to highNonce) was duplicated in markSubmitted and rollbackSubmitting. Extract it into clearSubmittingIf: both acks fire when the network call returns and retract the marker, while only markSubmitted also advances the submitted nonce. No behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 34 ++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 59e61ceb..1d556f9a 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -368,6 +368,16 @@ func (n *nonceTracker) markSubmitting(highNonce uint64) { n.submitting = toNonceWrapper(highNonce) } +// clearSubmittingIf retracts the in-flight marker, but only if it still refers +// to highNonce (a newer submission may have replaced it once submissions run +// concurrently). Shared by the success (markSubmitted) and failure +// (rollbackSubmitting) acks — both fire when the network call returns. +func (n *nonceTracker) clearSubmittingIf(highNonce uint64) { + if n.submitting.is(highNonce) { + n.submitting = nonceWrapper{} + } +} + // markSubmitted acks a successful submission: it advances the consecutively- // submitted nonce and clears the in-flight marker. It runs the moment the // submission succeeds, under the lock, rather than waiting for the index to @@ -376,26 +386,20 @@ func (n *nonceTracker) markSubmitting(highNonce uint64) { // submission, accepted in exchange for a state machine that is easy to reason // about. // -// Both updates are guarded so a stale ack cannot corrupt newer state (symmetric -// with rollbackSubmitting): lastConsecutivelySubmitted only advances (never -// regresses), and submitting is cleared only if it still refers to this batch — -// a newer submission may have replaced it once submissions run concurrently. +// Both updates are guarded so a stale ack cannot corrupt newer state: +// lastConsecutivelySubmitted only advances (never regresses), and submitting is +// cleared only if it still refers to this batch. func (n *nonceTracker) markSubmitted(highNonce uint64) { n.lastConsecutivelySubmitted = n.lastConsecutivelySubmitted.max(toNonceWrapper(highNonce)) - if n.submitting.is(highNonce) { - n.submitting = nonceWrapper{} - } + n.clearSubmittingIf(highNonce) } -// rollbackSubmitting clears the in-flight marker after a failed submission, but -// only when it still refers to highNonce (a newer submission may have replaced -// it). Because a failure never advances lastConsecutivelySubmitted, there is -// nothing else to undo: the next flush recomputes expectedNonce from the -// unchanged frontier. +// rollbackSubmitting clears the in-flight marker after a failed submission. +// Because a failure never advances lastConsecutivelySubmitted, there is nothing +// else to undo: the next flush recomputes expectedNonce from the unchanged +// frontier. func (n *nonceTracker) rollbackSubmitting(highNonce uint64) { - if n.submitting.is(highNonce) { - n.submitting = nonceWrapper{} - } + n.clearSubmittingIf(highNonce) } // refreshIndexed updates the cached on-chain frontier from a fresh index read. From 32a1f15dcf3505abf7d76a879f13daf1836e4ab8 Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Sun, 28 Jun 2026 12:26:39 -0400 Subject: [PATCH 28/31] refactor(requester): rename flushWork.inFlight -> needsReconcile Per @zhangchiqing (#974): the flag tells reconcileSubmission whether to update the tracker, but "inFlight" described the cause (the batch was marked submitting) and collided with nonceTracker.inFlight() (which means "a submission is outstanding"). Rename to needsReconcile, which states the flags purpose, and lead its doc with that. Pure rename; no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/tx_mempool.go | 21 +++++++++++---------- services/requester/tx_mempool_test.go | 16 ++++++++-------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 1d556f9a..78335762 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -672,12 +672,13 @@ func (t *TxMemPool) spacingElapsed(q *eoaQueue, now time.Time) bool { type flushWork struct { from gethCommon.Address txs []heldTx - // inFlight is true for consecutive-prefix batches, which mark the queue's - // nonceTracker "submitting" and must therefore reconcile it once the submit - // returns (markSubmitted on success, rollbackSubmitting on failure — see - // reconcileSubmission). TTL-expiry batches are not marked in flight and must - // never touch the tracker. - inFlight bool + // needsReconcile is true when reconcileSubmission must update the tracker for + // this batch. Consecutive-prefix batches set it: they mark the nonceTracker + // "submitting" and must reconcile it once the submit returns (markSubmitted on + // success, rollbackSubmitting on failure — see reconcileSubmission). TTL-expiry + // batches leave it false: they never mark the tracker, so there is nothing to + // reconcile. + needsReconcile bool // reason is why this batch was flushed, recorded purely for the submission // log (see logSubmission): flushReasonPrefix for a consecutive-prefix flush, // flushReasonTTL for a TTL-expiry submit-anyway. @@ -825,10 +826,10 @@ func (t *TxMemPool) logSubmission( // have replaced it). lastSubmittedAt is deliberately NOT restored: a brief, // self-correcting spacing delay after a rare failure is harmless. // -// TTL-expiry batches (w.inFlight == false) never mark the tracker, so there is -// nothing to reconcile for them. +// TTL-expiry batches (w.needsReconcile == false) never mark the tracker, so +// there is nothing to reconcile for them. func (t *TxMemPool) reconcileSubmission(w flushWork, submitErr error) { - if !w.inFlight { + if !w.needsReconcile { return } @@ -959,7 +960,7 @@ func (t *TxMemPool) collectPrefix( return flushWork{ from: from, txs: prefix, - inFlight: true, + needsReconcile: true, reason: flushReasonPrefix, localIndexedNonce: indexNonce, }, true diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 19d91ba2..48c6d466 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -392,7 +392,7 @@ func Test_TxMemPool_FailedFlushDoesNotWedgeEOA(t *testing.T) { work := pool.collectDueBatches() require.Len(t, work, 1) - assert.True(t, work[0].inFlight) + assert.True(t, work[0].needsReconcile) require.Len(t, work[0].txs, 2) // State was committed optimistically under the lock. @@ -425,28 +425,28 @@ func Test_ReconcileSubmission_OnlyReconcilesMatchingInFlightBatch(t *testing.T) // A different (newer) in-flight nonce owns the marker: not cleared. pool.reconcileSubmission( - flushWork{from: from, txs: []heldTx{makeHeldTx(5, time.Time{})}, inFlight: true}, + flushWork{from: from, txs: []heldTx{makeHeldTx(5, time.Time{})}, needsReconcile: true}, submitErr, ) assert.True(t, pool.queues[from].nonces.inFlight()) - // A TTL-expiry batch (inFlight false) never touches the tracker. + // A TTL-expiry batch (needsReconcile false) never touches the tracker. pool.reconcileSubmission( - flushWork{from: from, txs: []heldTx{makeHeldTx(7, time.Time{})}, inFlight: false}, + flushWork{from: from, txs: []heldTx{makeHeldTx(7, time.Time{})}, needsReconcile: false}, submitErr, ) assert.True(t, pool.queues[from].nonces.inFlight()) // The failed in-flight batch still owns the marker: cleared. pool.reconcileSubmission( - flushWork{from: from, txs: []heldTx{makeHeldTx(7, time.Time{})}, inFlight: true}, + flushWork{from: from, txs: []heldTx{makeHeldTx(7, time.Time{})}, needsReconcile: true}, submitErr, ) assert.False(t, pool.queues[from].nonces.inFlight()) // Unknown EOA: no panic. pool.reconcileSubmission( - flushWork{from: gethCommon.HexToAddress("0xdef"), txs: []heldTx{makeHeldTx(7, time.Time{})}, inFlight: true}, + flushWork{from: gethCommon.HexToAddress("0xdef"), txs: []heldTx{makeHeldTx(7, time.Time{})}, needsReconcile: true}, submitErr, ) } @@ -640,7 +640,7 @@ func Test_TxMemPool_TTLFlushCappedAtMaxBatch(t *testing.T) { work := pool.collectDueBatches() require.Len(t, work, 1) assert.Len(t, work[0].txs, 3, "TTL flush must be capped at TxMaxBatchSize") - assert.False(t, work[0].inFlight) + assert.False(t, work[0].needsReconcile) // Lowest nonces drained first; remainder stays queued. assert.Equal(t, uint64(10), work[0].txs[0].nonce) assert.Len(t, pool.queues[from].txs, 4) @@ -1051,7 +1051,7 @@ func Test_TxMemPool_TTLExpiryViaClock(t *testing.T) { work := pool.collectDueBatches() require.Len(t, work, 1) submitted = work - assert.False(t, submitted[0].inFlight) + assert.False(t, submitted[0].needsReconcile) assert.Equal(t, flushReasonTTL, submitted[0].reason) assert.Equal(t, []uint64{10}, noncesOf(submitted[0].txs)) assert.Empty(t, pool.queues[from].txs) From ab7fa95d1cc0d813aa4c953159efc9fe5c8e81d4 Mon Sep 17 00:00:00 2001 From: Vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Mon, 29 Jun 2026 17:44:53 -0400 Subject: [PATCH 29/31] Update services/requester/tx_mempool.go Co-authored-by: Leo Zhang --- services/requester/tx_mempool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 78335762..8f0893d4 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -683,7 +683,7 @@ type flushWork struct { // log (see logSubmission): flushReasonPrefix for a consecutive-prefix flush, // flushReasonTTL for a TTL-expiry submit-anyway. reason string - // localIndexedNonce is the on-chain frontier observed when the batch was + // localNextNonce is the on-chain next expected nonce observed when the batch was // collected. It is logged on drop so a "lost transaction" report can be // debugged against where the chain actually was. localIndexedNonce uint64 From 294c6315895b2789072526b3dadfbab4df7f1708 Mon Sep 17 00:00:00 2001 From: Vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Mon, 29 Jun 2026 18:01:59 -0400 Subject: [PATCH 30/31] Update services/requester/tx_mempool.go Co-authored-by: Leo Zhang --- services/requester/tx_mempool.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 8f0893d4..9753be4b 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -273,6 +273,8 @@ type nonceTracker struct { // strictly means "a network call is in flight": it is cleared the moment that // call returns, by markSubmitted on success or rollbackSubmitting on failure. // Unset when no submission is outstanding. + // it is also used to filter incoming txs to accept only whose nonce is bigger than this, + // otherwise would be rejected submitting nonceWrapper // lastConsecutivelySubmitted is the highest nonce consecutively and // successfully submitted (ack'd). Only consecutive-prefix batches advance it; From 60b0bb652fc639c20ad1cf41adb359f6ad3b476d Mon Sep 17 00:00:00 2001 From: vishal <1117327+vishalchangrani@users.noreply.github.com> Date: Tue, 30 Jun 2026 00:49:02 -0400 Subject: [PATCH 31/31] refactor(requester): rename indexed-nonce naming to next-nonce (Leo #974) Per @zhangchiqing: "Indexed" implies a nonce already used/seen on chain, but these hold the EOA next expected nonce (what GetNonce returns), so the naming was misleading. Rename for consistency: - NonceProvider.GetNonce -> GetNextNonce (interface, LocalNonceProvider impl, both mempool call sites, test fake). - nonceTracker.localIndexedNonce -> localNextNonce; refreshIndexed -> refreshNextNonce; flushWork.localIndexedNonce -> localNextNonce; local indexNonce vars -> nextNonce; the "reading indexed nonce" error and the "local-indexed-nonce" log key -> next-nonce; stray "indexed frontier" comments -> "on-chain frontier". NonceView.GetNonce is intentionally NOT renamed: the interface is satisfied directly by flow-go query.View (whose method is GetNonce), so renaming it would break compilation; documented inline. The test fake therefore carries both GetNextNonce (NonceProvider) and GetNonce (NonceView). Kept "indexed height" (block indexing, not the nonce). Pure rename; no behavior change. Co-Authored-By: Claude Opus 4.8 --- services/requester/nonce_provider.go | 18 +++-- services/requester/tx_mempool.go | 102 +++++++++++++------------- services/requester/tx_mempool_test.go | 32 ++++---- 3 files changed, 80 insertions(+), 72 deletions(-) diff --git a/services/requester/nonce_provider.go b/services/requester/nonce_provider.go index 13a8c782..2dafad99 100644 --- a/services/requester/nonce_provider.go +++ b/services/requester/nonce_provider.go @@ -17,25 +17,27 @@ import ( // rather than rebuilding the (expensive) view per address. It is an interface // so tests can fake it without constructing a real query.View. type NonceView interface { - // GetNonce returns the nonce of the given EOA at this view's state. + // GetNonce returns the EOA's account nonce (the next nonce to use) at this + // view's state. Named GetNonce — not GetNextNonce — because this interface is + // satisfied directly by flow-go's query.View, whose method is GetNonce. GetNonce(address gethCommon.Address) (uint64, error) } -// NonceProvider returns the current nonce of the given EOA address. -// The transaction mempool uses it to determine the expected next nonce. +// NonceProvider returns the next nonce of the given EOA address. The transaction +// mempool uses it to determine the expected next nonce. type NonceProvider interface { - // GetNonce returns the account nonce of the given EOA — its transaction count, - // i.e. the next nonce the EOA should use (matches eth_getTransactionCount). + // GetNextNonce returns the account nonce of the given EOA — its transaction + // count, i.e. the next nonce the EOA should use (matches eth_getTransactionCount). // // A non-nil error represents an EXCEPTION, not an expected condition: // the underlying read is a local state-index lookup that should not // fail under normal operation. Callers must therefore treat an error // as a hard failure (reject the transaction / abort the operation) // rather than a routine, recoverable condition to swallow. - GetNonce(address gethCommon.Address) (uint64, error) + GetNextNonce(address gethCommon.Address) (uint64, error) // GetBlockView returns a NonceView over the latest indexed EVM state. A - // non-nil error is an EXCEPTION, same contract as GetNonce. + // non-nil error is an EXCEPTION, same contract as GetNextNonce. GetBlockView() (NonceView, error) } @@ -107,7 +109,7 @@ func (p *LocalNonceProvider) GetBlockView() (NonceView, error) { return view, nil } -func (p *LocalNonceProvider) GetNonce(address gethCommon.Address) (uint64, error) { +func (p *LocalNonceProvider) GetNextNonce(address gethCommon.Address) (uint64, error) { view, err := p.GetBlockView() if err != nil { return 0, err diff --git a/services/requester/tx_mempool.go b/services/requester/tx_mempool.go index 9753be4b..3ca7b1db 100644 --- a/services/requester/tx_mempool.go +++ b/services/requester/tx_mempool.go @@ -67,7 +67,7 @@ import ( // until the gap fills or it ages out. // Example: expected nonce 5, tx with nonce 7 arrives (5 and 6 missing) → // 7 is held, not sent. -// 6. Stale pruning: a held tx whose nonce has fallen below the indexed +// 6. Stale pruning: a held tx whose nonce has fallen below the on-chain // frontier (e.g. filled via another gateway) can never execute and is // dropped with a WARN log before it would burn fees. // Example: nonce 3 sits in the queue; the frontier advances to 5 (3 and 4 @@ -91,7 +91,7 @@ import ( // Flow fees on a guaranteed nonce-mismatch. // Example: nonces 5,6 were just sent and are in flight; a new tx with // nonce 6 arrives → rejected. -// 11. Too-low: nonce below the indexed frontier (already used) → +// 11. Too-low: nonce below the on-chain frontier (already used) → // ErrNonceTooLow. Always enforced. // Example: frontier is 5, tx with nonce 4 arrives → rejected. // 12. Too-high: nonce more than TxMaxNonceGap beyond the frontier → @@ -264,16 +264,16 @@ func normalizeNonceGap(configGap uint64) uint64 { // held. Serialized access is guaranteed by the single submit goroutine plus // Add running under that same lock (see the TxMemPool docstring). type nonceTracker struct { - // localIndexedNonce is the EOA's next expected nonce per the local state + // localNextNonce is the EOA's next expected nonce per the local state // index (the on-chain frontier). A cache of a fact about the chain, refreshed - // from a fresh read via refreshIndexed — not a record of our own sends. - localIndexedNonce uint64 + // from a fresh read via refreshNextNonce — not a record of our own sends. + localNextNonce uint64 // submitting is the highest consecutive nonce sent to Flow but not yet ack'd. It marks the // window between detaching a batch and its submit result returning, and // strictly means "a network call is in flight": it is cleared the moment that // call returns, by markSubmitted on success or rollbackSubmitting on failure. // Unset when no submission is outstanding. - // it is also used to filter incoming txs to accept only whose nonce is bigger than this, + // it is also used to filter incoming txs to accept only whose nonce is bigger than this, // otherwise would be rejected submitting nonceWrapper // lastConsecutivelySubmitted is the highest nonce consecutively and @@ -281,11 +281,11 @@ type nonceTracker struct { // TTL-expiry (gapped) batches do NOT, so it never includes a nonce past a gap. // Unset before the EOA's first success. lastConsecutivelySubmitted nonceWrapper - // maxNonceGap is how far above localIndexedNonce a nonce may be before it is + // maxNonceGap is how far above localNextNonce a nonce may be before it is // rejected as too-high. math.MaxUint64 means no upper bound: the config value // TxMaxNonceGap, where 0 = "unbounded", is normalized to it at construction // (see normalizeNonceGap) so classify needs no per-call "is a gap configured?" - // guard. It does NOT affect the too-low check (a nonce below localIndexedNonce + // guard. It does NOT affect the too-low check (a nonce below localNextNonce // is always rejected). Set once when the queue is created. maxNonceGap uint64 } @@ -301,12 +301,12 @@ func (n *nonceTracker) highestSent() nonceWrapper { } // expectedNonce is the next nonce eligible for submission: one past the highest -// nonce already sent, or the indexed frontier, whichever is higher. +// nonce already sent, or the on-chain frontier, whichever is higher. func (n *nonceTracker) expectedNonce() uint64 { - if hi := n.highestSent(); hi.atLeast(n.localIndexedNonce) { + if hi := n.highestSent(); hi.atLeast(n.localNextNonce) { return hi.v + 1 } - return n.localIndexedNonce + return n.localNextNonce } // classify returns the verdict for an incoming nonce, refreshing the cached @@ -335,25 +335,25 @@ func (n *nonceTracker) classify( } // Beyond what we've sent: refresh the frontier for the remaining verdicts. - indexNonce, err := np.GetNonce(from) + nextNonce, err := np.GetNextNonce(from) if err != nil { - return 0, fmt.Errorf("reading indexed nonce for %s: %w", from.Hex(), err) + return 0, fmt.Errorf("reading next nonce for %s: %w", from.Hex(), err) } - n.refreshIndexed(indexNonce) + n.refreshNextNonce(nextNonce) // Below the on-chain frontier: already used, can never execute. - if nonce < n.localIndexedNonce { + if nonce < n.localNextNonce { return nonceTooLow, nil } // More than maxNonceGap beyond the frontier: cannot execute until the gap // fills. A behind (stale) index can only make this over-strict, which is // acceptable — the gateway is catching up. maxNonceGap is normalized // (math.MaxUint64 = unbounded), so no "is a gap configured?" guard is needed. - // We compare the distance rather than localIndexedNonce+maxNonceGap (which - // could overflow): the too-low check above guarantees nonce >= localIndexedNonce + // We compare the distance rather than localNextNonce+maxNonceGap (which + // could overflow): the too-low check above guarantees nonce >= localNextNonce // so the subtraction never underflows, and the distance is at most MaxUint64 // so the unbounded case never trips. - if nonce-n.localIndexedNonce > n.maxNonceGap { + if nonce-n.localNextNonce > n.maxNonceGap { return nonceTooHigh, nil } if nonce == n.expectedNonce() { @@ -404,9 +404,9 @@ func (n *nonceTracker) rollbackSubmitting(highNonce uint64) { n.clearSubmittingIf(highNonce) } -// refreshIndexed updates the cached on-chain frontier from a fresh index read. -func (n *nonceTracker) refreshIndexed(indexedNonce uint64) { - n.localIndexedNonce = indexedNonce +// refreshNextNonce updates the cached on-chain frontier from a fresh index read. +func (n *nonceTracker) refreshNextNonce(nextNonce uint64) { + n.localNextNonce = nextNonce } // eoaQueue tracks the held transactions and submission state for one EOA. @@ -617,7 +617,7 @@ func (t *TxMemPool) Add( submitCtx, cancel := context.WithTimeout(ctx, fastPathSubmitTimeout) submitErr := t.submitBatch(submitCtx, batch) cancel() - t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localIndexedNonce, submitErr) + t.logSubmission(from, batch, flushReasonFastPath, q.nonces.localNextNonce, submitErr) if submitErr != nil { return submitErr } @@ -688,7 +688,7 @@ type flushWork struct { // localNextNonce is the on-chain next expected nonce observed when the batch was // collected. It is logged on drop so a "lost transaction" report can be // debugged against where the chain actually was. - localIndexedNonce uint64 + localNextNonce uint64 } // flushReason values label why a batch was submitted, for the submission log. @@ -731,7 +731,7 @@ func (t *TxMemPool) processQueues(ctx context.Context) { // reconciles the queue's nonce state once the network call returns. func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { err := t.submitBatch(ctx, w.txs) - t.logSubmission(w.from, w.txs, w.reason, w.localIndexedNonce, err) + t.logSubmission(w.from, w.txs, w.reason, w.localNextNonce, err) t.reconcileSubmission(w, err) return err } @@ -739,7 +739,7 @@ func (t *TxMemPool) submitWork(ctx context.Context, w flushWork) error { // batchLogFields attaches to e the structured fields common to every per-batch // lifecycle log — submission failure, stale prune, and TTL submit-anyway — so // the no-silent-drops fields stay consistent and greppable across all three -// sites (eoa, tx hashes, nonce range, batch size, the indexed frontier). The +// sites (eoa, tx hashes, nonce range, batch size, the on-chain frontier). The // caller creates e at the desired level, adds any site-specific fields (reason, // error, expected-nonce, ...), and calls Msg. The nonce range is computed as the // true min/max so it is correct even when txs is not sorted (e.g. a stale-prune @@ -748,7 +748,7 @@ func batchLogFields( e *zerolog.Event, from gethCommon.Address, txs []heldTx, - indexNonce uint64, + nextNonce uint64, ) *zerolog.Event { var lowNonce, highNonce uint64 for i, htx := range txs { @@ -765,7 +765,7 @@ func batchLogFields( Uint64("low-nonce", lowNonce). Uint64("high-nonce", highNonce). Int("batch-size", len(txs)). - Uint64("local-indexed-nonce", indexNonce) + Uint64("local-next-nonce", nextNonce) } // logSubmission records the fate of a submitted batch so a transaction is never @@ -785,7 +785,7 @@ func (t *TxMemPool) logSubmission( from gethCommon.Address, txs []heldTx, reason string, - localIndexedNonce uint64, + localNextNonce uint64, submitErr error, ) { if len(txs) == 0 { @@ -793,7 +793,7 @@ func (t *TxMemPool) logSubmission( } if submitErr != nil { - batchLogFields(t.logger.Warn(), from, txs, localIndexedNonce). + batchLogFields(t.logger.Warn(), from, txs, localNextNonce). Str("reason", reason). Err(submitErr). Msg("Flow submission failed, EVM transactions dropped") @@ -855,7 +855,7 @@ func (t *TxMemPool) reconcileSubmission(w flushWork, submitErr error) { // submission, updates the queue state optimistically, and returns the detached // work items. The per-EOA decision lives in collectQueue. // -// Each due EOA's nonce is read via GetNonce; the provider caches the block view +// Each due EOA's nonce is read via GetNextNonce; the provider caches the block view // by indexed height, so all reads in this pass (and across ticks at the same // height) reuse one built view rather than rebuilding per address. func (t *TxMemPool) collectDueBatches() []flushWork { @@ -909,7 +909,7 @@ func (t *TxMemPool) collectQueue( return flushWork{}, false } - indexNonce, err := t.nonceProvider.GetNonce(from) + nextNonce, err := t.nonceProvider.GetNextNonce(from) if err != nil { // Exception: a local state-index nonce read should not fail under normal // operation. This is a background loop with no caller to reject the tx @@ -920,17 +920,17 @@ func (t *TxMemPool) collectQueue( return flushWork{}, false } - q.nonces.refreshIndexed(indexNonce) + q.nonces.refreshNextNonce(nextNonce) // Prune transactions that can never execute: their nonce is already used // on-chain (e.g. filled via another gateway). They would only burn fees at // TTL expiry. - t.pruneStaleTxs(q, from, indexNonce) + t.pruneStaleTxs(q, from, nextNonce) - if w, ok := t.collectPrefix(from, q, now, indexNonce); ok { + if w, ok := t.collectPrefix(from, q, now, nextNonce); ok { return w, true } - return t.collectExpired(from, q, now, indexNonce) + return t.collectExpired(from, q, now, nextNonce) } // collectPrefix detaches the longest consecutive nonce run starting at the @@ -941,7 +941,7 @@ func (t *TxMemPool) collectPrefix( from gethCommon.Address, q *eoaQueue, now time.Time, - indexNonce uint64, + nextNonce uint64, ) (flushWork, bool) { prefix := selectConsecutivePrefix(q.txs, q.nonces.expectedNonce(), t.config.TxMaxBatchSize) if len(prefix) == 0 { @@ -960,11 +960,11 @@ func (t *TxMemPool) collectPrefix( q.flushDeadline = now.Add(t.config.TxSubmissionSpacing) } return flushWork{ - from: from, - txs: prefix, - needsReconcile: true, - reason: flushReasonPrefix, - localIndexedNonce: indexNonce, + from: from, + txs: prefix, + needsReconcile: true, + reason: flushReasonPrefix, + localNextNonce: nextNonce, }, true } @@ -990,7 +990,7 @@ func (t *TxMemPool) collectExpired( from gethCommon.Address, q *eoaQueue, now time.Time, - indexNonce uint64, + nextNonce uint64, ) (flushWork, bool) { expired := selectExpired(q.txs, now, t.config.TxPoolTTL) if len(expired) > t.config.TxMaxBatchSize { @@ -1003,14 +1003,14 @@ func (t *TxMemPool) collectExpired( deleteByNonce(q.txs, expired) q.lastSubmittedAt = now q.lastActivity = now - batchLogFields(t.logger.Warn(), from, expired, indexNonce). + batchLogFields(t.logger.Warn(), from, expired, nextNonce). Uint64("expected-nonce", q.nonces.expectedNonce()). Msg("nonce gap never filled within TTL, submitting held transactions anyway") return flushWork{ - from: from, - txs: expired, - reason: flushReasonTTL, - localIndexedNonce: indexNonce, + from: from, + txs: expired, + reason: flushReasonTTL, + localNextNonce: nextNonce, }, true } @@ -1031,18 +1031,18 @@ func (t *TxMemPool) reportSize() { func (t *TxMemPool) pruneStaleTxs( q *eoaQueue, from gethCommon.Address, - indexNonce uint64, + nextNonce uint64, ) { var stale []heldTx for nonce, htx := range q.txs { - if nonce < indexNonce { + if nonce < nextNonce { stale = append(stale, htx) } } if len(stale) > 0 { deleteByNonce(q.txs, stale) - batchLogFields(t.logger.Warn(), from, stale, indexNonce). - Msg("dropping stale transactions with nonce below indexed state") + batchLogFields(t.logger.Warn(), from, stale, nextNonce). + Msg("dropping stale transactions with nonce below the on-chain frontier") } } diff --git a/services/requester/tx_mempool_test.go b/services/requester/tx_mempool_test.go index 48c6d466..6ee0f2e9 100644 --- a/services/requester/tx_mempool_test.go +++ b/services/requester/tx_mempool_test.go @@ -110,6 +110,12 @@ type fakeNonceProvider struct { err error } +// GetNextNonce satisfies NonceProvider; GetNonce satisfies the NonceView that +// GetBlockView returns. Both read every EOA's nonce as f.nonce/f.err. +func (f *fakeNonceProvider) GetNextNonce(_ gethCommon.Address) (uint64, error) { + return f.nonce, f.err +} + func (f *fakeNonceProvider) GetNonce(_ gethCommon.Address) (uint64, error) { return f.nonce, f.err } @@ -486,7 +492,7 @@ func Test_TxMemPool_SuccessfulFlushMarksSubmitted(t *testing.T) { // No silent drops: a failed flush submission must produce an observable WARN log // carrying the dropped tx hashes and enough context to debug a "lost -// transaction" report (eoa, nonce range, indexed nonce, batch size, reason, +// transaction" report (eoa, nonce range, next nonce, batch size, reason, // error). func Test_TxMemPool_FailedSubmitLogsDropWarning(t *testing.T) { key, err := crypto.GenerateKey() @@ -523,7 +529,7 @@ func Test_TxMemPool_FailedSubmitLogsDropWarning(t *testing.T) { assert.Contains(t, out, tx0.Hash().Hex(), "dropped tx hash must be logged") assert.Contains(t, out, tx1.Hash().Hex(), "dropped tx hash must be logged") assert.Contains(t, out, from.Hex(), "eoa must be logged") - assert.Contains(t, out, `"local-indexed-nonce":0`, "indexed frontier must be logged") + assert.Contains(t, out, `"local-next-nonce":0`, "next nonce must be logged") assert.Contains(t, out, `"batch-size":2`, "batch size must be logged") assert.Contains(t, out, "network down", "submit error must be logged") } @@ -694,7 +700,7 @@ func (c *countingCollector) TxPoolSize(queues int, queued int) { c.txPoolSizeSet = true } -// Stale txs (nonce below the indexed nonce) must be pruned from the queue, but +// Stale txs (nonce below the next nonce) must be pruned from the queue, but // pruning must NOT increment the TransactionsDropped metric — that metric is // reserved for build/submission errors of the Cadence transaction to Flow. func Test_TxMemPool_PruneStaleDoesNotIncrementDropped(t *testing.T) { @@ -767,7 +773,7 @@ func Test_NonceTracker_Classify(t *testing.T) { nonce uint64 want nonceVerdict }{ - {"indexed nonce is next-expected", nonceTracker{}, 5, 5, nonceNextExpected}, + {"nonce at the frontier is next-expected", nonceTracker{}, 5, 5, nonceNextExpected}, {"gap ahead queues", nonceTracker{}, 5, 7, nonceQueue}, {"below index is too low (no gap configured)", nonceTracker{}, 5, 3, nonceTooLow}, {"nonce 0 is next-expected on a zero tracker", nonceTracker{}, 0, 0, nonceNextExpected}, @@ -839,18 +845,18 @@ func Test_TxMemPool_RejectsNonceOutOfRange(t *testing.T) { } func Test_NonceTracker_ExpectedNonce(t *testing.T) { - assert.Equal(t, uint64(5), (&nonceTracker{localIndexedNonce: 5}).expectedNonce()) + assert.Equal(t, uint64(5), (&nonceTracker{localNextNonce: 5}).expectedNonce()) assert.Equal(t, uint64(7), - (&nonceTracker{localIndexedNonce: 5, lastConsecutivelySubmitted: toNonceWrapper(6)}).expectedNonce()) + (&nonceTracker{localNextNonce: 5, lastConsecutivelySubmitted: toNonceWrapper(6)}).expectedNonce()) assert.Equal(t, uint64(9), - (&nonceTracker{localIndexedNonce: 5, submitting: toNonceWrapper(8)}).expectedNonce()) - // The indexed frontier wins when it is ahead of our own sends. + (&nonceTracker{localNextNonce: 5, submitting: toNonceWrapper(8)}).expectedNonce()) + // The on-chain frontier wins when it is ahead of our own sends. assert.Equal(t, uint64(10), - (&nonceTracker{localIndexedNonce: 10, lastConsecutivelySubmitted: toNonceWrapper(6)}).expectedNonce()) + (&nonceTracker{localNextNonce: 10, lastConsecutivelySubmitted: toNonceWrapper(6)}).expectedNonce()) } func Test_NonceTracker_Transitions(t *testing.T) { - n := &nonceTracker{localIndexedNonce: 5} + n := &nonceTracker{localNextNonce: 5} // markSubmitting sets the in-flight marker. n.markSubmitting(7) @@ -871,9 +877,9 @@ func Test_NonceTracker_Transitions(t *testing.T) { // A rollback never disturbs the consecutively-submitted nonce. assert.Equal(t, toNonceWrapper(7), n.lastConsecutivelySubmitted) - // refreshIndexed updates the cached frontier. - n.refreshIndexed(12) - assert.Equal(t, uint64(12), n.localIndexedNonce) + // refreshNextNonce updates the cached frontier. + n.refreshNextNonce(12) + assert.Equal(t, uint64(12), n.localNextNonce) } // A stale success ack (e.g. once submissions run concurrently and a newer batch