New witness calculation and DA config#436
Merged
SozinM merged 4 commits intoFeb 21, 2025
Merged
Conversation
Add DA config to builder Account for builder_tx in block_da_size.
3 tasks
ferranbt
approved these changes
Feb 19, 2025
avalonche
reviewed
Feb 20, 2025
| // Check that it's possible to create builder tx, considering max_da_tx_size, otherwise panic | ||
| if let Some(tx_da_limit) = ctx.da_config.max_da_tx_size() { | ||
| // Panic indicate max_da_tx_size misconfiguration | ||
| assert!(tx_da_limit >= builder_tx_da_size as u64); |
Contributor
There was a problem hiding this comment.
is this copied from the payload builder in reth? wondering if there should be an error log as well here
Contributor
Author
There was a problem hiding this comment.
No, this little piece is mine
I'll add a message, thank you!
avalonche
reviewed
Feb 20, 2025
Comment on lines
+1302
to
+1329
| self.builder_signer() | ||
| .map(|signer| { | ||
| let base_fee = self.base_fee(); | ||
| // Create message with block number for the builder to sign | ||
| let nonce = db | ||
| .load_cache_account(signer.address) | ||
| .map(|acc| acc.account_info().unwrap_or_default().nonce) | ||
| .map_err(|_| { | ||
| PayloadBuilderError::other(OpPayloadBuilderError::AccountLoadFailed( | ||
| signer.address, | ||
| )) | ||
| })?; | ||
|
|
||
| // Create the EIP-1559 transaction | ||
| let eip1559 = OpTypedTransaction::Eip1559(TxEip1559 { | ||
| chain_id: self.chain_id(), | ||
| nonce, | ||
| gas_limit: builder_tx_gas, | ||
| max_fee_per_gas: base_fee.into(), | ||
| max_priority_fee_per_gas: 0, | ||
| to: TxKind::Call(Address::ZERO), | ||
| // Include the message as part of the transaction data | ||
| input: message.into(), | ||
| ..Default::default() | ||
| }); | ||
| let tx = eip1559; | ||
| // Sign the transaction | ||
| let builder_tx = signer.sign_tx(tx).map_err(PayloadBuilderError::other)?; |
Contributor
There was a problem hiding this comment.
duplicated code here, can encapsulate into a get_signed_builder_tx function
added 3 commits
February 20, 2025 16:07
Move builder tx creation to dedicated function
Contributor
Author
|
Considering it got approves from @ferranbt and @avalonche (in PM) i'm merging this |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
📝 Summary
New witness calculation algo.
DA config.
Reserve space for builder_tx in block_da_size.
Note:
If da_config.max_da_tx_size is too small, execute function of OpBuilder will panic, because it's improssible to insert builder transaction.
estimate_builder_tx_da_size may be a little bit heavy, maybe we could precompute tx size for builder_tx and reuse it
💡 Motivation and Context
✅ I have completed the following steps:
make lintmake test