Refactor: fix docs, clean up code quality #843

prestwich · 2025-12-19T16:37:50Z

📝 Summary

No longer based on #842, which has been closed

Improves and corrects documentation in the live_builder, particularly around Order propagation through the OrderPool. Functionality does not change. Most significant API change is around BlobTypeOrderFilter, by moving constructors from free functions to assoc functions

Improve and correct OrderPool documentation
Add Copy impl for ReplacementData<KeyType>
Add missing Copy impl for NonceKey
make implementation of BlobTypeOrderFilter construction idiomatic for Rust
add missing trace! event to BlobTypeOrderFilter when filtering an order
add rule_name to BlobTypeOrderFilter to ensure behavior is clear in tracing event
add From impls for ReplaceableOrderEvent, and remove some relevant .clone invocations
update AutoRemovingOrderPoolSubscriptionId to use Weak to prevent them from holding OrderPool memory alloc for dying subscriptions
use FuturesUnordered instead of Vec<JoinHandle<()>> in the order pool to ensure parallelization of handle reaping

✅ I have completed the following steps:

Run make lint
Run make test
Added tests (if applicable)

Copilot

Pull request overview

This PR refactors the live_builder order_input code with a focus on improving documentation, code quality, and idiomatic Rust patterns. The PR includes documentation improvements for OrderPool propagation, adding Copy trait implementations, cleaning up unnecessary clones, refactoring BlobTypeOrderFilter API from free functions to associated functions, and improving resource management with Weak pointers.

Improved documentation for OrderPool and related components to clarify order propagation behavior
Added Copy trait implementations for types that support it (e.g., ReplacementData<KeyType>, ValidBundleState, BundleReplacementState)
Refactored BlobTypeOrderFilter API from free functions to idiomatic associated functions

Reviewed changes

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

Show a summary per file

File	Description
crates/rbuilder/src/live_builder/order_input/orderpool.rs	Enhanced documentation for OrderPool and related structures; changed `process_commands` to use generic iterator; removed unnecessary `.clone()` calls
crates/rbuilder/src/live_builder/order_input/order_replacement_manager.rs	Improved documentation; added `Copy` trait to `ValidBundleState` and `BundleReplacementState`; removed `.clone()` calls in tests
crates/rbuilder/src/live_builder/order_input/mod.rs	Changed handles storage from `Vec` to `FuturesUnordered` for parallel reaping; updated `AutoRemovingOrderPoolSubscriptionId` to use `Weak` pointer; enhanced documentation
crates/rbuilder/src/live_builder/order_input/blob_type_order_filter.rs	Refactored from free functions to associated functions; added `rule_name` field; added trace event for filtered orders; changed filter logic
crates/rbuilder/src/live_builder/order_flow_tracing/order_flow_tracer.rs	Utilized new `From` trait implementations to remove `.clone()` calls; improved documentation
crates/rbuilder/src/live_builder/order_flow_tracing/events.rs	Added `From` trait implementations for `ReplaceableOrderEvent`
crates/rbuilder/src/live_builder/building/mod.rs	Updated to use new `BlobTypeOrderFilter` associated functions
crates/rbuilder-primitives/src/test_data_generator.rs	Removed unnecessary `.clone()` calls
crates/rbuilder-primitives/src/lib.rs	Added `Copy` impl for `ReplacementData<KeyType>`; removed unnecessary `.clone()` in `replacement_key_and_sequence_number`
crates/rbuilder-primitives/benches/ssz_proof.rs	Removed unnecessary `default()` calls and `.clone()` for unit-like structs

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

crates/rbuilder/src/live_builder/order_input/blob_type_order_filter.rs

crates/rbuilder/src/live_builder/order_input/orderpool.rs

Copilot · 2025-12-30T14:59:13Z

crates/rbuilder/src/live_builder/order_input/order_replacement_manager.rs

+/// Cancels are treated as highest-priority, and after that we must always
+/// honor the replacement with highest `sequence_number`.
+///
+/// Although all the structs and fields say "bundle" we always refer to Bundle


Spelling error: "reefer" should be "refer"

copilot this error is fixeeed in the diff

crates/rbuilder/src/live_builder/order_input/orderpool.rs

Copilot

Pull request overview

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

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

Copilot · 2025-12-30T15:30:08Z

crates/rbuilder/src/live_builder/order_input/orderpool.rs

 const TIME_TO_KEEP_BUNDLE_CANCELLATIONS: Duration = Duration::from_secs(60);
-/// Push to pull for OrderSink. Just poll de UnboundedReceiver to get the orders.
+
+/// Push to pull for OrderSink. Just poll de UnboundedReceiver to get the


Typo in documentation: "poll de UnboundedReceiver" should be "poll the UnboundedReceiver".

Suggested change

/// Push to pull for OrderSink. Just poll de UnboundedReceiver to get the

/// Push to pull for OrderSink. Just poll the UnboundedReceiver to get the

Copilot · 2025-12-30T15:30:08Z

crates/rbuilder/src/live_builder/order_input/blob_type_order_filter.rs

+    /// [EIP-7594]: https://eips.ethereum.org/EIPS/eip-7594
+    pub const fn new_fusaka(sink: Box<dyn ReplaceableOrderSink>) -> Self {
+        fn fusaka(tx: &TransactionSignedEcRecoveredWithBlobs) -> bool {
+            tx.as_ref().is_eip4844() && tx.blobs_sidecar.is_eip7594()


The filter logic has changed incorrectly. The old implementation returned true for non-EIP-4844 transactions (allowing them through), but the new logic returns false for them. This will incorrectly filter out all non-blob transactions. The correct logic should be: !tx.as_ref().is_eip4844() || tx.blobs_sidecar.is_eip7594() to accept non-blob transactions OR blob transactions with EIP-7594 sidecars.

Suggested change

tx.as_ref().is_eip4844() && tx.blobs_sidecar.is_eip7594()

!tx.as_ref().is_eip4844() || tx.blobs_sidecar.is_eip7594()

Copilot · 2025-12-30T15:30:08Z

crates/rbuilder/src/live_builder/order_input/blob_type_order_filter.rs

+    /// [EIP-7594]: https://eips.ethereum.org/EIPS/eip-7594
+    pub const fn new_pre_fusaka(sink: Box<dyn ReplaceableOrderSink>) -> Self {
+        fn pre_fusaka(tx: &TransactionSignedEcRecoveredWithBlobs) -> bool {
+            tx.as_ref().is_eip4844() && tx.blobs_sidecar.is_eip4844()


The filter logic has changed incorrectly. The old implementation returned true for non-EIP-4844 transactions (allowing them through), but the new logic returns false for them. This will incorrectly filter out all non-blob transactions. The correct logic should be: !tx.as_ref().is_eip4844() || tx.blobs_sidecar.is_eip4844() to accept non-blob transactions OR blob transactions with EIP-4844 sidecars.

Suggested change

tx.as_ref().is_eip4844() && tx.blobs_sidecar.is_eip4844()

!tx.as_ref().is_eip4844() || tx.blobs_sidecar.is_eip4844()

Copilot · 2025-12-30T15:30:08Z

crates/rbuilder/src/live_builder/order_input/blob_type_order_filter.rs

+/// provided to remove pre-Fusaka [EIP-4844] blobs ([`Self::new_fusaka`]) or
+/// post-Fusaka [EIP-7594] blobs ([`Self::new_pre_fusaka`]).
+///
+/// Since it's very unlikely what we have many wrong blobs we only filter on


Typo in comment: "we only filter on" should be clearer. Consider: "Since it's very unlikely that we have many wrong blobs, we only filter on..."

Suggested change

/// Since it's very unlikely what we have many wrong blobs we only filter on

/// Since it's very unlikely that we have many wrong blobs, we only filter on

prestwich · 2025-12-30T15:35:11Z

crates/rbuilder/src/live_builder/order_input/blob_type_order_filter.rs

+    /// [EIP-7594]: https://eips.ethereum.org/EIPS/eip-7594
+    pub const fn new_pre_fusaka(sink: Box<dyn ReplaceableOrderSink>) -> Self {
+        fn pre_fusaka(tx: &TransactionSignedEcRecoveredWithBlobs) -> bool {
+            !tx.as_ref().is_eip4844() || tx.blobs_sidecar.is_eip4844()


this logic relies on 4844 validity condition that transactions cannot have 0 blobs

See:
https://eips.ethereum.org/EIPS/eip-4844#blob-transaction:~:text=Execution%20layer%20validation,-On

prestwich · 2025-12-30T15:36:18Z

crates/rbuilder-primitives/src/lib.rs

+/// Don't like the fact that blobs_sidecar exists no matter if
+/// [`Recovered<TransactionSigned>`] contains a non blob tx.
+///
+/// Great effort was put in avoiding simple access to the internal tx so we


is this comment accurate? there is an AsRef<Recovered<TransactionSigned>> implementation that allows direct access to the internal tx

prestwich added 2 commits December 30, 2025 09:54

chore: update docs, clean up code quality

2e7feaf

lint: clippy --all-targets

bc4a166

prestwich force-pushed the prestwich/docs-and-cleanup branch from 1ea2cbd to bc4a166 Compare December 30, 2025 14:54

prestwich marked this pull request as ready for review December 30, 2025 14:55

prestwich requested review from ZanCorDX and dvush as code owners December 30, 2025 14:55

Copilot AI review requested due to automatic review settings December 30, 2025 14:55

Copilot started reviewing on behalf of prestwich December 30, 2025 14:56 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

prestwich added 2 commits December 30, 2025 10:03

docs: improve filter documentation

2fad1e0

doc: update SinkSubscription docs

a7a303d

Copilot AI review requested due to automatic review settings December 30, 2025 15:26

nit: assume

fecdaae

Copilot started reviewing on behalf of prestwich December 30, 2025 15:26 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

fix: filter logic

ffa51ec

prestwich commented Dec 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor: fix docs, clean up code quality #843

Refactor: fix docs, clean up code quality #843

prestwich commented Dec 19, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

prestwich Dec 30, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

prestwich Dec 30, 2025

Uh oh!

prestwich Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	/// Push to pull for OrderSink. Just poll de UnboundedReceiver to get the
	/// Push to pull for OrderSink. Just poll the UnboundedReceiver to get the

	tx.as_ref().is_eip4844() && tx.blobs_sidecar.is_eip7594()
	!tx.as_ref().is_eip4844() \|\| tx.blobs_sidecar.is_eip7594()

	/// Since it's very unlikely what we have many wrong blobs we only filter on
	/// Since it's very unlikely that we have many wrong blobs, we only filter on

Refactor: fix docs, clean up code quality #843

Are you sure you want to change the base?

Refactor: fix docs, clean up code quality #843

Conversation

prestwich commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

✅ I have completed the following steps:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

prestwich Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

prestwich Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

prestwich Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

prestwich commented Dec 19, 2025 •

edited

Loading