Closed MarcosNicolau closed 2 weeks ago
I have some questions regarding these fixes. Are we fixing some tests with these changes? If not, then how are we sure they are really necessary? I think that it would be good to add why we are making these changes to the PR description. They may be fixing some tests or just because you have found they are mentioned in the official specs.
The reasons seem quite clear. When comparing the gas_left
field during the execution of the interpreter tests, it was clear that the ret
operation was adding a lot more more gas to the new frame due to the stipend. Even though this issue might not cause any test failures right now, given that the issue is pretty evident, I would not wait until for a potential future test failure, which might never occur.
A good way of testing this would be using the benchmark-analyzer
of the compiler tester with the interpreter tests
The idea would be the following:
Run:
cargo run --verbose --features lambda_vm --release --bin compiler-tester -- --path tests/solidity/complex/interpreter/test.json --mode 'Y+M3B3 0.8.26' --benchmark lambda_vm.json
Then run:
cargo run --verbose --release --bin compiler-tester -- --path tests/solidity/complex/interpreter/test.json --mode 'Y+M3B3 0.8.26' --benchmark vm1.json
This will generate 2 json files with data for each interpreter test, we care about ergs, running:
cargo run --release --bin benchmark-analyzer -- --reference vm1.json --candidate lambda_vm.json
Will output a table like this
╔═╡ Size (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╗ ║ Mean 0.000 ║ ║ Best 0.000 ║ ║ Worst 0.000 ║ ║ Total 0.000 ║ ╠═╡ Cycles (-%) ╞═══╡ EVMInterpreter M3B3 ╞═╣ ║ Mean 4.834 ║ ║ Best 7.022 ║ ║ Worst -1.611 ║ ║ Total 5.573 ║ ╠═╡ Ergs (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╣ ║ Mean -3.166 ║ ║ Best 0.000 ║ ║ Worst -6.275 ║ ║ Total -3.573 ║ ╠═╡ Ergs/gas ╞══════╡ EVMInterpreter M3B3 ╞═╣ ║ ADD 67.667 ║ ║ MUL 39.400 ║ ║ SUB 67.667 ║ . . . ║ SWAP16 66.000 ║ ║ RETURN inf ║ ║ REVERT inf ║ ╠═╡ Ergs/gas (-%) ╞═╡ EVMInterpreter M3B3 ╞═╣ ║ ADD 29.021 ║ ║ MUL 31.119 ║ ║ SUB 29.021 ║ . . . ║ SWAP16 38.700 ║ ╚════════════════════════════════╝
We care about ergs/gas
which indicates the relation between the ergs spent by the eravm and the gas spent by the interpreter, since the gas should be the same in both, this will tell us if the ergs spent is correct, if all opcodes have ergs/gas
in grey or there are no opcodes in ergs / gas %
then we are calculating ergs correctly
Have in mind that we may have to change some things in compiler tester to let it know the ergs we spent
Various fixes related to how we were managing the gas consumption:
inexplicit_panic
andpanic_from_far_call
count as instructions and we should be decreasing the gas base cost for running them as well.far_call
frame if the bytecode was for the evm interpreter, we were returning back not only the left gas but also the stipend one.