-
Notifications
You must be signed in to change notification settings - Fork 169
Better bidding 2 #711
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
Better bidding 2 #711
Conversation
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.
Pull Request Overview
This PR implements a new bidding optimization feature called "Better bidding 2" that introduces a pipelined block finalization process to improve bidding response times. The main purpose is to enable blocks to be "prefinalized" first and then have payment transactions adjusted later, reducing the average bidding response time to around 0.5ms.
- Adds a new configuration option
adjust_finalized_blocks
to enable the pipelined finalization process - Implements prefinalized block processing where blocks are finalized in two steps: first finalization, then bid value adjustment
- Removes the optional coinbase payment mode and enforces transaction-based payments to validators
Reviewed Changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
docs/CONFIG.md | Updates documentation to remove coinbase_payment configuration option |
crates/rbuilder/src/utils/receipts.rs | Adds support for adjusting finalized blocks with caching optimizations |
crates/rbuilder/src/building/mod.rs | Removes optional coinbase payments, adds blob gas to BlockSpace, implements prefinalized block adjustment |
crates/rbuilder/src/live_builder/block_output/unfinished_block_processing.rs | Completely refactors block processing to support prefinalized blocks with separate threads |
crates/rbuilder/src/building/builders/block_building_helper.rs | Adds adjust_finalized_block method and implements prefinalized state management |
crates/rbuilder/src/telemetry/metrics/mod.rs | Adds new metric for tracking finalize adjust time |
Comments suppressed due to low confidence (2)
crates/rbuilder/src/building/mod.rs:1
- This hardcoded fallback to U256::ONE when the block value is non-zero seems like a magic number that could cause issues. Consider using a named constant or documenting why exactly 1 wei is used as the fallback value for contract compatibility.
use crate::{
crates/rbuilder/src/building/builders/block_building_helper.rs:1
- The magic number 2 should be better documented. Consider adding a comment explaining what these two state changes represent (payment transaction and requests processing) as mentioned in the comment above.
use alloy_primitives::{utils::format_ether, U256};
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
let mut i = 0; | ||
while i < finalized_blocks.len() { | ||
if finalized_blocks[i].block_id.0 < bid.block_id.0 { | ||
unused_blocks.push(finalized_blocks.remove(i)); | ||
continue; | ||
} | ||
if finalized_blocks[i].block_id == bid.block_id { | ||
found_block = Some(finalized_blocks[i].clone()); | ||
break; | ||
} | ||
i += 1; | ||
} |
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.
Removing elements from the middle of a Vec using remove(i)
is O(n) operation and combined with the loop this becomes O(n²). Consider using a more efficient approach like retain()
or collecting indices to remove and processing them in reverse order.
Copilot uses AI. Check for mistakes.
if adjust_finalized_block != self.prefinalize_state.is_some() { | ||
return Err(BlockBuildingHelperError::BlockFinalizedIncorrectly); | ||
} |
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.
The validation logic is inverted and confusing. When adjust_finalized_block
is true, we expect prefinalize_state
to be Some, but the condition checks if they're different. Consider renaming the error or restructuring the logic to make the expected state more clear.
Copilot uses AI. Check for mistakes.
crates/rbuilder/src/live_builder/block_output/unfinished_block_processing.rs
Outdated
Show resolved
Hide resolved
e4370c1
to
58be740
Compare
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.
Pull Request Overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
crates/rbuilder/src/building/order_commit.rs:747
- The field name
space_used
inTransactionExecutionInfo
is inconsistent with the old field namegas_used
that was replaced. This breaks the existing API contract and could cause confusion. Consider keeping both fields for backward compatibility or updating all call sites consistently.
space_used: BlockSpace,
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
crates/rbuilder/src/live_builder/block_output/unfinished_block_processing.rs
Outdated
Show resolved
Hide resolved
crates/rbuilder/src/live_builder/block_output/unfinished_block_processing.rs
Show resolved
Hide resolved
67a21bf
to
1026c04
Compare
📝 Summary
Adds a new way to seal blocks to enable it set
adjust_finalized_blocks = true
in the config.If set to true blocks will be processed in two steps:
The advantage is that bidding response becomes faster (it takes around 0.5ms on average to adjust finalized block).
In addition this PR:
💡 Motivation and Context
✅ I have completed the following steps:
make lint
make test