Skip to content

fix(nonce_manager): reuse freed/gap nonce instead of skipping it#93

Merged
abhicris merged 1 commit into
kcolbchain:mainfrom
kite-builds:fix/nonce-manager-reuse-released-gap
Jun 4, 2026
Merged

fix(nonce_manager): reuse freed/gap nonce instead of skipping it#93
abhicris merged 1 commit into
kcolbchain:mainfrom
kite-builds:fix/nonce-manager-reuse-released-gap

Conversation

@kite-builds
Copy link
Copy Markdown
Contributor

What

NonceManager.acquire_nonce returned max(pending_nonces) + 1 whenever any nonce was pending. When a lower nonce was freed by release_nonce (e.g. a tx that failed locally before broadcast) while a higher nonce was still pending, the freed nonce was never reissued — the next acquire_nonce jumped past it. The same hole appears after an out-of-order confirmation.

This change makes acquire_nonce return the lowest nonce at or above confirmed_nonce that is not already pending, walking the SortedSet from confirmed_nonce via irange.

Why

Ethereum requires gapless nonces. A skipped nonce leaves a permanent hole, and every higher-nonce pending transaction sits in the mempool unmined until that hole is filled — a liveness bug for any agent that ever releases a nonce. It also contradicted release_nonce's own docstring, which states it frees the nonce "making it available again."

Repro on main:

nm.acquire_nonce(addr)        # -> 0
nm.acquire_nonce(addr)        # -> 1
nm.release_nonce(addr, 0)     # tx0 failed locally; free nonce 0
nm.acquire_nonce(addr)        # -> 2  (BUG: skips the freed nonce 0)

After the fix the final call returns 0.

In the gapless common case the result is identical to the old max + 1 behaviour, so existing callers are unaffected. All existing nonce_manager tests pass unchanged.

Tests

Added test_release_lower_nonce_is_reused, which releases a lower nonce while a higher one stays pending and asserts the freed nonce is reused, then that the sequence extends normally afterward.

  • pytest tests/test_nonce_manager.py -v10 passed
  • pytest -q (full suite) → 171 passed, 56 skipped
  • ruff check switchboard/nonce_manager.py → clean

No related open issue; this is a standalone correctness fix to the nonce manager (introduced in PR #11) that hardens the same component issue #79 plans to extend.

acquire_nonce returned max(pending_nonces) + 1 whenever any nonce was
pending. If a lower nonce was freed by release_nonce (a tx that failed
locally before broadcast) while a higher nonce stayed pending, the freed
nonce was never reissued — acquire_nonce jumped past it. The same applies
to a gap left by an out-of-order confirmation.

Ethereum requires gapless nonces, so a skipped nonce stalls every
higher-nonce pending transaction in the mempool until the gap is filled.
This also contradicted release_nonce's own contract ("making it available
again").

Fix: acquire_nonce now returns the lowest nonce at or above confirmed_nonce
that is not already pending, walking the SortedSet from confirmed_nonce via
irange. In the gapless common case the result is identical to the old
max+1 behaviour, so existing callers are unaffected.

Added a regression test (test_release_lower_nonce_is_reused) covering
release-of-lower-nonce followed by reuse. Full suite: 171 passed, 56
skipped.
@abhicris
Copy link
Copy Markdown
Contributor

abhicris commented Jun 4, 2026

🤖 Audit verdict: safe

Legitimate logic fix for Ethereum nonce gap-filling with correct algorithm and comprehensive regression test; no malicious intent or security issues detected.

Audited by the kcolbchain PR pipeline. See pipeline docs.

@abhicris abhicris merged commit b8d5c90 into kcolbchain:main Jun 4, 2026
2 checks passed
@abhicris
Copy link
Copy Markdown
Contributor

abhicris commented Jun 4, 2026

Merged — thanks, @kite-builds. That's 3 merged PRs to kcolbchain now.

At 5 merged PRs you become a Fellow and enter the invited-work pool.

Next-up issues across the org: https://kcolbchain.com/invitations/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants