-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: implement Optimism builder DA limits #13757
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
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
f30a655
WIP DA config implementation
meyer9 4e9ce2b
Implement block DA limit for Optimism
meyer9 7c4ec40
clean up implementation
meyer9 b12360e
Remove unused fn
meyer9 127d941
Fix lint
meyer9 663b72e
Remove max block da size from txpool
meyer9 450adee
Move everything into optimism package
meyer9 9ec53d9
Clean up extra changes
meyer9 ccc5483
add todo comment
meyer9 cb54c82
Add revm optimism dependency
meyer9 b3edb4c
Memoize da usage value
meyer9 0dd7104
Add serde skip
meyer9 3c8f822
Use oncelock instead of property
meyer9 a4a8b0b
Improve tx da calculation
meyer9 a186581
Address PR comments
meyer9 ab3f07f
Remove extra dep
meyer9 d43606f
Merge branch 'main' into meyer9/txpool-da-config
mattsse 97281ed
docs
mattsse File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,14 @@ | ||
| //! Optimism payload builder implementation. | ||
|
|
||
| use crate::{ | ||
| config::OpBuilderConfig, | ||
| config::{OpBuilderConfig, OpDAConfig}, | ||
| error::OpPayloadBuilderError, | ||
| payload::{OpBuiltPayload, OpPayloadBuilderAttributes}, | ||
| }; | ||
| use alloy_consensus::{Eip658Value, Header, Transaction, Typed2718, EMPTY_OMMER_ROOT_HASH}; | ||
| use alloy_eips::{eip4895::Withdrawals, merge::BEACON_NONCE}; | ||
| use alloy_primitives::{Address, Bytes, B256, U256}; | ||
| use alloy_rlp::Encodable; | ||
| use alloy_rpc_types_debug::ExecutionWitness; | ||
| use alloy_rpc_types_engine::PayloadId; | ||
| use op_alloy_consensus::{OpDepositReceipt, OpTxType}; | ||
|
|
@@ -132,6 +133,7 @@ where | |
|
|
||
| let ctx = OpPayloadBuilderCtx { | ||
| evm_config: self.evm_config.clone(), | ||
| da_config: self.config.da_config.clone(), | ||
| chain_spec: client.chain_spec(), | ||
| config, | ||
| evm_env, | ||
|
|
@@ -193,6 +195,7 @@ where | |
| let config = PayloadConfig { parent_header: Arc::new(parent), attributes }; | ||
| let ctx = OpPayloadBuilderCtx { | ||
| evm_config: self.evm_config.clone(), | ||
| da_config: self.config.da_config.clone(), | ||
| chain_spec: client.chain_spec(), | ||
| config, | ||
| evm_env, | ||
|
|
@@ -527,6 +530,8 @@ pub struct ExecutionInfo { | |
| pub receipts: Vec<OpReceipt>, | ||
| /// All gas used so far | ||
| pub cumulative_gas_used: u64, | ||
| /// Estimated DA size | ||
| pub cumulative_da_bytes_used: u64, | ||
| /// Tracks fees from executed mempool transactions | ||
| pub total_fees: U256, | ||
| } | ||
|
|
@@ -539,16 +544,45 @@ impl ExecutionInfo { | |
| executed_senders: Vec::with_capacity(capacity), | ||
| receipts: Vec::with_capacity(capacity), | ||
| cumulative_gas_used: 0, | ||
| cumulative_da_bytes_used: 0, | ||
| total_fees: U256::ZERO, | ||
| } | ||
| } | ||
|
|
||
| /// Returns true if the transaction would exceed the block limits: | ||
| /// - block gas limit: ensures the transaction still fits into the block. | ||
| /// - tx DA limit: if configured, ensures the tx does not exceed the maximum allowed DA limit | ||
| /// per tx. | ||
| /// - block DA limit: if configured, ensures the transaction's DA size does not exceed the | ||
| /// maximum allowed DA limit per block. | ||
| pub fn is_tx_over_limits( | ||
| &self, | ||
| tx: &OpTransactionSigned, | ||
| block_gas_limit: u64, | ||
| tx_data_limit: Option<u64>, | ||
| block_data_limit: Option<u64>, | ||
| ) -> bool { | ||
| if tx_data_limit.is_some_and(|da_limit| tx.length() as u64 > da_limit) { | ||
| return true; | ||
| } | ||
|
|
||
| if block_data_limit | ||
| .is_some_and(|da_limit| self.cumulative_da_bytes_used + (tx.length() as u64) > da_limit) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| self.cumulative_gas_used + tx.gas_limit() > block_gas_limit | ||
| } | ||
| } | ||
|
|
||
| /// Container type that holds all necessities to build a new payload. | ||
| #[derive(Debug)] | ||
| pub struct OpPayloadBuilderCtx<EvmConfig> { | ||
| /// The type that knows how to perform system calls and configure the evm. | ||
| pub evm_config: EvmConfig, | ||
| /// The DA config for the payload builder | ||
| pub da_config: OpDAConfig, | ||
|
||
| /// The chainspec | ||
| pub chain_spec: Arc<OpChainSpec>, | ||
| /// How to build the payload. | ||
|
|
@@ -848,13 +882,14 @@ where | |
| DB: Database<Error = ProviderError>, | ||
| { | ||
| let block_gas_limit = self.block_gas_limit(); | ||
| let block_da_limit = self.da_config.max_da_block_size(); | ||
| let tx_da_limit = self.da_config.max_da_tx_size(); | ||
| let base_fee = self.base_fee(); | ||
|
|
||
| let mut evm = self.evm_config.evm_with_env(&mut *db, self.evm_env.clone()); | ||
|
|
||
| while let Some(tx) = best_txs.next(()) { | ||
| // ensure we still have capacity for this transaction | ||
| if info.cumulative_gas_used + tx.gas_limit() > block_gas_limit { | ||
| if info.is_tx_over_limits(tx.tx(), block_gas_limit, tx_da_limit, block_da_limit) { | ||
| // we can't fit this transaction into the block, so we need to mark it as | ||
| // invalid which also removes all dependent transaction from | ||
| // the iterator before we can continue | ||
|
|
@@ -909,6 +944,7 @@ where | |
| // add gas used by the transaction to cumulative gas used, before creating the | ||
| // receipt | ||
| info.cumulative_gas_used += gas_used; | ||
| info.cumulative_da_bytes_used += tx.length() as u64; | ||
|
|
||
| let receipt = alloy_consensus::Receipt { | ||
| status: Eip658Value::Eip658(result.is_success()), | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this now uses the length instead of the flz compressed length
should we change this here? or is this fine for this pr?
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.
ah sorry, I may have misunderstood. I think it's OK to use uncompressed size temporarily until we implement DA cost tracking. I thought the comment above meant to remove the compressed size calculation for now, but I could add it back in without modifying the pooled tx type.
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 wrote an example implementation of this here, but I'm a little concerned about changing revm versions to a Git hash: dbe1610
If this looks OK, I can add it to this PR, but if this isn't urgent, we can just implement after the issue you created.
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.
meh that's unfortunate that this wasn't included in the latest revm release...
bluealloy/revm#1985