Skip to content

fix(txpool): restore age-based future eviction - #686

Merged
n13 merged 1 commit into
mainfrom
n13/fix-future-queue-eviction
Sep 1, 2026
Merged

fix(txpool): restore age-based future eviction#686
n13 merged 1 commit into
mainfrom
n13/fix-future-queue-eviction

Conversation

@n13

@n13 n13 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Side note: This only affects the future queue, which is really only a temporary space to hold transactions that arrive out of order

An attacker could fill up future queue with high tip amounts and then basically cripple the future queue with that, so age based eviction is simpler.

But overall most transactions will end up in the ready queue and will be totally unaffected by any of this.

Tip based priority is easier to tamper with though so I still think we should do this.

Port of chain-private #39, which did not survive the private→public merge-back.

Priority-first eviction of the future queue is unsafe here: tips on nonce-gap transactions are not charged unless they execute, so an attacker can pin the queue with fake high tips and evict victims. Restore oldest-first eviction (Immunefi #90496).

future_limit_enforcement_evicts_oldest_first passes.

@n13
n13 marked this pull request as draft September 1, 2026 02:10
@n13

n13 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Not super important for now

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with the specific goal of confirming this restores the original upstream future-queue logic rather than writing a new age-based policy.

Upstream comparison — exact match confirmed

I extracted enforce_limits from four revisions and diffed them:

Revision Result vs. this PR
472423e3 — vendored sc-transaction-pool 44.1.0, post-cargo fmt byte-identical
b51fa56d^ — last commit before the tip/priority change byte-identical
4d3d6d45 — the chain-private commit this ports byte-identical
2b4eddf4 — raw vendor drop, pre-cargo fmt identical modulo rustfmt (Ordering::Equal => brace style, break; semicolons)

So the future-eviction fold is not a re-derivation — it is the upstream code, character for character, in the repo's own formatting. The revert is complete: nothing of b51fa56d's priority-first fold survives, and the intermediate future-queue fixes (98105919, 601b567b, 91e3a7bb) are untouched.

Correctness of the security argument

Confirmed in-tree rather than taken on faith:

  • runtime/src/lib.rs:192 includes pallet_transaction_payment::ChargeTransactionPayment, so validation priority is tip-derived, and a nonce-gapped transaction never dispatches — the advertised tip is never charged. Priority on a future transaction is therefore free to fabricate.
  • client/transaction-pool/src/graph/validated_pool.rs bans everything eviction removes (self.rotator.ban(&Instant::now(), removed.iter().copied())).

Together those make priority-first future eviction strictly worse than age-based: an attacker pins the queue at zero cost and gets every honest future transaction banned for ban_time. Age-based eviction doesn't make future-queue flooding free — an attacker can still push out older entries — but it costs them continuous spam and ages out their own entries too. Right call, and the PR description's framing of the tradeoff is accurate.

Checks

  • cargo test -p sc-transaction-pool --lib graph::base_pool — 19 passed, 0 failed, including future_limit_enforcement_evicts_oldest_first.
  • cargo +nightly fmt -p sc-transaction-pool -- --check — clean.
  • use std::cmp::Ordering (line 23) is still live: the ready-queue fold at lines 509–517 uses it. No dead import, so no -D warnings risk.

Notes (non-blocking)

  1. base_pool.rs:487-488 — doc comment now diverges from upstream. Upstream says "lowest priority first or those that occupy the pool for the longest time in case priority is the same," which describes only the ready queue and was never true of the future queue. The replacement is more accurate. It's the one intentional deviation and it adds a small merge-conflict surface on the next vendor refresh; I'd keep it — accuracy wins.

  2. Consider anchoring the rationale in the code. This policy has now flipped twice (b51fa56d → this PR), and after the revert nothing in the file records why age-based is deliberate. The next reader sees the same "age-only eviction looks exploitable" shape that motivated b51fa56d. A one-line reference to Immunefi #90496 would prevent a repeat — best placed in the doc comment above (already divergent from upstream) rather than inside the fold, so the restored block stays byte-identical to upstream.

  3. base_pool.rs:1465 — test comment reads backwards. "Priority must not allow an old future transaction to remain pinned in the pool" is hard to parse against the assertion that the old tx is evicted. Something like "high priority must not exempt an older future tx from age-based eviction" states what's being tested. Trivial.

Verdict: LGTM

The eviction logic is provably the pre-tip-change upstream code, the revert is complete and self-consistent, and the test correctly discriminates age-based from priority-based behavior (the older transaction is the higher-priority one, so it only passes under age-first). Only the three cosmetic notes above; none block merge.

@n13
n13 marked this pull request as ready for review September 1, 2026 02:20
@n13
n13 merged commit 389d32d into main Sep 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant