Add priority ordering core#1083
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the deterministic, pure “priority ordering core” for the transaction-priorities feature, including helper routines and unit tests, so block formation can produce a prioritized prefix while enforcing sender-nonce contiguity and per-entity gas budgets.
Changes:
- Updated
Config.MaxGasPerEntityPerBlockdocumentation to reflect sender-chain demotion under budget constraints. - Added
Prioritizeimplementation plus helper functions for classification, per-sender nonce-run reduction, budgeted greedy prefix selection, and recombination with the remainder. - Added unit tests covering comparators, helpers, and an end-to-end scenario spanning both constraints.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gossip/blockproc/priorities/types.go | Updates config comment to match budget/nonce-chain demotion semantics. |
| gossip/blockproc/priorities/ordering.go | Introduces the pure transaction prioritization/ordering core and helper routines. |
| gossip/blockproc/priorities/ordering_test.go | Adds unit tests for ordering semantics and helper behavior. |
Comments suppressed due to low confidence (2)
gossip/blockproc/priorities/ordering_test.go:89
- This test assumes a specific hash ordering between
makeTxG(0, 21000)andmakeTxG(0, 22000). Hash ordering between two arbitrary txs isn’t something the test should rely on. DerivelowHash/highHashby comparing the hashes and swapping to make the test resilient to changes in tx encoding/hashing.
// Same nonce, different hash
lowHash, highHash := makeTxG(0, 21000), makeTxG(0, 22000)
require.Negative(t, bytes.Compare(lowHash.Hash().Bytes(), highHash.Hash().Bytes()))
prio := Prio(1, 10, 100)
gossip/blockproc/priorities/ordering_test.go:366
- This test assumes
makeTxN(0)has a higher hash thanmakeTxN(3). Since tx hashes aren’t ordered by nonce, this is brittle across encoding/hashing changes. Swap the two based onbytes.ComparesolowHashTx/highHashTxare derived from the actual hashes produced.
highHashTx, lowHashTx := makeTxN(0), makeTxN(3)
require.Positive(t, bytes.Compare(highHashTx.Hash().Bytes(), lowHashTx.Hash().Bytes()))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ac08560 to
e48ba7c
Compare
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
LuisPH3
left a comment
There was a problem hiding this comment.
The pr proposes an algorithm which sorts a list of transactions using an order:
the order prepends transactions based in their priority level/weight.
It respects the nonce order of prioritised transactions. tiebreakers with hash make replicated resolution deterministic.
b8b8462 to
fee23b7
Compare
| if c := cmp.Compare(x.priority.Level, y.priority.Level); c != 0 { | ||
| return c | ||
| } | ||
| if c := cmp.Compare(x.priority.Weight, y.priority.Weight); c != 0 { |
There was a problem hiding this comment.
| if c := cmp.Compare(x.priority.Level, y.priority.Level); c != 0 { | |
| return c | |
| } | |
| if c := cmp.Compare(x.priority.Weight, y.priority.Weight); c != 0 { | |
| if c := x.priority.Cmp(y.priority); c != 0 { |
No need to reimplement the priority comparison.
There was a problem hiding this comment.
Not quite. Priority.Cmp returns equal if level is 0 no matter the weight. If we use it here we would have a situation where if the level is 0 we ignore the weight and break ties only using the hash. This would be weird since this breaks the order level→weight→hash
There was a problem hiding this comment.
Right, then a unit test case is missing. I tried it locally before the review and no test failed.
9ece95c
9ece95c to
8bbd752
Compare
HerbertJordan
left a comment
There was a problem hiding this comment.
Very nice and clean code, for the feature as well as for the tests. Great!
I added one suggestion for a named type and two potential issues that should be addressed. Please let me know what you think
1b983d6 to
e84267d
Compare
This PR adds the pure ordering core of the transaction-priorities feature.
This is used in a follow-up to order the transactions in the
c_block_callbacks.goafter the scrambler run.Prioritizereorders a base-ordered, pre-filtered transaction list so that registry-prioritized transactions form a prefix in (level desc, weight desc, hash asc) order, subject to two coupled constraints:Config.MaxGasPerEntityPerBlockgas (by gas limit) on prioritized transactions. Selection walks the per-sender frontiers greedily; a frontier that doesn't fit its entity's remaining budget blocks that sender's run, so budget is only spent on transactions that can actually execute in the prefix.Everything not selected (non-prioritized, nonce-blocked, over budget) keeps its relative base order after the prefix. The function is a pure, deterministic total function of its inputs.
Classification is abstracted behind a Classifier interface (the EVM-backed implementation and the block-formation call site land in a follow-up).
Unit tests cover each helper (classification, nonce-run, budgeted prefix selection, packing) plus an end-to-end scenario exercising all constraints together.