From 975444d4b1207373ac31c576784e5dad1738f4dc Mon Sep 17 00:00:00 2001 From: Ivan Litteri <67517699+ilitteri@users.noreply.github.com> Date: Wed, 4 Dec 2024 09:11:25 -0300 Subject: [PATCH 1/4] fix(levm): storage modification gas cost was off (#1400) --- crates/vm/levm/src/gas_cost.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/vm/levm/src/gas_cost.rs b/crates/vm/levm/src/gas_cost.rs index 0830c3f5d6..462fbdee63 100644 --- a/crates/vm/levm/src/gas_cost.rs +++ b/crates/vm/levm/src/gas_cost.rs @@ -99,7 +99,7 @@ pub const SSTORE_STATIC: U256 = U256::zero(); pub const SSTORE_COLD_DYNAMIC: U256 = U256([2100, 0, 0, 0]); pub const SSTORE_DEFAULT_DYNAMIC: U256 = U256([100, 0, 0, 0]); pub const SSTORE_STORAGE_CREATION: U256 = U256([20000, 0, 0, 0]); -pub const SSTORE_STORAGE_MODIFICATION: U256 = U256([5000, 0, 0, 0]); +pub const SSTORE_STORAGE_MODIFICATION: U256 = U256([2900, 0, 0, 0]); pub const BALANCE_STATIC: U256 = DEFAULT_STATIC; pub const BALANCE_COLD_DYNAMIC: U256 = DEFAULT_COLD_DYNAMIC; From b332a50e45d21073546f9bd3f4f80f53425dc55f Mon Sep 17 00:00:00 2001 From: Federico Borello <156438142+fborello-lambda@users.noreply.github.com> Date: Wed, 4 Dec 2024 12:07:37 -0300 Subject: [PATCH 2/4] feat(l2): standalone test target (#1401) **Motivation** We may want to simplify the L2 testing process. **Description** - Add one target to test the stack with the CI and another to test it locally. - Add a target that updates the CLI's contracts. --- .github/workflows/ci_l2.yaml | 2 +- crates/l2/Makefile | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci_l2.yaml b/.github/workflows/ci_l2.yaml index fb79774efe..71da80d4fa 100644 --- a/.github/workflows/ci_l2.yaml +++ b/.github/workflows/ci_l2.yaml @@ -35,4 +35,4 @@ jobs: run: | cd crates/l2 cp .env.example .env - make test + make ci_test diff --git a/crates/l2/Makefile b/crates/l2/Makefile index 5f4eb92694..344157131e 100644 --- a/crates/l2/Makefile +++ b/crates/l2/Makefile @@ -1,6 +1,6 @@ .DEFAULT_GOAL := help -.PHONY: help init down clean init-local-l1 down-local-l1 clean-local-l1 init-l2 down-l2 deploy-l1 deploy-block-executor deploy-inbox setup-prover test +.PHONY: help init down clean init-local-l1 down-local-l1 clean-local-l1 init-l2 down-l2 deploy-l1 deploy-block-executor deploy-inbox setup-prover test ci_test update-cli-contracts L2_GENESIS_FILE_PATH=../../test_data/genesis-l2.json L1_GENESIS_FILE_PATH=../../test_data/genesis-l1.json @@ -95,11 +95,23 @@ init-l2-prover-gpu: ## πŸš€ Initializes the Prover with GPU support rm_db_l2: ## πŸ›‘ Removes the DB used by the L2 cargo run --release --manifest-path ../../Cargo.toml --bin ethrex -- removedb --datadir ${ethrex_L2_DEV_LIBMDBX} +update-cli-contracts: ## πŸ“œ Update the CLI's config contracts + @if [ -z "$$C" ]; then \ + echo "Error: CONFIG_NAME (C) is missing.\nPlease provide it as an argument:\nmake update-cli-contracts C=."; \ + exit 1; \ + fi && \ + CB=$$(grep 'L1_WATCHER_BRIDGE_ADDRESS' .env | cut -d= -f2) && \ + ethrex_l2 config edit --common-bridge $$CB $$C && \ + OP=$$(grep 'COMMITTER_ON_CHAIN_PROPOSER_ADDRESS' .env | cut -d= -f2) && \ + ethrex_l2 config edit --on-chain-proposer $$OP $$C -# CI Testing +# Testing -test: ## 🚧 Runs the L2's integration test +ci_test: ## 🚧 Runs the L2's integration test, used by the github's CI docker compose -f ${ethrex_L2_DOCKER_COMPOSE_PATH} down docker compose -f ${ethrex_L2_DOCKER_COMPOSE_PATH} up -d --build BRIDGE_ADDRESS=$$(grep 'L1_WATCHER_BRIDGE_ADDRESS' .env | cut -d= -f2) ON_CHAIN_PROPOSER_ADDRESS=$$(grep 'COMMITTER_ON_CHAIN_PROPOSER_ADDRESS' .env | cut -d= -f2) cargo test --release testito -- --nocapture docker compose -f ${ethrex_L2_DOCKER_COMPOSE_PATH} down + +test: ## 🚧 Runs the L2's integration test, run `make init` and in a new terminal make test + BRIDGE_ADDRESS=$$(grep 'L1_WATCHER_BRIDGE_ADDRESS' .env | cut -d= -f2) ON_CHAIN_PROPOSER_ADDRESS=$$(grep 'COMMITTER_ON_CHAIN_PROPOSER_ADDRESS' .env | cut -d= -f2) cargo test --release testito -- --nocapture From 1a18e615b22b8e98dc12c947b0ff764b3bb08d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jerem=C3=ADas=20Salom=C3=B3n?= <48994069+JereSalo@users.noreply.github.com> Date: Wed, 4 Dec 2024 12:57:40 -0300 Subject: [PATCH 3/4] feat(levm): improve LEVM EFTests (skip tests, disable spinner flag, execute individual tests) (#1394) **Motivation** - Running tests is pretty slow but the bottleneck stands in a few tests only, this functionality will allow the CI to run in less time and for a person to execute most of the tests quickly. **Description** - It skips `loopMul.json` arbitrarily because it is the test that causes the biggest bottleneck. It is hardcoded just because if the person that's testing forgets to skip that test, they are going to regret it. - It allows the user to make use of the flag `--skip` to skip tests and directories. - It allows the user to send multiple tests to both flags --test and --skip like this: ... --skip folder1,test1,folder2... **Additional Features** - It adds flag flag disable-spinner, if set, the main spinners will stop and it will replace the "spinner prints" for `println!`, useful for CI. (not yet) - Adds functionality to send individual tests to `--test ` instead of only folders. Useful for fixing tests **CI** - Improves readability and speed of ci, now it is run with `make run-evm-ef-tests-ci` **Example Usage** `cargo test -p ef_tests-levm --test ef_tests_levm -- --tests intrinsic.json,add11.json,stChainId --skip chainId.json --disable-spinner` This command runst tests `intrinsic.json`, `add11.json` and the ones in the folder `stChainId`. But this also skips the test `chainId.json`, this one is inside `stChainId` and therefore only the rest of the tests in that folder will execute. --------- Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com> --- .github/workflows/ci_levm.yaml | 2 +- cmd/ef_tests/levm/parser.rs | 80 ++++++++++++++++----- cmd/ef_tests/levm/runner/mod.rs | 123 ++++++++++++++++++++++++-------- cmd/ef_tests/levm/utils.rs | 17 +++++ crates/vm/levm/Makefile | 4 ++ 5 files changed, 179 insertions(+), 47 deletions(-) diff --git a/.github/workflows/ci_levm.yaml b/.github/workflows/ci_levm.yaml index 172c625f32..21e2f0cda3 100644 --- a/.github/workflows/ci_levm.yaml +++ b/.github/workflows/ci_levm.yaml @@ -41,7 +41,7 @@ jobs: - name: Run tests run: | cd crates/vm/levm - make run-evm-ef-tests + make run-evm-ef-tests-ci test: # "Test" is a required check, don't change the name name: Test diff --git a/cmd/ef_tests/levm/parser.rs b/cmd/ef_tests/levm/parser.rs index 302d105453..01ce9bfd2b 100644 --- a/cmd/ef_tests/levm/parser.rs +++ b/cmd/ef_tests/levm/parser.rs @@ -2,6 +2,7 @@ use crate::{ report::format_duration_as_mm_ss, runner::EFTestRunnerOptions, types::{EFTest, EFTests}, + utils::{spinner_success_or_print, spinner_update_text_or_print}, }; use colored::Colorize; use spinoff::{spinners::Dots, Color, Spinner}; @@ -24,6 +25,9 @@ pub fn parse_ef_tests(opts: &EFTestRunnerOptions) -> Result, EFTestP let cargo_manifest_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); let ef_general_state_tests_path = cargo_manifest_dir.join("vectors/GeneralStateTests"); let mut spinner = Spinner::new(Dots, "Parsing EF Tests".bold().to_string(), Color::Cyan); + if opts.disable_spinner { + spinner.stop(); + } let mut tests = Vec::new(); for test_dir in std::fs::read_dir(ef_general_state_tests_path.clone()) .map_err(|err| { @@ -37,12 +41,13 @@ pub fn parse_ef_tests(opts: &EFTestRunnerOptions) -> Result, EFTestP let directory_tests = parse_ef_test_dir(test_dir, opts, &mut spinner)?; tests.extend(directory_tests); } - spinner.success( - &format!( + spinner_success_or_print( + &mut spinner, + format!( "Parsed EF Tests in {}", format_duration_as_mm_ss(parsing_time.elapsed()) - ) - .bold(), + ), + opts.disable_spinner, ); Ok(tests) } @@ -52,7 +57,11 @@ pub fn parse_ef_test_dir( opts: &EFTestRunnerOptions, directory_parsing_spinner: &mut Spinner, ) -> Result, EFTestParseError> { - directory_parsing_spinner.update_text(format!("Parsing directory {:?}", test_dir.file_name())); + spinner_update_text_or_print( + directory_parsing_spinner, + format!("Parsing directory {:?}", test_dir.file_name()), + opts.disable_spinner, + ); let mut directory_tests = Vec::new(); for test in std::fs::read_dir(test_dir.path()) @@ -78,31 +87,68 @@ pub fn parse_ef_test_dir( { continue; } - // Skip the ValueOverflowParis.json file. + // Skip the ValueOverflowParis.json file because of errors, and loopMul.json because it takes too long to run. if test .path() .file_name() - .is_some_and(|name| name == "ValueOverflowParis.json") + .is_some_and(|name| name == "ValueOverflowParis.json" || name == "loopMul.json") { continue; } - // Skip tests that are not in the list of tests to run. - if &test_dir.file_name().to_str().unwrap().to_owned() == "vmPerformance" { - return Ok(Vec::new()); - } - // Skip tests that are not in the list of tests to run. if !opts.tests.is_empty() && !opts .tests .contains(&test_dir.file_name().to_str().unwrap().to_owned()) + && !opts.tests.contains( + &test + .path() + .file_name() + .unwrap() + .to_str() + .unwrap() + .to_owned(), + ) { - directory_parsing_spinner.update_text(format!( - "Skipping test {:?} as it is not in the list of tests to run", - test.path().file_name() - )); - return Ok(Vec::new()); + continue; + } + + // Skips all tests in a particular directory. + if opts + .skip + .contains(&test_dir.file_name().to_str().unwrap().to_owned()) + { + spinner_update_text_or_print( + directory_parsing_spinner, + format!( + "Skipping test {:?} as it is in the folder of tests to skip", + test.path().file_name().unwrap() + ), + opts.disable_spinner, + ); + continue; + } + + // Skip tests by name (with .json extension) + if opts.skip.contains( + &test + .path() + .file_name() + .unwrap() + .to_str() + .unwrap() + .to_owned(), + ) { + spinner_update_text_or_print( + directory_parsing_spinner, + format!( + "Skipping test {:?} as it is in the list of tests to skip", + test.path().file_name().unwrap() + ), + opts.disable_spinner, + ); + continue; } let test_file = std::fs::File::open(test.path()).map_err(|err| { diff --git a/cmd/ef_tests/levm/runner/mod.rs b/cmd/ef_tests/levm/runner/mod.rs index 72aca963b9..eccbecd332 100644 --- a/cmd/ef_tests/levm/runner/mod.rs +++ b/cmd/ef_tests/levm/runner/mod.rs @@ -1,6 +1,7 @@ use crate::{ report::{self, format_duration_as_mm_ss, EFTestReport, TestReRunReport}, types::EFTest, + utils::{spinner_success_or_print, spinner_update_text_or_print}, }; use clap::Parser; use colored::Colorize; @@ -42,10 +43,14 @@ pub enum InternalError { pub struct EFTestRunnerOptions { #[arg(short, long, value_name = "FORK", default_value = "Cancun")] pub fork: Vec, - #[arg(short, long, value_name = "TESTS")] + #[arg(short, long, value_name = "TESTS", use_value_delimiter = true)] pub tests: Vec, #[arg(short, long, value_name = "SUMMARY", default_value = "false")] pub summary: bool, + #[arg(long, value_name = "SKIP", use_value_delimiter = true)] + pub skip: Vec, + #[arg(long, value_name = "DISABLE_SPINNER", default_value = "false")] + pub disable_spinner: bool, // Replaces spinner for normal prints. } pub fn run_ef_tests( @@ -59,7 +64,7 @@ pub fn run_ef_tests( if opts.summary { return Ok(()); } - re_run_with_revm(&mut reports, &ef_tests)?; + re_run_with_revm(&mut reports, &ef_tests, opts)?; write_report(&reports) } @@ -74,7 +79,13 @@ fn run_with_levm( report::progress(reports, levm_run_time.elapsed()), Color::Cyan, ); + if opts.disable_spinner { + levm_run_spinner.stop(); + } for test in ef_tests.iter() { + if opts.disable_spinner { + println!("Running test: {:?}", test.name); + } let ef_test_report = match levm_runner::run_ef_test(test) { Ok(ef_test_report) => ef_test_report, Err(EFTestRunnerError::Internal(err)) => return Err(EFTestRunnerError::Internal(err)), @@ -85,9 +96,17 @@ fn run_with_levm( } }; reports.push(ef_test_report); - levm_run_spinner.update_text(report::progress(reports, levm_run_time.elapsed())); + spinner_update_text_or_print( + &mut levm_run_spinner, + report::progress(reports, levm_run_time.elapsed()), + opts.disable_spinner, + ); } - levm_run_spinner.success(&report::progress(reports, levm_run_time.elapsed())); + spinner_success_or_print( + &mut levm_run_spinner, + report::progress(reports, levm_run_time.elapsed()), + opts.disable_spinner, + ); if opts.summary { report::write_summary_for_slack(reports)?; @@ -95,7 +114,15 @@ fn run_with_levm( } let mut summary_spinner = Spinner::new(Dots, "Loading summary...".to_owned(), Color::Cyan); - summary_spinner.success(&report::summary_for_shell(reports)); + if opts.disable_spinner { + summary_spinner.stop(); + } + spinner_success_or_print( + &mut summary_spinner, + report::summary_for_shell(reports), + opts.disable_spinner, + ); + Ok(()) } @@ -104,6 +131,7 @@ fn run_with_levm( fn _run_with_revm( reports: &mut Vec, ef_tests: &[EFTest], + opts: &EFTestRunnerOptions, ) -> Result<(), EFTestRunnerError> { let revm_run_time = std::time::Instant::now(); let mut revm_run_spinner = Spinner::new( @@ -111,14 +139,24 @@ fn _run_with_revm( "Running all tests with REVM...".to_owned(), Color::Cyan, ); + if opts.disable_spinner { + revm_run_spinner.stop(); + } for (idx, test) in ef_tests.iter().enumerate() { + if opts.disable_spinner { + println!("Running test: {:?}", test.name); + } let total_tests = ef_tests.len(); - revm_run_spinner.update_text(format!( - "{} {}/{total_tests} - {}", - "Running all tests with REVM".bold(), - idx + 1, - format_duration_as_mm_ss(revm_run_time.elapsed()) - )); + spinner_update_text_or_print( + &mut revm_run_spinner, + format!( + "{} {}/{total_tests} - {}", + "Running all tests with REVM".bold(), + idx + 1, + format_duration_as_mm_ss(revm_run_time.elapsed()) + ), + opts.disable_spinner, + ); let ef_test_report = match revm_runner::_run_ef_test_revm(test) { Ok(ef_test_report) => ef_test_report, Err(EFTestRunnerError::Internal(err)) => return Err(EFTestRunnerError::Internal(err)), @@ -129,18 +167,27 @@ fn _run_with_revm( } }; reports.push(ef_test_report); - revm_run_spinner.update_text(report::progress(reports, revm_run_time.elapsed())); + spinner_update_text_or_print( + &mut revm_run_spinner, + report::progress(reports, revm_run_time.elapsed()), + opts.disable_spinner, + ); } - revm_run_spinner.success(&format!( - "Ran all tests with REVM in {}", - format_duration_as_mm_ss(revm_run_time.elapsed()) - )); + spinner_success_or_print( + &mut revm_run_spinner, + format!( + "Ran all tests with REVM in {}", + format_duration_as_mm_ss(revm_run_time.elapsed()) + ), + opts.disable_spinner, + ); Ok(()) } fn re_run_with_revm( reports: &mut [EFTestReport], ef_tests: &[EFTest], + opts: &EFTestRunnerOptions, ) -> Result<(), EFTestRunnerError> { let revm_run_time = std::time::Instant::now(); let mut revm_run_spinner = Spinner::new( @@ -148,17 +195,31 @@ fn re_run_with_revm( "Running failed tests with REVM...".to_owned(), Color::Cyan, ); + if opts.disable_spinner { + revm_run_spinner.stop(); + } let failed_tests = reports.iter().filter(|report| !report.passed()).count(); - for (idx, failed_test_report) in reports.iter_mut().enumerate() { - if failed_test_report.passed() { - continue; + + // Iterate only over failed tests + for (idx, failed_test_report) in reports + .iter_mut() + .filter(|report| !report.passed()) + .enumerate() + { + if opts.disable_spinner { + println!("Running test: {:?}", failed_test_report.name); } - revm_run_spinner.update_text(format!( - "{} {}/{failed_tests} - {}", - "Re-running failed tests with REVM".bold(), - idx + 1, - format_duration_as_mm_ss(revm_run_time.elapsed()) - )); + spinner_update_text_or_print( + &mut revm_run_spinner, + format!( + "{} {}/{failed_tests} - {}", + "Re-running failed tests with REVM".bold(), + idx + 1, + format_duration_as_mm_ss(revm_run_time.elapsed()) + ), + opts.disable_spinner, + ); + match revm_runner::re_run_failed_ef_test( ef_tests .iter() @@ -184,10 +245,14 @@ fn re_run_with_revm( } } } - revm_run_spinner.success(&format!( - "Re-ran failed tests with REVM in {}", - format_duration_as_mm_ss(revm_run_time.elapsed()) - )); + spinner_success_or_print( + &mut revm_run_spinner, + format!( + "Re-ran failed tests with REVM in {}", + format_duration_as_mm_ss(revm_run_time.elapsed()) + ), + opts.disable_spinner, + ); Ok(()) } diff --git a/cmd/ef_tests/levm/utils.rs b/cmd/ef_tests/levm/utils.rs index d20ce428de..0c8f2e16b6 100644 --- a/cmd/ef_tests/levm/utils.rs +++ b/cmd/ef_tests/levm/utils.rs @@ -5,6 +5,7 @@ use crate::{ use ethrex_core::{types::Genesis, H256, U256}; use ethrex_storage::{EngineType, Store}; use ethrex_vm::{evm_state, EvmState}; +use spinoff::Spinner; pub fn load_initial_state(test: &EFTest) -> (EvmState, H256) { let genesis = Genesis::from(test); @@ -21,6 +22,22 @@ pub fn load_initial_state(test: &EFTest) -> (EvmState, H256) { ) } +pub fn spinner_update_text_or_print(spinner: &mut Spinner, text: String, no_spinner: bool) { + if no_spinner { + println!("{}", text); + } else { + spinner.update_text(text); + } +} + +pub fn spinner_success_or_print(spinner: &mut Spinner, text: String, no_spinner: bool) { + if no_spinner { + println!("{}", text); + } else { + spinner.success(&text); + } +} + // If gas price is not provided, calculate it with current base fee and priority fee pub fn effective_gas_price( test: &EFTest, diff --git a/crates/vm/levm/Makefile b/crates/vm/levm/Makefile index 8c7bed4190..04b089f6ec 100644 --- a/crates/vm/levm/Makefile +++ b/crates/vm/levm/Makefile @@ -35,6 +35,10 @@ run-evm-ef-tests: ## πŸƒβ€β™‚οΈ Run EF Tests cd ../../../ && \ time cargo test -p ef_tests-levm --test ef_tests_levm +run-evm-ef-tests-ci: ## πŸƒβ€β™‚οΈ Run EF Tests only with LEVM and without spinner, for CI. + cd ../../../ && \ + time cargo test -p ef_tests-levm --test ef_tests_levm -- --disable-spinner --summary + generate-evm-ef-tests-report: ## πŸ“Š Generate EF Tests Report cd ../../../ && \ cargo test -p ef_tests-levm --test ef_tests_levm -- --summary From a5e5da45bb0cbd9d794c4da241d34571154c5d6a Mon Sep 17 00:00:00 2001 From: Damian Ramirez Date: Wed, 4 Dec 2024 13:41:48 -0300 Subject: [PATCH 4/4] refactor(levm): refactor opcodes (#1398) **Motivation** In this PR I refactored some opcodes **Description** - `op_exp`: Move the conversion logic from `op_exp()` to `gas_cost::exp()`. Now `exp()` expects `U256` instead of `u64`. - `op_mulmod`: Add early return when `multiplicand` or `multiplier` are zero. - `op_addmod`: Add early return when `modulus` is zero. - `op_div`: Remove redundant checking. - `op_smod`: Add early return when `dividend` is zero. - `op_sdiv`: Remove logic to check if the number is negative and use the `abs()` function - `op_push`: I move the bytecode read and create the byte slice logic to helper functions. Then change the sign of `op_push` function, now it expect the number of bytes to move: `pub fn op_push(&mut self, current_call_frame: &mut CallFrame, n_bytes: usize) -> Result`. TBH, I don't know if this is enough to simplify the func or maybe we can change something else. What do you think about this? Closes #1380, #1381, #1382, #1383, #1384 and #1385 --- crates/vm/levm/src/gas_cost.rs | 14 ++-- .../vm/levm/src/opcode_handlers/arithmetic.rs | 44 ++++-------- crates/vm/levm/src/opcode_handlers/push.rs | 69 ++++++++++--------- crates/vm/levm/src/vm.rs | 7 +- 4 files changed, 61 insertions(+), 73 deletions(-) diff --git a/crates/vm/levm/src/gas_cost.rs b/crates/vm/levm/src/gas_cost.rs index 462fbdee63..944d97c221 100644 --- a/crates/vm/levm/src/gas_cost.rs +++ b/crates/vm/levm/src/gas_cost.rs @@ -158,13 +158,15 @@ pub const CALLDATA_COST_NON_ZERO_BYTE: U256 = U256([16, 0, 0, 0]); // Blob gas costs pub const BLOB_GAS_PER_BLOB: U256 = U256([131072, 0, 0, 0]); -pub fn exp(exponent_bits: u64) -> Result { - let exponent_byte_size = (exponent_bits - .checked_add(7) - .ok_or(OutOfGasError::GasCostOverflow)?) - / 8; +pub fn exp(exponent: U256) -> Result { + let exponent_byte_size = exponent + .checked_add(U256::from(7)) + .ok_or(OutOfGasError::GasCostOverflow)? + .checked_div(U256::from(8)) + .ok_or(OutOfGasError::ArithmeticOperationDividedByZero)?; // '8' will never be zero + let exponent_byte_size_cost = EXP_DYNAMIC_BASE - .checked_mul(exponent_byte_size.into()) + .checked_mul(exponent_byte_size) .ok_or(OutOfGasError::GasCostOverflow)?; EXP_STATIC .checked_add(exponent_byte_size_cost) diff --git a/crates/vm/levm/src/opcode_handlers/arithmetic.rs b/crates/vm/levm/src/opcode_handlers/arithmetic.rs index 0ca9a331fc..a126428682 100644 --- a/crates/vm/levm/src/opcode_handlers/arithmetic.rs +++ b/crates/vm/levm/src/opcode_handlers/arithmetic.rs @@ -53,10 +53,6 @@ impl VM { let dividend = current_call_frame.stack.pop()?; let divisor = current_call_frame.stack.pop()?; - if divisor.is_zero() { - current_call_frame.stack.push(U256::zero())?; - return Ok(OpcodeSuccess::Continue); - } let Some(quotient) = dividend.checked_div(divisor) else { current_call_frame.stack.push(U256::zero())?; return Ok(OpcodeSuccess::Continue); @@ -80,27 +76,11 @@ impl VM { return Ok(OpcodeSuccess::Continue); } - let dividend_is_negative = is_negative(dividend); - let divisor_is_negative = is_negative(divisor); - let dividend = if dividend_is_negative { - negate(dividend) - } else { - dividend - }; - let divisor = if divisor_is_negative { - negate(divisor) - } else { - divisor - }; + let dividend = abs(dividend); + let divisor = abs(divisor); + let quotient = match dividend.checked_div(divisor) { - Some(quot) => { - let quotient_is_negative = dividend_is_negative ^ divisor_is_negative; - if quotient_is_negative { - negate(quot) - } else { - quot - } - } + Some(quot) => quot, None => U256::zero(), }; @@ -133,7 +113,7 @@ impl VM { let unchecked_dividend = current_call_frame.stack.pop()?; let unchecked_divisor = current_call_frame.stack.pop()?; - if unchecked_divisor.is_zero() { + if unchecked_divisor.is_zero() || unchecked_dividend.is_zero() { current_call_frame.stack.push(U256::zero())?; return Ok(OpcodeSuccess::Continue); } @@ -171,6 +151,11 @@ impl VM { let addend = current_call_frame.stack.pop()?; let modulus = current_call_frame.stack.pop()?; + if modulus.is_zero() { + current_call_frame.stack.push(U256::zero())?; + return Ok(OpcodeSuccess::Continue); + } + let new_augend = augend.checked_rem(modulus).unwrap_or_default(); let new_addend = addend.checked_rem(modulus).unwrap_or_default(); @@ -194,7 +179,7 @@ impl VM { let multiplier = current_call_frame.stack.pop()?; let modulus = current_call_frame.stack.pop()?; - if modulus.is_zero() { + if modulus.is_zero() || multiplicand.is_zero() || multiplier.is_zero() { current_call_frame.stack.push(U256::zero())?; return Ok(OpcodeSuccess::Continue); } @@ -215,12 +200,7 @@ impl VM { let base = current_call_frame.stack.pop()?; let exponent = current_call_frame.stack.pop()?; - let exponent_bits: u64 = exponent - .bits() - .try_into() - .map_err(|_| VMError::Internal(InternalError::ConversionError))?; - - let gas_cost = gas_cost::exp(exponent_bits).map_err(VMError::OutOfGas)?; + let gas_cost = gas_cost::exp(exponent)?; self.increase_consumed_gas(current_call_frame, gas_cost)?; diff --git a/crates/vm/levm/src/opcode_handlers/push.rs b/crates/vm/levm/src/opcode_handlers/push.rs index dfd56c44ae..32ab9818d9 100644 --- a/crates/vm/levm/src/opcode_handlers/push.rs +++ b/crates/vm/levm/src/opcode_handlers/push.rs @@ -3,7 +3,6 @@ use crate::{ constants::WORD_SIZE, errors::{InternalError, OpcodeSuccess, VMError}, gas_cost, - opcodes::Opcode, vm::VM, }; use ethrex_core::U256; @@ -16,43 +15,16 @@ impl VM { pub fn op_push( &mut self, current_call_frame: &mut CallFrame, - op: Opcode, + n_bytes: usize, ) -> Result { self.increase_consumed_gas(current_call_frame, gas_cost::PUSHN)?; - let n_bytes = (usize::from(op)) - .checked_sub(usize::from(Opcode::PUSH1)) - .ok_or(VMError::InvalidOpcode)? - .checked_add(1) - .ok_or(VMError::InvalidOpcode)?; + let read_n_bytes = read_bytcode_slice(current_call_frame, n_bytes); + let value_to_push = bytes_to_word(&read_n_bytes, n_bytes)?; - let read_n_bytes: Vec = current_call_frame - .bytecode - .get(current_call_frame.pc()..) - .unwrap_or_default() - .iter() - .take(n_bytes) - .cloned() - .collect(); - - let mut value_to_push = [0u8; WORD_SIZE]; - - let start_index = WORD_SIZE.checked_sub(n_bytes).ok_or(VMError::Internal( - InternalError::ArithmeticOperationUnderflow, - ))?; - - // Rellenamos el array `value_to_push` con los bytes leΓ­dos - for (i, byte) in read_n_bytes.iter().enumerate() { - let index = start_index.checked_add(i).ok_or(VMError::Internal( - InternalError::ArithmeticOperationOverflow, - ))?; - if let Some(data_byte) = value_to_push.get_mut(index) { - *data_byte = *byte; - } - } - - let bytes_push: &[u8] = &value_to_push; - current_call_frame.stack.push(U256::from(bytes_push))?; + current_call_frame + .stack + .push(U256::from(value_to_push.as_slice()))?; current_call_frame.increment_pc_by(n_bytes)?; @@ -71,3 +43,32 @@ impl VM { Ok(OpcodeSuccess::Continue) } } + +fn read_bytcode_slice(current_call_frame: &CallFrame, n_bytes: usize) -> Vec { + current_call_frame + .bytecode + .get(current_call_frame.pc()..) + .unwrap_or_default() + .iter() + .take(n_bytes) + .cloned() + .collect() +} + +fn bytes_to_word(read_n_bytes: &[u8], n_bytes: usize) -> Result<[u8; WORD_SIZE], VMError> { + let mut value_to_push = [0u8; WORD_SIZE]; + let start_index = WORD_SIZE.checked_sub(n_bytes).ok_or(VMError::Internal( + InternalError::ArithmeticOperationUnderflow, + ))?; + + for (i, byte) in read_n_bytes.iter().enumerate() { + let index = start_index.checked_add(i).ok_or(VMError::Internal( + InternalError::ArithmeticOperationOverflow, + ))?; + if let Some(data_byte) = value_to_push.get_mut(index) { + *data_byte = *byte; + } + } + + Ok(value_to_push) +} diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index cbeeada88d..7a2a58a43e 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -225,7 +225,12 @@ impl VM { Opcode::PUSH0 => self.op_push0(current_call_frame), // PUSHn op if (Opcode::PUSH1..=Opcode::PUSH32).contains(&op) => { - self.op_push(current_call_frame, op) + let n_bytes = (usize::from(op)) + .checked_sub(usize::from(Opcode::PUSH1)) + .ok_or(VMError::InvalidOpcode)? + .checked_add(1) + .ok_or(VMError::InvalidOpcode)?; + self.op_push(current_call_frame, n_bytes) } Opcode::AND => self.op_and(current_call_frame), Opcode::OR => self.op_or(current_call_frame),