refactor(l2): add check for transaction state diff size #2616
Conversation
Lines of code reportTotal lines added: Detailed view |
tomip01
changed the title
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
crates/vm/backends/levm/l2_utils.rs
Outdated
| if is_withdrawal_l2(tx, receipt)? { | ||
| actual_size += L2_WITHDRAWAL_SIZE; | ||
| } | ||
| if is_deposit_l2(tx) { | ||
| actual_size += L2_DEPOSIT_SIZE; | ||
| } |
There was a problem hiding this comment.
I wonder why we do these checks. Aren't these modifications just contemplated in the calculated account_updates?
There was a problem hiding this comment.
Aren't these modifications just contemplated in the calculated account_updates?
No. From the list of account_updates we can't tell wheter a change in the balance was due to a deposit, a withdrawal or a simple transfer. We can only determine that by examining the transaction.
There was a problem hiding this comment.
Oh ok. I thought that the L2_DEPOSIT_SIZE and L2_WITHDRAWAL_SIZE were actually measuring changes performed to accounts that were reflected in account_updates but they are measuring something else. My bad. Thanks
| Err(ChainError::EvmError(ethrex_vm::EvmError::StateDiffSizeError)) => { | ||
| debug!( | ||
| "Skipping transaction: {}, doesn't fit in blob_size", | ||
| tx_hash | ||
| ); |
There was a problem hiding this comment.
How are we recovering the previous state? Are the account_updates of this tx being excluded?
There was a problem hiding this comment.
Nice catch! corrected here e5ea196
| @@ -172,6 +172,7 @@ pub struct PayloadBuildContext { | |||
| pub store: Store, | |||
| pub vm: Evm, | |||
| pub account_updates: Vec<AccountUpdate>, | |||
| pub acc_state_diff_size: Option<usize>, | |||
There was a problem hiding this comment.
I think we should discuss this, we don't want our L1 to have L2 stuff. Can't we have a vm.execute_tx_l2?
There was a problem hiding this comment.
Yeah you are probably right. We have to think what's the best choice we have so that we don't mix L2 fields with the L1.
There was a problem hiding this comment.
I second the idea of having an execute_tx_l2 type function to not interfere on the rest of the code
crates/vm/constants.rs
Outdated
|
|
||
| // transactions_root(H256) + receipts_root(H256) + parent_hash(H256) + gas_limit(u64) + gas_used(u64) + timestamp(u64) | ||
| // block_number(u64) + base_fee_per_gas(u64) | ||
| // 32bytes + 32bytes + 32bytes + 8bytes + 8bytes + 8bytes + 8bytes + 8bytes | ||
| pub const LAST_HEADER_FIELDS_SIZE: usize = 136; | ||
|
|
||
| // address(H160) + amount(U256) + tx_hash(H256). | ||
| // 20bytes + 32bytes + 32bytes. | ||
| pub const L2_WITHDRAWAL_SIZE: usize = 84; | ||
|
|
||
| // address(H160) + amount(U256). | ||
| // 20bytes + 32bytes | ||
| pub const L2_DEPOSIT_SIZE: usize = 52; | ||
|
|
||
| pub static COMMON_BRIDGE_L2_ADDRESS: LazyLock<Address> = LazyLock::new(|| { | ||
| Address::from_slice(&[ | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0xff, 0xff, | ||
| ]) | ||
| }); |
There was a problem hiding this comment.
Added this comment to #2655.
There was a problem hiding this comment.
Nice! I added the link to the issue for better clarity
5ee1e62
tomip01
force-pushed
the
l2/limit-state-diff
branch
from
23be6f8 to
e5ea196
Compare
| // We want to restore to the initial state, this includes reverting the changes made by the prepare execution | ||
| // and the changes made by the execution itself. | ||
| #[cfg(feature = "l2")] | ||
| { | ||
| let current_backup: &mut CallFrameBackup = | ||
| &mut self.current_call_frame_mut()?.call_frame_backup; | ||
| merge_callframe_backup(current_backup, callframe_backup); | ||
| } |
There was a problem hiding this comment.
I used the feature flag option instead of creating a new function execute_l2() because the changes are small and does not change the API. Should I go for the new function instead?
| .ok_or(VMError::OutOfBounds)? | ||
| .call_frame_backup; | ||
|
|
||
| if let Err(e) = update_state_diff_size(acc_state_diff_size, tx, &report.logs, db) { |
There was a problem hiding this comment.
I believe you should be able to call restore_cache inside the if statement instead of doing it manually
|
Closing because this turned out to be huge and can (and will) be split into 2 or more PRs. |
Motivation
This PR exists to avoid cloning the context and the execution cache if the transaction exceeds the state diff size allowed for that block.
Description
l2_utilsfile for that verification.StateDiffSizeError.Comparison against main
How to replicate:
Inside
crates/l2init-l1make deploy-l1 update-system-contracts init-l2cargo run --manifest-path ../../cmd/load_test/Cargo.toml -- -k ../../test_data/private_keys.txt -t erc20 -N 1000 -n http://localhost:1729For Terminal 3 if necessary run
ulimit -n 65536before the command.Comparison at 255 seconds (arbitrary timestamp):
main:
Waiting for transactions to be included from 0000bd19f707ca481886244bdd20bd6b8a81bd3e. Nonce: 69. Needs: 1000. Percentage: 6.9%. Elapsed time: 255s.this PR:
Waiting for transactions to be included from 0000bd19f707ca481886244bdd20bd6b8a81bd3e. Nonce: 567. Needs: 1000. Percentage: 56.699999999999996%. Elapsed time: 255s.The performance increase in this particular scenario is around 8x.
Closes #2413