Skip to content

Conversation

@dvush
Copy link
Collaborator

@dvush dvush commented Nov 14, 2025

This PR adds a prioritized orderpool.

This is a data-structure used to include bundles in certain order while keeping track of nonces.

The example usage using pipelines API is in src/prioritized_pool/step.rs
Its currently unfinished but will be updates in later PRs.

* adds a prioritized orderpool
* adds an example stage using prioritized orderpool (unfinished)
Copilot AI review requested due to automatic review settings November 14, 2025 12:53
Copilot finished reviewing on behalf of dvush November 14, 2025 12:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a prioritized orderpool data structure for managing bundles during block building while tracking nonces. The implementation enables ordered inclusion of bundles based on priority (e.g., effective gas price) while ensuring nonce validity.

Key changes include:

  • New PrioritizedOrderpool data structure with nonce tracking and priority-based ordering
  • Integration with the execution pipeline to track changed nonces after transaction execution
  • Example implementation using effective gas price ordering for bundle prioritization

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/primitives.rs Adds AccountNonce and BundleNonce primitive types for nonce tracking
src/prioritized_pool/mod.rs Core implementation of the prioritized orderpool with traits and algorithms for order management
src/prioritized_pool/tests.rs Comprehensive test suite covering various scenarios including competing orders, pending orders, and optional nonces
src/prioritized_pool/step.rs Example integration with pipelines API demonstrating effective gas price ordering
src/payload/exec.rs Extends execution results to track nonce changes after transaction execution
src/lib.rs Adds new module exports for prioritized_pool and primitives
Cargo.toml Adds priority-queue dependency for internal priority queue implementation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +49 to +52
// for each address we keep lowest nonce
nonces
.sort_by(|a, b| a.address.cmp(&b.address).then(a.nonce.cmp(&b.nonce)));
nonces.dedup_by_key(|n| n.address);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The comment says "for each address we keep lowest nonce" but the deduplication logic using dedup_by_key(|n| n.address) will keep only the first occurrence for each address after sorting. While this works correctly given the sort order, the logic would be clearer if the comment explicitly stated that sorting ensures the lowest nonce comes first, or if the deduplication was more explicit about the criteria (e.g., keeping min nonce per address).

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
state.merge_transitions(BundleRetention::Reverts);

let state = state.take_bundle();
let nonces_after_execution = extract_changed_nonces_for_executable(&state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a discussion starter, not a strong opinion:

extract_changed_nonces_for_executable seems like something that is derived from the data that is already stored in a checkpoint. So far we had this pattern where all essential information that cannot be derived is stored on the Checkpoint type and all derived and helper data is implemented in CheckpointExt for example:

fn gas_used(&self) -> u64 {
self.result().map_or(0, |result| result.gas_used())
}

there is also this function that retrieves the nonce after running txs in a checkpoint:

fn nonce_of(&self, address: Address) -> Result<u64, ProviderError> {
Ok(
self
.basic_ref(address)?
.map(|basic| basic.nonce)
.unwrap_or_default(),
)
}

and this tells you about all signers in a checkpoint:

fn signers(&self) -> Vec<Address> {
self
.transactions()
.iter()
.map(|tx| tx.signer())
.unique()
.collect()
}

If your goal here is to cache those values for performance reasons, maybe we can think of a more generalized caching mechanism at the Checkpoint level, or have a wrapper type that computes it once for a checkpoint. I'm hesitant to start adding more case-specific fields to checkpoints, as I'm trying to keep this primitive as generic as possible.

What are your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment also applies to state. the state is stored in results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can move it to the function call site, I just thought that since its a private field than adding it or removing it should be practically simple

PrioritizedOrderpoolPriority<Order = BundleWithNonces<P::Bundle, P>>,
{
pending: Pending,
prioritized_orderpool:
Copy link
Collaborator

@karim-agha karim-agha Nov 14, 2025

Choose a reason for hiding this comment

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

I would try to not make assumptions about the execution of the pipeline that is using this step. You might have many concurrent pipelines running a step and if they all share the same orderpool we might get undefined behavior.

If I'm understanding the intention here correctly (and the &mut self accessors on PrioritizedOrderpool), I would recommend one of the two following paths forward:

  1. Either the step consumes the PrioritizedOrderpool by value and PrioritizedOrderpool maintains some form of a mpsc::Receiver for incoming orders,
  2. Or use something like im that gives immutable data structures and each instance of the step operates on a snapshot/clone of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They should not share it, its only constructed by this pipeline and consumed internally.

Lets say I want to go by rote 1, who construct this pipeline and pass this argument?

Comment on lines +55 to +62
struct TestContext {
generated_id_count: u64,
nonces: PrioritizedOrderpoolHashMapNonces,
order_pool: PrioritizedOrderpool<
PrioritizedOrderpoolTestPriority,
PrioritizedOrderpoolTestBundle,
>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also a test utility called OneStep that you can use to unit test a single step in more realistic setting as if it was running in a pipeline and responding to a payload job. Here's an example of how it is used:

rblib/src/steps/revert.rs

Lines 300 to 313 in f4abbf1

async fn one_revert_one_ok<P: TestablePlatform>() {
let output = OneStep::<P>::new(RemoveRevertedTransactions::default())
.with_payload_tx(|tx| tx.transfer().with_default_signer().with_nonce(0))
.with_payload_tx(|tx| tx.reverting().with_default_signer().with_nonce(1))
.run()
.await
.unwrap();
let ControlFlow::Ok(payload) = output else {
panic!("Expected Ok payload, got: {output:?}");
};
assert_eq!(payload.history().transactions().count(), 1);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO this would be a good test for pipeline part of this but tests here are only for PrioritizedOrderpool by itself that do not depend on pipeline API. I would like this implementation to be work independently from pipelines and pipeline would be one of the users and other would be l1 builder.

@SozinM SozinM merged commit e71e401 into main Nov 25, 2025
4 checks passed
@SozinM SozinM deleted the dvush/prioritized_orderpool branch November 25, 2025 16:04
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.

4 participants