feat(ethexe/ethereum): add initial gas benchmarks#5145
feat(ethexe/ethereum): add initial gas benchmarks#5145StackOverflowExcept1on wants to merge 20 commits intomasterfrom
Conversation
StackOverflowExcept1on
added
the
A1-inprogress
Summary of ChangesHello @StackOverflowExcept1on, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive gas benchmarking infrastructure within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces extensive changes to add gas benchmarking capabilities to the smart contracts. This is achieved by adding numerous constants and commented-out DebugEvent emissions at various points in the code to measure gas usage between them. The changes are consistent with the goal of benchmarking.
My review has identified a couple of minor issues in the benchmarking code itself, specifically swapped constant values for BEFORE/AFTER event pairs. I've also noted a TODO comment that suggests some uncertainty in the logic.
For better maintainability and to avoid errors, I would suggest centralizing all the benchmark-related constants into a single, dedicated library file (e.g., BenchmarkEvents.sol). This would make them easier to manage and verify.
…ed version and not
| fn ensure_types_unchanged() { | ||
| const EXPECTED_TYPE_INFO_HASH: &str = | ||
| "8a30677f0819eeffd02548e0e91adf940fa39173e509ad24b50cbb92ce61f907"; | ||
| "1daa6179efabf521b9087edfd0244a64250abc27a9b93255a441e736fb2a6760"; |
There was a problem hiding this comment.
Just changing comment in the code changes TypeInfo???
StackOverflowExcept1on
added
A0-pleasereview
StackOverflowExcept1on
changed the title
StackOverflowExcept1on
changed the title
|
/gemini review |
| @@ -0,0 +1,138 @@ | |||
| // This file is part of Gear. | |||
There was a problem hiding this comment.
will be removed, just to demonstrate API
cargo run --package ethexe-ethereum --bin ethexe-ethereum
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive gas benchmarking framework for ethexe/ethereum using revm, adding instrumented versions of the Router and Mirror smart contracts for fine-grained gas measurement. It sets up a mock EVM environment with SimulationContext and uses revm::Inspector for gas measurement. A high-severity Denial of Service (DoS) vulnerability was identified in the Mirror contract due to unrestricted gas consumption in external Ether transfers, which could block protocol-wide batch commitments. Additionally, a medium-severity issue exists where the event blacklist for log spoofing protection is incomplete, potentially allowing the spoofing of failure events. There are also suggestions to improve code maintainability by removing dead code and reducing duplication.
| function _transferEther(address destination, uint128 value) private returns (bool) { | ||
| if (value != 0) { | ||
| (bool success,) = destination.call{value: value}(""); | ||
| return success; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The _transferEther function performs an external Ether transfer using a low-level call without specifying a gas limit. If the destination is a malicious contract that consumes all gas (e.g., via an infinite loop), it will cause the sub-call to consume 63/64 of the remaining gas. When this function is called within loops (such as in _claimValues on line 468) or sequences, the cumulative gas loss can cause the entire commitBatch transaction to revert with an 'Out of Gas' error. Since batches must be committed in order, this allows an attacker to block the entire protocol's progress, leading to a Denial of Service (DoS).
Recommendation: Implement a gas limit for external Ether transfers (e.g., destination.call{value: value, gas: 3000}("")) or adopt a 'pull' pattern where users must withdraw their funds instead of the contract 'pushing' them.
| if (!(topic1 != StateChanged.selector && topic1 != MessageQueueingRequested.selector | ||
| && topic1 != ReplyQueueingRequested.selector && topic1 != ValueClaimingRequested.selector | ||
| && topic1 != OwnedBalanceTopUpRequested.selector | ||
| && topic1 != ExecutableBalanceTopUpRequested.selector && topic1 != Message.selector | ||
| && topic1 != Reply.selector && topic1 != ValueClaimed.selector | ||
| && topic1 != TransferLockedValueToInheritorFailed.selector && topic1 != ReplyTransferFailed.selector | ||
| && topic1 != ValueClaimFailed.selector)) { |
There was a problem hiding this comment.
The event blacklist in _tryParseAndEmitSailsEvent is incomplete. It fails to include the MessageCallFailed and ReplyCallFailed event selectors. An attacker could potentially exploit this to spoof these events, tricking off-chain components into incorrectly processing message or reply failures.
Recommendation: Add MessageCallFailed.selector and ReplyCallFailed.selector to the blacklist check.
2 participants
Resolves #5176
The basic idea is to introduce
SimulationContext, which contains the context for the mock EVM blockchain. It createsrevminstance within this context and stores theblock_number,block_timestamp, mock smart contractdeployer_address(Router,Mirror,WrappedVara, etc.), mock smart contractdeployer_nonce, and validator private keys (we're using 4 validators in testnet).This
SimulationContextwill then be initialized, and all smart contracts (Router,Mirror,WrappedVara, etc.) and their instrumented versions will be deployed to the mock blockchain. It's also worth noting that about 10 blocks will be created inSimulationContextduring initialization. Two codes will also be loaded: one will be validated, and the second will be marked as pending validation. Two programs (Mirror) will also be created. One program will have an uninitializedstateHashand others, and the second will have an initializedstateHash. Basically, all these variations are needed to measure different gas cases (cold slot, hot slot, etc.). The instrumented version for the Router and Mirror smart contacts differs from the regular one in that it uses theevent DebugEvent(uint256 indexed topic0) anonymous. Since the event isanonymous, this allows calling opcodelog1(offset=0, size=0, topic0=some_debug_const)directly from Solidity, avoiding inline assembly. Next, we use therevm::InspectorAPI to measure difference between twolog1opcodes and thus calculate the execution cost for a fixed section of code that is bounded by topics.