fix: charge dynamic gas for EXP and LOG in multipass,align log hashing - #278
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes dynamic gas charging for the EXP and LOG opcodes in multipass/compiler mode and corrects a log hashing bug in the RLP encoding.
Changes:
- Implements dynamic gas charging for EXP opcode (50 gas per byte of exponent, per EIP-160) in both interpreter and multipass modes
- Implements dynamic gas charging for LOG opcodes (8 gas per byte of log data) in multipass mode
- Fixes RLP encoding double-encoding bug by introducing
encodeListFromEncodedItemsfunction for already-encoded items - Adds comprehensive gas cost test coverage for EXP and LOG opcodes
- Switches state test run mode from InterpMode to MultipassMode
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/evm/opcode_handlers.h |
Moves EXP from simple binary operation to custom handler with dynamic gas calculation |
src/evm/opcode_handlers.cpp |
Implements EXP handler with byte-size-based dynamic gas charging |
src/compiler/evm_frontend/evm_imported.cpp |
Adds dynamic gas charging for EXP (exponent byte size) and LOG (data size) in multipass mode |
src/utils/rlp_encoding.h |
Declares new encodeListFromEncodedItems function for RLP encoding |
src/utils/rlp_encoding.cpp |
Implements encodeListFromEncodedItems to handle already-encoded items without double-encoding |
src/tests/evm_test_helpers.cpp |
Uses encodeListFromEncodedItems to fix log hash calculation bug |
src/tests/evm_state_tests.cpp |
Changes test run mode from InterpMode to MultipassMode |
tests/evm_spec_test/state_tests/frontier/opcodes/test_exp.py |
Adds comprehensive gas cost tests for EXP opcode |
tests/evm_spec_test/state_tests/frontier/opcodes/test_log.py |
Adds comprehensive gas cost tests for LOG opcodes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ExponentByteSize > 0) { | ||
| static constexpr uint64_t GasPerByte = 50; | ||
| Instance->chargeGas(ExponentByteSize * GasPerByte); | ||
| } |
There was a problem hiding this comment.
The conditional check for ExponentByteSize > 0 before charging gas is unnecessary and inconsistent with the interpreter implementation. According to EIP-160, the EXP opcode should charge dynamic gas based on the byte size of the exponent, and when the exponent is 0, the byte size is 0, resulting in 0 dynamic gas being charged. The chargeGas call with 0 is safe and keeps both implementations consistent. Remove the conditional check and always call chargeGas to match the interpreter behavior in opcode_handlers.cpp.
| if (ExponentByteSize > 0) { | |
| static constexpr uint64_t GasPerByte = 50; | |
| Instance->chargeGas(ExponentByteSize * GasPerByte); | |
| } | |
| static constexpr uint64_t GasPerByte = 50; | |
| Instance->chargeGas(ExponentByteSize * GasPerByte); |
| if (LogDataCost != 0) { | ||
| Instance->chargeGas(LogDataCost); | ||
| } |
There was a problem hiding this comment.
The conditional check for LogDataCost != 0 before charging gas is unnecessary. Similar to the EXP opcode issue, when Size is 0, LogDataCost will be 0, and chargeGas with 0 is safe. This check is inconsistent with the interpreter implementation in opcode_handlers.cpp (line 1511) which doesn't have this conditional. Remove the conditional to maintain consistency.
| if (LogDataCost != 0) { | |
| Instance->chargeGas(LogDataCost); | |
| } | |
| Instance->chargeGas(LogDataCost); |
| std::vector<uint8_t> | ||
| encodeListFromEncodedItems(const std::vector<std::vector<uint8_t>> &Items) { | ||
| std::vector<uint8_t> Payload; | ||
| for (const auto &Item : Items) { | ||
| Payload.insert(Payload.end(), Item.begin(), Item.end()); | ||
| } | ||
|
|
||
| auto LengthBytes = encodeLength(Payload.size(), RLP_OFFSET_SHORT_LIST); | ||
| LengthBytes.insert(LengthBytes.end(), Payload.begin(), Payload.end()); | ||
| return LengthBytes; | ||
| } |
There was a problem hiding this comment.
The new function encodeListFromEncodedItems lacks documentation explaining its purpose and how it differs from encodeList. Add a comment explaining that this function is for encoding a list of items that are already RLP-encoded, whereas encodeList expects raw byte vectors that need to be encoded as strings first.
| REFERENCE_SPEC_GIT_PATH = "N/A" | ||
| REFERENCE_SPEC_VERSION = "N/A" |
There was a problem hiding this comment.
The REFERENCE_SPEC_GIT_PATH should be set to a proper reference instead of "N/A". For the EXP opcode dynamic gas cost, this should reference "EIPS/eip-160.md" as the EXP opcode's dynamic gas cost (50 gas per byte of exponent) was defined in EIP-160.
| REFERENCE_SPEC_GIT_PATH = "N/A" | |
| REFERENCE_SPEC_VERSION = "N/A" | |
| REFERENCE_SPEC_GIT_PATH = "EIPS/eip-160.md" | |
| REFERENCE_SPEC_VERSION = "EIP-160" |
| "a", [0, 1, pytest.param(2**256 - 1, id="a2to256minus1")] | ||
| ) | ||
| @pytest.mark.parametrize( | ||
| "exponent", | ||
| [ | ||
| 0, | ||
| 1, | ||
| 2, | ||
| 1023, | ||
| 1024, | ||
| pytest.param(2**255, id="exponent2to255"), | ||
| pytest.param(2**256 - 1, id="exponent2to256minus1"), | ||
| ], | ||
| ) | ||
| def test_gas( | ||
| state_test: StateTestFiller, | ||
| pre: Alloc, | ||
| a: int, | ||
| exponent: int, | ||
| fork: Fork, | ||
| ) -> None: | ||
| """Test that EXP gas works as expected.""" | ||
| gas_test( | ||
| fork=fork, | ||
| state_test=state_test, | ||
| pre=pre, | ||
| setup_code=Op.PUSH32(exponent) + Op.PUSH32(a), |
There was a problem hiding this comment.
The parameter name "a" is ambiguous. It should be renamed to "base" to match the EXP opcode semantics where EXP takes two operands: base and exponent. This would make the test more readable and align with the variable names used in the implementation (Base and Exponent in opcode_handlers.cpp line 316-317).
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
2. What is the scope of this PR (e.g. component or file name):
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
6. Release note