feat(vm): [DONT MERGE] 4844 POC #1056
Conversation
Detected VM performance changes
|
| @@ -8,7 +8,7 @@ pub const ETHEREUM_ADDRESS: Address = Address::zero(); | |||
| /// The maximum number of pubdata per L1 batch. This limit is due to the fact that the Ethereum | |||
| /// nodes do not accept transactions that have more than 128kb of pubdata. | |||
| /// The 18kb margin is left in case of any impreciseness of the pubdata calculation. | |||
| pub const MAX_PUBDATA_PER_L1_BATCH: u64 = 110000; | |||
| pub const MAX_PUBDATA_PER_L1_BATCH: u64 = 220000; | |||
There was a problem hiding this comment.
Please update the comment above
| i += 1; | ||
| } | ||
|
|
||
| output / denominator |
There was a problem hiding this comment.
I vaguely remember this being discussed - this is temporary right? - please add comments on the migration strategy
| /// The amount of ergs to be reserved at the end of the batch to ensure that it has enough ergs to verify compression, etc. | ||
| pub(crate) const BOOTLOADER_BATCH_TIP_OVERHEAD: u32 = 80_000_000; | ||
| pub(crate) const BOOTLOADER_BATCH_TIP_OVERHEAD_GAS: u32 = 200_000_000; |
There was a problem hiding this comment.
can you please explain why we need this change
| @@ -26,7 +26,7 @@ pub struct SystemL2ToL1Log(pub L2ToL1Log); | |||
| impl L2ToL1Log { | |||
| /// Determines the minimum number of items in the Merkle tree built from L2-to-L1 logs | |||
| /// for a certain batch. | |||
| pub const MIN_L2_L1_LOGS_TREE_SIZE: usize = 2048; | |||
| pub const MIN_L2_L1_LOGS_TREE_SIZE: usize = 4096; | |||
There was a problem hiding this comment.
can you please explain why th this change is needed
| .into_iter() | ||
| .take(MAX_NUMBER_OF_BATCH_COMMITS) | ||
| .collect(), | ||
| _ => unpublished_l1_batches |
There was a problem hiding this comment.
hmmmbut why is it done here and this way? We have seal criteria NumberCriterion that is already initialized with different constants for different l1 ops. That is, different l1 operations already have different associated sealed criteria, so it's covered by last_l1_batch_to_publish.
There was a problem hiding this comment.
ah looking further in code seems like it's temporary. But the PR is not marked as draft 😇
| /// | ||
| /// A commit operation may be either a blob transaction or a calldata | ||
| /// transaction. In case of a blob transaction the second field of | ||
| /// the returned tuple is `Some` and contains the encoded blob sidecar. |
There was a problem hiding this comment.
I think this comment is not updated
|
|
||
| let kzg_settings = load_kzg_settings(); | ||
|
|
||
| let side_car = op.l1_batches[0] |
There was a problem hiding this comment.
I understand this is temporary?
| @@ -414,6 +431,19 @@ impl EthTxManager { | |||
| opt.max_fee_per_gas = Some(U256::from(base_fee_per_gas + priority_fee_per_gas)); | |||
| opt.max_priority_fee_per_gas = Some(U256::from(priority_fee_per_gas)); | |||
| opt.nonce = Some(tx.nonce.0.into()); | |||
| opt.transaction_type = if tx.blob_sidecar.is_some() { | |||
| opt.max_fee_per_blob_gas = blob_gas_price; | |||
There was a problem hiding this comment.
hairsplitting: I feel like this is a rather convoluted way to assign fields.. Perhaps just match tx.blob_sidecar and assign variables that you need to assign in match arms
Detected VM performance changes
|
|
#1056 (comment) Is it true? @koloz193 cc @joonazan |
@RomanBrodetski hopefully this is not the case and was just a product of the POC code |
#1254) ## What ❔ Splits the part off of #1056 ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zk fmt` and `zk lint`. - [ ] Spellcheck has been run via `zk spellcheck`. - [ ] Linkcheck has been run via `zk linkcheck`.
|
@koloz193 given that it's a PoC, can this PR be closed now? |
Yes, I’ll close it out |
What ❔
Why ❔
Checklist
zk fmtandzk lint.zk spellcheck.zk linkcheck.