-
Notifications
You must be signed in to change notification settings - Fork 335
feat: sequence aware mempool prototype #2414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v0.39.x-celestia
Are you sure you want to change the base?
Conversation
|
||
- Signer-bundled sets: A `txSet` holds transactions from a single signer. The set tracks: | ||
- `txs`: maintained in ascending sequence order; ties broken by earlier arrival timestamp. | ||
- `aggregatedPriority`: gas-weighted average of member transaction priorities, floored to `int64`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the priority is the average, couldn't someone post very cheap transactions at the beginning, and have the 100th transaction with crazy high fee forcing you to include the first ones, but when you arrive to the last, the block space is full 😆
What do you think of switching to median?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial intuition was to use "next tx gas" as txSet
priority. So we can always extract the most profitable transactions from mempool and maintain ordering inside sets.
But Increasing fees under congestion
section clarifies this for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't someone post very cheap transactions at the beginning, and have the 100th transaction with crazy high fee forcing you to include the first ones, but when you arrive to the last, the block space is full 😆
The user would presumably still pay for the 100th transaction because it would just be included in the following block unless you can find a way to invalidate it.
A counter proposal to your median idea (which I think could also be effective):
Evaluate the priority based on the first n bytes worth of transactions (where n is less than the size of a block) rather than the aggregated priority of all the transactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we run some benchmarks while we're at it to see if this is introducing any delays in processing transactions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have existing benchmarks that would be good to understand any changes in the performance (or do we need to write new ones)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have anything on main. I have personal ones I use for my benchmarks, but they're unmergeable 😄
- Global ordering of sets: `orderedTxSets` is kept sorted by `(aggregatedPriority desc, firstTimestamp asc)`. This preserves FIFO between sets of equal aggregated priority. | ||
- Within-set ordering: Transactions are iterated in ascending sequence, guaranteeing that a later sequence from the same signer is never submitted before an earlier one. | ||
- Eviction policy when full: Compute the new set aggregated priority if the incoming transaction were added (`aggregatedPriorityAfterAdd`). Identify victim sets with aggregated priority strictly below the incoming set. Evict transactions from victim sets starting from the end (highest sequence numbers first) until enough bytes are freed. If insufficient bytes can be freed, reject the incoming transaction. | ||
- TTL purge: Expiration by height/time removes transactions from sets and reorders or removes empty sets accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when purging after TTL, we could delete the earliest nonce transaction while the ones after it are not removed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we'd remove an entire set all together - the reasoning is that all subsequent transactions would have invalid nonces and be rejected by recheck tx anyway
- transaction index, byte accounting, reservation tracking | ||
|
||
- Aggregation: | ||
- On insert: update aggregated prioritt `totalGasWanted += gasWanted`, `weightedPrioritySum += priority * gasWanted`, then `aggregatedPriority = weightedPrioritySum / totalGasWanted` (integer division). Aggregated priority is relative to the amount of gasWanted per transaction. Transactions with more gasWanted will have larger weighting. Alternatively we could have used transaction size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- On insert: update aggregated prioritt `totalGasWanted += gasWanted`, `weightedPrioritySum += priority * gasWanted`, then `aggregatedPriority = weightedPrioritySum / totalGasWanted` (integer division). Aggregated priority is relative to the amount of gasWanted per transaction. Transactions with more gasWanted will have larger weighting. Alternatively we could have used transaction size. | |
- On insert: update aggregated priority `totalGasWanted += gasWanted`, `weightedPrioritySum += priority * gasWanted`, then `aggregatedPriority = weightedPrioritySum / totalGasWanted` (integer division). Aggregated priority is relative to the amount of gasWanted per transaction. Transactions with more gasWanted will have larger weighting. Alternatively we could have used transaction size. |
mempool/cat/pool.go
Outdated
@@ -334,10 +334,11 @@ func (txmp *TxPool) TryAddNewTx(tx *types.CachedTx, key types.TxKey, txInfo memp | |||
txmp.metrics.FailedTxs.Add(1) | |||
return rsp, nil | |||
} | |||
// removed noisy print used during debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// removed noisy print used during debugging |
mempool/cat/pool.go
Outdated
@@ -461,6 +462,7 @@ func (txmp *TxPool) Update( | |||
newPreFn mempool.PreCheckFunc, | |||
newPostFn mempool.PostCheckFunc, | |||
) error { | |||
// removed noisy print used during debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// removed noisy print used during debugging |
mempool/cat/pool.go
Outdated
outer: | ||
for _, set := range victimSets { | ||
// iterate in reverse order removing the higher sequence numbers first | ||
for i := len(set.txs) - 1; i >= 0; i-- { | ||
tx := set.txs[i] | ||
txmp.evictTx(tx) | ||
availableBytes += tx.size() | ||
if availableBytes >= wtx.size() { | ||
break outer | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be reorganized to avoid having a tag? it becomes harder to understand the logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would emphasize two facts in context section:
-
Eviction never removes lower sequence numbers in a set before removing higher ones, preserving the ability to submit remaining sequences correctly.
-
Rather than replace by fee, users can improve the likeness of their transaction landing by increasing the gas price of later transactions, thus raising the aggregated priority. Given sequence awareness, a replace by fee protocol may easily be supported in the future.
|
||
- Signer-bundled sets: A `txSet` holds transactions from a single signer. The set tracks: | ||
- `txs`: maintained in ascending sequence order; ties broken by earlier arrival timestamp. | ||
- `aggregatedPriority`: gas-weighted average of member transaction priorities, floored to `int64`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial intuition was to use "next tx gas" as txSet
priority. So we can always extract the most profitable transactions from mempool and maintain ordering inside sets.
But Increasing fees under congestion
section clarifies this for me.
- Within a signer, transactions are never emitted out of ascending sequence order. | ||
- Across signers, ordering is by set weighted average priority (desc), with earlier `firstTimestamp` breaking ties (FIFO between equal-priority sets). | ||
- Weighted average priority updates are consistent with gas-weighted means as transactions are added/removed. | ||
- Eviction never removes lower sequence numbers in a set before removing higher ones, preserving the ability to submit remaining sequences correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This is very important - I would mention this even in context section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea to avoid the attack Rachid raised is instead of calculating the average weighted priority, preserve the logic to capture tx sets but only inspect the lowest sequence in each tx set when evaluating which txs to include in a block. Then choose txs to include based on priority.
In other words, I'm proposing a less sophisticated mempool that does not calculate average weighted priority but does respect the signer's need to get lower sequence txs included before higher sequence txs.
|
||
## Decision | ||
|
||
Group transactions by signer into signer-bundled sets ("tx sets") and order based on average weighted priority per set. Within tx set order transactions in ascending sequence order. Block building and recheckTx iterates through transactions in those order. This will preserve sequence ordering per signer while maximising tx fees per block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Is it possible for an attacker to submit two txs:
- tx 1 with sequence 1 and a low priority
- tx 2 with sequence 2 and a high priority
such that the second tx increases the average weighted priority of the tx set such that tx 1 gets included on-chain, however once tx 1 gets included on-chain it invalidates tx 2 so tx 2 can no longer get on chain. The end result is that the signer got tx 1 included on-chain ahead of competing txs that would have paid more in fees?
Update: oops disregard, I see Rachid raised the same concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded to it in rachid's comment
|
||
## Decision | ||
|
||
Group transactions by signer into signer-bundled sets ("tx sets") and order based on average weighted priority per set. Within tx set order transactions in ascending sequence order. Block building and recheckTx iterates through transactions in those order. This will preserve sequence ordering per signer while maximising tx fees per block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than an attack, is this an issue in the honest but congested case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need this mechanism in the case of congestion or more broadly whenever we expect priorities to be different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callum will hand this over to me and I'll focus on cleaning it up
Can you perhaps pseudocode the design? So could only the first transaction with the lowest sequence make it into a block or would you be able to do multiple? |
A priority mempool based on gas price has always been incompatible with the replay protection mechanism that relies on monotonically increasing sequences. This becomes increasingly apparent when the same signer is submitting multiple blobs per block.
I have written this ADR proposing a simple-ish solution which bundles pending transactions per signer. I also have written a working prototype (some of it vibe-coded) which solves the issue with the tx client test here: celestiaorg/celestia-app#5655