Keep each sender's transactions in nonce order when selecting for a block#17
Open
alpenmilch411 wants to merge 1 commit into
Open
Keep each sender's transactions in nonce order when selecting for a block#17alpenmilch411 wants to merge 1 commit into
alpenmilch411 wants to merge 1 commit into
Conversation
… block selectForBlock built each sender's mineable prefix in nonce order, then flattened every sender into one list and sorted it by fee-per-byte. The nonce tie-break only fires when two txs have equal fee-per-byte, so a single account with two pending txs at different fees got reordered: the higher-fee, higher-nonce tx sorted ahead of its own predecessor. A block built from that order fails to apply (applyTx requires tx.nonce === account.nonce), so the candidate block is invalid. Group and pack by sender instead: keep each sender's nonce-ordered prefix together (only ever trimming its tail to fit maxBytes) and order senders by their best fee-per-byte. Adds regression tests for the same-sender differing-fee case (which every existing test missed — they all use a uniform fee), a maxBytes truncation, and an end-to-end check that the selected set applies through the real block path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I came across a small correctness bug in
Mempool.selectForBlockand wanted to send a fix.What happens
When a block is built,
selectForBlockpicks transactions from the mempool. It correctly works out each sender's mineable run (their pending txs in nonce order), but then flattens everyone together and sorts the whole list by fee-per-byte. The comment there says the sort keeps each sender's txs in nonce order — but that only holds when two txs have the same fee-per-byte, which is the only time the nonce tie-break actually runs.So if one account has two pending txs with different fees — say nonce 0 with a small fee and nonce 1 with a big one — the higher-fee nonce 1 sorts ahead of nonce 0. The returned list is then out of nonce order for that sender, and a block built from it fails to apply:
applyTxrequirestx.nonce === account.nonce, so it rejects nonce 1 (bad nonce (expected 0, got 1)) and the block is invalid.It's easy to miss because every existing
selectForBlocktest uses a uniform fee, and the one cross-fee test uses two different senders — so the same-sender / different-fee case never comes up.Minimal repro
One account at nonce 0, two pending txs:
selectForBlockreturns them as[nonce 1, nonce 0], and feeding that toapplyBlockTxsfails withbad nonce (expected 0, got 1).The fix
Keep each sender's nonce-ordered run together and order the senders by their best fee-per-byte, instead of sorting individual txs. A sender's txs then always go in nonce order, cross-sender fee priority is preserved, and a
maxBytescutoff just trims the tail instead of leaving a gap. Equal-fee senders get a deterministic tie-break so the template order is stable.Tests
Added the same-sender different-fee case (the bug), a
maxBytestruncation check that we keep the leading prefix, and an end-to-end test that runs the selected set through the real block path. All existing tests still pass.Impact is low in practice — the miner notices the bad set and falls back to an empty block, so nothing invalid is broadcast — but
selectForBlockis supposed to return a directly-mineable set, and right now it doesn't when a sender raises the fee on a later tx. Happy to adjust anything.