Skip to content

refactor(p2p): (A-588) maintain sorted array in tx pool instead of sorting on read#21079

Merged
danielntmd merged 1 commit intomerge-train/spartanfrom
danielntmd/refactor-mempool-pending-priority
Mar 5, 2026
Merged

refactor(p2p): (A-588) maintain sorted array in tx pool instead of sorting on read#21079
danielntmd merged 1 commit intomerge-train/spartanfrom
danielntmd/refactor-mempool-pending-priority

Conversation

@danielntmd
Copy link
Contributor

@danielntmd danielntmd commented Mar 3, 2026 •
edited
Loading

Replace Map<bigint, Set<bigint>> priority index with a sorted array, eliminating O(n log n) re-sorting on every read of pending transactions.

[Edit]:

  • Add small optimization for iteratePendingTxs and
    iterateEligibilePendingTxs to use a single SerialQueue entry instead
    of N queue entries

@danielntmd danielntmd force-pushed the danielntmd/refactor-mempool-pending-priority branch from e8339da to b2bd0a2 Compare March 3, 2026 21:39
@ludamad
Copy link
Collaborator

ludamad commented Mar 3, 2026 •
edited
Loading

It sounds like we want to implement a priority queue? https://en.wikipedia.org/wiki/Heap_(data_structure)
the heap invariant is cheaper than sorted invariant

I guess if reads >> writes this is better

@danielntmd danielntmd force-pushed the danielntmd/refactor-mempool-pending-priority branch from b2bd0a2 to bbaa401 Compare March 3, 2026 21:43
@danielntmd
Copy link
Contributor Author

danielntmd commented Mar 3, 2026 •
edited
Loading

It sounds like we want to implement a priority queue? https://en.wikipedia.org/wiki/Heap_(data_structure) the heap invariant is cheaper than sorted invariant

I guess if reads >> writes this is better

Foundation's priority queue is O(n) insertion vs array's O(log n) + O(n) splice and because it's a queue it has no index removal/search.

Heap implementation could marginally improve but we have no foundation implementation currently. Also for iteration (which we do quite frequently), it would be worse on average.

PhilWindle
PhilWindle requested changes Mar 4, 2026
View reviewed changes
yarn-project/p2p/src/client/p2p_client.ts Outdated
for (const txHash of await this.txPool.getPendingTxHashes()) {
const tx = await this.txPool.getTxByHash(txHash);
const txHashes = await this.txPool.getPendingTxHashes();
const txs = await this.txPool.getTxsByHash(txHashes);
Copy link
Collaborator

@PhilWindle PhilWindle Mar 4, 2026

Choose a reason for hiding this comment

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

Isn't this going to load the entire pending transaction pool into memory? We can't do that, it will potentially consume too much memory.

Copy link
Contributor Author

@danielntmd danielntmd Mar 4, 2026

Choose a reason for hiding this comment

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

Given that we are typically using a low maxTxsPerBlock , you're right that we wouldn't want to hydrate all pending txs. Reverting back to the original implementation.

yarn-project/p2p/src/mem_pools/tx_pool_v2/tx_metadata.ts
* Returns negative if a < b, positive if a > b, 0 if equal.
*/
export function compareFee(a: bigint, b: bigint): number {
export function compareFee(a: bigint, b: bigint): -1 | 0 | 1 {
Copy link
Collaborator

@PhilWindle PhilWindle Mar 4, 2026

Choose a reason for hiding this comment

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

Nice change here.

yarn-project/p2p/src/mem_pools/tx_pool_v2/tx_pool_indices.ts Outdated
result.push(meta);
}
for (const entry of this.#pendingByPriority) {
const meta = this.#metadata.get(txHashFromBigInt(entry.txHashBigInt));
Copy link
Collaborator

@PhilWindle PhilWindle Mar 4, 2026

Choose a reason for hiding this comment

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

If we added the string version of txHash tp PriorityComparable, could we remove the need to txHashFromBigInt so frequently?

Copy link
Contributor Author

@danielntmd danielntmd Mar 4, 2026

Choose a reason for hiding this comment

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

I think we can remove txHashFromBigInt completely if we embed the string version. Let me make the change.

refactor(p2p): maintain sorted array in tx pool instead of sorting on…
c2758d8 
… read

- Replace Map<bigint, Set<bigint>> priority index with a sorted array, eliminating O(n log n) re-sorting on every read of pending transactions.
- Cache the txHash string version to remove type conversions.
@danielntmd danielntmd force-pushed the danielntmd/refactor-mempool-pending-priority branch from bbaa401 to c2758d8 Compare March 4, 2026 13:22
@danielntmd danielntmd merged commit 09fa4ad into merge-train/spartan Mar 5, 2026
10 checks passed
@danielntmd danielntmd deleted the danielntmd/refactor-mempool-pending-priority branch March 5, 2026 14:54
@AztecBot AztecBot mentioned this pull request Mar 5, 2026
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2026
@AztecBot
feat: merge-train/spartan (#21034)
ab47a6e 
BEGIN_COMMIT_OVERRIDE
test: update proving-real test to mbps (#20991)
chore: epoch proving log analyzer (#21033)
chore: update pause script to allow resume (#21032)
feat: price bump for RPC transaction replacement (#20806)
refactor: remove update checker, retain version checks (#20898)
fix: (A-592) p2p client proposal tx collector test (#20998)
refactor: use publishers-per-pod in deployments (#21039)
chore: web3signer refreshes keystore (#21045)
feat(sequencer): set block building limits from checkpoint limits
(#20974)
chore(e2e): fix e2e bot L1 tx nonce reuse (#21052)
feat: Update L1 to L2 message APIs (#20913)
fix: (A-589) epochs l1 reorgs test (#20999)
feat(sequencer): add SEQ_MAX_TX_PER_CHECKPOINT config (#21016)
fix: drop --pid=host from docker_isolate (#21081)
feat: standby mode for prover broker (#21098)
fix(p2p): remove default block handler in favor of block handler
(#21105)
feat(validator): add VALIDATOR_ env vars for independent block limits
(#21060)
refactor(p2p): decouple proposal validators from base class via
composition (#21075)
feat: additional validation in public setup allowlist (onlySelf + null
msg sender) (#21122)
fix: (A-591) aztecProofSubmissionEpochs incorrectly named as
aztecProofSubmissionWindow (#21108)
refactor(sequencer): rename SEQ_GAS_PER_BLOCK_ALLOCATION_MULTIPLIER to
SEQ_PER_BLOCK_ALLOCATION_MULTIPLIER (#21125)
fix: unbound variable in check_doc_references.sh with set -u (#21126)
feat: calldata length validation of public setup function allowlist
(#21139)
fix: include mismatched values in tx metadata validation errors (#21147)
feat: single-node implementation of slash-protection signer (#20894)
feat: Remove non-protocol contracts from public setup allowlist (#21154)
chore: More updated Alpha configuration (#21155)
chore: tally slashing pruning improvements (#21161)
fix: update dependencies (#20997)
fix: omit bigint priceBumpPercentage from IPC config in testbench worker
(#21169)
refactor(p2p): (A-588) maintain sorted array in tx pool instead of
sorting on read (#21079)
fix(p2p): report most severe failure in runValidations (#21185)
fix: use dedicated L1 account for bot bridge resume tests to avoid nonce
race (#21148)
fix: parse error.message in formatViemError (#21163)
fix: bump lighthouse consensus client v7.1.0 -> v8.0.1 (#21170)
chore: code decuplication + refactor (public setup allowlist) (#21200)
END_COMMIT_OVERRIDE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PhilWindle PhilWindle PhilWindle approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@danielntmd @ludamad @PhilWindle